Refactor the code that figures out what folder and filename to use for the letter pdf files.

Now we consistently use the created_at date, so we can always get the right file location and name.

The previous updates to this code were trying to solve the problem if a pdf being created at 17:29, but not ready to upload until 17:31 after the antivirus and validation check.
But in those cases we would have trouble finding the file.

app/dao/notifications_dao.py

@@ -27,7 +27,7 @@ from app import db, create_uuid
 from app.aws.s3 import remove_s3_object, get_s3_bucket_objects
 from app.dao.dao_utils import transactional
 from app.errors import InvalidRequest
-from app.letters.utils import LETTERS_PDF_FILE_LOCATION_STRUCTURE
+from app.letters.utils import get_letter_pdf_filename
 from app.models import (
     Notification,
     NotificationHistory,
@@ -402,17 +402,13 @@ def _delete_letters_from_s3(
     ).limit(query_limit).all()
     for letter in letters_to_delete_from_s3:
         bucket_name = current_app.config['LETTERS_PDF_BUCKET_NAME']
+        # I don't think we need this anymore, we should update the query to get letters sent 7 days ago
         if letter.sent_at:
-            sent_at = str(letter.sent_at.date())
+            prefix = get_letter_pdf_filename(reference=letter.reference,
-            prefix = LETTERS_PDF_FILE_LOCATION_STRUCTURE.format(
+                                             crown=letter.service.crown,
-                folder=sent_at + "/",
+                                             sending_date=letter.created_at,
-                reference=letter.reference,
+                                             is_scan_letter=letter.key_type == KEY_TYPE_TEST,
-                duplex="D",
+                                             postage=letter.postage)
-                letter_class="2",
-                colour="C",
-                crown="C" if letter.service.crown else "N",
-                date=''
-            ).upper()[:-5]
             s3_objects = get_s3_bucket_objects(bucket_name=bucket_name, subfolder=prefix)
             for s3_object in s3_objects:
                 try:

app/letters/utils.py

@@ -36,19 +36,16 @@ def get_folder_name(_now, is_test_or_scan_letter=False):
     return folder_name
 
 
-def get_letter_pdf_filename(reference, crown, is_scan_letter=False, postage=SECOND_CLASS):
+def get_letter_pdf_filename(reference, crown, sending_date, is_scan_letter=False, postage=SECOND_CLASS):
-    now = datetime.utcnow()
-
     upload_file_name = LETTERS_PDF_FILE_LOCATION_STRUCTURE.format(
-        folder=get_folder_name(now, is_scan_letter),
+        folder=get_folder_name(sending_date, is_scan_letter),
         reference=reference,
         duplex="D",
         letter_class=RESOLVE_POSTAGE_FOR_FILE_NAME[postage],
         colour="C",
         crown="C" if crown else "N",
-        date=now.strftime('%Y%m%d%H%M%S')
+        date=sending_date.strftime('%Y%m%d%H%M%S')
     ).upper()
-
     return upload_file_name
 
 
@@ -60,12 +57,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']
-        if notification.sent_at:
+        folder = get_folder_name(notification.created_at, False)
-            folder = "{}/".format(notification.sent_at.date())
-        elif notification.updated_at:
-            folder = get_folder_name(notification.updated_at, False)
-        else:
-            folder = get_folder_name(notification.created_at, False)
 
     upload_file_name = PRECOMPILED_BUCKET_PREFIX.format(
         folder=folder,
@@ -86,8 +78,9 @@ def upload_letter_pdf(notification, pdf_data, precompiled=False):
         notification.id, notification.reference, notification.created_at, len(pdf_data)))
 
     upload_file_name = get_letter_pdf_filename(
-        notification.reference,
+        reference=notification.reference,
-        notification.service.crown,
+        crown=notification.service.crown,
+        sending_date=notification.created_at,
         is_scan_letter=precompiled or notification.key_type == KEY_TYPE_TEST,
         postage=notification.postage
     )

app/service/send_notification.py

@@ -178,8 +178,9 @@ def send_pdf_letter_notification(service_id, post_data):
     )
 
     upload_filename = get_letter_pdf_filename(
-        notification.reference,
+        reference=notification.reference,
-        notification.service.crown,
+        crown=notification.service.crown,
+        sending_date=notification.created_at,
         is_scan_letter=False,
         postage=notification.postage
     )

tests/app/dao/notification_dao/test_notification_dao_delete_notifications.py

@@ -28,7 +28,7 @@ def create_test_data(notification_type, sample_service, days_of_retention=3):
     create_notification(template=email_template, status='delivered')
     create_notification(template=sms_template, status='permanent-failure')
     create_notification(template=letter_template, status='temporary-failure',
-                        reference='LETTER_REF', sent_at=datetime.utcnow())
+                        reference='LETTER_REF', created_at=datetime.utcnow(), sent_at=datetime.utcnow())
     create_notification(template=email_template, status='delivered',
                         created_at=datetime.utcnow() - timedelta(days=4))
     create_notification(template=sms_template, status='permanent-failure',
@@ -127,14 +127,25 @@ def test_delete_notifications_for_days_of_retention(sample_service, notification
     assert Notification.query.count() == 7
     assert Notification.query.filter_by(notification_type=notification_type).count() == 1
     if notification_type == 'letter':
-        mock_get_s3.assert_called_with(bucket_name=current_app.config['LETTERS_PDF_BUCKET_NAME'],
-                                       subfolder="{}/NOTIFY.LETTER_REF.D.2.C.C".format(str(datetime.utcnow().date()))
-                                       )
         assert mock_get_s3.call_count == 2
     else:
         mock_get_s3.assert_not_called()
 
 
+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")
+    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
+                        )
+    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'))
+                                        )
+
+
 def test_delete_notifications_inserts_notification_history(sample_service):
     create_test_data('sms', sample_service)
     assert Notification.query.count() == 9

tests/app/letters/test_letter_utils.py

@@ -55,7 +55,7 @@ def test_get_bucket_name_and_prefix_for_notification_valid_notification(sample_n
     ).upper()
 
 
-def test_get_bucket_name_and_prefix_for_notification_get_from_sent_at_date(sample_notification):
+def test_get_bucket_name_and_prefix_for_notification_is_tomorrow_after_17_30(sample_notification):
     sample_notification.created_at = datetime(2019, 8, 1, 17, 35)
     sample_notification.sent_at = datetime(2019, 8, 2, 17, 45)
 
@@ -68,7 +68,7 @@ def test_get_bucket_name_and_prefix_for_notification_get_from_sent_at_date(sampl
     ).upper()
 
 
-def test_get_bucket_name_and_prefix_for_notification_from_created_at_date(sample_notification):
+def test_get_bucket_name_and_prefix_for_notification_is_today_before_17_30(sample_notification):
     sample_notification.created_at = datetime(2019, 8, 1, 12, 00)
     sample_notification.updated_at = datetime(2019, 8, 2, 12, 00)
     sample_notification.sent_at = datetime(2019, 8, 3, 12, 00)
@@ -77,7 +77,7 @@ def test_get_bucket_name_and_prefix_for_notification_from_created_at_date(sample
 
     assert bucket == current_app.config['LETTERS_PDF_BUCKET_NAME']
     assert bucket_prefix == '{folder}/NOTIFY.{reference}'.format(
-        folder='2019-08-03',
+        folder='2019-08-01',
         reference=sample_notification.reference
     ).upper()
 
@@ -137,10 +137,10 @@ def test_get_bucket_name_and_prefix_for_notification_invalid_notification():
     (True, 'C'),
     (False, 'N'),
 ])
-@freeze_time("2017-12-04 17:29:00")
 def test_get_letter_pdf_filename_returns_correct_filename(
         notify_api, mocker, crown_flag, expected_crown_text):
-    filename = get_letter_pdf_filename(reference='foo', crown=crown_flag)
+    sending_date = datetime(2017, 12, 4, 17, 29)
+    filename = get_letter_pdf_filename(reference='foo', crown=crown_flag, sending_date=sending_date)
 
     assert filename == '2017-12-04/NOTIFY.FOO.D.2.C.{}.20171204172900.PDF'.format(expected_crown_text)
 
@@ -149,25 +149,25 @@ def test_get_letter_pdf_filename_returns_correct_filename(
     ('second', 2),
     ('first', 1),
 ])
-@freeze_time("2017-12-04 17:29:00")
 def test_get_letter_pdf_filename_returns_correct_postage_for_filename(
         notify_api, postage, expected_postage):
-    filename = get_letter_pdf_filename(reference='foo', crown=True, postage=postage)
+    sending_date = datetime(2017, 12, 4, 17, 29)
+    filename = get_letter_pdf_filename(reference='foo', crown=True, sending_date=sending_date, postage=postage)
 
     assert filename == '2017-12-04/NOTIFY.FOO.D.{}.C.C.20171204172900.PDF'.format(expected_postage)
 
 
-@freeze_time("2017-12-04 17:29:00")
 def test_get_letter_pdf_filename_returns_correct_filename_for_test_letters(
         notify_api, mocker):
-    filename = get_letter_pdf_filename(reference='foo', crown='C', is_scan_letter=True)
+    sending_date = datetime(2017, 12, 4, 17, 29)
+    filename = get_letter_pdf_filename(reference='foo', crown='C', sending_date=sending_date, is_scan_letter=True)
 
     assert filename == 'NOTIFY.FOO.D.2.C.C.20171204172900.PDF'
 
 
-@freeze_time("2017-12-04 17:31:00")
 def test_get_letter_pdf_filename_returns_tomorrows_filename(notify_api, mocker):
-    filename = get_letter_pdf_filename(reference='foo', crown=True)
+    sending_date = datetime(2017, 12, 4, 17, 31)
+    filename = get_letter_pdf_filename(reference='foo', crown=True, sending_date=sending_date)
 
     assert filename == '2017-12-05/NOTIFY.FOO.D.2.C.C.20171204173100.PDF'
 
@@ -214,6 +214,7 @@ 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,
         is_scan_letter=is_precompiled_letter
     )
 
@@ -240,6 +241,7 @@ 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,
         is_scan_letter=False,
         postage=letter_notification.postage
     )