From 310e79c386bcb10781aa1e0991c2c7d8d34b4a17 Mon Sep 17 00:00:00 2001 From: Michael Wellman Date: Thu, 5 Dec 2024 08:53:27 -0500 Subject: [PATCH] Updated status reason --- app/celery/provider_tasks.py | 6 +-- app/clients/sms/__init__.py | 1 + app/clients/sms/twilio.py | 9 +++-- tests/app/celery/test_provider_tasks.py | 49 ++++++++++++++++++++++++- 4 files changed, 57 insertions(+), 8 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index cdd5372dab..bf5a3ba9bf 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -8,7 +8,7 @@ from app.celery.exceptions import NonRetryableException, AutoRetryException from app.celery.service_callback_tasks import check_and_queue_callback_task from app.clients.email.aws_ses import AwsSesClientThrottlingSendRateException -from app.clients.sms import OPT_OUT_MESSAGE +from app.clients.sms import MESSAGE_TOO_LONG, OPT_OUT_MESSAGE from app.config import QueueNames from app.constants import NOTIFICATION_TECHNICAL_FAILURE from app.dao import notifications_dao @@ -77,10 +77,10 @@ def deliver_sms( ) raise NotificationTechnicalFailureException from e except NonRetryableException as e: - # Likely an opted out from pinpoint - if 'opted out' in str(e).lower(): status_reason = OPT_OUT_MESSAGE + elif str(e) == MESSAGE_TOO_LONG: + status_reason = MESSAGE_TOO_LONG else: status_reason = 'ERROR: NonRetryableException - permanent failure, not retrying' diff --git a/app/clients/sms/__init__.py b/app/clients/sms/__init__.py index 2ae71a05dd..14fa8783ef 100644 --- a/app/clients/sms/__init__.py +++ b/app/clients/sms/__init__.py @@ -10,6 +10,7 @@ RETRYABLE_AWS_RESPONSE = 'Encountered a temporary failure. Send the request to VA Notify again' UNABLE_TO_TRANSLATE = 'unable to translate delivery status' UNEXPECTED_PROVIDER_RESULT = 'Unexpected result' +MESSAGE_TOO_LONG = 'Message too long' class SmsClientResponseException(ClientException): diff --git a/app/clients/sms/twilio.py b/app/clients/sms/twilio.py index 81f57e873e..6c525f9706 100644 --- a/app/clients/sms/twilio.py +++ b/app/clients/sms/twilio.py @@ -10,7 +10,7 @@ from twilio.base.exceptions import TwilioRestException from app.celery.exceptions import NonRetryableException -from app.clients.sms import SmsClient, SmsStatusRecord, OPT_OUT_MESSAGE, UNABLE_TO_TRANSLATE +from app.clients.sms import SmsClient, SmsStatusRecord, OPT_OUT_MESSAGE, UNABLE_TO_TRANSLATE, MESSAGE_TOO_LONG from app.constants import ( NOTIFICATION_CREATED, NOTIFICATION_DELIVERED, @@ -58,7 +58,7 @@ class TwilioSMSClient(SmsClient): '21610': TwilioStatus(21610, NOTIFICATION_PERMANENT_FAILURE, OPT_OUT_MESSAGE), '21612': TwilioStatus(21612, NOTIFICATION_PERMANENT_FAILURE, 'Invalid to/from combo'), '21614': TwilioStatus(21614, NOTIFICATION_PERMANENT_FAILURE, 'Non-mobile number'), - '21617': TwilioStatus(21617, NOTIFICATION_PERMANENT_FAILURE, 'Message too long'), + '21617': TwilioStatus(21617, NOTIFICATION_PERMANENT_FAILURE, MESSAGE_TOO_LONG), '21635': TwilioStatus(21635, NOTIFICATION_PERMANENT_FAILURE, 'Non-mobile number'), '30001': TwilioStatus(30001, NOTIFICATION_TEMPORARY_FAILURE, 'Queue overflow'), '30002': TwilioStatus(30002, NOTIFICATION_PERMANENT_FAILURE, 'Account suspended'), @@ -239,11 +239,12 @@ def send_sms( self.logger.exception('Twilio send SMS request for %s failed', reference) raise InvalidProviderException from e elif e.status == 400 and e.code == 21617: # Twilio error code for max length exceeded + status = self.twilio_error_code_map.get('21617') self.logger.exception( - 'Twilio send SMS request for %s failed, message content max length exceeded.', reference + 'Twilio send SMS request for %s failed, message content length exceeded.', reference ) self.logger.debug('Twilio error details for %s - %s: %s', reference, e.code, e.msg) - raise NonRetryableException('Twilio request failed') from e + raise NonRetryableException(status.status_reason) from e else: raise except: diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 18a1de69b1..898deada16 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -4,7 +4,7 @@ from app.celery.exceptions import NonRetryableException, AutoRetryException from app.celery.provider_tasks import deliver_sms, deliver_email, deliver_sms_with_rate_limiting from app.clients.email.aws_ses import AwsSesClientThrottlingSendRateException -from app.clients.sms import OPT_OUT_MESSAGE +from app.clients.sms import MESSAGE_TOO_LONG, OPT_OUT_MESSAGE from app.config import QueueNames from app.constants import ( EMAIL_TYPE, @@ -485,3 +485,50 @@ def test_deliver_sms_opt_out( notify_db_session.session.refresh(notification) assert notification.status == NOTIFICATION_PERMANENT_FAILURE assert notification.status_reason == OPT_OUT_MESSAGE + + +@pytest.mark.parametrize( + 'exception_message, status_reason', + ( + ('Message too long', MESSAGE_TOO_LONG), + ('Destination phone number opted out', OPT_OUT_MESSAGE), + ), +) +def test_deliver_sms_content_exceeded( + notify_db_session, + mocker, + sample_service, + sample_sms_sender, + sample_template, + sample_notification, + exception_message, + status_reason, +): + """ + An SMS notification sent with content that exceeds the maximum length allowed + should result in permanent failure with a relevant status reason. + """ + + service = sample_service() + sms_sender = sample_sms_sender(service_id=service.id, sms_sender='17045555555') + template = sample_template(service=service) + notification = sample_notification( + template=template, + status=NOTIFICATION_CREATED, + sms_sender_id=sms_sender.id, + ) + assert notification.notification_type == SMS_TYPE + assert notification.status == NOTIFICATION_CREATED + assert notification.sms_sender_id == sms_sender.id + assert notification.status_reason is None + + mock_send_sms_to_provider = mocker.patch( + 'app.delivery.send_to_providers.send_sms_to_provider', + side_effect=NonRetryableException(exception_message), + ) + deliver_sms(notification.id, sms_sender_id=notification.sms_sender_id) + mock_send_sms_to_provider.assert_called_once() + + notify_db_session.session.refresh(notification) + assert notification.status == NOTIFICATION_PERMANENT_FAILURE + assert notification.status_reason == status_reason