Merge pull request #1698 from alphagov/add-exception-for-failed-letters

Added a new exception type for DVLAException

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):

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)
 

app/exceptions.py

@@ -0,0 +1,3 @@
+class DVLAException(Exception):
+    def __init__(self, message):
+        self.message = message

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,