Merge pull request #870 from alphagov/delete_users

Update delete user code and specs

lib/gdpr/gateway/user_details.rb

@@ -1,96 +1,77 @@
 require "logger"
 
 class Gdpr::Gateway::Userdetails
-  SESSION_BATCH_SIZE = 500
-  DAYS_IN_YEAR = 365
-  DAYS_IN_11_MONTHS = 335 # Approximation of 11 months
+  SESSION_BATCH_SIZE = 50
+  TIME_TO_DELETE = "1 YEAR".freeze
+  TIME_TO_NOTIFY = "11 MONTH".freeze
+  HEALTH_USER = "HEALTH".freeze
 
   def initialize
     @logger = Logger.new($stdout)
   end
 
   def delete_inactive_users
-    @logger.info("Finding users that have been inactive for 12 months")
-    inactive_users = find_inactive_users(DAYS_IN_YEAR)
-
-    @logger.info("Found #{inactive_users.size} users that have been inactive for 12 months")
-
-    @logger.info("Notifying users they have been removed from GovWifi")
-
-    inactive_users.each do |user|
-      username = user[:username]
-      contact = user[:contact]
-
-      begin
-        if user.mobile?
-          WifiUser::SMSSender.send_user_account_removed(username, contact)
-        else
-          WifiUser::EmailSender.send_user_account_removed(username, contact)
-        end
-      rescue Notifications::Client::BadRequestError => e
-        handle_email_error(e, username, contact)
-      rescue StandardError => e
-        @logger.warn(e.message)
-      end
-    end
-
     @logger.info("Starting daily old user deletion")
+    now = Time.now.strftime("%Y-%m-%d %H:%M:%S")
 
-    total = 0
-    loop do
-      deleted_rows = DB[:userdetails].with_sql_delete("
-        DELETE FROM userdetails WHERE (last_login < DATE_SUB(NOW(), INTERVAL #{DAYS_IN_YEAR} DAY)
-        OR (last_login IS NULL AND created_at < DATE_SUB(NOW(), INTERVAL #{DAYS_IN_YEAR} DAY)))
-        AND username != 'HEALTH'
-        LIMIT #{SESSION_BATCH_SIZE}")
-      total += deleted_rows
+    sql = Sequel.lit("((last_login < DATE_SUB(?, INTERVAL #{TIME_TO_DELETE})) OR
+                       (last_login IS NULL AND created_at < DATE_SUB(?, INTERVAL #{TIME_TO_DELETE}))) AND
+                       username != ?", now, now, HEALTH_USER)
 
-      break if deleted_rows.zero?
+    total = 0
+    while (inactive_users = WifiUser::User.where(sql).limit(SESSION_BATCH_SIZE)).count.positive?
+      send_delete_emails(inactive_users)
+      total += inactive_users.delete
     end
-
     @logger.info("Finished daily old user deletion, #{total} rows affected")
   end
 
   def obfuscate_sponsors
+    @logger.info("Starting sponsor obfuscation")
     DB.run("
       UPDATE userdetails ud1
       LEFT JOIN userdetails as ud2 ON ud1.sponsor = ud2.contact
       SET ud1.sponsor = REPLACE(ud1.sponsor, SUBSTRING_INDEX(ud1.sponsor, '@', '1'), 'user')
       WHERE ud2.username IS NULL
     ")
+    @logger.info("Sponsor obfuscation complete")
   end
 
   def notify_inactive_users
     @logger.info("Starting notification process for users inactive for 11 months")
-    inactive_users = find_inactive_users_for_exact_number_of_days(DAYS_IN_11_MONTHS)
+    now = Time.now.strftime("%Y-%m-%d %H:%M:%S")
 
-    @logger.info("Found #{inactive_users.size} inactive users")
+    sql = Sequel.lit("((DATE(last_login) = DATE(DATE_SUB(?, INTERVAL #{TIME_TO_NOTIFY}))) OR
+                       (last_login IS NULL AND DATE(created_at) = DATE(DATE_SUB(?, INTERVAL #{TIME_TO_NOTIFY})))) AND
+                       username != ? AND contact LIKE '%@%'", now, now, HEALTH_USER)
 
-    inactive_users.each do |user|
-      contact = user[:contact]
-      username = user[:username]
-
-      if user.mobile?
-        WifiUser::SMSSender.send_credentials_expiring_notification(username, contact)
-      elsif user.valid_email?(contact)
-        begin
-          WifiUser::EmailSender.send_credentials_expiring_notification(username, contact)
-          @logger.info("Email sent to #{username} at #{contact}")
-        rescue Notifications::Client::BadRequestError => e
-          handle_email_error(e, username, contact)
-        rescue StandardError => e
-          @logger.warn(e.message)
-        end
-      else
-        @logger.warn("Invalid contact for user #{username}")
-      end
-    end
+    inactive_users = WifiUser::User.where(sql)
+    inactive_users.each { |user| send_notify_email(user) }
 
-    @logger.info("Finished notification process for users inactive for for 11 months")
+    @logger.info("Notified #{inactive_users.count} users")
   end
 
 private
 
+  def send_delete_emails(inactive_users)
+    inactive_users.reject(&:mobile?).each do |user|
+      WifiUser::EmailSender.send_user_account_removed(user.username, user.contact)
+    rescue Notifications::Client::BadRequestError => e
+      handle_email_error(e, username, contact)
+    rescue StandardError => e
+      @logger.warn(e.message)
+    end
+  end
+
+  def send_notify_email(user)
+    WifiUser::EmailSender.send_credentials_expiring_notification(user.username, user.contact)
+    @logger.info("Email sent to #{user.username} at #{user.contact}")
+  rescue Notifications::Client::BadRequestError => e
+    handle_email_error(e, username, contact)
+  rescue StandardError => e
+    @logger.warn(e.message)
+  end
+
   def find_inactive_users(days)
     WifiUser::User.where(
       Sequel.lit("
@@ -100,15 +81,6 @@ def find_inactive_users(days)
     ).all
   end
 
-  def find_inactive_users_for_exact_number_of_days(days)
-    WifiUser::User.where(
-      Sequel.lit("
-      (DATE(last_login) = DATE_SUB(CURDATE(), INTERVAL ? DAY)
-      OR (last_login IS NULL AND DATE(created_at) = DATE_SUB(CURDATE(), INTERVAL ? DAY)))
-      AND username != 'HEALTH'", days, days),
-    ).all
-  end
-
   def handle_email_error(error, username, contact)
     if error.message.include? "ValidationError"
       @logger.warn("Failed to send email to #{username} at #{contact}: #{error.message}")

lib/wifi_user/user.rb

@@ -14,7 +14,7 @@ def before_create
   end
 
   def mobile?
-    contact.start_with? "+"
+    contact&.start_with? "+"
   end
 
 private

spec/lib/followups/followup_sender_spec.rb

@@ -1,5 +1,3 @@
-require "timecop"
-
 describe Followups::FollowupSender do
   include_context "fake notify"
 

spec/lib/gdpr/gateway/user_details_spec.rb

@@ -1,6 +1,10 @@
+# rubocop:disable Style/DateTime
+
 describe Gdpr::Gateway::Userdetails do
   include_context "fake notify"
-
+  let(:year) { 2024 }
+  let(:month) { 5 }
+  let(:day) { 10 }
   let(:templates) do
     [
       instance_double(Notifications::Client::Template, name: "credentials_expiring_notification_email", id: "credentials_expiring_notification_email_id"),
@@ -18,55 +22,64 @@
   before :each do
     ENV["DO_NOT_REPLY"] = "do_not_reply_email_template_id"
     user_details.delete
+    Timecop.freeze(Time.local(year, month, day, 18, 0, 0))
+  end
+
+  after do
+    Timecop.return
   end
 
   context "Deleting old users" do
     context "Based on last_login" do
       context "Given no inactive users" do
         before do
-          user_details.insert(username: "bob", contact: "+7391480025", last_login: Date.today)
-          user_details.insert(username: "sally", contact: "[email protected]", last_login: Date.today - 363)
+          FactoryBot.create(:user_details, :sms, last_login: Date.today - 364)
+          FactoryBot.create(:user_details, last_login: Date.today - 364)
         end
 
         it "does not delete any users" do
           expect { subject.delete_inactive_users }.not_to(change { user_details.count })
         end
       end
 
+      describe "Delete in batches" do
+        before do
+          FactoryBot.create_list(:user_details, 150, last_login: Date.today - 400)
+        end
+        it "deletes all users" do
+          expect { subject.delete_inactive_users }.to change { user_details.count }.from(150).to(0)
+        end
+        it "notifies all users" do
+          subject.delete_inactive_users
+          expect(notify_client).to have_received(:send_email).exactly(150).times
+        end
+      end
+
       context "Given inactive users" do
         before do
-          allow(Common::Gateway::S3ObjectFetcher).to receive(:allow_list_regexp).and_return(valid_email_regexp)
-          user_details.insert(username: "bob", contact: "+7391480025", last_login: Date.today)
-          user_details.insert(username: "sally", contact: "+7391488825", created_at: Date.today - 363)
-          user_details.insert(username: "george", contact: "[email protected]", last_login: Date.today - 367)
-          user_details.insert(username: "Tony", contact: "[email protected]", last_login: Date.today - 367)
+          FactoryBot.create(:user_details, contact: "[email protected]", last_login: Date.today - 364)
+          FactoryBot.create(:user_details, contact: "[email protected]", last_login: Date.today - 365)
+          FactoryBot.create(:user_details, contact: "[email protected]", last_login: Date.today - 366)
+          FactoryBot.create(:user_details, contact: "+001234567890", last_login: Date.today - 366)
 
           subject.delete_inactive_users
         end
 
-        it "deletes only the old user records" do
-          expect(user_details.select_map(:username)).to match_array(%w[bob sally])
+        it "deletes only user records that are at least a year old" do
+          expect(user_details.select_map(:contact)).to match_array(%w[[email protected] [email protected]])
         end
 
-        it "notifies the deleted user with a valid email" do
+        it "notifies the deleted user with an email" do
           expect(notify_client).to have_received(:send_email).with(
-            email_address: "george@gov.uk",
+            email_address: "delete@gov.uk",
             template_id: "user_account_removed_email_id",
             personalisation: { inactivity_period: "12 months" },
             email_reply_to_id: "do_not_reply_email_template_id",
           ).once
         end
-      end
-
-      context "Given multiple inactive users" do
-        before do
-          user_details.insert(username: "bob", contact: "+7391481225", last_login: Date.today - 367)
-          user_details.insert(username: "george", contact: "[email protected]", last_login: Date.today - 367)
-        end
 
-        it "deletes all the inactive users" do
-          subject.delete_inactive_users
-          expect(user_details.select_map(:username)).to be_empty
+        it "does not notify the deleted user with an sms" do
+          expect(notify_client).to_not have_received(:send_sms)
         end
       end
 
@@ -81,10 +94,10 @@
         end
       end
 
-      context "Sending an emails throws an exception" do
+      context "Sending emails throws an exception" do
         before :each do
           allow(Services.notify_client).to receive(:send_email).and_raise(UserSignupError)
-          user_details.insert(username: "george", contact: "[email protected]", last_login: Date.today - 367)
+          FactoryBot.create(:user_details, last_login: Date.today - 367)
         end
         it "still deletes the user" do
           expect { subject.delete_inactive_users }.to change { user_details.count }.by(-1)
@@ -96,37 +109,39 @@
     context "Based on created_at" do
       context "Given no inactive users" do
         before do
-          user_details.insert(username: "bob", contact: "[email protected]", created_at: Date.today)
-          user_details.insert(username: "sally", contact: "[email protected]", created_at: Date.today - 300)
+          FactoryBot.create(:user_details, :not_logged_in, created_at: Date.today)
+          FactoryBot.create(:user_details, :not_logged_in, created_at: Date.today - 300)
         end
 
         it "does not delete any user details" do
           expect { subject.delete_inactive_users }.not_to(change { user_details.count })
         end
       end
-
-      context "Given one inactive user" do
+      context "Given inactive users" do
         before do
-          user_details.insert(username: "bob", contact: "bob@gov.uk", created_at: Date.today)
-          user_details.insert(username: "sally", contact: "sally@gov.uk", created_at: Date.today - 300)
-          user_details.insert(username: "george", contact: "george@gov.uk", created_at: Date.today - 368)
-        end
+          FactoryBot.create(:user_details, :not_logged_in, contact: "do_not_delete_1@gov.uk", created_at: Date.today - 364)
+          FactoryBot.create(:user_details, :not_logged_in, contact: "do_not_delete_2@gov.uk", created_at: Date.today - 365)
+          FactoryBot.create(:user_details, :not_logged_in, contact: "delete@gov.uk", created_at: Date.today - 366)
+          FactoryBot.create(:user_details, :not_logged_in, contact: "+001234567890", created_at: Date.today - 366)
 
-        it "deletes only the old user record" do
           subject.delete_inactive_users
-          expect(user_details.all.map { |s| s.fetch(:username) }).to eq(%w[bob sally])
         end
-      end
 
-      context "Given multiple inactive users" do
-        before do
-          user_details.insert(username: "bob", contact: "[email protected]", created_at: Date.today - 370)
-          user_details.insert(username: "george", contact: "[email protected]", created_at: Date.today - 380)
+        it "deletes only user records that are at least a year old" do
+          expect(user_details.select_map(:contact)).to match_array(%w[[email protected] [email protected]])
         end
 
-        it "deletes all the inactive users" do
-          subject.delete_inactive_users
-          expect(user_details.all).to be_empty
+        it "notifies the deleted user with an email" do
+          expect(notify_client).to have_received(:send_email).with(
+            email_address: "[email protected]",
+            template_id: "user_account_removed_email_id",
+            personalisation: { inactivity_period: "12 months" },
+            email_reply_to_id: "do_not_reply_email_template_id",
+          ).once
+        end
+
+        it "does not notify the deleted user with an sms" do
+          expect(notify_client).to_not have_received(:send_sms)
         end
 
         context "Given the HEALTH user with an inactive created_at" do
@@ -200,35 +215,42 @@
   end
 
   context "Notifying inactive users" do
-    before do
-      user_details.insert(username: "Kate", contact: "[email protected]", last_login: Date.today - 335)
-      user_details.insert(username: "Rob", contact: "+07391491234", created_at: Date.today - 335)
-      user_details.insert(username: "Adam", contact: "+07391491678", last_login: Date.today)
-      user_details.insert(username: "Sim", contact: "[email protected]", last_login: Date.today)
-    end
-
     it "notifies inactive users" do
+      eleven_months_ago = DateTime.new(year, month, day, 13, 0, 0) << 11
+
+      email_user = FactoryBot.create(:user_details, last_login: eleven_months_ago)
+      FactoryBot.create(:user_details, :sms, last_login: eleven_months_ago)
+
       subject.notify_inactive_users
 
       expect(notify_client).to have_received(:send_email).with(
-        email_address: "[email protected]",
+        email_address: email_user.contact,
         template_id: "credentials_expiring_notification_email_id",
-        personalisation: { inactivity_period: "11 months", username: "Kate" },
+        personalisation: { inactivity_period: "11 months", username: email_user.username },
         email_reply_to_id: "do_not_reply_email_template_id",
       ).once
 
-      expect(notify_client).to have_received(:send_sms).with(
-        phone_number: "+07391491234",
-        template_id: "credentials_expiring_notification_sms_id",
-        personalisation: { inactivity_period: "11 months", username: "Rob" },
-      ).once
+      expect(notify_client).to_not have_received(:send_sms)
+    end
+
+    it "does not notify inactive users" do
+      FactoryBot.create(:user_details, last_login: nil)
+      subject.notify_inactive_users
+
+      expect(notify_client).not_to have_received(:send_email)
+      expect(notify_client).not_to have_received(:send_sms)
     end
 
-    it "does not notify active users" do
-      expect(notify_client).not_to receive(:send_email).with(email_address: "[email protected]")
-      expect(notify_client).not_to receive(:send_sms).with(phone_number: "+07391491678")
+    it "does not notify inactive users that are not exactly 11 months inactive" do
+      FactoryBot.create(:user_details, last_login: DateTime.new(year, month, day - 1, 13, 0, 0) << 11)
+      FactoryBot.create(:user_details, last_login: DateTime.new(year, month, day + 1, 13, 0, 0) << 11)
 
       subject.notify_inactive_users
+
+      expect(notify_client).not_to have_received(:send_email)
+      expect(notify_client).not_to have_received(:send_sms)
     end
   end
 end
+
+# rubocop:enable Style/DateTime