From de9ae08ecc9653e9728538eda26739b864a83c20 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 20 Dec 2021 12:05:53 +0000 Subject: [PATCH] Downgrade log for letter deletion exceptions If the S3 object is missing [1], then that's what we want, so we don't need such a severe log for it, but we still want to know as it's not expected. This is separate to more general "ClientError" exceptions, which could mean anything. There weren't any tests to cover missing S3 objects, so I've added one. I don't think we need a test for ClientErrors: - If there was no handler, the task would fail and we'd learn about it that way. - The scope of the calling task is now much smaller, so it matters less than it used to [2]. [1]: https://github.com/alphagov/notifications-api/blob/81a79e56ced891ad58901ea61ce707e907de3363/app/letters/utils.py#L52 [2]: https://github.com/alphagov/notifications-api/commit/f965322f25e37f8125463223eb5b2473e246e8d2 --- app/dao/notifications_dao.py | 7 +++++-- ...est_notification_dao_delete_notifications.py | 17 +++++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index eb1561e5b..6b4c3baac 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -428,9 +428,12 @@ def _delete_letters_from_s3( try: letter_pdf = find_letter_pdf_in_s3(letter) letter_pdf.delete() - except (ClientError, LetterPDFNotFound): + except ClientError: current_app.logger.exception( - "Could not delete S3 object for letter: {}".format(letter.id)) + "Error deleting S3 object for letter: {}".format(letter.id)) + except LetterPDFNotFound: + current_app.logger.warning( + "No S3 object to delete for letter: {}".format(letter.id)) @autocommit 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 b1f5f09bc..a616dee31 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 @@ -51,6 +51,23 @@ def test_move_notifications_deletes_letters_from_s3(sample_letter_template, mock s3.get_object(Bucket=bucket_name, Key=filename) +@mock_s3 +@freeze_time('2019-09-01 04:30') +def test_move_notifications_copes_if_letter_not_in_s3(sample_letter_template, mocker): + s3 = boto3.client('s3', region_name='eu-west-1') + s3.create_bucket( + Bucket=current_app.config['LETTERS_PDF_BUCKET_NAME'], + CreateBucketConfiguration={'LocationConstraint': 'eu-west-1'} + ) + + eight_days_ago = datetime.utcnow() - timedelta(days=8) + create_notification(template=sample_letter_template, status='delivered', sent_at=eight_days_ago) + + move_notifications_to_notification_history('letter', sample_letter_template.service_id, datetime(2020, 1, 2)) + assert Notification.query.count() == 0 + assert NotificationHistory.query.count() == 1 + + def test_move_notifications_does_nothing_if_notification_history_row_already_exists( sample_email_template, mocker ):