From aee7887c14b772ad8ff86808d5ca3ed7a0cc6811 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Tue, 15 Sep 2020 16:17:33 +0100 Subject: [PATCH] Fix the filenames for international precompiled letters We were determing the filename for precompiled letters before we had checked if the letters were international. This meant that a letter could have a filename indicating it was 2nd class, but once we had sanitised the letter and checked the address we were setting the notification to international. This stopped these letters from being picked up to be sent to the DVLA, since the filename and postage of the letter did not match. We now regenerate the filename after the letter has been sanitised (and when we know the postage) and use the updated filename when moving the letter into the live PDF letters bucket. --- app/celery/letters_pdf_tasks.py | 18 +++++++- app/letters/utils.py | 4 +- tests/app/celery/test_letters_pdf_tasks.py | 51 ++++++++++++++++++++-- tests/app/letters/test_letter_utils.py | 6 ++- 4 files changed, 70 insertions(+), 9 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index c269ec5a0..3248082bb 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -306,7 +306,23 @@ def process_sanitised_letter(self, sanitise_data): billable_units=billable_units, recipient_address=letter_details['address'] ) - move_sanitised_letter_to_test_or_live_pdf_bucket(filename, is_test_key, notification.created_at) + + # 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( + reference=notification.reference, + crown=notification.service.crown, + sending_date=notification.created_at, + dont_use_sending_date=True, + postage=notification.postage + ) + + move_sanitised_letter_to_test_or_live_pdf_bucket( + filename, + is_test_key, + notification.created_at, + upload_file_name, + ) # We've moved the sanitised PDF from the sanitise bucket, but still need to delete the original file: original_pdf_object.delete() diff --git a/app/letters/utils.py b/app/letters/utils.py index e2c2cbe2f..a04f9ffa9 100644 --- a/app/letters/utils.py +++ b/app/letters/utils.py @@ -148,10 +148,10 @@ 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): +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) + filename + target_filename = get_folder_name(created_at, dont_use_sending_date=is_test_letter) + new_filename _move_s3_object( source_bucket=current_app.config['LETTER_SANITISE_BUCKET_NAME'], diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 02e48980d..d1eadc9e3 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -592,18 +592,60 @@ def test_sanitise_letter_puts_letter_into_technical_failure_if_max_retries_excee @mock_s3 -@pytest.mark.parametrize('key_type, destination_bucket, expected_status, destination_filename', [ - (KEY_TYPE_NORMAL, 'LETTERS_PDF_BUCKET_NAME', NOTIFICATION_CREATED, '2018-07-01/NOTIFY.foo'), - (KEY_TYPE_TEST, 'TEST_LETTERS_BUCKET_NAME', NOTIFICATION_DELIVERED, 'NOTIFY.foo'), +@pytest.mark.parametrize('key_type, destination_bucket, expected_status, postage, destination_filename', [ + ( + KEY_TYPE_NORMAL, + 'LETTERS_PDF_BUCKET_NAME', + NOTIFICATION_CREATED, + 'first', + '2018-07-01/NOTIFY.FOO.D.1.C.C.20180701120000.PDF' + ), + ( + KEY_TYPE_NORMAL, + 'LETTERS_PDF_BUCKET_NAME', + NOTIFICATION_CREATED, + 'second', + '2018-07-01/NOTIFY.FOO.D.2.C.C.20180701120000.PDF' + ), + ( + KEY_TYPE_NORMAL, + 'LETTERS_PDF_BUCKET_NAME', + NOTIFICATION_CREATED, + 'europe', + '2018-07-01/NOTIFY.FOO.D.E.C.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' + ), + ( + KEY_TYPE_TEST, + 'TEST_LETTERS_BUCKET_NAME', + NOTIFICATION_DELIVERED, + 'second', + 'NOTIFY.FOO.D.2.C.C.20180701120000.PDF', + ), + ( + KEY_TYPE_TEST, + 'TEST_LETTERS_BUCKET_NAME', + NOTIFICATION_DELIVERED, + 'first', + 'NOTIFY.FOO.D.1.C.C.20180701120000.PDF', + ), ]) def test_process_sanitised_letter_with_valid_letter( sample_letter_notification, key_type, destination_bucket, expected_status, + postage, destination_filename, ): - filename = 'NOTIFY.{}'.format(sample_letter_notification.reference) + # 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' scan_bucket_name = current_app.config['LETTERS_SCAN_BUCKET_NAME'] template_preview_bucket_name = current_app.config['LETTER_SANITISE_BUCKET_NAME'] @@ -622,6 +664,7 @@ def test_process_sanitised_letter_with_valid_letter( sample_letter_notification.key_type = key_type sample_letter_notification.billable_units = 1 sample_letter_notification.created_at = datetime(2018, 7, 1, 12) + sample_letter_notification.postage = postage encrypted_data = encryption.encrypt({ 'page_count': 2, diff --git a/tests/app/letters/test_letter_utils.py b/tests/app/letters/test_letter_utils.py index d654f83bd..d9e471dec 100644 --- a/tests/app/letters/test_letter_utils.py +++ b/tests/app/letters/test_letter_utils.py @@ -336,7 +336,8 @@ def test_move_sanitised_letter_to_live_pdf_bucket(notify_api, mocker): move_sanitised_letter_to_test_or_live_pdf_bucket( filename=filename, is_test_letter=False, - created_at=datetime.utcnow() + created_at=datetime.utcnow(), + new_filename=filename ) assert not [x for x in source_bucket.objects.all()] @@ -359,7 +360,8 @@ def test_move_sanitised_letter_to_test_pdf_bucket(notify_api, mocker): move_sanitised_letter_to_test_or_live_pdf_bucket( filename=filename, is_test_letter=True, - created_at=datetime.utcnow() + created_at=datetime.utcnow(), + new_filename=filename ) assert not [x for x in source_bucket.objects.all()]