From 4a6143aeb1a0dc9151bad705654d4457d04c150d Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 25 Mar 2020 16:36:27 +0000 Subject: [PATCH] =?UTF-8?q?Remove=20the=20list=20from=20S3=20once=20we=20d?= =?UTF-8?q?on=E2=80=99t=20need=20it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Once a contact list is gone from the database there’s no way to reference it again. Any jobs have made their own copy. So we can clean it up, meaning we’re not storing personal data longer than we need to. --- app/aws/s3.py | 11 ++++++++++ app/config.py | 6 +++++ app/service/rest.py | 2 ++ .../service/test_service_contact_list_rest.py | 22 ++++++++++++++++--- 4 files changed, 38 insertions(+), 3 deletions(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index 5c91bf8fc..cfba0f265 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -43,6 +43,13 @@ def get_job_location(service_id, job_id): ) +def get_contact_list_location(service_id, contact_list_id): + return ( + current_app.config['CONTACT_LIST_BUCKET_NAME'], + FILE_LOCATION_STRUCTURE.format(service_id, contact_list_id), + ) + + def get_job_and_metadata_from_s3(service_id, job_id): obj = get_s3_object(*get_job_location(service_id, job_id)) return obj.get()['Body'].read().decode('utf-8'), obj.get()['Metadata'] @@ -62,6 +69,10 @@ def remove_job_from_s3(service_id, job_id): return remove_s3_object(*get_job_location(service_id, job_id)) +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') diff --git a/app/config.py b/app/config.py index 473c92222..2cd8dbcb7 100644 --- a/app/config.py +++ b/app/config.py @@ -364,6 +364,7 @@ class Development(Config): SQLALCHEMY_ECHO = False CSV_UPLOAD_BUCKET_NAME = 'development-notifications-csv-upload' + CONTACT_LIST_BUCKET_NAME = 'development-contact-list' TEST_LETTERS_BUCKET_NAME = 'development-test-letters' DVLA_RESPONSE_BUCKET_NAME = 'notify.tools-ftp' LETTERS_PDF_BUCKET_NAME = 'development-letters-pdf' @@ -405,6 +406,7 @@ class Test(Development): TESTING = True CSV_UPLOAD_BUCKET_NAME = 'test-notifications-csv-upload' + CONTACT_LIST_BUCKET_NAME = 'test-contact-list' TEST_LETTERS_BUCKET_NAME = 'test-test-letters' DVLA_RESPONSE_BUCKET_NAME = 'test.notify.com-ftp' LETTERS_PDF_BUCKET_NAME = 'test-letters-pdf' @@ -440,6 +442,7 @@ class Preview(Config): NOTIFY_EMAIL_DOMAIN = 'notify.works' NOTIFY_ENVIRONMENT = 'preview' CSV_UPLOAD_BUCKET_NAME = 'preview-notifications-csv-upload' + CONTACT_LIST_BUCKET_NAME = 'preview-contact-list' TEST_LETTERS_BUCKET_NAME = 'preview-test-letters' DVLA_RESPONSE_BUCKET_NAME = 'notify.works-ftp' LETTERS_PDF_BUCKET_NAME = 'preview-letters-pdf' @@ -456,6 +459,7 @@ class Staging(Config): NOTIFY_EMAIL_DOMAIN = 'staging-notify.works' NOTIFY_ENVIRONMENT = 'staging' CSV_UPLOAD_BUCKET_NAME = 'staging-notifications-csv-upload' + CONTACT_LIST_BUCKET_NAME = 'staging-contact-list' TEST_LETTERS_BUCKET_NAME = 'staging-test-letters' DVLA_RESPONSE_BUCKET_NAME = 'staging-notify.works-ftp' LETTERS_PDF_BUCKET_NAME = 'staging-letters-pdf' @@ -473,6 +477,7 @@ class Live(Config): NOTIFY_EMAIL_DOMAIN = 'notifications.service.gov.uk' NOTIFY_ENVIRONMENT = 'live' CSV_UPLOAD_BUCKET_NAME = 'live-notifications-csv-upload' + CONTACT_LIST_BUCKET_NAME = 'production-contact-list' TEST_LETTERS_BUCKET_NAME = 'production-test-letters' DVLA_RESPONSE_BUCKET_NAME = 'notifications.service.gov.uk-ftp' LETTERS_PDF_BUCKET_NAME = 'production-letters-pdf' @@ -497,6 +502,7 @@ class Sandbox(CloudFoundryConfig): NOTIFY_EMAIL_DOMAIN = 'notify.works' NOTIFY_ENVIRONMENT = 'sandbox' CSV_UPLOAD_BUCKET_NAME = 'cf-sandbox-notifications-csv-upload' + CONTACT_LIST_BUCKET_NAME = 'cf-sandbox-contact-list' LETTERS_PDF_BUCKET_NAME = 'cf-sandbox-letters-pdf' TEST_LETTERS_BUCKET_NAME = 'cf-sandbox-test-letters' DVLA_RESPONSE_BUCKET_NAME = 'notify.works-ftp' diff --git a/app/service/rest.py b/app/service/rest.py index 6f950f087..30487f48d 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -13,6 +13,7 @@ from sqlalchemy.exc import IntegrityError from sqlalchemy.orm.exc import NoResultFound from app import DATE_FORMAT, DATETIME_FORMAT_NO_TIMEZONE +from app.aws import s3 from app.config import QueueNames from app.dao import fact_notification_status_dao, notifications_dao from app.dao.dao_utils import dao_rollback @@ -1047,6 +1048,7 @@ def delete_contact_list_by_id(service_id, contact_list_id): ) dao_unlink_jobs_from_contact_list_id(contact_list.id) dao_delete_contact_list(contact_list) + s3.remove_contact_list_from_s3(service_id, contact_list_id) return '', 204 diff --git a/tests/app/service/test_service_contact_list_rest.py b/tests/app/service/test_service_contact_list_rest.py index 238c97688..c10c64d08 100644 --- a/tests/app/service/test_service_contact_list_rest.py +++ b/tests/app/service/test_service_contact_list_rest.py @@ -120,7 +120,8 @@ def test_dao_get_contact_list_by_id_does_not_return_if_contact_list_id_for_anoth assert response['message'] == "No result found" -def test_dao_delete_contact_list_by_id(admin_request, sample_service): +def test_dao_delete_contact_list_by_id(mocker, admin_request, sample_service): + mock_s3 = mocker.patch('app.service.rest.s3.remove_contact_list_from_s3') service_1 = create_service(service_name='Service under test') template_1 = create_template(service=service_1) expected_list = create_service_contact_list(service=service_1) @@ -140,8 +141,14 @@ def test_dao_delete_contact_list_by_id(admin_request, sample_service): assert job_2.contact_list_id == other_list.id assert job_3.contact_list_id is None + mock_s3.assert_called_once_with( + expected_list.service.id, + expected_list.id, + ) -def test_dao_delete_contact_list_when_unused(admin_request, sample_service): + +def test_dao_delete_contact_list_when_unused(mocker, admin_request, sample_service): + mock_s3 = mocker.patch('app.service.rest.s3.remove_contact_list_from_s3') service = create_service(service_name='Service under test') expected_list = create_service_contact_list(service=service) @@ -155,8 +162,16 @@ def test_dao_delete_contact_list_when_unused(admin_request, sample_service): assert ServiceContactList.query.count() == 0 + mock_s3.assert_called_once_with( + expected_list.service.id, + expected_list.id, + ) + + +def test_dao_delete_contact_list_by_id_for_different_service(mocker, admin_request, sample_service): + + mock_s3 = mocker.patch('app.service.rest.s3.remove_contact_list_from_s3') -def test_dao_delete_contact_list_by_id_for_different_service(admin_request, sample_service): service_1 = create_service(service_name='Service under test') service_2 = create_service(service_name='Other service') @@ -171,3 +186,4 @@ def test_dao_delete_contact_list_by_id_for_different_service(admin_request, samp ) assert ServiceContactList.query.count() == 1 + assert mock_s3.called is False