FIX: Always allow us to reject users, even if they are deleted

app/models/reviewable_user.rb

@@ -12,7 +12,7 @@ class ReviewableUser < Reviewable
     return unless pending?
 
     actions.add(:approve) if guardian.can_approve?(target) || args[:approved_by_invite]
-    actions.add(:reject) if guardian.can_delete_user?(target)
+    actions.add(:reject)
   end
 
   def perform_approve(performed_by, args)
@@ -34,13 +34,20 @@ class ReviewableUser < Reviewable
   end
 
   def perform_reject(performed_by, args)
-    destroyer = UserDestroyer.new(performed_by) unless args[:skip_delete]
 
-    # 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
-    # unapproved in the database. A staff member can still approve them
-    # via the admin.
-    destroyer.destroy(target) rescue UserDestroyer::PostsExistError
+    # We'll delete the user if we can
+    if target.present?
+      destroyer = UserDestroyer.new(performed_by)
+
+      begin
+        destroyer.destroy(target)
+      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
+        # unapproved in the database. A staff member can still approve them
+        # via the admin.
+      end
+    end
 
     create_result(:success, :rejected)
   end

app/services/user_destroyer.rb

@@ -23,15 +23,13 @@ class UserDestroyer
 
     prepare_for_destroy(user) if opts[:prepare_for_destroy] == true
 
+    result = nil
+
     optional_transaction(open_transaction: opts[:transaction]) do
 
       Draft.where(user_id: user.id).delete_all
       Reviewable.where(created_by_id: user.id).delete_all
 
-      if reviewable = Reviewable.find_by(target: user)
-        reviewable.perform(@actor, :reject, skip_delete: true) rescue Reviewable::InvalidAction
-      end
-
       if opts[:delete_posts]
         user.posts.each do |post|
 
@@ -75,47 +73,52 @@ class UserDestroyer
       # keep track of emails used
       user_emails = user.user_emails.pluck(:email)
 
-      user.destroy.tap do |u|
-        if u
-          if opts[:block_email]
-            user_emails.each do |email|
-              ScreenedEmail.block(email, ip_address: u.ip_address)&.record_match!
-            end
+      if result = user.destroy
+        if opts[:block_email]
+          user_emails.each do |email|
+            ScreenedEmail.block(email, ip_address: result.ip_address)&.record_match!
           end
-
-          if opts[:block_ip] && u.ip_address
-            ScreenedIpAddress.watch(u.ip_address)&.record_match!
-            if u.registration_ip_address && u.ip_address != u.registration_ip_address
-              ScreenedIpAddress.watch(u.registration_ip_address)&.record_match!
-            end
-          end
-
-          Post.unscoped.where(user_id: u.id).update_all(user_id: nil)
-
-          # If this user created categories, fix those up:
-          Category.where(user_id: u.id).each do |c|
-            c.user_id = Discourse::SYSTEM_USER_ID
-            c.save!
-            if topic = Topic.unscoped.find_by(id: c.topic_id)
-              topic.recover!
-              topic.user_id = Discourse::SYSTEM_USER_ID
-              topic.save!
-            end
-          end
-
-          unless opts[:quiet]
-            if @actor == user
-              deleted_by = Discourse.system_user
-              opts[:context] = I18n.t("staff_action_logs.user_delete_self", url: opts[:context])
-            else
-              deleted_by = @actor
-            end
-            StaffActionLogger.new(deleted_by).log_user_deletion(user, opts.slice(:context))
-          end
-          MessageBus.publish "/file-change", ["refresh"], user_ids: [u.id]
         end
+
+        if opts[:block_ip] && result.ip_address
+          ScreenedIpAddress.watch(result.ip_address)&.record_match!
+          if result.registration_ip_address && result.ip_address != result.registration_ip_address
+            ScreenedIpAddress.watch(result.registration_ip_address)&.record_match!
+          end
+        end
+
+        Post.unscoped.where(user_id: result.id).update_all(user_id: nil)
+
+        # If this user created categories, fix those up:
+        Category.where(user_id: result.id).each do |c|
+          c.user_id = Discourse::SYSTEM_USER_ID
+          c.save!
+          if topic = Topic.unscoped.find_by(id: c.topic_id)
+            topic.recover!
+            topic.user_id = Discourse::SYSTEM_USER_ID
+            topic.save!
+          end
+        end
+
+        unless opts[:quiet]
+          if @actor == user
+            deleted_by = Discourse.system_user
+            opts[:context] = I18n.t("staff_action_logs.user_delete_self", url: opts[:context])
+          else
+            deleted_by = @actor
+          end
+          StaffActionLogger.new(deleted_by).log_user_deletion(user, opts.slice(:context))
+        end
+        MessageBus.publish "/file-change", ["refresh"], user_ids: [result.id]
       end
     end
+
+    # After the user is deleted, remove the reviewable
+    if reviewable = Reviewable.pending.find_by(target: user)
+      reviewable.perform(@actor, :reject)
+    end
+
+    result
   end
 
   protected

spec/models/reviewable_user_spec.rb

@@ -87,6 +87,15 @@ RSpec.describe ReviewableUser, type: :model do
         expect(reviewable.target).to be_present
         expect(reviewable.target.approved).to eq(false)
       end
+
+      it "allows us to reject a user who has been deleted" do
+        reviewable.target.destroy!
+        reviewable.reload
+        result = reviewable.perform(moderator, :reject)
+        expect(result.success?).to eq(true)
+        expect(reviewable.rejected?).to eq(true)
+        expect(reviewable.target).to be_blank
+      end
     end
   end