From 4dc8c3c409138847ee48a81af0d8ec00feb136df Mon Sep 17 00:00:00 2001
From: Roman Rizzi <rizziromanalejandro@gmail.com>
Date: Tue, 15 Jun 2021 12:35:45 -0300
Subject: [PATCH] FEATURE: Blocking is optional when deleting a user from the
 review queue. (#13375)

Subclasses must call #delete_user_actions inside build_actions to support user deletion. The method adds a delete user bundle, which has a delete and a delete + block option. Every subclass is responsible for implementing these actions.
---
 app/jobs/scheduled/auto_queue_handler.rb     |  2 +-
 app/models/reviewable.rb                     | 22 +++++++++
 app/models/reviewable_flagged_post.rb        | 51 ++++++++++++--------
 app/models/reviewable_queued_post.rb         | 34 +++++++------
 app/models/reviewable_user.rb                | 24 ++-------
 app/models/user.rb                           |  2 +-
 app/services/user_destroyer.rb               |  2 +-
 config/locales/server.en.yml                 |  6 ---
 spec/models/reviewable_flagged_post_spec.rb  |  5 +-
 spec/models/reviewable_queued_post_spec.rb   |  6 ---
 spec/models/reviewable_spec.rb               | 19 ++++++++
 spec/models/reviewable_user_spec.rb          | 26 +++++-----
 spec/models/web_hook_spec.rb                 |  2 +-
 spec/requests/reviewables_controller_spec.rb |  4 +-
 14 files changed, 117 insertions(+), 88 deletions(-)

diff --git a/app/jobs/scheduled/auto_queue_handler.rb b/app/jobs/scheduled/auto_queue_handler.rb
index 500a45b079..dfc87b77f1 100644
--- a/app/jobs/scheduled/auto_queue_handler.rb
+++ b/app/jobs/scheduled/auto_queue_handler.rb
@@ -20,7 +20,7 @@ module Jobs
         elsif reviewable.is_a?(ReviewableQueuedPost)
           reviewable.perform(Discourse.system_user, :reject_post)
         elsif reviewable.is_a?(ReviewableUser)
-          reviewable.perform(Discourse.system_user, :reject_user_delete)
+          reviewable.perform(Discourse.system_user, :delete_user)
         end
       end
     end
diff --git a/app/models/reviewable.rb b/app/models/reviewable.rb
index 3fc8364413..6876462a75 100644
--- a/app/models/reviewable.rb
+++ b/app/models/reviewable.rb
@@ -643,6 +643,28 @@ class Reviewable < ActiveRecord::Base
     self.score
   end
 
+  def delete_user_actions(actions, require_reject_reason: false)
+    reject = actions.add_bundle(
+      'reject_user',
+      icon: 'user-times',
+      label: 'reviewables.actions.reject_user.title'
+    )
+
+    actions.add(:delete_user, bundle: reject) do |a|
+      a.icon = 'user-times'
+      a.label = "reviewables.actions.reject_user.delete.title"
+      a.require_reject_reason = require_reject_reason
+      a.description = "reviewables.actions.reject_user.delete.description"
+    end
+
+    actions.add(:delete_user_block, bundle: reject) do |a|
+      a.icon = 'ban'
+      a.label = "reviewables.actions.reject_user.block.title"
+      a.require_reject_reason = require_reject_reason
+      a.description = "reviewables.actions.reject_user.block.description"
+    end
+  end
+
 protected
 
   def increment_version!(version = nil)
diff --git a/app/models/reviewable_flagged_post.rb b/app/models/reviewable_flagged_post.rb
index dada954b57..f2971e7f6c 100644
--- a/app/models/reviewable_flagged_post.rb
+++ b/app/models/reviewable_flagged_post.rb
@@ -57,16 +57,6 @@ class ReviewableFlaggedPost < Reviewable
       build_action(actions, :agree_and_silence, icon: 'microphone-slash', bundle: agree, client_action: 'silence')
     end
 
-    if can_delete_spammer = potential_spam? && guardian.can_delete_all_posts?(target_created_by)
-      build_action(
-        actions,
-        :delete_spammer,
-        icon: 'exclamation-triangle',
-        bundle: agree,
-        confirm: true
-      )
-    end
-
     if post.user_deleted?
       build_action(actions, :agree_and_restore, icon: 'far-eye', bundle: agree)
     end
@@ -79,6 +69,10 @@ class ReviewableFlaggedPost < Reviewable
 
     build_action(actions, :ignore, icon: 'external-link-alt')
 
+    if potential_spam? && guardian.can_delete_all_posts?(target_created_by)
+      delete_user_actions(actions)
+    end
+
     if guardian.can_delete_post_or_topic?(post)
       delete = actions.add_bundle("#{id}-delete", icon: "far-trash-alt", label: "reviewables.actions.delete.title")
       build_action(actions, :delete_and_ignore, icon: 'external-link-alt', bundle: delete)
@@ -134,17 +128,22 @@ class ReviewableFlaggedPost < Reviewable
     agree(performed_by, args)
   end
 
-  def perform_delete_spammer(performed_by, args)
-    UserDestroyer.new(performed_by).destroy(
-      post.user,
-      delete_posts: true,
-      prepare_for_destroy: true,
-      block_email: true,
-      block_urls: true,
-      block_ip: true,
-      delete_as_spammer: true,
-      context: "review"
-    )
+  def perform_delete_user(performed_by, args)
+    delete_options = delete_opts
+
+    UserDestroyer.new(performed_by).destroy(post.user, delete_options)
+
+    agree(performed_by, args)
+  end
+
+  def perform_delete_user_block(performed_by, args)
+    delete_options = delete_opts
+
+    if Rails.env.production?
+      delete_options.merge!(block_email: true, block_ip: true)
+    end
+
+    UserDestroyer.new(performed_by).destroy(post.user, delete_options)
 
     agree(performed_by, args)
   end
@@ -302,6 +301,16 @@ protected
 
 private
 
+  def delete_opts
+    {
+      delete_posts: true,
+      prepare_for_destroy: true,
+      block_urls: true,
+      delete_as_spammer: true,
+      context: "review"
+    }
+  end
+
   def destroyer(performed_by, post)
     PostDestroyer.new(performed_by, post, reviewable: self)
   end
diff --git a/app/models/reviewable_queued_post.rb b/app/models/reviewable_queued_post.rb
index 105559444c..da791f6798 100644
--- a/app/models/reviewable_queued_post.rb
+++ b/app/models/reviewable_queued_post.rb
@@ -33,12 +33,7 @@ class ReviewableQueuedPost < Reviewable
     end
 
     if pending? && guardian.can_delete_user?(created_by)
-      actions.add(:delete_user) do |action|
-        action.icon = 'trash-alt'
-        action.button_class = 'btn-danger'
-        action.label = 'reviewables.actions.delete_user.title'
-        action.confirm_message = 'reviewables.actions.delete_user.confirm'
-      end
+      delete_user_actions(actions)
     end
 
     actions.add(:delete) if guardian.can_delete?(self)
@@ -133,24 +128,35 @@ class ReviewableQueuedPost < Reviewable
   end
 
   def perform_delete_user(performed_by, args)
-    delete_options = {
-      context: I18n.t('reviewables.actions.delete_user.reason'),
-      delete_posts: true,
-      block_urls: true,
-      block_email: true,
-      block_ip: true,
-      delete_as_spammer: true
-    }
+    delete_user(performed_by, delete_opts)
+  end
+
+  def perform_delete_user_block(performed_by, args)
+    delete_options = delete_opts
 
     if Rails.env.production?
       delete_options.merge!(block_email: true, block_ip: true)
     end
 
+    delete_user(performed_by, delete_options)
+  end
+
+  private
+
+  def delete_user(performed_by, delete_options)
     reviewable_ids = Reviewable.where(created_by: created_by).pluck(:id)
     UserDestroyer.new(performed_by).destroy(created_by, delete_options)
     create_result(:success) { |r| r.remove_reviewable_ids = reviewable_ids }
   end
 
+  def delete_opts
+    {
+      context: I18n.t('reviewables.actions.delete_user.reason'),
+      delete_posts: true,
+      block_urls: true,
+      delete_as_spammer: true
+    }
+  end
 end
 
 # == Schema Information
diff --git a/app/models/reviewable_user.rb b/app/models/reviewable_user.rb
index 93291bfd40..a2325a2239 100644
--- a/app/models/reviewable_user.rb
+++ b/app/models/reviewable_user.rb
@@ -19,23 +19,7 @@ class ReviewableUser < Reviewable
       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.require_reject_reason = !is_a_suspect_user?
-      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.require_reject_reason = !is_a_suspect_user?
-      a.description = "reviewables.actions.reject_user.block.description"
-    end
+    delete_user_actions(actions, require_reject_reason: !is_a_suspect_user?)
   end
 
   def perform_approve_user(performed_by, args)
@@ -56,7 +40,7 @@ class ReviewableUser < Reviewable
     create_result(:success, :approved)
   end
 
-  def perform_reject_user_delete(performed_by, args)
+  def perform_delete_user(performed_by, args)
     # We'll delete the user if we can
     if target.present?
       destroyer = UserDestroyer.new(performed_by)
@@ -96,10 +80,10 @@ class ReviewableUser < Reviewable
     create_result(:success, :rejected)
   end
 
-  def perform_reject_user_block(performed_by, args)
+  def perform_delete_user_block(performed_by, args)
     args[:block_email] = true
     args[:block_ip] = true
-    perform_reject_user_delete(performed_by, args)
+    perform_delete_user(performed_by, args)
   end
 
   # Update's the user's fields for approval but does not save. This
diff --git a/app/models/user.rb b/app/models/user.rb
index e90984675c..c43cfc7a34 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -1019,7 +1019,7 @@ class User < ActiveRecord::Base
     self.update!(active: false)
 
     if reviewable = ReviewableUser.pending.find_by(target: self)
-      reviewable.perform(performed_by, :reject_user_delete)
+      reviewable.perform(performed_by, :delete_user)
     end
   end
 
diff --git a/app/services/user_destroyer.rb b/app/services/user_destroyer.rb
index 24980ce96d..82ac26ab8d 100644
--- a/app/services/user_destroyer.rb
+++ b/app/services/user_destroyer.rb
@@ -108,7 +108,7 @@ class UserDestroyer
 
     # After the user is deleted, remove the reviewable
     if reviewable = ReviewableUser.pending.find_by(target: user)
-      reviewable.perform(@actor, :reject_user_delete)
+      reviewable.perform(@actor, :delete_user)
     end
 
     result
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index af1d8c4e46..cf7534a92f 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -4987,10 +4987,6 @@ en:
       agree_and_hide:
         title: "Hide Post"
         description: "Hide this post and automatically send the user a message urging them to edit it."
-      delete_spammer:
-        title: "Delete Spammer"
-        description: "Remove the user and all their posts and topics."
-        confirm: "Are you sure you want to delete all that user's posts, topics, and block their IP and email addresses?"
       delete_single:
         title: "Delete"
       delete:
@@ -5047,8 +5043,6 @@ en:
       approve_and_restore:
         title: "Approve and Restore post"
       delete_user:
-        title: "Delete User"
-        confirm: "Are you sure you want to delete that user? This will remove all of their posts and block their email and IP address."
         reason: "Deleted via review queue"
 
   email_style:
diff --git a/spec/models/reviewable_flagged_post_spec.rb b/spec/models/reviewable_flagged_post_spec.rb
index 339a7569a3..8b36f825ca 100644
--- a/spec/models/reviewable_flagged_post_spec.rb
+++ b/spec/models/reviewable_flagged_post_spec.rb
@@ -34,7 +34,8 @@ RSpec.describe ReviewableFlaggedPost, type: :model do
         expect(actions.has?(:agree_and_keep_hidden)).to eq(false)
         expect(actions.has?(:agree_and_silence)).to eq(true)
         expect(actions.has?(:agree_and_suspend)).to eq(true)
-        expect(actions.has?(:delete_spammer)).to eq(true)
+        expect(actions.has?(:delete_user)).to eq(true)
+        expect(actions.has?(:delete_user_block)).to eq(true)
         expect(actions.has?(:disagree)).to eq(true)
         expect(actions.has?(:ignore)).to eq(true)
         expect(actions.has?(:delete_and_ignore)).to eq(true)
@@ -137,7 +138,7 @@ RSpec.describe ReviewableFlaggedPost, type: :model do
     end
 
     it "supports deleting a spammer" do
-      reviewable.perform(moderator, :delete_spammer)
+      reviewable.perform(moderator, :delete_user_block)
       expect(reviewable).to be_approved
       expect(score.reload).to be_agreed
       expect(post.reload.deleted_at).to be_present
diff --git a/spec/models/reviewable_queued_post_spec.rb b/spec/models/reviewable_queued_post_spec.rb
index 97e5b61b76..92f17f7bf4 100644
--- a/spec/models/reviewable_queued_post_spec.rb
+++ b/spec/models/reviewable_queued_post_spec.rb
@@ -118,12 +118,6 @@ RSpec.describe ReviewableQueuedPost, type: :model do
       end
 
       context "delete_user" do
-        it "has the correct button class" do
-          expect(reviewable.actions_for(Guardian.new(moderator)).to_a.
-            find { |a| a.id == :delete_user }.button_class).
-            to eq("btn-danger")
-        end
-
         it "deletes the user and rejects the post" do
           other_reviewable = Fabricate(:reviewable_queued_post, created_by: reviewable.created_by)
 
diff --git a/spec/models/reviewable_spec.rb b/spec/models/reviewable_spec.rb
index f616c460db..9b9f32bf5d 100644
--- a/spec/models/reviewable_spec.rb
+++ b/spec/models/reviewable_spec.rb
@@ -539,4 +539,23 @@ RSpec.describe Reviewable, type: :model do
       expect(Reviewable.by_status(Reviewable.all, :reviewed)).to contain_exactly(reviewable)
     end
   end
+
+  context 'default actions' do
+    let(:reviewable) { Reviewable.new }
+    let(:actions) { Reviewable::Actions.new(reviewable, Guardian.new) }
+
+    describe '#delete_user_actions' do
+      it 'adds a bundle with the delete_user action' do
+        reviewable.delete_user_actions(actions)
+
+        expect(actions.has?(:delete_user)).to be true
+      end
+
+      it 'adds a bundle with the delete_user_block action' do
+        reviewable.delete_user_actions(actions)
+
+        expect(actions.has?(:delete_user_block)).to be true
+      end
+    end
+  end
 end
diff --git a/spec/models/reviewable_user_spec.rb b/spec/models/reviewable_user_spec.rb
index a16c6c2ac4..fde41e025b 100644
--- a/spec/models/reviewable_user_spec.rb
+++ b/spec/models/reviewable_user_spec.rb
@@ -17,35 +17,35 @@ RSpec.describe ReviewableUser, type: :model do
     it "returns correct actions in the pending state" do
       actions = reviewable.actions_for(Guardian.new(moderator))
       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)
+      expect(actions.has?(:delete_user)).to eq(true)
+      expect(actions.has?(:delete_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_user)).to eq(false)
-      expect(actions.has?(:reject_user_delete)).to eq(false)
+      expect(actions.has?(:delete_user_block)).to eq(false)
     end
 
     it 'can delete a user without a giving a rejection reason if the user was a spammer' do
       reviewable.reviewable_scores.build(user: admin, reason: 'suspect_user')
 
-      assert_require_reject_reason(:reject_user_delete, false)
+      assert_require_reject_reason(:delete_user, false)
     end
 
     it 'requires a rejection reason to delete a user' do
-      assert_require_reject_reason(:reject_user_delete, true)
+      assert_require_reject_reason(:delete_user, true)
     end
 
     it 'can delete and block a user without giving a rejection reason if the user was a spammer' do
       reviewable.reviewable_scores.build(user: admin, reason: 'suspect_user')
 
-      assert_require_reject_reason(:reject_user_block, false)
+      assert_require_reject_reason(:delete_user, false)
     end
 
     it 'requires a rejection reason to delete and block a user' do
-      assert_require_reject_reason(:reject_user_block, true)
+      assert_require_reject_reason(:delete_user_block, true)
     end
 
     def assert_require_reject_reason(id, expected)
@@ -95,7 +95,7 @@ RSpec.describe ReviewableUser, type: :model do
       end
 
       it "allows us to reject a user" do
-        result = reviewable.perform(moderator, :reject_user_delete, reject_reason: "reject reason")
+        result = reviewable.perform(moderator, :delete_user, reject_reason: "reject reason")
         expect(result.success?).to eq(true)
 
         expect(reviewable.pending?).to eq(false)
@@ -114,7 +114,7 @@ RSpec.describe ReviewableUser, type: :model do
         email = reviewable.target.email
         ip = reviewable.target.ip_address
 
-        result = reviewable.perform(moderator, :reject_user_block, reject_reason: "reject reason")
+        result = reviewable.perform(moderator, :delete_user_block, reject_reason: "reject reason")
         expect(result.success?).to eq(true)
 
         expect(reviewable.pending?).to eq(false)
@@ -132,18 +132,18 @@ RSpec.describe ReviewableUser, type: :model do
       it "is not sending email to the user about rejection" do
         SiteSetting.must_approve_users = true
         Jobs::CriticalUserEmail.any_instance.expects(:execute).never
-        reviewable.perform(moderator, :reject_user_block, reject_reason: "reject reason")
+        reviewable.perform(moderator, :delete_user_block, reject_reason: "reject reason")
       end
 
       it "optionally sends email with reject reason" do
         SiteSetting.must_approve_users = true
         Jobs::CriticalUserEmail.any_instance.expects(:execute).with(type: :signup_after_reject, user_id: reviewable.target_id, reject_reason: "reject reason").once
-        reviewable.perform(moderator, :reject_user_block, reject_reason: "reject reason", send_email: true)
+        reviewable.perform(moderator, :delete_user_block, reject_reason: "reject reason", send_email: true)
       end
 
       it "allows us to reject a user who has posts" do
         Fabricate(:post, user: reviewable.target)
-        result = reviewable.perform(moderator, :reject_user_delete)
+        result = reviewable.perform(moderator, :delete_user)
         expect(result.success?).to eq(true)
 
         expect(reviewable.pending?).to eq(false)
@@ -158,7 +158,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_user_delete)
+        result = reviewable.perform(moderator, :delete_user)
         expect(result.success?).to eq(true)
         expect(reviewable.rejected?).to eq(true)
         expect(reviewable.target).to be_blank
diff --git a/spec/models/web_hook_spec.rb b/spec/models/web_hook_spec.rb
index 73ef8c913a..fd8ed07966 100644
--- a/spec/models/web_hook_spec.rb
+++ b/spec/models/web_hook_spec.rb
@@ -506,7 +506,7 @@ describe WebHook do
       payload = JSON.parse(job_args["payload"])
       expect(payload["id"]).to eq(reviewable.id)
 
-      reviewable.perform(Discourse.system_user, :reject_user_delete)
+      reviewable.perform(Discourse.system_user, :delete_user)
       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 5b751099b5..37e7febadd 100644
--- a/spec/requests/reviewables_controller_spec.rb
+++ b/spec/requests/reviewables_controller_spec.rb
@@ -182,10 +182,10 @@ describe ReviewablesController do
         user.activate
         reviewable = ReviewableUser.find_by(target: user)
 
-        put "/review/#{reviewable.id}/perform/reject_user_delete.json?version=0"
+        put "/review/#{reviewable.id}/perform/delete_user.json?version=0"
         expect(response.code).to eq("200")
 
-        put "/review/#{reviewable.id}/perform/reject_user_delete.json?version=0&index=2"
+        put "/review/#{reviewable.id}/perform/delete_user.json?version=0&index=2"
         expect(response.code).to eq("404")
         json = response.parsed_body