diff --git a/app/celery/process_ses_receipts_tasks.py b/app/celery/process_ses_receipts_tasks.py index 4147e7483a..321cb1654f 100644 --- a/app/celery/process_ses_receipts_tasks.py +++ b/app/celery/process_ses_receipts_tasks.py @@ -30,7 +30,7 @@ NOTIFICATION_PERMANENT_FAILURE, NOTIFICATION_TEMPORARY_FAILURE, STATUS_REASON_RETRYABLE, - STATUS_REASON_UNDELIVERABLE, + STATUS_REASON_UNREACHABLE, ) from app.clients.email.aws_ses import get_aws_responses from app.dao import notifications_dao, services_dao, templates_dao @@ -177,7 +177,7 @@ def process_ses_results( # noqa: C901 (too complex 14 > 10) aws_response_dict = get_aws_responses(notification_type) # This is the prospective, updated status. - notification_status = aws_response_dict['notification_status'] + incoming_status = aws_response_dict['notification_status'] reference = ses_message['mail']['messageId'] try: @@ -188,44 +188,39 @@ def process_ses_results( # noqa: C901 (too complex 14 > 10) self.retry(queue=QueueNames.RETRY) else: current_app.logger.warning( - 'notification not found for reference: %s (update to %s)', reference, notification_status + 'notification not found for reference: %s (update to %s)', reference, incoming_status ) return # Prevent regressing bounce status. Note that this is a test of the existing status; not the new status. - if ( - notification.status_reason - and 'bounce' in notification.status_reason - and notification.status - in { - NOTIFICATION_TEMPORARY_FAILURE, - NOTIFICATION_PERMANENT_FAILURE, - } - ): + if notification.status_reason and notification.status in { + NOTIFICATION_TEMPORARY_FAILURE, + NOTIFICATION_PERMANENT_FAILURE, + }: # async from AWS means we may get a delivered status after a bounce, in rare cases current_app.logger.warning( 'Notification: %s was marked as a bounce, cannot be updated to: %s', notification.id, - notification_status, + incoming_status, ) return # This is a test of the new status. Is it a bounce? - if notification_status in (NOTIFICATION_TEMPORARY_FAILURE, NOTIFICATION_PERMANENT_FAILURE): + if incoming_status in (NOTIFICATION_TEMPORARY_FAILURE, NOTIFICATION_PERMANENT_FAILURE): # Add the failure status reason to the notification. - if notification_status == NOTIFICATION_PERMANENT_FAILURE: + if incoming_status == NOTIFICATION_PERMANENT_FAILURE: failure_reason = 'Failed to deliver email due to hard bounce' - status_reason = STATUS_REASON_UNDELIVERABLE + status_reason = STATUS_REASON_UNREACHABLE else: failure_reason = 'Temporarily failed to deliver email due to soft bounce' status_reason = STATUS_REASON_RETRYABLE notification.status_reason = status_reason - notification.status = notification_status + notification.status = incoming_status current_app.logger.warning( '%s - %s - in process_ses_results for notification %s', - notification_status, + incoming_status, failure_reason, notification.id, ) @@ -235,15 +230,15 @@ def process_ses_results( # noqa: C901 (too complex 14 > 10) check_and_queue_va_profile_notification_status_callback(notification) return - elif notification_status == NOTIFICATION_DELIVERED: + elif incoming_status == NOTIFICATION_DELIVERED: # Delivered messages should never have a status reason. notification.status_reason = None if notification.status not in (NOTIFICATION_SENDING, NOTIFICATION_PENDING): - notifications_dao.duplicate_update_warning(notification, notification_status) + notifications_dao.duplicate_update_warning(notification, incoming_status) return - notifications_dao._update_notification_status(notification=notification, status=notification_status) + notifications_dao._update_notification_status(notification=notification, status=incoming_status) if not aws_response_dict['success']: current_app.logger.info( @@ -255,14 +250,14 @@ def process_ses_results( # noqa: C901 (too complex 14 > 10) else: current_app.logger.info( 'SES callback return status of %s for notification: %s', - notification_status, + incoming_status, notification.id, ) log_notification_total_time( notification.id, notification.created_at, - notification_status, + incoming_status, 'ses', ) diff --git a/app/clients/email/aws_ses.py b/app/clients/email/aws_ses.py index 42ff56c351..13b7d737d3 100644 --- a/app/clients/email/aws_ses.py +++ b/app/clients/email/aws_ses.py @@ -10,29 +10,31 @@ from email.mime.text import MIMEText from email.mime.application import MIMEApplication +from app.constants import NOTIFICATION_DELIVERED, NOTIFICATION_PERMANENT_FAILURE, NOTIFICATION_TEMPORARY_FAILURE + ses_response_map = { 'Permanent': { 'message': 'Hard bounced', 'success': False, - 'notification_status': 'permanent-failure', + 'notification_status': NOTIFICATION_PERMANENT_FAILURE, 'notification_statistics_status': STATISTICS_FAILURE, }, 'Temporary': { 'message': 'Soft bounced', 'success': False, - 'notification_status': 'temporary-failure', + 'notification_status': NOTIFICATION_TEMPORARY_FAILURE, 'notification_statistics_status': STATISTICS_FAILURE, }, 'Delivery': { 'message': 'Delivered', 'success': True, - 'notification_status': 'delivered', + 'notification_status': NOTIFICATION_DELIVERED, 'notification_statistics_status': STATISTICS_DELIVERED, }, 'Complaint': { 'message': 'Complaint', 'success': True, - 'notification_status': 'delivered', + 'notification_status': NOTIFICATION_DELIVERED, 'notification_statistics_status': STATISTICS_DELIVERED, }, } diff --git a/app/clients/sms/aws_pinpoint.py b/app/clients/sms/aws_pinpoint.py index fe22cd0672..82ef7919a1 100644 --- a/app/clients/sms/aws_pinpoint.py +++ b/app/clients/sms/aws_pinpoint.py @@ -20,6 +20,7 @@ NOTIFICATION_PERMANENT_FAILURE, PINPOINT_PROVIDER, STATUS_REASON_BLOCKED, + STATUS_REASON_INVALID_NUMBER, STATUS_REASON_RETRYABLE, STATUS_REASON_UNDELIVERABLE, ) @@ -41,7 +42,7 @@ class AwsPinpointClient(SmsClient): 'SUCCESSFUL': (NOTIFICATION_DELIVERED, None), 'DELIVERED': (NOTIFICATION_DELIVERED, None), 'PENDING': (NOTIFICATION_SENDING, None), - 'INVALID': (NOTIFICATION_PERMANENT_FAILURE, STATUS_REASON_UNDELIVERABLE), + 'INVALID': (NOTIFICATION_PERMANENT_FAILURE, STATUS_REASON_INVALID_NUMBER), 'UNREACHABLE': (NOTIFICATION_TEMPORARY_FAILURE, STATUS_REASON_RETRYABLE), 'UNKNOWN': (NOTIFICATION_TEMPORARY_FAILURE, STATUS_REASON_RETRYABLE), 'BLOCKED': (NOTIFICATION_PERMANENT_FAILURE, STATUS_REASON_BLOCKED), diff --git a/app/clients/sms/twilio.py b/app/clients/sms/twilio.py index 37a34a0142..a2c93bf741 100644 --- a/app/clients/sms/twilio.py +++ b/app/clients/sms/twilio.py @@ -19,6 +19,7 @@ NOTIFICATION_SENT, NOTIFICATION_TEMPORARY_FAILURE, STATUS_REASON_BLOCKED, + STATUS_REASON_INVALID_NUMBER, STATUS_REASON_RETRYABLE, STATUS_REASON_UNDELIVERABLE, STATUS_REASON_UNREACHABLE, @@ -62,9 +63,9 @@ class TwilioSMSClient(SmsClient): '21610': TwilioStatus(21610, NOTIFICATION_PERMANENT_FAILURE, STATUS_REASON_BLOCKED), # 21612: 'Invalid to/from combo' '21612': TwilioStatus(21612, NOTIFICATION_PERMANENT_FAILURE, STATUS_REASON_UNREACHABLE), - '21614': TwilioStatus(21614, NOTIFICATION_PERMANENT_FAILURE, STATUS_REASON_UNREACHABLE), + '21614': TwilioStatus(21614, NOTIFICATION_PERMANENT_FAILURE, STATUS_REASON_INVALID_NUMBER), '21617': TwilioStatus(21617, NOTIFICATION_PERMANENT_FAILURE, STATUS_REASON_UNDELIVERABLE), - '21635': TwilioStatus(21635, NOTIFICATION_PERMANENT_FAILURE, STATUS_REASON_UNREACHABLE), + '21635': TwilioStatus(21635, NOTIFICATION_PERMANENT_FAILURE, STATUS_REASON_INVALID_NUMBER), '30001': TwilioStatus(30001, NOTIFICATION_TEMPORARY_FAILURE, STATUS_REASON_RETRYABLE), # 30002: 'Account suspended' '30002': TwilioStatus(30002, NOTIFICATION_PERMANENT_FAILURE, STATUS_REASON_UNDELIVERABLE), diff --git a/tests/app/celery/test_process_pinpoint_receipt_tasks.py b/tests/app/celery/test_process_pinpoint_receipt_tasks.py index b414871b70..80ec00302b 100644 --- a/tests/app/celery/test_process_pinpoint_receipt_tasks.py +++ b/tests/app/celery/test_process_pinpoint_receipt_tasks.py @@ -16,6 +16,7 @@ NOTIFICATION_PERMANENT_FAILURE, PINPOINT_PROVIDER, STATUS_REASON_BLOCKED, + STATUS_REASON_INVALID_NUMBER, STATUS_REASON_RETRYABLE, STATUS_REASON_UNDELIVERABLE, ) @@ -27,7 +28,7 @@ [ ('_SMS.BUFFERED', 'SUCCESSFUL', NOTIFICATION_DELIVERED, None), ('_SMS.SUCCESS', 'DELIVERED', NOTIFICATION_DELIVERED, None), - ('_SMS.FAILURE', 'INVALID', NOTIFICATION_PERMANENT_FAILURE, STATUS_REASON_UNDELIVERABLE), + ('_SMS.FAILURE', 'INVALID', NOTIFICATION_PERMANENT_FAILURE, STATUS_REASON_INVALID_NUMBER), ('_SMS.FAILURE', 'UNREACHABLE', NOTIFICATION_TEMPORARY_FAILURE, STATUS_REASON_RETRYABLE), ('_SMS.FAILURE', 'UNKNOWN', NOTIFICATION_TEMPORARY_FAILURE, STATUS_REASON_RETRYABLE), ('_SMS.FAILURE', 'BLOCKED', NOTIFICATION_PERMANENT_FAILURE, STATUS_REASON_BLOCKED), diff --git a/tests/app/celery/test_process_ses_receipts_tasks.py b/tests/app/celery/test_process_ses_receipts_tasks.py index c648e12997..37d6db3e10 100644 --- a/tests/app/celery/test_process_ses_receipts_tasks.py +++ b/tests/app/celery/test_process_ses_receipts_tasks.py @@ -21,6 +21,7 @@ NOTIFICATION_TEMPORARY_FAILURE, STATUS_REASON_RETRYABLE, STATUS_REASON_UNDELIVERABLE, + STATUS_REASON_UNREACHABLE, ) from app.dao.notifications_dao import get_notification_by_id from app.models import Complaint, Notification, Service, Template @@ -454,7 +455,7 @@ def test_ses_callback_should_set_status_to_permanent_failure( assert process_ses_receipts_tasks.process_ses_results(ses_hard_bounce_callback(reference=ref)) is None db_notification = notify_db_session.session.get(Notification, notification_id) assert db_notification.status == NOTIFICATION_PERMANENT_FAILURE - assert db_notification.status_reason == STATUS_REASON_UNDELIVERABLE + assert db_notification.status_reason == STATUS_REASON_UNREACHABLE assert send_mock.called @@ -618,12 +619,19 @@ def get_complaint_notification_and_email(mocker): return complaint, notification, recipient_email -@pytest.mark.parametrize('status', (NOTIFICATION_PERMANENT_FAILURE, NOTIFICATION_TEMPORARY_FAILURE)) +@pytest.mark.parametrize( + 'status, status_reason', + ( + (NOTIFICATION_PERMANENT_FAILURE, STATUS_REASON_UNREACHABLE), + (NOTIFICATION_TEMPORARY_FAILURE, STATUS_REASON_RETRYABLE), + ), +) def test_process_ses_results_no_bounce_regression( notify_db_session, sample_template, sample_notification, status, + status_reason, ): """ If a bounce status has been persisted for a notificaiton, no further status updates should occur. @@ -633,7 +641,7 @@ def test_process_ses_results_no_bounce_regression( notification = sample_notification( template=sample_template(template_type=EMAIL_TYPE), status=status, - status_reason='bounce', + status_reason=status_reason, reference=str(uuid4()), ) @@ -644,4 +652,4 @@ def test_process_ses_results_no_bounce_regression( notify_db_session.session.refresh(notification) assert notification.status == status, 'The status should not have changed.' - assert notification.status_reason == 'bounce' + assert notification.status_reason == status_reason