Split the update letter statuses from counting the daily sorted/unsorted numbers.

We need to back fill the daily_sorted_count tables, so we need to iterate through all the files. No need to update the notification status. So this task has been separated out.

app/celery/tasks.py

@@ -408,39 +408,49 @@ def get_template_class(template_type):
 @notify_celery.task(bind=True, name='update-letter-notifications-statuses')
 @statsd(namespace="tasks")
 def update_letter_notifications_statuses(self, filename):
+    notification_updates = parse_dvla_file(filename)
+
+    temporary_failures = []
+
+    for update in notification_updates:
+        check_billable_units(update)
+        update_letter_notification(filename, temporary_failures, update)
+    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)
+
+
+@notify_celery.task(bind=True, name="record-daily-sorted-counts")
+@statsd(namespace="tasks")
+def record_daily_sorted_counts(self, filename):
+    sorted_letter_counts = defaultdict(int)
+    notification_updates = parse_dvla_file(filename)
+    for update in notification_updates:
+        sorted_letter_counts[update.cost_threshold.lower()] += 1
+
+    unknown_status = sorted_letter_counts.keys() - {'unsorted', 'sorted'}
+    if unknown_status:
+        message = 'DVLA response file: {} contains unknown Sorted status {}'.format(
+            filename, unknown_status.__repr__()
+        )
+        raise DVLAException(message)
+
+    billing_date = get_billing_date_in_bst_from_filename(filename)
+    persist_daily_sorted_letter_counts(day=billing_date,
+                                       file_name=filename,
+                                       sorted_letter_counts=sorted_letter_counts)
+
+
+def parse_dvla_file(filename):
     bucket_location = '{}-ftp'.format(current_app.config['NOTIFY_EMAIL_DOMAIN'])
     response_file_content = s3.get_s3_file(bucket_location, filename)
-    sorted_letter_counts = defaultdict(int)
 
     try:
-        notification_updates = process_updates_from_file(response_file_content)
+        return process_updates_from_file(response_file_content)
     except TypeError:
         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)
-            sorted_letter_counts[update.cost_threshold.lower()] += 1
-
-        try:
-            unknown_status = sorted_letter_counts.keys() - {'unsorted', 'sorted'}
-            if unknown_status:
-                message = 'DVLA response file: {} contains unknown Sorted status {}'.format(
-                    filename, unknown_status
-                )
-                raise DVLAException(message)
-
-            billing_date = get_billing_date_in_bst_from_filename(filename)
-            persist_daily_sorted_letter_counts(day=billing_date,
-                                               file_name=filename,
-                                               sorted_letter_counts=sorted_letter_counts)
-        finally:
-            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 get_billing_date_in_bst_from_filename(filename):

app/notifications/notifications_letter_callback.py

@@ -9,7 +9,7 @@ from flask import (
     current_app
 )
 
-from app.celery.tasks import update_letter_notifications_statuses
+from app.celery.tasks import update_letter_notifications_statuses, record_daily_sorted_counts
 from app.v2.errors import register_errors
 from app.notifications.utils import autoconfirm_subscription
 from app.schema_validation import validate
@@ -57,6 +57,7 @@ def process_letter_response():
         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)
+            record_daily_sorted_counts.apply_async([filename], queue=QueueNames.NOTIFY)
 
     return jsonify(
         result="success", message="DVLA callback succeeded"

tests/app/celery/test_ftp_update_tasks.py

@@ -14,6 +14,7 @@ from app.models import (
     NOTIFICATION_SENDING,
     NOTIFICATION_TEMPORARY_FAILURE,
     NOTIFICATION_TECHNICAL_FAILURE,
+    DailySortedLetter
 )
 from app.celery.tasks import (
     check_billable_units,
@@ -22,7 +23,8 @@ from app.celery.tasks import (
     process_updates_from_file,
     update_letter_notifications_statuses,
     update_letter_notifications_to_error,
-    update_letter_notifications_to_sent_to_dvla
+    update_letter_notifications_to_sent_to_dvla,
+    record_daily_sorted_counts
 )
 from app.dao.daily_sorted_letter_dao import dao_get_daily_sorted_letter_by_billing_day
 
@@ -78,45 +80,6 @@ def test_update_letter_notifications_statuses_raises_dvla_exception(notify_api,
     ) in str(e)
 
 
-def test_update_letter_notifications_statuses_raises_error_for_unknown_sorted_status(
-    notify_api,
-    mocker,
-    sample_letter_template
-):
-    sent_letter_1 = create_notification(sample_letter_template, reference='ref-foo', status=NOTIFICATION_SENDING)
-    sent_letter_2 = create_notification(sample_letter_template, reference='ref-bar', status=NOTIFICATION_SENDING)
-    valid_file = '{}|Sent|1|Unsorted\n{}|Sent|2|Error'.format(
-        sent_letter_1.reference, sent_letter_2.reference)
-
-    mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=valid_file)
-
-    with pytest.raises(DVLAException) as e:
-        update_letter_notifications_statuses(filename='NOTIFY-20170823160812-RSP.TXT')
-
-    assert "DVLA response file: {filename} contains unknown Sorted status {unknown_status}".format(
-        filename="NOTIFY-20170823160812-RSP.TXT", unknown_status="{'error'}"
-    ) in str(e)
-
-
-def test_update_letter_notifications_statuses_still_raises_temp_failure_error_with_unknown_sorted_status(
-    notify_api,
-    mocker,
-    sample_letter_template
-):
-    valid_file = 'ref-foo|Failed|1|unknown'
-    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):
     s3_mock = mocker.patch('app.celery.tasks.s3.get_s3_object')
 
@@ -178,46 +141,6 @@ def test_update_letter_notifications_statuses_persisted(notify_api, mocker, samp
         filename="NOTIFY-20170823160812-RSP.TXT", failures=[format(failed_letter.reference)]) in str(e)
 
 
-def test_update_letter_notifications_statuses_persists_daily_sorted_letter_count(
-    notify_api,
-    mocker,
-    sample_letter_template
-):
-    sent_letter_1 = create_notification(sample_letter_template, reference='ref-foo', status=NOTIFICATION_SENDING)
-    sent_letter_2 = create_notification(sample_letter_template, reference='ref-bar', status=NOTIFICATION_SENDING)
-    valid_file = '{}|Sent|1|uNsOrTeD\n{}|Sent|2|SORTED'.format(
-        sent_letter_1.reference, sent_letter_2.reference)
-
-    mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=valid_file)
-    persist_letter_count_mock = mocker.patch('app.celery.tasks.persist_daily_sorted_letter_counts')
-
-    update_letter_notifications_statuses(filename='NOTIFY-20170823160812-RSP.TXT')
-
-    persist_letter_count_mock.assert_called_once_with(day=date(2017, 8, 23),
-                                                      file_name='NOTIFY-20170823160812-RSP.TXT',
-                                                      sorted_letter_counts={'unsorted': 1, 'sorted': 1})
-
-
-def test_update_letter_notifications_statuses_persists_daily_sorted_letter_count_with_no_sorted_values(
-    notify_api,
-    mocker,
-    sample_letter_template,
-    notify_db_session
-):
-    sent_letter_1 = create_notification(sample_letter_template, reference='ref-foo', status=NOTIFICATION_SENDING)
-    sent_letter_2 = create_notification(sample_letter_template, reference='ref-bar', status=NOTIFICATION_SENDING)
-    valid_file = '{}|Sent|1|Unsorted\n{}|Sent|2|Unsorted'.format(
-        sent_letter_1.reference, sent_letter_2.reference)
-    mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=valid_file)
-
-    update_letter_notifications_statuses(filename='NOTIFY-20170823160812-RSP.TXT')
-
-    daily_sorted_letter = dao_get_daily_sorted_letter_by_billing_day(date(2017, 8, 23))
-
-    assert daily_sorted_letter.unsorted_count == 2
-    assert daily_sorted_letter.sorted_count == 0
-
-
 def test_update_letter_notifications_does_not_call_send_callback_if_no_db_entry(notify_api, mocker,
                                                                                 sample_letter_template):
     sent_letter = create_notification(sample_letter_template, reference='ref-foo', status=NOTIFICATION_SENDING,
@@ -323,3 +246,77 @@ def test_persist_daily_sorted_letter_counts_saves_sorted_and_unsorted_values(cli
 
     assert day.unsorted_count == 5
     assert day.sorted_count == 1
+
+
+def test_record_daily_sorted_counts_persists_daily_sorted_letter_count(
+    notify_api,
+    notify_db_session,
+    mocker,
+):
+    valid_file = 'Letter1|Sent|1|uNsOrTeD\nLetter2|Sent|2|SORTED\nLetter3|Sent|2|Sorted'
+
+    mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=valid_file)
+
+    assert DailySortedLetter.query.count() == 0
+
+    record_daily_sorted_counts(filename='NOTIFY-20170823160812-RSP.TXT')
+
+    daily_sorted_counts = DailySortedLetter.query.all()
+    assert len(daily_sorted_counts) == 1
+    assert daily_sorted_counts[0].sorted_count == 2
+    assert daily_sorted_counts[0].unsorted_count == 1
+
+
+def test_record_daily_sorted_counts_raises_dvla_exception_with_unknown_sorted_status(
+    notify_api,
+    mocker,
+):
+    file_contents = 'ref-foo|Failed|1|invalid\nrow_2|Failed|1|MM'
+    mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=file_contents)
+    filename = "failed.txt"
+    with pytest.raises(DVLAException) as e:
+        record_daily_sorted_counts(filename=filename)
+    unknown_values = set({'invalid', 'mm'})
+    assert "DVLA response file: {} contains unknown Sorted status {}".format(
+        filename, unknown_values.__repr__()) == e.value.message
+
+
+def test_record_daily_sorted_counts_persists_daily_sorted_letter_count_with_no_sorted_values(
+    notify_api,
+    mocker,
+    notify_db_session
+):
+    valid_file = 'Letter1|Sent|1|Unsorted\nLetter2|Sent|2|Unsorted'
+    mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=valid_file)
+
+    record_daily_sorted_counts(filename='NOTIFY-20170823160812-RSP.TXT')
+
+    daily_sorted_letter = dao_get_daily_sorted_letter_by_billing_day(date(2017, 8, 23))
+
+    assert daily_sorted_letter.unsorted_count == 2
+    assert daily_sorted_letter.sorted_count == 0
+
+
+def test_record_daily_sorted_counts_can_run_twice_for_same_file(
+    notify_api,
+    mocker,
+    notify_db_session
+):
+    valid_file = 'Letter1|Sent|1|sorted\nLetter2|Sent|2|Unsorted'
+    mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=valid_file)
+
+    record_daily_sorted_counts(filename='NOTIFY-20170823160812-RSP.TXT')
+
+    daily_sorted_letter = dao_get_daily_sorted_letter_by_billing_day(date(2017, 8, 23))
+
+    assert daily_sorted_letter.unsorted_count == 1
+    assert daily_sorted_letter.sorted_count == 1
+
+    updated_file = 'Letter1|Sent|1|sorted\nLetter2|Sent|2|Unsorted\nLetter3|Sent|2|Unsorted'
+    mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=updated_file)
+
+    record_daily_sorted_counts(filename='NOTIFY-20170823160812-RSP.TXT')
+    daily_sorted_letter = dao_get_daily_sorted_letter_by_billing_day(date(2017, 8, 23))
+
+    assert daily_sorted_letter.unsorted_count == 2
+    assert daily_sorted_letter.sorted_count == 1

tests/app/notifications/rest/test_callbacks.py

@@ -82,36 +82,33 @@ def test_dvla_callback_calls_does_not_update_letter_notifications_task_with_inva
     assert not update_task.called
 
 
-def test_dvla_rs_txt_file_callback_calls_update_letter_notifications_task(client, mocker):
+@pytest.mark.parametrize("filename",
-    update_task = \
+                         ['Notify-20170411153023-rs.txt', 'Notify-20170411153023-rsp.txt'])
-        mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses.apply_async')
+def test_dvla_rs_and_rsp_txt_file_callback_calls_update_letter_notifications_task(client, mocker, filename):
-    data = _sample_sns_s3_callback('Notify-20170411153023-rs.txt')
+    update_task = mocker.patch(
+        'app.notifications.notifications_letter_callback.update_letter_notifications_statuses.apply_async')
+    daily_sorted_counts_task = mocker.patch(
+        'app.notifications.notifications_letter_callback.record_daily_sorted_counts.apply_async')
+    data = _sample_sns_s3_callback(filename)
     response = dvla_post(client, data)
 
     assert response.status_code == 200
     assert update_task.called
-    update_task.assert_called_with(['Notify-20170411153023-rs.txt'], queue='notify-internal-tasks')
+    update_task.assert_called_with([filename], queue='notify-internal-tasks')
-
+    daily_sorted_counts_task.assert_called_with([filename], 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 = \
+    update_task = mocker.patch(
-        mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses.apply_async')
+        'app.notifications.notifications_letter_callback.update_letter_notifications_statuses.apply_async')
+    daily_sorted_counts_task = mocker.patch(
+        'app.notifications.notifications_letter_callback.record_daily_sorted_counts.apply_async')
     data = _sample_sns_s3_callback('bar.ack.txt')
     response = dvla_post(client, data)
 
     assert response.status_code == 200
     update_task.assert_not_called()
+    daily_sorted_counts_task.assert_not_called()
 
 
 def test_firetext_callback_should_not_need_auth(client, mocker):