From 9740520d9f2c565d0d8148092a26a77066e670b6 Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Fri, 5 Apr 2024 17:10:35 -0400 Subject: [PATCH 01/35] rough in pinpoint --- app/__init__.py | 5 +- app/clients/sms/aws_pinpoint.py | 92 +++++++++++++++++++++++++++++++ app/delivery/send_to_providers.py | 8 ++- app/models.py | 3 +- 4 files changed, 105 insertions(+), 3 deletions(-) create mode 100644 app/clients/sms/aws_pinpoint.py diff --git a/app/__init__.py b/app/__init__.py index 77a2a7d545..65ff1b973a 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -29,6 +29,7 @@ ) from app.clients.salesforce.salesforce_client import SalesforceClient from app.clients.sms.aws_sns import AwsSnsClient +from app.clients.sms.aws_pinpoint import AwsPinpointClient from app.dbsetup import RoutingSQLAlchemy from app.encryption import CryptoSigner from app.json_provider import NotifyJSONProvider @@ -45,6 +46,7 @@ notify_celery = NotifyCelery() aws_ses_client = AwsSesClient() aws_sns_client = AwsSnsClient() +aws_pinpoint_client = AwsPinpointClient() signer_notification = CryptoSigner() signer_personalisation = CryptoSigner() signer_complaint = CryptoSigner() @@ -107,6 +109,7 @@ def create_app(application, config=None): statsd_client.init_app(application) logging.init_app(application, statsd_client) aws_sns_client.init_app(application, statsd_client=statsd_client) + aws_pinpoint_client.init_app(application, statsd_client=statsd_client) aws_ses_client.init_app(application.config["AWS_REGION"], statsd_client=statsd_client) notify_celery.init_app(application) @@ -120,7 +123,7 @@ def create_app(application, config=None): performance_platform_client.init_app(application) document_download_client.init_app(application) - clients.init_app(sms_clients=[aws_sns_client], email_clients=[aws_ses_client]) + clients.init_app(sms_clients=[aws_sns_client, aws_pinpoint_client], email_clients=[aws_ses_client]) if application.config["FF_SALESFORCE_CONTACT"]: salesforce_client.init_app(application) diff --git a/app/clients/sms/aws_pinpoint.py b/app/clients/sms/aws_pinpoint.py new file mode 100644 index 0000000000..c5628519a2 --- /dev/null +++ b/app/clients/sms/aws_pinpoint.py @@ -0,0 +1,92 @@ +import boto3 +from botocore.exceptions import ClientError +import phonenumbers +from time import monotonic +from app.clients.sms import SmsClient + + +class AwsPinpointClient(SmsClient): + ''' + AWS Pinpoint SMS client + ''' + + def init_app(self, current_app, statsd_client, *args, **kwargs): + self._client = boto3.client('pinpoint-sms-voice-v2', region_name="ca-central-1") + 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): + + + # The phone number or short code to send the message from. The phone number + # or short code that you specify has to be associated with your Amazon Pinpoint + # account. For best results, specify long codes in E.164 format. + # originationNumber = sender + + # The recipient's phone number. For best results, you should specify the + # phone number in E.164 format. + # destinationNumber = "+14255550142" + + # The Amazon Pinpoint project/application ID to use when you send this message. + # Make sure that the SMS channel is enabled for the project or application + # that you choose. + applicationId = self.current_app.config['AWS_PINPOINT_APP_ID'] + + # The type of SMS message that you want to send. If you plan to send + # time-sensitive content, specify TRANSACTIONAL. If you plan to send + # marketing-related content, specify PROMOTIONAL. + messageType = "TRANSACTIONAL" + + # The registered keyword associated with the originating short code. + registeredKeyword = self.current_app.config['AWS_PINPOINT_KEYWORD'] + # The sender ID to use when sending the message. Support for sender ID + # varies by country or region. For more information, see + # https://docs.aws.amazon.com/pinpoint/latest/userguide/channels-sms-countries.html + # senderId = "MySenderID" + + matched = False + + for match in phonenumbers.PhoneNumberMatcher(to, "US"): + matched = True + to = phonenumbers.format_number(match.number, phonenumbers.PhoneNumberFormat.E164) + destinationNumber = to + + try: + start_time = monotonic() + + # from https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/pinpoint-sms-voice-v2/client/send_text_message.html + response = self._client.send_text_message( + DestinationPhoneNumber=destinationNumber, + OriginationIdentity=self.pool_id, + MessageBody=content, + MessageType=messageType, + Keyword=registeredKeyword + ) + + # this will be true if the OriginationIdentity does not exist in pinpoint + if response['MessageResponse']['Result'][destinationNumber]['StatusCode'] == 400: + self.statsd_client.incr("clients.pinpoint.error") + raise Exception(response['MessageResponse']['Result'][destinationNumber]['StatusMessage']) + except ClientError as e: + self.statsd_client.incr("clients.pinpoint.error") + raise Exception(e) + except Exception as e: + self.statsd_client.incr("clients.pinpoint.error") + raise Exception(e) + finally: + elapsed_time = monotonic() - start_time + self.current_app.logger.info("AWS Pinpoint request finished in {}".format(elapsed_time)) + self.statsd_client.timing("clients.pinpoint.request-time", elapsed_time) + self.statsd_client.incr("clients.pinpoint.success") + + return response['MessageResponse']['Result'][destinationNumber]['MessageId'] + + if not matched: + self.statsd_client.incr("clients.pinpoint.error") + self.current_app.logger.error("No valid numbers found in {}".format(to)) + raise ValueError("No valid numbers found for SMS delivery") \ No newline at end of file diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 33590c7667..f5f94c571b 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -69,6 +69,8 @@ def send_sms_to_provider(notification): notification.international, notification.reply_to_text, ) + + template_dict = dao_get_template_by_id(notification.template_id, notification.template_version).__dict__ @@ -343,7 +345,11 @@ def provider_to_use(notification_type, notification_id, international=False, sen current_app.logger.error("{} {} failed as no active providers".format(notification_type, notification_id)) raise Exception("No active {} providers".format(notification_type)) - return clients.get_client_by_name_and_type(active_providers_in_order[0].identifier, notification_type) + # return clients.get_client_by_name_and_type(active_providers_in_order[0].identifier, notification_type) + + if notification_type == SMS_TYPE: + return clients.get_client_by_name_and_type('pinpoint', notification_type) + def get_html_email_options(service: Service): diff --git a/app/models.py b/app/models.py index 81767406a1..f7b4c3924c 100644 --- a/app/models.py +++ b/app/models.py @@ -1272,9 +1272,10 @@ def get_link(self): SNS_PROVIDER = "sns" +PINPOINT_PROVIDER = 'pinpoint' SES_PROVIDER = "ses" -SMS_PROVIDERS = [SNS_PROVIDER] +SMS_PROVIDERS = [SNS_PROVIDER, PINPOINT_PROVIDER] EMAIL_PROVIDERS = [SES_PROVIDER] PROVIDERS = SMS_PROVIDERS + EMAIL_PROVIDERS From 064e36d3e33a61206f21a6f2546806ba04605d63 Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Fri, 5 Apr 2024 17:20:21 -0400 Subject: [PATCH 02/35] add pool_id env var --- app/clients/sms/aws_pinpoint.py | 31 ++----------------------------- app/config.py | 1 + 2 files changed, 3 insertions(+), 29 deletions(-) diff --git a/app/clients/sms/aws_pinpoint.py b/app/clients/sms/aws_pinpoint.py index c5628519a2..ce937d2a78 100644 --- a/app/clients/sms/aws_pinpoint.py +++ b/app/clients/sms/aws_pinpoint.py @@ -21,34 +21,8 @@ def get_name(self): return self.name def send_sms(self, to, content, reference, multi=True, sender=None): - - - # The phone number or short code to send the message from. The phone number - # or short code that you specify has to be associated with your Amazon Pinpoint - # account. For best results, specify long codes in E.164 format. - # originationNumber = sender - - # The recipient's phone number. For best results, you should specify the - # phone number in E.164 format. - # destinationNumber = "+14255550142" - - # The Amazon Pinpoint project/application ID to use when you send this message. - # Make sure that the SMS channel is enabled for the project or application - # that you choose. - applicationId = self.current_app.config['AWS_PINPOINT_APP_ID'] - - # The type of SMS message that you want to send. If you plan to send - # time-sensitive content, specify TRANSACTIONAL. If you plan to send - # marketing-related content, specify PROMOTIONAL. + pool_id = self.current_app.config['AWS_PINPOINT_POOL_ID'] messageType = "TRANSACTIONAL" - - # The registered keyword associated with the originating short code. - registeredKeyword = self.current_app.config['AWS_PINPOINT_KEYWORD'] - # The sender ID to use when sending the message. Support for sender ID - # varies by country or region. For more information, see - # https://docs.aws.amazon.com/pinpoint/latest/userguide/channels-sms-countries.html - # senderId = "MySenderID" - matched = False for match in phonenumbers.PhoneNumberMatcher(to, "US"): @@ -62,10 +36,9 @@ def send_sms(self, to, content, reference, multi=True, sender=None): # from https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/pinpoint-sms-voice-v2/client/send_text_message.html response = self._client.send_text_message( DestinationPhoneNumber=destinationNumber, - OriginationIdentity=self.pool_id, + OriginationIdentity=pool_id, MessageBody=content, MessageType=messageType, - Keyword=registeredKeyword ) # this will be true if the OriginationIdentity does not exist in pinpoint diff --git a/app/config.py b/app/config.py index b8f8f8a3ec..19e4aeef9e 100644 --- a/app/config.py +++ b/app/config.py @@ -264,6 +264,7 @@ class Config(object): AWS_SES_ACCESS_KEY = os.getenv("AWS_SES_ACCESS_KEY") AWS_SES_SECRET_KEY = os.getenv("AWS_SES_SECRET_KEY") AWS_PINPOINT_REGION = os.getenv("AWS_PINPOINT_REGION", "us-west-2") + AWS_PINPOINT_POOL_ID = os.getenv("AWS_PINPOINT_POOL_ID", None) AWS_US_TOLL_FREE_NUMBER = os.getenv("AWS_US_TOLL_FREE_NUMBER") CSV_UPLOAD_BUCKET_NAME = os.getenv("CSV_UPLOAD_BUCKET_NAME", "notification-alpha-canada-ca-csv-upload") ASSET_DOMAIN = os.getenv("ASSET_DOMAIN", "assets.notification.canada.ca") From b9d0d3a513e69116313180ff39eb143aa710a010 Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Tue, 9 Apr 2024 15:40:38 -0400 Subject: [PATCH 03/35] sending with pinpoint pool --- app/clients/sms/aws_pinpoint.py | 11 ++++------- app/delivery/send_to_providers.py | 2 -- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/app/clients/sms/aws_pinpoint.py b/app/clients/sms/aws_pinpoint.py index ce937d2a78..918e0b5937 100644 --- a/app/clients/sms/aws_pinpoint.py +++ b/app/clients/sms/aws_pinpoint.py @@ -12,7 +12,8 @@ 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(SmsClient, self).__init__(*args, **kwargs) + 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 @@ -41,10 +42,6 @@ def send_sms(self, to, content, reference, multi=True, sender=None): MessageType=messageType, ) - # this will be true if the OriginationIdentity does not exist in pinpoint - if response['MessageResponse']['Result'][destinationNumber]['StatusCode'] == 400: - self.statsd_client.incr("clients.pinpoint.error") - raise Exception(response['MessageResponse']['Result'][destinationNumber]['StatusMessage']) except ClientError as e: self.statsd_client.incr("clients.pinpoint.error") raise Exception(e) @@ -57,9 +54,9 @@ def send_sms(self, to, content, reference, multi=True, sender=None): self.statsd_client.timing("clients.pinpoint.request-time", elapsed_time) self.statsd_client.incr("clients.pinpoint.success") - return response['MessageResponse']['Result'][destinationNumber]['MessageId'] + return response['MessageId'] if not matched: self.statsd_client.incr("clients.pinpoint.error") self.current_app.logger.error("No valid numbers found in {}".format(to)) - raise ValueError("No valid numbers found for SMS delivery") \ No newline at end of file + raise ValueError("No valid numbers found for SMS delivery") diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index f5f94c571b..76b72d10fb 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -69,8 +69,6 @@ def send_sms_to_provider(notification): notification.international, notification.reply_to_text, ) - - template_dict = dao_get_template_by_id(notification.template_id, notification.template_version).__dict__ From 0fa34c1c640e12f9d77bc213c67bde3182bb6b6d Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Tue, 9 Apr 2024 16:25:38 -0400 Subject: [PATCH 04/35] use pinpoint pool for specified templates --- app/clients/sms/aws_pinpoint.py | 5 ++--- app/config.py | 1 + app/delivery/send_to_providers.py | 12 ++++++------ 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/app/clients/sms/aws_pinpoint.py b/app/clients/sms/aws_pinpoint.py index 918e0b5937..b2b5373e4c 100644 --- a/app/clients/sms/aws_pinpoint.py +++ b/app/clients/sms/aws_pinpoint.py @@ -26,7 +26,7 @@ def send_sms(self, to, content, reference, multi=True, sender=None): messageType = "TRANSACTIONAL" matched = False - for match in phonenumbers.PhoneNumberMatcher(to, "US"): + for match in phonenumbers.PhoneNumberMatcher(to, "US"): # SJA why is this a loop? matched = True to = phonenumbers.format_number(match.number, phonenumbers.PhoneNumberFormat.E164) destinationNumber = to @@ -53,8 +53,7 @@ def send_sms(self, to, content, reference, multi=True, sender=None): self.current_app.logger.info("AWS Pinpoint request finished in {}".format(elapsed_time)) self.statsd_client.timing("clients.pinpoint.request-time", elapsed_time) self.statsd_client.incr("clients.pinpoint.success") - - return response['MessageId'] + return response['MessageId'] if not matched: self.statsd_client.incr("clients.pinpoint.error") diff --git a/app/config.py b/app/config.py index 19e4aeef9e..dd374bcd55 100644 --- a/app/config.py +++ b/app/config.py @@ -265,6 +265,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_POOL_ID = os.getenv("AWS_PINPOINT_POOL_ID", None) + AWS_PINPOINT_TEMPLATE_IDS = env.list("AWS_PINPOINT_TEMPLATE_IDS", []) AWS_US_TOLL_FREE_NUMBER = os.getenv("AWS_US_TOLL_FREE_NUMBER") CSV_UPLOAD_BUCKET_NAME = os.getenv("CSV_UPLOAD_BUCKET_NAME", "notification-alpha-canada-ca-csv-upload") ASSET_DOMAIN = os.getenv("ASSET_DOMAIN", "assets.notification.canada.ca") diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 76b72d10fb..3eaded502c 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -68,6 +68,7 @@ def send_sms_to_provider(notification): notification.id, 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__ @@ -334,7 +335,10 @@ def update_notification_to_sending(notification, provider): dao_update_notification(notification) -def provider_to_use(notification_type, notification_id, international=False, sender=None): +def provider_to_use(notification_type, notification_id, international=False, sender=None, template_id=None): + if notification_type == SMS_TYPE and template_id is not None and str(template_id) in Config.AWS_PINPOINT_TEMPLATE_IDS: + return clients.get_client_by_name_and_type('pinpoint', SMS_TYPE) + active_providers_in_order = [ p for p in get_provider_details_by_notification_type(notification_type, international) if p.active ] @@ -343,11 +347,7 @@ def provider_to_use(notification_type, notification_id, international=False, sen current_app.logger.error("{} {} failed as no active providers".format(notification_type, notification_id)) raise Exception("No active {} providers".format(notification_type)) - # return clients.get_client_by_name_and_type(active_providers_in_order[0].identifier, notification_type) - - if notification_type == SMS_TYPE: - return clients.get_client_by_name_and_type('pinpoint', notification_type) - + return clients.get_client_by_name_and_type(active_providers_in_order[0].identifier, notification_type) def get_html_email_options(service: Service): From 7e6970687b0a20d9e74768303963bb05e3b952a3 Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Tue, 9 Apr 2024 16:28:30 -0400 Subject: [PATCH 05/35] format --- app/__init__.py | 2 +- app/clients/sms/aws_pinpoint.py | 24 +++++++++++++----------- app/delivery/send_to_providers.py | 4 ++-- app/models.py | 2 +- 4 files changed, 17 insertions(+), 15 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 65ff1b973a..c3c144620e 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -28,8 +28,8 @@ PerformancePlatformClient, ) from app.clients.salesforce.salesforce_client import SalesforceClient -from app.clients.sms.aws_sns import AwsSnsClient from app.clients.sms.aws_pinpoint import AwsPinpointClient +from app.clients.sms.aws_sns import AwsSnsClient from app.dbsetup import RoutingSQLAlchemy from app.encryption import CryptoSigner from app.json_provider import NotifyJSONProvider diff --git a/app/clients/sms/aws_pinpoint.py b/app/clients/sms/aws_pinpoint.py index b2b5373e4c..7c405aefa7 100644 --- a/app/clients/sms/aws_pinpoint.py +++ b/app/clients/sms/aws_pinpoint.py @@ -1,39 +1,41 @@ +from time import monotonic + import boto3 -from botocore.exceptions import ClientError import phonenumbers -from time import monotonic +from botocore.exceptions import ClientError + from app.clients.sms import SmsClient class AwsPinpointClient(SmsClient): - ''' + """ AWS Pinpoint SMS client - ''' + """ def init_app(self, current_app, statsd_client, *args, **kwargs): - self._client = boto3.client('pinpoint-sms-voice-v2', region_name="ca-central-1") + 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.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_POOL_ID'] + pool_id = self.current_app.config["AWS_PINPOINT_POOL_ID"] messageType = "TRANSACTIONAL" matched = False - for match in phonenumbers.PhoneNumberMatcher(to, "US"): # SJA why is this a loop? + for match in phonenumbers.PhoneNumberMatcher(to, "US"): # SJA why is this a loop? matched = True to = phonenumbers.format_number(match.number, phonenumbers.PhoneNumberFormat.E164) destinationNumber = to - + try: start_time = monotonic() - + # from https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/pinpoint-sms-voice-v2/client/send_text_message.html response = self._client.send_text_message( DestinationPhoneNumber=destinationNumber, @@ -53,7 +55,7 @@ def send_sms(self, to, content, reference, multi=True, sender=None): self.current_app.logger.info("AWS Pinpoint request finished in {}".format(elapsed_time)) self.statsd_client.timing("clients.pinpoint.request-time", elapsed_time) self.statsd_client.incr("clients.pinpoint.success") - return response['MessageId'] + return response["MessageId"] if not matched: self.statsd_client.incr("clients.pinpoint.error") diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 3eaded502c..bbef717397 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -337,8 +337,8 @@ def update_notification_to_sending(notification, provider): def provider_to_use(notification_type, notification_id, international=False, sender=None, template_id=None): if notification_type == SMS_TYPE and template_id is not None and str(template_id) in Config.AWS_PINPOINT_TEMPLATE_IDS: - return clients.get_client_by_name_and_type('pinpoint', SMS_TYPE) - + return clients.get_client_by_name_and_type("pinpoint", SMS_TYPE) + active_providers_in_order = [ p for p in get_provider_details_by_notification_type(notification_type, international) if p.active ] diff --git a/app/models.py b/app/models.py index f7b4c3924c..296850fe5c 100644 --- a/app/models.py +++ b/app/models.py @@ -1272,7 +1272,7 @@ def get_link(self): SNS_PROVIDER = "sns" -PINPOINT_PROVIDER = 'pinpoint' +PINPOINT_PROVIDER = "pinpoint" SES_PROVIDER = "ses" SMS_PROVIDERS = [SNS_PROVIDER, PINPOINT_PROVIDER] From b3d12e5bd54d01cfe64bfd638c5683bd2127cb59 Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Tue, 9 Apr 2024 16:34:37 -0400 Subject: [PATCH 06/35] tweak --- app/clients/sms/aws_pinpoint.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/clients/sms/aws_pinpoint.py b/app/clients/sms/aws_pinpoint.py index 7c405aefa7..22977c2c9a 100644 --- a/app/clients/sms/aws_pinpoint.py +++ b/app/clients/sms/aws_pinpoint.py @@ -55,7 +55,7 @@ def send_sms(self, to, content, reference, multi=True, sender=None): self.current_app.logger.info("AWS Pinpoint request finished in {}".format(elapsed_time)) self.statsd_client.timing("clients.pinpoint.request-time", elapsed_time) self.statsd_client.incr("clients.pinpoint.success") - return response["MessageId"] + return response["MessageId"] if not matched: self.statsd_client.incr("clients.pinpoint.error") From d18ce27d17d718d2afef1bcb628d1caa1ab4f9ac Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Thu, 11 Apr 2024 16:53:20 -0400 Subject: [PATCH 07/35] add configuration set --- app/clients/sms/aws_pinpoint.py | 1 + app/config.py | 1 + 2 files changed, 2 insertions(+) diff --git a/app/clients/sms/aws_pinpoint.py b/app/clients/sms/aws_pinpoint.py index 22977c2c9a..e287de36aa 100644 --- a/app/clients/sms/aws_pinpoint.py +++ b/app/clients/sms/aws_pinpoint.py @@ -42,6 +42,7 @@ def send_sms(self, to, content, reference, multi=True, sender=None): OriginationIdentity=pool_id, MessageBody=content, MessageType=messageType, + ConfigurationSetName=self.current_app.config["AWS_PINPOINT_CONFIGURATION_SET_NAME"], ) except ClientError as e: diff --git a/app/config.py b/app/config.py index dd374bcd55..4edaea4a41 100644 --- a/app/config.py +++ b/app/config.py @@ -265,6 +265,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_POOL_ID = os.getenv("AWS_PINPOINT_POOL_ID", None) + AWS_PINPOINT_CONFIGURATION_SET_NAME = os.getenv("AWS_PINPOINT_CONFIGURATION_SET_NAME", "pinpoint-configuration") AWS_PINPOINT_TEMPLATE_IDS = env.list("AWS_PINPOINT_TEMPLATE_IDS", []) AWS_US_TOLL_FREE_NUMBER = os.getenv("AWS_US_TOLL_FREE_NUMBER") CSV_UPLOAD_BUCKET_NAME = os.getenv("CSV_UPLOAD_BUCKET_NAME", "notification-alpha-canada-ca-csv-upload") From fceab9fcfc91f51624501e26fd33ec5f6ecc5396 Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Fri, 12 Apr 2024 14:32:27 -0400 Subject: [PATCH 08/35] add task for processing pinpoint receipts --- app/celery/process_pinpoint_receipts_tasks.py | 141 ++++++++++++++++++ 1 file changed, 141 insertions(+) create mode 100644 app/celery/process_pinpoint_receipts_tasks.py diff --git a/app/celery/process_pinpoint_receipts_tasks.py b/app/celery/process_pinpoint_receipts_tasks.py new file mode 100644 index 0000000000..3d533dc442 --- /dev/null +++ b/app/celery/process_pinpoint_receipts_tasks.py @@ -0,0 +1,141 @@ +from datetime import datetime + +from flask import current_app, json +from notifications_utils.statsd_decorators import statsd +from sqlalchemy.orm.exc import NoResultFound + +from app import notify_celery, statsd_client +from app.config import QueueNames +from app.dao import notifications_dao +from app.models import ( + NOTIFICATION_DELIVERED, + NOTIFICATION_PERMANENT_FAILURE, + NOTIFICATION_SENT, + NOTIFICATION_TECHNICAL_FAILURE, + NOTIFICATION_TEMPORARY_FAILURE, + PINPOINT_PROVIDER, +) +from app.notifications.callbacks import _check_and_queue_callback_task +from celery.exceptions import Retry + + +# Pinpoint receipts are of the form: +# { +# "eventType": "TEXT_DELIVERED", +# "eventVersion": "1.0", +# "eventTimestamp": 1712944268877, +# "isFinal": true, +# "originationPhoneNumber": "+13655550100", +# "destinationPhoneNumber": "+16135550123", +# "isoCountryCode": "CA", +# "mcc": "302", +# "mnc": "610", +# "carrierName": "Bell Cellular Inc. / Aliant Telecom", +# "messageId": "221bc70c-7ee6-4987-b1ba-9684ba25be20", +# "messageRequestTimestamp": 1712944267685, +# "messageEncoding": "GSM", +# "messageType": "TRANSACTIONAL", +# "messageStatus": "DELIVERED", +# "messageStatusDescription": "Message has been accepted by phone", +# "totalMessageParts": 1, +# "totalMessagePrice": 0.00581, +# "totalCarrierFee": 0.006 +# } + +@notify_celery.task(bind=True, name="process-pinpoint-result", max_retries=5, default_retry_delay=300) +@statsd(namespace="tasks") +def process_pinpoint_results(self, response): + try: + receipt = json.loads(response) + reference = receipt["messageId"] + status = receipt["messageStatus"] + provider_response = receipt["messageStatusDescription"] + + notification_status = determine_pinpoint_status(status, provider_response) + if not notification_status: + current_app.logger.warning(f"unhandled provider response for reference {reference}, received '{provider_response}'") + notification_status = NOTIFICATION_TECHNICAL_FAILURE # revert to tech failure by default + + try: + notification = notifications_dao.dao_get_notification_by_reference(reference) + except NoResultFound: + try: + current_app.logger.warning( + f"RETRY {self.request.retries}: notification not found for Pinpoint reference {reference} (update to {notification_status}). " + f"Callback may have arrived before notification was persisted to the DB. Adding task to retry queue" + ) + self.retry(queue=QueueNames.RETRY) + except self.MaxRetriesExceededError: + current_app.logger.warning( + f"notification not found for Pinpoint reference: {reference} (update to {notification_status}). Giving up." + ) + return + if notification.sent_by != PINPOINT_PROVIDER: + current_app.logger.exception(f"Pinpoint callback handled notification {notification.id} not sent by Pinpoint") + return + + if notification.status != NOTIFICATION_SENT: + notifications_dao._duplicate_update_warning(notification, notification_status) + return + + notifications_dao._update_notification_status( + notification=notification, + status=notification_status, + provider_response=provider_response, + ) + + if notification_status != NOTIFICATION_DELIVERED: + current_app.logger.info( + ( + f"Pinpoint delivery failed: notification id {notification.id} and reference {reference} has error found. " + f"Provider response: {provider_response}" + ) + ) + else: + current_app.logger.info(f"Pinpoint callback return status of {notification_status} for notification: {notification.id}") + + statsd_client.incr(f"callback.sns.{notification_status}") # TODO: do we want a Pinpoint metric here? + + if notification.sent_at: + statsd_client.timing_with_dates("callback.sns.elapsed-time", datetime.utcnow(), notification.sent_at) # TODO: do we want a Pinpoint metric here? + + _check_and_queue_callback_task(notification) + + return True + + except Retry: + raise + + except Exception as e: + current_app.logger.exception(f"Error processing Pinpoint results: {str(e)}") + self.retry(queue=QueueNames.RETRY) + + +def determine_pinpoint_status(sns_status, provider_response): + if sns_status == "DELIVERED": + return NOTIFICATION_DELIVERED + + # See all the possible provider responses + # https://docs.aws.amazon.com/sns/latest/dg/sms_stats_cloudwatch.html#sms_stats_delivery_fail_reasons + reasons = { + "Blocked as spam by phone carrier": NOTIFICATION_TECHNICAL_FAILURE, + "Destination is on a blocked list": NOTIFICATION_TECHNICAL_FAILURE, + "Invalid phone number": NOTIFICATION_TECHNICAL_FAILURE, + "Message body is invalid": NOTIFICATION_TECHNICAL_FAILURE, + "Phone carrier has blocked this message": NOTIFICATION_TECHNICAL_FAILURE, + "Phone carrier is currently unreachable/unavailable": NOTIFICATION_TEMPORARY_FAILURE, + "Phone has blocked SMS": NOTIFICATION_TECHNICAL_FAILURE, + "Phone is on a blocked list": NOTIFICATION_TECHNICAL_FAILURE, + "Phone is currently unreachable/unavailable": NOTIFICATION_PERMANENT_FAILURE, + "Phone number is opted out": NOTIFICATION_PERMANENT_FAILURE, + "This delivery would exceed max price": NOTIFICATION_TECHNICAL_FAILURE, + "Unknown error attempting to reach phone": NOTIFICATION_TECHNICAL_FAILURE, + } + + status = reasons.get(provider_response) # could be None + if not status: + # TODO: Pattern matching in Python 3.10 should simplify this overall function logic. + if "is opted out" in provider_response: + return NOTIFICATION_PERMANENT_FAILURE + + return status From b8ceb91e23663373bdae3708e595b9ba4c72a890 Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Fri, 12 Apr 2024 14:53:25 -0400 Subject: [PATCH 09/35] add mocks for pinpoint receipt task testing --- app/aws/mocks.py | 68 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/app/aws/mocks.py b/app/aws/mocks.py index 46c6f5fe10..11d619d823 100644 --- a/app/aws/mocks.py +++ b/app/aws/mocks.py @@ -192,6 +192,58 @@ def sns_failed_callback(provider_response, reference=None, timestamp="2016-06-28 return _sns_callback(body) +# Note that 1467074434 = 2016-06-28 00:40:34.558 UTC +def pinpoint_success_callback(reference=None, timestamp=1467074434, destination="+1XXX5550100"): + body = { + "eventType": "TEXT_DELIVERED", + "eventVersion": "1.0", + "eventTimestamp": timestamp, + "isFinal": True, + "originationPhoneNumber": "+13655550100", + "destinationPhoneNumber": destination, + "isoCountryCode": "CA", + "mcc": "302", + "mnc": "610", + "carrierName": "Bell Cellular Inc. / Aliant Telecom", + "messageId": reference, + "messageRequestTimestamp": 1712944267685, + "messageEncoding": "GSM", + "messageType": "TRANSACTIONAL", + "messageStatus": "DELIVERED", + "messageStatusDescription": "Message has been accepted by phone", + "totalMessageParts": 1, + "totalMessagePrice": 0.00581, + "totalCarrierFee": 0.006 + } + + return _pinpoint_callback(body) + + +# Note that 1467074434 = 2016-06-28 00:40:34.558 UTC +def pinpoint_failed_callback(provider_response, reference=None, timestamp=1467074434, destination="+1XXX5550100"): + + body = { + "eventType": "TEXT_CARRIER_UNREACHABLE", + "eventVersion": "1.0", + "eventTimestamp": timestamp, + "isFinal": True, + "originationPhoneNumber": "+13655362471", + "destinationPhoneNumber": destination, + "isoCountryCode": "CA", + "messageId": reference, + "messageRequestTimestamp": 1712944592827, + "messageEncoding": "GSM", + "messageType": "TRANSACTIONAL", + "messageStatus": "CARRIER_UNREACHABLE", + "messageStatusDescription": provider_response, + "totalMessageParts": 1, + "totalMessagePrice": 0.00581, + "totalCarrierFee": 0.006 + } + + return _pinpoint_callback(body) + + def _ses_bounce_callback(reference, bounce_type, bounce_subtype=None): ses_message_body = { "bounce": { @@ -267,3 +319,19 @@ def _sns_callback(body): "UnsubscribeUrl": "https://sns.ca-central-1.amazonaws.com/?Action=Unsubscribe&SubscriptionArn=[REACTED]", "MessageAttributes": {}, } + +# TODO: can we just use the _sns_callback() function instead of this one? +def _pinpoint_callback(body): + return { + "Type": "Notification", + "MessageId": "8e83c020-1234-1234-1234-92a8ee9baa0a", + "TopicArn": "arn:aws:sns:ca-central-1:12341234:ses_notifications", + "Subject": None, + "Message": json.dumps(body), + "Timestamp": "2017-11-17T12:14:03.710Z", + "SignatureVersion": "1", + "Signature": "[REDACTED]", + "SigningCertUrl": "https://sns.ca-central-1.amazonaws.com/SimpleNotificationService-[REDACTED].pem", + "UnsubscribeUrl": "https://sns.ca-central-1.amazonaws.com/?Action=Unsubscribe&SubscriptionArn=[REACTED]", + "MessageAttributes": {}, + } \ No newline at end of file From 1a6ba9e3f7b2f0ade5d6ad512603db3de9e4dd01 Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Fri, 12 Apr 2024 14:58:44 -0400 Subject: [PATCH 10/35] rough in test_process_pinpoint_receipts_tasks.py --- .../test_process_pinpoint_receipts_tasks.py | 191 ++++++++++++++++++ 1 file changed, 191 insertions(+) create mode 100644 tests/app/celery/test_process_pinpoint_receipts_tasks.py diff --git a/tests/app/celery/test_process_pinpoint_receipts_tasks.py b/tests/app/celery/test_process_pinpoint_receipts_tasks.py new file mode 100644 index 0000000000..8e19e69300 --- /dev/null +++ b/tests/app/celery/test_process_pinpoint_receipts_tasks.py @@ -0,0 +1,191 @@ +from datetime import datetime + +import pytest +from freezegun import freeze_time + +from app import statsd_client +from app.aws.mocks import pinpoint_failed_callback, pinpoint_success_callback +from app.celery.process_pinpoint_receipts_tasks import process_pinpoint_results +from app.dao.notifications_dao import get_notification_by_id +from app.models import ( + NOTIFICATION_DELIVERED, + NOTIFICATION_PERMANENT_FAILURE, + NOTIFICATION_SENT, + NOTIFICATION_TECHNICAL_FAILURE, + NOTIFICATION_TEMPORARY_FAILURE, +) +from app.notifications.callbacks import create_delivery_status_callback_data +from celery.exceptions import MaxRetriesExceededError +from tests.app.conftest import create_sample_notification +from tests.app.db import ( + create_notification, + create_service_callback_api, + save_notification, +) + + +def test_process_pinpoint_results_delivered(sample_template, notify_db, notify_db_session, mocker): + mock_logger = mocker.patch("app.celery.process_pinpoint_receipts_tasks.current_app.logger.info") + + notification = create_sample_notification( + notify_db, + notify_db_session, + template=sample_template, + reference="ref", + status=NOTIFICATION_SENT, + sent_by="pinpoint", + sent_at=datetime.utcnow(), + ) + assert get_notification_by_id(notification.id).status == NOTIFICATION_SENT + assert process_pinpoint_results(pinpoint_success_callback(reference="ref")) + assert get_notification_by_id(notification.id).status == NOTIFICATION_DELIVERED + assert get_notification_by_id(notification.id).provider_response == "Message has been accepted by phone carrier" + + mock_logger.assert_called_once_with(f"Pinpoint callback return status of delivered for notification: {notification.id}") + + +@pytest.mark.parametrize( + "provider_response, expected_status, should_log_warning, should_save_provider_response", + [ + ( + "Blocked as spam by phone carrier", + NOTIFICATION_TECHNICAL_FAILURE, + False, + True, + ), + ( + "Phone carrier is currently unreachable/unavailable", + NOTIFICATION_TEMPORARY_FAILURE, + False, + True, + ), + ( + "Phone is currently unreachable/unavailable", + NOTIFICATION_PERMANENT_FAILURE, + False, + True, + ), + ("This is not a real response", NOTIFICATION_TECHNICAL_FAILURE, True, True), + ], +) +def test_process_pinpoint_results_failed( + sample_template, + notify_db, + notify_db_session, + mocker, + provider_response, + expected_status, + should_log_warning, + should_save_provider_response, +): + mock_logger = mocker.patch("app.celery.process_pinpoint_receipts_tasks.current_app.logger.info") + mock_warning_logger = mocker.patch("app.celery.process_pinpoint_receipts_tasks.current_app.logger.warning") + + notification = create_sample_notification( + notify_db, + notify_db_session, + template=sample_template, + reference="ref", + status=NOTIFICATION_SENT, + sent_by="pinpoint", + sent_at=datetime.utcnow(), + ) + assert get_notification_by_id(notification.id).status == NOTIFICATION_SENT + assert process_pinpoint_results(pinpoint_failed_callback(provider_response=provider_response, reference="ref")) + assert get_notification_by_id(notification.id).status == expected_status + + if should_save_provider_response: + assert get_notification_by_id(notification.id).provider_response == provider_response + else: + assert get_notification_by_id(notification.id).provider_response is None + + mock_logger.assert_called_once_with( + ( + f"Pinpoint delivery failed: notification id {notification.id} and reference ref has error found. " + f"Provider response: {provider_response}" + ) + ) + + assert mock_warning_logger.call_count == int(should_log_warning) + + +def test_pinpoint_callback_should_retry_if_notification_is_missing(notify_db, mocker): + mock_retry = mocker.patch("app.celery.process_pinpoint_receipts_tasks.process_pinpoint_results.retry") + assert process_pinpoint_results(pinpoint_success_callback(reference="ref")) is None + assert mock_retry.call_count == 1 + + +def test_pinpoint_callback_should_give_up_after_max_tries(notify_db, mocker): + mocker.patch( + "app.celery.process_pinpoint_receipts_tasks.process_pinpoint_results.retry", + side_effect=MaxRetriesExceededError, + ) + mock_logger = mocker.patch("app.celery.process_pinpoint_receipts_tasks.current_app.logger.warning") + + assert process_pinpoint_results(pinpoint_success_callback(reference="ref")) is None + mock_logger.assert_called_with("notification not found for Pinpoint reference: ref (update to delivered). Giving up.") + + +def test_process_pinpoint_results_retry_called(sample_template, mocker): + save_notification( + create_notification( + sample_template, + reference="ref1", + sent_at=datetime.utcnow(), + status=NOTIFICATION_SENT, + sent_by="pinpoint", + ) + ) + + mocker.patch( + "app.dao.notifications_dao._update_notification_status", + side_effect=Exception("EXPECTED"), + ) + mocked = mocker.patch("app.celery.process_pinpoint_receipts_tasks.process_pinpoint_results.retry") + process_pinpoint_results(response=pinpoint_success_callback(reference="ref1")) + assert mocked.call_count == 1 + + +def test_process_pinpoint_results_does_not_process_other_providers(sample_template, mocker): + mock_logger = mocker.patch("app.celery.process_pinpoint_receipts_tasks.current_app.logger.exception") + mock_dao = mocker.patch("app.dao.notifications_dao._update_notification_status") + save_notification( + create_notification( + sample_template, + reference="ref1", + sent_at=datetime.utcnow(), + status=NOTIFICATION_SENT, + sent_by="pinpoint", + ) + ) + + process_pinpoint_results(response=pinpoint_success_callback(reference="ref1")) is None + assert mock_logger.called_once_with("") + assert not mock_dao.called + + +def test_process_pinpoint_results_calls_service_callback(sample_template, notify_db_session, notify_db, mocker): + with freeze_time("2021-01-01T12:00:00"): + mocker.patch("app.statsd_client.incr") + mocker.patch("app.statsd_client.timing_with_dates") + send_mock = mocker.patch("app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async") + notification = create_sample_notification( + notify_db, + notify_db_session, + template=sample_template, + reference="ref", + status=NOTIFICATION_SENT, + sent_by="pinpoint", + sent_at=datetime.utcnow(), + ) + callback_api = create_service_callback_api(service=sample_template.service, url="https://example.com") + assert get_notification_by_id(notification.id).status == NOTIFICATION_SENT + + assert process_pinpoint_results(pinpoint_success_callback(reference="ref")) + assert get_notification_by_id(notification.id).status == NOTIFICATION_DELIVERED + assert get_notification_by_id(notification.id).provider_response == "Message has been accepted by phone carrier" + statsd_client.timing_with_dates.assert_any_call("callback.pinpoint.elapsed-time", datetime.utcnow(), notification.sent_at) + statsd_client.incr.assert_any_call("callback.pinpoint.delivered") + updated_notification = get_notification_by_id(notification.id) + signed_data = create_delivery_status_callback_data(updated_notification, callback_api) + send_mock.assert_called_once_with([str(notification.id), signed_data], queue="service-callbacks") From 6e3d339564772d423c7264b244fdffc29fbd6da9 Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Fri, 12 Apr 2024 15:12:52 -0400 Subject: [PATCH 11/35] make tests pass --- app/aws/mocks.py | 14 +++++++------- app/celery/process_pinpoint_receipts_tasks.py | 14 +++++++++----- .../celery/test_process_pinpoint_receipts_tasks.py | 10 +++++----- 3 files changed, 21 insertions(+), 17 deletions(-) diff --git a/app/aws/mocks.py b/app/aws/mocks.py index 11d619d823..62d6c21c0c 100644 --- a/app/aws/mocks.py +++ b/app/aws/mocks.py @@ -206,14 +206,14 @@ def pinpoint_success_callback(reference=None, timestamp=1467074434, destination= "mnc": "610", "carrierName": "Bell Cellular Inc. / Aliant Telecom", "messageId": reference, - "messageRequestTimestamp": 1712944267685, + "messageRequestTimestamp": timestamp, "messageEncoding": "GSM", "messageType": "TRANSACTIONAL", "messageStatus": "DELIVERED", "messageStatusDescription": "Message has been accepted by phone", "totalMessageParts": 1, "totalMessagePrice": 0.00581, - "totalCarrierFee": 0.006 + "totalCarrierFee": 0.006, } return _pinpoint_callback(body) @@ -221,24 +221,23 @@ def pinpoint_success_callback(reference=None, timestamp=1467074434, destination= # Note that 1467074434 = 2016-06-28 00:40:34.558 UTC def pinpoint_failed_callback(provider_response, reference=None, timestamp=1467074434, destination="+1XXX5550100"): - body = { "eventType": "TEXT_CARRIER_UNREACHABLE", "eventVersion": "1.0", "eventTimestamp": timestamp, "isFinal": True, - "originationPhoneNumber": "+13655362471", + "originationPhoneNumber": "+13655550100", "destinationPhoneNumber": destination, "isoCountryCode": "CA", "messageId": reference, - "messageRequestTimestamp": 1712944592827, + "messageRequestTimestamp": timestamp, "messageEncoding": "GSM", "messageType": "TRANSACTIONAL", "messageStatus": "CARRIER_UNREACHABLE", "messageStatusDescription": provider_response, "totalMessageParts": 1, "totalMessagePrice": 0.00581, - "totalCarrierFee": 0.006 + "totalCarrierFee": 0.006, } return _pinpoint_callback(body) @@ -320,6 +319,7 @@ def _sns_callback(body): "MessageAttributes": {}, } + # TODO: can we just use the _sns_callback() function instead of this one? def _pinpoint_callback(body): return { @@ -334,4 +334,4 @@ def _pinpoint_callback(body): "SigningCertUrl": "https://sns.ca-central-1.amazonaws.com/SimpleNotificationService-[REDACTED].pem", "UnsubscribeUrl": "https://sns.ca-central-1.amazonaws.com/?Action=Unsubscribe&SubscriptionArn=[REACTED]", "MessageAttributes": {}, - } \ No newline at end of file + } diff --git a/app/celery/process_pinpoint_receipts_tasks.py b/app/celery/process_pinpoint_receipts_tasks.py index 3d533dc442..21410bdcc8 100644 --- a/app/celery/process_pinpoint_receipts_tasks.py +++ b/app/celery/process_pinpoint_receipts_tasks.py @@ -18,8 +18,7 @@ from app.notifications.callbacks import _check_and_queue_callback_task from celery.exceptions import Retry - -# Pinpoint receipts are of the form: +# Pinpoint receipts are of the form: # { # "eventType": "TEXT_DELIVERED", # "eventVersion": "1.0", @@ -42,11 +41,12 @@ # "totalCarrierFee": 0.006 # } + @notify_celery.task(bind=True, name="process-pinpoint-result", max_retries=5, default_retry_delay=300) @statsd(namespace="tasks") def process_pinpoint_results(self, response): try: - receipt = json.loads(response) + receipt = json.loads(response["Message"]) reference = receipt["messageId"] status = receipt["messageStatus"] provider_response = receipt["messageStatusDescription"] @@ -92,12 +92,16 @@ def process_pinpoint_results(self, response): ) ) else: - current_app.logger.info(f"Pinpoint callback return status of {notification_status} for notification: {notification.id}") + current_app.logger.info( + f"Pinpoint callback return status of {notification_status} for notification: {notification.id}" + ) statsd_client.incr(f"callback.sns.{notification_status}") # TODO: do we want a Pinpoint metric here? if notification.sent_at: - statsd_client.timing_with_dates("callback.sns.elapsed-time", datetime.utcnow(), notification.sent_at) # TODO: do we want a Pinpoint metric here? + statsd_client.timing_with_dates( + "callback.sns.elapsed-time", datetime.utcnow(), notification.sent_at + ) # TODO: do we want a Pinpoint metric here? _check_and_queue_callback_task(notification) diff --git a/tests/app/celery/test_process_pinpoint_receipts_tasks.py b/tests/app/celery/test_process_pinpoint_receipts_tasks.py index 8e19e69300..e60180a0c9 100644 --- a/tests/app/celery/test_process_pinpoint_receipts_tasks.py +++ b/tests/app/celery/test_process_pinpoint_receipts_tasks.py @@ -39,7 +39,7 @@ def test_process_pinpoint_results_delivered(sample_template, notify_db, notify_d assert get_notification_by_id(notification.id).status == NOTIFICATION_SENT assert process_pinpoint_results(pinpoint_success_callback(reference="ref")) assert get_notification_by_id(notification.id).status == NOTIFICATION_DELIVERED - assert get_notification_by_id(notification.id).provider_response == "Message has been accepted by phone carrier" + assert get_notification_by_id(notification.id).provider_response == "Message has been accepted by phone" mock_logger.assert_called_once_with(f"Pinpoint callback return status of delivered for notification: {notification.id}") @@ -155,7 +155,7 @@ def test_process_pinpoint_results_does_not_process_other_providers(sample_templa reference="ref1", sent_at=datetime.utcnow(), status=NOTIFICATION_SENT, - sent_by="pinpoint", + sent_by="sns", ) ) @@ -183,9 +183,9 @@ def test_process_pinpoint_results_calls_service_callback(sample_template, notify assert process_pinpoint_results(pinpoint_success_callback(reference="ref")) assert get_notification_by_id(notification.id).status == NOTIFICATION_DELIVERED - assert get_notification_by_id(notification.id).provider_response == "Message has been accepted by phone carrier" - statsd_client.timing_with_dates.assert_any_call("callback.pinpoint.elapsed-time", datetime.utcnow(), notification.sent_at) - statsd_client.incr.assert_any_call("callback.pinpoint.delivered") + assert get_notification_by_id(notification.id).provider_response == "Message has been accepted by phone" + statsd_client.timing_with_dates.assert_any_call("callback.sns.elapsed-time", datetime.utcnow(), notification.sent_at) + statsd_client.incr.assert_any_call("callback.sns.delivered") updated_notification = get_notification_by_id(notification.id) signed_data = create_delivery_status_callback_data(updated_notification, callback_api) send_mock.assert_called_once_with([str(notification.id), signed_data], queue="service-callbacks") From ca2da8c33e36780aec7056a9360c817d8f1c66d6 Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Fri, 12 Apr 2024 15:24:38 -0400 Subject: [PATCH 12/35] add explicit return --- app/celery/process_pinpoint_receipts_tasks.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/celery/process_pinpoint_receipts_tasks.py b/app/celery/process_pinpoint_receipts_tasks.py index 21410bdcc8..ca0cc92cd5 100644 --- a/app/celery/process_pinpoint_receipts_tasks.py +++ b/app/celery/process_pinpoint_receipts_tasks.py @@ -114,6 +114,8 @@ def process_pinpoint_results(self, response): current_app.logger.exception(f"Error processing Pinpoint results: {str(e)}") self.retry(queue=QueueNames.RETRY) + return True + def determine_pinpoint_status(sns_status, provider_response): if sns_status == "DELIVERED": From 3db48e8f173f6a6525bd42fc92f1bb73792937c7 Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Thu, 18 Apr 2024 13:59:40 -0400 Subject: [PATCH 13/35] tweak --- app/aws/mocks.py | 1 - app/celery/process_pinpoint_receipts_tasks.py | 2 -- 2 files changed, 3 deletions(-) diff --git a/app/aws/mocks.py b/app/aws/mocks.py index 62d6c21c0c..19aa7a4108 100644 --- a/app/aws/mocks.py +++ b/app/aws/mocks.py @@ -320,7 +320,6 @@ def _sns_callback(body): } -# TODO: can we just use the _sns_callback() function instead of this one? def _pinpoint_callback(body): return { "Type": "Notification", diff --git a/app/celery/process_pinpoint_receipts_tasks.py b/app/celery/process_pinpoint_receipts_tasks.py index ca0cc92cd5..21410bdcc8 100644 --- a/app/celery/process_pinpoint_receipts_tasks.py +++ b/app/celery/process_pinpoint_receipts_tasks.py @@ -114,8 +114,6 @@ def process_pinpoint_results(self, response): current_app.logger.exception(f"Error processing Pinpoint results: {str(e)}") self.retry(queue=QueueNames.RETRY) - return True - def determine_pinpoint_status(sns_status, provider_response): if sns_status == "DELIVERED": From d2e8471dffdd32af7db4ad3ea4f351bc07fe4284 Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Mon, 22 Apr 2024 15:20:47 -0400 Subject: [PATCH 14/35] tweak --- app/clients/sms/aws_pinpoint.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/app/clients/sms/aws_pinpoint.py b/app/clients/sms/aws_pinpoint.py index e287de36aa..567af18a22 100644 --- a/app/clients/sms/aws_pinpoint.py +++ b/app/clients/sms/aws_pinpoint.py @@ -28,15 +28,13 @@ def send_sms(self, to, content, reference, multi=True, sender=None): messageType = "TRANSACTIONAL" matched = False - for match in phonenumbers.PhoneNumberMatcher(to, "US"): # SJA why is this a loop? + for match in phonenumbers.PhoneNumberMatcher(to, "US"): matched = True to = phonenumbers.format_number(match.number, phonenumbers.PhoneNumberFormat.E164) destinationNumber = to try: start_time = monotonic() - - # from https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/pinpoint-sms-voice-v2/client/send_text_message.html response = self._client.send_text_message( DestinationPhoneNumber=destinationNumber, OriginationIdentity=pool_id, @@ -44,7 +42,6 @@ def send_sms(self, to, content, reference, multi=True, sender=None): MessageType=messageType, ConfigurationSetName=self.current_app.config["AWS_PINPOINT_CONFIGURATION_SET_NAME"], ) - except ClientError as e: self.statsd_client.incr("clients.pinpoint.error") raise Exception(e) From 6c090ceb592f79b38ef904403eaf380d6056a54a Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Mon, 22 Apr 2024 17:11:07 -0400 Subject: [PATCH 15/35] working now --- app/delivery/send_to_providers.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index bbef717397..69f440697a 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -46,6 +46,7 @@ NOTIFICATION_SENT, NOTIFICATION_TECHNICAL_FAILURE, NOTIFICATION_VIRUS_SCAN_FAILED, + PINPOINT_PROVIDER, SMS_TYPE, BounceRateStatus, Notification, @@ -340,7 +341,9 @@ def provider_to_use(notification_type, notification_id, international=False, sen return clients.get_client_by_name_and_type("pinpoint", SMS_TYPE) active_providers_in_order = [ - p for p in get_provider_details_by_notification_type(notification_type, international) if p.active + p + for p in get_provider_details_by_notification_type(notification_type, international) + if p.active and p.identifier != PINPOINT_PROVIDER ] if not active_providers_in_order: From 3f59f2773a47dbe85cbb69b3632497b656cde96d Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Tue, 23 Apr 2024 10:07:49 -0400 Subject: [PATCH 16/35] add new env vars to .env.examples --- .env.example | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.env.example b/.env.example index 8e60a9b5ae..e45f751415 100644 --- a/.env.example +++ b/.env.example @@ -19,3 +19,6 @@ AWS_PINPOINT_REGION=us-west-2 AWS_EMF_ENVIRONMENT=local CONTACT_FORM_EMAIL_ADDRESS = "" + +AWS_PINPOINT_POOL_ID= +AWS_PINPOINT_TEMPLATE_IDS= From 646b86d6763a84c51eb444c58c188721d52e9e10 Mon Sep 17 00:00:00 2001 From: Steve Astels Date: Wed, 24 Apr 2024 15:28:22 -0400 Subject: [PATCH 17/35] Update .env.example Co-authored-by: Jimmy Royer --- .env.example | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.env.example b/.env.example index e45f751415..a21fa645a6 100644 --- a/.env.example +++ b/.env.example @@ -21,4 +21,4 @@ AWS_EMF_ENVIRONMENT=local CONTACT_FORM_EMAIL_ADDRESS = "" AWS_PINPOINT_POOL_ID= -AWS_PINPOINT_TEMPLATE_IDS= +AWS_PINPOINT_SC_TEMPLATE_IDS= From bde0f689c8c88f04b78d38f350ccf5d1002eb5a2 Mon Sep 17 00:00:00 2001 From: Steve Astels Date: Wed, 24 Apr 2024 15:28:31 -0400 Subject: [PATCH 18/35] Update .env.example Co-authored-by: Jimmy Royer --- .env.example | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.env.example b/.env.example index a21fa645a6..cb36eefda5 100644 --- a/.env.example +++ b/.env.example @@ -20,5 +20,5 @@ AWS_EMF_ENVIRONMENT=local CONTACT_FORM_EMAIL_ADDRESS = "" -AWS_PINPOINT_POOL_ID= +AWS_PINPOINT_SC_POOL_ID= AWS_PINPOINT_SC_TEMPLATE_IDS= From 12f539ff369c8a3a683163abd1903e30c9fd4daa Mon Sep 17 00:00:00 2001 From: Steve Astels Date: Wed, 24 Apr 2024 15:28:40 -0400 Subject: [PATCH 19/35] Update app/clients/sms/aws_pinpoint.py Co-authored-by: Jimmy Royer --- app/clients/sms/aws_pinpoint.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/clients/sms/aws_pinpoint.py b/app/clients/sms/aws_pinpoint.py index 567af18a22..3dff918c2a 100644 --- a/app/clients/sms/aws_pinpoint.py +++ b/app/clients/sms/aws_pinpoint.py @@ -24,7 +24,7 @@ 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_POOL_ID"] + pool_id = self.current_app.config["AWS_PINPOINT_SC_POOL_ID"] messageType = "TRANSACTIONAL" matched = False From 965fbff203b283e54d92837cf9ec27682e0e0a1c Mon Sep 17 00:00:00 2001 From: Steve Astels Date: Wed, 24 Apr 2024 15:28:48 -0400 Subject: [PATCH 20/35] Update app/config.py Co-authored-by: Jimmy Royer --- app/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/config.py b/app/config.py index 4cb1b575d7..0e91c892c9 100644 --- a/app/config.py +++ b/app/config.py @@ -268,7 +268,7 @@ class Config(object): AWS_PINPOINT_REGION = os.getenv("AWS_PINPOINT_REGION", "us-west-2") AWS_PINPOINT_POOL_ID = os.getenv("AWS_PINPOINT_POOL_ID", None) AWS_PINPOINT_CONFIGURATION_SET_NAME = os.getenv("AWS_PINPOINT_CONFIGURATION_SET_NAME", "pinpoint-configuration") - AWS_PINPOINT_TEMPLATE_IDS = env.list("AWS_PINPOINT_TEMPLATE_IDS", []) + AWS_PINPOINT_SC_TEMPLATE_IDS = env.list("AWS_PINPOINT_SC_TEMPLATE_IDS", []) AWS_US_TOLL_FREE_NUMBER = os.getenv("AWS_US_TOLL_FREE_NUMBER") CSV_UPLOAD_BUCKET_NAME = os.getenv("CSV_UPLOAD_BUCKET_NAME", "notification-alpha-canada-ca-csv-upload") ASSET_DOMAIN = os.getenv("ASSET_DOMAIN", "assets.notification.canada.ca") From 26d4419c8c8b2c82cd2cc472a9b8e83e267b3c86 Mon Sep 17 00:00:00 2001 From: Steve Astels Date: Wed, 24 Apr 2024 15:29:14 -0400 Subject: [PATCH 21/35] Update app/delivery/send_to_providers.py Co-authored-by: Jimmy Royer --- app/delivery/send_to_providers.py | 1 + 1 file changed, 1 insertion(+) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 69f440697a..197ee4abe1 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -337,6 +337,7 @@ def update_notification_to_sending(notification, provider): 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_TEMPLATE_IDS: return clients.get_client_by_name_and_type("pinpoint", SMS_TYPE) From 5710df24dfcfb4783315f0bf83c64be08f187cb1 Mon Sep 17 00:00:00 2001 From: Steve Astels Date: Wed, 24 Apr 2024 15:30:29 -0400 Subject: [PATCH 22/35] Update app/config.py Co-authored-by: Jimmy Royer --- app/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/config.py b/app/config.py index 0e91c892c9..b3c34feb7f 100644 --- a/app/config.py +++ b/app/config.py @@ -266,7 +266,7 @@ class Config(object): AWS_SES_ACCESS_KEY = os.getenv("AWS_SES_ACCESS_KEY") AWS_SES_SECRET_KEY = os.getenv("AWS_SES_SECRET_KEY") AWS_PINPOINT_REGION = os.getenv("AWS_PINPOINT_REGION", "us-west-2") - AWS_PINPOINT_POOL_ID = os.getenv("AWS_PINPOINT_POOL_ID", None) + AWS_PINPOINT_SC_POOL_ID = os.getenv("AWS_PINPOINT_SC_POOL_ID", None) 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") From efa0df993c58dc0f01e76d413045babdc5a7fadf Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Wed, 24 Apr 2024 16:55:01 -0400 Subject: [PATCH 23/35] add typing / docstring for determine_pinpoint_status --- app/celery/process_pinpoint_receipts_tasks.py | 22 ++++++++++++++----- app/delivery/send_to_providers.py | 2 +- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/app/celery/process_pinpoint_receipts_tasks.py b/app/celery/process_pinpoint_receipts_tasks.py index 21410bdcc8..0252024dac 100644 --- a/app/celery/process_pinpoint_receipts_tasks.py +++ b/app/celery/process_pinpoint_receipts_tasks.py @@ -1,4 +1,5 @@ from datetime import datetime +from typing import Literal, Union from flask import current_app, json from notifications_utils.statsd_decorators import statsd @@ -115,8 +116,19 @@ def process_pinpoint_results(self, response): self.retry(queue=QueueNames.RETRY) -def determine_pinpoint_status(sns_status, provider_response): - if sns_status == "DELIVERED": +def determine_pinpoint_status( + status: str, provider_response: str) -> Union[str, None]: + """Determine the notification status based on the SMS status and provider response. + + Args: + status (str): message status from AWS + provider_response (str): detailed status from the SMS provider + + Returns: + Union[str, None]: the notification status or None if the status is not handled + """ + + if status == "DELIVERED": return NOTIFICATION_DELIVERED # See all the possible provider responses @@ -136,10 +148,10 @@ def determine_pinpoint_status(sns_status, provider_response): "Unknown error attempting to reach phone": NOTIFICATION_TECHNICAL_FAILURE, } - status = reasons.get(provider_response) # could be None - if not status: + notification_status = reasons.get(provider_response) # could be None + if not notification_status: # TODO: Pattern matching in Python 3.10 should simplify this overall function logic. if "is opted out" in provider_response: return NOTIFICATION_PERMANENT_FAILURE - return status + return notification_status diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 197ee4abe1..a57b5c3238 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -338,7 +338,7 @@ def update_notification_to_sending(notification, provider): 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_TEMPLATE_IDS: + 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) active_providers_in_order = [ From d1612c2ad0065dec323810d72ef54d5afe8eb715 Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Thu, 25 Apr 2024 15:04:20 -0400 Subject: [PATCH 24/35] wip add pattern matching --- app/celery/process_pinpoint_receipts_tasks.py | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/app/celery/process_pinpoint_receipts_tasks.py b/app/celery/process_pinpoint_receipts_tasks.py index 0252024dac..462915704f 100644 --- a/app/celery/process_pinpoint_receipts_tasks.py +++ b/app/celery/process_pinpoint_receipts_tasks.py @@ -136,18 +136,27 @@ def determine_pinpoint_status( reasons = { "Blocked as spam by phone carrier": NOTIFICATION_TECHNICAL_FAILURE, "Destination is on a blocked list": NOTIFICATION_TECHNICAL_FAILURE, - "Invalid phone number": NOTIFICATION_TECHNICAL_FAILURE, - "Message body is invalid": NOTIFICATION_TECHNICAL_FAILURE, "Phone carrier has blocked this message": NOTIFICATION_TECHNICAL_FAILURE, - "Phone carrier is currently unreachable/unavailable": NOTIFICATION_TEMPORARY_FAILURE, "Phone has blocked SMS": NOTIFICATION_TECHNICAL_FAILURE, "Phone is on a blocked list": NOTIFICATION_TECHNICAL_FAILURE, + + "Invalid phone number": NOTIFICATION_TECHNICAL_FAILURE, + "Message body is invalid": NOTIFICATION_TECHNICAL_FAILURE, + + "Phone carrier is currently unreachable/unavailable": NOTIFICATION_TEMPORARY_FAILURE, "Phone is currently unreachable/unavailable": NOTIFICATION_PERMANENT_FAILURE, "Phone number is opted out": NOTIFICATION_PERMANENT_FAILURE, "This delivery would exceed max price": NOTIFICATION_TECHNICAL_FAILURE, "Unknown error attempting to reach phone": NOTIFICATION_TECHNICAL_FAILURE, } + match provider_response.lower().split(): + case [_, "blocked", _]: + return NOTIFICATION_TECHNICAL_FAILURE + case [_, "invalid", _]: + return NOTIFICATION_TECHNICAL_FAILURE + + notification_status = reasons.get(provider_response) # could be None if not notification_status: # TODO: Pattern matching in Python 3.10 should simplify this overall function logic. From c08cc149ee659f2bbb8dfb53de75b031b0a527de Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Thu, 25 Apr 2024 16:06:51 -0400 Subject: [PATCH 25/35] matchy-matchy --- app/celery/process_pinpoint_receipts_tasks.py | 45 +++++++------------ 1 file changed, 15 insertions(+), 30 deletions(-) diff --git a/app/celery/process_pinpoint_receipts_tasks.py b/app/celery/process_pinpoint_receipts_tasks.py index 462915704f..38733e3ca9 100644 --- a/app/celery/process_pinpoint_receipts_tasks.py +++ b/app/celery/process_pinpoint_receipts_tasks.py @@ -131,36 +131,21 @@ def determine_pinpoint_status( if status == "DELIVERED": return NOTIFICATION_DELIVERED - # See all the possible provider responses - # https://docs.aws.amazon.com/sns/latest/dg/sms_stats_cloudwatch.html#sms_stats_delivery_fail_reasons - reasons = { - "Blocked as spam by phone carrier": NOTIFICATION_TECHNICAL_FAILURE, - "Destination is on a blocked list": NOTIFICATION_TECHNICAL_FAILURE, - "Phone carrier has blocked this message": NOTIFICATION_TECHNICAL_FAILURE, - "Phone has blocked SMS": NOTIFICATION_TECHNICAL_FAILURE, - "Phone is on a blocked list": NOTIFICATION_TECHNICAL_FAILURE, - - "Invalid phone number": NOTIFICATION_TECHNICAL_FAILURE, - "Message body is invalid": NOTIFICATION_TECHNICAL_FAILURE, - - "Phone carrier is currently unreachable/unavailable": NOTIFICATION_TEMPORARY_FAILURE, - "Phone is currently unreachable/unavailable": NOTIFICATION_PERMANENT_FAILURE, - "Phone number is opted out": NOTIFICATION_PERMANENT_FAILURE, - "This delivery would exceed max price": NOTIFICATION_TECHNICAL_FAILURE, - "Unknown error attempting to reach phone": NOTIFICATION_TECHNICAL_FAILURE, - } - - match provider_response.lower().split(): - case [_, "blocked", _]: + response_lower = provider_response.lower() + match response_lower: + case response_lower if "blocked" in response_lower: return NOTIFICATION_TECHNICAL_FAILURE - case [_, "invalid", _]: + case response_lower if "invalid" in response_lower: return NOTIFICATION_TECHNICAL_FAILURE - - - notification_status = reasons.get(provider_response) # could be None - if not notification_status: - # TODO: Pattern matching in Python 3.10 should simplify this overall function logic. - if "is opted out" in provider_response: + case response_lower if "is opted out" in response_lower: return NOTIFICATION_PERMANENT_FAILURE - - return notification_status + case response_lower if "unknown error" in response_lower: + return NOTIFICATION_TECHNICAL_FAILURE + case response_lower if "exceed max price" in response_lower: + return NOTIFICATION_TECHNICAL_FAILURE + case "Phone carrier is currently unreachable/unavailable": + return NOTIFICATION_TEMPORARY_FAILURE + case "Phone is currently unreachable/unavailable": + return NOTIFICATION_PERMANENT_FAILURE + case _: + return None From e205bc1583b2c4f88624f4e330f3344baaf8e3e8 Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Mon, 6 May 2024 10:18:38 -0400 Subject: [PATCH 26/35] formatting --- app/celery/process_pinpoint_receipts_tasks.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/app/celery/process_pinpoint_receipts_tasks.py b/app/celery/process_pinpoint_receipts_tasks.py index 38733e3ca9..13fbc83341 100644 --- a/app/celery/process_pinpoint_receipts_tasks.py +++ b/app/celery/process_pinpoint_receipts_tasks.py @@ -1,5 +1,5 @@ from datetime import datetime -from typing import Literal, Union +from typing import Union from flask import current_app, json from notifications_utils.statsd_decorators import statsd @@ -116,8 +116,7 @@ def process_pinpoint_results(self, response): self.retry(queue=QueueNames.RETRY) -def determine_pinpoint_status( - status: str, provider_response: str) -> Union[str, None]: +def determine_pinpoint_status(status: str, provider_response: str) -> Union[str, None]: """Determine the notification status based on the SMS status and provider response. Args: @@ -127,7 +126,7 @@ def determine_pinpoint_status( Returns: Union[str, None]: the notification status or None if the status is not handled """ - + if status == "DELIVERED": return NOTIFICATION_DELIVERED @@ -142,10 +141,10 @@ def determine_pinpoint_status( case response_lower if "unknown error" in response_lower: return NOTIFICATION_TECHNICAL_FAILURE case response_lower if "exceed max price" in response_lower: - return NOTIFICATION_TECHNICAL_FAILURE + return NOTIFICATION_TECHNICAL_FAILURE case "Phone carrier is currently unreachable/unavailable": return NOTIFICATION_TEMPORARY_FAILURE case "Phone is currently unreachable/unavailable": return NOTIFICATION_PERMANENT_FAILURE - case _: - return None + + return None From 41e7eaa670ec78d38536367e14d6ed24e446683f Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Mon, 6 May 2024 10:26:20 -0400 Subject: [PATCH 27/35] add if/else alternative to matching --- app/celery/process_pinpoint_receipts_tasks.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/app/celery/process_pinpoint_receipts_tasks.py b/app/celery/process_pinpoint_receipts_tasks.py index 13fbc83341..b3e0edac52 100644 --- a/app/celery/process_pinpoint_receipts_tasks.py +++ b/app/celery/process_pinpoint_receipts_tasks.py @@ -148,3 +148,22 @@ def determine_pinpoint_status(status: str, provider_response: str) -> Union[str, return NOTIFICATION_PERMANENT_FAILURE return None + + # Alternative: + # + # if "blocked" in response_lower: + # return NOTIFICATION_TECHNICAL_FAILURE + # elif "invalid" in response_lower: + # return NOTIFICATION_TECHNICAL_FAILURE + # elif "is opted out" in response_lower: + # return NOTIFICATION_PERMANENT_FAILURE + # elif "unknown error" in response_lower: + # return NOTIFICATION_TECHNICAL_FAILURE + # elif "exceed max price" in response_lower: + # return NOTIFICATION_TECHNICAL_FAILURE + # elif "Phone carrier is currently unreachable/unavailable" in response_lower: + # return NOTIFICATION_TEMPORARY_FAILURE + # elif "Phone is currently unreachable/unavailable" in response_lower: + # return NOTIFICATION_PERMANENT_FAILURE + # else: + # return None From 40e4c87a56007b95aa2c0c8fa1c628b135da6e7a Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Mon, 6 May 2024 13:11:03 -0400 Subject: [PATCH 28/35] fix case --- app/celery/process_pinpoint_receipts_tasks.py | 22 ++----------------- 1 file changed, 2 insertions(+), 20 deletions(-) diff --git a/app/celery/process_pinpoint_receipts_tasks.py b/app/celery/process_pinpoint_receipts_tasks.py index b3e0edac52..277e041b41 100644 --- a/app/celery/process_pinpoint_receipts_tasks.py +++ b/app/celery/process_pinpoint_receipts_tasks.py @@ -142,28 +142,10 @@ def determine_pinpoint_status(status: str, provider_response: str) -> Union[str, return NOTIFICATION_TECHNICAL_FAILURE case response_lower if "exceed max price" in response_lower: return NOTIFICATION_TECHNICAL_FAILURE - case "Phone carrier is currently unreachable/unavailable": + case "phone carrier is currently unreachable/unavailable": return NOTIFICATION_TEMPORARY_FAILURE - case "Phone is currently unreachable/unavailable": + case "phone is currently unreachable/unavailable": return NOTIFICATION_PERMANENT_FAILURE return None - # Alternative: - # - # if "blocked" in response_lower: - # return NOTIFICATION_TECHNICAL_FAILURE - # elif "invalid" in response_lower: - # return NOTIFICATION_TECHNICAL_FAILURE - # elif "is opted out" in response_lower: - # return NOTIFICATION_PERMANENT_FAILURE - # elif "unknown error" in response_lower: - # return NOTIFICATION_TECHNICAL_FAILURE - # elif "exceed max price" in response_lower: - # return NOTIFICATION_TECHNICAL_FAILURE - # elif "Phone carrier is currently unreachable/unavailable" in response_lower: - # return NOTIFICATION_TEMPORARY_FAILURE - # elif "Phone is currently unreachable/unavailable" in response_lower: - # return NOTIFICATION_PERMANENT_FAILURE - # else: - # return None From fde4812bc936ae4c5058f84f15895e3d46e8103a Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Mon, 6 May 2024 13:53:48 -0400 Subject: [PATCH 29/35] add return --- app/celery/process_pinpoint_receipts_tasks.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/celery/process_pinpoint_receipts_tasks.py b/app/celery/process_pinpoint_receipts_tasks.py index ef76a83bbd..7fe5aaf6a3 100644 --- a/app/celery/process_pinpoint_receipts_tasks.py +++ b/app/celery/process_pinpoint_receipts_tasks.py @@ -115,6 +115,8 @@ def process_pinpoint_results(self, response): current_app.logger.exception(f"Error processing Pinpoint results: {str(e)}") self.retry(queue=QueueNames.RETRY) + return + def determine_pinpoint_status(status: str, provider_response: str) -> Union[str, None]: """Determine the notification status based on the SMS status and provider response. From 040a361e0a9a7ff4525737c17cf5e18d3054090c Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Mon, 6 May 2024 14:06:26 -0400 Subject: [PATCH 30/35] use new callback.pinpoint metrics --- app/celery/process_pinpoint_receipts_tasks.py | 6 +++--- tests/app/celery/test_process_pinpoint_receipts_tasks.py | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/celery/process_pinpoint_receipts_tasks.py b/app/celery/process_pinpoint_receipts_tasks.py index 7fe5aaf6a3..99c849ff30 100644 --- a/app/celery/process_pinpoint_receipts_tasks.py +++ b/app/celery/process_pinpoint_receipts_tasks.py @@ -97,12 +97,12 @@ def process_pinpoint_results(self, response): f"Pinpoint callback return status of {notification_status} for notification: {notification.id}" ) - statsd_client.incr(f"callback.sns.{notification_status}") # TODO: do we want a Pinpoint metric here? + statsd_client.incr(f"callback.pinpoint.{notification_status}") if notification.sent_at: statsd_client.timing_with_dates( - "callback.sns.elapsed-time", datetime.utcnow(), notification.sent_at - ) # TODO: do we want a Pinpoint metric here? + "callback.pinpoint.elapsed-time", datetime.utcnow(), notification.sent_at + ) _check_and_queue_callback_task(notification) diff --git a/tests/app/celery/test_process_pinpoint_receipts_tasks.py b/tests/app/celery/test_process_pinpoint_receipts_tasks.py index e60180a0c9..fe0f0ff29a 100644 --- a/tests/app/celery/test_process_pinpoint_receipts_tasks.py +++ b/tests/app/celery/test_process_pinpoint_receipts_tasks.py @@ -184,8 +184,8 @@ def test_process_pinpoint_results_calls_service_callback(sample_template, notify assert process_pinpoint_results(pinpoint_success_callback(reference="ref")) assert get_notification_by_id(notification.id).status == NOTIFICATION_DELIVERED assert get_notification_by_id(notification.id).provider_response == "Message has been accepted by phone" - statsd_client.timing_with_dates.assert_any_call("callback.sns.elapsed-time", datetime.utcnow(), notification.sent_at) - statsd_client.incr.assert_any_call("callback.sns.delivered") + statsd_client.timing_with_dates.assert_any_call("callback.pinpoint.elapsed-time", datetime.utcnow(), notification.sent_at) + statsd_client.incr.assert_any_call("callback.pinpoint.delivered") updated_notification = get_notification_by_id(notification.id) signed_data = create_delivery_status_callback_data(updated_notification, callback_api) send_mock.assert_called_once_with([str(notification.id), signed_data], queue="service-callbacks") From da4006f9586b3c2574c515accf2b25ae9876d42d Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Mon, 6 May 2024 15:50:09 -0400 Subject: [PATCH 31/35] remove redundant CliendError catching --- app/clients/sms/aws_pinpoint.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/app/clients/sms/aws_pinpoint.py b/app/clients/sms/aws_pinpoint.py index 3dff918c2a..5db8891758 100644 --- a/app/clients/sms/aws_pinpoint.py +++ b/app/clients/sms/aws_pinpoint.py @@ -42,9 +42,6 @@ def send_sms(self, to, content, reference, multi=True, sender=None): MessageType=messageType, ConfigurationSetName=self.current_app.config["AWS_PINPOINT_CONFIGURATION_SET_NAME"], ) - except ClientError as e: - self.statsd_client.incr("clients.pinpoint.error") - raise Exception(e) except Exception as e: self.statsd_client.incr("clients.pinpoint.error") raise Exception(e) From b4e2bd8682a5c24224a9758c7c7d8352f21d638c Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Mon, 6 May 2024 15:55:24 -0400 Subject: [PATCH 32/35] format --- app/celery/process_pinpoint_receipts_tasks.py | 4 +--- app/clients/sms/aws_pinpoint.py | 1 - 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/app/celery/process_pinpoint_receipts_tasks.py b/app/celery/process_pinpoint_receipts_tasks.py index 99c849ff30..1b4db9db79 100644 --- a/app/celery/process_pinpoint_receipts_tasks.py +++ b/app/celery/process_pinpoint_receipts_tasks.py @@ -100,9 +100,7 @@ def process_pinpoint_results(self, response): statsd_client.incr(f"callback.pinpoint.{notification_status}") if notification.sent_at: - statsd_client.timing_with_dates( - "callback.pinpoint.elapsed-time", datetime.utcnow(), notification.sent_at - ) + statsd_client.timing_with_dates("callback.pinpoint.elapsed-time", datetime.utcnow(), notification.sent_at) _check_and_queue_callback_task(notification) diff --git a/app/clients/sms/aws_pinpoint.py b/app/clients/sms/aws_pinpoint.py index 5db8891758..37140323c0 100644 --- a/app/clients/sms/aws_pinpoint.py +++ b/app/clients/sms/aws_pinpoint.py @@ -2,7 +2,6 @@ import boto3 import phonenumbers -from botocore.exceptions import ClientError from app.clients.sms import SmsClient From afec9126ac86766927aa3cb4c90f6830f5d3aa33 Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Tue, 7 May 2024 15:29:52 -0400 Subject: [PATCH 33/35] refactor process_pinpoint_results tests --- app/celery/process_pinpoint_receipts_tasks.py | 4 --- .../test_process_pinpoint_receipts_tasks.py | 32 +++++++++++++++---- 2 files changed, 25 insertions(+), 11 deletions(-) diff --git a/app/celery/process_pinpoint_receipts_tasks.py b/app/celery/process_pinpoint_receipts_tasks.py index 1b4db9db79..c4c944d6b5 100644 --- a/app/celery/process_pinpoint_receipts_tasks.py +++ b/app/celery/process_pinpoint_receipts_tasks.py @@ -104,8 +104,6 @@ def process_pinpoint_results(self, response): _check_and_queue_callback_task(notification) - return True - except Retry: raise @@ -113,8 +111,6 @@ def process_pinpoint_results(self, response): current_app.logger.exception(f"Error processing Pinpoint results: {str(e)}") self.retry(queue=QueueNames.RETRY) - return - def determine_pinpoint_status(status: str, provider_response: str) -> Union[str, None]: """Determine the notification status based on the SMS status and provider response. diff --git a/tests/app/celery/test_process_pinpoint_receipts_tasks.py b/tests/app/celery/test_process_pinpoint_receipts_tasks.py index fe0f0ff29a..03932428bd 100644 --- a/tests/app/celery/test_process_pinpoint_receipts_tasks.py +++ b/tests/app/celery/test_process_pinpoint_receipts_tasks.py @@ -26,6 +26,7 @@ def test_process_pinpoint_results_delivered(sample_template, notify_db, notify_db_session, mocker): mock_logger = mocker.patch("app.celery.process_pinpoint_receipts_tasks.current_app.logger.info") + mock_callback_task = mocker.patch("app.notifications.callbacks._check_and_queue_callback_task") notification = create_sample_notification( notify_db, @@ -37,7 +38,10 @@ def test_process_pinpoint_results_delivered(sample_template, notify_db, notify_d sent_at=datetime.utcnow(), ) assert get_notification_by_id(notification.id).status == NOTIFICATION_SENT - assert process_pinpoint_results(pinpoint_success_callback(reference="ref")) + + process_pinpoint_results(pinpoint_success_callback(reference="ref")) + + assert mock_callback_task.called_once_with(get_notification_by_id(notification.id)) assert get_notification_by_id(notification.id).status == NOTIFICATION_DELIVERED assert get_notification_by_id(notification.id).provider_response == "Message has been accepted by phone" @@ -80,6 +84,7 @@ def test_process_pinpoint_results_failed( ): mock_logger = mocker.patch("app.celery.process_pinpoint_receipts_tasks.current_app.logger.info") mock_warning_logger = mocker.patch("app.celery.process_pinpoint_receipts_tasks.current_app.logger.warning") + mock_callback_task = mocker.patch("app.notifications.callbacks._check_and_queue_callback_task") notification = create_sample_notification( notify_db, @@ -91,7 +96,9 @@ def test_process_pinpoint_results_failed( sent_at=datetime.utcnow(), ) assert get_notification_by_id(notification.id).status == NOTIFICATION_SENT - assert process_pinpoint_results(pinpoint_failed_callback(provider_response=provider_response, reference="ref")) + process_pinpoint_results(pinpoint_failed_callback(provider_response=provider_response, reference="ref")) + + assert mock_callback_task.called_once_with(get_notification_by_id(notification.id)) assert get_notification_by_id(notification.id).status == expected_status if should_save_provider_response: @@ -111,7 +118,11 @@ def test_process_pinpoint_results_failed( def test_pinpoint_callback_should_retry_if_notification_is_missing(notify_db, mocker): mock_retry = mocker.patch("app.celery.process_pinpoint_receipts_tasks.process_pinpoint_results.retry") - assert process_pinpoint_results(pinpoint_success_callback(reference="ref")) is None + mock_callback_task = mocker.patch("app.notifications.callbacks._check_and_queue_callback_task") + + process_pinpoint_results(pinpoint_success_callback(reference="ref")) + + mock_callback_task.assert_not_called() assert mock_retry.call_count == 1 @@ -121,8 +132,11 @@ def test_pinpoint_callback_should_give_up_after_max_tries(notify_db, mocker): side_effect=MaxRetriesExceededError, ) mock_logger = mocker.patch("app.celery.process_pinpoint_receipts_tasks.current_app.logger.warning") + mock_callback_task = mocker.patch("app.notifications.callbacks._check_and_queue_callback_task") + + process_pinpoint_results(pinpoint_success_callback(reference="ref")) is None + mock_callback_task.assert_not_called() - assert process_pinpoint_results(pinpoint_success_callback(reference="ref")) is None mock_logger.assert_called_with("notification not found for Pinpoint reference: ref (update to delivered). Giving up.") @@ -168,7 +182,9 @@ def test_process_pinpoint_results_calls_service_callback(sample_template, notify with freeze_time("2021-01-01T12:00:00"): mocker.patch("app.statsd_client.incr") mocker.patch("app.statsd_client.timing_with_dates") - send_mock = mocker.patch("app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async") + mock_send_status = mocker.patch("app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async") + mock_callback = mocker.patch("app.notifications.callbacks._check_and_queue_callback_task") + notification = create_sample_notification( notify_db, notify_db_session, @@ -181,11 +197,13 @@ def test_process_pinpoint_results_calls_service_callback(sample_template, notify callback_api = create_service_callback_api(service=sample_template.service, url="https://example.com") assert get_notification_by_id(notification.id).status == NOTIFICATION_SENT - assert process_pinpoint_results(pinpoint_success_callback(reference="ref")) + process_pinpoint_results(pinpoint_success_callback(reference="ref")) + + assert mock_callback.called_once_with(get_notification_by_id(notification.id)) assert get_notification_by_id(notification.id).status == NOTIFICATION_DELIVERED assert get_notification_by_id(notification.id).provider_response == "Message has been accepted by phone" statsd_client.timing_with_dates.assert_any_call("callback.pinpoint.elapsed-time", datetime.utcnow(), notification.sent_at) statsd_client.incr.assert_any_call("callback.pinpoint.delivered") updated_notification = get_notification_by_id(notification.id) signed_data = create_delivery_status_callback_data(updated_notification, callback_api) - send_mock.assert_called_once_with([str(notification.id), signed_data], queue="service-callbacks") + mock_send_status.assert_called_once_with([str(notification.id), signed_data], queue="service-callbacks") From 337c594b5ae15fed8c20a029a4162bdddde59752 Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Tue, 7 May 2024 15:36:20 -0400 Subject: [PATCH 34/35] if/else ftw --- app/celery/process_pinpoint_receipts_tasks.py | 34 +++++++++---------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/app/celery/process_pinpoint_receipts_tasks.py b/app/celery/process_pinpoint_receipts_tasks.py index c4c944d6b5..c2d98f7901 100644 --- a/app/celery/process_pinpoint_receipts_tasks.py +++ b/app/celery/process_pinpoint_receipts_tasks.py @@ -127,20 +127,20 @@ def determine_pinpoint_status(status: str, provider_response: str) -> Union[str, return NOTIFICATION_DELIVERED response_lower = provider_response.lower() - match response_lower: - case response_lower if "blocked" in response_lower: - return NOTIFICATION_TECHNICAL_FAILURE - case response_lower if "invalid" in response_lower: - return NOTIFICATION_TECHNICAL_FAILURE - case response_lower if "is opted out" in response_lower: - return NOTIFICATION_PERMANENT_FAILURE - case response_lower if "unknown error" in response_lower: - return NOTIFICATION_TECHNICAL_FAILURE - case response_lower if "exceed max price" in response_lower: - return NOTIFICATION_TECHNICAL_FAILURE - case "phone carrier is currently unreachable/unavailable": - return NOTIFICATION_TEMPORARY_FAILURE - case "phone is currently unreachable/unavailable": - return NOTIFICATION_PERMANENT_FAILURE - - return None + + if "blocked" in response_lower: + return NOTIFICATION_TECHNICAL_FAILURE + elif "invalid" in response_lower: + return NOTIFICATION_TECHNICAL_FAILURE + elif "is opted out" in response_lower: + return NOTIFICATION_PERMANENT_FAILURE + elif "unknown error" in response_lower: + return NOTIFICATION_TECHNICAL_FAILURE + elif "exceed max price" in response_lower: + return NOTIFICATION_TECHNICAL_FAILURE + elif "phone carrier is currently unreachable/unavailable" in response_lower: + return NOTIFICATION_TEMPORARY_FAILURE + elif "phone is currently unreachable/unavailable" in response_lower: + return NOTIFICATION_PERMANENT_FAILURE + else: + return None From 7fb51e17930d107736f513adc8a51de523e91ea8 Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Tue, 7 May 2024 15:42:33 -0400 Subject: [PATCH 35/35] format --- app/celery/process_pinpoint_receipts_tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/celery/process_pinpoint_receipts_tasks.py b/app/celery/process_pinpoint_receipts_tasks.py index c2d98f7901..5e9148b2cd 100644 --- a/app/celery/process_pinpoint_receipts_tasks.py +++ b/app/celery/process_pinpoint_receipts_tasks.py @@ -127,7 +127,7 @@ def determine_pinpoint_status(status: str, provider_response: str) -> Union[str, return NOTIFICATION_DELIVERED response_lower = provider_response.lower() - + if "blocked" in response_lower: return NOTIFICATION_TECHNICAL_FAILURE elif "invalid" in response_lower: