diff --git a/app/jobs/scheduled/purge_inactive.rb b/app/jobs/scheduled/purge_unactivated.rb similarity index 70% rename from app/jobs/scheduled/purge_inactive.rb rename to app/jobs/scheduled/purge_unactivated.rb index 21417bef216..992b1bddc88 100644 --- a/app/jobs/scheduled/purge_inactive.rb +++ b/app/jobs/scheduled/purge_unactivated.rb @@ -1,5 +1,5 @@ module Jobs - class PurgeInactive < Jobs::Scheduled + class PurgeUnactived < Jobs::Scheduled every 1.day def execute(args) diff --git a/app/models/user.rb b/app/models/user.rb index e17796c45b5..765050e92f9 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1198,22 +1198,22 @@ class User < ActiveRecord::Base end end - # Delete unactivated accounts (without verified email) that are over a week old def self.purge_unactivated return [] if SiteSetting.purge_unactivated_users_grace_period_days <= 0 - to_destroy = User.where(active: false) - .joins('INNER JOIN user_stats AS us ON us.user_id = users.id') - .where("created_at < ?", SiteSetting.purge_unactivated_users_grace_period_days.days.ago) - .where('NOT admin AND NOT moderator') - .limit(200) - destroyer = UserDestroyer.new(Discourse.system_user) - to_destroy.each do |u| + + User + .where(active: false) + .where("created_at < ?", SiteSetting.purge_unactivated_users_grace_period_days.days.ago) + .where("NOT admin AND NOT moderator") + .where("NOT EXISTS (SELECT 1 FROM topic_allowed_users WHERE user_id = users.id LIMIT 1)") + .limit(200) + .find_each do |user| begin - destroyer.destroy(u, context: I18n.t(:purge_reason)) + destroyer.destroy(user, context: I18n.t(:purge_reason)) rescue Discourse::InvalidAccess, UserDestroyer::PostsExistError - # if for some reason the user can't be deleted, continue on to the next one + # keep going end end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index f725472bc00..35983247a2c 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1236,24 +1236,28 @@ describe User do describe "#purge_unactivated" do let!(:user) { Fabricate(:user) } - let!(:inactive) { Fabricate(:user, active: false) } - let!(:inactive_old) { Fabricate(:user, active: false, created_at: 1.month.ago) } + let!(:unactivated) { Fabricate(:user, active: false) } + let!(:unactivated_old) { Fabricate(:user, active: false, created_at: 1.month.ago) } + let!(:unactivated_old_with_pm) { Fabricate(:user, active: false, created_at: 2.months.ago) } + + before do + PostCreator.new(Discourse.system_user, + title: "Welcome to our Discourse", + raw: "This is a welcome message", + archetype: Archetype.private_message, + target_usernames: [unactivated_old_with_pm.username], + ).create + end it 'should only remove old, unactivated users' do User.purge_unactivated - all_users = User.all - expect(all_users.include?(user)).to eq(true) - expect(all_users.include?(inactive)).to eq(true) - expect(all_users.include?(inactive_old)).to eq(false) + expect(User.real.all).to match_array([user, unactivated, unactivated_old_with_pm]) end it "does nothing if purge_unactivated_users_grace_period_days is 0" do SiteSetting.purge_unactivated_users_grace_period_days = 0 User.purge_unactivated - all_users = User.all - expect(all_users.include?(user)).to eq(true) - expect(all_users.include?(inactive)).to eq(true) - expect(all_users.include?(inactive_old)).to eq(true) + expect(User.real.all).to match_array([user, unactivated, unactivated_old, unactivated_old_with_pm]) end end