From fa3f4f3c2079c6c8ef02ee6f4947acaa39a8d393 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 25 Sep 2017 12:26:29 +0100 Subject: [PATCH 1/3] Add dao to store letter contacts and upsert --- app/dao/service_letter_contact_dao.py | 45 +++++++++++++ app/service/rest.py | 9 +-- .../dao/test_service_letter_contact_dao.py | 66 +++++++++++++++++++ 3 files changed, 116 insertions(+), 4 deletions(-) create mode 100644 app/dao/service_letter_contact_dao.py create mode 100644 tests/app/dao/test_service_letter_contact_dao.py diff --git a/app/dao/service_letter_contact_dao.py b/app/dao/service_letter_contact_dao.py new file mode 100644 index 000000000..578483f62 --- /dev/null +++ b/app/dao/service_letter_contact_dao.py @@ -0,0 +1,45 @@ +from app import db +from app.dao.dao_utils import transactional +from app.errors import InvalidRequest +from app.models import ServiceLetterContact + + +def dao_get_letter_contacts_by_service_id(service_id): + letter_contacts = db.session.query( + ServiceLetterContact + ).filter( + ServiceLetterContact.service_id == service_id + ).order_by( + ServiceLetterContact.created_at + ).all() + + return letter_contacts + + +def create_or_update_letter_contact(service_id, contact_block): + letter_contacts = dao_get_letter_contacts_by_service_id(service_id) + if len(letter_contacts) == 0: + letter_contact = ServiceLetterContact( + service_id=service_id, + contact_block=contact_block + ) + dao_create_letter_contact(letter_contact) + elif len(letter_contacts) == 1: + letter_contacts[0].contact_block = contact_block + dao_update_letter_contact(letter_contacts[0]) + else: + # Once we move allowing letter contact blocks, this method will be removed + raise InvalidRequest( + "Multiple letter contacts were found, this method should not be used.", + status_code=500 + ) + + +@transactional +def dao_create_letter_contact(letter_contact): + db.session.add(letter_contact) + + +@transactional +def dao_update_letter_contact(letter_contact): + db.session.add(letter_contact) diff --git a/app/service/rest.py b/app/service/rest.py index 48f1e6f44..909675df5 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -140,15 +140,16 @@ def update_service(service_id): # Capture the status change here as Marshmallow changes this later service_going_live = fetched_service.restricted and not req_json.get('restricted', True) - if 'reply_to_email_address' in req_json: - create_or_update_email_reply_to(fetched_service.id, req_json['reply_to_email_address']) - current_data = dict(service_schema.dump(fetched_service).data.items()) current_data.update(request.get_json()) update_dict = service_schema.load(current_data).data + dao_update_service(update_dict) + + if 'reply_to_email_address' in req_json: + create_or_update_email_reply_to(fetched_service.id, req_json['reply_to_email_address']) + if 'sms_sender' in req_json: insert_or_update_service_sms_sender(fetched_service, req_json['sms_sender']) - dao_update_service(update_dict) if service_going_live: send_notification_to_service_users( diff --git a/tests/app/dao/test_service_letter_contact_dao.py b/tests/app/dao/test_service_letter_contact_dao.py new file mode 100644 index 000000000..770fdfa16 --- /dev/null +++ b/tests/app/dao/test_service_letter_contact_dao.py @@ -0,0 +1,66 @@ +import pytest + +from app.dao.service_letter_contact_dao import ( + create_or_update_letter_contact, + dao_get_letter_contacts_by_service_id, +) +from app.errors import InvalidRequest +from app.models import ServiceLetterContact +from tests.app.db import create_letter_contact, create_service + + +def test_dao_get_letter_contacts_by_service_id(notify_db_session): + service = create_service() + default_letter_contact = create_letter_contact(service=service, contact_block='Edinburgh, ED1 1AA') + another_letter_contact = create_letter_contact(service=service, contact_block='Cardiff, CA1 2DB') + + results = dao_get_letter_contacts_by_service_id(service_id=service.id) + + assert len(results) == 2 + assert default_letter_contact in results + assert another_letter_contact in results + + +def test_create_or_update_letter_contact_creates_new_entry(notify_db_session): + service = create_service() + + create_or_update_letter_contact(service.id, 'Cardiff, CA1 2DB') + + letter_contacts = dao_get_letter_contacts_by_service_id(service.id) + + assert ServiceLetterContact.query.count() == 1 + assert letter_contacts[0].service.id == service.id + assert letter_contacts[0].contact_block == 'Cardiff, CA1 2DB' + + +def test_create_or_update_letter_contact_does_not_create_another_entry(notify_db_session): + service = create_service() + create_letter_contact(service, 'London, NW1 2DB') + create_or_update_letter_contact(service.id, 'Bristol, BR1 2DB') + + letter_contacts = dao_get_letter_contacts_by_service_id(service.id) + + assert len(letter_contacts) == 1 + + +def test_create_or_update_letter_contact_updates_existing_entry(notify_db_session): + service = create_service() + create_letter_contact(service, 'London, NW1 2DB') + + create_or_update_letter_contact(service.id, 'Bristol, BR1 2DB') + + letter_contact = dao_get_letter_contacts_by_service_id(service.id) + + assert len(letter_contact) == 1 + assert letter_contact[0].service.id == service.id + assert letter_contact[0].contact_block == 'Bristol, BR1 2DB' + + +def test_create_or_update_letter_contact_raises_exception_if_multiple_contact_blocks_exist(notify_db_session): + service = create_service() + create_letter_contact(service=service, contact_block='Edinburgh, ED1 1AA') + create_letter_contact(service=service, contact_block='Manchester, MA1 2BB', is_default=False) + + with pytest.raises(expected_exception=InvalidRequest) as e: + create_or_update_letter_contact(service_id=service.id, contact_block='Swansea, SN1 3CC') + assert e.value.message == "Multiple letter contacts were found, this method should not be used." From 3977574ef1a862b0fb581b821a44c1c93ffd4a42 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 25 Sep 2017 15:46:01 +0100 Subject: [PATCH 2/3] Add methods to add/update letter contacts for a service that handle defaults properly --- app/dao/service_letter_contact_dao.py | 64 ++++++++- .../dao/test_service_letter_contact_dao.py | 136 ++++++++++++++++++ 2 files changed, 199 insertions(+), 1 deletion(-) diff --git a/app/dao/service_letter_contact_dao.py b/app/dao/service_letter_contact_dao.py index 578483f62..2d0430299 100644 --- a/app/dao/service_letter_contact_dao.py +++ b/app/dao/service_letter_contact_dao.py @@ -28,7 +28,7 @@ def create_or_update_letter_contact(service_id, contact_block): letter_contacts[0].contact_block = contact_block dao_update_letter_contact(letter_contacts[0]) else: - # Once we move allowing letter contact blocks, this method will be removed + # TODO: Once we move allowing letter contact blocks, this method will be removed raise InvalidRequest( "Multiple letter contacts were found, this method should not be used.", status_code=500 @@ -43,3 +43,65 @@ def dao_create_letter_contact(letter_contact): @transactional def dao_update_letter_contact(letter_contact): db.session.add(letter_contact) + + +@transactional +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, + contact_block=contact_block, + is_default=is_default + ) + db.session.add(new_letter_contact) + return new_letter_contact + + +@transactional +def update_letter_contact(service_id, letter_contact_id, contact_block, is_default): + old_default = _get_existing_default(service_id) + # 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 + letter_contact_update.is_default = is_default + db.session.add(letter_contact_update) + return letter_contact_update + + +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 + + +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) diff --git a/tests/app/dao/test_service_letter_contact_dao.py b/tests/app/dao/test_service_letter_contact_dao.py index 770fdfa16..5af184547 100644 --- a/tests/app/dao/test_service_letter_contact_dao.py +++ b/tests/app/dao/test_service_letter_contact_dao.py @@ -1,8 +1,10 @@ import pytest from app.dao.service_letter_contact_dao import ( + add_letter_contact_for_service, create_or_update_letter_contact, dao_get_letter_contacts_by_service_id, + update_letter_contact ) from app.errors import InvalidRequest from app.models import ServiceLetterContact @@ -64,3 +66,137 @@ def test_create_or_update_letter_contact_raises_exception_if_multiple_contact_bl with pytest.raises(expected_exception=InvalidRequest) as e: create_or_update_letter_contact(service_id=service.id, contact_block='Swansea, SN1 3CC') assert e.value.message == "Multiple letter contacts were found, this method should not be used." + + +def test_create_or_update_letter_contact_raises_exception_if_multiple_letter_contacts_exist(notify_db_session): + service = create_service() + create_letter_contact(service=service, contact_block='Edinburgh, ED1 1AA') + create_letter_contact(service=service, contact_block='Manchester, MA1 2BB', is_default=False) + + with pytest.raises(expected_exception=InvalidRequest) as e: + create_or_update_letter_contact(service_id=service.id, contact_block='Swansea, SN1 3CC') + assert e.value.message == "Multiple letter contacts were found, this method should not be used." + + +def test_add_letter_contact_for_service_creates_additional_letter_contact_for_service(notify_db_session): + service = create_service() + + create_letter_contact(service=service, contact_block='Edinburgh, ED1 1AA') + add_letter_contact_for_service(service_id=service.id, contact_block='Swansea, SN1 3CC', is_default=False) + + results = dao_get_letter_contacts_by_service_id(service_id=service.id) + + assert len(results) == 2 + + assert results[0].contact_block == 'Edinburgh, ED1 1AA' + assert results[0].is_default + + assert results[1].contact_block == 'Swansea, SN1 3CC' + assert not results[1].is_default + + +def test_add_another_letter_contact_as_default_overrides_existing(notify_db_session): + service = create_service() + + create_letter_contact(service=service, contact_block='Edinburgh, ED1 1AA') + add_letter_contact_for_service(service_id=service.id, contact_block='Swansea, SN1 3CC', is_default=True) + + results = dao_get_letter_contacts_by_service_id(service_id=service.id) + + assert len(results) == 2 + + assert results[0].contact_block == 'Edinburgh, ED1 1AA' + assert not results[0].is_default + + assert results[1].contact_block == 'Swansea, SN1 3CC' + assert results[1].is_default + + +def test_add_letter_contact_does_not_override_default(notify_db_session): + service = create_service() + + add_letter_contact_for_service(service_id=service.id, contact_block='Edinburgh, ED1 1AA', is_default=True) + add_letter_contact_for_service(service_id=service.id, contact_block='Swansea, SN1 3CC', is_default=False) + + results = dao_get_letter_contacts_by_service_id(service_id=service.id) + + assert len(results) == 2 + + assert results[0].contact_block == 'Edinburgh, ED1 1AA' + assert results[0].is_default + + assert results[1].contact_block == 'Swansea, SN1 3CC' + assert not results[1].is_default + + +def test_add_letter_contact_with_no_default_raises_exception(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 + ) + + +def test_add_letter_contact_when_multiple_defaults_exist_raises_exception(notify_db_session): + service = create_service() + create_letter_contact(service=service, contact_block='Edinburgh, ED1 1AA') + create_letter_contact(service=service, contact_block='Aberdeen, AB12 23X') + + with pytest.raises(Exception): + add_letter_contact_for_service(service_id=service.id, contact_block='Swansea, SN1 3CC', is_default=False) + + +def test_can_update_letter_contact(notify_db_session): + service = create_service() + letter_contact = create_letter_contact(service=service, contact_block='Aberdeen, AB12 23X') + + update_letter_contact( + service_id=service.id, + letter_contact_id=letter_contact.id, + contact_block='Warwick, W14 TSR', + is_default=True + ) + + updated_letter_contact = ServiceLetterContact.query.get(letter_contact.id) + + assert updated_letter_contact.contact_block == 'Warwick, W14 TSR' + assert updated_letter_contact.updated_at + assert updated_letter_contact.is_default + + +def test_update_letter_contact_as_default_overides_existing_default(notify_db_session): + service = create_service() + + create_letter_contact(service=service, contact_block='Aberdeen, AB12 23X') + second_letter_contact = create_letter_contact(service=service, contact_block='Swansea, SN1 3CC', is_default=False) + + update_letter_contact( + service_id=service.id, + letter_contact_id=second_letter_contact.id, + contact_block='Warwick, W14 TSR', + is_default=True + ) + + results = dao_get_letter_contacts_by_service_id(service_id=service.id) + assert len(results) == 2 + + assert results[0].contact_block == 'Aberdeen, AB12 23X' + assert not results[0].is_default + + assert results[1].contact_block == 'Warwick, W14 TSR' + assert results[1].is_default + + +def test_update_letter_contact_unset_default_for_only_letter_contact_raises_exception(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 + ) From f0784109a1a72332f290ceba304c025fbd75127e Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 25 Sep 2017 16:01:24 +0100 Subject: [PATCH 3/3] Upsert into letter contact table when updating a service's letter contact block --- app/service/rest.py | 13 +++++++++++-- tests/app/service/test_rest.py | 20 +++++++++++++++++++- 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/app/service/rest.py b/app/service/rest.py index 909675df5..b54e792bc 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -46,8 +46,14 @@ from app.dao.service_whitelist_dao import ( dao_add_and_commit_whitelisted_contacts, dao_remove_service_whitelist ) -from app.dao.service_email_reply_to_dao import create_or_update_email_reply_to, dao_get_reply_to_by_service_id, \ - add_reply_to_email_address_for_service, update_reply_to_email_address, dao_get_reply_to_by_id +from app.dao.service_email_reply_to_dao import ( + add_reply_to_email_address_for_service, + create_or_update_email_reply_to, + dao_get_reply_to_by_id, + dao_get_reply_to_by_service_id, + update_reply_to_email_address +) +from app.dao.service_letter_contact_dao import create_or_update_letter_contact from app.dao.provider_statistics_dao import get_fragment_count from app.dao.users_dao import get_user_by_id from app.errors import ( @@ -151,6 +157,9 @@ def update_service(service_id): if 'sms_sender' in req_json: insert_or_update_service_sms_sender(fetched_service, req_json['sms_sender']) + if 'letter_contact_block' in req_json: + create_or_update_letter_contact(fetched_service.id, req_json['letter_contact_block']) + if service_going_live: send_notification_to_service_users( service_id=service_id, diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 0551ec7c5..7dbced56f 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -12,7 +12,7 @@ from app.dao.services_dao import dao_remove_user_from_service from app.dao.templates_dao import dao_redact_template from app.dao.users_dao import save_model_user from app.models import ( - User, Organisation, Service, ServicePermission, Notification, ServiceEmailReplyTo, + User, Organisation, Service, ServicePermission, Notification, ServiceEmailReplyTo, ServiceLetterContact, DVLA_ORG_LAND_REGISTRY, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST, EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INBOUND_SMS_TYPE, @@ -2301,3 +2301,21 @@ def test_get_email_reply_to_address(client, notify_db, notify_db_session): assert response.status_code == 200 assert json.loads(response.get_data(as_text=True)) == reply_to.serialize() + + +def test_update_service_letter_contact_upserts_letter_contact(admin_request, sample_service): + response = admin_request.post( + 'service.update_service', + service_id=sample_service.id, + _data={ + 'letter_contact_block': 'Aberdeen, AB23 1XH' + }, + _expected_status=200 + ) + + letter_contacts = ServiceLetterContact.query.all() + + assert len(letter_contacts) == 1 + assert letter_contacts[0].contact_block == 'Aberdeen, AB23 1XH' + assert letter_contacts[0].is_default + assert response['data']['letter_contact_block'] == 'Aberdeen, AB23 1XH'