From 43f1f48093e668f81fdcb5d51a6ad86f8ff83ca8 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Fri, 20 Nov 2020 14:10:13 +0000 Subject: [PATCH] Add notification ID to SES bounce reason At the moment we log everytime we get a bounce from SES, however we don't link it to a particular notification so it's hard to know for what sub reason a notifcation did not deliver by looking at the logs. This commit changes this by now looking the bounce reason after we have found the notification ID and including them together. So if you know search for a notification ID in Kibana, you will see full logs for why it failed to deliver. --- app/celery/process_ses_receipts_tasks.py | 6 +++++- app/notifications/notifications_ses_callback.py | 3 +-- tests/app/celery/test_process_ses_receipts_tasks.py | 4 ++++ 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/app/celery/process_ses_receipts_tasks.py b/app/celery/process_ses_receipts_tasks.py index 95e64b1f0..153835287 100644 --- a/app/celery/process_ses_receipts_tasks.py +++ b/app/celery/process_ses_receipts_tasks.py @@ -26,9 +26,10 @@ def process_ses_results(self, response): try: ses_message = json.loads(response['Message']) notification_type = ses_message['notificationType'] + bounce_message = None if notification_type == 'Bounce': - notification_type = determine_notification_bounce_type(notification_type, ses_message) + notification_type, bounce_message = determine_notification_bounce_type(notification_type, ses_message) elif notification_type == 'Complaint': _check_and_queue_complaint_callback_task(*handle_complaint(ses_message)) return True @@ -54,6 +55,9 @@ def process_ses_results(self, response): ) return + if bounce_message: + current_app.logger.info(f"SES bounce for notification ID {notification.id}: {bounce_message}") + if notification.status not in [NOTIFICATION_SENDING, NOTIFICATION_PENDING]: notifications_dao._duplicate_update_warning( notification=notification, diff --git a/app/notifications/notifications_ses_callback.py b/app/notifications/notifications_ses_callback.py index 101ff1ecc..a124e691f 100644 --- a/app/notifications/notifications_ses_callback.py +++ b/app/notifications/notifications_ses_callback.py @@ -17,12 +17,11 @@ from app.config import QueueNames def determine_notification_bounce_type(notification_type, ses_message): remove_emails_from_bounce(ses_message) - current_app.logger.info('SES bounce dict: {}'.format(ses_message)) if ses_message['bounce']['bounceType'] == 'Permanent': notification_type = ses_message['bounce']['bounceType'] # permanent or not else: notification_type = 'Temporary' - return notification_type + return notification_type, ses_message def handle_complaint(ses_message): diff --git a/tests/app/celery/test_process_ses_receipts_tasks.py b/tests/app/celery/test_process_ses_receipts_tasks.py index 2e7462904..fab6ccaec 100644 --- a/tests/app/celery/test_process_ses_receipts_tasks.py +++ b/tests/app/celery/test_process_ses_receipts_tasks.py @@ -200,6 +200,7 @@ def test_ses_callback_should_set_status_to_temporary_failure(client, send_mock = mocker.patch( 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' ) + mock_logger = mocker.patch('app.celery.process_ses_receipts_tasks.current_app.logger.info') notification = create_notification( template=sample_email_template, status='sending', @@ -210,6 +211,7 @@ def test_ses_callback_should_set_status_to_temporary_failure(client, assert process_ses_results(ses_soft_bounce_callback(reference='ref')) assert get_notification_by_id(notification.id).status == 'temporary-failure' assert send_mock.called + assert f'SES bounce for notification ID {notification.id}: ' in mock_logger.call_args[0][0] def test_ses_callback_should_set_status_to_permanent_failure(client, @@ -219,6 +221,7 @@ def test_ses_callback_should_set_status_to_permanent_failure(client, send_mock = mocker.patch( 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' ) + mock_logger = mocker.patch('app.celery.process_ses_receipts_tasks.current_app.logger.info') notification = create_notification( template=sample_email_template, status='sending', @@ -230,6 +233,7 @@ def test_ses_callback_should_set_status_to_permanent_failure(client, assert process_ses_results(ses_hard_bounce_callback(reference='ref')) assert get_notification_by_id(notification.id).status == 'permanent-failure' assert send_mock.called + assert f'SES bounce for notification ID {notification.id}: ' in mock_logger.call_args[0][0] def test_ses_callback_should_send_on_complaint_to_user_callback_api(sample_email_template, mocker):