From d0f8f44f897a222fe88c57386daba5faa4db61ed Mon Sep 17 00:00:00 2001
From: Splines <37160523+Splines@users.noreply.github.com>
Date: Tue, 13 Aug 2024 23:28:08 +0200
Subject: [PATCH] Delete users that haven't logged in for too long (#647)
* Add deletion_date to users in DB
* Set deletion date in user cleaner
* Remove old user cleaner code & add first unit test
* Don't just mount `spec` folder, but complete `app` folder for tests
This is to ensure that local changes in the `app` folder are reflected
in the Docker container such that newly written tests can be run through
without having to rebuild the whole Docker image.
* Revert "Don't just mount `spec` folder, but complete `app` folder for tests"
This reverts commit b3f3ec800c8d23b6ce87e9c93da0111b046b1bde.
* Add more unit tests for UserCleaner
* Improve tests for deletion of generic users only
* Split test for deletion of users
* Unset deletion date after successful user login
* Implement un-setting the deletion date & add tests
* Rewrite deletion unsetting to reuse existing method
* Send initial warning mail about deletion in 40 days
* Send additional warning mails when deletion date is near
* Add schema comment pointing to UserCleaner
* Test mail delivery
* Add "!" to unsafe method
* Use Rails logger to log info about deleted users
* Move UserCleaner from models to workers
* Setup user cleaner cron job to run daily
* Improve user cleaner doc strings & refactor
* Remove user ghost hash remnants
* Avoid unnecessary `to_date` conversion
* Improve comment for `after_authentication` hook
* Fix wording in comment
* Make better use of `let` RSpec keyword
* Limit the number of users deleted in one run
* Simplify unsetting deletion date logic
* Define `active_users` to reduce SQL operations
* Use `7.1` as Rails version specifier in migrations
* Explicitly set `INACTIVE_USER_THRESHOLD` in tests
* Make `MAX_DELETIONS_PER_RUN` an env variable
* Don't send deletion warning mails to non-generic users
* Send a final deletion mail once the user is deleted
* Move user cleaner file back to models folder
* Rename user cleaner files
* Count users without last_sign_in_at date as inactive
* Improve docstring for `inactive_users`
* Shorten user cleaner comment in docker.env
* Introduce `PRODUCTION_NAME` env variable
* Don't run user cleaner for mampf-experimental/dev
* Rewrite logic for when user cleaner is started
* Fix rubocop "where range"
* Try to use after instead of append_after
* Revert "Try to use after instead of append_after"
This reverts commit 90aa6621be350c09c32ad3b903122500b1d37da4.
* Try to not use cached docker in CI/CD unit tests
* Revert "Try to not use cached docker in CI/CD unit tests"
This reverts commit 069032b2e4cd5150d90806c0ca1e752e47d6a11e.
* Move transaction strategy to before each block
* Use around each in database cleaner config
* Use version 2.1.0 of database cleaner
* Revert "Use version 2.1.0 of database cleaner"
This reverts commit 6dfcd9694d6a4c2d29a38a0394aaa3823a591b4d.
* Add missing require "rails_helper"
* Try deletion strategy of database cleaner
---
app/mailers/mathi_mailer.rb | 8 -
app/mailers/user_cleaner_mailer.rb | 28 ++
app/models/user_cleaner.rb | 219 +++++++------
.../layouts/warning_mail_layout.html.erb | 18 ++
app/views/mathi_mailer/ghost_email.html.erb | 1 -
.../deletion_email.html.erb | 10 +
.../pending_deletion_email.html.erb | 12 +
app/workers/user_cleaner.rb | 13 +
app/workers/user_cleaner_job.rb | 8 -
config/environments/production.rb | 5 +
config/initializers/after_sign_in.rb | 10 +
config/locales/de.yml | 16 +
config/locales/en.yml | 15 +
config/schedule.yml | 7 +-
...0240530200000_add_deletion_date_to_user.rb | 5 +
...40605200000_remove_ghost_hash_from_user.rb | 5 +
db/schema.rb | 4 +-
docker/development/docker-compose.yml | 1 +
.../production/docker-compose.production.yml | 2 +
docker/production/docker.env | 3 +
docker/test/docker-compose.yml | 1 +
spec/factories/user_cleaners.rb | 12 -
spec/models/teachable_parser_spec.rb | 2 +
spec/models/user_cleaner_spec.rb | 293 +++++++++++++++++-
spec/rails_helper.rb | 4 +
25 files changed, 552 insertions(+), 150 deletions(-)
create mode 100644 app/mailers/user_cleaner_mailer.rb
create mode 100644 app/views/layouts/warning_mail_layout.html.erb
delete mode 100644 app/views/mathi_mailer/ghost_email.html.erb
create mode 100644 app/views/user_cleaner_mailer/deletion_email.html.erb
create mode 100644 app/views/user_cleaner_mailer/pending_deletion_email.html.erb
create mode 100644 app/workers/user_cleaner.rb
delete mode 100644 app/workers/user_cleaner_job.rb
create mode 100644 config/initializers/after_sign_in.rb
create mode 100644 db/migrate/20240530200000_add_deletion_date_to_user.rb
create mode 100644 db/migrate/20240605200000_remove_ghost_hash_from_user.rb
delete mode 100644 spec/factories/user_cleaners.rb
diff --git a/app/mailers/mathi_mailer.rb b/app/mailers/mathi_mailer.rb
index de9a71172..f53efa572 100644
--- a/app/mailers/mathi_mailer.rb
+++ b/app/mailers/mathi_mailer.rb
@@ -2,14 +2,6 @@ class MathiMailer < ApplicationMailer
default from: DefaultSetting::PROJECT_EMAIL
layout false
- def ghost_email(user)
- return if user.ghost_hash.nil?
-
- @name = user.name
- @hash = user.ghost_hash
- mail(to: user.email, subject: t("mailer.hash_mail_subject"))
- end
-
def data_request_email(user)
@mail = user.email
@id = user.id
diff --git a/app/mailers/user_cleaner_mailer.rb b/app/mailers/user_cleaner_mailer.rb
new file mode 100644
index 000000000..f8535ccc2
--- /dev/null
+++ b/app/mailers/user_cleaner_mailer.rb
@@ -0,0 +1,28 @@
+class UserCleanerMailer < ApplicationMailer
+ layout "warning_mail_layout"
+
+ # Creates an email to inform a user that their account will be deleted.
+ #
+ # @param [Integer] num_days_until_deletion:
+ # The number of days until the account will be deleted.
+ def pending_deletion_email(num_days_until_deletion)
+ user = params[:user]
+ sender = "#{t("mailer.warning")} <#{DefaultSetting::PROJECT_EMAIL}>"
+ I18n.locale = user.locale
+
+ @num_days_until_deletion = num_days_until_deletion
+ subject = t("mailer.pending_deletion_subject",
+ num_days_until_deletion: @num_days_until_deletion)
+ mail(from: sender, to: user.email, subject: subject, priority: "high")
+ end
+
+ # Creates an email to inform a user that their account has been deleted.
+ def deletion_email
+ user = params[:user]
+ sender = "#{t("mailer.warning")} <#{DefaultSetting::PROJECT_EMAIL}>"
+ I18n.locale = user.locale
+
+ subject = t("mailer.deletion_subject")
+ mail(from: sender, to: user.email, subject: subject, priority: "high")
+ end
+end
diff --git a/app/models/user_cleaner.rb b/app/models/user_cleaner.rb
index 8a5763e53..a70bb8f7f 100644
--- a/app/models/user_cleaner.rb
+++ b/app/models/user_cleaner.rb
@@ -1,125 +1,140 @@
-# PORO class that removes users with inactive emails
+# Deletes inactive users from the database.
+# See [1] for a description of how the flow works on a high level.
+#
+# Users have a deletion_date field that is nil by default. It is set to a future
+# date if the user has been inactive for too long (i.e. hasn't logged in).
+# Before the deletion date is reached, we send warning mails. If users log in
+# before the deletion date, that date is reset to nil such that the user is not
+# deleted. If the user is still inactive on the deletion date, the user is
+# ultimately deleted.
+#
+# [1] https://github.com/MaMpf-HD/mampf/issues/410#issuecomment-2136875776
class UserCleaner
- attr_accessor :imap, :email_dict, :hash_dict
-
- def login
- @imap = Net::IMAP.new(ENV.fetch("IMAPSERVER"), port: 993, ssl: true)
- @imap.authenticate("LOGIN", ENV.fetch("PROJECT_EMAIL_USERNAME"),
- ENV.fetch("PROJECT_EMAIL_PASSWORD"))
+ # The maximum number of users that can be deleted in one run.
+ # This is equivalent to the maximum of number of deletion dates set in one run.
+ #
+ # This flag can be used to prevent too many mails from being sent at once.
+ # Keep in mind that the mail server also handles other mails, e.g. notification
+ # mails etc., so we might want to set the limit very low here such that our
+ # mail server is not marked as "spam server".
+ #
+ # Note that this is just a soft limit, i.e. the actual number of deletion
+ # warning mails sent on a given day might be higher than this number:
+ # - If on a given day the cronjob is not run (for whatever reason),
+ # we have more users with a deletion date lying in the past than
+ # MAX_DELETIONS_PER_RUN. However, we don't send an additional mail once
+ # the user is deleted, so this shouldn't be a problem.
+ # - Despite that there cannot be more than MAX_DELETIONS_PER_RUN users with the
+ # same deletion date, warning mails might be sent on the same date to users
+ # with varying deletion dates, since the 40-, 14-, 7- and 2-day warning mails
+ # can overlap temporally.
+ MAX_DELETIONS_PER_RUN = ENV.fetch("MAX_DELETIONS_PER_RUN").to_i
+
+ # The threshold for inactive users. Users who have not logged in for this time
+ # are considered inactive.
+ INACTIVE_USER_THRESHOLD = 6.months
+
+ # 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
+ # the case for users who have never logged in since PR #553 was merged.
+ def inactive_users
+ User.where(last_sign_in_at: ...INACTIVE_USER_THRESHOLD.ago)
+ .or(User.where(last_sign_in_at: nil))
end
- def logout
- @imap.logout
+ # 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..)
end
- def search_emails_and_hashes
- @email_dict = {}
- @hash_dict = {}
- @imap.examine(ENV.fetch("PROJECT_EMAIL_MAILBOX"))
- # Mails containing multiple email addresses (Subject: "Undelivered Mail Returned to Sender")
- @imap.search(["SUBJECT",
- "Undelivered Mail Returned to Sender"]).each do |message_id|
- body = @imap.fetch(message_id,
- "BODY[TEXT]")[0].attr["BODY[TEXT]"].squeeze(" ")
- # rubocop:disable Layout/LineLength
- next unless (match = body.scan(/([a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,4})[\s\S]*?([a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,4})[\s\S]*?User has moved to ERROR: Account expired/))
- # rubocop:enable Layout/LineLength
-
- match = match.flatten.uniq
- match.each do |email|
- add_mail(email, message_id)
-
- try_get_hash(body, email)
- end
- end
- # Mails containing single email addresses (Subject: "Delivery Status Notification (Failure)")
- # define array containing all used regex patterns
- patterns = [
- '([a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,4})>[\s\S]*?Unknown recipient',
- # rubocop:disable Layout/LineLength
- '([a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,4})>[\s\S]*?User unknown in virtual mailbox table'
- # rubocop:enable Layout/LineLength
- ]
-
- @imap.search(["SUBJECT",
- "Delivery Status Notification (Failure)"]).each do |message_id|
- body = @imap.fetch(message_id,
- "BODY[TEXT]")[0].attr["BODY[TEXT]"].squeeze(" ")
- patterns.each do |pattern|
- next unless (match = body.scan(/#{pattern}/))
-
- match = match.flatten.uniq
- match.each do |email|
- add_mail(email, message_id)
-
- try_get_hash(body, email)
- end
+ # Sets the deletion date for inactive users and sends an initial warning mail.
+ #
+ # This method finds all inactive users whose deletion date is nil (not set yet)
+ # and updates their deletion date to be 40 days from the current date.
+ #
+ # The maximum number of deletion dates set in one run is limited by
+ # MAX_DELETIONS_PER_RUN.
+ def set_deletion_date_for_inactive_users
+ inactive_users.where(deletion_date: nil)
+ .limit(MAX_DELETIONS_PER_RUN)
+ .find_each do |user|
+ user.update(deletion_date: Date.current + 40.days)
+
+ if user.generic? # rubocop:disable Style/IfUnlessModifier
+ UserCleanerMailer.with(user: user).pending_deletion_email(40).deliver_later
end
end
end
- def add_mail(email, message_id)
- @email_dict = {} if @email_dict.blank?
- if @email_dict.key?(email)
- @email_dict[email] << message_id
- else
- @email_dict[email] = [message_id]
- end
+ # Unsets the deletion date for users who have been active recently.
+ #
+ # This method finds all users whose deletion date is set and unsets it if the
+ # user has been active recently.
+ #
+ # Note that this method just serves as a safety measure. The deletion date
+ # should be unset after every successful user sign-in, see the Warden callback
+ # in `config/initializers/after_sign_in.rb`. If for some reason, the callback
+ # does not work, this method will prevent active users from being deleted
+ # as a last resort.
+ def unset_deletion_date_for_recently_active_users
+ active_users.where.not(deletion_date: nil).update(deletion_date: nil)
end
- def try_get_hash(body, email)
- @hash_dict = {} if @hash_dict.blank?
- begin
- hash = body.match(/Hash:([a-zA-Z0-9]*)/).captures
- @hash_dict[email] = hash
- rescue StandardError
- nil
+ # Deletes all users whose deletion date is in the past or present.
+ #
+ # Technically, there should never be users with a deletion date in the past
+ # since the cron job is run daily and should delete users on the day of their
+ # deletion date. Should the cron job not run for some reason, we also delete
+ # users with a deletion date in the past via this method.
+ #
+ # The deletion date for the users must have been set beforehand by calling
+ # `set_deletion_date_for_inactive_users`.
+ #
+ # Even after having called this method, there might still exist users with a
+ # deletion date in the future, as we only delete generic users.
+ def delete_users_according_to_deletion_date!
+ num_deleted_users = 0
+
+ User.where(deletion_date: ..Date.current).find_each do |user|
+ next unless user.generic?
+
+ UserCleanerMailer.with(user: user).deletion_email.deliver_later
+ user.destroy
+ num_deleted_users += 1
end
- end
- def send_hashes
- @emails = @email_dict.keys
- @users = User.where(email: @emails)
-
- @users.each do |user|
- user.update(ghost_hash: Digest::SHA256.hexdigest(Time.now.to_i.to_s))
- MathiMailer.ghost_email(user).deliver_now
- move_mail(@email_dict[user])
- end
+ Rails.logger.info("UserCleaner deleted #{num_deleted_users} stale users")
end
- def delete_ghosts
- # @hash_dict.each do |mail, hash|
- # u = User.find_by(email: mail, ghost_hash: hash)
- # move_mail(@email_dict[mail]) if u.present? && @email_dict.present?
- # u.destroy! if u&.generic?
- # end
- end
-
- def move_mail(message_ids, attempt = 0)
- return if message_ids.blank?
+ # Sends additional warning mails to users whose deletion date is near.
+ #
+ # In addition to the initial warning mail 40 days before deletion, this method
+ # sends warning mails 14, 7 and 2 days before the account is deleted.
+ def send_additional_warning_mails
+ User.where.not(deletion_date: nil).find_each do |user|
+ next unless user.generic?
- message_ids = Array(message_ids)
- return if attempt > 3
+ num_days_until_deletion = (user.deletion_date - Date.current).to_i
- begin
- @imap.examine(ENV.fetch("PROJECT_EMAIL_MAILBOX"))
- @imap.move(message_ids, "Other Users/mampf/handled_bounces")
- rescue Net::IMAP::BadResponseError
- move_mail(message_ids, attempt + 1)
+ if [14, 7, 2].include?(num_days_until_deletion)
+ UserCleanerMailer.with(user: user)
+ .pending_deletion_email(num_days_until_deletion)
+ .deliver_later
+ end
end
end
- def clean!
- # TODO: Implement new user cleaner logic
- # login
- # search_emails_and_hashes
- # return if @email_dict.blank?
+ # Handles inactive users according to the deletion policy documented
+ # in the UserCleaner class description. Brief: users that haven't logged in
+ # to MaMpf for too long will be deleted.
+ def handle_inactive_users!
+ set_deletion_date_for_inactive_users
+ unset_deletion_date_for_recently_active_users
+ delete_users_according_to_deletion_date!
- # send_hashes
- # sleep(10)
- # search_emails_and_hashes
- # delete_ghosts
- # logout
+ send_additional_warning_mails
end
end
diff --git a/app/views/layouts/warning_mail_layout.html.erb b/app/views/layouts/warning_mail_layout.html.erb
new file mode 100644
index 000000000..9ae0d3135
--- /dev/null
+++ b/app/views/layouts/warning_mail_layout.html.erb
@@ -0,0 +1,18 @@
+
+
+
+
+
+ <%= stylesheet_link_tag :email %>
+
+
+
+
diff --git a/app/workers/user_cleaner.rb b/app/workers/user_cleaner.rb
new file mode 100644
index 000000000..184a21f5d
--- /dev/null
+++ b/app/workers/user_cleaner.rb
@@ -0,0 +1,13 @@
+class UserCleanerJob
+ include Sidekiq::Worker
+
+ def perform
+ # Only run this job in production, not for mampf-experimental or mampf-dev.
+ # Note that Rails.env.production? is not sufficient in this context
+ # as both mampf-experimental and mampf-dev also run in production mode.
+ production_name = ENV.fetch("PRODUCTION_NAME", nil)
+ return unless production_name == "mampf"
+
+ UserCleaner.new.handle_inactive_users!
+ end
+end
diff --git a/app/workers/user_cleaner_job.rb b/app/workers/user_cleaner_job.rb
deleted file mode 100644
index 4c20eb572..000000000
--- a/app/workers/user_cleaner_job.rb
+++ /dev/null
@@ -1,8 +0,0 @@
-class UserCleanerJob
- include Sidekiq::Worker
-
- def perform
- user_cleaner = UserCleaner.new
- user_cleaner.clean!
- end
-end
diff --git a/config/environments/production.rb b/config/environments/production.rb
index 18e070a38..7f0f4cec4 100644
--- a/config/environments/production.rb
+++ b/config/environments/production.rb
@@ -78,4 +78,9 @@
# Do not dump schema after migrations.
config.active_record.dump_schema_after_migration = false
+
+ config.after_initialize do
+ production_name = ENV.fetch("PRODUCTION_NAME", nil)
+ Rails.logger.info("PRODUCTION_NAME: #{production_name}")
+ end
end
diff --git a/config/initializers/after_sign_in.rb b/config/initializers/after_sign_in.rb
new file mode 100644
index 000000000..8d6a8c694
--- /dev/null
+++ b/config/initializers/after_sign_in.rb
@@ -0,0 +1,10 @@
+# Callback documentation for Warden (used by Devise):
+# https://github.com/wardencommunity/warden/wiki/Callbacks#after_authentication
+Warden::Manager.after_authentication do |user, _auth, _opts|
+ # User might have not logged in for a long time, which is why they have a
+ # deletion date set. If the user logs in, we unset this date to prevent the
+ # user from being deleted. See the UserCleaner class for more information.
+ # In case this callback does not work, a safety net is provided by the method
+ # `unset_deletion_date_for_recently_active_users` in the UserCleaner class.
+ user.deletion_date = nil
+end
diff --git a/config/locales/de.yml b/config/locales/de.yml
index 7bf8811e8..fa4875bc7 100644
--- a/config/locales/de.yml
+++ b/config/locales/de.yml
@@ -3053,6 +3053,22 @@ de:
en: 'Englisch'
de: 'Deutsch'
mailer:
+ warning: "MaMpf Warning"
+ pending_deletion_subject: "%{num_days_until_deletion} Tage bis zur
+ Löschung Deines MaMpf-Accounts"
+ deletion_subject: "Account gelöscht"
+ pending_deletion_body_html: >
+ Wir haben festgestellt, dass Du Dich in den letzten 6 Monaten nicht in
+ MaMpf eingeloggt hast. Dein Account wird in %{num_days_until_deletion}
+ Tagen gelöscht, sofern Du Dich nicht vorher einloggst.
+ Bitte beachte, dass nach der Löschung Deines Accounts alle bei MaMpf
+ gespeicherten Daten von Dir unwiderruflich verloren gehen.
+ deletion_body_html: >
+ Dein MaMpf-Account wurde vollständig gelöscht, da Du Dich seit
+ mehr als 6 Monaten nicht eingeloggt hast. Beachte, dass wir zuvor
+ mehrere Erinnerungen gesendet haben. Alle Deine bei MaMpf gespeicherten
+ Daten sind unwiderruflich verloren gegangen. Vielen Dank, dass Du MaMpf
+ genutzt hast.
notification: 'MaMpf-Benachrichtigung'
notification_header: 'Benachrichtigung von MaMpf'
medium_subject: 'Neues Medium'
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 6fae4ba62..454915f45 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -2871,6 +2871,21 @@ en:
no_zip: This file is not a .zip archive.
to_big: This file is too big.
mailer:
+ warning: "MaMpf Warning"
+ pending_deletion_subject: "%{num_days_until_deletion} days before your
+ account will be deleted"
+ deletion_subject: "Account deleted"
+ pending_deletion_body_html: >
+ We've noticed that you haven't logged in to your MaMpf account in the last
+ 6 months. Your account will be deleted in %{num_days_until_deletion} days
+ unless you log in to your MaMpf account before then.
+ Please note that once your account is deleted, all of your data stored
+ at MaMpf will be permanently lost.
+ deletion_body_html: >
+ Your MaMpf account has been deleted entirely as you have not
+ logged in for more than 6 months. Note that we've sent several
+ reminders beforehand. All of your data stored at MaMpf has been
+ permanently lost. Thanks for using MaMpf.
notification: 'MaMpf notification'
notification_header: 'MaMpf notification'
medium_subject: 'New Medium'
diff --git a/config/schedule.yml b/config/schedule.yml
index f2018526a..5421af058 100644
--- a/config/schedule.yml
+++ b/config/schedule.yml
@@ -14,6 +14,9 @@ cache_cleaner_job:
queue: critical
user_cleaner_job:
- cron: "0 4 * * 1,4"
+ # at 4:00 AM, every day
+ # This job has to run every day such that users will receive
+ # respective warning emails (about their accounts being deleted) on time.
+ cron: "0 4 * * *"
class: "UserCleanerJob"
- queue: critical
\ No newline at end of file
+ queue: critical
diff --git a/db/migrate/20240530200000_add_deletion_date_to_user.rb b/db/migrate/20240530200000_add_deletion_date_to_user.rb
new file mode 100644
index 000000000..cf95a22d8
--- /dev/null
+++ b/db/migrate/20240530200000_add_deletion_date_to_user.rb
@@ -0,0 +1,5 @@
+class AddDeletionDateToUser < ActiveRecord::Migration[7.1]
+ def change
+ add_column :users, :deletion_date, :date, null: true, default: nil
+ end
+end
diff --git a/db/migrate/20240605200000_remove_ghost_hash_from_user.rb b/db/migrate/20240605200000_remove_ghost_hash_from_user.rb
new file mode 100644
index 000000000..609d74045
--- /dev/null
+++ b/db/migrate/20240605200000_remove_ghost_hash_from_user.rb
@@ -0,0 +1,5 @@
+class RemoveGhostHashFromUser < ActiveRecord::Migration[7.1]
+ def change
+ remove_column :users, :ghost_hash, :string
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 6cd1b3f61..9f40b301b 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.
-ActiveRecord::Schema[7.1].define(version: 2024_04_22_200000) do
+ActiveRecord::Schema[7.1].define(version: 2024_06_05_200000) do
# These are extensions that must be enabled in order to support this database
enable_extension "pgcrypto"
enable_extension "plpgsql"
@@ -875,12 +875,12 @@
t.boolean "archived"
t.datetime "locked_at", precision: nil
t.text "image_data"
- t.string "ghost_hash"
t.integer "sign_in_count", default: 0, null: false
t.datetime "current_sign_in_at"
t.datetime "last_sign_in_at"
t.string "current_sign_in_ip"
t.string "last_sign_in_ip"
+ t.date "deletion_date"
t.index ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true
t.index ["email"], name: "index_users_on_email", unique: true
t.index ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true
diff --git a/docker/development/docker-compose.yml b/docker/development/docker-compose.yml
index 94e3b7591..103ffb436 100644
--- a/docker/development/docker-compose.yml
+++ b/docker/development/docker-compose.yml
@@ -85,6 +85,7 @@ services:
TEST_DATABASE_USERNAME: mampf
TEST_DATABASE_HOST: db
TEST_DATABASE_PORT: 5432
+ MAX_DELETIONS_PER_RUN: 50
MAILSERVER: mailcatcher
FROM_ADDRESS: development@localhost
URL_HOST: localhost
diff --git a/docker/production/docker-compose.production.yml b/docker/production/docker-compose.production.yml
index 8b4a8e55b..3a3ed0b36 100644
--- a/docker/production/docker-compose.production.yml
+++ b/docker/production/docker-compose.production.yml
@@ -4,6 +4,8 @@ x-mampf:
context: https://github.com/MaMpf-HD/mampf.git#main
dockerfile: docker/production/Dockerfile
env_file: docker.env
+ environment:
+ PRODUCTION_NAME:
networks:
- solr
- mampf
diff --git a/docker/production/docker.env b/docker/production/docker.env
index 886040d36..c24b29c93 100644
--- a/docker/production/docker.env
+++ b/docker/production/docker.env
@@ -11,6 +11,9 @@ MUESLI_SERVER=https://muesli.mathi.uni-heidelberg.de
ERDBEERE_API=https://erdbeere.mathi.uni-heidelberg.de/api/v1
MEMCACHED_SERVER=cache
+# User Cleaner
+MAX_DELETIONS_PER_RUN=50
+
# Email
FROM_ADDRESS=mampf@mathi.uni-heidelberg.de
MAILSERVER=mail.mathi.uni-heidelberg.de
diff --git a/docker/test/docker-compose.yml b/docker/test/docker-compose.yml
index d795b4ad3..204712069 100644
--- a/docker/test/docker-compose.yml
+++ b/docker/test/docker-compose.yml
@@ -47,6 +47,7 @@ services:
TEST_DATABASE_USERNAME: mampf
TEST_DATABASE_HOST: db
TEST_DATABASE_PORT: 5432
+ MAX_DELETIONS_PER_RUN: 50
MAILSERVER: mailcatcher
FROM_ADDRESS: development@localhost
URL_HOST: mampf
diff --git a/spec/factories/user_cleaners.rb b/spec/factories/user_cleaners.rb
deleted file mode 100644
index 8180e5f24..000000000
--- a/spec/factories/user_cleaners.rb
+++ /dev/null
@@ -1,12 +0,0 @@
-FactoryBot.define do
- factory :user_cleaner do
- # trait with user
- trait :with_hashed_user do
- after(:build) do |c|
- user = FactoryBot.create(:confirmed_user, email: Faker::Internet.email)
- user.update(ghost_hash: Digest::SHA256.hexdigest(Time.now.to_i.to_s))
- c.hash_dict = { user.email => user.ghost_hash }
- end
- end
- end
-end
diff --git a/spec/models/teachable_parser_spec.rb b/spec/models/teachable_parser_spec.rb
index d021b5c17..9c3031464 100644
--- a/spec/models/teachable_parser_spec.rb
+++ b/spec/models/teachable_parser_spec.rb
@@ -1,3 +1,5 @@
+require "rails_helper"
+
RSpec.describe(TeachableParser, type: :model) do
it "has a valid factory" do
expect(FactoryBot.build(:teachable_parser)).to be_kind_of(TeachableParser)
diff --git a/spec/models/user_cleaner_spec.rb b/spec/models/user_cleaner_spec.rb
index d33724a05..689962e3e 100644
--- a/spec/models/user_cleaner_spec.rb
+++ b/spec/models/user_cleaner_spec.rb
@@ -1,19 +1,282 @@
require "rails_helper"
RSpec.describe(UserCleaner, type: :model) do
- it "has a factory" do
- expect(FactoryBot.build(:user_cleaner))
- .to be_kind_of(UserCleaner)
- end
- # Right now, delete_ghosts is commented out in user_cleaner.rb
- # as we want to avoid accidental deletion of users in production.
- # We will come up with a new strategy for cleaning up old users.
- #
- # it "can destroy users" do
- # n_users = User.all.size
- # u = FactoryBot.build(:user_cleaner, :with_hashed_user)
- # expect(User.all.size).to eq(n_users + 1)
- # u.delete_ghosts
- # expect(User.all.size).to eq(n_users)
- # end
+ # 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)
+ end
+
+ let(:user_teacher) do
+ user_teacher = FactoryBot.create(: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)
+ FactoryBot.create(:lecture, editors: [user_editor])
+ return user_editor
+ end
+
+ before(:each) do
+ UserCleaner::INACTIVE_USER_THRESHOLD = 6.months
+ 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
+
+ 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)
+ 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)
+ 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)
+
+ UserCleaner.new.set_deletion_date_for_inactive_users
+ inactive_user.reload
+ active_user.reload
+
+ expect(inactive_user.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)
+
+ UserCleaner.new.set_deletion_date_for_inactive_users
+
+ users_flagged = User.where(deletion_date: Date.current + 40.days)
+ expect(users_flagged.count).to eq(max_deletions)
+ end
+ end
+
+ 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)
+
+ UserCleaner.new.set_deletion_date_for_inactive_users
+ user.reload
+
+ expect(user.deletion_date).to eq(Date.current + 42.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)
+
+ UserCleaner.new.unset_deletion_date_for_recently_active_users
+ user_inactive.reload
+ user_inactive2.reload
+ user_active.reload
+
+ expect(user_inactive.deletion_date).to eq(deletion_date)
+ expect(user_inactive2.deletion_date).to eq(deletion_date)
+ expect(user_active.deletion_date).to be_nil
+ end
+ end
+
+ 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)
+
+ UserCleaner.new.delete_users_according_to_deletion_date!
+
+ expect(User.where(id: user_past1.id)).not_to exist
+ expect(User.where(id: user_past2.id)).not_to exist
+ expect(User.where(id: user_present.id)).not_to exist
+ 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)
+
+ UserCleaner.new.delete_users_according_to_deletion_date!
+
+ expect(User.where(id: user_future1.id)).to exist
+ expect(User.where(id: user_future2.id)).to exist
+ end
+
+ it "does not delete users without a deletion date" do
+ user = FactoryBot.create(: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_admin
+ user_teacher
+ user_editor
+
+ UserCleaner.new.delete_users_according_to_deletion_date!
+
+ expect(User.where(id: user_generic.id)).not_to exist
+ expect(User.where(id: user_admin.id)).to exist
+ expect(User.where(id: user_teacher.id)).to exist
+ expect(User.where(id: user_editor.id)).to exist
+ end
+ end
+
+ 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)
+
+ expect do
+ UserCleaner.new.set_deletion_date_for_inactive_users
+ end.to have_enqueued_mail(UserCleanerMailer, :pending_deletion_email)
+ .with(a_hash_including(args: [40]))
+ end
+
+ it "does not enqueue a deletion warning mail (40 days) for non-generic users" do
+ user_admin
+ user_teacher
+ user_editor
+
+ expect do
+ UserCleaner.new.set_deletion_date_for_inactive_users
+ end.not_to have_enqueued_mail(UserCleanerMailer, :pending_deletion_email)
+ end
+ end
+
+ 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)
+
+ expect do
+ UserCleaner.new.send_additional_warning_mails
+ end.to have_enqueued_mail(UserCleanerMailer, :pending_deletion_email)
+ .with(a_hash_including(args: [num_days]))
+ end
+
+ it "enqueues additional deletion warning mails" do
+ test_enqueues_additional_deletion_warning_mails(14)
+ test_enqueues_additional_deletion_warning_mails(7)
+ test_enqueues_additional_deletion_warning_mails(2)
+ end
+
+ it "does not enqueue an additional deletion warning mail for 40 days" do
+ FactoryBot.create(:user, deletion_date: Date.current + 40.days)
+
+ expect do
+ UserCleaner.new.send_additional_warning_mails
+ end.not_to have_enqueued_mail(UserCleanerMailer, :pending_deletion_email)
+ end
+
+ it "does not enqueue additional deletion warning mails for non-generic users" do
+ user_admin.update(deletion_date: Date.current + 14.days)
+ user_teacher.update(deletion_date: Date.current + 7.days)
+ user_editor.update(deletion_date: Date.current + 2.days)
+
+ expect do
+ UserCleaner.new.send_additional_warning_mails
+ end.not_to have_enqueued_mail(UserCleanerMailer, :pending_deletion_email)
+ end
+ end
+
+ context "when a user is finally deleted" do
+ it "enqueues a deletion mail" do
+ FactoryBot.create(:user, deletion_date: Date.current - 1.day)
+
+ expect do
+ UserCleaner.new.delete_users_according_to_deletion_date!
+ end.to have_enqueued_mail(UserCleanerMailer, :deletion_email)
+ end
+ end
+ end
+
+ describe("#pending_deletion_mail") do
+ let(:user_de) { FactoryBot.create(:user, locale: "de") }
+ let(:user_en) { FactoryBot.create(:user, locale: "en") }
+
+ def test_subject_line(num_days)
+ user = FactoryBot.create(:user)
+ mailer = UserCleanerMailer.with(user: user).pending_deletion_email(num_days)
+ expect(mailer.subject).to include(num_days.to_s)
+ end
+
+ it "has mail subject containing the number of days until deletion" do
+ test_subject_line(40)
+ test_subject_line(14)
+ test_subject_line(7)
+ test_subject_line(2)
+ end
+
+ it "has mail subject localized to the user's locale" do
+ mailer = UserCleanerMailer.with(user: user_de).pending_deletion_email(40)
+ expect(mailer.subject).to include("Tage")
+ expect(mailer.subject).not_to include("days")
+
+ mailer = UserCleanerMailer.with(user: user_en).pending_deletion_email(40)
+ expect(mailer.subject).to include("days")
+ expect(mailer.subject).not_to include("Tage")
+ end
+
+ it "has mail body localized to the user's locale" do
+ expected_de = "verloren"
+ expected_en = "lost"
+
+ mailer = UserCleanerMailer.with(user: user_de).pending_deletion_email(40)
+ expect(mailer.html_part.body).to include(expected_de)
+ expect(mailer.html_part.body).not_to include(expected_en)
+
+ mailer = UserCleanerMailer.with(user: user_en).pending_deletion_email(40)
+ expect(mailer.html_part.body).to include(expected_en)
+ expect(mailer.html_part.body).not_to include(expected_de)
+ end
+ end
+
+ describe("#deletion_mail") do
+ let(:user_de) { FactoryBot.create(:user, locale: "de") }
+ let(:user_en) { FactoryBot.create(:user, locale: "en") }
+
+ it "has mail subject localized to the user's locale" do
+ mailer = UserCleanerMailer.with(user: user_de).deletion_email
+ expect(mailer.subject).to include("gelöscht")
+ expect(mailer.subject).not_to include("deleted")
+
+ mailer = UserCleanerMailer.with(user: user_en).deletion_email
+ expect(mailer.subject).to include("deleted")
+ expect(mailer.subject).not_to include("gelöscht")
+ end
+
+ it "has mail body localized to the user's locale" do
+ expected_de = "vollständig gelöscht"
+ expected_en = "deleted entirely"
+
+ mailer = UserCleanerMailer.with(user: user_de).deletion_email
+
+ expect(mailer.html_part.body).to include(expected_de)
+ expect(mailer.html_part.body).not_to include(expected_en)
+
+ mailer = UserCleanerMailer.with(user: user_en).deletion_email
+ expect(mailer.html_part.body).to include(expected_en)
+ expect(mailer.html_part.body).not_to include(expected_de)
+ end
+ end
end
diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb
index 202207bd1..57c66cd8d 100644
--- a/spec/rails_helper.rb
+++ b/spec/rails_helper.rb
@@ -35,6 +35,10 @@
# For Devise >= 4.1.0
config.include Devise::Test::ControllerHelpers, type: :controller
+ # e.g. make have_enqueued_mail matchers available
+ # see https://stackoverflow.com/a/57077395/
+ config.include ActiveJob::TestHelper
+
# If you're not using ActiveRecord, or you'd prefer not to run each of your
# examples within a transaction, remove the following line or assign false
# instead of true.