From b43a367d5f3dc56cea89353ee807f0e0436625f3 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 8 Mar 2021 15:23:37 +0000 Subject: [PATCH] Relax lookup of letter PDFs in S3 buckets Previously we generated the filename we expected a letter PDF to be stored at in S3, and used that to retrieve it. However, the generated filename can change over the course of a notification's lifetime e.g. if the service changes from crown ('.C.') to non-crown ('.N.'). The prefix of the filename is stable: it's based on properties of the notification - reference and creation - that don't change. This commit changes the way we interact with letter PDFs in S3: - Uploading uses the original method to generate the full file name. The method is renamed to 'generate_' to distinguish it from the new one. - Downloading uses a new 'find_' method to get the filename using just its prefix, which makes it agnostic to changes in the filename suffix. Making this change helps to decouple our code from the requirements DVLA have on the filenames. While it means more traffic to S3, we rely on S3 in any case to download the files. From experience, we know S3 is highly reliable and performant, so don't anticipate any issues. In the tests we favour using moto to mock S3, so that the behaviour is realistic. There are a couple of places where we just mock the method, since what it returns isn't important for the test. Note that, since the new method requires a notification object, we need to change a query in one place, the columns of which were only selected to appease the original method to generate a filename. --- app/celery/letters_pdf_tasks.py | 14 +-- app/dao/notifications_dao.py | 16 +--- app/letters/utils.py | 24 ++++- app/service/send_notification.py | 4 +- tests/app/celery/test_letters_pdf_tasks.py | 96 +++++++++---------- ...t_notification_dao_delete_notifications.py | 55 +++++++---- tests/app/letters/test_letter_utils.py | 44 ++++++--- 7 files changed, 147 insertions(+), 106 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 42ced40d1..cd555f0e6 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -27,9 +27,10 @@ from app.errors import VirusScanError from app.exceptions import NotificationTechnicalFailureException from app.letters.utils import ( ScanErrorType, + find_letter_pdf_filename, + generate_letter_pdf_filename, get_billable_units_for_letter_page_count, get_file_names_from_error_bucket, - get_letter_pdf_filename, get_reference_from_filename, move_error_pdf_to_scan_bucket, move_failed_pdf, @@ -58,7 +59,7 @@ from app.models import ( 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( + letter_filename = generate_letter_pdf_filename( reference=notification.reference, crown=notification.service.crown, created_at=notification.created_at, @@ -235,12 +236,7 @@ def get_key_and_size_of_letters_to_be_sent_to_print(print_run_deadline, postage) letters_awaiting_sending = dao_get_letters_to_be_printed(print_run_deadline, postage) for letter in letters_awaiting_sending: try: - letter_file_name = get_letter_pdf_filename( - reference=letter.reference, - crown=letter.crown, - created_at=letter.created_at, - postage=postage - ) + letter_file_name = find_letter_pdf_filename(letter) letter_head = s3.head_s3_object(current_app.config['LETTERS_PDF_BUCKET_NAME'], letter_file_name) yield { "Key": letter_file_name, @@ -375,7 +371,7 @@ def process_sanitised_letter(self, sanitise_data): # The original filename could be wrong because we didn't know the postage. # Now we know if the letter is international, we can check what the filename should be. - upload_file_name = get_letter_pdf_filename( + upload_file_name = generate_letter_pdf_filename( reference=notification.reference, crown=notification.service.crown, created_at=notification.created_at, diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index e873f5ad4..2a18a0b37 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -27,7 +27,7 @@ from app.clients.sms.firetext import ( get_message_status_and_reason_from_firetext_code, ) from app.dao.dao_utils import transactional -from app.letters.utils import get_letter_pdf_filename +from app.letters.utils import find_letter_pdf_filename from app.models import ( EMAIL_TYPE, KEY_TYPE_NORMAL, @@ -453,11 +453,7 @@ def _delete_letters_from_s3( Notification.status.in_(NOTIFICATION_STATUS_TYPES_COMPLETED) ).limit(query_limit).all() for letter in letters_to_delete_from_s3: - prefix = get_letter_pdf_filename(reference=letter.reference, - crown=letter.service.crown, - created_at=letter.created_at, - ignore_folder=letter.key_type == KEY_TYPE_TEST, - postage=letter.postage) + prefix = find_letter_pdf_filename(letter) s3_objects = get_s3_bucket_objects(bucket_name=bucket_name, subfolder=prefix) for s3_object in s3_objects: try: @@ -758,13 +754,7 @@ def dao_get_letters_to_be_printed(print_run_deadline, postage, query_limit=10000 https://docs.sqlalchemy.org/en/13/orm/query.html?highlight=yield_per#sqlalchemy.orm.query.Query.yield_per https://www.mail-archive.com/sqlalchemy@googlegroups.com/msg12443.html """ - notifications = db.session.query( - Notification.id, - Notification.created_at, - Notification.reference, - Notification.service_id, - Service.crown, - ).join( + notifications = Notification.query.join( Notification.service ).filter( Notification.created_at < convert_bst_to_utc(print_run_deadline), diff --git a/app/letters/utils.py b/app/letters/utils.py index 5487c8252..78503d4ba 100644 --- a/app/letters/utils.py +++ b/app/letters/utils.py @@ -37,7 +37,27 @@ def get_folder_name(created_at): return '{}/'.format(print_datetime.date()) -def get_letter_pdf_filename(reference, crown, created_at, ignore_folder=False, postage=SECOND_CLASS): +def find_letter_pdf_filename(notification): + """ + Retrieve the filename of a letter from s3 by searching for it based on a prefix. + + Use this when retrieving existing pdfs, so that we can be more resilient if the naming convention changes. + """ + bucket_name, prefix = get_bucket_name_and_prefix_for_notification(notification) + + s3 = boto3.resource('s3') + bucket = s3.Bucket(bucket_name) + item = next(x for x in bucket.objects.filter(Prefix=prefix)) + return item.key + + +def generate_letter_pdf_filename(reference, crown, created_at, ignore_folder=False, postage=SECOND_CLASS): + """ + Generate a filename for putting a letter into s3 or sending to dvla. + + We should only use this function when uploading data. If you need to get a letter or its metadata from s3 + then use `find_letter_pdf_filename` instead. + """ upload_file_name = LETTERS_PDF_FILE_LOCATION_STRUCTURE.format( folder='' if ignore_folder else get_folder_name(created_at), reference=reference, @@ -78,7 +98,7 @@ def upload_letter_pdf(notification, pdf_data, precompiled=False): current_app.logger.info("PDF Letter {} reference {} created at {}, {} bytes".format( notification.id, notification.reference, notification.created_at, len(pdf_data))) - upload_file_name = get_letter_pdf_filename( + upload_file_name = generate_letter_pdf_filename( reference=notification.reference, crown=notification.service.crown, created_at=notification.created_at, diff --git a/app/service/send_notification.py b/app/service/send_notification.py index 0d1a3b65d..af32962bb 100644 --- a/app/service/send_notification.py +++ b/app/service/send_notification.py @@ -17,8 +17,8 @@ from app.dao.templates_dao import ( ) from app.dao.users_dao import get_user_by_id from app.letters.utils import ( + generate_letter_pdf_filename, get_billable_units_for_letter_page_count, - get_letter_pdf_filename, get_page_count, move_uploaded_pdf_to_letters_bucket, ) @@ -197,7 +197,7 @@ def send_pdf_letter_notification(service_id, post_data): postage=post_data['postage'] or template.postage, ) - upload_filename = get_letter_pdf_filename( + upload_filename = generate_letter_pdf_filename( reference=notification.reference, crown=notification.service.crown, created_at=notification.created_at, diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 6202e3d99..540dc67cc 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -68,8 +68,8 @@ 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') - mock_get_letter_pdf_filename = mocker.patch( - 'app.celery.letters_pdf_tasks.get_letter_pdf_filename', + mock_generate_letter_pdf_filename = mocker.patch( + 'app.celery.letters_pdf_tasks.generate_letter_pdf_filename', return_value='LETTER.PDF' ) get_pdf_for_templated_letter(sample_letter_notification.id) @@ -96,7 +96,7 @@ 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( + mock_generate_letter_pdf_filename.assert_called_once_with( reference=sample_letter_notification.reference, crown=True, created_at=sample_letter_notification.created_at, @@ -112,7 +112,7 @@ def test_get_pdf_for_templated_letter_non_existent_notification(notify_db_sessio def test_get_pdf_for_templated_letter_retries_upon_error(mocker, sample_letter_notification): mock_celery = mocker.patch('app.celery.letters_pdf_tasks.notify_celery.send_task', side_effect=Exception()) - mocker.patch('app.celery.letters_pdf_tasks.get_letter_pdf_filename', return_value='LETTER.PDF') + mocker.patch('app.celery.letters_pdf_tasks.generate_letter_pdf_filename', return_value='LETTER.PDF') mock_retry = mocker.patch('app.celery.letters_pdf_tasks.get_pdf_for_templated_letter.retry') mock_logger = mocker.patch('app.celery.tasks.current_app.logger.exception') @@ -127,7 +127,7 @@ def test_get_pdf_for_templated_letter_retries_upon_error(mocker, sample_letter_n def test_get_pdf_for_templated_letter_sets_technical_failure_max_retries(mocker, sample_letter_notification): mock_celery = mocker.patch('app.celery.letters_pdf_tasks.notify_celery.send_task', side_effect=Exception()) - mocker.patch('app.celery.letters_pdf_tasks.get_letter_pdf_filename', return_value='LETTER.PDF') + mocker.patch('app.celery.letters_pdf_tasks.generate_letter_pdf_filename', return_value='LETTER.PDF') mock_retry = mocker.patch( 'app.celery.letters_pdf_tasks.get_pdf_for_templated_letter.retry', side_effect=MaxRetriesExceededError) mock_update_noti = mocker.patch('app.celery.letters_pdf_tasks.update_notification_status_by_id') @@ -170,8 +170,16 @@ def test_update_billable_units_for_letter_doesnt_update_if_sent_with_test_key(mo mock_logger.assert_not_called() +@mock_s3 @freeze_time('2020-02-17 18:00:00') def test_get_key_and_size_of_letters_to_be_sent_to_print(notify_api, mocker, sample_letter_template): + 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'), + # second class create_notification( template=sample_letter_template, @@ -218,51 +226,43 @@ def test_get_key_and_size_of_letters_to_be_sent_to_print(notify_api, mocker, sam key_type=KEY_TYPE_TEST ) - mock_s3 = mocker.patch('app.celery.tasks.s3.head_s3_object', side_effect=[ - {'ContentLength': 2}, - {'ContentLength': 1}, - {'ContentLength': 3}, - ]) - results = list( get_key_and_size_of_letters_to_be_sent_to_print(datetime.now() - timedelta(minutes=30), postage='second') ) - assert mock_s3.call_count == 3 - mock_s3.assert_has_calls( - [ - call(current_app.config['LETTERS_PDF_BUCKET_NAME'], '2020-02-16/NOTIFY.REF2.D.2.C.C.20200215180000.PDF'), - call(current_app.config['LETTERS_PDF_BUCKET_NAME'], '2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF'), - call(current_app.config['LETTERS_PDF_BUCKET_NAME'], '2020-02-17/NOTIFY.REF0.D.2.C.C.20200217160000.PDF'), - ] - ) - assert len(results) == 3 assert results == [ { 'Key': '2020-02-16/NOTIFY.REF2.D.2.C.C.20200215180000.PDF', - 'Size': 2, + 'Size': 3, 'ServiceId': str(sample_letter_template.service_id) }, { 'Key': '2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF', - 'Size': 1, + 'Size': 2, 'ServiceId': str(sample_letter_template.service_id) }, { 'Key': '2020-02-17/NOTIFY.REF0.D.2.C.C.20200217160000.PDF', - 'Size': 3, + 'Size': 1, 'ServiceId': str(sample_letter_template.service_id) }, ] +@mock_s3 @freeze_time('2020-02-17 18:00:00') -def test_get_key_and_size_of_letters_to_be_sent_to_print_catches_exception( +def test_get_key_and_size_of_letters_to_be_sent_to_print_handles_file_not_found( notify_api, mocker, sample_letter_template ): + 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'), + # no object for ref1 + create_notification( template=sample_letter_template, status='created', @@ -276,30 +276,11 @@ def test_get_key_and_size_of_letters_to_be_sent_to_print_catches_exception( reference='ref1', created_at=(datetime.now() - timedelta(hours=3)) ) - error_response = { - 'Error': { - 'Code': 'FileNotFound', - 'Message': 'some error message from amazon', - 'Type': 'Sender' - } - } - mock_head_s3_object = mocker.patch('app.celery.tasks.s3.head_s3_object', side_effect=[ - {'ContentLength': 2}, - ClientError(error_response, "File not found") - ]) results = list( get_key_and_size_of_letters_to_be_sent_to_print(datetime.now() - timedelta(minutes=30), postage='second') ) - assert mock_head_s3_object.call_count == 2 - mock_head_s3_object.assert_has_calls( - [ - call(current_app.config['LETTERS_PDF_BUCKET_NAME'], '2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF'), - call(current_app.config['LETTERS_PDF_BUCKET_NAME'], '2020-02-17/NOTIFY.REF0.D.2.C.C.20200217160000.PDF'), - ] - ) - assert results == [{ 'Key': '2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF', 'Size': 2, @@ -307,6 +288,7 @@ def test_get_key_and_size_of_letters_to_be_sent_to_print_catches_exception( ] +@mock_s3 @pytest.mark.parametrize('time_to_run_task', [ "2020-02-17 18:00:00", # after 5:30pm "2020-02-18 02:00:00", # the next day after midnight, before 5:30pm we expect the same results @@ -372,15 +354,25 @@ def test_collate_letter_pdfs_to_be_sent( created_at=(datetime.now() - timedelta(hours=2)) ) - mocker.patch('app.celery.tasks.s3.head_s3_object', side_effect=[ - {'ContentLength': 1}, - {'ContentLength': 1}, - {'ContentLength': 2}, - {'ContentLength': 1}, - {'ContentLength': 3}, - {'ContentLength': 1}, - {'ContentLength': 1}, - ]) + bucket_name = current_app.config['LETTERS_PDF_BUCKET_NAME'] + s3 = boto3.client('s3', region_name='eu-west-1') + s3.create_bucket( + Bucket=bucket_name, + CreateBucketConfiguration={'LocationConstraint': 'eu-west-1'} + ) + + 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' + ] + + for filename in filenames: + s3.put_object(Bucket=bucket_name, Key=filename, Body=b'f') mock_celery = mocker.patch('app.celery.letters_pdf_tasks.notify_celery.send_task') mock_send_email_to_dvla = mocker.patch( 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 490d01e64..be636cda1 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 @@ -1,8 +1,10 @@ from datetime import date, datetime, timedelta +import boto3 import pytest from flask import current_app from freezegun import freeze_time +from moto import mock_s3 from app.dao.notifications_dao import ( delete_notifications_older_than_retention_by_type, @@ -70,6 +72,7 @@ def test_should_delete_notifications_by_type_after_seven_days( expected_email_count, expected_letter_count ): + mocker.patch("app.dao.notifications_dao.find_letter_pdf_filename") mocker.patch("app.dao.notifications_dao.get_s3_bucket_objects") email_template, letter_template, sms_template = _create_templates(sample_service) # create one notification a day between 1st and 10th from 11:00 to 19:00 of each type @@ -118,6 +121,7 @@ def test_should_not_delete_notification_history(sample_service, mocker): @pytest.mark.parametrize('notification_type', ['sms', 'email', 'letter']) def test_delete_notifications_for_days_of_retention(sample_service, notification_type, mocker): + mocker.patch('app.dao.notifications_dao.find_letter_pdf_filename') mock_get_s3 = mocker.patch("app.dao.notifications_dao.get_s3_bucket_objects") create_test_data(notification_type, sample_service) assert Notification.query.count() == 9 @@ -130,19 +134,29 @@ def test_delete_notifications_for_days_of_retention(sample_service, notification mock_get_s3.assert_not_called() +@mock_s3 @freeze_time('2019-09-01 04:30') 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") + s3 = boto3.client('s3', region_name='eu-west-1') + bucket_name = current_app.config['LETTERS_PDF_BUCKET_NAME'] + s3.create_bucket( + Bucket=bucket_name, + CreateBucketConfiguration={'LocationConstraint': 'eu-west-1'} + ) + 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 - ) + reference='LETTER_REF', created_at=eight_days_ago, sent_at=eight_days_ago) + filename = "{}/NOTIFY.LETTER_REF.D.2.C.C.{}.PDF".format( + str(eight_days_ago.date()), + eight_days_ago.strftime('%Y%m%d%H%M%S') + ) + s3.put_object(Bucket=bucket_name, Key=filename, Body=b'foo') + 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')) - ) + + with pytest.raises(s3.exceptions.NoSuchKey): + s3.get_object(Bucket=bucket_name, Key=filename) def test_delete_notifications_inserts_notification_history(sample_service): @@ -234,12 +248,19 @@ def test_delete_notifications_deletes_letters_not_sent_and_in_final_state_from_t mock_get_s3.assert_not_called() +@mock_s3 @freeze_time('2020-12-24 04:30') @pytest.mark.parametrize('notification_status', ['delivered', 'returned-letter', 'technical-failure']) def test_delete_notifications_deletes_letters_sent_and_in_final_state_from_table_and_s3( sample_service, mocker, notification_status ): - mock_get_s3 = mocker.patch("app.dao.notifications_dao.get_s3_bucket_objects") + bucket_name = current_app.config['LETTERS_PDF_BUCKET_NAME'] + s3 = boto3.client('s3', region_name='eu-west-1') + s3.create_bucket( + Bucket=bucket_name, + CreateBucketConfiguration={'LocationConstraint': 'eu-west-1'} + ) + letter_template = create_template(service=sample_service, template_type='letter') eight_days_ago = datetime.utcnow() - timedelta(days=8) create_notification( @@ -252,17 +273,19 @@ 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( + str(eight_days_ago.date()), + eight_days_ago.strftime('%Y%m%d%H%M%S') + ) + s3.put_object(Bucket=bucket_name, Key=filename, Body=b'foo') + delete_notifications_older_than_retention_by_type('letter') assert Notification.query.count() == 0 assert NotificationHistory.query.count() == 1 - 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') - ) - ) + + with pytest.raises(s3.exceptions.NoSuchKey): + s3.get_object(Bucket=bucket_name, Key=filename) @pytest.mark.parametrize('notification_status', ['pending-virus-check', 'created', 'sending']) diff --git a/tests/app/letters/test_letter_utils.py b/tests/app/letters/test_letter_utils.py index bca8ff5a6..2ae4d1031 100644 --- a/tests/app/letters/test_letter_utils.py +++ b/tests/app/letters/test_letter_utils.py @@ -9,10 +9,11 @@ from moto import mock_s3 from app.letters.utils import ( ScanErrorType, + find_letter_pdf_filename, + generate_letter_pdf_filename, get_bucket_name_and_prefix_for_notification, get_folder_name, get_letter_pdf_and_metadata, - get_letter_pdf_filename, letter_print_day, move_failed_pdf, move_sanitised_letter_to_test_or_live_pdf_bucket, @@ -46,6 +47,21 @@ def _sample_precompiled_letter_notification_using_test_key(sample_precompiled_le return sample_precompiled_letter_notification +@mock_s3 +def test_find_letter_pdf_filename_returns_filename(sample_notification): + bucket_name = current_app.config['LETTERS_PDF_BUCKET_NAME'] + s3 = boto3.client('s3', region_name='eu-west-1') + s3.create_bucket( + Bucket=bucket_name, + CreateBucketConfiguration={'LocationConstraint': 'eu-west-1'} + ) + + _, prefix = get_bucket_name_and_prefix_for_notification(sample_notification) + s3.put_object(Bucket=bucket_name, Key=f'{prefix}-and-then-some', Body=b'f') + + assert find_letter_pdf_filename(sample_notification) == f'{prefix}-and-then-some' + + @pytest.mark.parametrize('created_at,folder', [ (datetime(2017, 1, 1, 17, 29), '2017-01-01'), (datetime(2017, 1, 1, 17, 31), '2017-01-02'), @@ -145,10 +161,10 @@ def test_get_bucket_name_and_prefix_for_notification_invalid_notification(): (True, 'C'), (False, 'N'), ]) -def test_get_letter_pdf_filename_returns_correct_filename( +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 = get_letter_pdf_filename(reference='foo', crown=crown_flag, created_at=created_at) + 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) @@ -157,26 +173,30 @@ def test_get_letter_pdf_filename_returns_correct_filename( ('second', 2), ('first', 1), ]) -def test_get_letter_pdf_filename_returns_correct_postage_for_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 = get_letter_pdf_filename(reference='foo', crown=True, created_at=created_at, postage=postage) + filename = generate_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( +def test_generate_letter_pdf_filename_returns_correct_filename_for_test_letters( notify_api, mocker): created_at = datetime(2017, 12, 4, 17, 29) - filename = get_letter_pdf_filename(reference='foo', crown='C', - created_at=created_at, ignore_folder=True) + 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' -def test_get_letter_pdf_filename_returns_tomorrows_filename(notify_api, mocker): +def test_generate_letter_pdf_filename_returns_tomorrows_filename(notify_api, mocker): created_at = datetime(2017, 12, 4, 17, 31) - filename = get_letter_pdf_filename(reference='foo', crown=True, created_at=created_at) + filename = generate_letter_pdf_filename(reference='foo', crown=True, created_at=created_at) assert filename == '2017-12-05/NOTIFY.FOO.D.2.C.C.20171204173100.PDF' @@ -223,7 +243,7 @@ def test_upload_letter_pdf_to_correct_bucket( mock_s3 = mocker.patch('app.letters.utils.s3upload') - filename = get_letter_pdf_filename( + filename = generate_letter_pdf_filename( reference=sample_letter_notification.reference, crown=sample_letter_notification.service.crown, created_at=sample_letter_notification.created_at, @@ -250,7 +270,7 @@ def test_upload_letter_pdf_uses_postage_from_notification( letter_notification = create_notification(template=sample_letter_template, postage=postage) mock_s3 = mocker.patch('app.letters.utils.s3upload') - filename = get_letter_pdf_filename( + filename = generate_letter_pdf_filename( reference=letter_notification.reference, crown=letter_notification.service.crown, created_at=letter_notification.created_at,