From 67c535a5eab61d6285aa78d72dda77eb8b7b32ce Mon Sep 17 00:00:00 2001 From: koetsier Date: Mon, 18 Nov 2024 12:15:45 +0000 Subject: [PATCH] Update delete user code and specs Make the code shorter and clearer and improve tests --- lib/gdpr/gateway/user_details.rb | 108 ++++++---------- lib/wifi_user/user.rb | 2 +- spec/lib/followups/followup_sender_spec.rb | 2 - spec/lib/gdpr/gateway/user_details_spec.rb | 140 ++++++++++++--------- spec/spec_helper.rb | 1 + 5 files changed, 123 insertions(+), 130 deletions(-) diff --git a/lib/gdpr/gateway/user_details.rb b/lib/gdpr/gateway/user_details.rb index 1609ef15..9a47c752 100644 --- a/lib/gdpr/gateway/user_details.rb +++ b/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}") diff --git a/lib/wifi_user/user.rb b/lib/wifi_user/user.rb index c21b1f38..5cd85828 100644 --- a/lib/wifi_user/user.rb +++ b/lib/wifi_user/user.rb @@ -14,7 +14,7 @@ def before_create end def mobile? - contact.start_with? "+" + contact&.start_with? "+" end private diff --git a/spec/lib/followups/followup_sender_spec.rb b/spec/lib/followups/followup_sender_spec.rb index c9dd9c1f..49616141 100644 --- a/spec/lib/followups/followup_sender_spec.rb +++ b/spec/lib/followups/followup_sender_spec.rb @@ -1,5 +1,3 @@ -require "timecop" - describe Followups::FollowupSender do include_context "fake notify" diff --git a/spec/lib/gdpr/gateway/user_details_spec.rb b/spec/lib/gdpr/gateway/user_details_spec.rb index 8aebd152..f995cfb0 100644 --- a/spec/lib/gdpr/gateway/user_details_spec.rb +++ b/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,14 +22,19 @@ 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: "sally@gov.uk", 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 @@ -33,40 +42,44 @@ 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: "george@gov.uk", last_login: Date.today - 367) - user_details.insert(username: "Tony", contact: "tony@example.com", last_login: Date.today - 367) + FactoryBot.create(:user_details, contact: "do_not_delete_1@gov.uk", last_login: Date.today - 364) + FactoryBot.create(:user_details, contact: "do_not_delete_2@gov.uk", last_login: Date.today - 365) + FactoryBot.create(:user_details, contact: "delete@gov.uk", 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[do_not_delete_1@gov.uk do_not_delete_2@gov.uk]) 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: "george@gov.uk", 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: "george@gov.uk", 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: "bob@gov.uk", created_at: Date.today) - user_details.insert(username: "sally", contact: "sally@gov.uk", 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: "bob@gov.uk", created_at: Date.today - 370) - user_details.insert(username: "george", contact: "george@gov.uk", 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[do_not_delete_1@gov.uk do_not_delete_2@gov.uk]) 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: "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 + + 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: "Kate@digital.cabinet-office.gov.uk", 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: "sim@gov.uk", 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: "Kate@digital.cabinet-office.gov.uk", + 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: "sim@gov.uk") - 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 diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 4d540b65..6b1f4207 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -3,6 +3,7 @@ require "rspec" require "simplecov" require "sequel" +require "timecop" require "webmock/rspec" require "shared"