Merge pull request #2974 from alphagov/make-letter-folder-name-code-readable

Make letter folder name code readable

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,
+            created_at=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 = {
@@ -183,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)
@@ -312,8 +311,8 @@ 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,
-            dont_use_sending_date=True,
+            ignore_folder=True,
             postage=notification.postage
         )
 

app/dao/notifications_dao.py

@@ -432,8 +432,8 @@ 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,
-                                             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:

app/letters/utils.py

@@ -27,26 +27,22 @@ LETTERS_PDF_FILE_LOCATION_STRUCTURE = \
 PRECOMPILED_BUCKET_PREFIX = '{folder}NOTIFY.{reference}'
 
 
-def get_folder_name(_now, *, dont_use_sending_date=False):
+def get_folder_name(created_at):
-    if dont_use_sending_date:
+    print_datetime = convert_utc_to_bst(created_at)
-        folder_name = ''
+    if print_datetime.time() > LETTER_PROCESSING_DEADLINE:
-    else:
+        print_datetime += timedelta(days=1)
-        print_datetime = convert_utc_to_bst(_now)
+    return '{}/'.format(print_datetime.date())
-        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, created_at, 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='' 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
 
@@ -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, dont_use_sending_date=False)
+        folder = get_folder_name(notification.created_at)
 
     upload_file_name = PRECOMPILED_BUCKET_PREFIX.format(
         folder=folder,
@@ -82,8 +78,8 @@ 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,
-        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 +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, dont_use_sending_date=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'],

app/service/send_notification.py

@@ -195,8 +195,8 @@ 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,
-        dont_use_sending_date=False,
+        ignore_folder=False,
         postage=notification.postage
     )
 

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,8 +90,16 @@ 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,
+        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)
 

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
@@ -139,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)
+    created_at = datetime(2017, 12, 4, 17, 29)
-    filename = get_letter_pdf_filename(reference='foo', crown=crown_flag, sending_date=sending_date)
+    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)
 
@@ -151,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)
+    created_at = datetime(2017, 12, 4, 17, 29)
-    filename = get_letter_pdf_filename(reference='foo', crown=True, sending_date=sending_date, postage=postage)
+    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, dont_use_sending_date=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)
+    created_at = datetime(2017, 12, 4, 17, 31)
-    filename = get_letter_pdf_filename(reference='foo', crown=True, sending_date=sending_date)
+    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'
 
@@ -215,8 +216,8 @@ 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,
-        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)
@@ -242,8 +243,8 @@ 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,
-        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,17 +310,12 @@ 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):
+def test_get_folder_name_in_british_summer_time(notify_api, timestamp, expected_folder_name):
-    with freeze_time(freeze_date):
+    timestamp = dateutil.parser.parse(timestamp)
-        now = datetime.utcnow()
+    folder_name = get_folder_name(created_at=timestamp)
-        folder_name = get_folder_name(_now=now, dont_use_sending_date=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)
-
-
 @mock_s3
 def test_move_sanitised_letter_to_live_pdf_bucket(notify_api, mocker):
     filename = 'my_letter.pdf'