diff --git a/app/models/user.rb b/app/models/user.rb index 3952d0d1d..4d3337b0a 100644 --- a/app/models/user.rb +++ b/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 diff --git a/app/models/user_cleaner.rb b/app/models/user_cleaner.rb index a70bb8f7f..566d0c8cd 100644 --- a/app/models/user_cleaner.rb +++ b/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. diff --git a/spec/factories/users.rb b/spec/factories/users.rb index b9affdd33..4c7633ac8 100644 --- a/spec/factories/users.rb +++ b/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) diff --git a/spec/models/user_cleaner_spec.rb b/spec/models/user_cleaner_spec.rb index 689962e3e..1223a9048 100644 --- a/spec/models/user_cleaner_spec.rb +++ b/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,33 +23,74 @@ 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 @@ -57,7 +98,8 @@ 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