diff --git a/app/models/user.rb b/app/models/user.rb index 8bc0b23ce..728545692 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -117,6 +117,7 @@ class User < ApplicationRecord scope :pending, -> { where(approved: false) } scope :approved, -> { where(approved: true) } scope :confirmed, -> { where.not(confirmed_at: nil) } + scope :unconfirmed, -> { where(confirmed_at: nil) } scope :enabled, -> { where(disabled: false) } scope :disabled, -> { where(disabled: true) } scope :active, -> { confirmed.signed_in_recently.account_not_suspended } diff --git a/app/workers/scheduler/user_cleanup_scheduler.rb b/app/workers/scheduler/user_cleanup_scheduler.rb index 74abc2370..9f58d9225 100644 --- a/app/workers/scheduler/user_cleanup_scheduler.rb +++ b/app/workers/scheduler/user_cleanup_scheduler.rb @@ -16,7 +16,7 @@ class Scheduler::UserCleanupScheduler private def clean_unconfirmed_accounts! - User.where('confirmed_at is NULL AND confirmation_sent_at <= ?', UNCONFIRMED_ACCOUNTS_MAX_AGE_DAYS.days.ago).reorder(nil).find_in_batches do |batch| + User.unconfirmed.where(confirmation_sent_at: ..UNCONFIRMED_ACCOUNTS_MAX_AGE_DAYS.days.ago).reorder(nil).find_in_batches do |batch| # We have to do it separately because of missing database constraints AccountModerationNote.where(target_account_id: batch.map(&:account_id)).delete_all Account.where(id: batch.map(&:account_id)).delete_all diff --git a/spec/workers/scheduler/user_cleanup_scheduler_spec.rb b/spec/workers/scheduler/user_cleanup_scheduler_spec.rb index c3940901d..7952f2c14 100644 --- a/spec/workers/scheduler/user_cleanup_scheduler_spec.rb +++ b/spec/workers/scheduler/user_cleanup_scheduler_spec.rb @@ -12,29 +12,31 @@ describe Scheduler::UserCleanupScheduler do describe '#perform' do before do - # Need to update the already-existing users because their initialization overrides confirmation_sent_at + # Update already-existing users because initialization overrides `confirmation_sent_at` new_unconfirmed_user.update!(confirmed_at: nil, confirmation_sent_at: Time.now.utc) old_unconfirmed_user.update!(confirmed_at: nil, confirmation_sent_at: 10.days.ago) confirmed_user.update!(confirmed_at: 1.day.ago) end - it 'deletes the old unconfirmed user, their account, and the moderation note' do + it 'deletes the old unconfirmed user and metadata while preserving confirmed user and newer unconfirmed user' do expect { subject.perform } - .to change { User.exists?(old_unconfirmed_user.id) }.from(true).to(false) - .and change { Account.exists?(old_unconfirmed_user.account_id) }.from(true).to(false) - expect { moderation_note.reload }.to raise_error(ActiveRecord::RecordNotFound) + .to change { User.exists?(old_unconfirmed_user.id) } + .from(true).to(false) + .and change { Account.exists?(old_unconfirmed_user.account_id) } + .from(true).to(false) + expect { moderation_note.reload } + .to raise_error(ActiveRecord::RecordNotFound) + expect_preservation_of(new_unconfirmed_user) + expect_preservation_of(confirmed_user) end - it 'does not delete the new unconfirmed user or their account' do - subject.perform - expect(User.exists?(new_unconfirmed_user.id)).to be true - expect(Account.exists?(new_unconfirmed_user.account_id)).to be true - end + private - it 'does not delete the confirmed user or their account' do - subject.perform - expect(User.exists?(confirmed_user.id)).to be true - expect(Account.exists?(confirmed_user.account_id)).to be true + def expect_preservation_of(user) + expect(User.exists?(user.id)) + .to be true + expect(Account.exists?(user.account_id)) + .to be true end end end