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