From ff7eebc90a98b965c4ea2d8faeb7b5e358cdd122 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 9 Mar 2021 16:44:31 +0000 Subject: [PATCH] Simplify deleting old letters Previously we made a call to S3 to list objects for a letter, even though we already had the precise key of the single object to hand. This removes the one usage of "get_s3_bucket_objects" and uses the filename directly in the call to remove the object. --- app/aws/s3.py | 16 -------- app/dao/notifications_dao.py | 16 ++------ tests/app/aws/test_s3.py | 40 +------------------ ...t_notification_dao_delete_notifications.py | 19 ++++----- 4 files changed, 13 insertions(+), 78 deletions(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index 5957ff870..a85131f25 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -69,22 +69,6 @@ def remove_contact_list_from_s3(service_id, contact_list_id): return remove_s3_object(*get_contact_list_location(service_id, contact_list_id)) -def get_s3_bucket_objects(bucket_name, subfolder=''): - boto_client = client('s3', current_app.config['AWS_REGION']) - paginator = boto_client.get_paginator('list_objects_v2') - page_iterator = paginator.paginate( - Bucket=bucket_name, - Prefix=subfolder - ) - - all_objects_in_bucket = [] - for page in page_iterator: - if page.get('Contents'): - all_objects_in_bucket.extend(page['Contents']) - - return all_objects_in_bucket - - def remove_s3_object(bucket_name, object_key): obj = get_s3_object(bucket_name, object_key) return obj.delete() diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index ce45cc46a..95e5ad1d1 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -22,7 +22,7 @@ from sqlalchemy.sql.expression import case from werkzeug.datastructures import MultiDict from app import create_uuid, db -from app.aws.s3 import get_s3_bucket_objects, remove_s3_object +from app.aws.s3 import remove_s3_object from app.clients.sms.firetext import ( get_message_status_and_reason_from_firetext_code, ) @@ -454,19 +454,11 @@ def _delete_letters_from_s3( ).limit(query_limit).all() for letter in letters_to_delete_from_s3: try: - prefix = find_letter_pdf_filename(letter) - except LetterPDFNotFound: + filename = find_letter_pdf_filename(letter) + remove_s3_object(bucket_name, filename) + except (ClientError, LetterPDFNotFound): current_app.logger.exception( "Could not delete S3 object for letter: {}".format(letter.id)) - continue - - s3_objects = get_s3_bucket_objects(bucket_name=bucket_name, subfolder=prefix) - for s3_object in s3_objects: - try: - remove_s3_object(bucket_name, s3_object['Key']) - except ClientError: - current_app.logger.exception( - "Could not delete S3 object with filename: {}".format(s3_object['Key'])) @transactional diff --git a/tests/app/aws/test_s3.py b/tests/app/aws/test_s3.py index cbb4fbe9f..5588e4a46 100644 --- a/tests/app/aws/test_s3.py +++ b/tests/app/aws/test_s3.py @@ -1,15 +1,10 @@ from datetime import datetime, timedelta -from unittest.mock import call import pytest import pytz from freezegun import freeze_time -from app.aws.s3 import ( - get_list_of_files_by_suffix, - get_s3_bucket_objects, - get_s3_file, -) +from app.aws.s3 import get_list_of_files_by_suffix, get_s3_file from tests.app.conftest import datetime_in_past @@ -31,39 +26,6 @@ def test_get_s3_file_makes_correct_call(notify_api, mocker): ) -def test_get_s3_bucket_objects_make_correct_pagination_call(notify_api, mocker): - paginator_mock = mocker.patch('app.aws.s3.client') - - get_s3_bucket_objects('foo-bucket', subfolder='bar') - - paginator_mock.assert_has_calls([ - call().get_paginator().paginate(Bucket='foo-bucket', Prefix='bar') - ]) - - -def test_get_s3_bucket_objects_builds_objects_list_from_paginator(notify_api, mocker): - AFTER_SEVEN_DAYS = datetime_in_past(days=8) - paginator_mock = mocker.patch('app.aws.s3.client') - multiple_pages_s3_object = [ - { - "Contents": [ - single_s3_object_stub('bar/foo.txt', AFTER_SEVEN_DAYS), - ] - }, - { - "Contents": [ - single_s3_object_stub('bar/foo1.txt', AFTER_SEVEN_DAYS), - ] - } - ] - paginator_mock.return_value.get_paginator.return_value.paginate.return_value = multiple_pages_s3_object - - bucket_objects = get_s3_bucket_objects('foo-bucket', subfolder='bar') - - assert len(bucket_objects) == 2 - assert set(bucket_objects[0].keys()) == set(['ETag', 'Key', 'LastModified']) - - @freeze_time("2018-01-11 00:00:00") @pytest.mark.parametrize('suffix_str, days_before, returned_no', [ ('.ACK.txt', None, 1), 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 be636cda1..1ac06f6e2 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 @@ -73,7 +73,7 @@ def test_should_delete_notifications_by_type_after_seven_days( expected_letter_count ): mocker.patch("app.dao.notifications_dao.find_letter_pdf_filename") - mocker.patch("app.dao.notifications_dao.get_s3_bucket_objects") + mocker.patch("app.dao.notifications_dao.remove_s3_object") email_template, letter_template, sms_template = _create_templates(sample_service) # create one notification a day between 1st and 10th from 11:00 to 19:00 of each type for i in range(1, 11): @@ -107,7 +107,6 @@ def test_should_delete_notifications_by_type_after_seven_days( @freeze_time("2016-01-10 12:00:00.000000") def test_should_not_delete_notification_history(sample_service, mocker): - mocker.patch("app.dao.notifications_dao.get_s3_bucket_objects") with freeze_time('2016-01-01 12:00'): email_template, letter_template, sms_template = _create_templates(sample_service) create_notification(template=email_template, status='permanent-failure') @@ -122,16 +121,16 @@ def test_should_not_delete_notification_history(sample_service, mocker): @pytest.mark.parametrize('notification_type', ['sms', 'email', 'letter']) def test_delete_notifications_for_days_of_retention(sample_service, notification_type, mocker): mocker.patch('app.dao.notifications_dao.find_letter_pdf_filename') - mock_get_s3 = mocker.patch("app.dao.notifications_dao.get_s3_bucket_objects") + mock_remove_s3 = mocker.patch("app.dao.notifications_dao.remove_s3_object") create_test_data(notification_type, sample_service) assert Notification.query.count() == 9 delete_notifications_older_than_retention_by_type(notification_type) assert Notification.query.count() == 7 assert Notification.query.filter_by(notification_type=notification_type).count() == 1 if notification_type == 'letter': - assert mock_get_s3.call_count == 2 + assert mock_remove_s3.call_count == 2 else: - mock_get_s3.assert_not_called() + mock_remove_s3.assert_not_called() @mock_s3 @@ -171,7 +170,6 @@ def test_delete_notifications_inserts_notification_history(sample_service): def test_delete_notifications_does_nothing_if_notification_history_row_already_exists( sample_email_template, mocker ): - mocker.patch("app.dao.notifications_dao.get_s3_bucket_objects") notification = create_notification( template=sample_email_template, created_at=datetime.utcnow() - timedelta(days=8), status='temporary-failure' @@ -197,7 +195,6 @@ def test_delete_notifications_keep_data_for_days_of_retention_is_longer(sample_s def test_delete_notifications_with_test_keys(sample_template, mocker): - mocker.patch("app.dao.notifications_dao.get_s3_bucket_objects") create_notification(template=sample_template, key_type='test', created_at=datetime.utcnow() - timedelta(days=8)) delete_notifications_older_than_retention_by_type('sms') assert Notification.query.count() == 0 @@ -230,7 +227,7 @@ def test_delete_notifications_delete_notification_type_for_default_time_if_no_da def test_delete_notifications_deletes_letters_not_sent_and_in_final_state_from_table_but_not_s3( sample_service, mocker, notification_status ): - mock_get_s3 = mocker.patch("app.dao.notifications_dao.get_s3_bucket_objects") + mock_remove_s3 = mocker.patch("app.dao.notifications_dao.remove_s3_object") letter_template = create_template(service=sample_service, template_type='letter') create_notification( template=letter_template, @@ -245,7 +242,7 @@ def test_delete_notifications_deletes_letters_not_sent_and_in_final_state_from_t assert Notification.query.count() == 0 assert NotificationHistory.query.count() == 1 - mock_get_s3.assert_not_called() + mock_remove_s3.assert_not_called() @mock_s3 @@ -292,7 +289,7 @@ def test_delete_notifications_deletes_letters_sent_and_in_final_state_from_table def test_delete_notifications_does_not_delete_letters_not_yet_in_final_state( sample_service, mocker, notification_status ): - mock_get_s3 = mocker.patch("app.dao.notifications_dao.get_s3_bucket_objects") + mock_remove_s3 = mocker.patch("app.dao.notifications_dao.remove_s3_object") letter_template = create_template(service=sample_service, template_type='letter') create_notification( template=letter_template, @@ -307,7 +304,7 @@ def test_delete_notifications_does_not_delete_letters_not_yet_in_final_state( assert Notification.query.count() == 1 assert NotificationHistory.query.count() == 0 - mock_get_s3.assert_not_called() + mock_remove_s3.assert_not_called() @freeze_time('2020-03-25 00:01')