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

Use Pinpoint by default #2173

Merged
merged 62 commits into from
May 22, 2024
Merged
Show file tree
Hide file tree
Changes from 58 commits
Commits
Show all changes
62 commits
Select commit Hold shift + click to select a range
9740520
rough in pinpoint
sastels Apr 5, 2024
064e36d
add pool_id env var
sastels Apr 5, 2024
b9d0d3a
sending with pinpoint pool
sastels Apr 9, 2024
0fa34c1
use pinpoint pool for specified templates
sastels Apr 9, 2024
7e69706
format
sastels Apr 9, 2024
b3d12e5
tweak
sastels Apr 9, 2024
fe263c6
Merge branch 'main' into rough-in-pinpoint
sastels Apr 9, 2024
d18ce27
add configuration set
sastels Apr 11, 2024
fceab9f
add task for processing pinpoint receipts
sastels Apr 12, 2024
b8ceb91
add mocks for pinpoint receipt task testing
sastels Apr 12, 2024
1a6ba9e
rough in test_process_pinpoint_receipts_tasks.py
sastels Apr 12, 2024
6e3d339
make tests pass
sastels Apr 12, 2024
ca2da8c
add explicit return
sastels Apr 12, 2024
7ec198b
Merge branch 'main' into rough-in-pinpoint
sastels Apr 12, 2024
3db48e8
tweak
sastels Apr 18, 2024
d2e8471
tweak
sastels Apr 22, 2024
c8eb6c5
Merge branch 'main' into rough-in-pinpoint
sastels Apr 22, 2024
6c090ce
working now
sastels Apr 22, 2024
3f59f27
add new env vars to .env.examples
sastels Apr 23, 2024
646b86d
Update .env.example
sastels Apr 24, 2024
bde0f68
Update .env.example
sastels Apr 24, 2024
12f539f
Update app/clients/sms/aws_pinpoint.py
sastels Apr 24, 2024
965fbff
Update app/config.py
sastels Apr 24, 2024
26d4419
Update app/delivery/send_to_providers.py
sastels Apr 24, 2024
5710df2
Update app/config.py
sastels Apr 24, 2024
28524bb
Merge branch 'main' into rough-in-pinpoint
sastels Apr 24, 2024
efa0df9
add typing / docstring for determine_pinpoint_status
sastels Apr 24, 2024
d1612c2
wip add pattern matching
sastels Apr 25, 2024
c08cc14
matchy-matchy
sastels Apr 25, 2024
aed401f
Merge branch 'main' into rough-in-pinpoint
sastels May 6, 2024
e205bc1
formatting
sastels May 6, 2024
41e7eaa
add if/else alternative to matching
sastels May 6, 2024
40e4c87
fix case
sastels May 6, 2024
f09cec5
Merge branch 'main' into rough-in-pinpoint
sastels May 6, 2024
fde4812
add return
sastels May 6, 2024
040a361
use new callback.pinpoint metrics
sastels May 6, 2024
da4006f
remove redundant CliendError catching
sastels May 6, 2024
188d0b6
Merge branch 'main' into rough-in-pinpoint
sastels May 6, 2024
b4e2bd8
format
sastels May 6, 2024
afec912
refactor process_pinpoint_results tests
sastels May 7, 2024
337c594
if/else ftw
sastels May 7, 2024
7fb51e1
format
sastels May 7, 2024
6e7de9f
rough in using default pinpoint pool
sastels May 8, 2024
ea7f071
format
sastels May 8, 2024
70e540c
Merge branch 'main' into pinpoint-for-everyone
sastels May 9, 2024
5dcaf6b
add param
sastels May 9, 2024
5717361
fix tests
sastels May 9, 2024
727070f
add pinpoint tests
sastels May 13, 2024
76ab640
reenable pinpoint client
sastels May 13, 2024
b6b427b
use sns for usa and delicated long code sending
sastels May 13, 2024
82f0726
don't need to set pinpoint active in test
sastels May 13, 2024
54d5b40
delete TODO because I did it
sastels May 13, 2024
f906489
fix tests
sastels May 13, 2024
7859b95
Merge branch 'main' into pinpoint-for-everyone
sastels May 13, 2024
abefbac
make tests serial
sastels May 14, 2024
635883b
fix exception
sastels May 14, 2024
fb8f395
nope
sastels May 14, 2024
2a80780
again
sastels May 14, 2024
45bd22b
Update app/clients/sms/aws_sns.py
sastels May 16, 2024
554ef3c
Update tests/app/dao/test_provider_details_dao.py
sastels May 16, 2024
91aca6a
Apply suggestions from code review
sastels May 16, 2024
8447d6f
Merge branch 'main' into pinpoint-for-everyone
sastels May 22, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .env.example
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,4 @@ CONTACT_FORM_EMAIL_ADDRESS = ""

AWS_PINPOINT_SC_POOL_ID=
AWS_PINPOINT_SC_TEMPLATE_IDS=
AWS_PINPOINT_DEFAULT_POOL_ID=
9 changes: 6 additions & 3 deletions app/clients/sms/aws_pinpoint.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,22 @@ class AwsPinpointClient(SmsClient):
def init_app(self, current_app, statsd_client, *args, **kwargs):
self._client = boto3.client("pinpoint-sms-voice-v2", region_name="ca-central-1")
super(AwsPinpointClient, self).__init__(*args, **kwargs)
# super(SmsClient, self).__init__(*args, **kwargs)
self.current_app = current_app
self.name = "pinpoint"
self.statsd_client = statsd_client

def get_name(self):
return self.name

def send_sms(self, to, content, reference, multi=True, sender=None):
pool_id = self.current_app.config["AWS_PINPOINT_SC_POOL_ID"]
def send_sms(self, to, content, reference, multi=True, sender=None, template_id=None):
messageType = "TRANSACTIONAL"
matched = False

if template_id is not None and str(template_id) in self.current_app.config["AWS_PINPOINT_SC_TEMPLATE_IDS"]:
pool_id = self.current_app.config["AWS_PINPOINT_SC_POOL_ID"]
else:
pool_id = self.current_app.config["AWS_PINPOINT_DEFAULT_POOL_ID"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if the pool_id is None? Is a random OriginationIdentity would be used then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the OriginationIdentity is None in the boto call then the call returns an error

Invalid type for parameter OriginationIdentity, value: None, type: <class 'NoneType'>, valid types: <class 'str'>

But as you mention below, we catch that case in our code and fall back to SNS.


for match in phonenumbers.PhoneNumberMatcher(to, "US"):
matched = True
to = phonenumbers.format_number(match.number, phonenumbers.PhoneNumberFormat.E164)
Expand Down
8 changes: 2 additions & 6 deletions app/clients/sms/aws_sns.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
from time import monotonic

import boto3
import botocore
import phonenumbers
from notifications_utils.statsd_decorators import statsd

Expand All @@ -27,7 +26,7 @@ def get_name(self):
return self.name

@statsd(namespace="clients.sns")
def send_sms(self, to, content, reference, multi=True, sender=None):
def send_sms(self, to, content, reference, multi=True, sender=None, template_id=None):
matched = False

for match in phonenumbers.PhoneNumberMatcher(to, "US"):
Expand Down Expand Up @@ -66,12 +65,9 @@ def send_sms(self, to, content, reference, multi=True, sender=None):
try:
start_time = monotonic()
response = client.publish(PhoneNumber=to, Message=content, MessageAttributes=attributes)
except botocore.exceptions.ClientError as e:
self.statsd_client.incr("clients.sns.error")
raise str(e)
except Exception as e:
self.statsd_client.incr("clients.sns.error")
raise str(e)
Copy link
Collaborator Author

@sastels sastels May 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This stuff was redundant and also the raise didn't work! You can't raise a string, just some subclass of Exception.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pylint for the win! I feel bad we had that for that long in the code. The whole error management does look weird in retrospective, thank you for refactoring.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha, I didn't find it with pylint but by accidentally triggering the error locally (I was trying to send to a USA number without setting the AWS_US_TOLL_FREE_NUMBER variable)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. I thought it might be pylint because the latter did raise it as a potential problem when I looked at this code locally in vscode.

raise Exception(e)
sastels marked this conversation as resolved.
Show resolved Hide resolved
finally:
elapsed_time = monotonic() - start_time
self.current_app.logger.info("AWS SNS request finished in {}".format(elapsed_time))
Expand Down
1 change: 1 addition & 0 deletions app/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,7 @@ class Config(object):
AWS_SES_SECRET_KEY = os.getenv("AWS_SES_SECRET_KEY")
AWS_PINPOINT_REGION = os.getenv("AWS_PINPOINT_REGION", "us-west-2")
AWS_PINPOINT_SC_POOL_ID = os.getenv("AWS_PINPOINT_SC_POOL_ID", None)
AWS_PINPOINT_DEFAULT_POOL_ID = os.getenv("AWS_PINPOINT_DEFAULT_POOL_ID", None)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we really want None here as a fallback default, or rather prevent the server from booting up to prevent a misconfiguration. It depends how OriginationIdentity set to None in the send_text_message call behaves.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok reading the code further, it seems that a None value would route the SNS implementation to get used instead. I got this right?

Copy link
Collaborator Author

@sastels sastels May 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah exactly. A None value will break the Pinpoint send, so right now we route to SNS. In the future we may want to throw an error instead (ie when we insist on Pinooint for everything). Right now it allows production to ignore pinpoint entirely since we don't have it set there yet.

AWS_PINPOINT_CONFIGURATION_SET_NAME = os.getenv("AWS_PINPOINT_CONFIGURATION_SET_NAME", "pinpoint-configuration")
AWS_PINPOINT_SC_TEMPLATE_IDS = env.list("AWS_PINPOINT_SC_TEMPLATE_IDS", [])
AWS_US_TOLL_FREE_NUMBER = os.getenv("AWS_US_TOLL_FREE_NUMBER")
Expand Down
64 changes: 53 additions & 11 deletions app/delivery/send_to_providers.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@
import os
import re
from datetime import datetime
from typing import Dict
from typing import Any, Dict, Union
Fixed Show fixed Hide fixed
from uuid import UUID

import phonenumbers
from flask import current_app
from notifications_utils.recipients import (
validate_and_format_email_address,
Expand Down Expand Up @@ -48,6 +49,7 @@
NOTIFICATION_VIRUS_SCAN_FAILED,
PINPOINT_PROVIDER,
SMS_TYPE,
SNS_PROVIDER,
BounceRateStatus,
Notification,
Service,
Expand All @@ -67,9 +69,9 @@ def send_sms_to_provider(notification):
provider = provider_to_use(
SMS_TYPE,
notification.id,
notification.to,
notification.international,
notification.reply_to_text,
template_id=notification.template_id,
)

template_dict = dao_get_template_by_id(notification.template_id, notification.template_version).__dict__
Expand Down Expand Up @@ -105,6 +107,7 @@ def send_sms_to_provider(notification):
content=str(template),
reference=str(notification.id),
sender=notification.reply_to_text,
template_id=notification.template_id,
)
except Exception as e:
notification.billable_units = template.fragment_count
Expand Down Expand Up @@ -336,16 +339,55 @@ def update_notification_to_sending(notification, provider):
dao_update_notification(notification)


def provider_to_use(notification_type, notification_id, international=False, sender=None, template_id=None):
# Temporary redirect setup for template IDs that are meant for the short code usage.
if notification_type == SMS_TYPE and template_id is not None and str(template_id) in Config.AWS_PINPOINT_SC_TEMPLATE_IDS:
return clients.get_client_by_name_and_type("pinpoint", SMS_TYPE)
def provider_to_use(
notification_type: str,
notification_id: UUID,
to: Union[str, None] = None,
sastels marked this conversation as resolved.
Show resolved Hide resolved
international: bool = False,
sender: Union[str, None] = None,
sastels marked this conversation as resolved.
Show resolved Hide resolved
) -> Any:
"""
Get the provider to use for sending the notification.
SMS that are being sent with a dedicated number or to a US number should not use Pinpoint.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the documentation and typing annotations.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured I had added enough new logic to this function that some documentation was useful 😬


Args:
notification_type (str): SMS or EMAIL.
notification_id (UUID): id of notification. Just used for logging.
to (str, optional): recipient. Defaults to None.
international (bool, optional): Recipient is international. Defaults to False.
sender (str, optional): reply_to_text to use. Defaults to None.

Raises:
Exception: No active providers.

active_providers_in_order = [
p
for p in get_provider_details_by_notification_type(notification_type, international)
if p.active and p.identifier != PINPOINT_PROVIDER
]
Returns:
provider: Provider to use to send the notification.
"""

has_dedicated_number = sender is not None and sender.startswith("+1")
sending_to_us_number = False
if to is not None:
match = next(iter(phonenumbers.PhoneNumberMatcher(to, "US")), None)
if match and phonenumbers.region_code_for_number(match.number) == "US":
sending_to_us_number = True

if (
has_dedicated_number
or sending_to_us_number
or current_app.config["AWS_PINPOINT_SC_POOL_ID"] is None
or current_app.config["AWS_PINPOINT_DEFAULT_POOL_ID"] is None
):
active_providers_in_order = [
p
for p in get_provider_details_by_notification_type(notification_type, international)
if p.active and p.identifier != PINPOINT_PROVIDER
]
else:
active_providers_in_order = [
p
for p in get_provider_details_by_notification_type(notification_type, international)
if p.active and p.identifier != SNS_PROVIDER
]

if not active_providers_in_order:
current_app.logger.error("{} {} failed as no active providers".format(notification_type, notification_id))
Expand Down
19 changes: 19 additions & 0 deletions migrations/versions/0450_enable_pinpoint_provider.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
"""

Revision ID: 0450_enable_pinpoint_provider
Revises: 0449_update_magic_link_auth
Create Date: 2021-01-08 09:03:00 .214680

"""
from alembic import op

revision = "0450_enable_pinpoint_provider"
down_revision = "0449_update_magic_link_auth"


def upgrade():
op.execute("UPDATE provider_details set active=true where identifier in ('pinpoint');")


def downgrade():
op.execute("UPDATE provider_details set active=false where identifier in ('pinpoint');")
73 changes: 73 additions & 0 deletions tests/app/clients/test_aws_pinpoint.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import pytest

from app import aws_pinpoint_client
from tests.conftest import set_config_values


@pytest.mark.serial
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some sort of race condition in our tests that I couldn't figure out :/ If these two tests are not run serially then something resets the config variables that they change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be a great topic for a code review session. It seems we have a misunderstanding on our some of our test concurrency is setup and fails when they should be successful. We can put our mind to it and try to brainstorm on this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave some more thoughts to this and one thing I wish we could do better is OOP in our code base. We barely have much and this could be our focus on code reviews session. We rely a lot on environment variables which is not a good practice in general, as the code relies on environment rather than configuration of the logic to run. Encapsulating the pool config in an object would make this easier probably to run instead of relying on environments. Do you know what I mean here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do pull the env vars out of the env and put into the Config object, but we should make sure that we always read that object and never the env vars directly (which I think we're pretty good at?). I think that there's magic going on behind the scenes though (with flask, for example), where it takes vars directly from the env :/

In this case, though, in the test I made serial the app wasn't using the values of Config that I added in the test setup 😞 Which - how can that happen?

def test_send_sms_sends_to_default_pool(notify_api, mocker, sample_template):
boto_mock = mocker.patch.object(aws_pinpoint_client, "_client", create=True)
mocker.patch.object(aws_pinpoint_client, "statsd_client", create=True)
to = "6135555555"
content = "foo"
reference = "ref"

with set_config_values(
notify_api,
{
"AWS_PINPOINT_SC_POOL_ID": "sc_pool_id",
"AWS_PINPOINT_DEFAULT_POOL_ID": "default_pool_id",
"AWS_PINPOINT_CONFIGURATION_SET_NAME": "config_set_name",
"AWS_PINPOINT_SC_TEMPLATE_IDS": [],
},
):
aws_pinpoint_client.send_sms(to, content, reference=reference, template_id=sample_template.id)

boto_mock.send_text_message.assert_called_once_with(
DestinationPhoneNumber="+16135555555",
OriginationIdentity="default_pool_id",
MessageBody=content,
MessageType="TRANSACTIONAL",
ConfigurationSetName="config_set_name",
)


@pytest.mark.serial
def test_send_sms_sends_to_shortcode_pool(notify_api, mocker, sample_template):
boto_mock = mocker.patch.object(aws_pinpoint_client, "_client", create=True)
mocker.patch.object(aws_pinpoint_client, "statsd_client", create=True)
to = "6135555555"
content = "foo"
reference = "ref"

with set_config_values(
notify_api,
{
"AWS_PINPOINT_SC_POOL_ID": "sc_pool_id",
"AWS_PINPOINT_DEFAULT_POOL_ID": "default_pool_id",
"AWS_PINPOINT_CONFIGURATION_SET_NAME": "config_set_name",
"AWS_PINPOINT_SC_TEMPLATE_IDS": [str(sample_template.id)],
},
):
aws_pinpoint_client.send_sms(to, content, reference=reference, template_id=sample_template.id)

boto_mock.send_text_message.assert_called_once_with(
DestinationPhoneNumber="+16135555555",
OriginationIdentity="sc_pool_id",
MessageBody=content,
MessageType="TRANSACTIONAL",
ConfigurationSetName="config_set_name",
)


def test_send_sms_returns_raises_error_if_there_is_no_valid_number_is_found(notify_api, mocker):
mocker.patch.object(aws_pinpoint_client, "_client", create=True)
mocker.patch.object(aws_pinpoint_client, "statsd_client", create=True)

to = ""
content = reference = "foo"

with pytest.raises(ValueError) as excinfo:
aws_pinpoint_client.send_sms(to, content, reference)

assert "No valid numbers found for SMS delivery" in str(excinfo.value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have a warning alert in TF specifically for this one? 🤔 I guess it might fall into celery errors but these might be generic enough that we don't always check on these?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

possibly, though this is a bit of a strange test - testing that if the notification has an invalid phone number in it then we get a log. Agree though that this is a strange thing that we'd like surfaced immediately though. Basically it means that we screwed up the phone number and AWS doesn't recognize it. So, for example, we start sending texts to a new country and our number formatter messes up. Yeah, I'll add a warning for these 👍

7 changes: 6 additions & 1 deletion tests/app/dao/test_provider_details_dao.py
Original file line number Diff line number Diff line change
Expand Up @@ -241,9 +241,14 @@ def test_get_sms_provider_with_equal_priority_returns_provider(


def test_get_current_sms_provider_returns_active_only(restore_provider_details):
# Note that we currently have two acive sms providers, sns and pinpoint
sastels marked this conversation as resolved.
Show resolved Hide resolved
current_provider = get_current_provider("sms")
current_provider.active = False
dao_update_provider_details(current_provider)
current_provider = get_current_provider("sms")
current_provider.active = False
dao_update_provider_details(current_provider)

new_current_provider = get_current_provider("sms")

assert new_current_provider is None
Expand Down Expand Up @@ -308,5 +313,5 @@ def test_dao_get_provider_stats(notify_db_session):
assert result[5].identifier == "pinpoint"
assert result[5].notification_type == "sms"
assert result[5].supports_international is False
assert result[5].active is False
assert result[5].active is True
assert result[5].current_month_billable_sms == 0
Loading
Loading