From 79b5a735e229e54637a7cb7c7eda629b15f04d56 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Mon, 12 Mar 2018 16:09:05 +0000 Subject: [PATCH 1/2] Ensure update letter notification status always handles temporary failure In the 'update-letter-notifications-statuses' task we want to ensure that temporary failures are always handled, regardless of whether the response file we receive contains unknown Sorted statuses or not. --- app/celery/tasks.py | 29 ++++++++++++----------- tests/app/celery/test_ftp_update_tasks.py | 19 +++++++++++++++ 2 files changed, 34 insertions(+), 14 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index d7f3d7a15..f9c68d874 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -420,22 +420,23 @@ def update_letter_notifications_statuses(self, filename): update_letter_notification(filename, temporary_failures, update) sorted_letter_counts[update.cost_threshold] += 1 - 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) + try: + if sorted_letter_counts.keys() - {'Unsorted', 'Sorted'}: + unknown_status = sorted_letter_counts.keys() - {'Unsorted', 'Sorted'} - if sorted_letter_counts.keys() - {'Unsorted', 'Sorted'}: - unknown_status = sorted_letter_counts.keys() - {'Unsorted', 'Sorted'} + message = 'DVLA response file: {} contains unknown Sorted status {}'.format( + filename, unknown_status + ) + raise DVLAException(message) - 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(billing_date, sorted_letter_counts) + billing_date = get_billing_date_in_bst_from_filename(filename) + persist_daily_sorted_letter_counts(billing_date, 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/tests/app/celery/test_ftp_update_tasks.py b/tests/app/celery/test_ftp_update_tasks.py index 435ccab93..5dc714046 100644 --- a/tests/app/celery/test_ftp_update_tasks.py +++ b/tests/app/celery/test_ftp_update_tasks.py @@ -113,6 +113,25 @@ def test_update_letter_notifications_statuses_raises_error_for_unknown_sorted_st ) 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') From 41a9f5a06e916d8f7d30e8d6dfdd29d0ad4d3b5a Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Tue, 13 Mar 2018 11:47:31 +0000 Subject: [PATCH 2/2] Change format of the letter response file we expect We were previously expecting the letter response files to be in the format of 'NOTIFY..RSP.TXT' but the response files we receive use '-' in the filenames instead of '.' which was causing an error when we tried to get the date from the filename. --- app/celery/research_mode_tasks.py | 2 +- app/celery/tasks.py | 2 +- tests/app/celery/test_ftp_update_tasks.py | 28 +++++++++---------- tests/app/celery/test_research_mode_tasks.py | 4 +-- .../app/notifications/rest/test_callbacks.py | 4 +-- 5 files changed, 20 insertions(+), 20 deletions(-) diff --git a/app/celery/research_mode_tasks.py b/app/celery/research_mode_tasks.py index 0e0586466..ac2e1e8dd 100644 --- a/app/celery/research_mode_tasks.py +++ b/app/celery/research_mode_tasks.py @@ -123,7 +123,7 @@ def firetext_callback(notification_id, to): def create_fake_letter_response_file(self, reference): now = datetime.utcnow() dvla_response_data = '{}|Sent|0|Sorted'.format(reference) - upload_file_name = 'NOTIFY.{}.RSP.TXT'.format(now.strftime('%Y%m%d%H%M%S')) + upload_file_name = 'NOTIFY-{}-RSP.TXT'.format(now.strftime('%Y%m%d%H%M%S')) s3upload( filedata=dvla_response_data, diff --git a/app/celery/tasks.py b/app/celery/tasks.py index f9c68d874..167bcf58b 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -440,7 +440,7 @@ def update_letter_notifications_statuses(self, filename): def get_billing_date_in_bst_from_filename(filename): - datetime_string = filename.split('.')[1] + datetime_string = filename.split('-')[1] datetime_obj = datetime.strptime(datetime_string, '%Y%m%d%H%M%S') return convert_utc_to_bst(datetime_obj).date() diff --git a/tests/app/celery/test_ftp_update_tasks.py b/tests/app/celery/test_ftp_update_tasks.py index 5dc714046..cffb22e28 100644 --- a/tests/app/celery/test_ftp_update_tasks.py +++ b/tests/app/celery/test_ftp_update_tasks.py @@ -59,8 +59,8 @@ def test_update_letter_notifications_statuses_raises_for_invalid_format(notify_a mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=invalid_file) with pytest.raises(DVLAException) as e: - update_letter_notifications_statuses(filename='NOTIFY.20170823160812.RSP.TXT') - assert 'DVLA response file: {} has an invalid format'.format('NOTIFY.20170823160812.RSP.TXT') in str(e) + update_letter_notifications_statuses(filename='NOTIFY-20170823160812-RSP.TXT') + assert 'DVLA response file: {} has an invalid format'.format('NOTIFY-20170823160812-RSP.TXT') in str(e) def test_update_letter_notification_statuses_when_notification_does_not_exist_updates_notification_history( @@ -73,7 +73,7 @@ def test_update_letter_notification_statuses_when_notification_does_not_exist_up billable_units=1) Notification.query.filter_by(id=notification.id).delete() - update_letter_notifications_statuses(filename="NOTIFY.20170823160812.RSP.TXT") + update_letter_notifications_statuses(filename="NOTIFY-20170823160812-RSP.TXT") updated_history = NotificationHistory.query.filter_by(id=notification.id).one() assert updated_history.status == NOTIFICATION_DELIVERED @@ -106,10 +106,10 @@ def test_update_letter_notifications_statuses_raises_error_for_unknown_sorted_st 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') + 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'}" + filename="NOTIFY-20170823160812-RSP.TXT", unknown_status="{'Error'}" ) in str(e) @@ -136,10 +136,10 @@ def test_update_letter_notifications_statuses_calls_with_correct_bucket_location s3_mock = mocker.patch('app.celery.tasks.s3.get_s3_object') with set_config(notify_api, 'NOTIFY_EMAIL_DOMAIN', 'foo.bar'): - update_letter_notifications_statuses(filename='NOTIFY.20170823160812.RSP.TXT') + update_letter_notifications_statuses(filename='NOTIFY-20170823160812-RSP.TXT') s3_mock.assert_called_with('{}-ftp'.format( current_app.config['NOTIFY_EMAIL_DOMAIN']), - 'NOTIFY.20170823160812.RSP.TXT' + 'NOTIFY-20170823160812-RSP.TXT' ) @@ -148,7 +148,7 @@ def test_update_letter_notifications_statuses_builds_updates_from_content(notify mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=valid_file) update_mock = mocker.patch('app.celery.tasks.process_updates_from_file') - update_letter_notifications_statuses(filename='NOTIFY.20170823160812.RSP.TXT') + update_letter_notifications_statuses(filename='NOTIFY-20170823160812-RSP.TXT') update_mock.assert_called_with('ref-foo|Sent|1|Unsorted\nref-bar|Sent|2|Sorted') @@ -181,7 +181,7 @@ def test_update_letter_notifications_statuses_persisted(notify_api, mocker, samp mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=valid_file) with pytest.raises(expected_exception=DVLAException) as e: - update_letter_notifications_statuses(filename='NOTIFY.20170823160812.RSP.TXT') + update_letter_notifications_statuses(filename='NOTIFY-20170823160812-RSP.TXT') assert sent_letter.status == NOTIFICATION_DELIVERED assert sent_letter.billable_units == 1 @@ -190,7 +190,7 @@ def test_update_letter_notifications_statuses_persisted(notify_api, mocker, samp 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="NOTIFY.20170823160812.RSP.TXT", failures=[format(failed_letter.reference)]) in str(e) + filename="NOTIFY-20170823160812-RSP.TXT", failures=[format(failed_letter.reference)]) in str(e) def test_update_letter_notifications_statuses_persists_daily_sorted_letter_count( @@ -206,7 +206,7 @@ def test_update_letter_notifications_statuses_persists_daily_sorted_letter_count 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') + update_letter_notifications_statuses(filename='NOTIFY-20170823160812-RSP.TXT') persist_letter_count_mock.assert_called_once_with(date(2017, 8, 23), {'Unsorted': 1, 'Sorted': 1}) @@ -223,7 +223,7 @@ def test_update_letter_notifications_statuses_persists_daily_sorted_letter_count 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') + update_letter_notifications_statuses(filename='NOTIFY-20170823160812-RSP.TXT') daily_sorted_letter = dao_get_daily_sorted_letter_by_billing_day(date(2017, 8, 23)) @@ -242,7 +242,7 @@ def test_update_letter_notifications_does_not_call_send_callback_if_no_db_entry( 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' ) - update_letter_notifications_statuses(filename='NOTIFY.20170823160812.RSP.TXT') + update_letter_notifications_statuses(filename='NOTIFY-20170823160812-RSP.TXT') send_mock.assert_not_called() @@ -320,7 +320,7 @@ def test_check_billable_units_when_billable_units_does_not_match_page_count( ('20170120230000', date(2017, 1, 20)) ]) def test_get_billing_date_in_bst_from_filename(filename_date, billing_date): - filename = 'NOTIFY.{}.RSP.TXT'.format(filename_date) + filename = 'NOTIFY-{}-RSP.TXT'.format(filename_date) result = get_billing_date_in_bst_from_filename(filename) assert result == billing_date diff --git a/tests/app/celery/test_research_mode_tasks.py b/tests/app/celery/test_research_mode_tasks.py index 7c1b7d1f3..77656a62e 100644 --- a/tests/app/celery/test_research_mode_tasks.py +++ b/tests/app/celery/test_research_mode_tasks.py @@ -112,7 +112,7 @@ def test_failure_firetext_callback(phone_number): def test_create_fake_letter_response_file_uploads_response_file_s3( notify_api, mocker): mock_s3upload = mocker.patch('app.celery.research_mode_tasks.s3upload') - filename = 'NOTIFY.20180125140000.RSP.TXT' + filename = 'NOTIFY-20180125140000-RSP.TXT' with requests_mock.Mocker() as request_mock: request_mock.post( @@ -135,7 +135,7 @@ def test_create_fake_letter_response_file_uploads_response_file_s3( def test_create_fake_letter_response_file_calls_dvla_callback_on_development( notify_api, mocker): mocker.patch('app.celery.research_mode_tasks.s3upload') - filename = 'NOTIFY.20180125140000.RSP.TXT' + filename = 'NOTIFY-20180125140000-RSP.TXT' with set_config_values(notify_api, { 'NOTIFY_ENVIRONMENT': 'development' diff --git a/tests/app/notifications/rest/test_callbacks.py b/tests/app/notifications/rest/test_callbacks.py index 1654b3b68..9dc5bfa1c 100644 --- a/tests/app/notifications/rest/test_callbacks.py +++ b/tests/app/notifications/rest/test_callbacks.py @@ -96,12 +96,12 @@ def test_dvla_rs_txt_file_callback_calls_update_letter_notifications_task(client 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') + 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(['NOTIFY-20170823160812-RSP.TXT'], queue='notify-internal-tasks') def test_dvla_ack_calls_does_not_call_letter_notifications_task(client, mocker):