diff --git a/app/notifications/notifications_ses_callback.py b/app/notifications/notifications_ses_callback.py index 45053d615..4780b4bd7 100644 --- a/app/notifications/notifications_ses_callback.py +++ b/app/notifications/notifications_ses_callback.py @@ -27,11 +27,12 @@ register_errors(ses_callback_blueprint) @ses_callback_blueprint.route('/notifications/email/ses', methods=['POST']) def sns_callback_handler(): - errors, status, kwargs = process_ses_response(json.loads(request.data)) + errors = process_ses_response(json.loads(request.data)) if errors: - raise InvalidRequest(errors, status) + current_app.logger.error(errors) + raise InvalidRequest(errors, 400) - return jsonify(**kwargs), status + return 200 def process_ses_response(ses_request): @@ -44,12 +45,12 @@ def process_ses_response(ses_request): errors = validate_callback_data(data=ses_request, fields=['Message'], client_name=client_name) if errors: - return errors, 400, {} + return errors ses_message = json.loads(ses_request['Message']) errors = validate_callback_data(data=ses_message, fields=['notificationType'], client_name=client_name) if errors: - return errors, 400, {} + return errors notification_type = ses_message['notificationType'] if notification_type == 'Bounce': @@ -61,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) - return error, 400, {} + return error notification_status = aws_response_dict['notification_status'] @@ -74,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) - return error, 404, {} + return error if not aws_response_dict['success']: current_app.logger.info( @@ -99,12 +100,12 @@ def process_ses_response(ses_request): create_outcome_notification_statistic_tasks(notification) - return [], 200, {'result': "success", 'message': "SES callback succeeded"} + return except KeyError: error = "SES callback failed: messageId missing" - return error, 400, {} + return error except ValueError: error = "{} callback failed: invalid json".format(client_name) - return error, 400, {} + return error diff --git a/tests/app/notifications/test_notifications_ses_callback.py b/tests/app/notifications/test_notifications_ses_callback.py index 5d6ec9943..cfcb4a4b8 100644 --- a/tests/app/notifications/test_notifications_ses_callback.py +++ b/tests/app/notifications/test_notifications_ses_callback.py @@ -16,15 +16,14 @@ def test_ses_callback_should_not_need_auth(client): data=ses_notification_callback(), headers=[('Content-Type', 'text/plain; charset=UTF-8')] ) - assert response.status_code == 404 + assert response.status_code == 400 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' ) - errors, status, _ = process_ses_response('nonsense') - assert status == 400 + errors = process_ses_response('nonsense') assert errors == 'SES callback failed: invalid json' stats_mock.assert_not_called() @@ -34,8 +33,7 @@ def test_ses_callback_should_fail_if_invalid_notification_type(client, mocker): 'app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks' ) - errors, status, _ = process_ses_response(json.loads(ses_invalid_notification_type_callback())) - assert status == 400 + errors = process_ses_response(json.loads(ses_invalid_notification_type_callback())) assert errors == 'SES callback failed: status Unknown not found' stats_mock.assert_not_called() @@ -45,8 +43,7 @@ def test_ses_callback_should_fail_if_missing_message_id(client, mocker): 'app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks' ) - errors, status, _ = process_ses_response(json.loads(ses_missing_notification_id_callback())) - assert status == 400 + errors = process_ses_response(json.loads(ses_missing_notification_id_callback())) assert errors == 'SES callback failed: messageId missing' stats_mock.assert_not_called() @@ -56,8 +53,7 @@ def test_ses_callback_should_fail_if_notification_cannot_be_found(notify_db, not 'app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks' ) - errors, status, _ = process_ses_response(json.loads(ses_invalid_notification_id_callback())) - assert status == 404 + errors = process_ses_response(json.loads(ses_invalid_notification_id_callback())) 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() @@ -86,10 +82,8 @@ def test_ses_callback_should_update_notification_status( assert get_notification_by_id(notification.id).status == 'sending' - _, status, res = process_ses_response(json.loads(ses_notification_callback())) - assert status == 200 - assert res['result'] == 'success' - assert res['message'] == 'SES callback succeeded' + errors = process_ses_response(json.loads(ses_notification_callback())) + assert errors is None 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 @@ -133,13 +127,10 @@ def test_ses_callback_should_update_multiple_notification_status_sent( sent_at=datetime.utcnow(), status='sending') - 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 process_ses_response(json.loads(ses_notification_callback(ref='ref1'))) is None + assert process_ses_response(json.loads(ses_notification_callback(ref='ref2'))) is None + assert process_ses_response(json.loads(ses_notification_callback(ref='ref3'))) is None - assert resp1[1] == 200 - assert resp2[1] == 200 - assert resp3[1] == 200 stats_mock.assert_has_calls([ call(notification1), call(notification2), @@ -166,12 +157,7 @@ def test_ses_callback_should_set_status_to_temporary_failure(client, sent_at=datetime.utcnow() ) assert get_notification_by_id(notification.id).status == 'sending' - - _, status, res = process_ses_response(json.loads(ses_soft_bounce_callback())) - - assert status == 200 - assert res['result'] == 'success' - assert res['message'] == 'SES callback succeeded' + assert process_ses_response(json.loads(ses_soft_bounce_callback())) is None assert get_notification_by_id(notification.id).status == 'temporary-failure' stats_mock.assert_called_once_with(notification) @@ -195,9 +181,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(ses_soft_bounce_callback())) - - assert status == 404 + error = process_ses_response(json.loads(ses_soft_bounce_callback())) 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() @@ -222,11 +206,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(ses_hard_bounce_callback())) - - assert status == 200 - assert res['result'] == 'success' - assert res['message'] == 'SES callback succeeded' + assert process_ses_response(json.loads(ses_hard_bounce_callback())) is None assert get_notification_by_id(notification.id).status == 'permanent-failure' stats_mock.assert_called_once_with(notification)