From fa8e4b29f2207f0a708207be3c5d3808a887606e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 20 Sep 2017 10:27:18 +0100 Subject: [PATCH 1/5] Return placeholders when getting a template MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit > Currently when retrieving a template via one of the clients, we do > not return the personalisation fields that are required for that > template. > > This is useful for services who want to perform template validation on > their own systems. A service user has also requested this. – https://www.pivotaltracker.com/story/show/150674476 This commit adds an extra attribute to the JSON response containing an array of the placeholder names. This key is called "personalisation", to match the argument that developers use to pass in the values of placeholders. --- app/models.py | 25 ++++++++++++ tests/app/conftest.py | 10 ++++- tests/app/v2/template/test_get_template.py | 46 ++++++++++++++++++++++ 3 files changed, 79 insertions(+), 2 deletions(-) diff --git a/app/models.py b/app/models.py index 4b3c24b94..2e7ef2b2f 100644 --- a/app/models.py +++ b/app/models.py @@ -18,6 +18,11 @@ from notifications_utils.recipients import ( InvalidEmailError ) from notifications_utils.letter_timings import get_letter_timings +from notifications_utils.template import ( + PlainTextEmailTemplate, + SMSMessageTemplate, + LetterDVLATemplate, +) from app.encryption import ( hashpw, @@ -516,6 +521,22 @@ class Template(db.Model): _external=True ) + def _as_utils_template(self): + if self.template_type == EMAIL_TYPE: + return PlainTextEmailTemplate( + {'content': self.content, 'subject': self.subject} + ) + if self.template_type == SMS_TYPE: + return SMSMessageTemplate( + {'content': self.content} + ) + if self.template_type == LETTER_TYPE: + return LetterDVLATemplate( + {'content': self.content, 'subject': self.subject}, + notification_reference=1, + contact_block=self.service.letter_contact_block, + ) + def serialize(self): serialized = { "id": str(self.id), @@ -527,6 +548,7 @@ class Template(db.Model): "body": self.content, "subject": self.subject if self.template_type != SMS_TYPE else None, "name": self.name, + "personalisation": list(self._as_utils_template().placeholders), } return serialized @@ -567,6 +589,9 @@ class TemplateHistory(db.Model): nullable=False, default=NORMAL) + def _as_utils_template(self): + return Template._as_utils_template(self) + def serialize(self): return Template.serialize(self) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index c3a9d4b9f..042a20905 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -137,7 +137,8 @@ def sample_service( limit=1000, email_from=None, permissions=[SMS_TYPE, EMAIL_TYPE], - research_mode=None + research_mode=None, + letter_contact_block='London,\nSW1A 1AA', ): if user is None: user = create_user() @@ -150,7 +151,7 @@ def sample_service( 'restricted': restricted, 'email_from': email_from, 'created_by': user, - 'letter_contact_block': 'London,\nSW1A 1AA' + 'letter_contact_block': letter_contact_block, } service = Service.query.filter_by(name=service_name).first() if not service: @@ -181,6 +182,11 @@ def sample_service_full_permissions(notify_db, notify_db_session): ) +@pytest.fixture(scope='function') +def sample_service_custom_letter_contact_block(notify_db, notify_db_session): + return sample_service(notify_db, notify_db_session, letter_contact_block='((contact block))') + + @pytest.fixture(scope='function') def sample_template( notify_db, diff --git a/tests/app/v2/template/test_get_template.py b/tests/app/v2/template/test_get_template.py index 56906451e..ef4385e25 100644 --- a/tests/app/v2/template/test_get_template.py +++ b/tests/app/v2/template/test_get_template.py @@ -40,11 +40,57 @@ def test_get_template_by_id_returns_200(client, sample_service, tmp_type, expect 'body': template.content, "subject": expected_subject, 'name': expected_name, + 'personalisation': [], } assert json_response == expected_response +@pytest.mark.parametrize("create_template_args, expected_personalisation", [ + ( + { + "template_type": SMS_TYPE, + "content": "Hello ((placeholder)) ((conditional??yes))", + }, + ["placeholder", "conditional"] + ), + ( + { + "template_type": EMAIL_TYPE, + "subject": "((subject))", + "content": "((content))", + }, + ["subject", "content"] + ), + ( + { + "template_type": LETTER_TYPE, + "subject": "((letterSubject))", + "content": "((letter_content))", + }, + ["letterSubject", "letter_content", "contact block"] + ) +]) +@pytest.mark.parametrize("version", valid_version_params) +def test_get_template_by_id_returns_placeholders( + client, + sample_service_custom_letter_contact_block, + version, + create_template_args, + expected_personalisation, +): + template = create_template(sample_service_custom_letter_contact_block, **create_template_args) + auth_header = create_authorization_header(service_id=sample_service_custom_letter_contact_block.id) + + version_path = '/version/{}'.format(version) if version else '' + + response = client.get(path='/v2/template/{}{}'.format(template.id, version_path), + headers=[('Content-Type', 'application/json'), auth_header]) + + json_response = json.loads(response.get_data(as_text=True)) + assert json_response['personalisation'] == expected_personalisation + + def test_get_template_with_non_existent_template_id_returns_404(client, fake_uuid, sample_service): auth_header = create_authorization_header(service_id=sample_service.id) From 55d185f50d39f282644a4a2648140e3dced82881 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 22 Sep 2017 10:12:32 +0100 Subject: [PATCH 2/5] Return metadata about placeholders MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the future, we may want to return additional information about placeholders. We came up with three possible formats: 1. list of `dict`s, eg `[{'name': 'first name', 'required': True}]` 2. `dict` of `list`s, eg `{'required': ['first name']}` 3. `dict` of `dict`s, eg `{'name': {'required': True}}` I don’t like 1. because it’s harder to traverse if all you want is the name of the placeholders, and suggests that you could have two placeholders with the same name (which you can’t). I don’t like 2. because it only lets the data be sliced by one dimension (unless the inner lists aren’t exclusive, in which case you’d need to filter duplicates when just listing placeholders). I think 3. has the two advantages of: - represents that personalisation is unique, ie you can’t pass back in two different values for the same key - is forward compatible, ie we can add many more properties of a placeholder without breaking anything So this commit implements 3. --- app/models.py | 7 ++++- tests/app/v2/template/test_get_template.py | 32 +++++++++++++++++++--- 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/app/models.py b/app/models.py index 2e7ef2b2f..42d3fb74d 100644 --- a/app/models.py +++ b/app/models.py @@ -548,7 +548,12 @@ class Template(db.Model): "body": self.content, "subject": self.subject if self.template_type != SMS_TYPE else None, "name": self.name, - "personalisation": list(self._as_utils_template().placeholders), + "personalisation": { + key: { + 'required': True, + } + for key in self._as_utils_template().placeholders + }, } return serialized diff --git a/tests/app/v2/template/test_get_template.py b/tests/app/v2/template/test_get_template.py index ef4385e25..3be2ce889 100644 --- a/tests/app/v2/template/test_get_template.py +++ b/tests/app/v2/template/test_get_template.py @@ -40,7 +40,7 @@ def test_get_template_by_id_returns_200(client, sample_service, tmp_type, expect 'body': template.content, "subject": expected_subject, 'name': expected_name, - 'personalisation': [], + 'personalisation': {}, } assert json_response == expected_response @@ -52,7 +52,14 @@ def test_get_template_by_id_returns_200(client, sample_service, tmp_type, expect "template_type": SMS_TYPE, "content": "Hello ((placeholder)) ((conditional??yes))", }, - ["placeholder", "conditional"] + { + "placeholder": { + "required": True + }, + "conditional": { + "required": True + }, + }, ), ( { @@ -60,7 +67,14 @@ def test_get_template_by_id_returns_200(client, sample_service, tmp_type, expect "subject": "((subject))", "content": "((content))", }, - ["subject", "content"] + { + "subject": { + "required": True + }, + "content": { + "required": True + }, + }, ), ( { @@ -68,7 +82,17 @@ def test_get_template_by_id_returns_200(client, sample_service, tmp_type, expect "subject": "((letterSubject))", "content": "((letter_content))", }, - ["letterSubject", "letter_content", "contact block"] + { + "letterSubject": { + "required": True, + }, + "letter_content": { + "required": True, + }, + "contact block": { + "required": True, + }, + }, ) ]) @pytest.mark.parametrize("version", valid_version_params) From fa3f4f3c2079c6c8ef02ee6f4947acaa39a8d393 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 25 Sep 2017 12:26:29 +0100 Subject: [PATCH 3/5] 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 4/5] 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 5/5] 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'