From f1daf452f34866c2d3b81b7f1f3bcbe8f8262582 Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Thu, 4 Jul 2024 16:34:23 -0400 Subject: [PATCH 1/5] fix opted out sms in pinpoint --- app/clients/sms/aws_pinpoint.py | 9 ++++++++- app/delivery/send_to_providers.py | 14 +++++++++++++- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/app/clients/sms/aws_pinpoint.py b/app/clients/sms/aws_pinpoint.py index bdb3ba7fa7..3cf314f8ce 100644 --- a/app/clients/sms/aws_pinpoint.py +++ b/app/clients/sms/aws_pinpoint.py @@ -32,6 +32,7 @@ def send_sms(self, to, content, reference, multi=True, sender=None, template_id= for match in phonenumbers.PhoneNumberMatcher(to, "US"): matched = True + opted_out = False to = phonenumbers.format_number(match.number, phonenumbers.PhoneNumberFormat.E164) destinationNumber = to @@ -44,6 +45,12 @@ def send_sms(self, to, content, reference, multi=True, sender=None, template_id= MessageType=messageType, ConfigurationSetName=self.current_app.config["AWS_PINPOINT_CONFIGURATION_SET_NAME"], ) + except self._client.exceptions.ConflictException as e: + if e.response.get("Reason") == "DESTINATION_PHONE_NUMBER_OPTED_OUT": + opted_out = True + else: + raise e + except Exception as e: self.statsd_client.incr("clients.pinpoint.error") raise Exception(e) @@ -52,7 +59,7 @@ def send_sms(self, to, content, reference, multi=True, sender=None, template_id= 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 "opted_out" if opted_out else 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 331e846b7d..b3e4dcaefe 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -43,6 +43,7 @@ EMAIL_TYPE, KEY_TYPE_TEST, NOTIFICATION_CONTAINS_PII, + NOTIFICATION_PERMANENT_FAILURE, NOTIFICATION_SENDING, NOTIFICATION_SENT, NOTIFICATION_TECHNICAL_FAILURE, @@ -118,7 +119,10 @@ def send_sms_to_provider(notification): else: notification.reference = reference notification.billable_units = template.fragment_count - update_notification_to_sending(notification, provider) + if reference == "opted_out": + update_notification_to_opted_out(notification, provider) + else: + update_notification_to_sending(notification, provider) # Record StatsD stats to compute SLOs statsd_client.timing_with_dates("sms.total-time", notification.sent_at, notification.created_at) @@ -340,6 +344,14 @@ def update_notification_to_sending(notification, provider): dao_update_notification(notification) +def update_notification_to_opted_out(notification, provider): + notification.sent_at = datetime.utcnow() + notification.sent_by = provider.get_name() + notification.status = NOTIFICATION_PERMANENT_FAILURE + notification.provider_response = "Phone number is opted out" + dao_update_notification(notification) + + def provider_to_use( notification_type: str, notification_id: UUID, From 288b13460d91e21ca24402cf9c09dc385df4e545 Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Mon, 8 Jul 2024 12:17:04 -0400 Subject: [PATCH 2/5] tests --- app/clients/sms/aws_pinpoint.py | 1 + tests/app/clients/test_aws_pinpoint.py | 15 +++++++++++ tests/app/delivery/test_send_to_providers.py | 26 ++++++++++++++++++++ 3 files changed, 42 insertions(+) diff --git a/app/clients/sms/aws_pinpoint.py b/app/clients/sms/aws_pinpoint.py index 3cf314f8ce..a56b64d800 100644 --- a/app/clients/sms/aws_pinpoint.py +++ b/app/clients/sms/aws_pinpoint.py @@ -24,6 +24,7 @@ def get_name(self): def send_sms(self, to, content, reference, multi=True, sender=None, template_id=None): messageType = "TRANSACTIONAL" matched = False + opted_out = 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"] diff --git a/tests/app/clients/test_aws_pinpoint.py b/tests/app/clients/test_aws_pinpoint.py index ad7546d1ad..8879c98682 100644 --- a/tests/app/clients/test_aws_pinpoint.py +++ b/tests/app/clients/test_aws_pinpoint.py @@ -71,3 +71,18 @@ def test_send_sms_returns_raises_error_if_there_is_no_valid_number_is_found(noti aws_pinpoint_client.send_sms(to, content, reference) assert "No valid numbers found for SMS delivery" in str(excinfo.value) + + +def test_handles_opted_out_numbers(notify_api, mocker, sample_template): + # mock_client = mocker.patch.object(aws_pinpoint_client, "_client", create=True) + # mocker.patch.object(aws_pinpoint_client, "statsd_client", create=True) + + conflict_error = aws_pinpoint_client._client.exceptions.ConflictException( + error_response={"Reason": "DESTINATION_PHONE_NUMBER_OPTED_OUT"}, operation_name="send_text_message" + ) + mocker.patch("app.aws_pinpoint_client._client.send_text_message", side_effect=conflict_error) + + to = "6135555555" + content = "foo" + reference = "ref" + assert aws_pinpoint_client.send_sms(to, content, reference=reference, template_id=sample_template.id) == "opted_out" diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 05482eca2d..a848696e1f 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -189,6 +189,32 @@ def test_should_return_highest_priority_active_provider(restore_provider_details assert send_to_providers.provider_to_use("sms", "1234").name == first.identifier +def test_should_handle_opted_out_phone_numbers_if_using_pinpoint(notify_api, sample_template, mocker): + # statsd_mock = mocker.patch("app.delivery.send_to_providers.statsd_client") + mocker.patch("app.aws_pinpoint_client.send_sms", return_value="opted_out") + db_notification = save_notification( + create_notification( + template=sample_template, + to_field="+16135551234", + status="created", + reply_to_text=sample_template.service.get_default_sms_sender(), + ) + ) + + with set_config_values( + notify_api, + { + "AWS_PINPOINT_SC_POOL_ID": "sc_pool_id", + "AWS_PINPOINT_DEFAULT_POOL_ID": "default_pool_id", + }, + ): + send_to_providers.send_sms_to_provider(db_notification) + + notification = Notification.query.filter_by(id=db_notification.id).one() + assert notification.status == "permanent-failure" + assert notification.provider_response == "Phone number is opted out" + + def test_should_send_personalised_template_to_correct_sms_provider_and_persist(sample_sms_template_with_html, mocker): db_notification = save_notification( create_notification( From 629b5afb091fcd005192505eb4df708a38f42d9f Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Mon, 8 Jul 2024 13:55:29 -0400 Subject: [PATCH 3/5] tweak --- app/clients/sms/aws_pinpoint.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/clients/sms/aws_pinpoint.py b/app/clients/sms/aws_pinpoint.py index a56b64d800..a06226af03 100644 --- a/app/clients/sms/aws_pinpoint.py +++ b/app/clients/sms/aws_pinpoint.py @@ -25,6 +25,7 @@ def send_sms(self, to, content, reference, multi=True, sender=None, template_id= messageType = "TRANSACTIONAL" matched = False opted_out = False + response = {} 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"] @@ -60,7 +61,7 @@ def send_sms(self, to, content, reference, multi=True, sender=None, template_id= 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 "opted_out" if opted_out else response["MessageId"] + return "opted_out" if opted_out else response.get("MessageId") if not matched: self.statsd_client.incr("clients.pinpoint.error") From 05c36a43133a1963693723785f33566ffe492937 Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Mon, 8 Jul 2024 14:07:09 -0400 Subject: [PATCH 4/5] simplify a raise --- 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 a06226af03..1ce3255d5c 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, template_id= except Exception as e: self.statsd_client.incr("clients.pinpoint.error") - raise Exception(e) + raise e finally: elapsed_time = monotonic() - start_time self.current_app.logger.info("AWS Pinpoint request finished in {}".format(elapsed_time)) From adb7ae8a8b0e740f1540bd93e15b8eb50fdb2445 Mon Sep 17 00:00:00 2001 From: Stephen Astels Date: Mon, 8 Jul 2024 14:47:26 -0400 Subject: [PATCH 5/5] delete commented out code --- tests/app/clients/test_aws_pinpoint.py | 3 --- tests/app/delivery/test_send_to_providers.py | 1 - 2 files changed, 4 deletions(-) diff --git a/tests/app/clients/test_aws_pinpoint.py b/tests/app/clients/test_aws_pinpoint.py index 8879c98682..33d1d65225 100644 --- a/tests/app/clients/test_aws_pinpoint.py +++ b/tests/app/clients/test_aws_pinpoint.py @@ -74,9 +74,6 @@ def test_send_sms_returns_raises_error_if_there_is_no_valid_number_is_found(noti def test_handles_opted_out_numbers(notify_api, mocker, sample_template): - # mock_client = mocker.patch.object(aws_pinpoint_client, "_client", create=True) - # mocker.patch.object(aws_pinpoint_client, "statsd_client", create=True) - conflict_error = aws_pinpoint_client._client.exceptions.ConflictException( error_response={"Reason": "DESTINATION_PHONE_NUMBER_OPTED_OUT"}, operation_name="send_text_message" ) diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index a848696e1f..374f478394 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -190,7 +190,6 @@ def test_should_return_highest_priority_active_provider(restore_provider_details def test_should_handle_opted_out_phone_numbers_if_using_pinpoint(notify_api, sample_template, mocker): - # statsd_mock = mocker.patch("app.delivery.send_to_providers.statsd_client") mocker.patch("app.aws_pinpoint_client.send_sms", return_value="opted_out") db_notification = save_notification( create_notification(