Merge pull request #2971 from alphagov/fix-international-filenames

Fix the filenames for international precompiled letters

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()
 

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'],

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', [
+@pytest.mark.parametrize('key_type, destination_bucket, expected_status, postage, 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'),
+        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,

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()]