From 062084dcc483977c30e094a8b8cf62231d8881a3 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Thu, 3 Aug 2017 18:05:42 +0100 Subject: [PATCH 1/5] Allow for both an endpoint callback and SQS consumption --- app/celery/tasks.py | 7 +++++++ app/notifications/notifications_ses_callback.py | 8 +++++--- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index bece59f7c..ec3cf0a7e 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -60,6 +60,7 @@ from app.models import ( SMS_TYPE, ) from app.notifications.process_notifications import persist_notification +from app.notifications.notifications_ses_callback import process_ses_response from app.service.utils import service_allowed_to_send_to from app.statsd_decorators import statsd from notifications_utils.s3 import s3upload @@ -530,3 +531,9 @@ def process_incomplete_job(job_id): process_row(row_number, recipient, personalisation, template, job, job.service) job_complete(job, job.service, template, resumed=True) + + +@notify_celery.task(bind=True, name='process-ses-result') +@statsd(namespace="tasks") +def process_ses_results(self, response): + process_ses_response(response) diff --git a/app/notifications/notifications_ses_callback.py b/app/notifications/notifications_ses_callback.py index b1d0ba589..101b6bbe1 100644 --- a/app/notifications/notifications_ses_callback.py +++ b/app/notifications/notifications_ses_callback.py @@ -27,11 +27,13 @@ register_errors(ses_callback_blueprint) @ses_callback_blueprint.route('/notifications/email/ses', methods=['POST']) -def process_ses_response(): +def sns_callback_handler(): + process_ses_response(json.loads(request.data)) + + +def process_ses_response(ses_request): client_name = 'SES' try: - ses_request = json.loads(request.data) - subscribed_topic = autoconfirm_subscription(ses_request) if subscribed_topic: current_app.logger.info("Automatically subscribed to topic: {}".format(subscribed_topic)) From 7611d473b40c526ecf160fb988fb2e63a5b2fb81 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Fri, 4 Aug 2017 12:16:25 +0100 Subject: [PATCH 2/5] We no longer have guaranteed access to a request context --- .../notifications_ses_callback.py | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/app/notifications/notifications_ses_callback.py b/app/notifications/notifications_ses_callback.py index 101b6bbe1..62af94317 100644 --- a/app/notifications/notifications_ses_callback.py +++ b/app/notifications/notifications_ses_callback.py @@ -28,7 +28,11 @@ register_errors(ses_callback_blueprint) @ses_callback_blueprint.route('/notifications/email/ses', methods=['POST']) def sns_callback_handler(): - process_ses_response(json.loads(request.data)) + errors, status, kwargs = process_ses_response(json.loads(request.data)) + if errors: + raise InvalidRequest(errors, status) + + return jsonify(**kwargs), status def process_ses_response(ses_request): @@ -37,18 +41,16 @@ def process_ses_response(ses_request): subscribed_topic = autoconfirm_subscription(ses_request) if subscribed_topic: current_app.logger.info("Automatically subscribed to topic: {}".format(subscribed_topic)) - return jsonify( - result="success", message="SES callback succeeded" - ), 200 + return [], 200, {'result': "success", 'message': "SES callback succeeded"} errors = validate_callback_data(data=ses_request, fields=['Message'], client_name=client_name) if errors: - raise InvalidRequest(errors, status_code=400) + return errors, 400, {} ses_message = json.loads(ses_request['Message']) errors = validate_callback_data(data=ses_message, fields=['notificationType'], client_name=client_name) if errors: - raise InvalidRequest(errors, status_code=400) + return errors, 400, {} notification_type = ses_message['notificationType'] if notification_type == 'Bounce': @@ -60,7 +62,7 @@ def process_ses_response(ses_request): aws_response_dict = get_aws_responses(notification_type) except KeyError: error = "{} callback failed: status {} not found".format(client_name, notification_type) - raise InvalidRequest(error, status_code=400) + return error, 400, {} notification_status = aws_response_dict['notification_status'] @@ -73,7 +75,7 @@ def process_ses_response(ses_request): if not notification: error = "SES callback failed: notification either not found or already updated " \ "from sending. Status {} for notification reference {}".format(notification_status, reference) - raise InvalidRequest(error, status_code=404) + return error, 404, {} if not aws_response_dict['success']: current_app.logger.info( @@ -98,14 +100,12 @@ def process_ses_response(ses_request): create_outcome_notification_statistic_tasks(notification) - return jsonify( - result="success", message="SES callback succeeded" - ), 200 + return [], 200, {'result': "success", 'message': "SES callback succeeded"} except KeyError: - message = "SES callback failed: messageId missing" - raise InvalidRequest(message, status_code=400) + error = "SES callback failed: messageId missing" + return error, 400, {} - except ValueError as ex: + except ValueError: error = "{} callback failed: invalid json".format(client_name) - raise InvalidRequest(error, status_code=400) + return error, 400, {} From 61b665c908fb68d6f37f93ccdc5089a9aa7fbe86 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Fri, 4 Aug 2017 13:50:25 +0100 Subject: [PATCH 3/5] Update tests --- .../notifications_ses_callback.py | 6 - .../test_notifications_ses_callback.py | 179 ++++-------------- 2 files changed, 34 insertions(+), 151 deletions(-) diff --git a/app/notifications/notifications_ses_callback.py b/app/notifications/notifications_ses_callback.py index 62af94317..6d3c2807e 100644 --- a/app/notifications/notifications_ses_callback.py +++ b/app/notifications/notifications_ses_callback.py @@ -15,7 +15,6 @@ from app.dao import ( ) from app.celery.statistics_tasks import create_outcome_notification_statistic_tasks from app.notifications.process_client_response import validate_callback_data -from app.notifications.utils import autoconfirm_subscription ses_callback_blueprint = Blueprint('notifications_ses_callback', __name__) @@ -38,11 +37,6 @@ def sns_callback_handler(): def process_ses_response(ses_request): client_name = 'SES' try: - subscribed_topic = autoconfirm_subscription(ses_request) - if subscribed_topic: - current_app.logger.info("Automatically subscribed to topic: {}".format(subscribed_topic)) - return [], 200, {'result': "success", 'message': "SES callback succeeded"} - errors = validate_callback_data(data=ses_request, fields=['Message'], client_name=client_name) if errors: return errors, 400, {} diff --git a/tests/app/notifications/test_notifications_ses_callback.py b/tests/app/notifications/test_notifications_ses_callback.py index 0a02b5d58..de2241f69 100644 --- a/tests/app/notifications/test_notifications_ses_callback.py +++ b/tests/app/notifications/test_notifications_ses_callback.py @@ -3,11 +3,10 @@ from unittest.mock import call from flask import json from freezegun import freeze_time -from requests import HTTPError -import pytest from app import statsd_client from app.dao.notifications_dao import get_notification_by_id +from app.notifications.notifications_ses_callback import process_ses_response from tests.app.conftest import sample_notification as create_sample_notification @@ -24,68 +23,9 @@ def test_ses_callback_should_fail_if_invalid_json(client, mocker): stats_mock = mocker.patch( 'app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks' ) - - response = client.post( - path='/notifications/email/ses', - data="nonsense", - headers=[('Content-Type', 'text/plain; charset=UTF-8')] - ) - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 400 - assert json_resp['result'] == 'error' - assert json_resp['message'] == 'SES callback failed: invalid json' - stats_mock.assert_not_called() - - -def test_ses_callback_should_autoconfirm_subscriptions(client, rmock, mocker): - stats_mock = mocker.patch( - 'app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks' - ) - - endpoint = json.loads(ses_confirmation_callback())['SubscribeURL'] - rmock.request( - "GET", - endpoint, - json={"status": "success"}, - status_code=200) - - response = client.post( - path='/notifications/email/ses', - data=ses_confirmation_callback(), - headers=[('Content-Type', 'text/plain; charset=UTF-8')] - ) - json_resp = json.loads(response.get_data(as_text=True)) - - assert rmock.called - assert rmock.request_history[0].url == endpoint - assert response.status_code == 200 - assert json_resp['result'] == 'success' - assert json_resp['message'] == 'SES callback succeeded' - stats_mock.assert_not_called() - - -def test_ses_callback_autoconfirm_raises_exception_if_not_200(client, rmock, mocker): - stats_mock = mocker.patch( - 'app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks' - ) - - endpoint = json.loads(ses_confirmation_callback())['SubscribeURL'] - rmock.request( - "GET", - endpoint, - json={"status": "not allowed"}, - status_code=405) - - with pytest.raises(HTTPError) as exc: - client.post( - path='/notifications/email/ses', - data=ses_confirmation_callback(), - headers=[('Content-Type', 'text/plain; charset=UTF-8')] - ) - - assert rmock.called - assert rmock.request_history[0].url == endpoint - assert exc.value.response.status_code == 405 + errors, status, _ = process_ses_response('nonsense') + assert status == 400 + assert errors == 'SES callback failed: invalid json' stats_mock.assert_not_called() @@ -94,15 +34,9 @@ def test_ses_callback_should_fail_if_invalid_notification_type(client, mocker): 'app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks' ) - response = client.post( - path='/notifications/email/ses', - data=ses_invalid_notification_type_callback(), - headers=[('Content-Type', 'text/plain; charset=UTF-8')] - ) - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 400 - assert json_resp['result'] == 'error' - assert json_resp['message'] == 'SES callback failed: status Unknown not found' + errors, status, _ = process_ses_response(json.loads(ses_invalid_notification_type_callback())) + assert status == 400 + assert errors == 'SES callback failed: status Unknown not found' stats_mock.assert_not_called() @@ -111,15 +45,9 @@ def test_ses_callback_should_fail_if_missing_message_id(client, mocker): 'app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks' ) - response = client.post( - path='/notifications/email/ses', - data=ses_missing_notification_id_callback(), - headers=[('Content-Type', 'text/plain; charset=UTF-8')] - ) - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 400 - assert json_resp['result'] == 'error' - assert json_resp['message'] == 'SES callback failed: messageId missing' + errors, status, _ = process_ses_response(json.loads(ses_missing_notification_id_callback())) + assert status == 400 + assert errors == 'SES callback failed: messageId missing' stats_mock.assert_not_called() @@ -128,15 +56,9 @@ def test_ses_callback_should_fail_if_notification_cannot_be_found(notify_db, not 'app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks' ) - response = client.post( - path='/notifications/email/ses', - data=ses_invalid_notification_id_callback(), - headers=[('Content-Type', 'text/plain; charset=UTF-8')] - ) - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 404 - assert json_resp['result'] == 'error' - assert json_resp['message'] == 'SES callback failed: notification either not found or already updated from sending. Status delivered for notification reference missing' # noqa + errors, status, _ = process_ses_response(json.loads(ses_invalid_notification_id_callback())) + assert status == 404 + assert errors == 'SES callback failed: notification either not found or already updated from sending. Status delivered for notification reference missing' # noqa stats_mock.assert_not_called() @@ -164,15 +86,10 @@ def test_ses_callback_should_update_notification_status( assert get_notification_by_id(notification.id).status == 'sending' - response = client.post( - path='/notifications/email/ses', - data=ses_notification_callback(), - headers=[('Content-Type', 'text/plain; charset=UTF-8')] - ) - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 200 - assert json_resp['result'] == 'success' - assert json_resp['message'] == 'SES callback succeeded' + _, status, res = process_ses_response(json.loads(ses_notification_callback())) + assert status == 200 + assert res['result'] == 'success' + assert res['message'] == 'SES callback succeeded' assert get_notification_by_id(notification.id).status == 'delivered' statsd_client.timing_with_dates.assert_any_call( "callback.ses.elapsed-time", datetime.utcnow(), notification.sent_at @@ -216,25 +133,13 @@ def test_ses_callback_should_update_multiple_notification_status_sent( sent_at=datetime.utcnow(), status='sending') - resp1 = client.post( - path='/notifications/email/ses', - data=ses_notification_callback(ref='ref1'), - headers=[('Content-Type', 'text/plain; charset=UTF-8')] - ) - resp2 = client.post( - path='/notifications/email/ses', - data=ses_notification_callback(ref='ref2'), - headers=[('Content-Type', 'text/plain; charset=UTF-8')] - ) - resp3 = client.post( - path='/notifications/email/ses', - data=ses_notification_callback(ref='ref3'), - headers=[('Content-Type', 'text/plain; charset=UTF-8')] - ) + resp1 = process_ses_response(json.loads(data=ses_notification_callback(ref='ref1'))) + resp2 = process_ses_response(json.loads(data=ses_notification_callback(ref='ref2'))) + resp3 = process_ses_response(json.loads(data=ses_notification_callback(ref='ref3'))) - assert resp1.status_code == 200 - assert resp2.status_code == 200 - assert resp3.status_code == 200 + assert resp1[1] == 200 + assert resp2[1] == 200 + assert resp3[1] == 200 stats_mock.assert_has_calls([ call(notification1), call(notification2), @@ -262,15 +167,10 @@ def test_ses_callback_should_set_status_to_temporary_failure(client, ) assert get_notification_by_id(notification.id).status == 'sending' - response = client.post( - path='/notifications/email/ses', - data=ses_soft_bounce_callback(), - headers=[('Content-Type', 'text/plain; charset=UTF-8')] - ) - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 200 - assert json_resp['result'] == 'success' - assert json_resp['message'] == 'SES callback succeeded' + _, status, res = process_ses_response(json.loads(data=ses_soft_bounce_callback())) + assert status == 200 + assert res['result'] == 'success' + assert res['message'] == 'SES callback succeeded' assert get_notification_by_id(notification.id).status == 'temporary-failure' stats_mock.assert_called_once_with(notification) @@ -295,15 +195,9 @@ def test_ses_callback_should_not_set_status_once_status_is_delivered(client, assert get_notification_by_id(notification.id).status == 'delivered' - response = client.post( - path='/notifications/email/ses', - data=ses_soft_bounce_callback(), - headers=[('Content-Type', 'text/plain; charset=UTF-8')] - ) - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 404 - assert json_resp['result'] == 'error' - assert json_resp['message'] == 'SES callback failed: notification either not found or already updated from sending. Status temporary-failure for notification reference ref' # noqa + error, status, _ = process_ses_response(json.loads(data=ses_soft_bounce_callback())) + assert status == 404 + assert error == 'SES callback failed: notification either not found or already updated from sending. Status temporary-failure for notification reference ref' # noqa assert get_notification_by_id(notification.id).status == 'delivered' stats_mock.assert_not_called() @@ -328,15 +222,10 @@ def test_ses_callback_should_set_status_to_permanent_failure(client, assert get_notification_by_id(notification.id).status == 'sending' - response = client.post( - path='/notifications/email/ses', - data=ses_hard_bounce_callback(), - headers=[('Content-Type', 'text/plain; charset=UTF-8')] - ) - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 200 - assert json_resp['result'] == 'success' - assert json_resp['message'] == 'SES callback succeeded' + _, status, res = process_ses_response(json.loads(data=ses_hard_bounce_callback())) + assert status == 200 + assert res['result'] == 'success' + assert res['message'] == 'SES callback succeeded' assert get_notification_by_id(notification.id).status == 'permanent-failure' stats_mock.assert_called_once_with(notification) From fe5756a1aa11e55bf25c304b037d3d4b6aee7604 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Fri, 4 Aug 2017 14:15:27 +0100 Subject: [PATCH 4/5] Fix tests --- .../notifications/test_notifications_ses_callback.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/app/notifications/test_notifications_ses_callback.py b/tests/app/notifications/test_notifications_ses_callback.py index de2241f69..94c93e7bf 100644 --- a/tests/app/notifications/test_notifications_ses_callback.py +++ b/tests/app/notifications/test_notifications_ses_callback.py @@ -133,9 +133,9 @@ def test_ses_callback_should_update_multiple_notification_status_sent( sent_at=datetime.utcnow(), status='sending') - resp1 = process_ses_response(json.loads(data=ses_notification_callback(ref='ref1'))) - resp2 = process_ses_response(json.loads(data=ses_notification_callback(ref='ref2'))) - resp3 = process_ses_response(json.loads(data=ses_notification_callback(ref='ref3'))) + resp1 = process_ses_response(json.loads(ses_notification_callback(ref='ref1'))) + resp2 = process_ses_response(json.loads(ses_notification_callback(ref='ref2'))) + resp3 = process_ses_response(json.loads(ses_notification_callback(ref='ref3'))) assert resp1[1] == 200 assert resp2[1] == 200 @@ -167,7 +167,7 @@ def test_ses_callback_should_set_status_to_temporary_failure(client, ) assert get_notification_by_id(notification.id).status == 'sending' - _, status, res = process_ses_response(json.loads(data=ses_soft_bounce_callback())) + _, status, res = process_ses_response(json.loads(ses_soft_bounce_callback())) assert status == 200 assert res['result'] == 'success' assert res['message'] == 'SES callback succeeded' @@ -195,7 +195,7 @@ def test_ses_callback_should_not_set_status_once_status_is_delivered(client, assert get_notification_by_id(notification.id).status == 'delivered' - error, status, _ = process_ses_response(json.loads(data=ses_soft_bounce_callback())) + error, status, _ = process_ses_response(json.loads(ses_soft_bounce_callback())) assert status == 404 assert error == 'SES callback failed: notification either not found or already updated from sending. Status temporary-failure for notification reference ref' # noqa assert get_notification_by_id(notification.id).status == 'delivered' @@ -222,7 +222,7 @@ def test_ses_callback_should_set_status_to_permanent_failure(client, assert get_notification_by_id(notification.id).status == 'sending' - _, status, res = process_ses_response(json.loads(data=ses_hard_bounce_callback())) + _, status, res = process_ses_response(json.loads(ses_hard_bounce_callback())) assert status == 200 assert res['result'] == 'success' assert res['message'] == 'SES callback succeeded' From 6e6c3c050bc62d2388f43d6cb2444ba2eb8aaebd Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Fri, 4 Aug 2017 15:59:54 +0100 Subject: [PATCH 5/5] Temporary test fix This should be removed when the SES endpoint is removed --- app/notifications/notifications_ses_callback.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/notifications/notifications_ses_callback.py b/app/notifications/notifications_ses_callback.py index 6d3c2807e..45053d615 100644 --- a/app/notifications/notifications_ses_callback.py +++ b/app/notifications/notifications_ses_callback.py @@ -37,6 +37,11 @@ def sns_callback_handler(): def process_ses_response(ses_request): client_name = 'SES' try: + + # TODO: remove this check once the sns_callback_handler is removed + if not isinstance(ses_request, dict): + ses_request = json.loads(ses_request) + errors = validate_callback_data(data=ses_request, fields=['Message'], client_name=client_name) if errors: return errors, 400, {}