From 7fa42c2cc5d98b1f2c9fda024b80a11c3406f883 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 4 Jun 2018 17:29:58 +0100 Subject: [PATCH] When SES callback is for a complaint save that to the new complaints table. When handling the complaint we don't want to throw an exception if the message is missing fields. Only log an exception if we are unable to tie a complaint to a notification. --- app/dao/complaint_dao.py | 12 ++++ .../notifications_ses_callback.py | 31 +++++++-- .../celery/test_process_ses_receipts_tasks.py | 53 +++------------ tests/app/dao/test_complaint_dao.py | 64 ++++++++++++++++++ tests/app/db.py | 66 +++++++++++++++++++ .../test_notifications_ses_callback.py | 38 ++++++++++- 6 files changed, 214 insertions(+), 50 deletions(-) create mode 100644 app/dao/complaint_dao.py create mode 100644 tests/app/dao/test_complaint_dao.py diff --git a/app/dao/complaint_dao.py b/app/dao/complaint_dao.py new file mode 100644 index 000000000..85ebe3b0b --- /dev/null +++ b/app/dao/complaint_dao.py @@ -0,0 +1,12 @@ +from app import db +from app.dao.dao_utils import transactional +from app.models import Complaint + + +@transactional +def save_complaint(complaint): + db.session.add(complaint) + + +def fetch_complaints_by_service(service_id): + return Complaint.query.filter_by(service_id=service_id).all() diff --git a/app/notifications/notifications_ses_callback.py b/app/notifications/notifications_ses_callback.py index 8650f8019..6e2351fb4 100644 --- a/app/notifications/notifications_ses_callback.py +++ b/app/notifications/notifications_ses_callback.py @@ -10,7 +10,10 @@ from app.clients.email.aws_ses import get_aws_responses from app.dao import ( notifications_dao ) +from app.dao.complaint_dao import save_complaint +from app.dao.notifications_dao import dao_get_notification_history_by_reference from app.dao.service_callback_api_dao import get_service_callback_api_for_service +from app.models import Complaint from app.notifications.process_client_response import validate_callback_data from app.celery.service_callback_tasks import ( send_delivery_status_to_service, @@ -35,10 +38,7 @@ def process_ses_response(ses_request): if notification_type == 'Bounce': notification_type = determine_notification_bounce_type(notification_type, ses_message) elif notification_type == 'Complaint': - # Complaints are going to be stored in a table of it's own, - # this will no longer update the status of a notification as it does now. - remove_emails_from_complaint(ses_message) - current_app.logger.info("Complaint from SES: \n{}".format(ses_message)) + handle_complaint(ses_request) return try: @@ -108,6 +108,29 @@ def remove_emails_from_bounce(bounce_dict): recip.pop('emailAddress') +def handle_complaint(ses_request): + ses_message = json.loads(ses_request['Message']) + remove_emails_from_complaint(ses_message) + current_app.logger.info("Complaint from SES: \n{}".format(ses_message)) + # It is possible that the we get a key error, let this fail so we can investigate. + try: + reference = ses_request['MessageId'] + except KeyError as e: + current_app.logger.exception("Complaint from SES failed to get reference from message", e) + return + notification = dao_get_notification_history_by_reference(reference) + ses_complaint = ses_message.get('complaint', None) + + complaint = Complaint( + notification_id=notification.id, + service_id=notification.service_id, + ses_feedback_id=ses_complaint.get('feedbackId', None) if ses_complaint else None, + complaint_type=ses_complaint.get('complaintFeedbackType', None) if ses_complaint else None, + complaint_date=ses_complaint.get('timestamp', None) if ses_complaint else None + ) + save_complaint(complaint) + + def remove_emails_from_complaint(complaint_dict): complaint_dict['complaint'].pop('complainedRecipients') diff --git a/tests/app/celery/test_process_ses_receipts_tasks.py b/tests/app/celery/test_process_ses_receipts_tasks.py index 7c881abbf..d8f97096b 100644 --- a/tests/app/celery/test_process_ses_receipts_tasks.py +++ b/tests/app/celery/test_process_ses_receipts_tasks.py @@ -2,9 +2,13 @@ import json from datetime import datetime from app.celery.process_ses_receipts_tasks import process_ses_results +from app.models import Complaint from app.notifications.notifications_ses_callback import remove_emails_from_complaint -from tests.app.db import create_notification +from tests.app.db import ( + create_notification, ses_complaint_callback, + ses_notification_callback +) def test_process_ses_results(sample_email_template): @@ -33,11 +37,15 @@ def test_process_ses_results_retry_called(notify_db, mocker): assert mocked.call_count != 0 -def test_process_ses_results_in_complaint(notify_db, mocker): +def test_process_ses_results_in_complaint(sample_email_template, mocker): + notification = create_notification(template=sample_email_template, reference='ref1') mocked = mocker.patch("app.dao.notifications_dao.update_notification_status_by_reference") response = json.loads(ses_complaint_callback()) process_ses_results(response=response) assert mocked.call_count == 0 + complaints = Complaint.query.all() + assert len(complaints) == 1 + assert complaints[0].notification_id == notification.id def test_remove_emails_from_complaint(): @@ -45,44 +53,3 @@ def test_remove_emails_from_complaint(): test_json = json.loads(json.loads(test_message)['Message']) remove_emails_from_complaint(test_json) assert "recipient1@example.com" not in test_json - - -def ses_notification_callback(): - return '{\n "Type" : "Notification",\n "MessageId" : "ref1",' \ - '\n "TopicArn" : "arn:aws:sns:eu-west-1:123456789012:testing",' \ - '\n "Message" : "{\\"notificationType\\":\\"Delivery\\",' \ - '\\"mail\\":{\\"timestamp\\":\\"2016-03-14T12:35:25.909Z\\",' \ - '\\"source\\":\\"test@test-domain.com\\",' \ - '\\"sourceArn\\":\\"arn:aws:ses:eu-west-1:123456789012:identity/testing-notify\\",' \ - '\\"sendingAccountId\\":\\"123456789012\\",' \ - '\\"messageId\\":\\"ref1\\",' \ - '\\"destination\\":[\\"testing@digital.cabinet-office.gov.uk\\"]},' \ - '\\"delivery\\":{\\"timestamp\\":\\"2016-03-14T12:35:26.567Z\\",' \ - '\\"processingTimeMillis\\":658,' \ - '\\"recipients\\":[\\"testing@digital.cabinet-office.gov.uk\\"],' \ - '\\"smtpResponse\\":\\"250 2.0.0 OK 1457958926 uo5si26480932wjc.221 - gsmtp\\",' \ - '\\"reportingMTA\\":\\"a6-238.smtp-out.eu-west-1.amazonses.com\\"}}",' \ - '\n "Timestamp" : "2016-03-14T12:35:26.665Z",\n "SignatureVersion" : "1",' \ - '\n "Signature" : "X8d7eTAOZ6wlnrdVVPYanrAlsX0SMPfOzhoTEBnQqYkrNWTqQY91C0f3bxtPdUhUt' \ - 'OowyPAOkTQ4KnZuzphfhVb2p1MyVYMxNKcBFB05/qaCX99+92fjw4x9LeUOwyGwMv5F0Vkfi5qZCcEw69uVrhYL' \ - 'VSTFTrzi/yCtru+yFULMQ6UhbY09GwiP6hjxZMVr8aROQy5lLHglqQzOuSZ4KeD85JjifHdKzlx8jjQ+uj+FLzHXPMA' \ - 'PmPU1JK9kpoHZ1oPshAFgPDpphJe+HwcJ8ezmk+3AEUr3wWli3xF+49y8Z2anASSVp6YI2YP95UT8Rlh3qT3T+V9V8rbSVislxA==",' \ - '\n "SigningCertURL" : "https://sns.eu-west-1.amazonaws.com/SimpleNotificationService-bb750' \ - 'dd426d95ee9390147a5624348ee.pem",' \ - '\n "UnsubscribeURL" : "https://sns.eu-west-1.amazonaws.com/?Action=Unsubscribe&S' \ - 'ubscriptionArn=arn:aws:sns:eu-west-1:302763885840:preview-emails:d6aad3ef-83d6-4cf3-a470-54e2e75916da"\n}' - - -def ses_complaint_callback(): - """ - https://docs.aws.amazon.com/ses/latest/DeveloperGuide/notification-contents.html#complaint-object - """ - return '{\n "Type" : "Notification",\n "MessageId" : "ref1",' \ - '\n "TopicArn" : "arn:aws:sns:eu-west-1:123456789012:testing",' \ - '\n "Message" : "{\\"notificationType\\":\\"Complaint\\",' \ - '\\"complaint\\": {\\"userAgent\\":\\"AnyCompany Feedback Loop (V0.01)\\",' \ - '\\"complainedRecipients\\":[{\\"emailAddress\\":\\"recipient1@example.com\\"}],' \ - '\\"complaintFeedbackType\\":\\"abuse\\", ' \ - '\\"arrivalDate\\":\\"2009-12-03T04:24:21.000-05:00\\", ' \ - '\\"timestamp\\":\\"2012-05-25T14:59:38.623Z\\", ' \ - '\\"feedbackId\\":\\"someSESID\\"}}"\n}' diff --git a/tests/app/dao/test_complaint_dao.py b/tests/app/dao/test_complaint_dao.py new file mode 100644 index 000000000..92478cc33 --- /dev/null +++ b/tests/app/dao/test_complaint_dao.py @@ -0,0 +1,64 @@ +import uuid + +from datetime import datetime + +from app.dao.complaint_dao import save_complaint, fetch_complaints_by_service +from app.models import Complaint +from tests.app.db import create_service, create_template, create_notification + + +def test_fetch_complaint_by_service_returns_one(sample_service, sample_email_notification): + complaint = Complaint(notification_id=sample_email_notification.id, + service_id=sample_service.id, + ses_feedback_id=str(uuid.uuid4()), + complaint_type='abuse', + complaint_date=datetime.utcnow() + ) + + save_complaint(complaint) + + complaints = fetch_complaints_by_service(service_id=sample_service.id) + assert len(complaints) == 1 + assert complaints[0] == complaint + + +def test_fetch_complaint_by_service_returns_empty_list(sample_service, sample_email_notification): + complaints = fetch_complaints_by_service(service_id=sample_service.id) + assert len(complaints) == 0 + + +def test_fetch_complaint_by_service_return_many(notify_db_session): + service_1 = create_service(service_name='first') + service_2 = create_service(service_name='second') + template_1 = create_template(service=service_1, template_type='email') + template_2 = create_template(service=service_2, template_type='email') + notification_1 = create_notification(template=template_1) + notification_2 = create_notification(template=template_2) + notification_3 = create_notification(template=template_2) + complaint_1 = Complaint(notification_id=notification_1.id, + service_id=service_1.id, + ses_feedback_id=str(uuid.uuid4()), + complaint_type='abuse', + complaint_date=datetime.utcnow() + ) + complaint_2 = Complaint(notification_id=notification_2.id, + service_id=service_2.id, + ses_feedback_id=str(uuid.uuid4()), + complaint_type='abuse', + complaint_date=datetime.utcnow() + ) + complaint_3 = Complaint(notification_id=notification_3.id, + service_id=service_2.id, + ses_feedback_id=str(uuid.uuid4()), + complaint_type='abuse', + complaint_date=datetime.utcnow() + ) + + save_complaint(complaint_1) + save_complaint(complaint_2) + save_complaint(complaint_3) + + complaints = fetch_complaints_by_service(service_id=service_2.id) + assert len(complaints) == 2 + assert complaints[0] == complaint_2 + assert complaints[1] == complaint_3 diff --git a/tests/app/db.py b/tests/app/db.py index b7b618ca9..76fa27895 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -562,3 +562,69 @@ def create_ft_billing(bst_date, db.session.add(data) db.session.commit() return data + + +def ses_complaint_callback_malformed_message_id(): + return '{\n "Type" : "Notification",\n "msgId" : "ref1",' \ + '\n "TopicArn" : "arn:aws:sns:eu-west-1:123456789012:testing",' \ + '\n "Message" : "{\\"notificationType\\":\\"Complaint\\",' \ + '\\"complaint\\": {\\"userAgent\\":\\"AnyCompany Feedback Loop (V0.01)\\",' \ + '\\"complainedRecipients\\":[{\\"emailAddress\\":\\"recipient1@example.com\\"}],' \ + '\\"arrivalDate\\":\\"2009-12-03T04:24:21.000-05:00\\", ' \ + '\\"timestamp\\":\\"2012-05-25T14:59:38.623Z\\", ' \ + '\\"feedbackId\\":\\"someSESID\\"}}"\n}' + + +def ses_complaint_callback_with_missing_complaint_type(): + """ + https://docs.aws.amazon.com/ses/latest/DeveloperGuide/notification-contents.html#complaint-object + """ + return '{\n "Type" : "Notification",\n "MessageId" : "ref1",' \ + '\n "TopicArn" : "arn:aws:sns:eu-west-1:123456789012:testing",' \ + '\n "Message" : "{\\"notificationType\\":\\"Complaint\\",' \ + '\\"complaint\\": {\\"userAgent\\":\\"AnyCompany Feedback Loop (V0.01)\\",' \ + '\\"complainedRecipients\\":[{\\"emailAddress\\":\\"recipient1@example.com\\"}],' \ + '\\"arrivalDate\\":\\"2009-12-03T04:24:21.000-05:00\\", ' \ + '\\"timestamp\\":\\"2012-05-25T14:59:38.623Z\\", ' \ + '\\"feedbackId\\":\\"someSESID\\"}}"\n}' + + +def ses_complaint_callback(): + """ + https://docs.aws.amazon.com/ses/latest/DeveloperGuide/notification-contents.html#complaint-object + """ + return '{\n "Type" : "Notification",\n "MessageId" : "ref1",' \ + '\n "TopicArn" : "arn:aws:sns:eu-west-1:123456789012:testing",' \ + '\n "Message" : "{\\"notificationType\\":\\"Complaint\\",' \ + '\\"complaint\\": {\\"userAgent\\":\\"AnyCompany Feedback Loop (V0.01)\\",' \ + '\\"complainedRecipients\\":[{\\"emailAddress\\":\\"recipient1@example.com\\"}],' \ + '\\"complaintFeedbackType\\":\\"abuse\\", ' \ + '\\"arrivalDate\\":\\"2009-12-03T04:24:21.000-05:00\\", ' \ + '\\"timestamp\\":\\"2012-05-25T14:59:38.623Z\\", ' \ + '\\"feedbackId\\":\\"someSESID\\"}}"\n}' + + +def ses_notification_callback(): + return '{\n "Type" : "Notification",\n "MessageId" : "ref1",' \ + '\n "TopicArn" : "arn:aws:sns:eu-west-1:123456789012:testing",' \ + '\n "Message" : "{\\"notificationType\\":\\"Delivery\\",' \ + '\\"mail\\":{\\"timestamp\\":\\"2016-03-14T12:35:25.909Z\\",' \ + '\\"source\\":\\"test@test-domain.com\\",' \ + '\\"sourceArn\\":\\"arn:aws:ses:eu-west-1:123456789012:identity/testing-notify\\",' \ + '\\"sendingAccountId\\":\\"123456789012\\",' \ + '\\"messageId\\":\\"ref1\\",' \ + '\\"destination\\":[\\"testing@digital.cabinet-office.gov.uk\\"]},' \ + '\\"delivery\\":{\\"timestamp\\":\\"2016-03-14T12:35:26.567Z\\",' \ + '\\"processingTimeMillis\\":658,' \ + '\\"recipients\\":[\\"testing@digital.cabinet-office.gov.uk\\"],' \ + '\\"smtpResponse\\":\\"250 2.0.0 OK 1457958926 uo5si26480932wjc.221 - gsmtp\\",' \ + '\\"reportingMTA\\":\\"a6-238.smtp-out.eu-west-1.amazonses.com\\"}}",' \ + '\n "Timestamp" : "2016-03-14T12:35:26.665Z",\n "SignatureVersion" : "1",' \ + '\n "Signature" : "X8d7eTAOZ6wlnrdVVPYanrAlsX0SMPfOzhoTEBnQqYkrNWTqQY91C0f3bxtPdUhUt' \ + 'OowyPAOkTQ4KnZuzphfhVb2p1MyVYMxNKcBFB05/qaCX99+92fjw4x9LeUOwyGwMv5F0Vkfi5qZCcEw69uVrhYL' \ + 'VSTFTrzi/yCtru+yFULMQ6UhbY09GwiP6hjxZMVr8aROQy5lLHglqQzOuSZ4KeD85JjifHdKzlx8jjQ+uj+FLzHXPMA' \ + 'PmPU1JK9kpoHZ1oPshAFgPDpphJe+HwcJ8ezmk+3AEUr3wWli3xF+49y8Z2anASSVp6YI2YP95UT8Rlh3qT3T+V9V8rbSVislxA==",' \ + '\n "SigningCertURL" : "https://sns.eu-west-1.amazonaws.com/SimpleNotificationService-bb750' \ + 'dd426d95ee9390147a5624348ee.pem",' \ + '\n "UnsubscribeURL" : "https://sns.eu-west-1.amazonaws.com/?Action=Unsubscribe&S' \ + 'subscriptionArn=arn:aws:sns:eu-west-1:302763885840:preview-emails:d6aad3ef-83d6-4cf3-a470-54e2e75916da"\n}' diff --git a/tests/app/notifications/test_notifications_ses_callback.py b/tests/app/notifications/test_notifications_ses_callback.py index 2002cee82..c508ff5b4 100644 --- a/tests/app/notifications/test_notifications_ses_callback.py +++ b/tests/app/notifications/test_notifications_ses_callback.py @@ -5,13 +5,20 @@ from freezegun import freeze_time from app import statsd_client from app.dao.notifications_dao import get_notification_by_id -from app.models import Notification -from app.notifications.notifications_ses_callback import process_ses_response, remove_emails_from_bounce +from app.models import Notification, Complaint +from app.notifications.notifications_ses_callback import ( + process_ses_response, remove_emails_from_bounce, + handle_complaint +) from app.celery.research_mode_tasks import ses_hard_bounce_callback, ses_soft_bounce_callback, ses_notification_callback from app.celery.service_callback_tasks import create_encrypted_callback_data from tests.app.conftest import sample_notification as create_sample_notification -from tests.app.db import create_service_callback_api +from tests.app.db import ( + create_service_callback_api, create_notification, ses_complaint_callback_malformed_message_id, + ses_complaint_callback_with_missing_complaint_type, + ses_complaint_callback +) def test_ses_callback_should_update_notification_status( @@ -190,3 +197,28 @@ def test_remove_emails_from_bounce(): remove_emails_from_bounce(message_dict['bounce']) assert 'not-real@gmail.com' not in json.dumps(message_dict) + + +def test_process_ses_results_in_complaint(sample_email_template): + notification = create_notification(template=sample_email_template, reference='ref1') + response = json.loads(ses_complaint_callback()) + handle_complaint(response) + complaints = Complaint.query.all() + assert len(complaints) == 1 + assert complaints[0].notification_id == notification.id + + +def test_handle_complaint_does_not_raise_exception_if_reference_is_missing(notify_api): + response = json.loads(ses_complaint_callback_malformed_message_id()) + handle_complaint(response) + assert len(Complaint.query.all()) == 0 + + +def test_process_ses_results_in_complaint_save_complaint_with_null_complaint_type(notify_api, sample_email_template): + notification = create_notification(template=sample_email_template, reference='ref1') + response = json.loads(ses_complaint_callback_with_missing_complaint_type()) + handle_complaint(response) + complaints = Complaint.query.all() + assert len(complaints) == 1 + assert complaints[0].notification_id == notification.id + assert not complaints[0].complaint_type