From 14f9d40e486c39e74abfacdebbda86db1a8afd70 Mon Sep 17 00:00:00 2001
From: Robin Ward <robin.ward@gmail.com>
Date: Wed, 17 Apr 2019 11:26:43 -0400
Subject: [PATCH] FEATURE: Clarify Reviewable User Actions
"Approve" is now "Approve User" and "Delete" is a dropdown with a choice
that allows you to block.
---
app/controllers/admin/users_controller.rb | 4 +-
app/jobs/scheduled/auto_queue_handler.rb | 4 +-
app/models/invite_redeemer.rb | 2 +-
app/models/reviewable_user.rb | 41 +++++++++++++++---
app/models/user.rb | 4 +-
.../reviewable_bundled_action_serializer.rb | 10 ++++-
app/services/user_destroyer.rb | 2 +-
config/locales/server.en.yml | 10 +++++
spec/models/reviewable_history_spec.rb | 4 +-
spec/models/reviewable_user_spec.rb | 43 +++++++++++++------
spec/models/web_hook_spec.rb | 4 +-
spec/requests/reviewables_controller_spec.rb | 10 ++---
12 files changed, 103 insertions(+), 35 deletions(-)
diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index 976ea91beb8..8cd5a0f4126 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -293,12 +293,12 @@ class Admin::UsersController < Admin::AdminController
reviewable = ReviewableUser.find_by(target: @user) ||
Jobs::CreateUserReviewable.new.execute(user_id: @user.id).reviewable
- reviewable.perform(current_user, :approve)
+ reviewable.perform(current_user, :approve_user)
render body: nil
end
def approve_bulk
- Reviewable.bulk_perform_targets(current_user, :approve, 'ReviewableUser', params[:users])
+ Reviewable.bulk_perform_targets(current_user, :approve_user, 'ReviewableUser', params[:users])
render body: nil
end
diff --git a/app/jobs/scheduled/auto_queue_handler.rb b/app/jobs/scheduled/auto_queue_handler.rb
index ff7d6ea8536..46072b8698f 100644
--- a/app/jobs/scheduled/auto_queue_handler.rb
+++ b/app/jobs/scheduled/auto_queue_handler.rb
@@ -15,8 +15,10 @@ module Jobs
if reviewable.is_a?(ReviewableFlaggedPost)
reviewable.perform(Discourse.system_user, :ignore)
- else
+ elsif reviewable.is_a?(ReviewableQueuedPost)
reviewable.perform(Discourse.system_user, :reject)
+ elsif reviewable.is_a?(ReviewableUser)
+ reviewable.perform(Discourse.system_user, :reject_user_delete)
end
end
end
diff --git a/app/models/invite_redeemer.rb b/app/models/invite_redeemer.rb
index 66d2352b92d..ce80b21bff9 100644
--- a/app/models/invite_redeemer.rb
+++ b/app/models/invite_redeemer.rb
@@ -127,7 +127,7 @@ InviteRedeemer = Struct.new(:invite, :username, :name, :password, :user_custom_f
if invited_user.present? && reviewable_user = ReviewableUser.find_by(target: invited_user)
reviewable_user.perform(
invite.invited_by,
- :approve,
+ :approve_user,
send_email: false,
approved_by_invite: true
)
diff --git a/app/models/reviewable_user.rb b/app/models/reviewable_user.rb
index d19f7702cf1..3f1f2ee470f 100644
--- a/app/models/reviewable_user.rb
+++ b/app/models/reviewable_user.rb
@@ -12,11 +12,31 @@ class ReviewableUser < Reviewable
def build_actions(actions, guardian, args)
return unless pending?
- actions.add(:approve) if guardian.can_approve?(target) || args[:approved_by_invite]
- actions.add(:reject)
+ if guardian.can_approve?(target) || args[:approved_by_invite]
+ actions.add(:approve_user) do |a|
+ a.icon = 'user-plus'
+ a.label = "reviewables.actions.approve_user.title"
+ end
+ end
+
+ reject = actions.add_bundle(
+ 'reject_user',
+ icon: 'user-times',
+ label: 'reviewables.actions.reject_user.title'
+ )
+ actions.add(:reject_user_delete, bundle: reject) do |a|
+ a.icon = 'user-times'
+ a.label = "reviewables.actions.reject_user.delete.title"
+ a.description = "reviewables.actions.reject_user.delete.description"
+ end
+ actions.add(:reject_user_block, bundle: reject) do |a|
+ a.icon = 'ban'
+ a.label = "reviewables.actions.reject_user.block.title"
+ a.description = "reviewables.actions.reject_user.block.description"
+ end
end
- def perform_approve(performed_by, args)
+ def perform_approve_user(performed_by, args)
ReviewableUser.set_approved_fields!(target, performed_by)
target.save!
@@ -34,14 +54,17 @@ class ReviewableUser < Reviewable
create_result(:success, :approved)
end
- def perform_reject(performed_by, args)
-
+ def perform_reject_user_delete(performed_by, args)
# We'll delete the user if we can
if target.present?
destroyer = UserDestroyer.new(performed_by)
begin
- destroyer.destroy(target)
+ delete_args = {}
+ delete_args[:block_ip] = true if args[:block_ip]
+ delete_args[:block_email] = true if args[:block_email]
+
+ destroyer.destroy(target, delete_args)
rescue UserDestroyer::PostsExistError
# If a user has posts, we won't delete them to preserve their content.
# However the reviable record will be "rejected" and they will remain
@@ -53,6 +76,12 @@ class ReviewableUser < Reviewable
create_result(:success, :rejected)
end
+ def perform_reject_user_block(performed_by, args)
+ args[:block_email] = true
+ args[:block_ip] = true
+ perform_reject_user_delete(performed_by, args)
+ end
+
# Update's the user's fields for approval but does not save. This
# can be used when generating a new user that is approved on create
def self.set_approved_fields!(user, approved_by)
diff --git a/app/models/user.rb b/app/models/user.rb
index f1f79896473..ea0cb0b8189 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -421,7 +421,7 @@ class User < ActiveRecord::Base
approved_by = User.find_by(id: approved_by) if approved_by.is_a?(Numeric)
if reviewable_user = ReviewableUser.find_by(target: self)
- result = reviewable_user.perform(approved_by, :approve, send_email: send_mail)
+ result = reviewable_user.perform(approved_by, :approve_user, send_email: send_mail)
if result.success?
Reviewable.set_approved_fields!(self, approved_by)
return true
@@ -904,7 +904,7 @@ class User < ActiveRecord::Base
self.update!(active: false)
if reviewable = ReviewableUser.pending.find_by(target: self)
- reviewable.perform(performed_by, :reject)
+ reviewable.perform(performed_by, :reject_user_delete)
end
end
diff --git a/app/serializers/reviewable_bundled_action_serializer.rb b/app/serializers/reviewable_bundled_action_serializer.rb
index 06654390152..ecce2fae472 100644
--- a/app/serializers/reviewable_bundled_action_serializer.rb
+++ b/app/serializers/reviewable_bundled_action_serializer.rb
@@ -3,6 +3,14 @@ class ReviewableBundledActionSerializer < ApplicationSerializer
has_many :actions, serializer: ReviewableActionSerializer, root: 'actions'
def label
- I18n.t(object.label)
+ I18n.t(object.label, default: nil)
+ end
+
+ def include_label?
+ label.present?
+ end
+
+ def include_icon?
+ icon.present?
end
end
diff --git a/app/services/user_destroyer.rb b/app/services/user_destroyer.rb
index f6c40ac47c1..900ffbb8b0b 100644
--- a/app/services/user_destroyer.rb
+++ b/app/services/user_destroyer.rb
@@ -115,7 +115,7 @@ class UserDestroyer
# After the user is deleted, remove the reviewable
if reviewable = Reviewable.pending.find_by(target: user)
- reviewable.perform(@actor, :reject)
+ reviewable.perform(@actor, :reject_user_delete)
end
result
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index a008d44dbeb..bf274cca176 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -4461,6 +4461,16 @@ en:
title: "Ignore"
approve:
title: "Approve"
+ approve_user:
+ title: "Approve User"
+ reject_user:
+ title: "Delete User..."
+ delete:
+ title: "Delete User"
+ description: "The user will be deleted from the forum."
+ block:
+ title: "Delete and Block User"
+ description: "The user will be deleted, and we'll block their IP and email address."
reject:
title: "Reject"
delete_user:
diff --git a/spec/models/reviewable_history_spec.rb b/spec/models/reviewable_history_spec.rb
index 735695d9735..bb8b7bb7cd0 100644
--- a/spec/models/reviewable_history_spec.rb
+++ b/spec/models/reviewable_history_spec.rb
@@ -8,7 +8,7 @@ RSpec.describe ReviewableHistory, type: :model do
it "adds a `created` history event when a reviewable is created" do
reviewable = ReviewableUser.needs_review!(target: user, created_by: admin)
- reviewable.perform(moderator, :approve)
+ reviewable.perform(moderator, :approve_user)
reviewable = ReviewableUser.needs_review!(target: user, created_by: admin)
history = reviewable.history
@@ -21,7 +21,7 @@ RSpec.describe ReviewableHistory, type: :model do
it "adds a `transitioned` event when transitioning" do
reviewable = ReviewableUser.needs_review!(target: user, created_by: admin)
- reviewable.perform(moderator, :approve)
+ reviewable.perform(moderator, :approve_user)
reviewable = ReviewableUser.needs_review!(target: user, created_by: admin)
history = reviewable.history
diff --git a/spec/models/reviewable_user_spec.rb b/spec/models/reviewable_user_spec.rb
index 189e99fe4d6..ba8f9929cd2 100644
--- a/spec/models/reviewable_user_spec.rb
+++ b/spec/models/reviewable_user_spec.rb
@@ -12,17 +12,18 @@ RSpec.describe ReviewableUser, type: :model do
context "actions_for" do
let(:reviewable) { Fabricate(:reviewable) }
- it "returns approve/disapprove in the pending state" do
+ it "returns correct actions in the pending state" do
actions = reviewable.actions_for(Guardian.new(moderator))
- expect(actions.has?(:approve)).to eq(true)
- expect(actions.has?(:reject)).to eq(true)
+ expect(actions.has?(:approve_user)).to eq(true)
+ expect(actions.has?(:reject_user_delete)).to eq(true)
+ expect(actions.has?(:reject_user_block)).to eq(true)
end
it "doesn't return anything in the approved state" do
reviewable.status = Reviewable.statuses[:approved]
actions = reviewable.actions_for(Guardian.new(moderator))
- expect(actions.has?(:approve)).to eq(false)
- expect(actions.has?(:reject)).to eq(false)
+ expect(actions.has?(:approve_user)).to eq(false)
+ expect(actions.has?(:reject_user_delete)).to eq(false)
end
end
@@ -52,7 +53,7 @@ RSpec.describe ReviewableUser, type: :model do
let(:reviewable) { Fabricate(:reviewable) }
context "approve" do
it "allows us to approve a user" do
- result = reviewable.perform(moderator, :approve)
+ result = reviewable.perform(moderator, :approve_user)
expect(result.success?).to eq(true)
expect(reviewable.pending?).to eq(false)
@@ -64,7 +65,7 @@ RSpec.describe ReviewableUser, type: :model do
end
it "allows us to reject a user" do
- result = reviewable.perform(moderator, :reject)
+ result = reviewable.perform(moderator, :reject_user_delete)
expect(result.success?).to eq(true)
expect(reviewable.pending?).to eq(false)
@@ -75,9 +76,27 @@ RSpec.describe ReviewableUser, type: :model do
expect(reviewable.target).to be_blank
end
+ it "allows us to reject and block a user" do
+ email = reviewable.target.email
+ ip = reviewable.target.ip_address
+
+ result = reviewable.perform(moderator, :reject_user_block)
+ expect(result.success?).to eq(true)
+
+ expect(reviewable.pending?).to eq(false)
+ expect(reviewable.rejected?).to eq(true)
+
+ # Rejecting deletes the user record
+ reviewable.reload
+ expect(reviewable.target).to be_blank
+
+ expect(ScreenedEmail.should_block?(email)).to eq(true)
+ expect(ScreenedIpAddress.should_block?(ip)).to eq(true)
+ end
+
it "allows us to reject a user who has posts" do
Fabricate(:post, user: reviewable.target)
- result = reviewable.perform(moderator, :reject)
+ result = reviewable.perform(moderator, :reject_user_delete)
expect(result.success?).to eq(true)
expect(reviewable.pending?).to eq(false)
@@ -92,7 +111,7 @@ RSpec.describe ReviewableUser, type: :model do
it "allows us to reject a user who has been deleted" do
reviewable.target.destroy!
reviewable.reload
- result = reviewable.perform(moderator, :reject)
+ result = reviewable.perform(moderator, :reject_user_delete)
expect(result.success?).to eq(true)
expect(reviewable.rejected?).to eq(true)
expect(reviewable.target).to be_blank
@@ -131,7 +150,7 @@ RSpec.describe ReviewableUser, type: :model do
end
after do
- ReviewableUser.find_by(target: user).perform(admin, :approve)
+ ReviewableUser.find_by(target: user).perform(admin, :approve_user)
end
it "enqueues a 'signup after approval' email if must_approve_users is true" do
@@ -151,7 +170,7 @@ RSpec.describe ReviewableUser, type: :model do
it 'triggers a extensibility event' do
user && admin # bypass the user_created event
event = DiscourseEvent.track_events {
- ReviewableUser.find_by(target: user).perform(admin, :approve)
+ ReviewableUser.find_by(target: user).perform(admin, :approve_user)
}.first
expect(event[:event_name]).to eq(:user_approved)
@@ -161,7 +180,7 @@ RSpec.describe ReviewableUser, type: :model do
it 'triggers a extensibility event' do
user && admin # bypass the user_created event
event = DiscourseEvent.track_events {
- ReviewableUser.find_by(target: user).perform(admin, :approve)
+ ReviewableUser.find_by(target: user).perform(admin, :approve_user)
}.first
expect(event[:event_name]).to eq(:user_approved)
diff --git a/spec/models/web_hook_spec.rb b/spec/models/web_hook_spec.rb
index 7e038006608..e2834b5b316 100644
--- a/spec/models/web_hook_spec.rb
+++ b/spec/models/web_hook_spec.rb
@@ -322,7 +322,7 @@ describe WebHook do
payload = JSON.parse(job_args["payload"])
expect(payload["id"]).to eq(admin.id)
- ReviewableUser.find_by(target: user).perform(admin, :approve)
+ ReviewableUser.find_by(target: user).perform(admin, :approve_user)
job_args = Jobs::EmitWebHookEvent.jobs.last["args"].first
expect(job_args["event_name"]).to eq("user_approved")
@@ -515,7 +515,7 @@ describe WebHook do
payload = JSON.parse(job_args["payload"])
expect(payload["id"]).to eq(reviewable.id)
- reviewable.perform(Discourse.system_user, :reject)
+ reviewable.perform(Discourse.system_user, :reject_user_delete)
job_args = Jobs::EmitWebHookEvent.jobs.last["args"].first
expect(job_args["event_name"]).to eq("reviewable_transitioned_to")
diff --git a/spec/requests/reviewables_controller_spec.rb b/spec/requests/reviewables_controller_spec.rb
index f88a872d673..e70551ce653 100644
--- a/spec/requests/reviewables_controller_spec.rb
+++ b/spec/requests/reviewables_controller_spec.rb
@@ -241,7 +241,7 @@ describe ReviewablesController do
end
it "returns 404 when the reviewable does not exist" do
- put "/review/12345/perform/approve.json?version=0"
+ put "/review/12345/perform/approve_user.json?version=0"
expect(response.code).to eq("404")
end
@@ -252,7 +252,7 @@ describe ReviewablesController do
it "ensures the user can see the reviewable" do
reviewable.update_column(:reviewable_by_moderator, false)
- put "/review/#{reviewable.id}/perform/approve.json?version=#{reviewable.version}"
+ put "/review/#{reviewable.id}/perform/approve_user.json?version=#{reviewable.version}"
expect(response.code).to eq("404")
end
@@ -265,7 +265,7 @@ describe ReviewablesController do
end
it "requires a version parameter" do
- put "/review/#{reviewable.id}/perform/approve.json"
+ put "/review/#{reviewable.id}/perform/approve_user.json"
expect(response.code).to eq("422")
result = ::JSON.parse(response.body)
expect(result['errors']).to be_present
@@ -274,7 +274,7 @@ describe ReviewablesController do
it "succeeds for a valid action" do
other_reviewable = Fabricate(:reviewable)
- put "/review/#{reviewable.id}/perform/approve.json?version=#{reviewable.version}"
+ put "/review/#{reviewable.id}/perform/approve_user.json?version=#{reviewable.version}"
expect(response.code).to eq("200")
json = ::JSON.parse(response.body)
expect(json['reviewable_perform_result']['success']).to eq(true)
@@ -290,7 +290,7 @@ describe ReviewablesController do
describe "simultaneous perform" do
it "fails when the version is wrong" do
- put "/review/#{reviewable.id}/perform/approve.json?version=#{reviewable.version + 1}"
+ put "/review/#{reviewable.id}/perform/approve_user.json?version=#{reviewable.version + 1}"
expect(response.code).to eq("409")
json = ::JSON.parse(response.body)
expect(json['errors']).to be_present