From efb4a55b9bba2dae02021255b999c3f57bf428f3 Mon Sep 17 00:00:00 2001 From: Arpit Jalan Date: Mon, 22 Feb 2021 18:37:47 +0530 Subject: [PATCH] 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 | 9 ++++++-- app/models/user.rb | 2 +- app/services/destroy_task.rb | 2 +- config/locales/server.en.yml | 2 ++ .../auto_reject_reviewable_users_spec.rb | 22 +++++++++++++++++++ spec/models/reviewable_user_spec.rb | 5 ++++- spec/services/user_destroyer_spec.rb | 20 ++++++++--------- 7 files changed, 47 insertions(+), 15 deletions(-) create mode 100644 spec/integration/auto_reject_reviewable_users_spec.rb diff --git a/app/models/reviewable_user.rb b/app/models/reviewable_user.rb index 4ede3c2ebe..e23fa06475 100644 --- a/app/models/reviewable_user.rb +++ b/app/models/reviewable_user.rb @@ -68,7 +68,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, @@ -80,11 +80,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 diff --git a/app/models/user.rb b/app/models/user.rb index c33b5f99a0..c9b41068a4 100644 --- a/app/models/user.rb +++ b/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" diff --git a/app/services/destroy_task.rb b/app/services/destroy_task.rb index 57afc0e3d7..4c5036eca6 100644 --- a/app/services/destroy_task.rb +++ b/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" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index c7c554cba8..6eb78d0523 100644 --- a/config/locales/server.en.yml +++ b/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}." diff --git a/spec/integration/auto_reject_reviewable_users_spec.rb b/spec/integration/auto_reject_reviewable_users_spec.rb new file mode 100644 index 0000000000..1b827c0440 --- /dev/null +++ b/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 diff --git a/spec/models/reviewable_user_spec.rb b/spec/models/reviewable_user_spec.rb index 6ec3b4d88c..a50cc9a2e2 100644 --- a/spec/models/reviewable_user_spec.rb +++ b/spec/models/reviewable_user_spec.rb @@ -77,6 +77,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 @@ -101,7 +104,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 diff --git a/spec/services/user_destroyer_spec.rb b/spec/services/user_destroyer_spec.rb index ea23dbf0b9..9b474bf8e6 100644 --- a/spec/services/user_destroyer_spec.rb +++ b/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