Skip to content

Put the entry for the gpo reminder job back#9099

Merged
jmax-gsa merged 6 commits intomainfrom
jmax/LG-9565-re-enable-reminder-letter-job
Sep 1, 2023
Merged

Put the entry for the gpo reminder job back#9099
jmax-gsa merged 6 commits intomainfrom
jmax/LG-9565-re-enable-reminder-letter-job

Conversation

@jmax-gsa
Copy link
Contributor

@jmax-gsa jmax-gsa commented Aug 28, 2023

🎫 Ticket

LG-9565

🛠 Summary of changes

The initial work on LG-9565 had an incorrect DB query. Hooper pushed a patch to disable the job, and I corrected the query. This PR is to re-enable the job after the updated query is merged.

@jmax-gsa jmax-gsa requested review from a team and jmhooper August 28, 2023 16:31
@jmax-gsa jmax-gsa marked this pull request as ready for review August 28, 2023 16:31
@jmax-gsa jmax-gsa force-pushed the jmax/LG-9565-re-enable-reminder-letter-job branch from e292390 to d4035fc Compare August 28, 2023 17:13
@jmhooper
Copy link
Contributor

This one looks pretty straightforward. Let's manually run that GPO confirmation code query to make sure we get the number of reminder emails we expect before we merge this.

If you don't have the perms to run that lmk and I'm happy to help once #9080 lands

@jmhooper
Copy link
Contributor

@jmax-gsa and I just test drove the query from that branch. It appears to work as expected, however we observed it timing out occasionally with the default timeout. I am going to suggest making the query in a transaction with a longer timeout so that our job does not fail looking for profiles.

Here is an example of what that pattern looks like form the report job:

def self.transaction_with_timeout(rails_env = Rails.env)

changelog: Upcoming Features, USPS verification, re-enable reminder job.
@jmax-gsa jmax-gsa force-pushed the jmax/LG-9565-re-enable-reminder-letter-job branch from d4035fc to ccd0a53 Compare August 30, 2023 18:28
Moved `profiles_due_for_reminder` out to its own method and set up a
constant for the database timeout.
Copy link
Contributor

@jmhooper jmhooper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with one small nit about the timeout

@jmax-gsa jmax-gsa merged commit bd306f4 into main Sep 1, 2023
@jmax-gsa jmax-gsa deleted the jmax/LG-9565-re-enable-reminder-letter-job branch September 1, 2023 19:03
@@ -1,16 +1,11 @@
class GpoReminderSender
LOCAL_DATABASE_TIMEOUT = 60_000
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like the kind of thing better suited to be a config value than a constant, IMO, because the size of the tables, etc changes over time and having to do a code deploy to change that is not optimal

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants