From bb33927b3d37265d7c3dd136d0b02f318f00bae8 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 21 Sep 2020 13:46:31 +0100 Subject: [PATCH 1/3] rename letter get_folder_name args `_now`? why would we ever use a different _now? instead say created_at, because that's what it'll always be set to, even if we're replaying old letters. We always set the folder name to when the letter was created_at, or we might not know where to look to find it. `dont_use_sending_date` doesn't really tell us what might happen if we don't use it - the answer is we return an empty string. we ignore the folder entirely. so lets call it that. Also, remove use of freeze_gun in the tests, to prove that we don't use the current time in any calculations. Also add an assert to a mock in the get_pdf_for_templated_letter test, because we were mocking but not asserting before, so the tests didn't fail when the function signature changed. --- app/celery/letters_pdf_tasks.py | 5 ++--- app/dao/notifications_dao.py | 2 +- app/letters/utils.py | 16 ++++++++-------- app/service/send_notification.py | 2 +- tests/app/celery/test_letters_pdf_tasks.py | 13 ++++++++++++- tests/app/letters/test_letter_utils.py | 18 +++++++++--------- 6 files changed, 33 insertions(+), 23 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 3248082bb..0edab6092 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -60,12 +60,11 @@ def create_letters_pdf(self, notification_id): def get_pdf_for_templated_letter(self, notification_id): try: notification = get_notification_by_id(notification_id, _raise=True) - letter_filename = get_letter_pdf_filename( reference=notification.reference, crown=notification.service.crown, sending_date=notification.created_at, - dont_use_sending_date=notification.key_type == KEY_TYPE_TEST, + ignore_folder=notification.key_type == KEY_TYPE_TEST, postage=notification.postage ) letter_data = { @@ -313,7 +312,7 @@ def process_sanitised_letter(self, sanitise_data): reference=notification.reference, crown=notification.service.crown, sending_date=notification.created_at, - dont_use_sending_date=True, + ignore_folder=True, postage=notification.postage ) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 85986e396..2f75f0cd9 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -433,7 +433,7 @@ def _delete_letters_from_s3( prefix = get_letter_pdf_filename(reference=letter.reference, crown=letter.service.crown, sending_date=letter.created_at, - dont_use_sending_date=letter.key_type == KEY_TYPE_TEST, + ignore_folder=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 a04f9ffa9..1e113fefb 100644 --- a/app/letters/utils.py +++ b/app/letters/utils.py @@ -27,20 +27,20 @@ LETTERS_PDF_FILE_LOCATION_STRUCTURE = \ PRECOMPILED_BUCKET_PREFIX = '{folder}NOTIFY.{reference}' -def get_folder_name(_now, *, dont_use_sending_date=False): - if dont_use_sending_date: +def get_folder_name(created_at, *, ignore_folder=False): + if ignore_folder: folder_name = '' else: - print_datetime = convert_utc_to_bst(_now) + print_datetime = convert_utc_to_bst(created_at) if print_datetime.time() > LETTER_PROCESSING_DEADLINE: print_datetime += timedelta(days=1) folder_name = '{}/'.format(print_datetime.date()) return folder_name -def get_letter_pdf_filename(reference, crown, sending_date, dont_use_sending_date=False, postage=SECOND_CLASS): +def get_letter_pdf_filename(reference, crown, sending_date, ignore_folder=False, postage=SECOND_CLASS): upload_file_name = LETTERS_PDF_FILE_LOCATION_STRUCTURE.format( - folder=get_folder_name(sending_date, dont_use_sending_date=dont_use_sending_date), + folder=get_folder_name(sending_date, ignore_folder=ignore_folder), reference=reference, duplex="D", letter_class=RESOLVE_POSTAGE_FOR_FILE_NAME[postage], @@ -59,7 +59,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, dont_use_sending_date=False) + folder = get_folder_name(notification.created_at, ignore_folder=False) upload_file_name = PRECOMPILED_BUCKET_PREFIX.format( folder=folder, @@ -83,7 +83,7 @@ def upload_letter_pdf(notification, pdf_data, precompiled=False): reference=notification.reference, crown=notification.service.crown, sending_date=notification.created_at, - dont_use_sending_date=precompiled or notification.key_type == KEY_TYPE_TEST, + ignore_folder=precompiled or notification.key_type == KEY_TYPE_TEST, postage=notification.postage ) @@ -151,7 +151,7 @@ def move_uploaded_pdf_to_letters_bucket(source_filename, upload_filename): def move_sanitised_letter_to_test_or_live_pdf_bucket(filename, is_test_letter, created_at, new_filename): 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(created_at, dont_use_sending_date=is_test_letter) + new_filename + target_filename = get_folder_name(created_at, ignore_folder=is_test_letter) + new_filename _move_s3_object( source_bucket=current_app.config['LETTER_SANITISE_BUCKET_NAME'], diff --git a/app/service/send_notification.py b/app/service/send_notification.py index 38cd5cac8..f7d9197a8 100644 --- a/app/service/send_notification.py +++ b/app/service/send_notification.py @@ -196,7 +196,7 @@ def send_pdf_letter_notification(service_id, post_data): reference=notification.reference, crown=notification.service.crown, sending_date=notification.created_at, - dont_use_sending_date=False, + ignore_folder=False, postage=notification.postage ) diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index d1eadc9e3..d91daa192 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -62,7 +62,10 @@ def test_get_pdf_for_templated_letter_happy_path(mocker, sample_letter_notificat letter_branding = create_letter_branding(name=branding_name, filename=logo_filename) sample_letter_notification.service.letter_branding = letter_branding mock_celery = mocker.patch('app.celery.letters_pdf_tasks.notify_celery.send_task') - mocker.patch('app.celery.letters_pdf_tasks.get_letter_pdf_filename', return_value='LETTER.PDF') + mock_get_letter_pdf_filename = mocker.patch( + 'app.celery.letters_pdf_tasks.get_letter_pdf_filename', + return_value='LETTER.PDF' + ) get_pdf_for_templated_letter(sample_letter_notification.id) letter_data = { @@ -87,6 +90,14 @@ def test_get_pdf_for_templated_letter_happy_path(mocker, sample_letter_notificat queue=QueueNames.SANITISE_LETTERS ) + mock_get_letter_pdf_filename.assert_called_once_with( + reference=sample_letter_notification.reference, + crown=True, + sending_date=sample_letter_notification.created_at, + ignore_folder=False, + postage='second' + ) + def test_get_pdf_for_templated_letter_non_existent_notification(notify_api, mocker, fake_uuid): with pytest.raises(expected_exception=NoResultFound): diff --git a/tests/app/letters/test_letter_utils.py b/tests/app/letters/test_letter_utils.py index d9e471dec..2bd117fe5 100644 --- a/tests/app/letters/test_letter_utils.py +++ b/tests/app/letters/test_letter_utils.py @@ -1,6 +1,7 @@ import pytest from datetime import datetime +import dateutil import boto3 from flask import current_app from freezegun import freeze_time @@ -161,7 +162,7 @@ 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, dont_use_sending_date=True) + sending_date=sending_date, ignore_folder=True) assert filename == 'NOTIFY.FOO.D.2.C.C.20171204172900.PDF' @@ -216,7 +217,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, - dont_use_sending_date=is_precompiled_letter + ignore_folder=is_precompiled_letter ) upload_letter_pdf(sample_letter_notification, b'\x00\x01', precompiled=is_precompiled_letter) @@ -243,7 +244,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, - dont_use_sending_date=False, + ignore_folder=False, postage=letter_notification.postage ) @@ -293,7 +294,7 @@ def test_move_failed_pdf_scan_failed(notify_api): assert filename not in [o.key for o in bucket.objects.all()] -@pytest.mark.parametrize("freeze_date, expected_folder_name", +@pytest.mark.parametrize("timestamp, expected_folder_name", [("2018-04-01 17:50:00", "2018-04-02/"), ("2018-07-02 16:29:00", "2018-07-02/"), ("2018-07-02 16:30:00", "2018-07-02/"), @@ -309,15 +310,14 @@ def test_move_failed_pdf_scan_failed(notify_api): ("2018-01-02 23:30:00", "2018-01-03/"), ("2018-01-03 00:30:00", "2018-01-03/"), ]) -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, dont_use_sending_date=False) +def test_get_folder_name_in_british_summer_time(notify_api, timestamp, expected_folder_name): + timestamp = dateutil.parser.parse(timestamp) + folder_name = get_folder_name(created_at=timestamp, ignore_folder=False) assert folder_name == expected_folder_name def test_get_folder_name_returns_empty_string_for_test_letter(): - assert '' == get_folder_name(datetime.utcnow(), dont_use_sending_date=True) + assert '' == get_folder_name(datetime.utcnow(), ignore_folder=True) @mock_s3 From 4f1e5ee5a3eb01546b4b85dff7f60475e6aef944 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 21 Sep 2020 14:24:43 +0100 Subject: [PATCH 2/3] remove ignore_folder from get_folder_name it doesn't make sense to get the folder name if you know you aren't going to use it. --- app/letters/utils.py | 21 +++++++++------------ tests/app/letters/test_letter_utils.py | 6 +----- 2 files changed, 10 insertions(+), 17 deletions(-) diff --git a/app/letters/utils.py b/app/letters/utils.py index 1e113fefb..81496747a 100644 --- a/app/letters/utils.py +++ b/app/letters/utils.py @@ -27,20 +27,16 @@ LETTERS_PDF_FILE_LOCATION_STRUCTURE = \ PRECOMPILED_BUCKET_PREFIX = '{folder}NOTIFY.{reference}' -def get_folder_name(created_at, *, ignore_folder=False): - if ignore_folder: - folder_name = '' - else: - print_datetime = convert_utc_to_bst(created_at) - if print_datetime.time() > LETTER_PROCESSING_DEADLINE: - print_datetime += timedelta(days=1) - folder_name = '{}/'.format(print_datetime.date()) - return folder_name +def get_folder_name(created_at): + print_datetime = convert_utc_to_bst(created_at) + if print_datetime.time() > LETTER_PROCESSING_DEADLINE: + print_datetime += timedelta(days=1) + return '{}/'.format(print_datetime.date()) def get_letter_pdf_filename(reference, crown, sending_date, ignore_folder=False, postage=SECOND_CLASS): upload_file_name = LETTERS_PDF_FILE_LOCATION_STRUCTURE.format( - folder=get_folder_name(sending_date, ignore_folder=ignore_folder), + folder='' if ignore_folder else get_folder_name(sending_date), reference=reference, duplex="D", letter_class=RESOLVE_POSTAGE_FOR_FILE_NAME[postage], @@ -59,7 +55,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, ignore_folder=False) + folder = get_folder_name(notification.created_at) upload_file_name = PRECOMPILED_BUCKET_PREFIX.format( folder=folder, @@ -151,7 +147,8 @@ def move_uploaded_pdf_to_letters_bucket(source_filename, upload_filename): def move_sanitised_letter_to_test_or_live_pdf_bucket(filename, is_test_letter, created_at, new_filename): 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(created_at, ignore_folder=is_test_letter) + new_filename + target_folder = '' if is_test_letter else get_folder_name(created_at) + target_filename = target_folder + new_filename _move_s3_object( source_bucket=current_app.config['LETTER_SANITISE_BUCKET_NAME'], diff --git a/tests/app/letters/test_letter_utils.py b/tests/app/letters/test_letter_utils.py index 2bd117fe5..f6f76bc3a 100644 --- a/tests/app/letters/test_letter_utils.py +++ b/tests/app/letters/test_letter_utils.py @@ -312,14 +312,10 @@ def test_move_failed_pdf_scan_failed(notify_api): ]) def test_get_folder_name_in_british_summer_time(notify_api, timestamp, expected_folder_name): timestamp = dateutil.parser.parse(timestamp) - folder_name = get_folder_name(created_at=timestamp, ignore_folder=False) + folder_name = get_folder_name(created_at=timestamp) assert folder_name == expected_folder_name -def test_get_folder_name_returns_empty_string_for_test_letter(): - assert '' == get_folder_name(datetime.utcnow(), ignore_folder=True) - - @mock_s3 def test_move_sanitised_letter_to_live_pdf_bucket(notify_api, mocker): filename = 'my_letter.pdf' From 1e928a926a78f7d03427df40a5f533fb82de2050 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 21 Sep 2020 14:40:22 +0100 Subject: [PATCH 3/3] rename sending_date to created_at we don't name letters based on the day we send them on, rather, the day we create them on. If we process a letter for a second time for whatever reason, even if it's a couple of days later, it'll still go in a folder based on the created_at timestamp. There's still a slight confusion, however - if the timestamp is after 5:30pm, the folder will be for the day after. However, still the day after creation, so I think created_at still makes the most sense. Remove the term `sending_date` to try and make this relationship more apparent. --- app/celery/letters_pdf_tasks.py | 6 +++--- app/dao/notifications_dao.py | 2 +- app/letters/utils.py | 8 ++++---- app/service/send_notification.py | 2 +- tests/app/celery/test_letters_pdf_tasks.py | 4 ++-- tests/app/letters/test_letter_utils.py | 20 ++++++++++---------- 6 files changed, 21 insertions(+), 21 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 0edab6092..805eb6cc8 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -63,7 +63,7 @@ def get_pdf_for_templated_letter(self, notification_id): letter_filename = get_letter_pdf_filename( reference=notification.reference, crown=notification.service.crown, - sending_date=notification.created_at, + created_at=notification.created_at, ignore_folder=notification.key_type == KEY_TYPE_TEST, postage=notification.postage ) @@ -182,7 +182,7 @@ def get_key_and_size_of_letters_to_be_sent_to_print(print_run_deadline, postage) letter_file_name = get_letter_pdf_filename( reference=letter.reference, crown=letter.service.crown, - sending_date=letter.created_at, + created_at=letter.created_at, postage=letter.postage ) letter_head = s3.head_s3_object(current_app.config['LETTERS_PDF_BUCKET_NAME'], letter_file_name) @@ -311,7 +311,7 @@ def process_sanitised_letter(self, sanitise_data): upload_file_name = get_letter_pdf_filename( reference=notification.reference, crown=notification.service.crown, - sending_date=notification.created_at, + created_at=notification.created_at, ignore_folder=True, postage=notification.postage ) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 2f75f0cd9..ca11def46 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -432,7 +432,7 @@ def _delete_letters_from_s3( if letter.sent_at: prefix = get_letter_pdf_filename(reference=letter.reference, crown=letter.service.crown, - sending_date=letter.created_at, + created_at=letter.created_at, ignore_folder=letter.key_type == KEY_TYPE_TEST, postage=letter.postage) s3_objects = get_s3_bucket_objects(bucket_name=bucket_name, subfolder=prefix) diff --git a/app/letters/utils.py b/app/letters/utils.py index 81496747a..ed9a7b4f9 100644 --- a/app/letters/utils.py +++ b/app/letters/utils.py @@ -34,15 +34,15 @@ def get_folder_name(created_at): return '{}/'.format(print_datetime.date()) -def get_letter_pdf_filename(reference, crown, sending_date, ignore_folder=False, postage=SECOND_CLASS): +def get_letter_pdf_filename(reference, crown, created_at, ignore_folder=False, postage=SECOND_CLASS): upload_file_name = LETTERS_PDF_FILE_LOCATION_STRUCTURE.format( - folder='' if ignore_folder else get_folder_name(sending_date), + folder='' if ignore_folder else get_folder_name(created_at), reference=reference, duplex="D", letter_class=RESOLVE_POSTAGE_FOR_FILE_NAME[postage], colour="C", crown="C" if crown else "N", - date=sending_date.strftime('%Y%m%d%H%M%S') + date=created_at.strftime('%Y%m%d%H%M%S') ).upper() return upload_file_name @@ -78,7 +78,7 @@ def upload_letter_pdf(notification, pdf_data, precompiled=False): upload_file_name = get_letter_pdf_filename( reference=notification.reference, crown=notification.service.crown, - sending_date=notification.created_at, + created_at=notification.created_at, ignore_folder=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 f7d9197a8..b1e1e8224 100644 --- a/app/service/send_notification.py +++ b/app/service/send_notification.py @@ -195,7 +195,7 @@ def send_pdf_letter_notification(service_id, post_data): upload_filename = get_letter_pdf_filename( reference=notification.reference, crown=notification.service.crown, - sending_date=notification.created_at, + created_at=notification.created_at, ignore_folder=False, postage=notification.postage ) diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index d91daa192..ec34ab1e4 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -93,13 +93,13 @@ def test_get_pdf_for_templated_letter_happy_path(mocker, sample_letter_notificat mock_get_letter_pdf_filename.assert_called_once_with( reference=sample_letter_notification.reference, crown=True, - sending_date=sample_letter_notification.created_at, + created_at=sample_letter_notification.created_at, ignore_folder=False, postage='second' ) -def test_get_pdf_for_templated_letter_non_existent_notification(notify_api, mocker, fake_uuid): +def test_get_pdf_for_templated_letter_non_existent_notification(notify_db_session, mocker, fake_uuid): with pytest.raises(expected_exception=NoResultFound): get_pdf_for_templated_letter(fake_uuid) diff --git a/tests/app/letters/test_letter_utils.py b/tests/app/letters/test_letter_utils.py index f6f76bc3a..5f96988a8 100644 --- a/tests/app/letters/test_letter_utils.py +++ b/tests/app/letters/test_letter_utils.py @@ -140,8 +140,8 @@ def test_get_bucket_name_and_prefix_for_notification_invalid_notification(): ]) def test_get_letter_pdf_filename_returns_correct_filename( notify_api, mocker, crown_flag, expected_crown_text): - sending_date = datetime(2017, 12, 4, 17, 29) - filename = get_letter_pdf_filename(reference='foo', crown=crown_flag, sending_date=sending_date) + created_at = datetime(2017, 12, 4, 17, 29) + filename = get_letter_pdf_filename(reference='foo', crown=crown_flag, created_at=created_at) assert filename == '2017-12-04/NOTIFY.FOO.D.2.C.{}.20171204172900.PDF'.format(expected_crown_text) @@ -152,24 +152,24 @@ def test_get_letter_pdf_filename_returns_correct_filename( ]) def test_get_letter_pdf_filename_returns_correct_postage_for_filename( notify_api, postage, expected_postage): - sending_date = datetime(2017, 12, 4, 17, 29) - filename = get_letter_pdf_filename(reference='foo', crown=True, sending_date=sending_date, postage=postage) + created_at = datetime(2017, 12, 4, 17, 29) + filename = get_letter_pdf_filename(reference='foo', crown=True, created_at=created_at, postage=postage) assert filename == '2017-12-04/NOTIFY.FOO.D.{}.C.C.20171204172900.PDF'.format(expected_postage) def test_get_letter_pdf_filename_returns_correct_filename_for_test_letters( notify_api, mocker): - sending_date = datetime(2017, 12, 4, 17, 29) + created_at = datetime(2017, 12, 4, 17, 29) filename = get_letter_pdf_filename(reference='foo', crown='C', - sending_date=sending_date, ignore_folder=True) + created_at=created_at, ignore_folder=True) assert filename == 'NOTIFY.FOO.D.2.C.C.20171204172900.PDF' def test_get_letter_pdf_filename_returns_tomorrows_filename(notify_api, mocker): - sending_date = datetime(2017, 12, 4, 17, 31) - filename = get_letter_pdf_filename(reference='foo', crown=True, sending_date=sending_date) + created_at = datetime(2017, 12, 4, 17, 31) + filename = get_letter_pdf_filename(reference='foo', crown=True, created_at=created_at) assert filename == '2017-12-05/NOTIFY.FOO.D.2.C.C.20171204173100.PDF' @@ -216,7 +216,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, + created_at=sample_letter_notification.created_at, ignore_folder=is_precompiled_letter ) @@ -243,7 +243,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, + created_at=letter_notification.created_at, ignore_folder=False, postage=letter_notification.postage )