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/2] 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/2] 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)