From 912ceeae8150af9549ecbd21bfdf2c551e4f8161 Mon Sep 17 00:00:00 2001 From: Matt Hinz Date: Tue, 14 Nov 2023 11:43:28 -0800 Subject: [PATCH 1/3] Refine GPO expiration job + rake task - Make the rake task invoke the job's .perform method directly - Note earliest + latest timestamps found in rake task output - Add configurable statement timeout to job [skip changelog] --- app/jobs/gpo_expiration_job.rb | 63 +++++++++++++++++++------- lib/tasks/backfill_gpo_expiration.rake | 34 +++++++------- spec/jobs/gpo_expiration_job_spec.rb | 24 ++++++++++ 3 files changed, 86 insertions(+), 35 deletions(-) diff --git a/app/jobs/gpo_expiration_job.rb b/app/jobs/gpo_expiration_job.rb index 6e9bc06d0c3..c06dc47ca11 100644 --- a/app/jobs/gpo_expiration_job.rb +++ b/app/jobs/gpo_expiration_job.rb @@ -1,32 +1,39 @@ class GpoExpirationJob < ApplicationJob queue_as :low - def initialize(analytics: nil) + def initialize(analytics: nil, on_profile_expired: nil) @analytics = analytics + @on_profile_expired = on_profile_expired end - def expire_profile(profile:) - gpo_verification_pending_at = profile.gpo_verification_pending_at - - profile.deactivate_due_to_gpo_expiration - - analytics.idv_gpo_expired( - user_id: profile.user.uuid, - user_has_active_profile: profile.user.active_profile.present?, - letters_sent: profile.gpo_confirmation_codes.count, - gpo_verification_pending_at: gpo_verification_pending_at, - ) - end - - def perform(now: Time.zone.now, limit: nil, min_profile_age: nil) + def perform( + dry_run: false, + limit: nil, + min_profile_age: nil, + now: Time.zone.now, + statement_timeout: 10.minutes + ) profiles = gpo_profiles_that_should_be_expired(as_of: now, min_profile_age: min_profile_age) if limit.present? profiles = profiles.limit(limit) end - profiles.find_each do |profile| - expire_profile(profile: profile) + with_statement_timeout(statement_timeout) do + profiles.find_each do |profile| + gpo_verification_pending_at = profile.gpo_verification_pending_at + + if gpo_verification_pending_at.blank? + raise "Profile #{profile.id} does not have gpo_verification_pending_at" + end + + expire_profile(profile: profile) unless dry_run + + on_profile_expired&.call( + profile: profile, + gpo_verification_pending_at: gpo_verification_pending_at, + ) + end end end @@ -40,6 +47,28 @@ def gpo_profiles_that_should_be_expired(as_of:, min_profile_age: nil) private + attr_reader :on_profile_expired + + def expire_profile(profile:) + gpo_verification_pending_at = profile.gpo_verification_pending_at + + profile.deactivate_due_to_gpo_expiration + + analytics.idv_gpo_expired( + user_id: profile.user.uuid, + user_has_active_profile: profile.user.active_profile.present?, + letters_sent: profile.gpo_confirmation_codes.count, + gpo_verification_pending_at: gpo_verification_pending_at, + ) + end + + def with_statement_timeout(timeout) + ActiveRecord::Base.transaction do + ActiveRecord::Base.connection.execute("SET LOCAL statement_timeout = '#{timeout.seconds}s'") + yield + end + end + def analytics @analytics ||= Analytics.new(user: AnonymousUser.new, request: nil, session: {}, sp: nil) end diff --git a/lib/tasks/backfill_gpo_expiration.rake b/lib/tasks/backfill_gpo_expiration.rake index 63a73505e7f..6950e217e6c 100644 --- a/lib/tasks/backfill_gpo_expiration.rake +++ b/lib/tasks/backfill_gpo_expiration.rake @@ -14,33 +14,31 @@ namespace :profiles do min_profile_age = (ENV['MIN_PROFILE_AGE_IN_DAYS'].to_i || 100).days update_profiles = ENV['UPDATE_PROFILES'] == 'true' - job = GpoExpirationJob.new - - profiles = job.gpo_profiles_that_should_be_expired( - as_of: Time.zone.now, - min_profile_age: min_profile_age, - ) - count = 0 + earliest = nil + latest = nil - profiles.find_each do |profile| + on_profile_expired = ->(profile:, gpo_verification_pending_at:) do count += 1 - gpo_verification_pending_at = profile.gpo_verification_pending_at - - if gpo_verification_pending_at.blank? - raise "Profile #{profile.id} does not have gpo_verification_pending_at" - end + earliest = [earliest, gpo_verification_pending_at].compact.min + latest = [latest, gpo_verification_pending_at].compact.max puts "#{profile.id},#{gpo_verification_pending_at.iso8601}" - if update_profiles - warn "Expired #{count} profiles" if count % 100 == 0 - job.expire_profile(profile: profile) - elsif count % 100 == 0 - warn "Found #{count} profiles" + if count % 100 == 0 + verb = update_profiles ? 'Expired' : 'Found' + warn "#{verb} #{count} profiles (earliest: #{earliest}, latest: #{latest})" end end + + job = GpoExpirationJob.new(on_profile_expired: on_profile_expired) + + job.perform( + now: Time.zone.now, + min_profile_age: min_profile_age, + dry_run: !update_profiles, + ) end ## diff --git a/spec/jobs/gpo_expiration_job_spec.rb b/spec/jobs/gpo_expiration_job_spec.rb index f92c499437a..c1e841eb2dd 100644 --- a/spec/jobs/gpo_expiration_job_spec.rb +++ b/spec/jobs/gpo_expiration_job_spec.rb @@ -113,6 +113,30 @@ ) end + context 'when a callback is provided' do + it 'calls it for expired profiles' do + profile = users[:user_with_one_expired_gpo_profile].reload.gpo_verification_pending_profile + gpo_verification_pending_at = profile.gpo_verification_pending_at + + on_profile_expired = spy + expect(on_profile_expired).to receive(:call).with( + profile: profile, + gpo_verification_pending_at: gpo_verification_pending_at, + ) + + job = described_class.new(analytics: analytics, on_profile_expired: on_profile_expired) + job.perform + end + end + + context('when dry_run is specified') do + it 'does not write changes' do + job.perform(dry_run: true) + profile = users[:user_with_one_expired_gpo_profile].reload.gpo_verification_pending_profile + expect(profile.gpo_verification_expired_at).to eql(nil) + end + end + context 'when the user has an active profile' do let!(:active_profile) do create(:profile, :active, user: users[:user_with_one_expired_gpo_profile]) From 08f15be0dd0f6f329b3acd0da4a0175c64a95818 Mon Sep 17 00:00:00 2001 From: Matt Hinz Date: Tue, 14 Nov 2023 11:59:22 -0800 Subject: [PATCH 2/3] Allow configuring statement timeout via ENV --- lib/tasks/backfill_gpo_expiration.rake | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/tasks/backfill_gpo_expiration.rake b/lib/tasks/backfill_gpo_expiration.rake index 6950e217e6c..59d1db6f9d0 100644 --- a/lib/tasks/backfill_gpo_expiration.rake +++ b/lib/tasks/backfill_gpo_expiration.rake @@ -13,6 +13,7 @@ namespace :profiles do task backfill_gpo_expiration: :environment do |_task, _args| min_profile_age = (ENV['MIN_PROFILE_AGE_IN_DAYS'].to_i || 100).days update_profiles = ENV['UPDATE_PROFILES'] == 'true' + statement_timeout = ENV['STATEMENT_TIMEOUT_IN_SECONDS'].to_i.seconds || 10.minutes count = 0 earliest = nil @@ -38,6 +39,7 @@ namespace :profiles do now: Time.zone.now, min_profile_age: min_profile_age, dry_run: !update_profiles, + statement_timeout: statement_timeout, ) end From 986bb18a853849dc80b187df4fa676344c6d30e1 Mon Sep 17 00:00:00 2001 From: Matt Hinz Date: Tue, 14 Nov 2023 13:12:05 -0800 Subject: [PATCH 3/3] Don't allow sql injection (Fix brakeman warning) --- app/jobs/gpo_expiration_job.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/jobs/gpo_expiration_job.rb b/app/jobs/gpo_expiration_job.rb index c06dc47ca11..5403d9008e7 100644 --- a/app/jobs/gpo_expiration_job.rb +++ b/app/jobs/gpo_expiration_job.rb @@ -64,7 +64,10 @@ def expire_profile(profile:) def with_statement_timeout(timeout) ActiveRecord::Base.transaction do - ActiveRecord::Base.connection.execute("SET LOCAL statement_timeout = '#{timeout.seconds}s'") + quoted_timeout = ActiveRecord::Base.connection.quote("#{timeout.seconds}s") + ActiveRecord::Base.connection.execute( + "SET LOCAL statement_timeout = #{quoted_timeout}", + ) yield end end