Add User.unconfirmed scope, reduce factories in scheduler/user_cleanup spec (#31063)

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 }

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

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)
+        .to change { User.exists?(old_unconfirmed_user.id) }
-        .and change { Account.exists?(old_unconfirmed_user.account_id) }.from(true).to(false)
+        .from(true).to(false)
-      expect { moderation_note.reload }.to raise_error(ActiveRecord::RecordNotFound)
+        .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
+    private
-      subject.perform
-      expect(User.exists?(new_unconfirmed_user.id)).to be true
-      expect(Account.exists?(new_unconfirmed_user.account_id)).to be true
-    end
 
-    it 'does not delete the confirmed user or their account' do
+    def expect_preservation_of(user)
-      subject.perform
+      expect(User.exists?(user.id))
-      expect(User.exists?(confirmed_user.id)).to be true
+        .to be true
-      expect(Account.exists?(confirmed_user.account_id)).to be true
+      expect(Account.exists?(user.account_id))
+        .to be true
     end
   end
 end