Merge pull request #2619 from alphagov/use-created-at-2

Use created at 2

app/celery/letters_pdf_tasks.py

@@ -253,7 +253,9 @@ def process_virus_scan_passed(self, filename):
         _upload_pdf_to_test_or_live_pdf_bucket(
             new_pdf,
             filename,
-            is_test_letter=is_test_key)
+            is_test_letter=is_test_key,
+            created_at=notification.created_at
+        )
 
         update_letter_pdf_status(
             reference=reference,
@@ -284,10 +286,10 @@ def _move_invalid_letter_and_update_status(notification, filename, scan_pdf_obje
         update_notification_status_by_id(notification.id, NOTIFICATION_TECHNICAL_FAILURE)
 
 
-def _upload_pdf_to_test_or_live_pdf_bucket(pdf_data, filename, is_test_letter):
+def _upload_pdf_to_test_or_live_pdf_bucket(pdf_data, filename, is_test_letter, created_at):
     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(datetime.utcnow(), is_test_letter) + filename
+    target_filename = get_folder_name(created_at, dont_use_sending_date=is_test_letter) + filename
 
     s3upload(
         filedata=pdf_data,

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,
+                                             dont_use_sending_date=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

@@ -25,8 +25,8 @@ LETTERS_PDF_FILE_LOCATION_STRUCTURE = \
 PRECOMPILED_BUCKET_PREFIX = '{folder}NOTIFY.{reference}'
 
 
-def get_folder_name(_now, is_test_or_scan_letter=False):
+def get_folder_name(_now, *, dont_use_sending_date=False):
-    if is_test_or_scan_letter:
+    if dont_use_sending_date:
         folder_name = ''
     else:
         print_datetime = convert_utc_to_bst(_now)
@@ -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, dont_use_sending_date=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, dont_use_sending_date=dont_use_sending_date),
         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, dont_use_sending_date=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,9 +78,10 @@ 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,
-        is_scan_letter=precompiled or notification.key_type == KEY_TYPE_TEST,
+        sending_date=notification.created_at,
+        dont_use_sending_date=precompiled or notification.key_type == KEY_TYPE_TEST,
         postage=notification.postage
     )
 

app/service/send_notification.py

@@ -178,9 +178,10 @@ 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,
-        is_scan_letter=False,
+        sending_date=notification.created_at,
+        dont_use_sending_date=False,
         postage=notification.postage
     )
 

tests/app/celery/test_letters_pdf_tasks.py

@@ -114,11 +114,12 @@ def test_get_letters_pdf_calculates_billing_units(
 
 
 @freeze_time("2017-12-04 17:31:00")
-def test_create_letters_pdf_calls_s3upload(mocker, sample_letter_notification):
+def test_create_letters_pdf_calls_s3upload(mocker, sample_letter_template):
     mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', return_value=(b'\x00\x01', '1'))
     mock_s3 = mocker.patch('app.letters.utils.s3upload')
+    notification = create_notification(template=sample_letter_template, reference='FOO', key_type='normal')
 
-    create_letters_pdf(sample_letter_notification.id)
+    create_letters_pdf(notification.id)
 
     mock_s3.assert_called_with(
         bucket_name=current_app.config['LETTERS_PDF_BUCKET_NAME'],
@@ -129,12 +130,12 @@ def test_create_letters_pdf_calls_s3upload(mocker, sample_letter_notification):
 
 
 @freeze_time("2017-12-04 17:31:00")
-def test_create_letters_pdf_calls_s3upload_for_test_letters(mocker, sample_letter_notification):
+def test_create_letters_pdf_calls_s3upload_for_test_letters(mocker, sample_letter_template):
     mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', return_value=(b'\x00\x01', '1'))
     mock_s3 = mocker.patch('app.letters.utils.s3upload')
-    sample_letter_notification.key_type = 'test'
+    notification = create_notification(template=sample_letter_template, reference='FOO', key_type='test')
 
-    create_letters_pdf(sample_letter_notification.id)
+    create_letters_pdf(notification.id)
 
     mock_s3.assert_called_with(
         bucket_name=current_app.config['TEST_LETTERS_BUCKET_NAME'],

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,26 @@ 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, dont_use_sending_date=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,7 +215,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,
-        is_scan_letter=is_precompiled_letter
+        sending_date=sample_letter_notification.created_at,
+        dont_use_sending_date=is_precompiled_letter
     )
 
     upload_letter_pdf(sample_letter_notification, b'\x00\x01', precompiled=is_precompiled_letter)
@@ -240,7 +242,8 @@ def test_upload_letter_pdf_uses_postage_from_notification(
     filename = get_letter_pdf_filename(
         reference=letter_notification.reference,
         crown=letter_notification.service.crown,
-        is_scan_letter=False,
+        sending_date=letter_notification.created_at,
+        dont_use_sending_date=False,
         postage=letter_notification.postage
     )
 
@@ -327,12 +330,12 @@ def test_copy_redaction_failed_pdf(notify_api):
 def test_get_folder_name_in_british_summer_time(notify_api, freeze_date, expected_folder_name):
     with freeze_time(freeze_date):
         now = datetime.utcnow()
-        folder_name = get_folder_name(_now=now, is_test_or_scan_letter=False)
+        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(), is_test_or_scan_letter=True)
+    assert '' == get_folder_name(datetime.utcnow(), dont_use_sending_date=True)
 
 
 @freeze_time('2017-07-07 20:00:00')