From 0e0c18583f0ab0a9ac681564ad6b66f91675e4af Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Fri, 19 May 2017 10:16:34 +0100 Subject: [PATCH 1/2] Fix test data and how we parse the JSON --- .../notifications_letter_callback.py | 5 +++- .../app/notifications/rest/test_callbacks.py | 30 +------------------ 2 files changed, 5 insertions(+), 30 deletions(-) diff --git a/app/notifications/notifications_letter_callback.py b/app/notifications/notifications_letter_callback.py index 1de6c0009..cfcaf28df 100644 --- a/app/notifications/notifications_letter_callback.py +++ b/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') diff --git a/tests/app/notifications/rest/test_callbacks.py b/tests/app/notifications/rest/test_callbacks.py index 5c8f96baa..2c2ffd9cf 100644 --- a/tests/app/notifications/rest/test_callbacks.py +++ b/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 }) From 02db3be37c8ffd986baef3b946efc5068676d50e Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 22 May 2017 10:12:18 +0100 Subject: [PATCH 2/2] General refactor --- app/celery/tasks.py | 25 +++++++++++-------------- tests/app/celery/test_tasks.py | 16 +++++++++++++--- 2 files changed, 24 insertions(+), 17 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 48923d228..cce6b05f0 100644 --- a/app/celery/tasks.py +++ b/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 diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 560f68c27..27c9833a4 100644 --- a/tests/app/celery/test_tasks.py +++ b/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