department-of-veterans-affairs / notification-api

Notification API
MIT License
16 stars 8 forks source link

Send text notifications utilizing Pinpoint #251

Closed QaysarA closed 3 years ago

QaysarA commented 3 years ago

Value Statement

As a VANotify consumer I want to send text messages So that text message notifications can be sent to the Veteran

Acceptance Criteria

GIVEN VANotify receives a request to send a text notification
WHEN Pinpoint is setup THEN a text message is attempted to be sent to the respective mobile phone

GIVEN VANotify receives a text request THEN VANotify passes the request to Pinpoint, and they receive it Pinpoint will try to deliver the text

GIVEN service is newly created When the service has not specified a sender from number THEN the text will be sent from the global default sender from number

GIVEN VANotify receives a text request When the service has been assigned a sender ID / from number THEN the text will be sent from that assigned sender ID / from number

GIVEN VANotify receives a text request When the request contains a specific / non-default sender ID / from number that has overridden the assigned service sender ID / from number THEN the text will be sent from that specific sender ID / from number

GIVEN VANotify receives a request to send a text notification
WHEN VANotify makes a request to Pinpoint THEN environment-specific project ID is used

GIVEN VANotify receives a request to send a text notification
WHEN VANotify makes a request to Pinpoint THEN the message type is set to transactional

Checklist

Assumptions

Additional Info/Resources

Resources

Out of Scope

Open Questions

miabecker commented 3 years ago

@QaysarA can we review what pinpoint is and how we are using it? I think it is related to the short and long codes right?

We don't have access to it yet, waiting on a Security waiver. Need admin security rights.

We have requested a short-code for Pinpoint - has to be registered with all of the telephony companies i.e. verizon, t-mobile etc. Can take up to 2 months to get the code. Only recently did we get documentation from Paras / VAs. AWS Support via VA Enterprise Cloud Support - ECSO (Enterprise Cloud Solutions Office).

miabecker commented 3 years ago

List of things to test in QA: Check provider list to make sure pin point is prioritized Send a text message to self one more thing I didn't here

lingtran commented 3 years ago

Kickoff questions:

ffafara-tw commented 3 years ago

Are we doing any additional or modifying existing rate-limits?

No. However I'm curious where did this question come from - maybe we are missing something?

clarification for out-of-scope item "Delivery status and research mode works" - meaning we do not need to confirm delivery status?

Handling delivery statuses and research mode will be handled in a separate story - #273 So you might want to manually confirm in AWS console (if possible) that SMS was delivered. But passing delivery status to VANotify and subsequent processing is out of scope.

enable feature flag?

I've been changing my stance on that. Initially I wanted all the providers to be behind feature flags. This was primarily driven by the fact that we need to put settings for Twilio provider even when we do not use it, or the application won't start. However providers already have "active" flag that we can change using an API call (or via DB migrations). So my current stance is to not have feature flags as long as the provider does not interfere with the app (and startup) when not used / deactivated. I'll appreciate any feedback, thoughts, discussions on that topic.

confirm this is adding 'pinpoint' as new SMS provider

I confirm.

lingtran commented 3 years ago

@ffafara-tw

re: rate-limiting:

I may have misunderstood from our group discussion last week on Pinpoint, where we talked about this (straight from my notes)STORMING SESSION AROUND rate-limiting around SMS capabilities with primary focus on Pinpoint. This was towards the end of the chat. There are options to set limits within Pinpoint, but not required.

re: feature flag:

We are kind of all over the place with how we are going about it, the recipient identifier flag is an environment variable set in task defs, whereas the govdelivery client is in our config file...and also maybe we have not been great at managing the flags, for instance we still have our feature flag for GOVDELIVERY_EMAIL_CLIENT_ENABLED. Apart of the downside of managing flags, the upside is how it contextualizes features in our codebase from a product perspective. Leverage an api call or db migration might not express the same level of context. That being said, this is a simple make active true/false scenario, so either way is fine for this instance.

lingtran commented 3 years ago

Marisa and I combed through the documentations on both AWS and Python SDK side for Pinpoint. We did some testing in the AWS cli as well as the console. We were trying to send text via the CLI but ran into cli errors...to be continued on Monday. If successful, ducks are in a row to go! 🦆🦆🦆

marisahoenig commented 3 years ago

Possible question for meeting with pinpoint rep: do they know if it's possible to programmatically create long codes?

ffafara-tw commented 3 years ago

I don't think it is. I was looking for that in their documentation and AWS CLI docs and there wasn't anything there. We can ask them to make sure.

miabecker commented 3 years ago

Cool -lets ask to be sure. @Filip Fafara ffafara@thoughtworks.com I've asked Marisa, Ling and Emily to come prepared with some questions for tomorrow.

M

On Tue, Dec 15, 2020 at 5:24 PM Filip Fafara notifications@github.com wrote:

I don't think it is. I was looking for that in their documentation and AWS CLI docs and there wasn't anything there.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/department-of-veterans-affairs/notification-api/issues/251#issuecomment-745606118, or unsubscribe https://github.com/notifications/unsubscribe-auth/ARTMDO7WHHXODCOKGMVHTZDSU7O2LANCNFSM4TPXKIEQ .

-- Maria Becker Product Manager Email maria.becker@thoughtworks.com Telephone 347-461-7592 Pronouns they/them or she/her [image: ThoughtWorks] http://www.thoughtworks.com/?utm_campaign=maria-becker-signature&utm_medium=email&utm_source=thoughtworks-email-signature-generator

lingtran commented 3 years ago

Things we hope to bring up during call:

FROM CALL:

New question:

miabecker commented 3 years ago

This is great! Do we need to ask anything about opt outs?

M

thought with care written with haste reading up to you

On Dec 16, 2020, at 10:44 AM, Ling Tran notifications@github.com wrote:

 Things we hope to bring up during call:

Longcodes - more explanation around this? Can we specific longcodes specific to projects? Seems to be account level setting. How does 'quiet time hours' get enforced? Does this apply to api calls to Pinpoint? — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

jsmithVA commented 3 years ago

Test plan doc (WIP) - https://docs.google.com/document/d/1XvyGqeji2u8lMCoCS-tHex3vyNTsZ-HCByFlJQOwNS8/edit

lingtran commented 3 years ago

Tested in dev Pinpoint integration, cross-referenced with logs. Datadog dev-pinpoint dashboard in the last four hours today (12/23): https://app.datadoghq.com/dashboard/rgg-5ee-rwf?from_ts=1608751307447&live=true&to_ts=1608765707447

It looks like celery workers stopped updating in the logs, so the last few messages I tried to send my phone never got, although the logs indicate success.

Emily and I tested without setting any default sms_sender information for service. The 'FROM_NUMBER' gets set when a service gets created without it. Initially when we tested, we did it without having Pinpoint as the primary provider nor was it active. In that scenario, the two long codes we have are alternately used.

Once activated Pinpoint and gave it a higher priority of 5 (while lowering SNS priority to 10), we were able to verify that Pinpoint was the provider used and text messages were sent. However, it appears that rather than use the specified long-code (pinpoint origination number) for dev, the messages were sent to the last number I received from the VA, which happened to be the number we assigned for staging. We will have to investigate why. We have a feeling it may be related to the 'FROM_NUMBER' usage...at this time the only things we have verified about the 'FROM_NUMBER' config var are:

Attached are also Datadog graphs. There was an initial error due to access issues (and it was logged...issue has been fixed).Screen Shot 2020-12-23 at 6.00.43 PM.png Screen Shot 2020-12-23 at 5.30.14 PM.png

Remaining to do:

ffafara-tw commented 3 years ago

It looks like celery workers stopped updating in the logs, so the last few messages I tried to send my phone never got, although the logs indicate success.

Which logs indicate success? It sounds like this might be the problem we have seen in the past where celery stops processing events. Can you check SQS queue?

Sent from my iPhone

On Dec 23, 2020, at 6:38 PM, Ling Tran notifications@github.com wrote:

It looks like celery workers stopped updating in the logs, so the last few messages I tried to send my phone never got, although the logs indicate success.

lingtran commented 3 years ago

end of day update:

lingtran commented 3 years ago

:) This log stream shows in staging we were able to send a SMS notification with Pinpoint given highest priority: ecs/notification-celery/9da2ee2070c3412289fdb83171857c1e

Datadog metrics for successes also captured here, note that staging.notifications.delivery.tasks.deliver_sms shows all the SMS activity, and the Pinpoint and SNS graphs show respective client activity: https://app.datadoghq.com/metric/explorer?from_ts=1609850439711&to_ts=1609864839711&live=true&page=0&is_auto=false&tile_size=m&exp_metric=staging.notifications.delivery.clients.pinpoint.success%2Cstaging.notifications.delivery.clients.pinpoint.error%2Cstaging.notifications.delivery.clients.pinpoint.request_time.count%2Cstaging.notifications.delivery.tasks.deliver_sms%2Cstaging.notifications.delivery.clients.sns.success&exp_agg=avg&exp_row_type=metric

lingtran commented 3 years ago

Update at the moment: dev and staging environments are set with default sms senders to respective long codes. Pinpoint has been elevated in priority over SNS as SMS client in dev and staging.

Prod project will be created upon infra deployment to production. After which, we can then grab the app id of prod project and drop it as env var on api side. Also, update default sms senders before activating Pinpoint

lingtran commented 3 years ago

Handy cli command to check delivered messages from specific pinpoint project/app (takes about 5 minutes for stats to update after a delivery): aws pinpoint get-application-date-range-kpi \ --application-id <app-id> \ --kpi-name txn-sms-delivered

ffafara-tw commented 3 years ago

Can you document that CLI command somewhere in vanotify-team repo? Maybe in documents/support folder?

On Tue, Jan 5, 2021 at 2:25 PM Ling Tran notifications@github.com wrote:

Handy cli command to check delivered messages from specific pinpoint project/app (takes about 5 minutes for stats to update after a delivery): aws pinpoint get-application-date-range-kpi \ --application-id \ --kpi-name txn-sms-delivered

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/department-of-veterans-affairs/notification-api/issues/251#issuecomment-754847180, or unsubscribe https://github.com/notifications/unsubscribe-auth/ANNLRX6UWOJHV4CDIPSMEZDSYNRSPANCNFSM4TPXKIEQ .

-- Filip Fafara Consultant Email ffafara@thoughtworks.com Telephone +1 267 262 0689 <+1+267+262+0689> [image: ThoughtWorks] http://www.thoughtworks.com/?utm_campaign=filip-fafara-signature&utm_medium=email&utm_source=thoughtworks-email-signature-generator

lingtran commented 3 years ago

added user flow test for simple happy path, do not pass in personalisation in payload. do we want to test with personalisation? not sure what will be passed.

ffafara-tw commented 3 years ago

No need to use template with personalization.