Deal with registration edge cases in UserCleaner (#693)

* Deal with registration edge cases in UserCleaner

* Replace `last_sign_in_at` by `current_sign_in_at`

* Simplify inactive_users method via usage of scopes

* Add another scope for recently active users

app/models/user.rb

@@ -122,6 +122,14 @@ class User < ApplicationRecord
   scope :no_tutorial_name,
         -> { where(name_in_tutorials: nil) }
 
+  # Scopes for usage in the UserCleaner
+  scope :confirmed, -> { where.not(confirmed_at: nil) }
+  scope :unconfirmed, -> { where(confirmed_at: nil) }
+  scope :no_sign_in_data, -> { where(current_sign_in_at: nil) }
+  scope :active_recently, ->(threshold) { where(current_sign_in_at: threshold.ago..) }
+  scope :inactive_for, ->(threshold) { where(current_sign_in_at: ...threshold.ago) }
+  scope :confirmation_sent_before, ->(threshold) { where(confirmation_sent_at: ...threshold.ago) }
+
   searchable do
     text :tutorial_name
   end

app/models/user_cleaner.rb

@@ -37,17 +37,30 @@ class UserCleaner
   # Returns all users who have been inactive for INACTIVE_USER_THRESHOLD months,
   # i.e. their last sign-in date is more than INACTIVE_USER_THRESHOLD months ago.
   #
-  # Users without a last_sign_in_at date are also considered inactive. This is
+  # Users without a current_sign_in_at date are also considered inactive. This is
   # the case for users who have never logged in since PR #553 was merged.
+  #
+  # Edge cases for registration (that refine the above statements):
+  # - A user might have registered but never actually logged in (confirmed their
+  #   email address). In this case, we don't look at the current_sign_in_at date
+  #   (as this one is still nil), but at the confirmation_sent_at date to
+  #   determine if the user is considered inactive.
+  # - Another edge case is when users have registered and confirmed their mail,
+  #   but never logged in after that. In this case, current_sign_in_at is indeed nil,
+  #   but the user should only be considered inactive if the confirmation_sent_at
+  #   date is older than the threshold.
   def inactive_users
-    User.where(last_sign_in_at: ...INACTIVE_USER_THRESHOLD.ago)
-        .or(User.where(last_sign_in_at: nil))
+    threshold = INACTIVE_USER_THRESHOLD
+    User.confirmed.and(
+      User.inactive_for(threshold)
+      .or(User.no_sign_in_data.confirmation_sent_before(threshold))
+    ).or(User.unconfirmed.confirmation_sent_before(threshold))
   end
 
   # Returns all users who have been active in the last INACTIVE_USER_THRESHOLD months,
   # i.e. their last sign-in date is less than INACTIVE_USER_THRESHOLD months ago.
   def active_users
-    User.where(last_sign_in_at: INACTIVE_USER_THRESHOLD.ago..)
+    User.active_recently(INACTIVE_USER_THRESHOLD)
   end
 
   # Sets the deletion date for inactive users and sends an initial warning mail.

spec/factories/users.rb

@@ -17,6 +17,16 @@
       after(:create, &:confirm)
     end
 
+    trait :with_confirmation_sent_date do
+      transient do
+        confirmation_sent_date { Time.zone.now }
+      end
+
+      after(:create) do |user, context|
+        user.update(confirmation_sent_at: context.confirmation_sent_date)
+      end
+    end
+
     trait :consented do
       after(:create) do |user|
         user.update(consents: true, consented_at: Time.zone.now)

spec/models/user_cleaner_spec.rb

@@ -3,17 +3,17 @@
 RSpec.describe(UserCleaner, type: :model) do
   # Non-generic users are either admins, teachers or editors
   let(:user_admin) do
-    return FactoryBot.create(:user, deletion_date: Date.current - 1.day, admin: true)
+    return FactoryBot.create(:confirmed_user, deletion_date: Date.current - 1.day, admin: true)
   end
 
   let(:user_teacher) do
-    user_teacher = FactoryBot.create(:user, deletion_date: Date.current - 1.day)
+    user_teacher = FactoryBot.create(:confirmed_user, deletion_date: Date.current - 1.day)
     FactoryBot.create(:lecture, teacher: user_teacher)
     return user_teacher
   end
 
   let(:user_editor) do
-    user_editor = FactoryBot.create(:user, deletion_date: Date.current - 1.day)
+    user_editor = FactoryBot.create(:confirmed_user, deletion_date: Date.current - 1.day)
     FactoryBot.create(:lecture, editors: [user_editor])
     return user_editor
   end
@@ -23,41 +23,83 @@
   end
 
   describe("#inactive_users") do
-    it "counts users without last_sign_in_at date as inactive" do
-      FactoryBot.create(:user, last_sign_in_at: nil)
-      expect(UserCleaner.new.inactive_users.count).to eq(1)
-    end
+    context "when user is confirmed" do
+      it "counts users without current_sign_in_at date as inactive" do
+        # but only if also the confirmation date is older than the threshold
+        FactoryBot.create(:confirmed_user, :with_confirmation_sent_date,
+                          confirmation_sent_date: 5.months.ago, current_sign_in_at: nil)
+        FactoryBot.create(:confirmed_user, :with_confirmation_sent_date,
+                          confirmation_sent_date: 7.months.ago, current_sign_in_at: nil)
+        expect(UserCleaner.new.inactive_users.count).to eq(1)
+      end
 
-    it("counts users with last_sign_in_at date older than threshold as inactive") do
-      FactoryBot.create(:user, last_sign_in_at: 7.months.ago)
-      expect(UserCleaner.new.inactive_users.count).to eq(1)
+      it("counts users with current_sign_in_at date older than threshold as inactive") do
+        FactoryBot.create(:confirmed_user, current_sign_in_at: 7.months.ago)
+        expect(UserCleaner.new.inactive_users.count).to eq(1)
+      end
+
+      it "does not count users with current_sign_in_at date younger than threshold as inactive" do
+        FactoryBot.create(:confirmed_user, current_sign_in_at: 5.months.ago)
+        expect(UserCleaner.new.inactive_users.count).to eq(0)
+      end
     end
 
-    it "does not count users with last_sign_in_at date younger than threshold as inactive" do
-      FactoryBot.create(:user, last_sign_in_at: 5.months.ago)
-      expect(UserCleaner.new.inactive_users.count).to eq(0)
+    context "when user is not confirmed yet" do
+      def test_non_confirmed_user(confirmation_sent_date, expected_inactive_users_count)
+        user = FactoryBot.create(:user, :with_confirmation_sent_date,
+                                 confirmation_sent_date: confirmation_sent_date,
+                                 current_sign_in_at: nil)
+        FactoryBot.create(:user, :with_confirmation_sent_date,
+                          confirmation_sent_date: confirmation_sent_date,
+                          current_sign_in_at: 5.months.ago)
+        FactoryBot.create(:user, :with_confirmation_sent_date,
+                          confirmation_sent_date: confirmation_sent_date,
+                          current_sign_in_at: 7.months.ago)
+
+        expect(user.confirmed_at).to be_nil
+        expect(user.confirmation_sent_at).to eq(confirmation_sent_date)
+
+        expect(UserCleaner.new.inactive_users.count).to eq(expected_inactive_users_count)
+      end
+
+      context "when registration was recently" do
+        it "does not count user as inactive regardless of value of last_sign_in_date" do
+          test_non_confirmed_user(5.days.ago, 0)
+        end
+      end
+
+      context "when registration was long ago" do
+        it "counts users as inactive regardless of value of last_sign_in_date" do
+          test_non_confirmed_user(7.months.ago, 3)
+        end
+      end
     end
   end
 
   describe("#set/unset_deletion_date") do
     context "when deletion date is nil" do
       it "assigns a deletion date to inactive users" do
-        inactive_user = FactoryBot.create(:user, last_sign_in_at: 7.months.ago)
-        active_user = FactoryBot.create(:user, last_sign_in_at: 5.months.ago)
+        inactive_user = FactoryBot.create(:confirmed_user, current_sign_in_at: 7.months.ago)
+        inactive_user2 = FactoryBot.create(:user, :with_confirmation_sent_date,
+                                           confirmation_sent_date: 7.months.ago)
+        active_user = FactoryBot.create(:confirmed_user, current_sign_in_at: 5.months.ago)
 
         UserCleaner.new.set_deletion_date_for_inactive_users
         inactive_user.reload
+        inactive_user2.reload
         active_user.reload
 
         expect(inactive_user.deletion_date).to eq(Date.current + 40.days)
+        expect(inactive_user2.deletion_date).to eq(Date.current + 40.days)
         expect(active_user.deletion_date).to be_nil
       end
 
       it "only assigns a deletion date to a limited number of users" do
         max_deletions = 3
         UserCleaner::MAX_DELETIONS_PER_RUN = max_deletions
 
-        FactoryBot.create_list(:user, max_deletions + 2, last_sign_in_at: 7.months.ago)
+        FactoryBot.create_list(:confirmed_user, max_deletions + 2,
+                               current_sign_in_at: 7.months.ago)
 
         UserCleaner.new.set_deletion_date_for_inactive_users
 
@@ -68,24 +110,29 @@
 
     context "when a deletion date is assigned" do
       it "does not overwrite the deletion date" do
-        user = FactoryBot.create(:user, last_sign_in_at: 7.months.ago,
-                                        deletion_date: Date.current + 42.days)
+        user = FactoryBot.create(:confirmed_user, current_sign_in_at: 7.months.ago,
+                                                  deletion_date: Date.current + 42.days)
+        user2 = FactoryBot.create(:user, :with_confirmation_sent_date,
+                                  confirmation_sent_date: 7.months.ago,
+                                  deletion_date: Date.current + 44.days)
 
         UserCleaner.new.set_deletion_date_for_inactive_users
         user.reload
+        user2.reload
 
         expect(user.deletion_date).to eq(Date.current + 42.days)
+        expect(user2.deletion_date).to eq(Date.current + 44.days)
       end
     end
 
     it "unassigns a deletion date from recently active users" do
       deletion_date = Date.current + 5.days
-      user_inactive = FactoryBot.create(:user, deletion_date: deletion_date,
-                                               last_sign_in_at: 7.months.ago)
-      user_inactive2 = FactoryBot.create(:user, deletion_date: deletion_date,
-                                                last_sign_in_at: 6.months.ago - 1.day)
-      user_active = FactoryBot.create(:user, deletion_date: deletion_date,
-                                             last_sign_in_at: 2.days.ago)
+      user_inactive = FactoryBot.create(:confirmed_user, deletion_date: deletion_date,
+                                                         current_sign_in_at: 7.months.ago)
+      user_inactive2 = FactoryBot.create(:confirmed_user, deletion_date: deletion_date,
+                                                          current_sign_in_at: 6.months.ago - 1.day)
+      user_active = FactoryBot.create(:confirmed_user, deletion_date: deletion_date,
+                                                       current_sign_in_at: 2.days.ago)
 
       UserCleaner.new.unset_deletion_date_for_recently_active_users
       user_inactive.reload
@@ -100,9 +147,9 @@
 
   describe("#delete_users") do
     it "deletes users with a deletion date in the past or present" do
-      user_past1 = FactoryBot.create(:user, deletion_date: Date.current - 1.day)
-      user_past2 = FactoryBot.create(:user, deletion_date: Date.current - 1.year)
-      user_present = FactoryBot.create(:user, deletion_date: Date.current)
+      user_past1 = FactoryBot.create(:confirmed_user, deletion_date: Date.current - 1.day)
+      user_past2 = FactoryBot.create(:confirmed_user, deletion_date: Date.current - 1.year)
+      user_present = FactoryBot.create(:confirmed_user, deletion_date: Date.current)
 
       UserCleaner.new.delete_users_according_to_deletion_date!
 
@@ -112,8 +159,8 @@
     end
 
     it "does not delete users with a deletion date in the future" do
-      user_future1 = FactoryBot.create(:user, deletion_date: Date.current + 1.day)
-      user_future2 = FactoryBot.create(:user, deletion_date: Date.current + 1.year)
+      user_future1 = FactoryBot.create(:confirmed_user, deletion_date: Date.current + 1.day)
+      user_future2 = FactoryBot.create(:confirmed_user, deletion_date: Date.current + 1.year)
 
       UserCleaner.new.delete_users_according_to_deletion_date!
 
@@ -122,13 +169,13 @@
     end
 
     it "does not delete users without a deletion date" do
-      user = FactoryBot.create(:user, deletion_date: nil)
+      user = FactoryBot.create(:confirmed_user, deletion_date: nil)
       UserCleaner.new.delete_users_according_to_deletion_date!
       expect(User.where(id: user.id)).to exist
     end
 
     it "deletes only generic users" do
-      user_generic = FactoryBot.create(:user, deletion_date: Date.current - 1.day)
+      user_generic = FactoryBot.create(:confirmed_user, deletion_date: Date.current - 1.day)
       user_admin
       user_teacher
       user_editor
@@ -145,7 +192,7 @@
   describe("mails") do
     context "when setting a deletion date" do
       it "enqueues a deletion warning mail (40 days)" do
-        FactoryBot.create(:user, last_sign_in_at: 7.months.ago)
+        FactoryBot.create(:confirmed_user, current_sign_in_at: 7.months.ago)
 
         expect do
           UserCleaner.new.set_deletion_date_for_inactive_users
@@ -166,7 +213,7 @@
 
     context "when a deletion date is assigned" do
       def test_enqueues_additional_deletion_warning_mails(num_days)
-        FactoryBot.create(:user, deletion_date: Date.current + num_days.days)
+        FactoryBot.create(:confirmed_user, deletion_date: Date.current + num_days.days)
 
         expect do
           UserCleaner.new.send_additional_warning_mails
@@ -181,7 +228,7 @@ def test_enqueues_additional_deletion_warning_mails(num_days)
       end
 
       it "does not enqueue an additional deletion warning mail for 40 days" do
-        FactoryBot.create(:user, deletion_date: Date.current + 40.days)
+        FactoryBot.create(:confirmed_user, deletion_date: Date.current + 40.days)
 
         expect do
           UserCleaner.new.send_additional_warning_mails
@@ -201,7 +248,7 @@ def test_enqueues_additional_deletion_warning_mails(num_days)
 
     context "when a user is finally deleted" do
       it "enqueues a deletion mail" do
-        FactoryBot.create(:user, deletion_date: Date.current - 1.day)
+        FactoryBot.create(:confirmed_user, deletion_date: Date.current - 1.day)
 
         expect do
           UserCleaner.new.delete_users_according_to_deletion_date!
@@ -211,11 +258,11 @@ def test_enqueues_additional_deletion_warning_mails(num_days)
   end
 
   describe("#pending_deletion_mail") do
-    let(:user_de) { FactoryBot.create(:user, locale: "de") }
-    let(:user_en) { FactoryBot.create(:user, locale: "en") }
+    let(:user_de) { FactoryBot.create(:confirmed_user, locale: "de") }
+    let(:user_en) { FactoryBot.create(:confirmed_user, locale: "en") }
 
     def test_subject_line(num_days)
-      user = FactoryBot.create(:user)
+      user = FactoryBot.create(:confirmed_user)
       mailer = UserCleanerMailer.with(user: user).pending_deletion_email(num_days)
       expect(mailer.subject).to include(num_days.to_s)
     end
@@ -252,8 +299,8 @@ def test_subject_line(num_days)
   end
 
   describe("#deletion_mail") do
-    let(:user_de) { FactoryBot.create(:user, locale: "de") }
-    let(:user_en) { FactoryBot.create(:user, locale: "en") }
+    let(:user_de) { FactoryBot.create(:confirmed_user, locale: "de") }
+    let(:user_en) { FactoryBot.create(:confirmed_user, locale: "en") }
 
     it "has mail subject localized to the user's locale" do
       mailer = UserCleanerMailer.with(user: user_de).deletion_email