From 6b1ff0edd394b74ab6da8beef6bb64ef9c3be80a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9gis=20Hanol?= <regis@hanol.fr>
Date: Wed, 9 May 2018 16:40:52 +0200
Subject: [PATCH] FIX: always update bounce score (instead of doing it once per
 day)

---
 lib/email/receiver.rb                  | 39 ++++++++++----------------
 spec/components/email/receiver_spec.rb | 28 +++++++++++-------
 2 files changed, 33 insertions(+), 34 deletions(-)

diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb
index 4cbcfb175c0..01d2be82836 100644
--- a/lib/email/receiver.rb
+++ b/lib/email/receiver.rb
@@ -200,33 +200,24 @@ module Email
     end
 
     def self.update_bounce_score(email, score)
-      # only update bounce score once per day
-      key = "bounce_score:#{email}:#{Date.today}"
+      if user = User.find_by_email(email)
+        old_bounce_score = user.user_stat.bounce_score
+        new_bounce_score = old_bounce_score + score
+        range = (old_bounce_score + 1..new_bounce_score)
 
-      if $redis.setnx(key, "1")
-        $redis.expire(key, 25.hours)
+        user.user_stat.bounce_score = new_bounce_score
+        user.user_stat.reset_bounce_score_after = SiteSetting.reset_bounce_score_after_days.days.from_now
+        user.user_stat.save!
 
-        if user = User.find_by_email(email)
-          user.user_stat.bounce_score += score
-          user.user_stat.reset_bounce_score_after = SiteSetting.reset_bounce_score_after_days.days.from_now
-          user.user_stat.save!
-
-          bounce_score = user.user_stat.bounce_score
-          if user.active && bounce_score >= SiteSetting.bounce_score_threshold_deactivate
-            user.update!(active: false)
-            reason = I18n.t("user.deactivated", email: user.email)
-            StaffActionLogger.new(Discourse.system_user).log_user_deactivate(user, reason)
-          elsif bounce_score >= SiteSetting.bounce_score_threshold
-            # NOTE: we check bounce_score before sending emails, nothing to do
-            # here other than log it happened.
-            reason = I18n.t("user.email.revoked", email: user.email, date: user.user_stat.reset_bounce_score_after)
-            StaffActionLogger.new(Discourse.system_user).log_revoke_email(user, reason)
-          end
+        if user.active && range === SiteSetting.bounce_score_threshold_deactivate
+          user.update!(active: false)
+          reason = I18n.t("user.deactivated", email: user.email)
+          StaffActionLogger.new(Discourse.system_user).log_user_deactivate(user, reason)
+        elsif range === SiteSetting.bounce_score_threshold
+          # NOTE: we check bounce_score before sending emails, nothing to do here other than log it happened.
+          reason = I18n.t("user.email.revoked", email: user.email, date: user.user_stat.reset_bounce_score_after)
+          StaffActionLogger.new(Discourse.system_user).log_revoke_email(user, reason)
         end
-
-        true
-      else
-        false
       end
     end
 
diff --git a/spec/components/email/receiver_spec.rb b/spec/components/email/receiver_spec.rb
index 153d9a39569..5d9b217458e 100644
--- a/spec/components/email/receiver_spec.rb
+++ b/spec/components/email/receiver_spec.rb
@@ -100,17 +100,12 @@ describe Email::Receiver do
     let!(:email_log) { Fabricate(:email_log, user: user, bounce_key: bounce_key) }
     let!(:email_log_2) { Fabricate(:email_log, user: user, bounce_key: bounce_key_2) }
 
-    before do
-      $redis.del("bounce_score:#{user.email}:#{Date.today}")
-      $redis.del("bounce_score:#{user.email}:#{2.days.from_now.to_date}")
-    end
-
     it "deals with soft bounces" do
       expect { process(:soft_bounce_via_verp) }.to raise_error(Email::Receiver::BouncedEmailError)
 
       email_log.reload
       expect(email_log.bounced).to eq(true)
-      expect(email_log.user.user_stat.bounce_score).to eq(1)
+      expect(email_log.user.user_stat.bounce_score).to eq(SiteSetting.soft_bounce_score)
     end
 
     it "deals with hard bounces" do
@@ -118,17 +113,30 @@ describe Email::Receiver do
 
       email_log.reload
       expect(email_log.bounced).to eq(true)
-      expect(email_log.user.user_stat.bounce_score).to eq(2)
-
-      freeze_time 2.days.from_now
+      expect(email_log.user.user_stat.bounce_score).to eq(SiteSetting.hard_bounce_score)
 
       expect { process(:hard_bounce_via_verp_2) }.to raise_error(Email::Receiver::BouncedEmailError)
 
       email_log_2.reload
-      expect(email_log_2.user.user_stat.bounce_score).to eq(4)
+      expect(email_log_2.user.user_stat.bounce_score).to eq(SiteSetting.hard_bounce_score * 2)
       expect(email_log_2.bounced).to eq(true)
     end
 
+    it "automatically deactive users once they reach the 'bounce_score_threshold_deactivate' threshold" do
+      expect(user.active).to eq(true)
+
+      user.user_stat.bounce_score = SiteSetting.bounce_score_threshold_deactivate - 1
+      user.user_stat.save!
+
+      expect { process(:soft_bounce_via_verp) }.to raise_error(Email::Receiver::BouncedEmailError)
+
+      user.reload
+      email_log.reload
+
+      expect(email_log.bounced).to eq(true)
+      expect(user.active).to eq(false)
+    end
+
   end
 
   context "reply" do