FIX: When disagreeing with a flag that silenced a user, unsilence them

FIX: When disagreeing with a flag that silenced a user, unsilence them

Previously it would unhide their post but leave them silenced.

This fix also cleans up some of the helper classes to make it easier to pass extra data to the silencing code (for example, a link to the post that caused the user to be silenced.)

This patch also refactors the auto_silence specs to avoid using stubs.

diff --git a/app/controllers/admin/flags_controller.rb b/app/controllers/admin/flags_controller.rb
index a94eae4..5df6bcd 100644
--- a/app/controllers/admin/flags_controller.rb
+++ b/app/controllers/admin/flags_controller.rb
@@ -114,8 +114,6 @@ class Admin::FlagsController < Admin::AdminController
 
     PostAction.clear_flags!(post, current_user)
 
-    post.unhide!
-
     render body: nil
   end
 
diff --git a/app/models/post_action.rb b/app/models/post_action.rb
index f0d7f07..c814f55 100644
--- a/app/models/post_action.rb
+++ b/app/models/post_action.rb
@@ -231,6 +231,8 @@ class PostAction < ActiveRecord::Base
     end
 
     update_flagged_posts_count
+
+    undo_hide_and_silence(post)
   end
 
   def self.defer_flags!(post, moderator, delete_post = false)
@@ -375,6 +377,13 @@ class PostAction < ActiveRecord::Base
     PostAction.where(where_attrs).first
   end
 
+  def self.undo_hide_and_silence(post)
+    return unless post.hidden?
+
+    post.unhide!
+    UserSilencer.unsilence(post.user) if UserSilencer.was_silenced_for?(post)
+  end
+
   def self.copy(original_post, target_post)
     cols_to_copy = (column_names - %w{id post_id}).join(', ')
 
@@ -537,7 +546,7 @@ class PostAction < ActiveRecord::Base
     post = Post.with_deleted.where(id: post_id).first
     PostAction.auto_close_if_threshold_reached(post.topic)
     PostAction.auto_hide_if_needed(user, post, post_action_type_key)
-    SpamRulesEnforcer.enforce!(post.user)
+    SpamRule::AutoSilence.new(post.user, post).perform
   end
 
   def create_user_action
diff --git a/app/services/spam_rule/auto_silence.rb b/app/services/spam_rule/auto_silence.rb
index ac5173e..7eeb9ba 100644
--- a/app/services/spam_rule/auto_silence.rb
+++ b/app/services/spam_rule/auto_silence.rb
@@ -1,37 +1,38 @@
 class SpamRule::AutoSilence
 
-  def initialize(user)
-    @user = user
-  end
+  attr_reader :group_message
 
-  def self.silence?(user)
-    self.new(user).silence?
+  def initialize(user, post = nil)
+    @user = user
+    @post = post
   end
 
-  def self.punish!(user)
-    self.new(user).silence_user
+  def perform
+    I18n.with_locale(SiteSetting.default_locale) do
+      silence_user if should_autosilence?
+    end
   end
 
-  def perform
-    silence_user if silence?
+  def self.prevent_posting?(user)
+    user.blank? || user.silenced? || new(user).should_autosilence?
   end
 
-  def silence?
-    return true if @user.silenced?
+  def should_autosilence?
+    return false if @user.blank?
     return false if @user.staged?
     return false if @user.has_trust_level?(TrustLevel[1])
 
-    if SiteSetting.num_spam_flags_to_silence_new_user > (0) &&
-        SiteSetting.num_users_to_silence_new_user > (0) &&
-        num_spam_flags_against_user >= (SiteSetting.num_spam_flags_to_silence_new_user) &&
-        num_users_who_flagged_spam_against_user >= (SiteSetting.num_users_to_silence_new_user)
+    if SiteSetting.num_spam_flags_to_silence_new_user > 0 &&
+        SiteSetting.num_users_to_silence_new_user > 0 &&
+        num_spam_flags_against_user >= SiteSetting.num_spam_flags_to_silence_new_user &&
+        num_users_who_flagged_spam_against_user >= SiteSetting.num_users_to_silence_new_user
       return true
     end
 
-    if SiteSetting.num_tl3_flags_to_silence_new_user > (0) &&
-        SiteSetting.num_tl3_users_to_silence_new_user > (0) &&
-        num_tl3_flags_against_user >= (SiteSetting.num_tl3_flags_to_silence_new_user) &&
-        num_tl3_users_who_flagged >= (SiteSetting.num_tl3_users_to_silence_new_user)
+    if SiteSetting.num_tl3_flags_to_silence_new_user > 0 &&
+        SiteSetting.num_tl3_users_to_silence_new_user > 0 &&
+        num_tl3_flags_against_user >= SiteSetting.num_tl3_flags_to_silence_new_user &&
+        num_tl3_users_who_flagged >= SiteSetting.num_tl3_users_to_silence_new_user
       return true
     end
 
@@ -72,8 +73,16 @@ class SpamRule::AutoSilence
 
   def silence_user
     Post.transaction do
-      if UserSilencer.silence(@user, Discourse.system_user, message: :too_many_spam_flags) && SiteSetting.notify_mods_when_user_silenced
-        GroupMessage.create(Group[:moderators].name, :user_automatically_silenced, user: @user, limit_once_per: false)
+
+      silencer = UserSilencer.new(
+        @user,
+        Discourse.system_user,
+        message: :too_many_spam_flags,
+        post_id: @post&.id
+      )
+
+      if silencer.silence && SiteSetting.notify_mods_when_user_silenced
+        @group_message = GroupMessage.create(Group[:moderators].name, :user_automatically_silenced, user: @user, limit_once_per: false)
       end
     end
   end
diff --git a/app/services/spam_rule/flag_sockpuppets.rb b/app/services/spam_rule/flag_sockpuppets.rb
index e3df8a8..16100a7 100644
--- a/app/services/spam_rule/flag_sockpuppets.rb
+++ b/app/services/spam_rule/flag_sockpuppets.rb
@@ -5,11 +5,13 @@ class SpamRule::FlagSockpuppets
   end
 
   def perform
-    if SiteSetting.flag_sockpuppets && reply_is_from_sockpuppet?
-      flag_sockpuppet_users
-      true
-    else
-      false
+    I18n.with_locale(SiteSetting.default_locale) do
+      if SiteSetting.flag_sockpuppets && reply_is_from_sockpuppet?
+        flag_sockpuppet_users
+        true
+      else
+        false
+      end
     end
   end
 
diff --git a/app/services/spam_rules_enforcer.rb b/app/services/spam_rules_enforcer.rb
deleted file mode 100644
index 7a5133c..0000000
--- a/app/services/spam_rules_enforcer.rb
+++ /dev/null
@@ -1,22 +0,0 @@
-# The SpamRulesEnforcer class takes action against users based on flags that their posts
-# receive, their trust level, etc.
-class SpamRulesEnforcer
-
-  def self.enforce!(arg)
-    SpamRulesEnforcer.new(arg).enforce!
-  end
-
-  def initialize(arg)
-    @user = arg if arg.is_a?(User)
-    @post = arg if arg.is_a?(Post)
-  end
-
-  def enforce!
-    I18n.with_locale(SiteSetting.default_locale) do
-      SpamRule::AutoSilence.new(@user).perform if @user
-      SpamRule::FlagSockpuppets.new(@post).perform if @post
-    end
-    true
-  end
-
-end
diff --git a/app/services/user_silencer.rb b/app/services/user_silencer.rb
index 3044431..be1bbeb 100644
--- a/app/services/user_silencer.rb
+++ b/app/services/user_silencer.rb
@@ -16,6 +16,12 @@ class UserSilencer
     UserSilencer.new(user, by_user, opts).unsilence
   end
 
+  def self.was_silenced_for?(post)
+    return false unless post.present?
+
+    UserHistory.where(action: UserHistory.actions[:silence_user], post: post).exists?
+  end
+
   def silence
     hide_posts unless @opts[:keep_posts]
     unless @user.silenced_till.present?
diff --git a/lib/guardian/post_guardian.rb b/lib/guardian/post_guardian.rb
index fd48124..c49d3cd 100644
--- a/lib/guardian/post_guardian.rb
+++ b/lib/guardian/post_guardian.rb
@@ -108,10 +108,9 @@ module PostGuardian
 
   # Creating Method
   def can_create_post?(parent)
-
     return false if !SiteSetting.enable_system_message_replies? && parent.try(:subtype) == "system_message"
 
-    (!SpamRule::AutoSilence.silence?(@user) || (!!parent.try(:private_message?) && parent.allowed_users.include?(@user))) && (
+    (!SpamRule::AutoSilence.prevent_posting?(@user) || (!!parent.try(:private_message?) && parent.allowed_users.include?(@user))) && (
       !parent ||
       !parent.category ||
       Category.post_create_allowed(self).where(id: parent.category.id).count == 1
diff --git a/lib/post_creator.rb b/lib/post_creator.rb
index 4c4c079..d8bee40 100644
--- a/lib/post_creator.rb
+++ b/lib/post_creator.rb
@@ -359,7 +359,7 @@ class PostCreator
                            limit_once_per: 24.hours,
                            message_params: { domains: @post.linked_hosts.keys.join(', ') })
     elsif @post && errors.blank? && !skip_validations?
-      SpamRulesEnforcer.enforce!(@post)
+      SpamRule::FlagSockpuppets.new(@post).perform
     end
   end
 
diff --git a/spec/integration/same_ip_spammers_spec.rb b/spec/integration/same_ip_spammers_spec.rb
index 63f339f..827dba1 100644
--- a/spec/integration/same_ip_spammers_spec.rb
+++ b/spec/integration/same_ip_spammers_spec.rb

[... diff too long, it was truncated ...]

GitHub sha: bc3efab8

2 Likes