LG-10126: send sms notification for USPS proofing result #8772
LG-10126: send sms notification for USPS proofing result #8772dawei-nava merged 34 commits intomainfrom
Conversation
changelog: In-Person Proofing: send sms notification when proofing result is available.
changelog: User-Facing Improvements, In-Person Proofing, send sms notification when enrollment status is available.
…ification when enrollment status is available (LG-10126)
| end | ||
|
|
||
| def notification_message(enrollment:) | ||
| proof_date = enrollment.proofed_at ? enrollment.proofed_at.strftime('%m/%d/%Y') : 'NA' |
There was a problem hiding this comment.
should we define this date format in https://github.com/18F/identity-idp/blob/main/config/locales/time/en.yml#L29 and use I18n.l() for it?
dawei-nava
requested review from
a team and
JackRyan1989
and removed request for
JackRyan1989
| enrollment = InPersonEnrollment.find( | ||
| enrollment_id, | ||
| include: [:notification_phone_configuration, | ||
| :user], | ||
| ) |
There was a problem hiding this comment.
This can throw an exception and prevent the following analytics call from completing if it's related to a database issue.
There was a problem hiding this comment.
The exception issue here was not resolved by the changes.
| if !enrollment.notification_phone_configuration.present? || | ||
| (!enrollment.passed? && !enrollment.failed?) |
There was a problem hiding this comment.
It looks like we're excluding the expired status from consideration here. This means that we could end up retaining the phone configuration indefinitely, which is contrary to the stated goal of the story.
There was a problem hiding this comment.
Should cancelled be considered here too?
There was a problem hiding this comment.
@gina-yamada , cancelled seems not sending email in get_usps_proofing_results_job
There was a problem hiding this comment.
@dawei-nava - @gina-yamada makes a good point here. The issue isn't just whether we send a notification, but also whether we retain the phone number.
| if !enrollment.notification_phone_configuration.present? || | ||
| (!enrollment.passed? && !enrollment.failed?) |
There was a problem hiding this comment.
@dawei-nava - @gina-yamada makes a good point here. The issue isn't just whether we send a notification, but also whether we retain the phone number.
NavaTim
left a comment
NavaTim
left a comment
There was a problem hiding this comment.
Also regarding cancelled enrollments - since those aren't processed by the job, you may want to handle those with an ActiveRecord lifecycle callback instead.
dawei-nava
force-pushed
the
dwang/LG-10126-sms-notification-job
branch
from
2cef996 to
cc10444
Compare
| enrollment = InPersonEnrollment.find( | ||
| enrollment_id, | ||
| include: [:notification_phone_configuration, | ||
| :user], | ||
| ) |
There was a problem hiding this comment.
The exception issue here was not resolved by the changes.
| unless enrollment.present? | ||
| enrollment = InPersonEnrollment.find_by( | ||
| { id: enrollment_id }, | ||
| include: [:notification_phone_configuration, | ||
| :user], | ||
| ) | ||
| end | ||
| analytics(user: enrollment&.user). | ||
| idv_in_person_usps_proofing_results_notification_job_completed( | ||
| enrollment_code: enrollment.enrollment_code, enrollment_id: enrollment&.id, | ||
| ) |
There was a problem hiding this comment.
If the enrollment isn't present by this point, then we should log the completion with a note that an error occurred. We should also avoid trying to re-fetch the enrollment.
There was a problem hiding this comment.
@NavaTim, this is not a refetch, since it's in final ensure block, the enrollment may not be fetched if the flags are disabled(first two lines).
There was a problem hiding this comment.
If that's what you intend, then I recommend moving the find_by to the section where the flags are disabled. Also note that the accessor for enrollment_code here isn't using the nil-safe operator.
There was a problem hiding this comment.
@NavaTim , refactor it a little bit, logging error and use safe operator.
| if self.notification_sent_at && self.notification_phone_configuration | ||
| self.notification_phone_configuration.destroy | ||
| end | ||
| end |
There was a problem hiding this comment.
@dawei-nava You might want to check in with @night-jellyfish. I think delete might be handled in her issue? Check in with her maybe, I could be wrong. See her PR
Update: I typed this out before standup but my comment was marked as pending. I submitted feedback after standup so you probably just got notified. During stand up it sounds like you are aware that you may have some overlap depending on the situation/case
There was a problem hiding this comment.
@gina-yamada Only the cancellation is clearly handled in LG-10167 / main...brittany/lg-10167-delete-phone-no-when-enroll-cancelled; this or another ticket still needs to delete the phone number for other status changes.
NavaTim
left a comment
NavaTim
left a comment
There was a problem hiding this comment.
I highly recommend some cleanup, but this looks ok.
| if enrollment.expired? | ||
| # no sending message for expired status | ||
| enrollment.notification_phone_configuration&.destroy | ||
| return | ||
| end |
There was a problem hiding this comment.
Since we are now deleting expired enrollments' phone numbers in the proofing results job, this conditional is redundant.
There was a problem hiding this comment.
@NavaTim , i thought about it, since there may be a small chance to run the job manually with specific enrollment, i kept it here.
| # Track sms notification attempt | ||
| # @param [boolean] success sms notification successful or not | ||
| # @param [String] enrollment_code enrollment_code | ||
| # @param [String] enrollment_id enrollment_id | ||
| # @param [Hash] extra extra information | ||
| def idv_in_person_usps_proofing_results_notification_sent_attempted( | ||
| success:, | ||
| enrollment_code:, | ||
| enrollment_id:, | ||
| **extra | ||
| ) | ||
| track_event( | ||
| 'IdV: in person notification SMS send attempted', | ||
| success: success, | ||
| enrollment_code: enrollment_code, | ||
| enrollment_id: enrollment_id, | ||
| **extra, | ||
| ) | ||
| end |
There was a problem hiding this comment.
The telephony_result field isn't documented nor required here. Note that the docblock here will be used to generate documentation for analytics.
| end | ||
| end | ||
| context 'when failed to send notification' do | ||
| it 'logs sms send failre when number is opt out and enrollment not updated' do |
There was a problem hiding this comment.
| it 'logs sms send failre when number is opt out and enrollment not updated' do | |
| it 'logs sms send failure when number is opt out and enrollment not updated' do |
7 participants
🎫 Ticket
LG-10126: Create a job to send notification SMSs and delete phone numbers
🛠 Summary of changes
Create a job that when the result of enrollment is available and send sms notification if a notification phone is available, remind user to check result.
A new column is added to in_person_enrollments table to record when the sms notification is sent.
Job scheduled by the usps enrollment status update job.
Job is controlled by in_person_send_proofing_notifications_enabled in addition to in_person_proofing_enabled.
📜 Testing Plan
Provide a checklist of steps to confirm the changes.