LG-8738: Handle unexpected API responses when checking enrollment status#7743
Conversation
test for unexpected number of days until expiration
tomas-nava
changed the title
eileen-nava
left a comment
eileen-nava
left a comment
There was a problem hiding this comment.
I am approving this but want to note that I didn't test it locally.
| enrollment.update(deadline_passed_sent: true) | ||
| end | ||
|
|
||
| # check for an unexpected number of days until expiration |
There was a problem hiding this comment.
TY for this helpful comment. 🙏🏻
|
|
||
| expect(job_analytics).to have_logged_event( | ||
| 'GetUspsProofingResultsJob: Enrollment status updated', | ||
| hash_including(reason: 'Enrollment has expired'), |
JackRyan1989
left a comment
JackRyan1989
left a comment
There was a problem hiding this comment.
Nice! Very clear. I don't have any suggestions or changes. Tests pass locally and no other issues noticed when running locally.
| expired_after_days = response_message&.match(IPP_EXPIRED_ERROR_MESSAGE)&.[](1) | ||
| if expired_after_days.present? && |
There was a problem hiding this comment.
maybe a good use case for named captures?
if the regex was
IPP_EXPIRED_ERROR_MESSAGE = /More than (?<days>\d+) days have passed since opt-in to IPP/then this could be
| expired_after_days = response_message&.match(IPP_EXPIRED_ERROR_MESSAGE)&.[](1) | |
| if expired_after_days.present? && | |
| expired_after_days = response_message&.match(IPP_EXPIRED_ERROR_MESSAGE)&.[](:days) | |
| if expired_after_days.present? && |
or maybe getting rid of &.[] syntax
| expired_after_days = response_message&.match(IPP_EXPIRED_ERROR_MESSAGE)&.[](1) | |
| if expired_after_days.present? && | |
| match = response_message&.match(IPP_EXPIRED_ERROR_MESSAGE) | |
| expired_after_days = match && match[:days] | |
| if expired_after_days.present? && |
There was a problem hiding this comment.
yeah, I like that, changed in a307a61
Do you prefer match && match[:days] to match&.[](:days) because the former is more human readable?
There was a problem hiding this comment.
Yeah personally, I don't like the amount of visual noise combining nil-safe navigator (.&) with brackets [] resulting in &.[]()
| def handle_unexpected_response(enrollment, response_message, reason:, cancel: true) | ||
| enrollment.cancelled! if cancel | ||
|
|
||
| analytics(user: enrollment.user). | ||
| idv_in_person_usps_proofing_results_job_unexpected_response( | ||
| enrollment_code: enrollment.enrollment_code, | ||
| enrollment_id: enrollment.id, | ||
| response_message: response_message, | ||
| reason: reason, | ||
| ) | ||
| end |
There was a problem hiding this comment.
Do we have a specific plan for how we can be notified of these issues and respond?
There was a problem hiding this comment.
no, not yet. I'm going to confirm that they work in lower environments, then I'll probably write up a ticket for alerting.
tomas-nava
deleted the
tomas/lg-8738-handle-applicant-does-not-exist-errors-from-usps
branch
5 participants
🎫 Ticket
closes LG-8738 and LG-8737
🛠 Summary of changes
Handles three types of unexpected error responses when checking enrollment status that we've seen in non-production environments:
This PR catches and logs those errors.