From 6a3c2734caa1bf4b887d515ec151fbc1c3c5901f Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 15 Jan 2018 14:09:46 +0000 Subject: [PATCH 1/3] Update callback handling to process DVLA response files - handle `RS.TXT` and `RSP.TXT` files --- .../notifications_letter_callback.py | 3 +- .../app/notifications/rest/test_callbacks.py | 51 +++++++++++-------- 2 files changed, 32 insertions(+), 22 deletions(-) diff --git a/app/notifications/notifications_letter_callback.py b/app/notifications/notifications_letter_callback.py index 05cf890a9..ae722e017 100644 --- a/app/notifications/notifications_letter_callback.py +++ b/app/notifications/notifications_letter_callback.py @@ -54,7 +54,8 @@ def process_letter_response(): filename = message['Records'][0]['s3']['object']['key'] current_app.logger.info('Received file from DVLA: {}'.format(filename)) - if 'rs.txt' in filename.lower(): + if filename.lower().endswith('rs.txt') or filename.lower().endswith('rsp.txt'): + current_app.logger.info('DVLA callback: Calling task to update letter notifications') update_letter_notifications_statuses.apply_async([filename], queue=QueueNames.NOTIFY) return jsonify( diff --git a/tests/app/notifications/rest/test_callbacks.py b/tests/app/notifications/rest/test_callbacks.py index e81a91042..6db37085c 100644 --- a/tests/app/notifications/rest/test_callbacks.py +++ b/tests/app/notifications/rest/test_callbacks.py @@ -69,21 +69,43 @@ def test_dvla_callback_autoconfirm_does_not_call_update_letter_notifications_tas assert not update_task.called -def test_dvla_callback_calls_update_letter_notifications_task(client, mocker): +def test_dvla_callback_calls_does_not_update_letter_notifications_task_with_invalid_file_type(client, mocker): update_task = \ mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses.apply_async') - data = _sample_sns_s3_dvla_response_callback() + + data = _sample_sns_s3_callback("bar.txt") + response = dvla_post(client, data) + + assert response.status_code == 200 + assert not update_task.called + + +def test_dvla_rs_txt_file_callback_calls_update_letter_notifications_task(client, mocker): + update_task = \ + mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses.apply_async') + data = _sample_sns_s3_callback('Notify-20170411153023-rs.txt') response = dvla_post(client, data) assert response.status_code == 200 assert update_task.called - update_task.assert_called_with(['bar.rs.txt'], queue='notify-internal-tasks') + update_task.assert_called_with(['Notify-20170411153023-rs.txt'], queue='notify-internal-tasks') + + +def test_dvla_rsp_txt_file_callback_calls_update_letter_notifications_task(client, mocker): + update_task = \ + mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses.apply_async') + data = _sample_sns_s3_callback('NOTIFY.20170823160812.RSP.TXT') + response = dvla_post(client, data) + + assert response.status_code == 200 + assert update_task.called + update_task.assert_called_with(['NOTIFY.20170823160812.RSP.TXT'], queue='notify-internal-tasks') def test_dvla_ack_calls_does_not_call_letter_notifications_task(client, mocker): update_task = \ mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses.apply_async') - data = _sample_sns_s3_dvla_ack() + data = _sample_sns_s3_callback('bar.ack.txt') response = dvla_post(client, data) assert response.status_code == 200 @@ -462,7 +484,9 @@ def test_firetext_callback_should_record_statsd(client, notify_db, notify_db_ses app.statsd_client.incr.assert_any_call("callback.firetext.delivered") -def _sample_sns_s3_dvla_ack(): +def _sample_sns_s3_callback(filename): + message_contents = '''{"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":"%s"}}}]}''' % (filename) # noqa return json.dumps({ "SigningCertURL": "foo.pem", "UnsubscribeURL": "bar", @@ -473,22 +497,7 @@ def _sample_sns_s3_dvla_ack(): "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-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.ack.txt","size":200,"eTag":"some-e-tag","versionId":"some-v-id","sequencer":"some-seq"}}}]}' # noqa - }) - - -def _sample_sns_s3_dvla_response_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-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.rs.txt","size":200,"eTag":"some-e-tag","versionId":"some-v-id","sequencer":"some-seq"}}}]}' # noqa + "Message": message_contents }) From f1c75c5c5db3e532ab157414e9da6d257d4476b0 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Mon, 15 Jan 2018 14:22:09 +0000 Subject: [PATCH 2/3] Change notification status of failed letters - Changed the notification status of letters for letters that DVLA marks as 'failed' from NOTIFICATION_TECHNICAL_FAILURE to NOTIFICATION_TEMPORARY_FAILURE. --- app/celery/tasks.py | 3 ++- tests/app/celery/test_ftp_update_tasks.py | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 57a7c5bc8..3b065f9fc 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -63,6 +63,7 @@ from app.models import ( LETTER_TYPE, NOTIFICATION_DELIVERED, NOTIFICATION_SENDING, + NOTIFICATION_TEMPORARY_FAILURE, NOTIFICATION_TECHNICAL_FAILURE, SMS_TYPE, ) @@ -476,7 +477,7 @@ def update_letter_notifications_statuses(self, filename): else: for update in notification_updates: status = NOTIFICATION_DELIVERED if update.status == DVLA_RESPONSE_STATUS_SENT \ - else NOTIFICATION_TECHNICAL_FAILURE + else NOTIFICATION_TEMPORARY_FAILURE updated_count = dao_update_notifications_by_reference( references=[update.reference], update_dict={"status": status, diff --git a/tests/app/celery/test_ftp_update_tasks.py b/tests/app/celery/test_ftp_update_tasks.py index 880189297..135f8e240 100644 --- a/tests/app/celery/test_ftp_update_tasks.py +++ b/tests/app/celery/test_ftp_update_tasks.py @@ -10,6 +10,7 @@ from app.models import ( NOTIFICATION_CREATED, NOTIFICATION_DELIVERED, NOTIFICATION_SENDING, + NOTIFICATION_TEMPORARY_FAILURE, NOTIFICATION_TECHNICAL_FAILURE ) from app.celery.tasks import ( @@ -107,7 +108,7 @@ def test_update_letter_notifications_statuses_persisted(notify_api, mocker, samp assert sent_letter.status == NOTIFICATION_DELIVERED assert sent_letter.billable_units == 1 assert sent_letter.updated_at - assert failed_letter.status == NOTIFICATION_TECHNICAL_FAILURE + assert failed_letter.status == NOTIFICATION_TEMPORARY_FAILURE assert failed_letter.billable_units == 2 assert failed_letter.updated_at From 84e25d6b98bf23e577791dfe6de0583fdadb313e Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Wed, 17 Jan 2018 09:52:13 +0000 Subject: [PATCH 3/3] Compare letter page count with billable units in DVLA response file We compare the page_count in the response file we receive from the DVLA with the billable_units of the letter. If these don't match, we log an error. --- app/celery/tasks.py | 13 ++++++ app/dao/notifications_dao.py | 7 +++ tests/app/celery/test_ftp_update_tasks.py | 45 +++++++++++++++++++ .../notification_dao/test_notification_dao.py | 21 +++++++++ 4 files changed, 86 insertions(+) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 3b065f9fc..8908496cf 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -45,6 +45,7 @@ from app.dao.notifications_dao import ( dao_update_notifications_for_job_to_sent_to_dvla, dao_update_notifications_by_reference, dao_get_last_notification_added_for_job_id, + dao_get_notification_by_reference, ) from app.dao.provider_details_dao import get_current_provider from app.dao.service_inbound_api_dao import get_service_inbound_api_for_service @@ -476,6 +477,8 @@ def update_letter_notifications_statuses(self, filename): raise else: for update in notification_updates: + check_billable_units(update) + status = NOTIFICATION_DELIVERED if update.status == DVLA_RESPONSE_STATUS_SENT \ else NOTIFICATION_TEMPORARY_FAILURE updated_count = dao_update_notifications_by_reference( @@ -503,6 +506,16 @@ def process_updates_from_file(response_file): return notification_updates +def check_billable_units(notification_update): + notification = dao_get_notification_by_reference(notification_update.reference) + + if int(notification_update.page_count) != notification.billable_units: + msg = 'Notification with id {} had {} billable_units but a page count of {}'.format( + notification.id, notification.billable_units, notification_update.page_count) + + current_app.logger.error(msg) + + @notify_celery.task(bind=True, name="send-inbound-sms", max_retries=5, default_retry_delay=300) @statsd(namespace="tasks") def send_inbound_sms_to_service(self, inbound_sms_id, service_id): diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 0d1c6e72a..61977b1c9 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -471,6 +471,13 @@ def dao_get_notifications_by_to_field(service_id, search_term, statuses=None): return results +@statsd(namespace="dao") +def dao_get_notification_by_reference(reference): + return Notification.query.filter( + Notification.reference == reference + ).one() + + @statsd(namespace="dao") def dao_get_notifications_by_references(references): return Notification.query.filter( diff --git a/tests/app/celery/test_ftp_update_tasks.py b/tests/app/celery/test_ftp_update_tasks.py index 135f8e240..9fceade95 100644 --- a/tests/app/celery/test_ftp_update_tasks.py +++ b/tests/app/celery/test_ftp_update_tasks.py @@ -1,3 +1,4 @@ +from collections import namedtuple from datetime import datetime import pytest @@ -14,6 +15,7 @@ from app.models import ( NOTIFICATION_TECHNICAL_FAILURE ) from app.celery.tasks import ( + check_billable_units, process_updates_from_file, update_dvla_job_to_error, update_job_to_sent_to_dvla, @@ -26,6 +28,15 @@ from tests.app.db import create_notification, create_service_callback_api from tests.conftest import set_config +@pytest.fixture +def notification_update(): + """ + Returns a namedtuple to use as the argument for the check_billable_units function + """ + NotificationUpdate = namedtuple('NotificationUpdate', ['reference', 'status', 'page_count', 'cost_threshold']) + return NotificationUpdate('REFERENCE_ABC', 'sent', '1', 'cost') + + def test_update_job_to_sent_to_dvla(sample_letter_template, sample_letter_job): create_notification(template=sample_letter_template, job=sample_letter_job) create_notification(template=sample_letter_template, job=sample_letter_job) @@ -163,3 +174,37 @@ def test_update_letter_notifications_to_error_updates_based_on_notification_refe assert first.sent_at is None assert first.updated_at == dt assert second.status == NOTIFICATION_CREATED + + +def test_check_billable_units_when_billable_units_matches_page_count( + client, + sample_letter_template, + mocker, + notification_update +): + mock_logger = mocker.patch('app.celery.tasks.current_app.logger.error') + + notification = create_notification(sample_letter_template, reference='REFERENCE_ABC') + notification.billable_units = 1 + + check_billable_units(notification_update) + + mock_logger.assert_not_called() + + +def test_check_billable_units_when_billable_units_does_not_match_page_count( + client, + sample_letter_template, + mocker, + notification_update +): + mock_logger = mocker.patch('app.celery.tasks.current_app.logger.error') + + notification = create_notification(sample_letter_template, reference='REFERENCE_ABC') + notification.billable_units = 3 + + check_billable_units(notification_update) + + mock_logger.assert_called_once_with( + 'Notification with id {} had 3 billable_units but a page count of 1'.format(notification.id) + ) diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 7d5083307..43b738d77 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -31,6 +31,7 @@ from app.dao.notifications_dao import ( set_scheduled_notification_to_processed, update_notification_status_by_id, update_notification_status_by_reference, + dao_get_notification_by_reference, dao_get_notifications_by_references ) from app.dao.services_dao import dao_update_service @@ -1994,6 +1995,26 @@ def test_dao_update_notifications_by_reference_returns_zero_when_no_notification assert updated_count == 0 +def test_dao_get_notification_by_reference_with_one_match_returns_notification(sample_letter_template, notify_db): + create_notification(template=sample_letter_template, reference='REF1') + notification = dao_get_notification_by_reference('REF1') + + assert notification.reference == 'REF1' + + +def test_dao_get_notification_by_reference_with_multiple_matches_raises_error(sample_letter_template, notify_db): + create_notification(template=sample_letter_template, reference='REF1') + create_notification(template=sample_letter_template, reference='REF1') + + with pytest.raises(SQLAlchemyError): + dao_get_notification_by_reference('REF1') + + +def test_dao_get_notification_by_reference_with_no_matches_raises_error(notify_db): + with pytest.raises(SQLAlchemyError): + dao_get_notification_by_reference('REF1') + + def test_dao_get_notifications_by_reference(sample_template): create_notification(template=sample_template, reference='noref') notification_1 = create_notification(template=sample_template, reference='ref')