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,