From 4105f6638e1f3f1bbb731a3c8b9ca2d9427e25f7 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 25 Mar 2019 15:30:48 +0000 Subject: [PATCH 1/2] 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 | 64 ++++--- .../notifications_letter_callback.py | 3 +- tests/app/celery/test_ftp_update_tasks.py | 157 +++++++++--------- .../app/notifications/rest/test_callbacks.py | 33 ++-- 4 files changed, 131 insertions(+), 126 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 2928a29f3..cd4e00642 100644 --- a/app/celery/tasks.py +++ b/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): diff --git a/app/notifications/notifications_letter_callback.py b/app/notifications/notifications_letter_callback.py index 97a6bebf2..d2b9634e5 100644 --- a/app/notifications/notifications_letter_callback.py +++ b/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" diff --git a/tests/app/celery/test_ftp_update_tasks.py b/tests/app/celery/test_ftp_update_tasks.py index 70e1479b7..22367974e 100644 --- a/tests/app/celery/test_ftp_update_tasks.py +++ b/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 diff --git a/tests/app/notifications/rest/test_callbacks.py b/tests/app/notifications/rest/test_callbacks.py index 9dc5bfa1c..1fce6a79b 100644 --- a/tests/app/notifications/rest/test_callbacks.py +++ b/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): - update_task = \ - mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses.apply_async') - data = _sample_sns_s3_callback('Notify-20170411153023-rs.txt') +@pytest.mark.parametrize("filename", + ['Notify-20170411153023-rs.txt', 'Notify-20170411153023-rsp.txt']) +def test_dvla_rs_and_rsp_txt_file_callback_calls_update_letter_notifications_task(client, mocker, filename): + 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') - - -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') + 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_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') + 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('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): From cb94f949a198cf43e1ee5b0d4373277995e8e48a Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 26 Mar 2019 15:49:21 +0000 Subject: [PATCH 2/2] New command to iterate through the files on S3 and call record_daily_sorted_counts. There are many files from early on that have not had the sorted counts recorded. --- app/commands.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/app/commands.py b/app/commands.py index 67a35efed..2e7dc0599 100644 --- a/app/commands.py +++ b/app/commands.py @@ -11,6 +11,8 @@ from sqlalchemy.orm.exc import NoResultFound from notifications_utils.statsd_decorators import statsd from app import db, DATETIME_FORMAT, encryption +from app.aws import s3 +from app.celery.tasks import record_daily_sorted_counts from app.celery.nightly_tasks import send_total_sent_notifications_to_performance_platform from app.celery.service_callback_tasks import send_delivery_status_to_service from app.celery.letters_pdf_tasks import create_letters_pdf @@ -661,3 +663,15 @@ def update_emails_to_remove_gsi(service_id): """ db.session.execute(update_stmt, {'user_id': str(user.user_id)}) db.session.commit() + + +@notify_command(name='replay-daily-sorted-count-files') +@click.option('-f', '--file_extension', required=False, help="File extension to search for, defaults to rs.txt") +@statsd(namespace="tasks") +def replay_daily_sorted_count_files(file_extension): + bucket_location = '{}-ftp'.format(current_app.config['NOTIFY_EMAIL_DOMAIN']) + for filename in s3.get_list_of_files_by_suffix(bucket_name=bucket_location, + subfolder='root/dispatch', + suffix=file_extension or '.rs.txt'): + print("Create task to record daily sorted counts for file: ", filename) + record_daily_sorted_counts.apply_async([filename], queue=QueueNames.NOTIFY)