Conversation
co-authored by: Jonathan Hooper <jonathan.hooper@gsa.gov> Changelog: Bug Fixes,Verify by Mail,Fixed database query for reminder letters
jmax-gsa
force-pushed
the
jmax/LG-9565-gpo-reminder-fixed-query
branch
from
4918695 to
f3858c0
Compare
soniaconnolly
left a comment
soniaconnolly
left a comment
There was a problem hiding this comment.
LGTM, and I'm deferring to @jmhooper to make sure we didn't miss any more edge cases.
app/services/gpo_reminder_sender.rb
Outdated
| gpo_verification_pending_at: ..for_letters_sent_before, | ||
| gpo_verification_pending_at: letter_eligible_range, | ||
| gpo_confirmation_codes: { reminder_sent_at: nil }, | ||
| deactivation_reason: nil, |
There was a problem hiding this comment.
I think this will skip users that are also going through in person verification. If we wait to merge until the transition to in_person_verification_pending_at is complete, this will work as intended. Otherwise we need to include deactivation_reason: in_person_verification_pending.
There was a problem hiding this comment.
Yeah, otherwise we need to add some clauses here to filter on deactivation_reason being nil or deactivation_reason being the in-person pending enum.
There was a problem hiding this comment.
Adding a test case to catch this and code to handle it.
There was a problem hiding this comment.
I looked for a case where the deactivation reason is set to the in-person pending enum but couldn't find it. Where should I be looking for that one?
There was a problem hiding this comment.
Nevermind, I see now. The with_pending_in_person_enrollment on the user factory creates a profile with the deactivation reason.
| profiles_due_for_reminder = Profile.joins(:gpo_confirmation_codes). | ||
| where( | ||
| gpo_verification_pending_at: ..for_letters_sent_before, | ||
| gpo_verification_pending_at: letter_eligible_range, |
There was a problem hiding this comment.
I think this should work. I can't figure out a way that we would send a letter to user who is not GPO eligible. If a case does exist I'm pretty confident it is a rare corner case.
A user who is in the in-person flow can also request a GPO letter. They should get GPO reminder letters.
3 participants
🎫 Ticket
LG-9565
🛠 Summary of changes
The original query to retrieve users who need a reminder letter did not respect the 30-day cutoff (we shouldn't remind users if their request for a letter is older than 30 days).
📜 Testing Plan
Provide a checklist of steps to confirm the changes.
GpoReminderJob.new.perform(14.days.ago))