From 5fe0fafadf579dbe9709b8d244f41d1b35286f8d Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 26 Mar 2020 11:38:04 +0000 Subject: [PATCH] =?UTF-8?q?Archive,=20don=E2=80=99t=20delete=20contact=20l?= =?UTF-8?q?ists?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit So we keep a record of who first uploaded a list it’s better to archive a list than completely delete it. The list in the database doesn’t contain any recipient info so this isn’t a change to what data we’re retaining. This means updating the endpoints that get contact lists to exclude ones that are archived. --- app/dao/jobs_dao.py | 8 ------ app/dao/service_contact_list_dao.py | 11 +++++--- app/models.py | 1 + app/service/rest.py | 8 +++--- .../versions/0319_contact_list_archived.py | 26 +++++++++++++++++++ .../app/dao/test_service_contact_list_dao.py | 5 ++++ tests/app/db.py | 4 ++- tests/app/job/test_rest.py | 13 ++++++++-- .../service/test_service_contact_list_rest.py | 26 +++++++++++++++---- 9 files changed, 77 insertions(+), 25 deletions(-) create mode 100644 migrations/versions/0319_contact_list_archived.py diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index 73b6430ec..baff3bbb5 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -135,14 +135,6 @@ def dao_update_job(job): db.session.commit() -def dao_unlink_jobs_from_contact_list_id(contact_list_id): - Job.query.filter( - Job.contact_list_id == contact_list_id, - ).update({ - 'contact_list_id': None, - }) - - def dao_get_jobs_older_than_data_retention(notification_types): flexible_data_retention = ServiceDataRetention.query.filter( ServiceDataRetention.notification_type.in_(notification_types) diff --git a/app/dao/service_contact_list_dao.py b/app/dao/service_contact_list_dao.py index 10045bc80..6a55cf405 100644 --- a/app/dao/service_contact_list_dao.py +++ b/app/dao/service_contact_list_dao.py @@ -5,7 +5,8 @@ from app.models import ServiceContactList def dao_get_contact_list_by_id(service_id, contact_list_id): contact_list = ServiceContactList.query.filter_by( service_id=service_id, - id=contact_list_id + id=contact_list_id, + archived=False, ).one() return contact_list @@ -13,7 +14,8 @@ def dao_get_contact_list_by_id(service_id, contact_list_id): def dao_get_contact_lists(service_id): contact_lists = ServiceContactList.query.filter_by( - service_id=service_id + service_id=service_id, + archived=False, ).order_by( ServiceContactList.created_at.desc() ) @@ -25,6 +27,7 @@ def save_service_contact_list(service_contact_list): db.session.commit() -def dao_delete_contact_list(service_contact_list): - db.session.delete(service_contact_list) +def dao_archive_contact_list(service_contact_list): + service_contact_list.archived = True + db.session.add(service_contact_list) db.session.commit() diff --git a/app/models.py b/app/models.py index 802567046..64beaca52 100644 --- a/app/models.py +++ b/app/models.py @@ -2135,6 +2135,7 @@ class ServiceContactList(db.Model): created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=True) created_at = db.Column(db.DateTime, nullable=False) updated_at = db.Column(db.DateTime, nullable=True, onupdate=datetime.datetime.utcnow) + archived = db.Column(db.Boolean, nullable=False, default=False) def serialize(self): created_at_in_bst = convert_utc_to_bst(self.created_at) diff --git a/app/service/rest.py b/app/service/rest.py index 30487f48d..a1300f462 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -30,7 +30,6 @@ from app.dao.fact_notification_status_dao import ( fetch_stats_for_all_services_by_date_range, fetch_monthly_template_usage_for_service ) from app.dao.inbound_numbers_dao import dao_allocate_number_for_service -from app.dao.jobs_dao import dao_unlink_jobs_from_contact_list_id from app.dao.organisation_dao import dao_get_organisation_by_service_id from app.dao.returned_letters_dao import ( fetch_most_recent_returned_letter, @@ -39,10 +38,10 @@ from app.dao.returned_letters_dao import ( fetch_returned_letters, ) from app.dao.service_contact_list_dao import ( + dao_archive_contact_list, dao_get_contact_lists, - save_service_contact_list, dao_get_contact_list_by_id, - dao_delete_contact_list, + save_service_contact_list, ) from app.dao.service_data_retention_dao import ( fetch_service_data_retention, @@ -1046,8 +1045,7 @@ def delete_contact_list_by_id(service_id, contact_list_id): service_id=service_id, contact_list_id=contact_list_id, ) - dao_unlink_jobs_from_contact_list_id(contact_list.id) - dao_delete_contact_list(contact_list) + dao_archive_contact_list(contact_list) s3.remove_contact_list_from_s3(service_id, contact_list_id) return '', 204 diff --git a/migrations/versions/0319_contact_list_archived.py b/migrations/versions/0319_contact_list_archived.py new file mode 100644 index 000000000..7316651f5 --- /dev/null +++ b/migrations/versions/0319_contact_list_archived.py @@ -0,0 +1,26 @@ +""" + +Revision ID: 0319_contact_list_archived +Revises: 0318_service_contact_list +Create Date: 2020-03-26 11:16:12.389524 + +""" +from alembic import op +import sqlalchemy as sa + +revision = '0319_contact_list_archived' +down_revision = '0318_service_contact_list' + + +def upgrade(): + op.add_column( + 'service_contact_list', + sa.Column('archived', sa.Boolean(), nullable=False, server_default=sa.false()), + ) + + +def downgrade(): + op.drop_column( + 'service_contact_list', + 'archived', + ) diff --git a/tests/app/dao/test_service_contact_list_dao.py b/tests/app/dao/test_service_contact_list_dao.py index df09fe0a4..a38b9923a 100644 --- a/tests/app/dao/test_service_contact_list_dao.py +++ b/tests/app/dao/test_service_contact_list_dao.py @@ -4,6 +4,11 @@ from tests.app.db import create_service_contact_list def test_dao_get_contact_lists(notify_db_session): contact_list = create_service_contact_list() + create_service_contact_list( + service=contact_list.service, + archived=True, + ) + fetched_list = dao_get_contact_lists(contact_list.service_id) assert len(fetched_list) == 1 diff --git a/tests/app/db.py b/tests/app/db.py index 1397fa4d5..fb248a633 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -976,7 +976,8 @@ def create_service_contact_list( original_file_name='EmergencyContactList.xls', row_count=100, template_type='email', - created_by_id=None + created_by_id=None, + archived=False, ): if not service: service = create_service(service_name='service for contact list', user=create_user()) @@ -988,6 +989,7 @@ def create_service_contact_list( template_type=template_type, created_by_id=created_by_id or service.users[0].id, created_at=datetime.utcnow(), + archived=archived, ) db.session.add(contact_list) db.session.commit() diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index 4ddb50d6f..98605b0d3 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -238,12 +238,21 @@ def test_create_scheduled_job(client, sample_template, mocker, fake_uuid): assert resp_json['data']['notification_count'] == 1 -def test_create_job_with_contact_list_id(client, mocker, sample_template, fake_uuid): +@pytest.mark.parametrize('contact_list_archived', ( + True, False, +)) +def test_create_job_with_contact_list_id( + client, + mocker, + sample_template, + fake_uuid, + contact_list_archived, +): mocker.patch('app.celery.tasks.process_job.apply_async') mocker.patch('app.job.rest.get_job_metadata_from_s3', return_value={ 'template_id': str(sample_template.id) }) - contact_list = create_service_contact_list() + contact_list = create_service_contact_list(archived=contact_list_archived) data = { 'id': fake_uuid, 'valid': 'True', diff --git a/tests/app/service/test_service_contact_list_rest.py b/tests/app/service/test_service_contact_list_rest.py index c10c64d08..a71ff574c 100644 --- a/tests/app/service/test_service_contact_list_rest.py +++ b/tests/app/service/test_service_contact_list_rest.py @@ -75,6 +75,7 @@ def test_get_contact_list_returns_for_service(admin_request, notify_db_session): expected_list_2 = create_service_contact_list(service=service_1) # not included in results create_service_contact_list(service=service_2) + create_service_contact_list(service=service_1, archived=True) response = admin_request.get( 'service.get_contact_list', @@ -101,6 +102,16 @@ def test_dao_get_contact_list_by_id(admin_request, sample_service): assert response == expected_list_1.serialize() +def test_dao_get_archived_contact_list_by_id(admin_request): + contact_list = create_service_contact_list(archived=True) + admin_request.get( + 'service.get_contact_list_by_id', + service_id=contact_list.service.id, + contact_list_id=contact_list.id, + _expected_status=404, + ) + + def test_dao_get_contact_list_by_id_does_not_return_if_contact_list_id_for_another_service( admin_request, sample_service ): @@ -120,7 +131,7 @@ 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(mocker, admin_request, sample_service): +def test_archive_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) @@ -137,8 +148,12 @@ def test_dao_delete_contact_list_by_id(mocker, admin_request, sample_service): contact_list_id=expected_list.id, ) - assert job_1.contact_list_id is None + assert job_1.contact_list_id == expected_list.id + assert expected_list.archived is True + assert job_2.contact_list_id == other_list.id + assert other_list.archived is False + assert job_3.contact_list_id is None mock_s3.assert_called_once_with( @@ -147,7 +162,7 @@ def test_dao_delete_contact_list_by_id(mocker, admin_request, sample_service): ) -def test_dao_delete_contact_list_when_unused(mocker, admin_request, sample_service): +def test_archive_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) @@ -160,7 +175,8 @@ def test_dao_delete_contact_list_when_unused(mocker, admin_request, sample_servi contact_list_id=expected_list.id ) - assert ServiceContactList.query.count() == 0 + assert ServiceContactList.query.count() == 1 + assert expected_list.archived is True mock_s3.assert_called_once_with( expected_list.service.id, @@ -168,7 +184,7 @@ def test_dao_delete_contact_list_when_unused(mocker, admin_request, sample_servi ) -def test_dao_delete_contact_list_by_id_for_different_service(mocker, admin_request, sample_service): +def test_archive_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')