From 702d8fa85fffafb6577fee42b18d643e3d025173 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 16 Sep 2019 14:20:40 +0100 Subject: [PATCH 1/4] Refactor the code that figures out what folder and filename to use for the letter pdf files. Now we consistently use the created_at date, so we can always get the right file location and name. The previous updates to this code were trying to solve the problem if a pdf being created at 17:29, but not ready to upload until 17:31 after the antivirus and validation check. But in those cases we would have trouble finding the file. --- app/dao/notifications_dao.py | 18 ++++++-------- app/letters/utils.py | 21 ++++++---------- app/service/send_notification.py | 5 ++-- ...t_notification_dao_delete_notifications.py | 19 +++++++++++---- tests/app/letters/test_letter_utils.py | 24 ++++++++++--------- 5 files changed, 45 insertions(+), 42 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 8976bbebc..0cd260f35 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -27,7 +27,7 @@ from app import db, create_uuid from app.aws.s3 import remove_s3_object, get_s3_bucket_objects from app.dao.dao_utils import transactional from app.errors import InvalidRequest -from app.letters.utils import LETTERS_PDF_FILE_LOCATION_STRUCTURE +from app.letters.utils import get_letter_pdf_filename from app.models import ( Notification, NotificationHistory, @@ -402,17 +402,13 @@ def _delete_letters_from_s3( ).limit(query_limit).all() for letter in letters_to_delete_from_s3: bucket_name = current_app.config['LETTERS_PDF_BUCKET_NAME'] + # I don't think we need this anymore, we should update the query to get letters sent 7 days ago if letter.sent_at: - sent_at = str(letter.sent_at.date()) - prefix = LETTERS_PDF_FILE_LOCATION_STRUCTURE.format( - folder=sent_at + "/", - reference=letter.reference, - duplex="D", - letter_class="2", - colour="C", - crown="C" if letter.service.crown else "N", - date='' - ).upper()[:-5] + prefix = get_letter_pdf_filename(reference=letter.reference, + crown=letter.service.crown, + sending_date=letter.created_at, + is_scan_letter=letter.key_type == KEY_TYPE_TEST, + postage=letter.postage) s3_objects = get_s3_bucket_objects(bucket_name=bucket_name, subfolder=prefix) for s3_object in s3_objects: try: diff --git a/app/letters/utils.py b/app/letters/utils.py index f97824b9b..48a6f405c 100644 --- a/app/letters/utils.py +++ b/app/letters/utils.py @@ -36,19 +36,16 @@ def get_folder_name(_now, is_test_or_scan_letter=False): return folder_name -def get_letter_pdf_filename(reference, crown, is_scan_letter=False, postage=SECOND_CLASS): - now = datetime.utcnow() - +def get_letter_pdf_filename(reference, crown, sending_date, is_scan_letter=False, postage=SECOND_CLASS): upload_file_name = LETTERS_PDF_FILE_LOCATION_STRUCTURE.format( - folder=get_folder_name(now, is_scan_letter), + folder=get_folder_name(sending_date, is_scan_letter), reference=reference, duplex="D", letter_class=RESOLVE_POSTAGE_FOR_FILE_NAME[postage], colour="C", crown="C" if crown else "N", - date=now.strftime('%Y%m%d%H%M%S') + date=sending_date.strftime('%Y%m%d%H%M%S') ).upper() - return upload_file_name @@ -60,12 +57,7 @@ def get_bucket_name_and_prefix_for_notification(notification): bucket_name = current_app.config['TEST_LETTERS_BUCKET_NAME'] else: bucket_name = current_app.config['LETTERS_PDF_BUCKET_NAME'] - if notification.sent_at: - folder = "{}/".format(notification.sent_at.date()) - elif notification.updated_at: - folder = get_folder_name(notification.updated_at, False) - else: - folder = get_folder_name(notification.created_at, False) + folder = get_folder_name(notification.created_at, False) upload_file_name = PRECOMPILED_BUCKET_PREFIX.format( folder=folder, @@ -86,8 +78,9 @@ def upload_letter_pdf(notification, pdf_data, precompiled=False): notification.id, notification.reference, notification.created_at, len(pdf_data))) upload_file_name = get_letter_pdf_filename( - notification.reference, - notification.service.crown, + reference=notification.reference, + crown=notification.service.crown, + sending_date=notification.created_at, is_scan_letter=precompiled or notification.key_type == KEY_TYPE_TEST, postage=notification.postage ) diff --git a/app/service/send_notification.py b/app/service/send_notification.py index 0f660cfe3..7ccb6ca36 100644 --- a/app/service/send_notification.py +++ b/app/service/send_notification.py @@ -178,8 +178,9 @@ def send_pdf_letter_notification(service_id, post_data): ) upload_filename = get_letter_pdf_filename( - notification.reference, - notification.service.crown, + reference=notification.reference, + crown=notification.service.crown, + sending_date=notification.created_at, is_scan_letter=False, postage=notification.postage ) diff --git a/tests/app/dao/notification_dao/test_notification_dao_delete_notifications.py b/tests/app/dao/notification_dao/test_notification_dao_delete_notifications.py index 92e0acebe..a1b80c750 100644 --- a/tests/app/dao/notification_dao/test_notification_dao_delete_notifications.py +++ b/tests/app/dao/notification_dao/test_notification_dao_delete_notifications.py @@ -28,7 +28,7 @@ def create_test_data(notification_type, sample_service, days_of_retention=3): create_notification(template=email_template, status='delivered') create_notification(template=sms_template, status='permanent-failure') create_notification(template=letter_template, status='temporary-failure', - reference='LETTER_REF', sent_at=datetime.utcnow()) + reference='LETTER_REF', created_at=datetime.utcnow(), sent_at=datetime.utcnow()) create_notification(template=email_template, status='delivered', created_at=datetime.utcnow() - timedelta(days=4)) create_notification(template=sms_template, status='permanent-failure', @@ -127,14 +127,25 @@ def test_delete_notifications_for_days_of_retention(sample_service, notification assert Notification.query.count() == 7 assert Notification.query.filter_by(notification_type=notification_type).count() == 1 if notification_type == 'letter': - mock_get_s3.assert_called_with(bucket_name=current_app.config['LETTERS_PDF_BUCKET_NAME'], - subfolder="{}/NOTIFY.LETTER_REF.D.2.C.C".format(str(datetime.utcnow().date())) - ) assert mock_get_s3.call_count == 2 else: mock_get_s3.assert_not_called() +def test_delete_notifications_deletes_letters_from_s3(sample_letter_template, mocker): + mock_get_s3 = mocker.patch("app.dao.notifications_dao.get_s3_bucket_objects") + eight_days_ago = datetime.utcnow() - timedelta(days=8) + create_notification(template=sample_letter_template, status='delivered', + reference='LETTER_REF', created_at=eight_days_ago, sent_at=eight_days_ago + ) + delete_notifications_older_than_retention_by_type(notification_type='letter') + mock_get_s3.assert_called_once_with(bucket_name=current_app.config['LETTERS_PDF_BUCKET_NAME'], + subfolder="{}/NOTIFY.LETTER_REF.D.2.C.C.{}.PDF".format( + str(eight_days_ago.date()), + eight_days_ago.strftime('%Y%m%d%H%M%S')) + ) + + def test_delete_notifications_inserts_notification_history(sample_service): create_test_data('sms', sample_service) assert Notification.query.count() == 9 diff --git a/tests/app/letters/test_letter_utils.py b/tests/app/letters/test_letter_utils.py index 37a0c1cd0..ea8e926c5 100644 --- a/tests/app/letters/test_letter_utils.py +++ b/tests/app/letters/test_letter_utils.py @@ -55,7 +55,7 @@ def test_get_bucket_name_and_prefix_for_notification_valid_notification(sample_n ).upper() -def test_get_bucket_name_and_prefix_for_notification_get_from_sent_at_date(sample_notification): +def test_get_bucket_name_and_prefix_for_notification_is_tomorrow_after_17_30(sample_notification): sample_notification.created_at = datetime(2019, 8, 1, 17, 35) sample_notification.sent_at = datetime(2019, 8, 2, 17, 45) @@ -68,7 +68,7 @@ def test_get_bucket_name_and_prefix_for_notification_get_from_sent_at_date(sampl ).upper() -def test_get_bucket_name_and_prefix_for_notification_from_created_at_date(sample_notification): +def test_get_bucket_name_and_prefix_for_notification_is_today_before_17_30(sample_notification): sample_notification.created_at = datetime(2019, 8, 1, 12, 00) sample_notification.updated_at = datetime(2019, 8, 2, 12, 00) sample_notification.sent_at = datetime(2019, 8, 3, 12, 00) @@ -77,7 +77,7 @@ def test_get_bucket_name_and_prefix_for_notification_from_created_at_date(sample assert bucket == current_app.config['LETTERS_PDF_BUCKET_NAME'] assert bucket_prefix == '{folder}/NOTIFY.{reference}'.format( - folder='2019-08-03', + folder='2019-08-01', reference=sample_notification.reference ).upper() @@ -137,10 +137,10 @@ def test_get_bucket_name_and_prefix_for_notification_invalid_notification(): (True, 'C'), (False, 'N'), ]) -@freeze_time("2017-12-04 17:29:00") def test_get_letter_pdf_filename_returns_correct_filename( notify_api, mocker, crown_flag, expected_crown_text): - filename = get_letter_pdf_filename(reference='foo', crown=crown_flag) + sending_date = datetime(2017, 12, 4, 17, 29) + filename = get_letter_pdf_filename(reference='foo', crown=crown_flag, sending_date=sending_date) assert filename == '2017-12-04/NOTIFY.FOO.D.2.C.{}.20171204172900.PDF'.format(expected_crown_text) @@ -149,25 +149,25 @@ def test_get_letter_pdf_filename_returns_correct_filename( ('second', 2), ('first', 1), ]) -@freeze_time("2017-12-04 17:29:00") def test_get_letter_pdf_filename_returns_correct_postage_for_filename( notify_api, postage, expected_postage): - filename = get_letter_pdf_filename(reference='foo', crown=True, postage=postage) + sending_date = datetime(2017, 12, 4, 17, 29) + filename = get_letter_pdf_filename(reference='foo', crown=True, sending_date=sending_date, postage=postage) assert filename == '2017-12-04/NOTIFY.FOO.D.{}.C.C.20171204172900.PDF'.format(expected_postage) -@freeze_time("2017-12-04 17:29:00") def test_get_letter_pdf_filename_returns_correct_filename_for_test_letters( notify_api, mocker): - filename = get_letter_pdf_filename(reference='foo', crown='C', is_scan_letter=True) + sending_date = datetime(2017, 12, 4, 17, 29) + filename = get_letter_pdf_filename(reference='foo', crown='C', sending_date=sending_date, is_scan_letter=True) assert filename == 'NOTIFY.FOO.D.2.C.C.20171204172900.PDF' -@freeze_time("2017-12-04 17:31:00") def test_get_letter_pdf_filename_returns_tomorrows_filename(notify_api, mocker): - filename = get_letter_pdf_filename(reference='foo', crown=True) + sending_date = datetime(2017, 12, 4, 17, 31) + filename = get_letter_pdf_filename(reference='foo', crown=True, sending_date=sending_date) assert filename == '2017-12-05/NOTIFY.FOO.D.2.C.C.20171204173100.PDF' @@ -214,6 +214,7 @@ def test_upload_letter_pdf_to_correct_bucket( filename = get_letter_pdf_filename( reference=sample_letter_notification.reference, crown=sample_letter_notification.service.crown, + sending_date=sample_letter_notification.created_at, is_scan_letter=is_precompiled_letter ) @@ -240,6 +241,7 @@ def test_upload_letter_pdf_uses_postage_from_notification( filename = get_letter_pdf_filename( reference=letter_notification.reference, crown=letter_notification.service.crown, + sending_date=letter_notification.created_at, is_scan_letter=False, postage=letter_notification.postage ) From f234e945725008b8176921ab07774ee315aa4cf5 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 16 Sep 2019 15:10:10 +0100 Subject: [PATCH 2/4] Change the variable name to make a little more sense. --- app/dao/notifications_dao.py | 2 +- app/letters/utils.py | 10 +++++----- app/service/send_notification.py | 2 +- tests/app/letters/test_letter_utils.py | 11 ++++++----- 4 files changed, 13 insertions(+), 12 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 0cd260f35..28c0ea5ef 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -407,7 +407,7 @@ def _delete_letters_from_s3( prefix = get_letter_pdf_filename(reference=letter.reference, crown=letter.service.crown, sending_date=letter.created_at, - is_scan_letter=letter.key_type == KEY_TYPE_TEST, + dont_use_sending_date=letter.key_type == KEY_TYPE_TEST, postage=letter.postage) s3_objects = get_s3_bucket_objects(bucket_name=bucket_name, subfolder=prefix) for s3_object in s3_objects: diff --git a/app/letters/utils.py b/app/letters/utils.py index 48a6f405c..25836f475 100644 --- a/app/letters/utils.py +++ b/app/letters/utils.py @@ -25,8 +25,8 @@ LETTERS_PDF_FILE_LOCATION_STRUCTURE = \ PRECOMPILED_BUCKET_PREFIX = '{folder}NOTIFY.{reference}' -def get_folder_name(_now, is_test_or_scan_letter=False): - if is_test_or_scan_letter: +def get_folder_name(_now, dont_use_sending_date=False): + if dont_use_sending_date: folder_name = '' else: print_datetime = convert_utc_to_bst(_now) @@ -36,9 +36,9 @@ def get_folder_name(_now, is_test_or_scan_letter=False): return folder_name -def get_letter_pdf_filename(reference, crown, sending_date, is_scan_letter=False, postage=SECOND_CLASS): +def get_letter_pdf_filename(reference, crown, sending_date, dont_use_sending_date=False, postage=SECOND_CLASS): upload_file_name = LETTERS_PDF_FILE_LOCATION_STRUCTURE.format( - folder=get_folder_name(sending_date, is_scan_letter), + folder=get_folder_name(sending_date, dont_use_sending_date), reference=reference, duplex="D", letter_class=RESOLVE_POSTAGE_FOR_FILE_NAME[postage], @@ -81,7 +81,7 @@ def upload_letter_pdf(notification, pdf_data, precompiled=False): reference=notification.reference, crown=notification.service.crown, sending_date=notification.created_at, - is_scan_letter=precompiled or notification.key_type == KEY_TYPE_TEST, + dont_use_sending_date=precompiled or notification.key_type == KEY_TYPE_TEST, postage=notification.postage ) diff --git a/app/service/send_notification.py b/app/service/send_notification.py index 7ccb6ca36..00c78936b 100644 --- a/app/service/send_notification.py +++ b/app/service/send_notification.py @@ -181,7 +181,7 @@ def send_pdf_letter_notification(service_id, post_data): reference=notification.reference, crown=notification.service.crown, sending_date=notification.created_at, - is_scan_letter=False, + dont_use_sending_date=False, postage=notification.postage ) diff --git a/tests/app/letters/test_letter_utils.py b/tests/app/letters/test_letter_utils.py index ea8e926c5..844ada741 100644 --- a/tests/app/letters/test_letter_utils.py +++ b/tests/app/letters/test_letter_utils.py @@ -160,7 +160,8 @@ def test_get_letter_pdf_filename_returns_correct_postage_for_filename( def test_get_letter_pdf_filename_returns_correct_filename_for_test_letters( notify_api, mocker): sending_date = datetime(2017, 12, 4, 17, 29) - filename = get_letter_pdf_filename(reference='foo', crown='C', sending_date=sending_date, is_scan_letter=True) + filename = get_letter_pdf_filename(reference='foo', crown='C', + sending_date=sending_date, dont_use_sending_date=True) assert filename == 'NOTIFY.FOO.D.2.C.C.20171204172900.PDF' @@ -215,7 +216,7 @@ def test_upload_letter_pdf_to_correct_bucket( reference=sample_letter_notification.reference, crown=sample_letter_notification.service.crown, sending_date=sample_letter_notification.created_at, - is_scan_letter=is_precompiled_letter + dont_use_sending_date=is_precompiled_letter ) upload_letter_pdf(sample_letter_notification, b'\x00\x01', precompiled=is_precompiled_letter) @@ -242,7 +243,7 @@ def test_upload_letter_pdf_uses_postage_from_notification( reference=letter_notification.reference, crown=letter_notification.service.crown, sending_date=letter_notification.created_at, - is_scan_letter=False, + dont_use_sending_date=False, postage=letter_notification.postage ) @@ -329,12 +330,12 @@ def test_copy_redaction_failed_pdf(notify_api): def test_get_folder_name_in_british_summer_time(notify_api, freeze_date, expected_folder_name): with freeze_time(freeze_date): now = datetime.utcnow() - folder_name = get_folder_name(_now=now, is_test_or_scan_letter=False) + folder_name = get_folder_name(_now=now, dont_use_sending_date=False) assert folder_name == expected_folder_name def test_get_folder_name_returns_empty_string_for_test_letter(): - assert '' == get_folder_name(datetime.utcnow(), is_test_or_scan_letter=True) + assert '' == get_folder_name(datetime.utcnow(), dont_use_sending_date=True) @freeze_time('2017-07-07 20:00:00') From a1863fa419c533de95fcaea58c3e707bf1062e51 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 20 Sep 2019 16:25:25 +0100 Subject: [PATCH 3/4] Update all calls to `get_folder_name` to include the parameter name. Use created_at date of the notification for precompiled letters. --- app/celery/letters_pdf_tasks.py | 8 +++++--- app/letters/utils.py | 6 +++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 1dccb9b2a..434f2fdb7 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -253,7 +253,9 @@ def process_virus_scan_passed(self, filename): _upload_pdf_to_test_or_live_pdf_bucket( new_pdf, filename, - is_test_letter=is_test_key) + is_test_letter=is_test_key, + created_at=notification.created_at + ) update_letter_pdf_status( reference=reference, @@ -284,10 +286,10 @@ def _move_invalid_letter_and_update_status(notification, filename, scan_pdf_obje update_notification_status_by_id(notification.id, NOTIFICATION_TECHNICAL_FAILURE) -def _upload_pdf_to_test_or_live_pdf_bucket(pdf_data, filename, is_test_letter): +def _upload_pdf_to_test_or_live_pdf_bucket(pdf_data, filename, is_test_letter, created_at): target_bucket_config = 'TEST_LETTERS_BUCKET_NAME' if is_test_letter else 'LETTERS_PDF_BUCKET_NAME' target_bucket_name = current_app.config[target_bucket_config] - target_filename = get_folder_name(datetime.utcnow(), is_test_letter) + filename + target_filename = get_folder_name(created_at, dont_use_sending_date=is_test_letter) + filename s3upload( filedata=pdf_data, diff --git a/app/letters/utils.py b/app/letters/utils.py index 25836f475..98e954fb5 100644 --- a/app/letters/utils.py +++ b/app/letters/utils.py @@ -25,7 +25,7 @@ LETTERS_PDF_FILE_LOCATION_STRUCTURE = \ PRECOMPILED_BUCKET_PREFIX = '{folder}NOTIFY.{reference}' -def get_folder_name(_now, dont_use_sending_date=False): +def get_folder_name(_now, *, dont_use_sending_date=False): if dont_use_sending_date: folder_name = '' else: @@ -38,7 +38,7 @@ def get_folder_name(_now, dont_use_sending_date=False): def get_letter_pdf_filename(reference, crown, sending_date, dont_use_sending_date=False, postage=SECOND_CLASS): upload_file_name = LETTERS_PDF_FILE_LOCATION_STRUCTURE.format( - folder=get_folder_name(sending_date, dont_use_sending_date), + folder=get_folder_name(sending_date, dont_use_sending_date=dont_use_sending_date), reference=reference, duplex="D", letter_class=RESOLVE_POSTAGE_FOR_FILE_NAME[postage], @@ -57,7 +57,7 @@ def get_bucket_name_and_prefix_for_notification(notification): bucket_name = current_app.config['TEST_LETTERS_BUCKET_NAME'] else: bucket_name = current_app.config['LETTERS_PDF_BUCKET_NAME'] - folder = get_folder_name(notification.created_at, False) + folder = get_folder_name(notification.created_at, dont_use_sending_date=False) upload_file_name = PRECOMPILED_BUCKET_PREFIX.format( folder=folder, From 1d0ddeb17e3ffbda27413097a4594b87780cb2ac Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 23 Sep 2019 13:42:45 +0100 Subject: [PATCH 4/4] Fix changes made with the merge conflicts. --- tests/app/celery/test_letters_pdf_tasks.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 2ee2ee388..441e3bffc 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -114,11 +114,12 @@ def test_get_letters_pdf_calculates_billing_units( @freeze_time("2017-12-04 17:31:00") -def test_create_letters_pdf_calls_s3upload(mocker, sample_letter_notification): +def test_create_letters_pdf_calls_s3upload(mocker, sample_letter_template): mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', return_value=(b'\x00\x01', '1')) mock_s3 = mocker.patch('app.letters.utils.s3upload') + notification = create_notification(template=sample_letter_template, reference='FOO', key_type='normal') - create_letters_pdf(sample_letter_notification.id) + create_letters_pdf(notification.id) mock_s3.assert_called_with( bucket_name=current_app.config['LETTERS_PDF_BUCKET_NAME'], @@ -129,12 +130,12 @@ def test_create_letters_pdf_calls_s3upload(mocker, sample_letter_notification): @freeze_time("2017-12-04 17:31:00") -def test_create_letters_pdf_calls_s3upload_for_test_letters(mocker, sample_letter_notification): +def test_create_letters_pdf_calls_s3upload_for_test_letters(mocker, sample_letter_template): mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', return_value=(b'\x00\x01', '1')) mock_s3 = mocker.patch('app.letters.utils.s3upload') - sample_letter_notification.key_type = 'test' + notification = create_notification(template=sample_letter_template, reference='FOO', key_type='test') - create_letters_pdf(sample_letter_notification.id) + create_letters_pdf(notification.id) mock_s3.assert_called_with( bucket_name=current_app.config['TEST_LETTERS_BUCKET_NAME'],