Merge pull request #2554 from alphagov/dissociate-default-template-replies

Dissociate letter contact blocks before archiving

app/dao/service_letter_contact_dao.py

@@ -2,8 +2,6 @@ from sqlalchemy import desc
 
 from app import db
 from app.dao.dao_utils import transactional
-from app.errors import InvalidRequest
-from app.exceptions import ArchiveValidationError
 from app.models import ServiceLetterContact, Template
 
 
@@ -37,8 +35,6 @@ def add_letter_contact_for_service(service_id, contact_block, is_default):
     old_default = _get_existing_default(service_id)
     if is_default:
         _reset_old_default_to_false(old_default)
-    else:
-        _raise_when_no_default(old_default)
 
     new_letter_contact = ServiceLetterContact(
         service_id=service_id,
@@ -55,9 +51,6 @@ def update_letter_contact(service_id, letter_contact_id, contact_block, is_defau
     # if we want to make this the default, ensure there are no other existing defaults
     if is_default:
         _reset_old_default_to_false(old_default)
-    else:
-        if old_default.id == letter_contact_id:
-            raise InvalidRequest("You must have at least one letter contact as the default.", 400)
 
     letter_contact_update = ServiceLetterContact.query.get(letter_contact_id)
     letter_contact_update.contact_block = contact_block
@@ -73,10 +66,11 @@ def archive_letter_contact(service_id, letter_contact_id):
         service_id=service_id
     ).one()
 
-    if _is_template_default(letter_contact_id):
-        raise ArchiveValidationError("You cannot delete the default letter contact block for a template")
-    if letter_contact_to_archive.is_default:
-        raise ArchiveValidationError("You cannot delete a default letter contact block")
+    Template.query.filter_by(
+        service_letter_contact_id=letter_contact_id
+    ).update({
+        'service_letter_contact_id': None
+    })
 
     letter_contact_to_archive.archived = True
 
@@ -84,37 +78,28 @@ def archive_letter_contact(service_id, letter_contact_id):
     return letter_contact_to_archive
 
 
-def _is_template_default(letter_contact_id):
-    template_defaults = Template.query.filter_by(
-        service_letter_contact_id=letter_contact_id
-    ).all()
-
-    return any(template_defaults)
-
-
 def _get_existing_default(service_id):
-    letter_contacts = dao_get_letter_contacts_by_service_id(service_id=service_id)
-    if letter_contacts:
-        old_default = [x for x in letter_contacts if x.is_default]
-        if len(old_default) == 1:
-            return old_default[0]
-        else:
-            raise Exception(
-                "There should only be one default letter contact for each service. Service {} has {}".format(
-                    service_id,
-                    len(old_default)
-                )
-            )
-    return None
+    old_defaults = [
+        x for x
+        in dao_get_letter_contacts_by_service_id(service_id=service_id)
+        if x.is_default
+    ]
+
+    if len(old_defaults) == 0:
+        return None
+
+    if len(old_defaults) == 1:
+        return old_defaults[0]
+
+    raise Exception(
+        "There should only be one default letter contact for each service. Service {} has {}".format(
+            service_id,
+            len(old_defaults)
+        )
+    )
 
 
 def _reset_old_default_to_false(old_default):
     if old_default:
         old_default.is_default = False
         db.session.add(old_default)
-
-
-def _raise_when_no_default(old_default):
-    # check that the update is not updating the only default to false
-    if not old_default:
-        raise InvalidRequest("You must have at least one letter contact as the default.", 400)

tests/app/dao/test_service_letter_contact_dao.py

@@ -9,8 +9,6 @@ from app.dao.service_letter_contact_dao import (
     dao_get_letter_contact_by_id,
     update_letter_contact
 )
-from app.errors import InvalidRequest
-from app.exceptions import ArchiveValidationError
 from app.models import ServiceLetterContact
 from tests.app.db import create_letter_contact, create_service, create_template
 
@@ -99,14 +97,14 @@ def test_add_letter_contact_does_not_override_default(notify_db_session):
     assert not results[1].is_default
 
 
-def test_add_letter_contact_with_no_default_raises_exception(notify_db_session):
+def test_add_letter_contact_with_no_default_is_fine(notify_db_session):
     service = create_service()
-    with pytest.raises(expected_exception=InvalidRequest):
-        add_letter_contact_for_service(
-            service_id=service.id,
-            contact_block='Swansea, SN1 3CC',
-            is_default=False
-        )
+    letter_contact = add_letter_contact_for_service(
+        service_id=service.id,
+        contact_block='Swansea, SN1 3CC',
+        is_default=False
+    )
+    assert service.letter_contacts == [letter_contact]
 
 
 def test_add_letter_contact_when_multiple_defaults_exist_raises_exception(notify_db_session):
@@ -159,17 +157,16 @@ def test_update_letter_contact_as_default_overides_existing_default(notify_db_se
     assert not results[1].is_default
 
 
-def test_update_letter_contact_unset_default_for_only_letter_contact_raises_exception(notify_db_session):
+def test_update_letter_contact_unset_default_for_only_letter_contact_is_fine(notify_db_session):
     service = create_service()
     only_letter_contact = create_letter_contact(service=service, contact_block='Aberdeen, AB12 23X')
-
-    with pytest.raises(expected_exception=InvalidRequest):
-        update_letter_contact(
-            service_id=service.id,
-            letter_contact_id=only_letter_contact.id,
-            contact_block='Warwick, W14 TSR',
-            is_default=False
-        )
+    update_letter_contact(
+        service_id=service.id,
+        letter_contact_id=only_letter_contact.id,
+        contact_block='Warwick, W14 TSR',
+        is_default=False
+    )
+    assert only_letter_contact.is_default is False
 
 
 def test_archive_letter_contact(notify_db_session):
@@ -199,26 +196,29 @@ def test_archive_letter_contact_does_not_archive_a_letter_contact_for_a_differen
     assert not letter_contact.archived
 
 
-def test_archive_letter_contact_does_not_archive_a_service_default_letter_contact(notify_db_session):
+def test_archive_letter_contact_can_archive_a_service_default_letter_contact(notify_db_session):
     service = create_service()
     letter_contact = create_letter_contact(service=service, contact_block='Edinburgh, ED1 1AA')
-
-    with pytest.raises(ArchiveValidationError) as e:
-        archive_letter_contact(service.id, letter_contact.id)
-
-    assert 'You cannot delete a default letter contact block' in str(e.value)
+    archive_letter_contact(service.id, letter_contact.id)
+    assert letter_contact.archived is True
 
 
-def test_archive_letter_contact_does_not_archive_a_template_default_letter_contact(notify_db_session):
+def test_archive_letter_contact_does_dissociates_template_defaults_before_archiving(notify_db_session):
     service = create_service()
     create_letter_contact(service=service, contact_block='Edinburgh, ED1 1AA')
     template_default = create_letter_contact(service=service, contact_block='Aberdeen, AB12 23X', is_default=False)
-    create_template(service=service, template_type='letter', reply_to=template_default.id)
+    associated_template_1 = create_template(service=service, template_type='letter', reply_to=template_default.id)
+    associated_template_2 = create_template(service=service, template_type='letter', reply_to=template_default.id)
 
-    with pytest.raises(ArchiveValidationError) as e:
-        archive_letter_contact(service.id, template_default.id)
+    assert associated_template_1.reply_to == template_default.id
+    assert associated_template_2.reply_to == template_default.id
+    assert template_default.archived is False
 
-    assert 'You cannot delete the default letter contact block for a template' in str(e.value)
+    archive_letter_contact(service.id, template_default.id)
+
+    assert associated_template_1.reply_to is None
+    assert associated_template_2.reply_to is None
+    assert template_default.archived is True
 
 
 def test_dao_get_letter_contact_by_id(sample_service):

tests/app/service/test_rest.py

@@ -2829,14 +2829,12 @@ def test_add_service_letter_contact_can_add_multiple_addresses(client, sample_se
     assert first_letter_contact_not_default[0].contact_block == 'London, E1 8QS'
 
 
-def test_add_service_letter_contact_block_raise_exception_if_no_default(client, sample_service):
+def test_add_service_letter_contact_block_fine_if_no_default(client, sample_service):
     data = json.dumps({"contact_block": "London, E1 8QS", "is_default": False})
     response = client.post('/service/{}/letter-contact'.format(sample_service.id),
                            data=data,
                            headers=[('Content-Type', 'application/json'), create_authorization_header()])
-    assert response.status_code == 400
-    json_resp = json.loads(response.get_data(as_text=True))
-    assert json_resp['message'] == 'You must have at least one letter contact as the default.'
+    assert response.status_code == 201
 
 
 def test_add_service_letter_contact_block_404s_when_invalid_service_id(client, notify_db, notify_db_session):
@@ -2864,16 +2862,13 @@ def test_update_service_letter_contact(client, sample_service):
     assert json_resp['data'] == results[0].serialize()
 
 
-def test_update_service_letter_contact_returns_400_when_no_default(client, sample_service):
+def test_update_service_letter_contact_returns_200_when_no_default(client, sample_service):
     original_reply_to = create_letter_contact(service=sample_service, contact_block="Aberdeen, AB23 1XH")
     data = json.dumps({"contact_block": "London, E1 8QS", "is_default": False})
     response = client.post('/service/{}/letter-contact/{}'.format(sample_service.id, original_reply_to.id),
                            data=data,
                            headers=[('Content-Type', 'application/json'), create_authorization_header()])
-
-    assert response.status_code == 400
-    json_resp = json.loads(response.get_data(as_text=True))
-    assert json_resp['message'] == 'You must have at least one letter contact as the default.'
+    assert response.status_code == 200
 
 
 def test_update_service_letter_contact_returns_404_when_invalid_service_id(client, notify_db, notify_db_session):
@@ -2901,7 +2896,7 @@ def test_delete_service_letter_contact_can_archive_letter_contact(admin_request,
     assert letter_contact.archived is True
 
 
-def test_delete_service_letter_contact_returns_400_if_archiving_template_default(admin_request, notify_db_session):
+def test_delete_service_letter_contact_returns_200_if_archiving_template_default(admin_request, notify_db_session):
     service = create_service()
     create_letter_contact(service=service, contact_block='Edinburgh, ED1 1AA')
     letter_contact = create_letter_contact(service=service, contact_block='Swansea, SN1 3CC', is_default=False)
@@ -2911,12 +2906,9 @@ def test_delete_service_letter_contact_returns_400_if_archiving_template_default
         'service.delete_service_letter_contact',
         service_id=service.id,
         letter_contact_id=letter_contact.id,
-        _expected_status=400
+        _expected_status=200
     )
-    assert response == {
-        'message': 'You cannot delete the default letter contact block for a template',
-        'result': 'error'}
-    assert letter_contact.archived is False
+    assert response['data']['archived'] is True
 
 
 def test_add_service_sms_sender_can_add_multiple_senders(client, notify_db_session):