Merge pull request #968 from alphagov/imdad-fix-parse-json-properly-sns-callback

Fix issue with parsing JSON in SNS callback

app/celery/tasks.py

@@ -1,5 +1,3 @@
-import random
-
 from datetime import (datetime)
 from collections import namedtuple
 
@@ -361,21 +359,20 @@ def get_template_class(template_type):
 @statsd(namespace="tasks")
 def update_letter_notifications_statuses(self, filename):
     bucket_location = '{}-ftp'.format(current_app.config['NOTIFY_EMAIL_DOMAIN'])
-    response_file = s3.get_s3_file(bucket_location, filename)
+    response_file_content = s3.get_s3_file(bucket_location, filename)
 
     try:
-        NotificationUpdate = namedtuple('NotificationUpdate', ['reference', 'status', 'page_count', 'cost_threshold'])
-        notification_updates = [NotificationUpdate(*line.split('|')) for line in response_file.splitlines()]
-
+        notification_updates = process_updates_from_file(response_file_content)
     except TypeError:
         current_app.logger.exception('DVLA response file: {} has an invalid format'.format(filename))
         raise
-
     else:
-        if notification_updates:
-            for update in notification_updates:
-                current_app.logger.info('DVLA update: {}'.format(str(update)))
-                # TODO: Update notifications with desired status
-            return notification_updates
-        else:
-            current_app.logger.exception('DVLA response file contained no updates')
+        for update in notification_updates:
+            current_app.logger.info('DVLA update: {}'.format(str(update)))
+            # TODO: Update notifications with desired status
+
+
+def process_updates_from_file(response_file):
+    NotificationUpdate = namedtuple('NotificationUpdate', ['reference', 'status', 'page_count', 'cost_threshold'])
+    notification_updates = [NotificationUpdate(*line.split('|')) for line in response_file.splitlines()]
+    return notification_updates

app/notifications/notifications_letter_callback.py

@@ -1,3 +1,5 @@
+import json
+
 from functools import wraps
 
 from flask import (
@@ -48,7 +50,8 @@ def process_letter_response():
     current_app.logger.info('Received SNS callback: {}'.format(req_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']
+        message = json.loads(req_json['Message'])
+        filename = message['Records'][0]['s3']['object']['key']
         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')

tests/app/celery/test_tasks.py

@@ -24,7 +24,8 @@ from app.celery.tasks import (
     persist_letter,
     get_template_class,
     update_job_to_sent_to_dvla,
-    update_letter_notifications_statuses
+    update_letter_notifications_statuses,
+    process_updates_from_file
 )
 from app.dao import jobs_dao, services_dao
 from app.models import (
@@ -1094,10 +1095,19 @@ def test_update_letter_notifications_statuses_calls_with_correct_bucket_location
         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):
+def test_update_letter_notifications_statuses_builds_updates_from_content(notify_api, mocker):
     valid_file = 'ref-foo|Sent|1|Unsorted\nref-bar|Sent|2|Sorted'
     mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=valid_file)
-    updates = update_letter_notifications_statuses(filename='foo.txt')
+    update_mock = mocker.patch('app.celery.tasks.process_updates_from_file')
+
+    update_letter_notifications_statuses(filename='foo.txt')
+
+    update_mock.assert_called_with('ref-foo|Sent|1|Unsorted\nref-bar|Sent|2|Sorted')
+
+
+def test_update_letter_notifications_statuses_builds_updates_list(notify_api, mocker):
+    valid_file = 'ref-foo|Sent|1|Unsorted\nref-bar|Sent|2|Sorted'
+    updates = process_updates_from_file(valid_file)
 
     assert len(updates) == 2
 

tests/app/notifications/rest/test_callbacks.py

@@ -531,33 +531,5 @@ def _sample_sns_s3_callback():
         "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"
-                    }
-                }
-            }
-            ]
-        }
+        "Message": '{"Records":[{"eventVersion":"2.0","eventSource":"aws:s3","awsRegion":"eu-west-1","eventTime":"2017-05-16T11:38:41.073Z","eventName":"ObjectCreated:Put","userIdentity":{"principalId":"some-p-id"},"requestParameters":{"sourceIPAddress":"8.8.8.8"},"responseElements":{"x-amz-request-id":"some-r-id","x-amz-id-2":"some-x-am-id"},"s3":{"s3SchemaVersion":"1.0","configurationId":"some-c-id","bucket":{"name":"some-bucket","ownerIdentity":{"principalId":"some-p-id"},"arn":"some-bucket-arn"},"object":{"key":"bar.txt","size":200,"eTag":"some-e-tag","versionId":"some-v-id","sequencer":"some-seq"}}}]}'  # noqa
     })