From 385653af44b5a4004d611e6499e65531aa36bd34 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 22 Feb 2018 15:05:37 +0000 Subject: [PATCH] Added a new exception type for DVLAException. The Notify team needs to investigate when a notification is marked as failed. We will process the whole file and mark the notifications with the appropriate status, if any are failed an exception is raised. The exception will trigger a cloud watch error for the team to investigate. --- app/celery/celery.py | 2 +- app/celery/tasks.py | 52 ++++++++++++++--------- app/exceptions.py | 3 ++ tests/app/celery/test_ftp_update_tasks.py | 23 +++++++++- 4 files changed, 56 insertions(+), 24 deletions(-) create mode 100644 app/exceptions.py diff --git a/app/celery/celery.py b/app/celery/celery.py index 5fc30a51c..4a9792cf0 100644 --- a/app/celery/celery.py +++ b/app/celery/celery.py @@ -18,7 +18,7 @@ def make_task(app): def on_failure(self, exc, task_id, args, kwargs, einfo): # ensure task will log exceptions to correct handlers - app.logger.exception('Celery task failed') + app.logger.exception('Celery task: {} failed'.format(self.name)) super().on_failure(exc, task_id, args, kwargs, einfo) def __call__(self, *args, **kwargs): diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 697a160ed..39d0755bc 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -52,6 +52,7 @@ from app.dao.provider_details_dao import get_current_provider from app.dao.service_inbound_api_dao import get_service_inbound_api_for_service 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 +from app.exceptions import DVLAException from app.models import ( DVLA_RESPONSE_STATUS_SENT, EMAIL_TYPE, @@ -469,31 +470,18 @@ def update_letter_notifications_statuses(self, filename): try: 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 + raise DVLAException('DVLA response file: {} has an invalid format'.format(filename)) else: + temporary_failures = [] for update in notification_updates: check_billable_units(update) + update_letter_notification(filename, temporary_failures, update) - status = NOTIFICATION_DELIVERED if update.status == DVLA_RESPONSE_STATUS_SENT \ - else NOTIFICATION_TEMPORARY_FAILURE - updated_count = dao_update_notifications_by_reference( - references=[update.reference], - update_dict={"status": status, - "billable_units": update.page_count, - "updated_at": datetime.utcnow() - } - ) - - if not updated_count: - msg = "Update letter notification file {filename} failed: notification either not found " \ - "or already updated from delivered. Status {status} for notification reference {reference}".format( - filename=filename, status=status, reference=update.reference) - current_app.logger.error(msg) - else: - current_app.logger.info( - 'DVLA file: {filename}, notification updated to {status}: {reference}'.format( - filename=filename, status=status, reference=str(update.reference))) + if temporary_failures: + # This will alert Notify that DVLA was unable to deliver the letters, we need to investigate + message = "DVLA response file: {filename} has failed letters with notification.reference {failures}".format( + filename=filename, failures=temporary_failures) + raise DVLAException(message) def process_updates_from_file(response_file): @@ -502,6 +490,28 @@ def process_updates_from_file(response_file): return notification_updates +def update_letter_notification(filename, temporary_failures, update): + if update.status == DVLA_RESPONSE_STATUS_SENT: + status = NOTIFICATION_DELIVERED + else: + status = NOTIFICATION_TEMPORARY_FAILURE + temporary_failures.append(update.reference) + + updated_count = dao_update_notifications_by_reference( + references=[update.reference], + update_dict={"status": status, + "billable_units": update.page_count, + "updated_at": datetime.utcnow() + } + ) + + if not updated_count: + msg = "Update letter notification file {filename} failed: notification either not found " \ + "or already updated from delivered. Status {status} for notification reference {reference}".format( + filename=filename, status=status, reference=update.reference) + current_app.logger.error(msg) + + def check_billable_units(notification_update): notification = dao_get_notification_by_reference(notification_update.reference) diff --git a/app/exceptions.py b/app/exceptions.py new file mode 100644 index 000000000..0fe1b7fe2 --- /dev/null +++ b/app/exceptions.py @@ -0,0 +1,3 @@ +class DVLAException(Exception): + def __init__(self, message): + self.message = message diff --git a/tests/app/celery/test_ftp_update_tasks.py b/tests/app/celery/test_ftp_update_tasks.py index 9fceade95..e5917757a 100644 --- a/tests/app/celery/test_ftp_update_tasks.py +++ b/tests/app/celery/test_ftp_update_tasks.py @@ -5,6 +5,7 @@ import pytest from freezegun import freeze_time from flask import current_app +from app.exceptions import DVLAException from app.models import ( Job, Notification, @@ -65,8 +66,23 @@ def test_update_letter_notifications_statuses_raises_for_invalid_format(notify_a invalid_file = 'ref-foo|Sent|1|Unsorted\nref-bar|Sent|2' mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=invalid_file) - with pytest.raises(TypeError): + with pytest.raises(DVLAException) as e: update_letter_notifications_statuses(filename='foo.txt') + assert 'DVLA response file: {} has an invalid format'.format('foo.txt') in str(e) + + +def test_update_letter_notifications_statuses_raises_dvla_exception(notify_api, mocker, sample_letter_template): + valid_file = 'ref-foo|Failed|1|Unsorted' + mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=valid_file) + create_notification(sample_letter_template, reference='ref-foo', status=NOTIFICATION_SENDING, + billable_units=0) + + with pytest.raises(DVLAException) as e: + update_letter_notifications_statuses(filename="failed.txt") + failed = ["ref-foo"] + assert "DVLA response file: {filename} has failed letters with notification.reference {failures}".format( + filename="failed.txt", failures=failed + ) in str(e) def test_update_letter_notifications_statuses_calls_with_correct_bucket_location(notify_api, mocker): @@ -114,7 +130,8 @@ def test_update_letter_notifications_statuses_persisted(notify_api, mocker, samp sent_letter.reference, failed_letter.reference) mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=valid_file) - update_letter_notifications_statuses(filename='foo.txt') + with pytest.raises(expected_exception=DVLAException) as e: + update_letter_notifications_statuses(filename='foo.txt') assert sent_letter.status == NOTIFICATION_DELIVERED assert sent_letter.billable_units == 1 @@ -122,6 +139,8 @@ def test_update_letter_notifications_statuses_persisted(notify_api, mocker, samp assert failed_letter.status == NOTIFICATION_TEMPORARY_FAILURE assert failed_letter.billable_units == 2 assert failed_letter.updated_at + assert "DVLA response file: {filename} has failed letters with notification.reference {failures}".format( + filename="foo.txt", failures=[format(failed_letter.reference)]) in str(e) def test_update_letter_notifications_does_not_call_send_callback_if_no_db_entry(notify_api, mocker,