Refactor user blocking code; hide the Block button in admin

app/assets/javascripts/admin/templates/user.js.handlebars

@@ -175,15 +175,16 @@
         {{i18n admin.user.unban}}
       </button>
       {{banDuration}}
+      {{i18n admin.user.banned_explanation}}
     {{else}}
       {{#if canBan}}
         <button class='btn btn-danger' {{action ban target="content"}}>
           <i class='icon icon-ban-circle'></i>
           {{i18n admin.user.ban}}
         </button>
+        {{i18n admin.user.banned_explanation}}
       {{/if}}
     {{/if}}
-    {{i18n admin.user.banned_explanation}}
     </div>
   </div>
   <div class='display-row'>
@@ -195,13 +196,8 @@
           <i class='icon icon-thumbs-up'></i>
           {{i18n admin.user.unblock}}
         </button>
-      {{else}}
-        <button class='btn btn-danger' {{action block target="content"}}>
-          <i class='icon icon-ban-circle'></i>
-          {{i18n admin.user.block}}
-        </button>
+        {{i18n admin.user.block_explanation}}
       {{/if}}
-      {{i18n admin.user.block_explanation}}
     </div>
   </div>
 </section>

app/controllers/admin/users_controller.rb

@@ -96,13 +96,13 @@ class Admin::UsersController < Admin::AdminController
 
   def block
     guardian.ensure_can_block_user! @user
-    SpamRulesEnforcer.punish! @user
+    UserBlocker.block(@user, current_user)
     render nothing: true
   end
 
   def unblock
     guardian.ensure_can_unblock_user! @user
-    SpamRulesEnforcer.clear @user
+    UserBlocker.unblock(@user, current_user)
     render nothing: true
   end
 

app/services/spam_rules_enforcer.rb

@@ -17,10 +17,6 @@ class SpamRulesEnforcer
     SpamRulesEnforcer.new(user).punish_user
   end
 
-  def self.clear(user)
-    SpamRulesEnforcer.new(user).clear_user
-  end
-
 
   def initialize(user)
     @user = user
@@ -52,20 +48,10 @@ class SpamRulesEnforcer
 
   def punish_user
     Post.transaction do
-      Post.update_all(["hidden = true, hidden_reason_id = COALESCE(hidden_reason_id, ?)", Post.hidden_reasons[:new_user_spam_threshold_reached]], user_id: @user.id)
-      topic_ids = Post.where('user_id = ? and post_number = ?', @user.id, 1).pluck(:topic_id)
-      Topic.update_all({ visible: false }, id: topic_ids) unless topic_ids.empty?
-      SystemMessage.create(@user, :too_many_spam_flags)
+      UserBlocker.block(@user, nil, {message: :too_many_spam_flags})
       GroupMessage.create(Group[:moderators].name, :user_automatically_blocked, {user: @user, limit_once_per: false})
-      @user.blocked = true
-      @user.save
     end
   end
 
-  def clear_user
-    SystemMessage.create(@user, :unblocked)
-    @user.blocked = false
-    @user.save
-  end
 
 end

app/services/user_blocker.rb

@@ -0,0 +1,36 @@
+class UserBlocker
+
+  def initialize(user, by_user=nil, opts={})
+    @user, @by_user, @opts = user, by_user, opts
+  end
+
+  def self.block(user, by_user=nil, opts={})
+    UserBlocker.new(user, by_user, opts).block
+  end
+
+  def self.unblock(user, by_user=nil, opts={})
+    UserBlocker.new(user, by_user, opts).unblock
+  end
+
+  def block
+    hide_posts
+    @user.blocked = true
+    if @user.save
+      SystemMessage.create(@user, @opts[:message] || :blocked_by_staff)
+    end
+  end
+
+  def hide_posts
+    Post.update_all(["hidden = true, hidden_reason_id = COALESCE(hidden_reason_id, ?)", Post.hidden_reasons[:new_user_spam_threshold_reached]], user_id: @user.id)
+    topic_ids = Post.where('user_id = ? and post_number = ?', @user.id, 1).pluck(:topic_id)
+    Topic.update_all({ visible: false }, id: topic_ids) unless topic_ids.empty?
+  end
+
+  def unblock
+    @user.blocked = false
+    if @user.save
+      SystemMessage.create(@user, :unblocked)
+    end
+  end
+
+end

config/locales/server.en.yml

@@ -872,6 +872,15 @@ en:
 
         For additional guidance, please refer to our [FAQ](%{base_url}/faq).
 
+    blocked_by_staff:
+      subject_template: "Account blocked"
+      text_body_template: |
+        Hello,
+
+        This is an automated message from %{site_name} to inform you that your account has been blocked by a staff member.
+
+        For additional guidance, please refer to our [FAQ](%{base_url}/faq).
+
     user_automatically_blocked:
       subject_template: "User %{username} was automatically blocked"
       text_body_template: |

spec/controllers/admin/users_controller_spec.rb

@@ -191,13 +191,13 @@ describe Admin::UsersController do
 
     context 'block' do
       before do
-        @user = Fabricate(:user)
+        @reg_user = Fabricate(:user)
       end
 
       it "raises an error when the user doesn't have permission" do
-        Guardian.any_instance.expects(:can_block_user?).with(@user).returns(false)
-        SpamRulesEnforcer.expects(:punish!).never
-        xhr :put, :block, user_id: @user.id
+        Guardian.any_instance.expects(:can_block_user?).with(@reg_user).returns(false)
+        UserBlocker.expects(:block).never
+        xhr :put, :block, user_id: @reg_user.id
         response.should be_forbidden
       end
 
@@ -207,19 +207,19 @@ describe Admin::UsersController do
       end
 
       it "punishes the user for spamming" do
-        SpamRulesEnforcer.expects(:punish!).with(@user)
-        xhr :put, :block, user_id: @user.id
+        UserBlocker.expects(:block).with(@reg_user, @user, anything)
+        xhr :put, :block, user_id: @reg_user.id
       end
     end
 
     context 'unblock' do
       before do
-        @user = Fabricate(:user)
+        @reg_user = Fabricate(:user)
       end
 
       it "raises an error when the user doesn't have permission" do
-        Guardian.any_instance.expects(:can_unblock_user?).with(@user).returns(false)
-        xhr :put, :unblock, user_id: @user.id
+        Guardian.any_instance.expects(:can_unblock_user?).with(@reg_user).returns(false)
+        xhr :put, :unblock, user_id: @reg_user.id
         response.should be_forbidden
       end
 
@@ -229,8 +229,8 @@ describe Admin::UsersController do
       end
 
       it "punishes the user for spamming" do
-        SpamRulesEnforcer.expects(:clear).with(@user)
-        xhr :put, :unblock, user_id: @user.id
+        UserBlocker.expects(:unblock).with(@reg_user, @user, anything)
+        xhr :put, :unblock, user_id: @reg_user.id
       end
     end
 

spec/services/spam_rules_enforcer_spec.rb

@@ -2,6 +2,10 @@ require 'spec_helper'
 
 describe SpamRulesEnforcer do
 
+  before do
+    SystemMessage.stubs(:create)
+  end
+
   before do
     SiteSetting.stubs(:flags_required_to_hide_post).returns(0) # never
     SiteSetting.stubs(:num_flags_to_block_new_user).returns(2)
@@ -107,14 +111,9 @@ describe SpamRulesEnforcer do
       subject.stubs(:block?).returns(true)
     end
 
-    it "hides all the user's posts" do
+    it "blocks the user" do
+      UserBlocker.expects(:block).with(user, nil, has_entries(message: :too_many_spam_flags))
       subject.punish_user
-      expect(post.reload).to be_hidden
-    end
-
-    it "hides the topic if the post was the first post" do
-      subject.punish_user
-      expect(post.topic.reload).to_not be_visible
     end
 
     it 'prevents the user from making new posts' do
@@ -122,19 +121,13 @@ describe SpamRulesEnforcer do
       expect(Guardian.new(user).can_create_post?(nil)).to be_false
     end
 
-    it 'sends private messages to the user and to moderators' do
-      SystemMessage.expects(:create).with(user, anything, anything)
+    it 'sends private message to moderators' do
       moderator = Fabricate(:moderator)
       GroupMessage.expects(:create).with do |group, msg_type, params|
         group == Group[:moderators].name and msg_type == :user_automatically_blocked and params[:user].id == user.id
       end
       subject.punish_user
     end
-
-    it 'sets the blocked flag' do
-      subject.punish_user
-      expect(user.reload.blocked).to be_true
-    end
   end
 
   describe 'block?' do
@@ -220,25 +213,4 @@ describe SpamRulesEnforcer do
     end
   end
 
-  describe "clear_user" do
-    let!(:admin)  { Fabricate(:admin) } # needed for SystemMessage
-    let(:user)    { Fabricate(:user) }
-    subject       { SpamRulesEnforcer.new(user) }
-
-    it 'sets blocked flag to false' do
-      subject.clear_user
-      expect(user.reload).to_not be_blocked
-    end
-
-    it 'sends a system message' do
-      SystemMessage.expects(:create).with(user, anything, anything)
-      subject.clear_user
-    end
-
-    it 'allows user to make new posts' do
-      subject.clear_user
-      expect(Guardian.new(user).can_create_post?(nil)).to be_true
-    end
-  end
-
 end

spec/services/user_blocker_spec.rb

@@ -0,0 +1,99 @@
+require 'spec_helper'
+
+describe UserBlocker do
+
+  before do
+    SystemMessage.stubs(:create)
+  end
+
+  describe 'block' do
+    let(:user)           { stub_everything(save: true) }
+    let(:blocker)        { UserBlocker.new(user) }
+    subject(:block_user) { blocker.block }
+
+    it 'blocks the user' do
+      u = Fabricate(:user)
+      expect { UserBlocker.block(u) }.to change { u.reload.blocked? }
+    end
+
+    it 'hides posts' do
+      blocker.expects(:hide_posts)
+      block_user
+    end
+
+    context 'given a staff user argument' do
+      it 'sends the correct message to the blocked user' do
+        SystemMessage.unstub(:create)
+        SystemMessage.expects(:create).with(user, :blocked_by_staff).returns(true)
+        UserBlocker.block(user, Fabricate.build(:admin))
+      end
+
+      # TODO: it 'logs the action'
+    end
+
+    context 'not given a staff user argument' do
+      it 'sends a default message to the user' do
+        SystemMessage.unstub(:create)
+        SystemMessage.expects(:create).with(user, :blocked_by_staff).returns(true)
+        UserBlocker.block(user, Fabricate.build(:admin))
+      end
+    end
+
+    context 'given a message option' do
+      it 'sends that message to the user' do
+        SystemMessage.unstub(:create)
+        SystemMessage.expects(:create).with(user, :the_custom_message).returns(true)
+        UserBlocker.block(user, Fabricate.build(:admin), {message: :the_custom_message})
+      end
+    end
+
+    it "doesn't send a pm if save fails" do
+      user.stubs(:save).returns(false)
+      SystemMessage.unstub(:create)
+      SystemMessage.expects(:create).never
+      block_user
+    end
+  end
+
+  describe 'unblock' do
+    let(:user)             { stub_everything(save: true) }
+    subject(:unblock_user) { UserBlocker.unblock(user, Fabricate.build(:admin)) }
+
+    it 'unblocks the user' do
+      u = Fabricate(:user, blocked: true)
+      expect { UserBlocker.unblock(u) }.to change { u.reload.blocked? }
+    end
+
+    it 'sends a message to the user' do
+      SystemMessage.unstub(:create)
+      SystemMessage.expects(:create).with(user, :unblocked).returns(true)
+      unblock_user
+    end
+
+    it "doesn't send a pm if save fails" do
+      user.stubs(:save).returns(false)
+      SystemMessage.unstub(:create)
+      SystemMessage.expects(:create).never
+      unblock_user
+    end
+
+    # TODO: it 'logs the action'
+  end
+
+  describe 'hide_posts' do
+    let(:user)    { Fabricate(:user) }
+    let!(:post)   { Fabricate(:post, user: user) }
+    subject       { UserBlocker.new(user) }
+
+    it "hides all the user's posts" do
+      subject.block
+      expect(post.reload).to be_hidden
+    end
+
+    it "hides the topic if the post was the first post" do
+      subject.block
+      expect(post.topic.reload).to_not be_visible
+    end
+  end
+
+end