FIX: Exclude users with posts from purge_unactivated query (#12231)

Unactivated users that have posts cannot be deleted so we shouldn't
include them in the initial query to try and purge them. Otherwise we
are just loading up sidekiq with pointless work to be doing every day.

Without this change if there are 201 unactivated users to purge, but the
first 200 have posts, the 201st user will never be deleted even though
it is the only user that doesn't have a post and is actually the one
that should be deleted.
This commit is contained in:
Blake Erickson 2021-02-28 22:46:28 -07:00 committed by GitHub
parent feaf3cb97e
commit f53546c03d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 14 additions and 4 deletions

View File

@ -1575,11 +1575,14 @@ class User < ActiveRecord::Base
.where("NOT EXISTS .where("NOT EXISTS
(SELECT 1 FROM topic_allowed_users tu JOIN topics t ON t.id = tu.topic_id AND t.user_id > 0 WHERE tu.user_id = users.id LIMIT 1) (SELECT 1 FROM topic_allowed_users tu JOIN topics t ON t.id = tu.topic_id AND t.user_id > 0 WHERE tu.user_id = users.id LIMIT 1)
") ")
.where("NOT EXISTS
(SELECT 1 FROM posts p WHERE p.user_id = users.id LIMIT 1)
")
.limit(200) .limit(200)
.find_each do |user| .find_each do |user|
begin begin
destroyer.destroy(user, context: I18n.t(:purge_reason)) destroyer.destroy(user, context: I18n.t(:purge_reason))
rescue Discourse::InvalidAccess, UserDestroyer::PostsExistError rescue Discourse::InvalidAccess
# keep going # keep going
end end
end end
@ -1668,7 +1671,6 @@ class User < ActiveRecord::Base
) )
SQL SQL
end end
end end
# == Schema Information # == Schema Information

View File

@ -1471,6 +1471,7 @@ describe User do
let!(:unactivated_old) { Fabricate(:user, active: false, created_at: 1.month.ago) } let!(:unactivated_old) { Fabricate(:user, active: false, created_at: 1.month.ago) }
let!(:unactivated_old_with_system_pm) { Fabricate(:user, active: false, created_at: 2.months.ago) } let!(:unactivated_old_with_system_pm) { Fabricate(:user, active: false, created_at: 2.months.ago) }
let!(:unactivated_old_with_human_pm) { Fabricate(:user, active: false, created_at: 2.months.ago) } let!(:unactivated_old_with_human_pm) { Fabricate(:user, active: false, created_at: 2.months.ago) }
let!(:unactivated_old_with_post) { Fabricate(:user, active: false, created_at: 1.month.ago) }
before do before do
PostCreator.new(Discourse.system_user, PostCreator.new(Discourse.system_user,
@ -1486,17 +1487,24 @@ describe User do
archetype: Archetype.private_message, archetype: Archetype.private_message,
target_usernames: [unactivated_old_with_human_pm.username], target_usernames: [unactivated_old_with_human_pm.username],
).create ).create
PostCreator.new(unactivated_old_with_post,
title: "Test topic from a user",
raw: "This is a sample message"
).create
end end
it 'should only remove old, unactivated users' do it 'should only remove old, unactivated users' do
User.purge_unactivated User.purge_unactivated
expect(User.real.all).to match_array([user, unactivated, unactivated_old_with_human_pm]) expect(User.real.all).to match_array([user, unactivated, unactivated_old_with_human_pm, unactivated_old_with_post])
end end
it "does nothing if purge_unactivated_users_grace_period_days is 0" do it "does nothing if purge_unactivated_users_grace_period_days is 0" do
SiteSetting.purge_unactivated_users_grace_period_days = 0 SiteSetting.purge_unactivated_users_grace_period_days = 0
User.purge_unactivated User.purge_unactivated
expect(User.real.all).to match_array([user, unactivated, unactivated_old, unactivated_old_with_system_pm, unactivated_old_with_human_pm]) expect(User.real.all).to match_array([
user, unactivated, unactivated_old, unactivated_old_with_system_pm, unactivated_old_with_human_pm, unactivated_old_with_post
])
end end
end end