FIX: do not send rejection emails to auto-deleted reviewable users (#12160)

FIX: add context when user is deleted via auto handle queued reviewable
FIX: do not delete email_log when a user is deleted

app/models/reviewable_user.rb

@@ -66,7 +66,7 @@ class ReviewableUser < Reviewable
       begin
         self.reject_reason = args[:reject_reason]
 
-        if args[:send_email] != false && SiteSetting.must_approve_users?
+        if args[:send_email] && SiteSetting.must_approve_users?
           # Execute job instead of enqueue because user has to exists to send email
           Jobs::CriticalUserEmail.new.execute({
             type: :signup_after_reject,
@@ -78,11 +78,16 @@ class ReviewableUser < Reviewable
         delete_args = {}
         delete_args[:block_ip] = true if args[:block_ip]
         delete_args[:block_email] = true if args[:block_email]
+        delete_args[:context] = if performed_by.id == Discourse.system_user.id
+          I18n.t("user.destroy_reasons.reviewable_reject_auto")
+        else
+          I18n.t("user.destroy_reasons.reviewable_reject")
+        end
 
         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
+        # However the reviewable record will be "rejected" and they will remain
         # unapproved in the database. A staff member can still approve them
         # via the admin.
       end

app/models/user.rb

@@ -53,7 +53,6 @@ class User < ActiveRecord::Base
   has_many :bookmarks, dependent: :delete_all
   has_many :notifications, dependent: :delete_all
   has_many :topic_users, dependent: :delete_all
-  has_many :email_logs, dependent: :delete_all
   has_many :incoming_emails, dependent: :delete_all
   has_many :user_visits, dependent: :delete_all
   has_many :user_auth_token_logs, dependent: :delete_all
@@ -67,6 +66,7 @@ class User < ActiveRecord::Base
   has_many :post_actions
   has_many :post_timings
   has_many :directory_items
+  has_many :email_logs
   has_many :security_keys, -> {
     where(enabled: true)
   }, class_name: "UserSecurityKey"

app/services/destroy_task.rb

@@ -81,7 +81,7 @@ class DestroyTask
   def destroy_users
     User.human_users.where(admin: false).find_each do |user|
       begin
-        if UserDestroyer.new(Discourse.system_user).destroy(user, delete_posts: true)
+        if UserDestroyer.new(Discourse.system_user).destroy(user, delete_posts: true, context: "destroy task")
           @io.puts "#{user.username} deleted"
         else
           @io.puts "#{user.username} not deleted"

config/locales/server.en.yml

@@ -2564,6 +2564,8 @@ en:
       fixed_primary_email: "Fixed primary email for staged user"
       same_ip_address: "Same IP address (%{ip_address}) as other users"
       inactive_user: "Inactive user"
+      reviewable_reject_auto: "Auto handle queued reviewables"
+      reviewable_reject: "Reviewable user rejected"
     email_in_spam_header: "User's first email was flagged as spam"
     already_silenced: "User was already silenced by %{staff} %{time_ago}."
     already_suspended: "User was already suspended by %{staff} %{time_ago}."

spec/integration/auto_reject_reviewable_users_spec.rb

@@ -0,0 +1,22 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe "auto reject reviewable users" do
+  context "reviewable users" do
+    fab!(:old_user) { Fabricate(:reviewable, created_at: 80.days.ago) }
+
+    it "does not send email to rejected user" do
+      SiteSetting.must_approve_users = true
+      SiteSetting.auto_handle_queued_age = 60
+
+      Jobs::CriticalUserEmail.any_instance.expects(:execute).never
+      Jobs::AutoQueueHandler.new.execute({})
+
+      expect(old_user.reload.rejected?).to eq(true)
+      expect(UserHistory.last.context).to eq(
+        I18n.t("user.destroy_reasons.reviewable_reject_auto")
+      )
+    end
+  end
+end

spec/models/reviewable_user_spec.rb

@@ -105,6 +105,9 @@ RSpec.describe ReviewableUser, type: :model do
         reviewable.reload
         expect(reviewable.target).to be_blank
         expect(reviewable.reject_reason).to eq("reject reason")
+        expect(UserHistory.last.context).to eq(
+          I18n.t("user.destroy_reasons.reviewable_reject")
+        )
       end
 
       it "allows us to reject and block a user" do
@@ -129,7 +132,7 @@ 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", send_email: false)
+        reviewable.perform(moderator, :reject_user_block, reject_reason: "reject reason")
       end
 
       it "optionaly sends email with reject reason" do

spec/services/user_destroyer_spec.rb

@@ -364,16 +364,6 @@ describe UserDestroyer do
       end
     end
 
-    context 'user got an email' do
-      let!(:email_log) { Fabricate(:email_log, user: user) }
-
-      it "deletes the email log" do
-        expect {
-          UserDestroyer.new(admin).destroy(user, delete_posts: true)
-        }.to change { EmailLog.count }.by(-1)
-      end
-    end
-
     context 'user liked things' do
       before do
         @topic = Fabricate(:topic, user: Fabricate(:user))
@@ -446,6 +436,16 @@ describe UserDestroyer do
         expect(log.details).to include(username)
       end
     end
+
+    context 'user got an email' do
+      let!(:email_log) { Fabricate(:email_log, user: user) }
+
+      it "does not delete the email log" do
+        expect {
+          UserDestroyer.new(admin).destroy(user, delete_posts: true)
+        }.to_not change { EmailLog.count }
+      end
+    end
   end
 
 end