diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 8976bbebc..0cd260f35 100644 --- a/app/dao/notifications_dao.py +++ b/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 = LETTERS_PDF_FILE_LOCATION_STRUCTURE.format( - folder=sent_at + "/", - reference=letter.reference, - duplex="D", - letter_class="2", - colour="C", - crown="C" if letter.service.crown else "N", - date='' - ).upper()[:-5] + prefix = get_letter_pdf_filename(reference=letter.reference, + crown=letter.service.crown, + sending_date=letter.created_at, + is_scan_letter=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: try: diff --git a/app/letters/utils.py b/app/letters/utils.py index f97824b9b..48a6f405c 100644 --- a/app/letters/utils.py +++ b/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): - now = datetime.utcnow() - +def get_letter_pdf_filename(reference, crown, sending_date, is_scan_letter=False, postage=SECOND_CLASS): 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 = "{}/".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) + 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, - notification.service.crown, + reference=notification.reference, + crown=notification.service.crown, + sending_date=notification.created_at, is_scan_letter=precompiled or notification.key_type == KEY_TYPE_TEST, postage=notification.postage ) diff --git a/app/service/send_notification.py b/app/service/send_notification.py index 0f660cfe3..7ccb6ca36 100644 --- a/app/service/send_notification.py +++ b/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, - notification.service.crown, + reference=notification.reference, + crown=notification.service.crown, + sending_date=notification.created_at, is_scan_letter=False, postage=notification.postage ) diff --git a/tests/app/dao/notification_dao/test_notification_dao_delete_notifications.py b/tests/app/dao/notification_dao/test_notification_dao_delete_notifications.py index 92e0acebe..a1b80c750 100644 --- a/tests/app/dao/notification_dao/test_notification_dao_delete_notifications.py +++ b/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 diff --git a/tests/app/letters/test_letter_utils.py b/tests/app/letters/test_letter_utils.py index 37a0c1cd0..ea8e926c5 100644 --- a/tests/app/letters/test_letter_utils.py +++ b/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 )