Improving documentation and refactor (#21)

* Fix rubocop

* Improve installation instructions (#16)

* Fix compat range (#17)

* Fix compat range

* Fix offenses

* Fix lint

* Fix tests

* Fix/no email (#18)

* Fix rubocop

* Don't query deleted users

* Don't query empty email users

* Bump gem to prepare release

* refactor module, use warning timestamps

* lint

* Update lib/decidim/cleaner.rb

Co-authored-by: Armand Fardeau <armandfardeau@users.noreply.github.com>

* Update lib/decidim/cleaner.rb

Co-authored-by: Armand Fardeau <armandfardeau@users.noreply.github.com>

* Update lib/decidim/cleaner.rb

Co-authored-by: Armand Fardeau <armandfardeau@users.noreply.github.com>

* add notes for env variables in readme

* rollback config in ENV

* Revert "rollback config in ENV"

This reverts commit d074afc.

---------

Co-authored-by: armandfardeau <fardeauarmand@gmail.com>
Co-authored-by: Armand Fardeau <armandfardeau@users.noreply.github.com>

README.md

@@ -22,6 +22,19 @@ bundle exec rails decidim_cleaner:install:migrations
 bundle exec rails db:migrate
 ```
 
+You can then modify the default values of the cleaner in your .ENV with the following variables:
+
+```bash
+# Delay until a user is considered inactive and receive a warning email (in days)
+DECIDIM_CLEANER_INACTIVE_USERS_MAIL=
+
+# Delay until a user is deleted after receiving an email (in days)
+DECIDIM_CLEANER_DELETE_INACTIVE_USERS=
+
+# Delay until an admin log is deleted (in days)
+DECIDIM_CLEANER_DELETE_ADMIN_LOGS=
+```
+
 ### Sidekiq Scheduler
 [Further documentation](https://github.com/sidekiq-scheduler/sidekiq-scheduler)
 

app/helpers/decidim/cleaner/delays_helper.rb

@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+module Decidim
+  module Cleaner
+    module DelaysHelper
+      def email_inactive_after(organization)
+        organization.delete_inactive_users_email_after || Decidim::Cleaner.delete_inactive_users_email_after
+      end
+
+      def delete_inactive_after(organization)
+        organization.delete_inactive_users_after || Decidim::Cleaner.delete_inactive_users_after
+      end
+    end
+  end
+end

app/jobs/decidim/cleaner/clean_admin_logs_job.rb

@@ -10,10 +10,16 @@ def perform
           next unless organization.delete_admin_logs?
 
           Decidim::ActionLog.where(organization:)
-                            .where("created_at < ?", Time.zone.now - (organization.delete_admin_logs_after || 365).days)
+                            .where("created_at < ?", delete_admin_logs_before_date(organization))
                             .delete_all
         end
       end
+
+      private
+
+      def delete_admin_logs_before_date(organization)
+        Time.zone.now - (organization.delete_admin_logs_after || Decidim::Cleaner.delete_admin_logs_after).days
+      end
     end
   end
 end

app/jobs/decidim/cleaner/clean_inactive_users_job.rb

@@ -12,32 +12,48 @@ def perform
           send_warning(Decidim::User.where(organization:)
                                     .not_deleted
                                     .where.not(email: "")
-                                    .where("last_sign_in_at < ?", Time.zone.now - (organization.delete_inactive_users_email_after || 365).days)
-                                    .where("last_sign_in_at > ?", Time.zone.now - (organization.delete_inactive_users_email_after || 365).days - 1.day))
-
+                                    .where("last_sign_in_at < ?", email_inactive_before_date(organization)))
           delete_user_and_send_email(Decidim::User.where(organization:)
                                                   .not_deleted
                                                   .where.not(email: "")
-                                                  .where("last_sign_in_at < ?", Time.zone.now - (organization.delete_inactive_users_after || 390).days))
+                                                  .where("warning_date < ?", delete_inactive_before_date(organization)))
         end
       end
 
       def send_warning(users)
         users.find_each do |user|
-          InactiveUsersMailer.warning_inactive(user).deliver_now
+          next if user.warning_date.present?
+
+          user.update!(warning_date: Time.zone.now) if InactiveUsersMailer.warning_inactive(user).deliver_now
           Rails.logger.info "Inactive warning sent to #{user.email}"
         end
       end
 
       def delete_user_and_send_email(users)
         users.find_each do |user|
+          if user.last_sign_in_at > user.warning_date
+            user.update!(warning_date: nil)
+            Rails.logger.info "User with id #{user.id} has logged in again, warning date reset"
+            next
+          end
+
           InactiveUsersMailer.warning_deletion(user).deliver_now
           Rails.logger.info "Deletion warning sent to #{user.email}"
 
           Decidim::DestroyAccount.call(user, Decidim::DeleteAccountForm.from_params({ delete_reason: I18n.t("decidim.cleaner.delete_reason") }))
           Rails.logger.info "User with id #{user.id} destroyed"
         end
       end
+
+      private
+
+      def email_inactive_before_date(organization)
+        Time.zone.now - (organization.delete_inactive_users_email_after || Decidim::Cleaner.delete_inactive_users_email_after).days
+      end
+
+      def delete_inactive_before_date(organization)
+        Time.zone.now - (organization.delete_inactive_users_after || Decidim::Cleaner.delete_inactive_users_after).days
+      end
     end
   end
 end

app/mailers/decidim/cleaner/inactive_users_mailer.rb

@@ -5,6 +5,7 @@ module Cleaner
     # A custom mailer for Decidim so we can notify users
     # when their account was blocked
     class InactiveUsersMailer < Decidim::ApplicationMailer
+      helper Decidim::Cleaner::DelaysHelper
       def warning_inactive(user)
         with_user(user) do
           @user = user

app/views/decidim/cleaner/inactive_users_mailer/warning_deletion.html.erb

@@ -1,6 +1,6 @@
 <p class="email-greeting"><%= t ".hello" %></p>
 
-<p class="email-instructions"><%= t(".body_1", organization_name: h(@organization.name), organization_url: decidim.root_url(host: @organization.host), days: @organization.delete_inactive_users_after||390).html_safe %></p>
+<p class="email-instructions"><%= t(".body_1", organization_name: h(@organization.name), organization_url: decidim.root_url(host: @organization.host), days: email_inactive_after(@organization) + delete_inactive_after(@organization)).html_safe %></p>
 
 <p class="email-instructions"><%= t ".body_2" %></p>
 

app/views/decidim/cleaner/inactive_users_mailer/warning_inactive.html.erb

@@ -1,7 +1,7 @@
 <p class="email-greeting"><%= t ".hello" %></p>
 
-<p class="email-instructions"><%= t ".body_1", organization_name: h(@organization.name), days: @organization.delete_inactive_users_email_after %></p>
+<p class="email-instructions"><%= t ".body_1", organization_name: h(@organization.name), days: email_inactive_after(@organization) %></p>
 
-<p class="email-instructions"><%= t(".body_2", remaining_days: (@organization.delete_inactive_users_after||390) - (@organization.delete_inactive_users_email_after||365), organization_url: decidim.root_url(host: @organization.host)).html_safe %></p>
+<p class="email-instructions"><%= t(".body_2", remaining_days: delete_inactive_after(@organization), organization_url: decidim.root_url(host: @organization.host)).html_safe %></p>
 
 <p class="email-closing"><%= t(".greetings", organization_name: h(@organization.name), organization_url: decidim.root_url(host: @organization.host)).html_safe %></p>

config/locales/en.yml

@@ -6,7 +6,8 @@ en:
         delete_admin_logs: Enable admin logs deletion
         delete_admin_logs_after: Delete admin logs after (days, default 365)
         delete_inactive_users: Enable inactive users deletion
-        delete_inactive_users_after: Delete inactive users after (days, default 390)
+        delete_inactive_users_after: Delete inactive users x days after the mail(days,
+          default 30)
         delete_inactive_users_email_after: Send email to inactive users before deletion
           (days, default 365)
   decidim:

config/locales/fr.yml

@@ -7,8 +7,8 @@ fr:
         delete_admin_logs_after: Supprimer l'historique d'administration après (jours,
           par défaut 365)
         delete_inactive_users: Activer la suppression des utilisateurs inactifs
-        delete_inactive_users_after: Supprimer les utilisateurs inactifs après (jours,
-          par défaut 390)
+        delete_inactive_users_after: Supprimer les utilisateurs inactifs au bout de
+          x jours après le courriel (jours, par défaut 30)
         delete_inactive_users_email_after: Envoyer un courriel aux utilisateurs inactifs
           avant la suppression (jours, par défaut 365)
   decidim:

db/migrate/20230328094652_add_warning_date_to_users.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class AddWarningDateToUsers < ActiveRecord::Migration[6.1]
+  def change
+    add_column :decidim_users, :warning_date, :datetime
+  end
+end

lib/decidim/cleaner.rb

@@ -7,5 +7,18 @@
 module Decidim
   # This namespace holds the logic of the `Cleaner` module.
   module Cleaner
+    include ActiveSupport::Configurable
+
+    config_accessor :delete_admin_logs_after do
+      ENV.fetch("DECIDIM_CLEANER_DELETE_ADMIN_LOGS", "365").to_i
+    end
+
+    config_accessor :delete_inactive_users_after do
+      ENV.fetch("DECIDIM_CLEANER_DELETE_INACTIVE_USERS", "30").to_i
+    end
+
+    config_accessor :delete_inactive_users_email_after do
+      ENV.fetch("DECIDIM_CLEANER_INACTIVE_USERS_MAIL", "365").to_i
+    end
   end
 end

spec/jobs/decidim/cleaner/clean_inactive_users_job_spec.rb

@@ -6,9 +6,9 @@
   subject { described_class }
 
   context "when the delay is specified" do
-    let!(:organization) { create(:organization, delete_inactive_users: true, delete_inactive_users_email_after: 25, delete_inactive_users_after: 30) }
-    let!(:pending_user) { create(:user, organization:, last_sign_in_at: 25.days.ago - 1.hour) }
-    let!(:inactive_user) { create(:user, organization:, last_sign_in_at: 50.days.ago) }
+    let!(:organization) { create(:organization, delete_inactive_users: true, delete_inactive_users_email_after: 25, delete_inactive_users_after: 5) }
+    let!(:pending_user) { create(:user, organization:, last_sign_in_at: 27.days.ago) }
+    let!(:inactive_user) { create(:user, organization:, last_sign_in_at: 35.days.ago, warning_date: 10.days.ago) }
     let!(:user) { create(:user, organization:) }
 
     it "enqueues job in queue 'cleaner'" do
@@ -32,8 +32,8 @@
     end
 
     context "when users have destroyed his/her account" do
-      let!(:pending_user) { create(:user, :deleted, organization:, last_sign_in_at: 25.days.ago - 1.hour) }
-      let!(:inactive_user) { create(:user, :deleted, organization:, last_sign_in_at: 50.days.ago) }
+      let!(:pending_user) { create(:user, :deleted, organization:, last_sign_in_at: 27.days.ago) }
+      let!(:inactive_user) { create(:user, :deleted, organization:, last_sign_in_at: 35.days.ago, warning_date: 10.days.ago) }
 
       it "doesn't send email" do
         expect(Decidim::Cleaner::InactiveUsersMailer).not_to receive(:warning_inactive).with(pending_user).and_call_original
@@ -51,5 +51,27 @@
         expect(user.reload).not_to be_deleted
       end
     end
+
+    context "when user reconnect after warning" do
+      let!(:inactive_user) { create(:user, organization:, last_sign_in_at: 7.days.ago, warning_date: 10.days.ago) }
+
+      it "doesn't send email" do
+        expect(Decidim::Cleaner::InactiveUsersMailer).not_to receive(:warning_deletion).with(inactive_user).and_call_original
+
+        subject.perform_now
+      end
+
+      it "doesn't destroy user" do
+        subject.perform_now
+
+        expect(inactive_user.reload).not_to be_deleted
+      end
+
+      it "reset warning date" do
+        subject.perform_now
+
+        expect(inactive_user.reload.warning_date).to be_nil
+      end
+    end
   end
 end

spec/system/admin_manages_organization_cleaning_spec.rb

@@ -18,13 +18,13 @@
       expect(page).to have_content("Enable admin logs deletion")
       expect(page).to have_content("Delete admin logs after")
       expect(page).to have_content("Enable inactive users deletion")
-      expect(page).to have_content("Delete inactive users after")
+      expect(page).to have_content("Delete inactive users x days after")
       expect(page).to have_content("Send email to inactive users before deletion")
 
       find(:css, "input[name='organization[delete_admin_logs]'").set(true)
       fill_in "Delete admin logs after", with: 365
       find(:css, "input[name='organization[delete_inactive_users]'").set(true)
-      fill_in "Delete inactive users after", with: 390
+      fill_in "Delete inactive users x days after", with: 30
       fill_in "Send email to inactive users before deletion", with: 365
 
       click_button "Update"