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.

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
 

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

app/services/spam_rule/auto_silence.rb

@@ -1,37 +1,38 @@
 class SpamRule::AutoSilence
 
-  def initialize(user)
+  attr_reader :group_message
+
+  def initialize(user, post = nil)
     @user = user
-  end
-
-  def self.silence?(user)
-    self.new(user).silence?
-  end
-
-  def self.punish!(user)
-    self.new(user).silence_user
+    @post = post
   end
 
   def perform
-    silence_user if silence?
+    I18n.with_locale(SiteSetting.default_locale) do
+      silence_user if should_autosilence?
+    end
   end
 
-  def silence?
-    return true if @user.silenced?
+  def self.prevent_posting?(user)
+    user.blank? || user.silenced? || new(user).should_autosilence?
+  end
+
+  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

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
 

app/services/spam_rules_enforcer.rb

@@ -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

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?

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

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
 

spec/integration/same_ip_spammers_spec.rb

@@ -2,7 +2,7 @@
 
 require 'rails_helper'
 
-describe SpamRulesEnforcer do
+describe "spammers on same IP" do
 
   let(:ip_address)  { '182.189.119.174' }
   let!(:spammer1)   { Fabricate(:user, ip_address: ip_address) }

spec/integration/spam_rules_spec.rb

@@ -2,9 +2,9 @@
 
 require 'rails_helper'
 
-describe SpamRulesEnforcer do
+describe "spam rules for users" do
 
-  describe 'auto-silenceing users based on flagging' do
+  describe 'auto-silence users based on flagging' do
     let!(:admin)     { Fabricate(:admin) } # needed to send a system message
     let!(:moderator) { Fabricate(:moderator) }
     let(:user1)      { Fabricate(:user) }

spec/requests/admin/flags_controller_spec.rb

@@ -102,4 +102,26 @@ RSpec.describe Admin::FlagsController do
       expect(first_post.deleted_at).to eq(nil)
     end
   end
+
+  context '#disagree' do
+    it "unhides the post and unsilences the user if disagreed" do
+      SiteSetting.num_spam_flags_to_silence_new_user = 1
+      SiteSetting.num_users_to_silence_new_user = 1
+
+      new_user = Fabricate(:newuser)
+      new_post = create_post(user: new_user)
+
+      PostAction.act(Fabricate(:leader), new_post, PostActionType.types[:spam])
+
+      post "/admin/flags/disagree/#{new_post.id}.json"
+      expect(response.status).to eq(200)
+
+      new_post.reload
+      new_user.reload
+
+      expect(new_post).to_not be_hidden
+      expect(new_post.spam_count).to eq(0)
+      expect(new_user).to_not be_silenced
+    end
+  end
 end

spec/services/auto_silence_spec.rb

@@ -9,24 +9,25 @@ describe SpamRule::AutoSilence do
   end
 
   describe 'perform' do
-    let(:post)  { Fabricate.build(:post, user: Fabricate.build(:user, trust_level: TrustLevel[0])) }
-    subject     { described_class.new(post.user) }
+    let(:user) { Fabricate.build(:newuser) }
+    let(:post) { Fabricate(:post, user: user) }
+    subject { described_class.new(post.user) }
 
     it 'takes no action if user should not be silenced' do
-      subject.stubs(:silence?).returns(false)
-      subject.expects(:silence_user).never
       subject.perform
+      expect(post.user.reload).not_to be_silenced
     end
 
     it 'delivers punishment when user should be silenced' do
-      subject.stubs(:silence?).returns(true)
-      subject.expects(:silence_user)
+      SiteSetting.num_spam_flags_to_silence_new_user = 1
+      SiteSetting.num_users_to_silence_new_user = 1
+      PostAction.act(Discourse.system_user, post, PostActionType.types[:spam])
       subject.perform
+      expect(post.user.reload).to be_silenced
     end
   end
 
   describe 'num_spam_flags_against_user' do
-    before { described_class.any_instance.stubs(:silence_user) }
     let(:post)     { Fabricate(:post) }
     let(:enforcer) { described_class.new(post.user) }
     subject        { enforcer.num_spam_flags_against_user }
@@ -53,7 +54,6 @@ describe SpamRule::AutoSilence do
   end
 
   describe 'num_users_who_flagged_spam_against_user' do
-    before { described_class.any_instance.stubs(:silence_user) }
     let(:post)     { Fabricate(:post) }
     let(:enforcer) { described_class.new(post.user) }
     subject        { enforcer.num_users_who_flagged_spam_against_user }
@@ -130,146 +130,157 @@ describe SpamRule::AutoSilence do
     let!(:post)   { Fabricate(:post, user: user) }
     subject       { described_class.new(user) }
 
-    before do
-      described_class.stubs(:silence?).with { |u| u.id != user.id }.returns(false)
-      described_class.stubs(:silence?).with { |u| u.id == user.id }.returns(true)
-      subject.stubs(:silence?).returns(true)
-    end
-
     context 'user is not silenced' do
-      before do
-        UserSilencer.expects(:silence).with(user, Discourse.system_user, message: :too_many_spam_flags).returns(true)
-      end
-
       it 'prevents the user from making new posts' do
         subject.silence_user
+        expect(user).to be_silenced
         expect(Guardian.new(user).can_create_post?(nil)).to be_falsey
       end
 
-      it 'sends private message to moderators' do
-        SiteSetting.notify_mods_when_user_silenced = true
-        moderator = Fabricate(:moderator)
-        GroupMessage.expects(:create).with do |group, msg_type, params|
-          group == (Group[:moderators].name) && msg_type == (:user_automatically_silenced) && params[:user].id == (user.id)
-        end
-        subject.silence_user
-      end
+      context "with a moderator" do
+        let!(:moderator) { Fabricate(:moderator) }
 
-      it "doesn't send a pm to moderators if notify_mods_when_user_silenced is false" do
-        SiteSetting.notify_mods_when_user_silenced = false
-        GroupMessage.expects(:create).never
-        subject.silence_user
+        it 'sends private message to moderators' do
+          SiteSetting.notify_mods_when_user_silenced = true
+          subject.silence_user
+          expect(subject.group_message).to be_present
+        end
+
+        it "doesn't send a pm to moderators if notify_mods_when_user_silenced is false" do
+          SiteSetting.notify_mods_when_user_silenced = false
+          subject.silence_user
+          expect(subject.group_message).to be_blank
+        end
       end
     end
 
     context 'user is already silenced' do
       before do
-        UserSilencer.expects(:silence).with(user, Discourse.system_user, message: :too_many_spam_flags).returns(false)
+        UserSilencer.silence(user)
       end
 
       it "doesn't send a pm to moderators if the user is already silenced" do
-        GroupMessage.expects(:create).never
         subject.silence_user
+        expect(subject.group_message).to be_blank
       end
     end
   end
 
-  describe 'silence?' do
+  describe 'autosilenced?' do
+    let(:user) { Fabricate(:newuser) }
+    let(:flagger) { Fabricate(:user) }
+    let(:flagger2) { Fabricate(:user) }
+    let(:post) { Fabricate(:post, user: user) }
+    let(:post2) { Fabricate(:post, user: user) }
 
-    context 'never been silenced' do
-      shared_examples "can't be silenced" do
-        it "returns false" do
-          enforcer = described_class.new(user)
-          enforcer.expects(:num_spam_flags_against_user).never
-          enforcer.expects(:num_users_who_flagged_spam_against_user).never
-          enforcer.expects(:num_flags_against_user).never
-          enforcer.expects(:num_users_who_flagged).never
-          expect(enforcer.silence?).to eq(false)
-        end
-      end
+    context "higher trust levels or staff" do
+      it "should not autosilence any of them" do
+        PostAction.act(flagger, post, PostActionType.types[:spam])
+        PostAction.act(flagger2, post, PostActionType.types[:spam])
 
-      (1..4).each do |trust_level|
-        context "user has trust level #{trust_level}" do
-          let(:user) { Fabricate(:user, trust_level: trust_level) }
-          include_examples "can't be silenced"
-        end
-      end
+        enforcer = described_class.new(user)
+        expect(enforcer.should_autosilence?).to eq(true)
 
-      context "user is an admin" do
-        let(:user) { Fabricate(:admin) }
-        include_examples "can't be silenced"
-      end
+        user.trust_level = 1
+        expect(enforcer.should_autosilence?).to eq(false)
 
-      context "user is a moderator" do
-        let(:user) { Fabricate(:moderator) }
-        include_examples "can't be silenced"
+        user.trust_level = 2
+        expect(enforcer.should_autosilence?).to eq(false)
+
+        user.trust_level = 3
+        expect(enforcer.should_autosilence?).to eq(false)
+
+        user.trust_level = 4
+        expect(enforcer.should_autosilence?).to eq(false)
+
+        user.trust_level = 0
+        user.moderator = true
+        expect(enforcer.should_autosilence?).to eq(false)
+
+        user.moderator = false
+        user.admin = true
+        expect(enforcer.should_autosilence?).to eq(false)
       end
     end
 
     context 'new user' do
-      let(:user)  { Fabricate(:user, trust_level: TrustLevel[0]) }
-      subject     { described_class.new(user) }
+      subject { described_class.new(user) }
 
       it 'returns false if there are no spam flags' do
-        subject.stubs(:num_spam_flags_against_user).returns(0)
-        subject.stubs(:num_users_who_flagged_spam_against_user).returns(0)
-        expect(subject.silence?).to be_falsey
+        expect(subject.num_spam_flags_against_user).to eq(0)
+        expect(subject.num_users_who_flagged_spam_against_user).to eq(0)
+        expect(subject.should_autosilence?).to eq(false)
       end
 
       it 'returns false if there are not received enough flags' do
-        subject.stubs(:num_spam_flags_against_user).returns(1)
-        subject.stubs(:num_users_who_flagged_spam_against_user).returns(2)
-        expect(subject.silence?).to be_falsey
+        PostAction.act(flagger, post, PostActionType.types[:spam])
+        expect(subject.num_spam_flags_against_user).to eq(1)
+        expect(subject.num_users_who_flagged_spam_against_user).to eq(1)
+        expect(subject.should_autosilence?).to eq(false)
       end
 
       it 'returns false if there have not been enough users' do
-        subject.stubs(:num_spam_flags_against_user).returns(2)
-        subject.stubs(:num_users_who_flagged_spam_against_user).returns(1)
-        expect(subject.silence?).to be_falsey
+        PostAction.act(flagger, post, PostActionType.types[:spam])
+        PostAction.act(flagger, post2, PostActionType.types[:spam])
+        expect(subject.num_spam_flags_against_user).to eq(2)
+        expect(subject.num_users_who_flagged_spam_against_user).to eq(1)
+        expect(subject.should_autosilence?).to eq(false)
       end
 
       it 'returns false if num_spam_flags_to_silence_new_user is 0' do
         SiteSetting.num_spam_flags_to_silence_new_user = 0
-        subject.stubs(:num_spam_flags_against_user).returns(100)
-        subject.stubs(:num_users_who_flagged_spam_against_user).returns(100)
-        expect(subject.silence?).to be_falsey
+        PostAction.act(flagger, post, PostActionType.types[:spam])
+        PostAction.act(flagger2, post, PostActionType.types[:spam])
+        expect(subject.should_autosilence?).to eq(false)
       end
 
       it 'returns false if num_users_to_silence_new_user is 0' do
         SiteSetting.num_users_to_silence_new_user = 0
-        subject.stubs(:num_spam_flags_against_user).returns(100)
-        subject.stubs(:num_users_who_flagged_spam_against_user).returns(100)
-        expect(subject.silence?).to be_falsey
+        PostAction.act(flagger, post, PostActionType.types[:spam])
+        PostAction.act(flagger2, post, PostActionType.types[:spam])
+        expect(subject.should_autosilence?).to eq(false)
       end
 
       it 'returns true when there are enough flags from enough users' do
-        subject.stubs(:num_spam_flags_against_user).returns(2)
-        subject.stubs(:num_users_who_flagged_spam_against_user).returns(2)
-        expect(subject.silence?).to be_truthy
+        PostAction.act(flagger, post, PostActionType.types[:spam])
+        PostAction.act(flagger2, post, PostActionType.types[:spam])
+        expect(subject.num_spam_flags_against_user).to eq(2)
+        expect(subject.num_users_who_flagged_spam_against_user).to eq(2)
+        expect(subject.should_autosilence?).to eq(true)
       end
 
       context "all types of flags" do
+        let(:leader1) { Fabricate(:leader) }
+        let(:leader2) { Fabricate(:leader) }
+
         before do
           SiteSetting.num_tl3_flags_to_silence_new_user = 3
           SiteSetting.num_tl3_users_to_silence_new_user = 2
         end
 
         it 'returns false if there are not enough flags' do
-          subject.stubs(:num_tl3_flags_against_user).returns(1)
-          subject.stubs(:num_tl3_users_who_flagged).returns(1)
-          expect(subject.silence?).to be_falsey
+          PostAction.act(leader1, post, PostActionType.types[:inappropriate])
+          expect(subject.num_tl3_flags_against_user).to eq(1)
+          expect(subject.num_tl3_users_who_flagged).to eq(1)
+          expect(subject.should_autosilence?).to be_falsey
         end
 
         it 'returns false if enough flags but not enough users' do
-          subject.stubs(:num_tl3_flags_against_user).returns(3)
-          subject.stubs(:num_tl3_users_who_flagged).returns(1)
-          expect(subject.silence?).to be_falsey
+          PostAction.act(leader1, post, PostActionType.types[:inappropriate])
+          PostAction.act(leader1, post2, PostActionType.types[:inappropriate])
+          PostAction.act(leader1, Fabricate(:post, user: user), PostActionType.types[:inappropriate])
+          expect(subject.num_tl3_flags_against_user).to eq(3)
+          expect(subject.num_tl3_users_who_flagged).to eq(1)
+          expect(subject.should_autosilence?).to eq(false)
         end
 
         it 'returns true if enough flags and users' do
-          subject.stubs(:num_tl3_flags_against_user).returns(3)
-          subject.stubs(:num_tl3_users_who_flagged).returns(2)
-          expect(subject.silence?).to eq(true)
+          PostAction.act(leader1, post, PostActionType.types[:inappropriate])
+          PostAction.act(leader1, post2, PostActionType.types[:inappropriate])
+          PostAction.act(leader2, post, PostActionType.types[:inappropriate])
+          expect(subject.num_tl3_flags_against_user).to eq(3)
+          expect(subject.num_tl3_users_who_flagged).to eq(2)
+          expect(subject.should_autosilence?).to eq(true)
         end
       end
     end
@@ -279,7 +290,7 @@ describe SpamRule::AutoSilence do
       subject     { described_class.new(user) }
 
       it 'returns false' do
-        expect(subject.silence?).to be_truthy
+        expect(subject.should_autosilence?).to eq(false)
       end
     end
   end

spec/services/spam_rules_enforcer_spec.rb

@@ -1,29 +0,0 @@
-require 'rails_helper'
-
-describe SpamRulesEnforcer do
-
-  before do
-    SystemMessage.stubs(:create)
-  end
-
-  describe 'enforce!' do
-    context 'post argument' do
-      subject(:enforce) { described_class.enforce!(Fabricate.build(:post)) }
-
-      it 'performs the FlagSockpuppetRule' do
-        SpamRule::FlagSockpuppets.any_instance.expects(:perform).once
-        enforce
-      end
-    end
-
-    context 'user argument' do
-      subject(:enforce) { described_class.enforce!(Fabricate.build(:user)) }
-
-      it 'performs the AutoSilence' do
-        SpamRule::AutoSilence.any_instance.expects(:perform).once
-        enforce
-      end
-    end
-  end
-
-end

spec/services/user_silencer_spec.rb

@@ -112,6 +112,13 @@ describe UserSilencer do
       expect(post.topic.reload).to_not be_visible
     end
 
+    it "allows us to silence the user for a particular post" do
+      expect(UserSilencer.was_silenced_for?(post)).to eq(false)
+      UserSilencer.new(user, Discourse.system_user, post_id: post.id).silence
+      expect(user).to be_silenced
+      expect(UserSilencer.was_silenced_for?(post)).to eq(true)
+    end
+
     it "doesn't hide posts if user is TL1" do
       user.trust_level = 1
       subject.silence