From 8219b3c032e59de8cbab5a4392761fd59ca9c14d Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 18 Mar 2021 13:01:12 +0000 Subject: [PATCH] Remove non/crown indicator in letter filenames This is not required by DVLA and since [1] we no longer care about the end of letter filenames when collating them, so removing it is safe to do. Note that the name of the ZIP files of collated letters is based on a hash of the filenames, which needed updating in tests. Before merging this we need to do a test run in Staging, so DVLA can check that a mixture of the old / new filenames won't cause issues. [1]: https://github.com/alphagov/notifications-api/pull/3172 --- app/celery/letters_pdf_tasks.py | 2 - app/letters/utils.py | 8 +-- app/service/send_notification.py | 1 - tests/app/celery/test_letters_pdf_tasks.py | 71 +++++++++---------- ...t_notification_dao_delete_notifications.py | 4 +- tests/app/letters/test_letter_utils.py | 29 ++------ .../test_send_pdf_letter_notification.py | 2 +- 7 files changed, 48 insertions(+), 69 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index a4c7e2628..9307bc8ec 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -62,7 +62,6 @@ def get_pdf_for_templated_letter(self, notification_id): notification = get_notification_by_id(notification_id, _raise=True) letter_filename = generate_letter_pdf_filename( reference=notification.reference, - crown=notification.service.crown, created_at=notification.created_at, ignore_folder=notification.key_type == KEY_TYPE_TEST, postage=notification.postage @@ -373,7 +372,6 @@ def process_sanitised_letter(self, sanitise_data): # Now we know if the letter is international, we can check what the filename should be. upload_file_name = generate_letter_pdf_filename( reference=notification.reference, - crown=notification.service.crown, created_at=notification.created_at, ignore_folder=True, postage=notification.postage diff --git a/app/letters/utils.py b/app/letters/utils.py index f79a589af..4bdb85d2a 100644 --- a/app/letters/utils.py +++ b/app/letters/utils.py @@ -25,7 +25,7 @@ class ScanErrorType(Enum): LETTERS_PDF_FILE_LOCATION_STRUCTURE = \ - '{folder}NOTIFY.{reference}.{duplex}.{letter_class}.{colour}.{crown}.{date}.pdf' + '{folder}NOTIFY.{reference}.{duplex}.{letter_class}.{colour}.{date}.pdf' PRECOMPILED_BUCKET_PREFIX = '{folder}NOTIFY.{reference}' @@ -53,14 +53,13 @@ def find_letter_pdf_in_s3(notification): return item -def generate_letter_pdf_filename(reference, crown, created_at, ignore_folder=False, postage=SECOND_CLASS): +def generate_letter_pdf_filename(reference, 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(created_at), reference=reference, duplex="D", letter_class=RESOLVE_POSTAGE_FOR_FILE_NAME[postage], colour="C", - crown="C" if crown else "N", date=created_at.strftime('%Y%m%d%H%M%S') ).upper() return upload_file_name @@ -85,7 +84,7 @@ def get_bucket_name_and_prefix_for_notification(notification): def get_reference_from_filename(filename): - # filename looks like '2018-01-13/NOTIFY.ABCDEF1234567890.D.2.C.C.20180113120000.PDF' + # filename looks like '2018-01-13/NOTIFY.ABCDEF1234567890.D.2.C.20180113120000.PDF' filename_parts = filename.split('.') return filename_parts[1] @@ -96,7 +95,6 @@ def upload_letter_pdf(notification, pdf_data, precompiled=False): upload_file_name = generate_letter_pdf_filename( reference=notification.reference, - crown=notification.service.crown, 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 af32962bb..f6d7e5e5b 100644 --- a/app/service/send_notification.py +++ b/app/service/send_notification.py @@ -199,7 +199,6 @@ def send_pdf_letter_notification(service_id, post_data): upload_filename = generate_letter_pdf_filename( reference=notification.reference, - crown=notification.service.crown, 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 540dc67cc..c9bac6535 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -98,7 +98,6 @@ def test_get_pdf_for_templated_letter_happy_path(mocker, sample_letter_notificat mock_generate_letter_pdf_filename.assert_called_once_with( reference=sample_letter_notification.reference, - crown=True, created_at=sample_letter_notification.created_at, ignore_folder=False, postage='second' @@ -176,9 +175,9 @@ def test_get_key_and_size_of_letters_to_be_sent_to_print(notify_api, mocker, sam pdf_bucket = current_app.config['LETTERS_PDF_BUCKET_NAME'] s3 = boto3.client('s3', region_name='eu-west-1') s3.create_bucket(Bucket=pdf_bucket, CreateBucketConfiguration={'LocationConstraint': 'eu-west-1'}) - s3.put_object(Bucket=pdf_bucket, Key='2020-02-17/NOTIFY.REF0.D.2.C.C.20200217160000.PDF', Body=b'1'), - s3.put_object(Bucket=pdf_bucket, Key='2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF', Body=b'22'), - s3.put_object(Bucket=pdf_bucket, Key='2020-02-16/NOTIFY.REF2.D.2.C.C.20200215180000.PDF', Body=b'333'), + s3.put_object(Bucket=pdf_bucket, Key='2020-02-17/NOTIFY.REF0.D.2.C.20200217160000.PDF', Body=b'1'), + s3.put_object(Bucket=pdf_bucket, Key='2020-02-17/NOTIFY.REF1.D.2.C.20200217150000.PDF', Body=b'22'), + s3.put_object(Bucket=pdf_bucket, Key='2020-02-16/NOTIFY.REF2.D.2.C.20200215180000.PDF', Body=b'333'), # second class create_notification( @@ -235,17 +234,17 @@ def test_get_key_and_size_of_letters_to_be_sent_to_print(notify_api, mocker, sam assert results == [ { - 'Key': '2020-02-16/NOTIFY.REF2.D.2.C.C.20200215180000.PDF', + 'Key': '2020-02-16/NOTIFY.REF2.D.2.C.20200215180000.PDF', 'Size': 3, 'ServiceId': str(sample_letter_template.service_id) }, { - 'Key': '2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF', + 'Key': '2020-02-17/NOTIFY.REF1.D.2.C.20200217150000.PDF', 'Size': 2, 'ServiceId': str(sample_letter_template.service_id) }, { - 'Key': '2020-02-17/NOTIFY.REF0.D.2.C.C.20200217160000.PDF', + 'Key': '2020-02-17/NOTIFY.REF0.D.2.C.20200217160000.PDF', 'Size': 1, 'ServiceId': str(sample_letter_template.service_id) }, @@ -260,7 +259,7 @@ def test_get_key_and_size_of_letters_to_be_sent_to_print_handles_file_not_found( pdf_bucket = current_app.config['LETTERS_PDF_BUCKET_NAME'] s3 = boto3.client('s3', region_name='eu-west-1') s3.create_bucket(Bucket=pdf_bucket, CreateBucketConfiguration={'LocationConstraint': 'eu-west-1'}) - s3.put_object(Bucket=pdf_bucket, Key='2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF', Body=b'12'), + s3.put_object(Bucket=pdf_bucket, Key='2020-02-17/NOTIFY.REF1.D.2.C.20200217150000.PDF', Body=b'12'), # no object for ref1 create_notification( @@ -282,7 +281,7 @@ def test_get_key_and_size_of_letters_to_be_sent_to_print_handles_file_not_found( ) assert results == [{ - 'Key': '2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF', + 'Key': '2020-02-17/NOTIFY.REF1.D.2.C.20200217150000.PDF', 'Size': 2, 'ServiceId': str(sample_letter_template.service_id)} ] @@ -362,13 +361,13 @@ def test_collate_letter_pdfs_to_be_sent( ) filenames = [ - '2020-02-17/NOTIFY.FIRST_CLASS.D.1.C.C.20200217140000.PDF', - '2020-02-16/NOTIFY.REF2.D.2.C.C.20200215180000.PDF', - '2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF', - '2020-02-17/NOTIFY.REF0.D.2.C.C.20200217160000.PDF', - '2020-02-15/NOTIFY.INTERNATIONAL.D.E.C.C.20200214180000.PDF', - '2020-02-14/NOTIFY.INTERNATIONAL.D.N.C.C.20200213180000.PDF', - '2020-02-17/NOTIFY.ANOTHER_SERVICE.D.2.C.C.20200217160000.PDF' + '2020-02-17/NOTIFY.FIRST_CLASS.D.1.C.20200217140000.PDF', + '2020-02-16/NOTIFY.REF2.D.2.C.20200215180000.PDF', + '2020-02-17/NOTIFY.REF1.D.2.C.20200217150000.PDF', + '2020-02-17/NOTIFY.REF0.D.2.C.20200217160000.PDF', + '2020-02-15/NOTIFY.INTERNATIONAL.D.E.C.20200214180000.PDF', + '2020-02-14/NOTIFY.INTERNATIONAL.D.N.C.20200213180000.PDF', + '2020-02-17/NOTIFY.ANOTHER_SERVICE.D.2.C.20200217160000.PDF' ] for filename in filenames: @@ -392,9 +391,9 @@ def test_collate_letter_pdfs_to_be_sent( name='zip-and-send-letter-pdfs', kwargs={ 'filenames_to_zip': [ - '2020-02-17/NOTIFY.FIRST_CLASS.D.1.C.C.20200217140000.PDF' + '2020-02-17/NOTIFY.FIRST_CLASS.D.1.C.20200217140000.PDF' ], - 'upload_filename': f'NOTIFY.2020-02-17.1.001.kHh01fdUxT9iEIYUt5Wx.{letter_template_1.service_id}.ZIP' + 'upload_filename': f'NOTIFY.2020-02-17.1.001.sO6RKzPyNrkxrR8OLonl.{letter_template_1.service_id}.ZIP' }, queue='process-ftp-tasks', compression='zlib' @@ -402,8 +401,8 @@ def test_collate_letter_pdfs_to_be_sent( assert mock_celery.call_args_list[1] == call( name='zip-and-send-letter-pdfs', kwargs={ - 'filenames_to_zip': ['2020-02-17/NOTIFY.ANOTHER_SERVICE.D.2.C.C.20200217160000.PDF'], - 'upload_filename': f'NOTIFY.2020-02-17.2.001.MezXnKP3IvNZEoMsSlVo.{service_2.id}.ZIP' + 'filenames_to_zip': ['2020-02-17/NOTIFY.ANOTHER_SERVICE.D.2.C.20200217160000.PDF'], + 'upload_filename': f'NOTIFY.2020-02-17.2.001.bGS-FKKV0QHcOUZgacEu.{service_2.id}.ZIP' }, queue='process-ftp-tasks', compression='zlib' @@ -412,10 +411,10 @@ def test_collate_letter_pdfs_to_be_sent( name='zip-and-send-letter-pdfs', kwargs={ 'filenames_to_zip': [ - '2020-02-16/NOTIFY.REF2.D.2.C.C.20200215180000.PDF', - '2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF' + '2020-02-16/NOTIFY.REF2.D.2.C.20200215180000.PDF', + '2020-02-17/NOTIFY.REF1.D.2.C.20200217150000.PDF' ], - 'upload_filename': f'NOTIFY.2020-02-17.2.002.k3x_WqC5KhB6e2DWv9Ma.{letter_template_1.service_id}.ZIP' + 'upload_filename': f'NOTIFY.2020-02-17.2.002.AmmswUYqPToXwlSZiFyK.{letter_template_1.service_id}.ZIP' }, queue='process-ftp-tasks', compression='zlib' @@ -424,9 +423,9 @@ def test_collate_letter_pdfs_to_be_sent( name='zip-and-send-letter-pdfs', kwargs={ 'filenames_to_zip': [ - '2020-02-17/NOTIFY.REF0.D.2.C.C.20200217160000.PDF' + '2020-02-17/NOTIFY.REF0.D.2.C.20200217160000.PDF' ], - 'upload_filename': f'NOTIFY.2020-02-17.2.003.J85cUw-FWlKuAIOcwdLS.{letter_template_1.service_id}.ZIP' + 'upload_filename': f'NOTIFY.2020-02-17.2.003.36PwhyI9lFKjzbPiWxwv.{letter_template_1.service_id}.ZIP' }, queue='process-ftp-tasks', compression='zlib' @@ -435,9 +434,9 @@ def test_collate_letter_pdfs_to_be_sent( name='zip-and-send-letter-pdfs', kwargs={ 'filenames_to_zip': [ - '2020-02-15/NOTIFY.INTERNATIONAL.D.E.C.C.20200214180000.PDF' + '2020-02-15/NOTIFY.INTERNATIONAL.D.E.C.20200214180000.PDF' ], - 'upload_filename': f'NOTIFY.2020-02-17.E.001.4YajCZzgzIl7zf8bjWK2.{letter_template_1.service_id}.ZIP' + 'upload_filename': f'NOTIFY.2020-02-17.E.001.lDBwqhnG__URJeGz3tH1.{letter_template_1.service_id}.ZIP' }, queue='process-ftp-tasks', compression='zlib' @@ -446,9 +445,9 @@ def test_collate_letter_pdfs_to_be_sent( name='zip-and-send-letter-pdfs', kwargs={ 'filenames_to_zip': [ - '2020-02-14/NOTIFY.INTERNATIONAL.D.N.C.C.20200213180000.PDF', + '2020-02-14/NOTIFY.INTERNATIONAL.D.N.C.20200213180000.PDF', ], - 'upload_filename': f'NOTIFY.2020-02-17.N.001.eSvP8Ph6EBKhh3k7BSA2.{letter_template_1.service_id}.ZIP' + 'upload_filename': f'NOTIFY.2020-02-17.N.001.ZE7k_jm7Bg5sYwLswkr4.{letter_template_1.service_id}.ZIP' }, queue='process-ftp-tasks', compression='zlib' @@ -727,42 +726,42 @@ def test_sanitise_letter_puts_letter_into_technical_failure_if_max_retries_excee 'LETTERS_PDF_BUCKET_NAME', NOTIFICATION_CREATED, 'first', - '2018-07-01/NOTIFY.FOO.D.1.C.C.20180701120000.PDF' + '2018-07-01/NOTIFY.FOO.D.1.C.20180701120000.PDF' ), ( KEY_TYPE_NORMAL, 'LETTERS_PDF_BUCKET_NAME', NOTIFICATION_CREATED, 'second', - '2018-07-01/NOTIFY.FOO.D.2.C.C.20180701120000.PDF' + '2018-07-01/NOTIFY.FOO.D.2.C.20180701120000.PDF' ), ( KEY_TYPE_NORMAL, 'LETTERS_PDF_BUCKET_NAME', NOTIFICATION_CREATED, 'europe', - '2018-07-01/NOTIFY.FOO.D.E.C.C.20180701120000.PDF' + '2018-07-01/NOTIFY.FOO.D.E.C.20180701120000.PDF' ), ( KEY_TYPE_NORMAL, 'LETTERS_PDF_BUCKET_NAME', NOTIFICATION_CREATED, 'rest-of-world', - '2018-07-01/NOTIFY.FOO.D.N.C.C.20180701120000.PDF' + '2018-07-01/NOTIFY.FOO.D.N.C.20180701120000.PDF' ), ( KEY_TYPE_TEST, 'TEST_LETTERS_BUCKET_NAME', NOTIFICATION_DELIVERED, 'second', - 'NOTIFY.FOO.D.2.C.C.20180701120000.PDF', + 'NOTIFY.FOO.D.2.C.20180701120000.PDF', ), ( KEY_TYPE_TEST, 'TEST_LETTERS_BUCKET_NAME', NOTIFICATION_DELIVERED, 'first', - 'NOTIFY.FOO.D.1.C.C.20180701120000.PDF', + 'NOTIFY.FOO.D.1.C.20180701120000.PDF', ), ]) def test_process_sanitised_letter_with_valid_letter( @@ -774,7 +773,7 @@ def test_process_sanitised_letter_with_valid_letter( destination_filename, ): # We save the letter as if it's 2nd class initially, and the task changes the filename to have the correct postage - filename = 'NOTIFY.FOO.D.2.C.C.20180701120000.PDF' + filename = 'NOTIFY.FOO.D.2.C.20180701120000.PDF' scan_bucket_name = current_app.config['LETTERS_SCAN_BUCKET_NAME'] template_preview_bucket_name = current_app.config['LETTER_SANITISE_BUCKET_NAME'] 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 88c4f8717..808b15386 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 @@ -144,7 +144,7 @@ def test_delete_notifications_deletes_letters_from_s3(sample_letter_template, mo 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) - filename = "{}/NOTIFY.LETTER_REF.D.2.C.C.{}.PDF".format( + filename = "{}/NOTIFY.LETTER_REF.D.2.C.{}.PDF".format( str(eight_days_ago.date()), eight_days_ago.strftime('%Y%m%d%H%M%S') ) @@ -268,7 +268,7 @@ def test_delete_notifications_deletes_letters_sent_and_in_final_state_from_table assert Notification.query.count() == 1 assert NotificationHistory.query.count() == 0 - filename = "{}/NOTIFY.LETTER_REF.D.2.C.C.{}.PDF".format( + filename = "{}/NOTIFY.LETTER_REF.D.2.C.{}.PDF".format( str(eight_days_ago.date()), eight_days_ago.strftime('%Y%m%d%H%M%S') ) diff --git a/tests/app/letters/test_letter_utils.py b/tests/app/letters/test_letter_utils.py index fb17feb84..f4821a95d 100644 --- a/tests/app/letters/test_letter_utils.py +++ b/tests/app/letters/test_letter_utils.py @@ -171,18 +171,6 @@ def test_get_bucket_name_and_prefix_for_notification_invalid_notification(): get_bucket_name_and_prefix_for_notification(None) -@pytest.mark.parametrize('crown_flag,expected_crown_text', [ - (True, 'C'), - (False, 'N'), -]) -def test_generate_letter_pdf_filename_returns_correct_filename( - notify_api, mocker, crown_flag, expected_crown_text): - created_at = datetime(2017, 12, 4, 17, 29) - filename = generate_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) - - @pytest.mark.parametrize('postage,expected_postage', [ ('second', 2), ('first', 1), @@ -190,9 +178,9 @@ def test_generate_letter_pdf_filename_returns_correct_filename( def test_generate_letter_pdf_filename_returns_correct_postage_for_filename( notify_api, postage, expected_postage): created_at = datetime(2017, 12, 4, 17, 29) - filename = generate_letter_pdf_filename(reference='foo', crown=True, created_at=created_at, postage=postage) + filename = generate_letter_pdf_filename(reference='foo', created_at=created_at, postage=postage) - assert filename == '2017-12-04/NOTIFY.FOO.D.{}.C.C.20171204172900.PDF'.format(expected_postage) + assert filename == '2017-12-04/NOTIFY.FOO.D.{}.C.20171204172900.PDF'.format(expected_postage) def test_generate_letter_pdf_filename_returns_correct_filename_for_test_letters( @@ -200,25 +188,24 @@ def test_generate_letter_pdf_filename_returns_correct_filename_for_test_letters( created_at = datetime(2017, 12, 4, 17, 29) filename = generate_letter_pdf_filename( reference='foo', - crown='C', created_at=created_at, ignore_folder=True ) - assert filename == 'NOTIFY.FOO.D.2.C.C.20171204172900.PDF' + assert filename == 'NOTIFY.FOO.D.2.C.20171204172900.PDF' def test_generate_letter_pdf_filename_returns_tomorrows_filename(notify_api, mocker): created_at = datetime(2017, 12, 4, 17, 31) - filename = generate_letter_pdf_filename(reference='foo', crown=True, created_at=created_at) + filename = generate_letter_pdf_filename(reference='foo', created_at=created_at) - assert filename == '2017-12-05/NOTIFY.FOO.D.2.C.C.20171204173100.PDF' + assert filename == '2017-12-05/NOTIFY.FOO.D.2.C.20171204173100.PDF' @mock_s3 @pytest.mark.parametrize('bucket_config_name,filename_format', [ - ('TEST_LETTERS_BUCKET_NAME', 'NOTIFY.FOO.D.2.C.C.%Y%m%d%H%M%S.PDF'), - ('LETTERS_PDF_BUCKET_NAME', '%Y-%m-%d/NOTIFY.FOO.D.2.C.C.%Y%m%d%H%M%S.PDF') + ('TEST_LETTERS_BUCKET_NAME', 'NOTIFY.FOO.D.2.C.%Y%m%d%H%M%S.PDF'), + ('LETTERS_PDF_BUCKET_NAME', '%Y-%m-%d/NOTIFY.FOO.D.2.C.%Y%m%d%H%M%S.PDF') ]) @freeze_time(FROZEN_DATE_TIME) def test_get_letter_pdf_gets_pdf_from_correct_bucket( @@ -259,7 +246,6 @@ def test_upload_letter_pdf_to_correct_bucket( filename = generate_letter_pdf_filename( reference=sample_letter_notification.reference, - crown=sample_letter_notification.service.crown, created_at=sample_letter_notification.created_at, ignore_folder=is_precompiled_letter ) @@ -286,7 +272,6 @@ def test_upload_letter_pdf_uses_postage_from_notification( filename = generate_letter_pdf_filename( reference=letter_notification.reference, - crown=letter_notification.service.crown, created_at=letter_notification.created_at, ignore_folder=False, postage=letter_notification.postage diff --git a/tests/app/service/test_send_pdf_letter_notification.py b/tests/app/service/test_send_pdf_letter_notification.py index c374691bb..850db1f96 100644 --- a/tests/app/service/test_send_pdf_letter_notification.py +++ b/tests/app/service/test_send_pdf_letter_notification.py @@ -118,5 +118,5 @@ def test_send_pdf_letter_notification_creates_notification_and_moves_letter( s3_mock.assert_called_once_with( 'service-{}/{}.pdf'.format(sample_service_full_permissions.id, file_id), - '2019-08-02/NOTIFY.{}.D.2.C.C.20190802110000.PDF'.format(notification.reference) + '2019-08-02/NOTIFY.{}.D.2.C.20190802110000.PDF'.format(notification.reference) )