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 Email Notification Delivery status #1770

Closed
11 of 15 tasks
MackHalliday opened this issue May 1, 2024 · 11 comments
Closed
11 of 15 tasks

Send VA Profile data on Email Notification Delivery status #1770

MackHalliday opened this issue May 1, 2024 · 11 comments

Comments

@MackHalliday
Copy link

MackHalliday commented May 1, 2024

User Story - Business Need

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

  • Ticket is understood, and QA has been contacted (if the ticket has a QA label).

User Story

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

Additional Info and Resources

Engineering Checklist

  • Create POST request method in the VAProfileClient class to send delivery status data to VA Profile. The request body should mirror our current callback structure
    "id":"6ba01111-f3ee-4a45-9d04-234asdfb6abbb9a",  -- this is the notification id
    "reference":null,  
     "to":"",  -- this is the recipient's contact info
     "status":"delivered",  -- this will specify the delivery status of the notification
     "created_at":"2023-01-10T00:04:25.273410Z",  
     "completed_at":"2023-01-10T00:05:33.255911Z",  
     "sent_at":"2023-01-10T00:04:25.775363Z",  
     "notification_type":"SMS",   -- this is the channel/type of notification
     "status_reason": "", -- populated if there's additional context on the delivery status
     "provider":"pinpoint"
    
  • Add the bearer token value in the secrets section of the task definitions that extracts values from the parameter store using valueFrom in the secrets section
  • Hard code the path to their endpoint (example here)
  • Create new function(s) to handle this new feature and ensure the call stack is behind a Feature Flag
  • Add a Feature Flag in the Celery Task Definition for all environments, set to False
  • Modify process_ses_results Celery Task to use the new function. The status of ALL email notifications should be sent to VA Profile.
  • Utilize create_delivery_status_callback_data to build the data that will be sent to VA Profile (bonus points for only calling this once, and only when necessary). I don't think this makes sense given how it's implemented.
  • Implement error handling for new feature
  • Add unit testing
  • Update documentation

Acceptance Criteria

  • When ANY 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
    • SMS delivery data should NOT trigger a POST request to VA Profile
    • Email status data prior to the delivery attempt should NOT trigger a POST request to VA Profile
  • The system should handle errors gracefully and log them appropriately.
  • The feature should work seamlessly with existing infrastructure, with minimal need for new development.
  • 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.
  • Attempt negative test cases.
  • Verify the feature flag and parameters
  • Take a look at the documentation.
@cris-oddball cris-oddball changed the title Add ability to send VA Profile data on Email Notification Delivery status Send VA Profile data on Email Notification Delivery status May 2, 2024
@cris-oddball
Copy link

@kalbfled
Copy link
Member

Clarification about TTL: We don't want to lookup the access token every time we need to make a request to VA Profile. We discussed using the TTL cache package for this purpose.

@mjones-oddball
Copy link

Ian McEwan from VA Profile sent @k-macmillan info on their INT environment which can be configured in our Dev to test this

@EvanParish EvanParish self-assigned this Jul 23, 2024
@EvanParish
Copy link

Today I worked on implementing the feature to send data to va profile. The ticket says to "Utilize create_delivery_status_callback_data to build the data..." but I don't think that really make sense considering that is requires a service_callback to be passed and and this feature is not tied to a specific service. Maybe I'm missing something there, but I'll revisit tomorrow as I work on it more and start implementing unit tests.

Tomorrow I should have the feature work mostly wrapped up and I'll be working on unit testing.

@EvanParish EvanParish linked a pull request Jul 26, 2024 that will close this issue
25 tasks
@EvanParish EvanParish removed a link to a pull request Jul 26, 2024
25 tasks
@EvanParish
Copy link

Okay, I think I'm good on unit testing, but when deploying my code I didn't see any logs, so there's obviously something I'm missing when it comes to the celery task. I'll connect with @kalbfled or @mchlwellman tomorrow to see if they can help out with this issue.

@EvanParish
Copy link

I ran into issues with the celery task being executed when deployed. It seems that you can't pass objects into a task, so you have to make the data json serializable first. It looks like the task is executing now, so I'll continue testing tomorrow. It looks like unit tests will need to be fixed up a bit because of the required changes, so I'll be working on that as well.

@EvanParish
Copy link

The cause of the errors seen when deploying to perf were because I connected to VA Profile's int environment for my testing. This caused lookups to fail, most likely because of certificates not matching up. When deploying to the dev environment everything passes as expected. I'm doing some final code cleanup today and should be ready to open the PR in the morning.

@cris-oddball
Copy link

No QA possible since feature flag is not enabled in any env other than Perf. Reviewed logs with Evan and it looks great.

@cris-oddball
Copy link

cris-oddball commented Aug 1, 2024

QA PASSED CONDITIONALLY

It is possible to validate in Perf by virtue of the following log message:

Email status not sent to VA Profile, feature flag disabled | notification
  • Log message appears for email notifications

Email status not sent to VA Profile, feature flag disabled | notification 9ceb396f-df19-4eb9-b5bc-a38cc098eb35

  • total of 13 logs should be seen.
    • 11 v2 emails are delivered in the test suite, expect 11 logs for that
    • 2 v3 emails are delivered in the test suite, expect 2 logs - FAILED

Evan to create the bug ticket for the v3 email notifications not showing the logging statement.
Still deploying up to Prod since this feature is not yet enabled.

@cris-oddball
Copy link

The bug is in v3, not this code. See #1616.

@EvanParish
Copy link

The documentation has been added to the team repo here.

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

6 participants