From 075ed1ab53bef4f55c1dfc0334269db2a87bc4c0 Mon Sep 17 00:00:00 2001
From: Neil Lalonde <neillalonde@gmail.com>
Date: Tue, 2 Jul 2013 14:42:30 -0400
Subject: [PATCH] Refactor user blocking code; hide the Block button in admin
---
.../admin/templates/user.js.handlebars | 10 +-
app/controllers/admin/users_controller.rb | 4 +-
app/services/spam_rules_enforcer.rb | 16 +--
app/services/user_blocker.rb | 36 +++++++
config/locales/server.en.yml | 9 ++
.../admin/users_controller_spec.rb | 22 ++---
spec/services/spam_rules_enforcer_spec.rb | 42 ++------
spec/services/user_blocker_spec.rb | 99 +++++++++++++++++++
8 files changed, 168 insertions(+), 70 deletions(-)
create mode 100644 app/services/user_blocker.rb
create mode 100644 spec/services/user_blocker_spec.rb
diff --git a/app/assets/javascripts/admin/templates/user.js.handlebars b/app/assets/javascripts/admin/templates/user.js.handlebars
index fbad6ba19..2e953e7f3 100644
--- a/app/assets/javascripts/admin/templates/user.js.handlebars
+++ b/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>
diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index c579ba8bc..1c51293b6 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/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
diff --git a/app/services/spam_rules_enforcer.rb b/app/services/spam_rules_enforcer.rb
index afda275c3..e48b7853b 100644
--- a/app/services/spam_rules_enforcer.rb
+++ b/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
diff --git a/app/services/user_blocker.rb b/app/services/user_blocker.rb
new file mode 100644
index 000000000..5024b2f3f
--- /dev/null
+++ b/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
\ No newline at end of file
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index 954053deb..83bfac2cd 100644
--- a/config/locales/server.en.yml
+++ b/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: |
diff --git a/spec/controllers/admin/users_controller_spec.rb b/spec/controllers/admin/users_controller_spec.rb
index 5e1f142ab..846759413 100644
--- a/spec/controllers/admin/users_controller_spec.rb
+++ b/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
diff --git a/spec/services/spam_rules_enforcer_spec.rb b/spec/services/spam_rules_enforcer_spec.rb
index 00c1dc2f7..ff410cdad 100644
--- a/spec/services/spam_rules_enforcer_spec.rb
+++ b/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
diff --git a/spec/services/user_blocker_spec.rb b/spec/services/user_blocker_spec.rb
new file mode 100644
index 000000000..232b2b380
--- /dev/null
+++ b/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