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
diff --git a/app/models/reviewable_user.rb b/app/models/reviewable_user.rb
index 3f05984..93291bf 100644
--- a/app/models/reviewable_user.rb
+++ b/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
diff --git a/app/models/user.rb b/app/models/user.rb
index 5722ae6..166494b 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 57afc0e..4c5036e 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 c7c554c..6eb78d0 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 0000000..1b827c0
--- /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 eabc362..b63c490 100644
--- a/spec/models/reviewable_user_spec.rb
+++ b/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
diff --git a/spec/services/user_destroyer_spec.rb b/spec/services/user_destroyer_spec.rb
index ea23dbf..9b474bf 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
GitHub sha: f75e1867