Merge pull request #2759 from alphagov/delete-contact-list

Add an endpoint to delete a contact list

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')

app/config.py

@@ -363,6 +363,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'
@@ -406,6 +407,7 @@ class Test(Development):
     HIGH_VOLUME_SERVICE = ['941b6f9a-50d7-4742-8d50-f365ca74bf27']
 
     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'
@@ -441,6 +443,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'
@@ -457,6 +460,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'
@@ -474,6 +478,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'
@@ -498,6 +503,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'

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()
     )
@@ -23,3 +25,9 @@ def dao_get_contact_lists(service_id):
 def save_service_contact_list(service_contact_list):
     db.session.add(service_contact_list)
     db.session.commit()
+
+
+def dao_archive_contact_list(service_contact_list):
+    service_contact_list.archived = True
+    db.session.add(service_contact_list)
+    db.session.commit()

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)

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
@@ -36,8 +37,12 @@ from app.dao.returned_letters_dao import (
     fetch_returned_letter_summary,
     fetch_returned_letters,
 )
-from app.dao.service_contact_list_dao import dao_get_contact_lists, save_service_contact_list, \
-    dao_get_contact_list_by_id
+from app.dao.service_contact_list_dao import (
+    dao_archive_contact_list,
+    dao_get_contact_lists,
+    dao_get_contact_list_by_id,
+    save_service_contact_list,
+)
 from app.dao.service_data_retention_dao import (
     fetch_service_data_retention,
     fetch_service_data_retention_by_id,
@@ -1034,6 +1039,18 @@ def get_contact_list_by_id(service_id, contact_list_id):
     return jsonify(contact_list.serialize())
 
 
+@service_blueprint.route('/<uuid:service_id>/contact-list/<uuid:contact_list_id>', methods=['DELETE'])
+def delete_contact_list_by_id(service_id, contact_list_id):
+    contact_list = dao_get_contact_list_by_id(
+        service_id=service_id,
+        contact_list_id=contact_list_id,
+    )
+    dao_archive_contact_list(contact_list)
+    s3.remove_contact_list_from_s3(service_id, contact_list_id)
+
+    return '', 204
+
+
 @service_blueprint.route('/<uuid:service_id>/contact-list', methods=['POST'])
 def create_contact_list(service_id):
     service_contact_list = validate(request.get_json(), create_service_contact_list_schema)

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',
+    )

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

tests/app/db.py

@@ -384,7 +384,8 @@ def create_job(
         processing_started=None,
         processing_finished=None,
         original_file_name='some.csv',
-        archived=False
+        archived=False,
+        contact_list_id=None,
 ):
     data = {
         'id': uuid.uuid4(),
@@ -400,7 +401,8 @@ def create_job(
         'scheduled_for': scheduled_for,
         'processing_started': processing_started,
         'processing_finished': processing_finished,
-        'archived': archived
+        'archived': archived,
+        'contact_list_id': contact_list_id,
     }
     job = Job(**data)
     dao_create_job(job)
@@ -974,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())
@@ -986,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()

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',

tests/app/service/test_service_contact_list_rest.py

@@ -1,7 +1,12 @@
 import uuid
 
 from app.models import ServiceContactList
-from tests.app.db import create_service_contact_list, create_service
+from tests.app.db import (
+    create_job,
+    create_service_contact_list,
+    create_service,
+    create_template,
+)
 
 
 def test_create_service_contact_list(sample_service, admin_request):
@@ -70,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',
@@ -96,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
 ):
@@ -113,3 +129,79 @@ 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_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)
+    expected_list = create_service_contact_list(service=service_1)
+    other_list = create_service_contact_list(service=service_1)
+
+    # Job linked to the contact list we’re deleting
+    job_1 = create_job(template=template_1, contact_list_id=expected_list.id)
+    # Other jobs and lists shouldn’t be affected
+    job_2 = create_job(template=template_1, contact_list_id=other_list.id)
+    job_3 = create_job(template=template_1)
+
+    admin_request.delete(
+        'service.delete_contact_list_by_id',
+        service_id=service_1.id,
+        contact_list_id=expected_list.id,
+    )
+
+    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(
+        expected_list.service.id,
+        expected_list.id,
+    )
+
+
+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)
+
+    assert ServiceContactList.query.count() == 1
+
+    admin_request.delete(
+        'service.delete_contact_list_by_id',
+        service_id=service.id,
+        contact_list_id=expected_list.id
+    )
+
+    assert ServiceContactList.query.count() == 1
+    assert expected_list.archived is True
+
+    mock_s3.assert_called_once_with(
+        expected_list.service.id,
+        expected_list.id,
+    )
+
+
+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')
+
+    service_1 = create_service(service_name='Service under test')
+    service_2 = create_service(service_name='Other service')
+
+    contact_list = create_service_contact_list(service=service_1)
+    assert ServiceContactList.query.count() == 1
+
+    admin_request.delete(
+        'service.delete_contact_list_by_id',
+        service_id=service_2.id,
+        contact_list_id=contact_list.id,
+        _expected_status=404,
+    )
+
+    assert ServiceContactList.query.count() == 1
+    assert mock_s3.called is False