From e881a5967149e34046f91d3f4445ebb457348b93 Mon Sep 17 00:00:00 2001
From: Matt Jankowski <matt@jankowski.online>
Date: Thu, 25 Jul 2024 10:18:24 -0400
Subject: [PATCH] Add `User.unconfirmed` scope, reduce factories in
 `scheduler/user_cleanup` spec (#31063)

---
 app/models/user.rb                            |  1 +
 .../scheduler/user_cleanup_scheduler.rb       |  2 +-
 .../scheduler/user_cleanup_scheduler_spec.rb  | 30 ++++++++++---------
 3 files changed, 18 insertions(+), 15 deletions(-)

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