From cf91ce57fcb8817437f119e81edc83de5c4961e8 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 16 Jun 2016 13:51:20 +0100 Subject: [PATCH 1/2] =?UTF-8?q?Add=20a=20=E2=80=98preview=20template?= =?UTF-8?q?=E2=80=99=20endpoint?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There’s a need for users of the API to be able to take advantage of Notify’s template rendering. For example, there’s a service that’s building a case management system. Their users are sending emails on a case-by-case basis. Before they send an email, it’s ressuring to double check that the right data is being inserted, that the right template is being used, etc. This commit: - adds a separate endpoint for previewing a template, with personalisation taken from the get parameters of the request - beefs up the tests around getting a template Not part of this pull request: - making this enpoint publicly accessible --- app/template/rest.py | 25 +++++++++++ tests/app/template/test_rest.py | 78 +++++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+) diff --git a/app/template/rest.py b/app/template/rest.py index dc285e9d1..692f27d14 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -86,6 +86,31 @@ def get_template_by_id_and_service_id(service_id, template_id): return jsonify(data=data) +@template.route('//preview', methods=['GET']) +def preview_template_by_id_and_service_id(service_id, template_id): + fetched_template = dao_get_template_by_id_and_service_id(template_id=template_id, service_id=service_id) + data = template_schema.dump(fetched_template).data + template_object = Template(data, values=request.args.to_dict()) + + if template_object.missing_data: + raise InvalidRequest( + {'template': [ + 'Missing personalisation: {}'.format(", ".join(template_object.missing_data)) + ]}, status_code=400 + ) + + if template_object.additional_data: + raise InvalidRequest( + {'template': [ + 'Personalisation not needed for template: {}'.format(", ".join(template_object.additional_data)) + ]}, status_code=400 + ) + + data['subject'], data['content'] = template_object.replaced_subject, template_object.replaced + + return jsonify(data=data) + + @template.route('//version/') def get_template_version(service_id, template_id, version): data = template_history_schema.dump( diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 25e03d390..ff69342fe 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -1,8 +1,10 @@ import json import random import string +import pytest from app.models import Template from tests import create_authorization_header +from tests.app.conftest import sample_template as create_sample_template from app.dao.templates_dao import dao_get_template_by_id @@ -324,6 +326,82 @@ def test_should_get_only_templates_for_that_service(notify_api, sample_user, ser assert len(json_resp_4['data']) == 1 +@pytest.mark.parametrize( + "subject, content, path, expected_subject, expected_content, expected_error", [ + ( + 'about your ((thing))', + 'hello ((name)) we’ve received your ((thing))', + '/service/{}/template/{}', + 'about your ((thing))', + 'hello ((name)) we’ve received your ((thing))', + None + ), + ( + 'about your thing', + 'hello user we’ve received your thing', + '/service/{}/template/{}/preview', + 'about your thing', + 'hello user we’ve received your thing', + None + ), + ( + 'about your ((thing))', + 'hello ((name)) we’ve received your ((thing))', + '/service/{}/template/{}/preview?name=Amala&thing=document', + 'about your document', + 'hello Amala we’ve received your document', + None + ), + ( + 'about your ((thing))', + 'hello ((name)) we’ve received your ((thing))', + '/service/{}/template/{}/preview?eman=Amala&gniht=document', + None, None, + 'Missing personalisation: thing, name' + ), + ( + 'about your ((thing))', + 'hello ((name)) we’ve received your ((thing))', + '/service/{}/template/{}/preview?name=Amala&thing=document&foo=bar', + None, None, + 'Personalisation not needed for template: foo' + ) + ] +) +def test_should_get_a_single_template( + notify_db, + notify_api, + sample_user, + service_factory, + subject, + content, + path, + expected_subject, + expected_content, + expected_error +): + with notify_api.test_request_context(), notify_api.test_client() as client: + + template = create_sample_template( + notify_db, notify_db.session, subject_line=subject, content=content, template_type='email' + ) + + response = client.get( + path.format(template.service.id, template.id), + headers=[create_authorization_header()] + ) + + content = json.loads(response.get_data(as_text=True)) + + if expected_error: + assert response.status_code == 400 + assert content['message']['template'] == [expected_error] + else: + assert response.status_code == 200 + assert content['data']['content'] == expected_content + assert content['data']['subject'] == expected_subject + + def test_should_return_empty_array_if_no_templates_for_service(notify_api, sample_service): with notify_api.test_request_context(): with notify_api.test_client() as client: From 0d9519c656522aacd9f8d516d0bb620794cde1ba Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 17 Jun 2016 12:57:43 +0100 Subject: [PATCH 2/2] Remove wrapper around response object MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before: ```json {'data': {'template': '…'}} ``` There’s no need to wrap the response in key because there will only ever be one valid key for the template preview endpoint. Flatter is better: ```json { 'content': '…', 'subject': '…', 'template_type': '…', … } ``` The response will be different if there’s an error, but you should be checking the status code first anyway. This commit: - changes the template preview endpoint to return the above format - adds a test to make sure that the original `/service/…/template/…` endpoint still returns JSON in the same format (with a `data` key) --- app/template/rest.py | 2 +- tests/app/template/test_rest.py | 49 +++++++++++++++++++++++++++------ 2 files changed, 42 insertions(+), 9 deletions(-) diff --git a/app/template/rest.py b/app/template/rest.py index 692f27d14..a032f6e39 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -108,7 +108,7 @@ def preview_template_by_id_and_service_id(service_id, template_id): data['subject'], data['content'] = template_object.replaced_subject, template_object.replaced - return jsonify(data=data) + return jsonify(data) @template.route('//version/') diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index ff69342fe..2d36e4360 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -327,15 +327,48 @@ def test_should_get_only_templates_for_that_service(notify_api, sample_user, ser @pytest.mark.parametrize( - "subject, content, path, expected_subject, expected_content, expected_error", [ + "subject, content, template_type", [ ( 'about your ((thing))', 'hello ((name)) we’ve received your ((thing))', - '/service/{}/template/{}', - 'about your ((thing))', - 'hello ((name)) we’ve received your ((thing))', - None + 'email' ), + ( + None, + 'hello ((name)) we’ve received your ((thing))', + 'sms' + ) + ] +) +def test_should_get_a_single_template( + notify_db, + notify_api, + sample_user, + service_factory, + subject, + content, + template_type +): + with notify_api.test_request_context(), notify_api.test_client() as client: + + template = create_sample_template( + notify_db, notify_db.session, subject_line=subject, content=content, template_type=template_type + ) + + response = client.get( + '/service/{}/template/{}'.format(template.service.id, template.id), + headers=[create_authorization_header()] + ) + + data = json.loads(response.get_data(as_text=True))['data'] + + assert response.status_code == 200 + assert data['content'] == content + assert data['subject'] == subject + + +@pytest.mark.parametrize( + "subject, content, path, expected_subject, expected_content, expected_error", [ ( 'about your thing', 'hello user we’ve received your thing', @@ -368,7 +401,7 @@ def test_should_get_only_templates_for_that_service(notify_api, sample_user, ser ) ] ) -def test_should_get_a_single_template( +def test_should_preview_a_single_template( notify_db, notify_api, sample_user, @@ -398,8 +431,8 @@ def test_should_get_a_single_template( assert content['message']['template'] == [expected_error] else: assert response.status_code == 200 - assert content['data']['content'] == expected_content - assert content['data']['subject'] == expected_subject + assert content['content'] == expected_content + assert content['subject'] == expected_subject def test_should_return_empty_array_if_no_templates_for_service(notify_api, sample_service):