From f766f90207cc7a6c3cef52935b324a859dff239f Mon Sep 17 00:00:00 2001
From: Imdad Ahad <imdad.ahad@digital.cabinet-office.gov.uk>
Date: Thu, 4 May 2017 10:31:18 +0100
Subject: [PATCH 1/7] Add task to process a DVLA response file: * Currently we
 do nothing with the parsed response. We will * update the status of the
 notifications in a separate PR

---
 app/aws/s3.py                  | 20 ++++++++++++--------
 app/celery/tasks.py            | 32 +++++++++++++++++++++++++++++++-
 app/dao/notifications_dao.py   |  3 ++-
 tests/app/celery/test_tasks.py | 34 ++++++++++++++++++++++++++++++++--
 4 files changed, 77 insertions(+), 12 deletions(-)

diff --git a/app/aws/s3.py b/app/aws/s3.py
index 2aa7aac39..e901416d4 100644
--- a/app/aws/s3.py
+++ b/app/aws/s3.py
@@ -4,20 +4,24 @@ from flask import current_app
 FILE_LOCATION_STRUCTURE = 'service-{}-notify/{}.csv'
 
 
-def get_s3_job_object(bucket_name, file_location):
+def get_s3_object(bucket_name, file_location):
     s3 = resource('s3')
-    return s3.Object(bucket_name, file_location)
+    s3_object = s3.Object(bucket_name, file_location)
+    return s3_object.get()['Body'].read()
 
 
 def get_job_from_s3(service_id, job_id):
-    bucket_name = current_app.config['CSV_UPLOAD_BUCKET_NAME']
-    file_location = FILE_LOCATION_STRUCTURE.format(service_id, job_id)
-    obj = get_s3_job_object(bucket_name, file_location)
-    return obj.get()['Body'].read().decode('utf-8')
+    job = _job_from_s3(service_id, job_id)
+    return job
 
 
 def remove_job_from_s3(service_id, job_id):
+    job = _job_from_s3(service_id, job_id)
+    return job.delete()
+
+
+def _job_from_s3():
     bucket_name = current_app.config['CSV_UPLOAD_BUCKET_NAME']
     file_location = FILE_LOCATION_STRUCTURE.format(service_id, job_id)
-    obj = get_s3_job_object(bucket_name, file_location)
-    return obj.delete()
+    obj = get_s3_object(bucket_name, file_location).decode('utf-8')
+    return obj
diff --git a/app/celery/tasks.py b/app/celery/tasks.py
index 464e3e1be..4ebb73a42 100644
--- a/app/celery/tasks.py
+++ b/app/celery/tasks.py
@@ -1,6 +1,7 @@
 import random
 
 from datetime import (datetime)
+from collections import namedtuple
 
 from flask import current_app
 from notifications_utils.recipients import (
@@ -23,7 +24,10 @@ from app.dao.jobs_dao import (
     all_notifications_are_created_for_job,
     dao_get_all_notifications_for_job,
     dao_update_job_status)
-from app.dao.notifications_dao import get_notification_by_id, dao_update_notifications_sent_to_dvla
+from app.dao.notifications_dao import (
+    get_notification_by_id,
+    dao_update_notifications_sent_to_dvla
+)
 from app.dao.provider_details_dao import get_current_provider
 from app.dao.services_dao import dao_fetch_service_by_id, fetch_todays_total_message_count
 from app.dao.templates_dao import dao_get_template_by_id
@@ -354,3 +358,29 @@ def get_template_class(template_type):
         # since we don't need rendering capabilities (we only need to extract placeholders) both email and letter can
         # use the same base template
         return WithSubjectTemplate
+
+
+@notify_celery.task(bind=True, name='update-letter-notifications-statuses')
+@statsd(namespace="tasks")
+def update_letter_notifications_statuses(self, filename):
+    response_file = s3.get_s3_object('development-notifications-csv-upload', filename).decode('utf-8')
+    lines = response_file.splitlines()
+    notification_updates = []
+
+    try:
+        NotificationUpdate = namedtuple('NotificationUpdate', ['reference', 'status', 'page_count', 'cost_threshold'])
+        for line in lines:
+            notification_updates.append(NotificationUpdate(*line.split('|')))
+
+    except TypeError:
+        current_app.logger.exception('DVLA response file has an invalid format')
+        raise
+
+    else:
+        if notification_updates:
+            for update in notification_updates:
+                current_app.logger.error(str(update))
+                # TODO: Update notifications with desired status
+            return notification_updates
+        else:
+            current_app.logger.exception('DVLA response file contained no updates')
diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py
index da81c348b..66cf031d6 100644
--- a/app/dao/notifications_dao.py
+++ b/app/dao/notifications_dao.py
@@ -27,7 +27,8 @@ from app.models import (
     NOTIFICATION_PERMANENT_FAILURE,
     KEY_TYPE_NORMAL, KEY_TYPE_TEST,
     LETTER_TYPE,
-    NOTIFICATION_SENT)
+    NOTIFICATION_SENT
+)
 
 from app.dao.dao_utils import transactional
 from app.statsd_decorators import statsd
diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py
index 8b9631b4f..4eb74da47 100644
--- a/tests/app/celery/test_tasks.py
+++ b/tests/app/celery/test_tasks.py
@@ -12,15 +12,19 @@ from celery.exceptions import Retry
 from app import (encryption, DATETIME_FORMAT)
 from app.celery import provider_tasks
 from app.celery import tasks
-from app.celery.tasks import s3, build_dvla_file, create_dvla_file_contents, update_dvla_job_to_error
 from app.celery.tasks import (
+    s3,
+    build_dvla_file,
+    create_dvla_file_contents,
+    update_dvla_job_to_error,
     process_job,
     process_row,
     send_sms,
     send_email,
     persist_letter,
     get_template_class,
-    update_job_to_sent_to_dvla
+    update_job_to_sent_to_dvla,
+    update_letter_notifications_statuses
 )
 from app.dao import jobs_dao, services_dao
 from app.models import (
@@ -1071,3 +1075,29 @@ def test_update_dvla_job_to_error(sample_letter_template, sample_letter_job):
         assert not n.sent_by
 
     assert 'error' == Job.query.filter_by(id=sample_letter_job.id).one().job_status
+
+
+def test_update_letter_notifications_statuses_raises_for_invalid_format(notify_api, mocker):
+    invalid_file = b'ref-foo|Sent|1|Unsorted\nref-bar|Sent|2'
+    mocker.patch('app.celery.tasks.s3.get_s3_object', return_value=invalid_file)
+
+    with pytest.raises(TypeError):
+        update_letter_notifications_statuses(filename='foo.txt')
+
+
+def test_update_letter_notifications_statuses_builds_updates_list(notify_api, mocker):
+    valid_file = b'ref-foo|Sent|1|Unsorted\nref-bar|Sent|2|Sorted'
+    mocker.patch('app.celery.tasks.s3.get_s3_object', return_value=valid_file)
+    updates = update_letter_notifications_statuses(filename='foo.txt')
+
+    assert len(updates) == 2
+
+    assert updates[0].reference == 'ref-foo'
+    assert updates[0].status == 'Sent'
+    assert updates[0].page_count == '1'
+    assert updates[0].cost_threshold == 'Unsorted'
+
+    assert updates[1].reference == 'ref-bar'
+    assert updates[1].status == 'Sent'
+    assert updates[1].page_count == '2'
+    assert updates[1].cost_threshold == 'Sorted'

From 20bb91bfdde8a1ffb591973e576bae155ec02772 Mon Sep 17 00:00:00 2001
From: Imdad Ahad <imdad.ahad@digital.cabinet-office.gov.uk>
Date: Thu, 4 May 2017 10:33:44 +0100
Subject: [PATCH 2/7] Update DVLA callback to process request and call task (if
 it can)

---
 .../notifications_letter_callback.py          | 18 ++--
 .../app/notifications/rest/test_callbacks.py  | 82 ++++++++++++++++++-
 2 files changed, 92 insertions(+), 8 deletions(-)

diff --git a/app/notifications/notifications_letter_callback.py b/app/notifications/notifications_letter_callback.py
index f7c357055..3b4a0fbb3 100644
--- a/app/notifications/notifications_letter_callback.py
+++ b/app/notifications/notifications_letter_callback.py
@@ -9,6 +9,7 @@ from flask import (
 )
 
 from app import statsd_client
+from app.celery.tasks import update_letter_notifications_statuses
 from app.clients.email.aws_ses import get_aws_responses
 from app.dao import (
     notifications_dao
@@ -29,11 +30,18 @@ register_errors(letter_callback_blueprint)
 @letter_callback_blueprint.route('/notifications/letter/dvla', methods=['POST'])
 def process_letter_response():
     try:
-        dvla_request = json.loads(request.data)
-        current_app.logger.info(dvla_request)
+        req_json = json.loads(request.data)
+        # The callback should have one record for an S3 Put Event.
+        filename = req_json['Message']['Records'][0]['s3']['object']['key']
+
+    except (ValueError, KeyError):
+        error = "DVLA callback failed: Invalid JSON"
+        raise InvalidRequest(error, status_code=400)
+
+    else:
+        current_app.logger.info('DVLA callback: Calling task to update letter notifications')
+        update_letter_notifications_statuses.apply_async([filename], queue='notify')
+
         return jsonify(
             result="success", message="DVLA callback succeeded"
         ), 200
-    except ValueError:
-        error = "DVLA callback failed: invalid json"
-        raise InvalidRequest(error, status_code=400)
diff --git a/tests/app/notifications/rest/test_callbacks.py b/tests/app/notifications/rest/test_callbacks.py
index 780076b34..0dd070271 100644
--- a/tests/app/notifications/rest/test_callbacks.py
+++ b/tests/app/notifications/rest/test_callbacks.py
@@ -1,3 +1,4 @@
+import pytest
 import uuid
 
 from datetime import datetime
@@ -6,6 +7,7 @@ from flask import json
 from freezegun import freeze_time
 
 import app.celery.tasks
+from app.errors import InvalidRequest
 from app.dao.notifications_dao import (
     get_notification_by_id
 )
@@ -13,16 +15,47 @@ from app.models import NotificationStatistics
 from tests.app.conftest import sample_notification as create_sample_notification
 
 
-def test_dvla_callback_should_not_need_auth(client):
-    data = json.dumps({"somekey": "somevalue"})
+def test_dvla_callback_returns_400_with_invalid_request(client):
+    data = json.dumps({"foo": "bar"})
     response = client.post(
         path='/notifications/letter/dvla',
         data=data,
-        headers=[('Content-Type', 'application/json')])
+        headers=[('Content-Type', 'application/json')]
+    )
+    json_resp = json.loads(response.get_data(as_text=True))
+
+    assert response.status_code == 400
+    assert json_resp['result'] == 'error'
+    assert json_resp['message'] == 'DVLA callback failed: Invalid JSON'
+
+
+def test_dvla_callback_returns_200_with_valid_request(client):
+    data = _sample_sns_s3_callback()
+    response = client.post(
+        path='/notifications/letter/dvla',
+        data=data,
+        headers=[('Content-Type', 'application/json')]
+    )
+    json_resp = json.loads(response.get_data(as_text=True))
 
     assert response.status_code == 200
 
 
+def test_dvla_callback_calls_update_letter_notifications_task(client, mocker):
+    update_notifications_mock = \
+        mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses')
+    data = _sample_sns_s3_callback()
+    response = client.post(
+        path='/notifications/letter/dvla',
+        data=data,
+        headers=[('Content-Type', 'application/json')]
+    )
+    json_resp = json.loads(response.get_data(as_text=True))
+
+    assert response.status_code == 200
+    assert update_notifications_mock.apply_async.called is True
+
+
 def test_firetext_callback_should_not_need_auth(client, mocker):
     mocker.patch('app.statsd_client.incr')
     response = client.post(
@@ -458,3 +491,46 @@ def test_firetext_callback_should_record_statsd(client, notify_db, notify_db_ses
 
 def get_notification_stats(service_id):
     return NotificationStatistics.query.filter_by(service_id=service_id).one()
+
+
+def _sample_sns_s3_callback():
+    return json.dumps({
+        "SigningCertURL": "foo.pem",
+        "UnsubscribeURL": "bar",
+        "Signature": "some-signature",
+        "Type": "Notification",
+        "Timestamp": "2016-05-03T08:35:12.884Z",
+        "SignatureVersion": "1",
+        "MessageId": "6adbfe0a-d610-509a-9c47-af894e90d32d",
+        "Subject": "Amazon S3 Notification",
+        "TopicArn": "sample-topic-arn",
+        "Message": {
+            "Records": [{
+                "eventVersion": "2.0",
+                "eventSource": "aws:s3",
+                "awsRegion": "eu-west-1",
+                "eventTime": "2017-05-03T08:35:12.826Z",
+                "eventName": "ObjectCreated:Put",
+                "userIdentity": {"principalId": "some-p-id"},
+                "requestParameters": {"sourceIPAddress": "8.8.8.8"},
+                "responseElements": {"x-amz-request-id": "some-req-id", "x-amz-id-2": "some-amz-id"},
+                "s3": {
+                    "s3SchemaVersion": "1.0",
+                    "configurationId": "some-config-id",
+                    "bucket": {
+                        "name": "some-bucket",
+                        "ownerIdentity": {"principalId": "some-p-id"},
+                        "arn": "some-bucket-arn"
+                    },
+                    "object": {
+                        "key": "bar.txt",
+                        "size": 200,
+                        "eTag": "some-etag",
+                        "versionId": "some-v-id",
+                        "sequencer": "some-seq"
+                    }
+                }
+            }
+            ]
+        }
+    })

From 8a5e82904e4ffe58bbd6ffcb3696e44db905ec66 Mon Sep 17 00:00:00 2001
From: Imdad Ahad <imdad.ahad@digital.cabinet-office.gov.uk>
Date: Thu, 4 May 2017 11:42:59 +0100
Subject: [PATCH 3/7] Update to pull from correct bucket and fix tests not
 mocking out correctly

---
 app/celery/tasks.py                            |  3 ++-
 tests/app/celery/test_tasks.py                 | 10 ++++++++++
 tests/app/notifications/rest/test_callbacks.py | 10 ++++++----
 3 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/app/celery/tasks.py b/app/celery/tasks.py
index 4ebb73a42..8498ff05a 100644
--- a/app/celery/tasks.py
+++ b/app/celery/tasks.py
@@ -363,7 +363,8 @@ def get_template_class(template_type):
 @notify_celery.task(bind=True, name='update-letter-notifications-statuses')
 @statsd(namespace="tasks")
 def update_letter_notifications_statuses(self, filename):
-    response_file = s3.get_s3_object('development-notifications-csv-upload', filename).decode('utf-8')
+    bucket_location = '{}-ftp'.format(current_app.config['NOTIFY_EMAIL_DOMAIN'])
+    response_file = s3.get_s3_object(bucket_location, filename).decode('utf-8')
     lines = response_file.splitlines()
     notification_updates = []
 
diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py
index 4eb74da47..12f558f7a 100644
--- a/tests/app/celery/test_tasks.py
+++ b/tests/app/celery/test_tasks.py
@@ -38,6 +38,7 @@ from app.models import (
     Job)
 
 from tests.app import load_example_csv
+from tests.conftest import set_config
 from tests.app.conftest import (
     sample_service,
     sample_template,
@@ -1085,6 +1086,15 @@ def test_update_letter_notifications_statuses_raises_for_invalid_format(notify_a
         update_letter_notifications_statuses(filename='foo.txt')
 
 
+def test_update_letter_notifications_statuses_calls_with_correct_bucket_location(notify_api, mocker):
+    invalid_file = b'ref-foo|Sent|1|Unsorted\nref-bar|Sent|2'
+    s3_mock = mocker.patch('app.celery.tasks.s3.get_s3_object')
+
+    with set_config(notify_api, 'NOTIFY_EMAIL_DOMAIN', 'foo.bar'):
+        update_letter_notifications_statuses(filename='foo.txt')
+        s3_mock.assert_called_with('{}-ftp'.format(current_app.config['NOTIFY_EMAIL_DOMAIN']), 'foo.txt')
+
+
 def test_update_letter_notifications_statuses_builds_updates_list(notify_api, mocker):
     valid_file = b'ref-foo|Sent|1|Unsorted\nref-bar|Sent|2|Sorted'
     mocker.patch('app.celery.tasks.s3.get_s3_object', return_value=valid_file)
diff --git a/tests/app/notifications/rest/test_callbacks.py b/tests/app/notifications/rest/test_callbacks.py
index 0dd070271..095384836 100644
--- a/tests/app/notifications/rest/test_callbacks.py
+++ b/tests/app/notifications/rest/test_callbacks.py
@@ -29,8 +29,9 @@ def test_dvla_callback_returns_400_with_invalid_request(client):
     assert json_resp['message'] == 'DVLA callback failed: Invalid JSON'
 
 
-def test_dvla_callback_returns_200_with_valid_request(client):
+def test_dvla_callback_returns_200_with_valid_request(client, mocker):
     data = _sample_sns_s3_callback()
+    mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses.apply_async')
     response = client.post(
         path='/notifications/letter/dvla',
         data=data,
@@ -42,8 +43,8 @@ def test_dvla_callback_returns_200_with_valid_request(client):
 
 
 def test_dvla_callback_calls_update_letter_notifications_task(client, mocker):
-    update_notifications_mock = \
-        mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses')
+    update_task = \
+        mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses.apply_async')
     data = _sample_sns_s3_callback()
     response = client.post(
         path='/notifications/letter/dvla',
@@ -53,7 +54,8 @@ def test_dvla_callback_calls_update_letter_notifications_task(client, mocker):
     json_resp = json.loads(response.get_data(as_text=True))
 
     assert response.status_code == 200
-    assert update_notifications_mock.apply_async.called is True
+    assert update_task.called is True
+    update_task.assert_called_with(['bar.txt'], queue='notify')
 
 
 def test_firetext_callback_should_not_need_auth(client, mocker):

From 37165e5b6a16d6ac0d1b3f22380a650a1277eb95 Mon Sep 17 00:00:00 2001
From: Imdad Ahad <imdad.ahad@digital.cabinet-office.gov.uk>
Date: Tue, 9 May 2017 14:20:56 +0100
Subject: [PATCH 4/7] Add autoconfirm sns in dvla callback

---
 .../notifications_letter_callback.py          | 58 ++++++++++++-------
 app/notifications/utils.py                    |  7 +++
 .../app/notifications/rest/test_callbacks.py  | 32 +++++++---
 3 files changed, 69 insertions(+), 28 deletions(-)

diff --git a/app/notifications/notifications_letter_callback.py b/app/notifications/notifications_letter_callback.py
index 3b4a0fbb3..c7bc80d9e 100644
--- a/app/notifications/notifications_letter_callback.py
+++ b/app/notifications/notifications_letter_callback.py
@@ -1,4 +1,5 @@
 from datetime import datetime
+from functools import wraps
 
 from flask import (
     Blueprint,
@@ -11,37 +12,52 @@ from flask import (
 from app import statsd_client
 from app.celery.tasks import update_letter_notifications_statuses
 from app.clients.email.aws_ses import get_aws_responses
-from app.dao import (
-    notifications_dao
-)
-
+from app.dao import notifications_dao
+from app.v2.errors import register_errors
 from app.notifications.process_client_response import validate_callback_data
+from app.notifications.utils import autoconfirm_subscription
+from app.schema_validation import validate
+
 
 letter_callback_blueprint = Blueprint('notifications_letter_callback', __name__)
-
-from app.errors import (
-    register_errors,
-    InvalidRequest
-)
-
 register_errors(letter_callback_blueprint)
 
 
+dvla_sns_callback_schema = {
+    "$schema": "http://json-schema.org/draft-04/schema#",
+    "description": "sns callback received on s3 update",
+    "type": "object",
+    "title": "dvla internal sns callback",
+    "properties": {
+        "Type": {"enum": ["Notification", "SubscriptionConfirmation"]},
+        "MessageId": {"type": "string"},
+        "Message": {"type": ["string", "object"]}
+    },
+    "required": ["Type", "MessageId", "Message"]
+}
+
+
+def validate_schema(schema):
+    def decorator(f):
+        @wraps(f)
+        def wrapper(*args, **kw):
+            validate(request.json, schema)
+            return f(*args, **kw)
+        return wrapper
+    return decorator
+
+
 @letter_callback_blueprint.route('/notifications/letter/dvla', methods=['POST'])
+@validate_schema(dvla_sns_callback_schema)
 def process_letter_response():
-    try:
-        req_json = json.loads(request.data)
+    req_json = request.json
+    if not autoconfirm_subscription(req_json):
         # The callback should have one record for an S3 Put Event.
         filename = req_json['Message']['Records'][0]['s3']['object']['key']
-
-    except (ValueError, KeyError):
-        error = "DVLA callback failed: Invalid JSON"
-        raise InvalidRequest(error, status_code=400)
-
-    else:
+        current_app.logger.info('Received file from DVLA: {}'.format(filename))
         current_app.logger.info('DVLA callback: Calling task to update letter notifications')
         update_letter_notifications_statuses.apply_async([filename], queue='notify')
 
-        return jsonify(
-            result="success", message="DVLA callback succeeded"
-        ), 200
+    return jsonify(
+        result="success", message="DVLA callback succeeded"
+    ), 200
diff --git a/app/notifications/utils.py b/app/notifications/utils.py
index 80346c475..5f1443f09 100644
--- a/app/notifications/utils.py
+++ b/app/notifications/utils.py
@@ -16,3 +16,10 @@ def confirm_subscription(confirmation_request):
         raise e
 
     return confirmation_request['TopicArn']
+
+
+def autoconfirm_subscription(req_json):
+    if req_json.get('Type') == 'SubscriptionConfirmation':
+        current_app.logger.info("SNS subscription confirmation url: {}".format(req_json['SubscribeURL']))
+        subscribed_topic = confirm_subscription(req_json)
+        return subscribed_topic
diff --git a/tests/app/notifications/rest/test_callbacks.py b/tests/app/notifications/rest/test_callbacks.py
index 095384836..242f18a46 100644
--- a/tests/app/notifications/rest/test_callbacks.py
+++ b/tests/app/notifications/rest/test_callbacks.py
@@ -12,6 +12,7 @@ from app.dao.notifications_dao import (
     get_notification_by_id
 )
 from app.models import NotificationStatistics
+from tests.app.notifications.test_notifications_ses_callback import ses_confirmation_callback
 from tests.app.conftest import sample_notification as create_sample_notification
 
 
@@ -22,24 +23,41 @@ def test_dvla_callback_returns_400_with_invalid_request(client):
         data=data,
         headers=[('Content-Type', 'application/json')]
     )
+
     json_resp = json.loads(response.get_data(as_text=True))
 
     assert response.status_code == 400
-    assert json_resp['result'] == 'error'
-    assert json_resp['message'] == 'DVLA callback failed: Invalid JSON'
 
 
-def test_dvla_callback_returns_200_with_valid_request(client, mocker):
-    data = _sample_sns_s3_callback()
-    mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses.apply_async')
+def test_dvla_callback_autoconfirms_subscription(client, mocker):
+    autoconfirm_mock = mocker.patch('app.notifications.notifications_letter_callback.autoconfirm_subscription')
+
+    data = ses_confirmation_callback()
     response = client.post(
         path='/notifications/letter/dvla',
         data=data,
         headers=[('Content-Type', 'application/json')]
     )
-    json_resp = json.loads(response.get_data(as_text=True))
 
     assert response.status_code == 200
+    assert autoconfirm_mock.called
+
+
+def test_dvla_callback_autoconfirm_does_not_call_update_letter_notifications_task(client, mocker):
+    autoconfirm_mock = mocker.patch('app.notifications.notifications_letter_callback.autoconfirm_subscription')
+    update_task = \
+        mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses.apply_async')
+
+    data = ses_confirmation_callback()
+    response = client.post(
+        path='/notifications/letter/dvla',
+        data=data,
+        headers=[('Content-Type', 'application/json')]
+    )
+
+    assert response.status_code == 200
+    assert autoconfirm_mock.called
+    assert not update_task.called
 
 
 def test_dvla_callback_calls_update_letter_notifications_task(client, mocker):
@@ -54,7 +72,7 @@ def test_dvla_callback_calls_update_letter_notifications_task(client, mocker):
     json_resp = json.loads(response.get_data(as_text=True))
 
     assert response.status_code == 200
-    assert update_task.called is True
+    assert update_task.called
     update_task.assert_called_with(['bar.txt'], queue='notify')
 
 

From 4d82512ec65bc369e69b52d4fb4aecce74e4ce7c Mon Sep 17 00:00:00 2001
From: Imdad Ahad <imdad.ahad@digital.cabinet-office.gov.uk>
Date: Tue, 9 May 2017 14:21:57 +0100
Subject: [PATCH 5/7] Update SES callback to use autconfirm method

---
 app/notifications/notifications_ses_callback.py | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/app/notifications/notifications_ses_callback.py b/app/notifications/notifications_ses_callback.py
index ce8dec9ba..e78fb9394 100644
--- a/app/notifications/notifications_ses_callback.py
+++ b/app/notifications/notifications_ses_callback.py
@@ -13,9 +13,8 @@ from app.clients.email.aws_ses import get_aws_responses
 from app.dao import (
     notifications_dao
 )
-
 from app.notifications.process_client_response import validate_callback_data
-from app.notifications.utils import confirm_subscription
+from app.notifications.utils import autoconfirm_subscription
 
 ses_callback_blueprint = Blueprint('notifications_ses_callback', __name__)
 
@@ -32,14 +31,12 @@ def process_ses_response():
     try:
         ses_request = json.loads(request.data)
 
-        if ses_request.get('Type') == 'SubscriptionConfirmation':
-            current_app.logger.info("SNS subscription confirmation url: {}".format(ses_request['SubscribeURL']))
-            subscribed_topic = confirm_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
+        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
 
         errors = validate_callback_data(data=ses_request, fields=['Message'], client_name=client_name)
         if errors:

From 95aa5680f97228791bfbbf5579483173c3e5673e Mon Sep 17 00:00:00 2001
From: Imdad Ahad <imdad.ahad@digital.cabinet-office.gov.uk>
Date: Tue, 9 May 2017 14:22:10 +0100
Subject: [PATCH 6/7] Add more logging in update letter notifications task

---
 app/celery/tasks.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/celery/tasks.py b/app/celery/tasks.py
index 8498ff05a..988977fcd 100644
--- a/app/celery/tasks.py
+++ b/app/celery/tasks.py
@@ -380,7 +380,7 @@ def update_letter_notifications_statuses(self, filename):
     else:
         if notification_updates:
             for update in notification_updates:
-                current_app.logger.error(str(update))
+                current_app.logger.info('DVLA update: {}'.format(str(update)))
                 # TODO: Update notifications with desired status
             return notification_updates
         else:

From 0f7093fc386f23ed450864e9c384b8b8058d2d03 Mon Sep 17 00:00:00 2001
From: Imdad Ahad <imdad.ahad@digital.cabinet-office.gov.uk>
Date: Fri, 12 May 2017 14:24:17 +0100
Subject: [PATCH 7/7] Refactor and add filename in logging

---
 app/celery/tasks.py            | 7 ++-----
 tests/app/celery/test_tasks.py | 1 -
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/app/celery/tasks.py b/app/celery/tasks.py
index 988977fcd..da90ba6f0 100644
--- a/app/celery/tasks.py
+++ b/app/celery/tasks.py
@@ -365,16 +365,13 @@ def get_template_class(template_type):
 def update_letter_notifications_statuses(self, filename):
     bucket_location = '{}-ftp'.format(current_app.config['NOTIFY_EMAIL_DOMAIN'])
     response_file = s3.get_s3_object(bucket_location, filename).decode('utf-8')
-    lines = response_file.splitlines()
-    notification_updates = []
 
     try:
         NotificationUpdate = namedtuple('NotificationUpdate', ['reference', 'status', 'page_count', 'cost_threshold'])
-        for line in lines:
-            notification_updates.append(NotificationUpdate(*line.split('|')))
+        notification_updates = [NotificationUpdate(*line.split('|')) for line in response_file.splitlines()]
 
     except TypeError:
-        current_app.logger.exception('DVLA response file has an invalid format')
+        current_app.logger.exception('DVLA response file: {} has an invalid format'.format(filename))
         raise
 
     else:
diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py
index 12f558f7a..e48932302 100644
--- a/tests/app/celery/test_tasks.py
+++ b/tests/app/celery/test_tasks.py
@@ -1087,7 +1087,6 @@ def test_update_letter_notifications_statuses_raises_for_invalid_format(notify_a
 
 
 def test_update_letter_notifications_statuses_calls_with_correct_bucket_location(notify_api, mocker):
-    invalid_file = b'ref-foo|Sent|1|Unsorted\nref-bar|Sent|2'
     s3_mock = mocker.patch('app.celery.tasks.s3.get_s3_object')
 
     with set_config(notify_api, 'NOTIFY_EMAIL_DOMAIN', 'foo.bar'):