From e82fa2c8d48ad08250d5a69ca99f5ef671750e91 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 30 May 2018 16:16:36 +0100 Subject: [PATCH 1/4] Added a way to handle complaint responses from SES. At this point we are just logging the message so that we can confirm the contents of the SES message. refer to: https://www.pivotaltracker.com/story/show/157969699 --- .../notifications_ses_callback.py | 4 ++++ .../celery/test_process_ses_receipts_tasks.py | 22 +++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/app/notifications/notifications_ses_callback.py b/app/notifications/notifications_ses_callback.py index c5f65e0bd..f62b654d9 100644 --- a/app/notifications/notifications_ses_callback.py +++ b/app/notifications/notifications_ses_callback.py @@ -38,6 +38,10 @@ def process_ses_response(ses_request): notification_type = ses_message['bounce']['bounceType'] # permanent or not else: notification_type = 'Temporary' + if notification_type == 'Complaint': + current_app.logger.info("Complaint from SES: \n{}".format(ses_message)) + return + try: aws_response_dict = get_aws_responses(notification_type) except KeyError: diff --git a/tests/app/celery/test_process_ses_receipts_tasks.py b/tests/app/celery/test_process_ses_receipts_tasks.py index ddca32e0e..4be35cd1f 100644 --- a/tests/app/celery/test_process_ses_receipts_tasks.py +++ b/tests/app/celery/test_process_ses_receipts_tasks.py @@ -32,6 +32,13 @@ 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): + 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 + + def ses_notification_callback(): return '{\n "Type" : "Notification",\n "MessageId" : "ref1",' \ '\n "TopicArn" : "arn:aws:sns:eu-west-1:123456789012:testing",' \ @@ -56,3 +63,18 @@ def ses_notification_callback(): '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}' From 621c81a9d8c7c534ac640b3ab7f83577653799b7 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 30 May 2018 16:25:49 +0100 Subject: [PATCH 2/4] Small refactor to reduce complexity and satisfy codestyle. This module could do with a look to check if we can simplify it. But at the moment we just want to record the complaint. --- app/notifications/notifications_ses_callback.py | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/app/notifications/notifications_ses_callback.py b/app/notifications/notifications_ses_callback.py index f62b654d9..402a4db27 100644 --- a/app/notifications/notifications_ses_callback.py +++ b/app/notifications/notifications_ses_callback.py @@ -33,12 +33,8 @@ def process_ses_response(ses_request): notification_type = ses_message['notificationType'] if notification_type == 'Bounce': - current_app.logger.info('SES bounce dict: {}'.format(remove_emails_from_bounce(ses_message['bounce']))) - if ses_message['bounce']['bounceType'] == 'Permanent': - notification_type = ses_message['bounce']['bounceType'] # permanent or not - else: - notification_type = 'Temporary' - if notification_type == 'Complaint': + notification_type = determine_notification_bounce_type(notification_type, ses_message) + elif notification_type == 'Complaint': current_app.logger.info("Complaint from SES: \n{}".format(ses_message)) return @@ -95,6 +91,15 @@ def process_ses_response(ses_request): return error +def determine_notification_bounce_type(notification_type, ses_message): + current_app.logger.info('SES bounce dict: {}'.format(remove_emails_from_bounce(ses_message['bounce']))) + if ses_message['bounce']['bounceType'] == 'Permanent': + notification_type = ses_message['bounce']['bounceType'] # permanent or not + else: + notification_type = 'Temporary' + return notification_type + + def remove_emails_from_bounce(bounce_dict): for recip in bounce_dict['bouncedRecipients']: recip.pop('emailAddress') From 23e6b57c262fbe019727d616eb449fa120897ee1 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 30 May 2018 16:45:18 +0100 Subject: [PATCH 3/4] Remove email from the log message --- app/notifications/notifications_ses_callback.py | 5 +++++ tests/app/celery/test_process_ses_receipts_tasks.py | 8 ++++++++ 2 files changed, 13 insertions(+) diff --git a/app/notifications/notifications_ses_callback.py b/app/notifications/notifications_ses_callback.py index 402a4db27..c3c9808c4 100644 --- a/app/notifications/notifications_ses_callback.py +++ b/app/notifications/notifications_ses_callback.py @@ -35,6 +35,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': + remove_emails_from_complaint(ses_message) current_app.logger.info("Complaint from SES: \n{}".format(ses_message)) return @@ -105,6 +106,10 @@ def remove_emails_from_bounce(bounce_dict): recip.pop('emailAddress') +def remove_emails_from_complaint(complaint_dict): + complaint_dict['complaint'].pop('complainedRecipients') + + def _check_and_queue_callback_task(notification): # queue callback task only if the service_callback_api exists service_callback_api = get_service_callback_api_for_service(service_id=notification.service_id) diff --git a/tests/app/celery/test_process_ses_receipts_tasks.py b/tests/app/celery/test_process_ses_receipts_tasks.py index 4be35cd1f..7c881abbf 100644 --- a/tests/app/celery/test_process_ses_receipts_tasks.py +++ b/tests/app/celery/test_process_ses_receipts_tasks.py @@ -2,6 +2,7 @@ import json from datetime import datetime from app.celery.process_ses_receipts_tasks import process_ses_results +from app.notifications.notifications_ses_callback import remove_emails_from_complaint from tests.app.db import create_notification @@ -39,6 +40,13 @@ def test_process_ses_results_in_complaint(notify_db, mocker): assert mocked.call_count == 0 +def test_remove_emails_from_complaint(): + test_message = ses_complaint_callback() + 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",' \ From 1faba916b24be8c481c4019f0f355a1909b803e7 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 31 May 2018 14:43:49 +0100 Subject: [PATCH 4/4] New complaints model to persist complaints from SES. If a someone gets an email from one of our services and then complain about it (mark as spam or otherwise), we get a callback from SES. The service needs to know about these complaints so they can remove that email from their mailing list. --- app/models.py | 13 +++++++ .../notifications_ses_callback.py | 2 ++ app/schemas.py | 1 + migrations/versions/0196_complaints_table_.py | 36 +++++++++++++++++++ 4 files changed, 52 insertions(+) create mode 100644 migrations/versions/0196_complaints_table_.py diff --git a/app/models.py b/app/models.py index e7d0c8443..b8e65b891 100644 --- a/app/models.py +++ b/app/models.py @@ -1814,3 +1814,16 @@ class FactNotificationStatus(db.Model): notification_count = db.Column(db.Integer(), nullable=False) created_at = db.Column(db.DateTime, nullable=False, default=datetime.datetime.utcnow) updated_at = db.Column(db.DateTime, nullable=True, onupdate=datetime.datetime.utcnow) + + +class Complaint(db.Model): + __tablename__ = 'complaints' + id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) + notification_id = db.Column(UUID(as_uuid=True), db.ForeignKey('notification_history.id'), + index=True, nullable=False) + service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), unique=False, index=True, nullable=False) + service = db.relationship(Service, backref=db.backref('complaints')) + ses_feedback_id = db.Column(db.Text, nullable=True) + complaint_type = db.Column(db.Text, nullable=True) + complaint_date = db.Column(db.DateTime, nullable=True) + created_at = db.Column(db.DateTime, nullable=False, default=datetime.datetime.utcnow) diff --git a/app/notifications/notifications_ses_callback.py b/app/notifications/notifications_ses_callback.py index c3c9808c4..8650f8019 100644 --- a/app/notifications/notifications_ses_callback.py +++ b/app/notifications/notifications_ses_callback.py @@ -35,6 +35,8 @@ 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)) return diff --git a/app/schemas.py b/app/schemas.py index b9d3b5abf..31b28c32a 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -235,6 +235,7 @@ class ServiceSchema(BaseSchema): 'monthly_billing', 'reply_to_email_addresses', 'letter_contacts', + 'complaints', ) strict = True diff --git a/migrations/versions/0196_complaints_table_.py b/migrations/versions/0196_complaints_table_.py new file mode 100644 index 000000000..45170864c --- /dev/null +++ b/migrations/versions/0196_complaints_table_.py @@ -0,0 +1,36 @@ +""" + +Revision ID: 0196_complaints_table +Revises: 0195_ft_notification_timestamps +Create Date: 2018-05-31 14:31:36.649544 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +revision = '0196_complaints_table' +down_revision = '0195_ft_notification_timestamps' + + +def upgrade(): + op.create_table('complaints', + sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('notification_id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('service_id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('ses_feedback_id', sa.Text(), nullable=True), + sa.Column('complaint_type', sa.Text(), nullable=True), + sa.Column('complaint_date', sa.DateTime(), nullable=True), + sa.Column('created_at', sa.DateTime(), nullable=False), + sa.ForeignKeyConstraint(['notification_id'], ['notification_history.id'], ), + sa.ForeignKeyConstraint(['service_id'], ['services.id'], ), + sa.PrimaryKeyConstraint('id') + ) + op.create_index(op.f('ix_complaints_notification_id'), 'complaints', ['notification_id'], unique=False) + op.create_index(op.f('ix_complaints_service_id'), 'complaints', ['service_id'], unique=False) + + +def downgrade(): + op.drop_index(op.f('ix_complaints_service_id'), table_name='complaints') + op.drop_index(op.f('ix_complaints_notification_id'), table_name='complaints') + op.drop_table('complaints')