Skip to content

Commit

Permalink
Deal with registration edge cases in UserCleaner (#693) (#716)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
Splines authored Nov 27, 2024
1 parent 46a3133 commit 2dc4bd6
Show file tree
Hide file tree
Showing 4 changed files with 122 additions and 44 deletions.
8 changes: 8 additions & 0 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
21 changes: 17 additions & 4 deletions app/models/user_cleaner.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
10 changes: 10 additions & 0 deletions spec/factories/users.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
127 changes: 87 additions & 40 deletions spec/models/user_cleaner_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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

Expand All @@ -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
Expand All @@ -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!

Expand All @@ -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!

Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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!
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 2dc4bd6

Please sign in to comment.