Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send VA Profile data on SMS Notification Delivery status #2137

Closed
14 tasks done
MackHalliday opened this issue Nov 19, 2024 · 13 comments
Closed
14 tasks done

Send VA Profile data on SMS Notification Delivery status #2137

MackHalliday opened this issue Nov 19, 2024 · 13 comments

Comments

@MackHalliday
Copy link

MackHalliday commented Nov 19, 2024

User Story - Business Need

VA Profile is interested in tracking SMS delivery status so that they might offer data to their partners regarding the quality of contact data/the likelihood of delivery success. VA Notify needs to send VA Profile the delivery status of ALL SMS notifications so VA Profile can track this information.

It is recommended to reuse the code created in #1770 to send VAProfile Email Notification statues. check_and_queue_va_profile_email_status_callback and it's childern method could be updated to be notification-type ambiguous, handling both SMS and Email notification types. check_and_queue_callback_task can also be modified to capture the status of all SMS Notifications and send them to VAProfile.

  • Ticket is understood, and QA has been contacted (if the ticket has a QA label).
  • Touch base with Cris when she is back.

User Story(ies)

As a developer on the VA Notify team
I want to send real-time SMS delivery status updates to VA Profile
So that the VA system can improve the management and reliability of contact data, thereby enhancing communication with veterans and maintaining the organization's contact reputation.

Additional Info and Resources

Acceptance Criteria

  • When ANY SMS or email delivery status is processed, it should trigger a POST request to VA Profile with accurate and complete delivery status data in the DEV/PERF environments.
  • Delivery status data prior to the delivery attempt should NOT trigger a POST request to VA Profile No 'preprovider' notification statuses should be sent - these statuses include NOTIFICATION_CREATED, NOTIFICATION_SENDING, or NOTIFICATION_PENDING
  • The system should handle errors gracefully and log them appropriately
  • The feature should work seamlessly with existing infrastructure, with minimal need for new development.
  • Reuse feature flag for VA_PROFILE_EMAIL_STATUS_ENABLED
  • Include logic to continue to send statuses to VAPROFILE if EMAIL regardless of FF
  • Make ticket for feature flag removal
  • This work is added to the sprint review slide deck (key win bullet point and demo slide)
  • All changes should be well-documented

QA Considerations

  • Verify that a delivery status triggers the POST request to VA Profile. This probably requires checking the logs. Work with developer to understand the logging.
  • Verify the feature flag and parameters
  • Take a look at the documentation.

Potential Dependencies

VA Profile's Pilot will happen next PI (starting mid-December), most likely wait until the new year.

@MackHalliday
Copy link
Author

MackHalliday commented Nov 26, 2024

Notes from Conversation with Kyle and Cris

  1. How should the feature flag be handled?
  • Repurpose VA_PROFILE_EMAIL_STATUS_ENABLED
  • @cris-oddball Which environment should the feature flag be enabled? Enable in DEV and PERF, not enable STAGING and PROD
  • @cris-oddball When sending to PROD, need to send PR to PROD to enable FF ASAP
  • Add in logic to continue to send if notification_type is EMAIL
  1. Should this be implemented in the check_and_queue_callback_task function?
  • Better to handle under sms_status_update under if last_updated_at != notification.updated_at
  1. AC Delivery status data prior to the delivery attempt should NOT trigger a POST request to VA Profile. What status are considered pre-delivery?
    See notification-api/app/constants.py

Will touch base with @cris-oddball when she's back on Monday

@MackHalliday
Copy link
Author

Yesterday

  • Spoke to Kyle regarding implementation (See notes above)
  • Create initial test for sending SMS to VAProfile
  • Refactor send_email_status_to_va_profile to handle SMS

Today

  • Rename and move send_email_status_to_va_profile to own module
  • Fix tests that will break

I'm out on Friday. So I'll leave an update at the EOD

@MackHalliday
Copy link
Author

Wednesday

  • Got testing for EMAIL working
  • Update FEATURE FLAG
  • Start refactoring

Today

  • Update testing for SMS after refactor
  • TEST

MackHalliday added a commit that referenced this issue Dec 2, 2024
MackHalliday added a commit that referenced this issue Dec 2, 2024
…le. Sent env values for PERF and DEV to True and STAGING and PROD to False
@MackHalliday
Copy link
Author

MackHalliday commented Dec 3, 2024

Yesterday

  • Talk to Cris about ticket.
  • Another minor refactor
  • Add testing for minor adjust to FF logic
  • Working thru SMS testing updates

Today

  • Finish SMS testing updates
  • Deploy to DEV

MackHalliday added a commit that referenced this issue Dec 3, 2024
MackHalliday added a commit that referenced this issue Dec 3, 2024
MackHalliday added a commit that referenced this issue Dec 3, 2024
MackHalliday added a commit that referenced this issue Dec 3, 2024
MackHalliday added a commit that referenced this issue Dec 4, 2024
MackHalliday added a commit that referenced this issue Dec 4, 2024
…t file to reflect renamed celery task, clean up imports
MackHalliday added a commit that referenced this issue Dec 4, 2024
MackHalliday added a commit that referenced this issue Dec 4, 2024
MackHalliday added a commit that referenced this issue Dec 4, 2024
MackHalliday added a commit that referenced this issue Dec 4, 2024
@MackHalliday
Copy link
Author

Yesterday

  • Debug way celery failing
  • Test PR
  • Submit PR
  • Rewrite some docs

Today

  • Merge PR once approved.

@cris-oddball
Copy link

PR approved and merged, sending to Perf for validation.

@cris-oddball
Copy link

QA FAILED

This is sending things to VaProfile that are not a successful delivery or permanent failure due to invalid phone number, like in 'sending'

Image

Will continue to send it through pipeline since it is all behind a feature flag.

@MackHalliday
Copy link
Author

MackHalliday commented Dec 5, 2024

Talked to @k-macmillan and got clarification around AC Delivery status data prior to the delivery attempt should NOT trigger a POST request to VA Profile. We should be sending ALL notification statuses that are NOT pre-provider. Pre-provider statuses include NOTIFICATION_CREATED, NOTIFICATION_SENDING, or NOTIFICATION_PENDING. Updated AC.

@cris-oddball
Copy link

PR approved and merged, sending to Perf for validation after the next staging/prod deployment is complete.

@cris-oddball
Copy link

cris-oddball commented Dec 5, 2024

QA PASSED

  • Had previously verified that only email delivery statuses are being sent to staging/prod (feature flag is disabled there).
  • Delivery statuses that are not pre-provider are sent to VAProfile, validated by means of the regression which actually sends 15 sms and 11 email messages. There may be multiple sends for the same notification (i.e., sent, delivered, as shown in screenshot above). Check for new logging in this format:
'Sending notification status to VA Profile with url: %s | notification_id: %s | notification_status: %s'
'VA Profile response when receiving status of notification_id: %s | notification_status: %s | status code: %s | json: %s',

logs demonstrating that 11 email statuses were sent on to VAProfile, and 17 sms statuses, two of which are multiples for sent and delivered., which makes it 15 sms notifications.
Image

Note that VAProfile is not yest able to accept sms notifications, so they will need to be notified when we turn on the feature flag in staging/prod.

@cris-oddball
Copy link

Holding ticket open for documentation and sending code to prod.

@cris-oddball
Copy link

Documentation complete (and looks good), closing ticket.

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

No branches or pull requests

5 participants