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

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

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

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
       )

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)

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
 

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

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

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:

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

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)

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")

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