From a8a556d02a2c8519c41bfac48339f082b181ce5b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 7 Jul 2016 15:59:50 +0100 Subject: [PATCH 1/3] Use `PassThrough` renderer Implements and depends on: - [ ] https://github.com/alphagov/notifications-utils/pull/49 --- app/notifications/rest.py | 3 ++- app/schemas.py | 8 +++++++- requirements.txt | 2 +- tests/app/celery/test_provider_tasks.py | 6 +++--- tests/app/conftest.py | 2 +- tests/app/notifications/test_rest.py | 4 ++-- 6 files changed, 16 insertions(+), 9 deletions(-) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index ed1e1ee92..6ee2dab89 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -9,6 +9,7 @@ from flask import ( ) from notifications_utils.recipients import allowed_to_send_to, first_column_heading from notifications_utils.template import Template +from notifications_utils.renderers import PassThrough from app.clients.email.aws_ses import get_aws_responses from app import api_user, encryption, create_uuid, DATETIME_FORMAT, DATE_FORMAT, statsd_client from app.models import KEY_TYPE_TEAM @@ -243,7 +244,7 @@ def send_notification(notification_type): template_object = Template( template.__dict__, notification.get('personalisation', {}), - renderer=lambda content: content + renderer=PassThrough() ) if template_object.missing_data: message = 'Missing personalisation: {}'.format(", ".join(template_object.missing_data)) diff --git a/app/schemas.py b/app/schemas.py index 0cb9010e3..9f02cb83d 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -24,6 +24,8 @@ from notifications_utils.recipients import ( validate_and_format_phone_number ) +from notifications_utils.renderers import PassThrough + from app import ma from app import models from app.dao.permissions_dao import permission_dao @@ -272,7 +274,11 @@ class NotificationStatusSchema(BaseSchema): @post_dump def handle_template_merge(self, in_data): from notifications_utils.template import Template - template = Template(in_data['template'], in_data['personalisation']) + template = Template( + in_data['template'], + in_data['personalisation'], + renderer=PassThrough() + ) in_data['body'] = template.replaced if in_data['template']['template_type'] == 'email': in_data['subject'] = template.replaced_subject diff --git a/requirements.txt b/requirements.txt index 442efe262..015df2abb 100644 --- a/requirements.txt +++ b/requirements.txt @@ -23,4 +23,4 @@ statsd==3.2.1 git+https://github.com/alphagov/notifications-python-client.git@1.0.0#egg=notifications-python-client==1.0.0 -git+https://github.com/alphagov/notifications-utils.git@8.1.0#egg=notifications-utils==8.1.0 +git+https://github.com/alphagov/notifications-utils.git@8.2.0#egg=notifications-utils==8.2.0 diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 1651000aa..13ddc0449 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -141,7 +141,7 @@ def test_send_sms_should_use_template_version_from_notification_not_latest( mmg_client.send_sms.assert_called_once_with( to=format_phone_number(validate_phone_number("+447234123123")), - content="Sample service: This is a template", + content="Sample service: This is a template:\nwith a newline", reference=str(db_notification.id), sender=None ) @@ -151,7 +151,7 @@ def test_send_sms_should_use_template_version_from_notification_not_latest( assert persisted_notification.template_id == sample_template.id assert persisted_notification.template_version == version_on_notification assert persisted_notification.template_version != sample_template.version - assert persisted_notification.content_char_count == len("Sample service: This is a template") + assert persisted_notification.content_char_count == len("Sample service: This is a template:\nwith a newline") assert persisted_notification.status == 'sending' assert not persisted_notification.personalisation @@ -332,7 +332,7 @@ def test_should_send_sms_sender_from_service_if_present( mmg_client.send_sms.assert_called_once_with( to=format_phone_number(validate_phone_number("+447234123123")), - content="Sample service: This is a template", + content="Sample service: This is a template:\nwith a newline", reference=str(db_notification.id), sender=sample_service.sms_sender ) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 610b5e63c..b322ef63d 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -154,7 +154,7 @@ def sample_template(notify_db, notify_db_session, template_name="Template Name", template_type="sms", - content="This is a template", + content="This is a template:\nwith a newline", archived=False, subject_line='Subject', user=None, diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 28244df89..115fa13b3 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -34,7 +34,7 @@ def test_get_sms_notification_by_id(notify_api, sample_notification): } assert notification['to'] == '+447700900855' assert notification['service'] == str(sample_notification.service_id) - assert notification['body'] == "This is a template" # sample_template.content + assert notification['body'] == "This is a template:\nwith a newline" assert not notification.get('subject') @@ -172,7 +172,7 @@ def test_get_all_notifications(notify_api, sample_notification): assert notifications['notifications'][0]['to'] == '+447700900855' assert notifications['notifications'][0]['service'] == str(sample_notification.service_id) - assert notifications['notifications'][0]['body'] == "This is a template" # sample_template.content + assert notifications['notifications'][0]['body'] == "This is a template:\nwith a newline" def test_normal_api_key_returns_notifications_created_from_jobs_and_from_api( From bb3c55ca6c1421b2251d4d02c86c6edf4f1dbc0b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 8 Jul 2016 10:23:33 +0100 Subject: [PATCH 2/3] Make send sms test use a template with a newline --- tests/app/celery/test_provider_tasks.py | 4 ++-- tests/app/conftest.py | 2 +- tests/app/notifications/test_rest.py | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 13ddc0449..e3d000c17 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -101,7 +101,7 @@ def test_should_send_personalised_template_to_correct_sms_provider_and_persist( mmg_client.send_sms.assert_called_once_with( to=format_phone_number(validate_phone_number("+447234123123")), - content="Sample service: Hello Jo", + content="Sample service: Hello Jo\nYour thing is due soon", reference=str(db_notification.id), sender=None ) @@ -110,7 +110,7 @@ def test_should_send_personalised_template_to_correct_sms_provider_and_persist( assert notification.status == 'sending' assert notification.sent_at <= datetime.utcnow() assert notification.sent_by == 'mmg' - assert notification.content_char_count == 24 + assert notification.content_char_count == len("Sample service: Hello Jo\nYour thing is due soon") assert notification.personalisation == {"name": "Jo"} diff --git a/tests/app/conftest.py b/tests/app/conftest.py index b322ef63d..975fa94f0 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -185,7 +185,7 @@ def sample_template(notify_db, @pytest.fixture(scope='function') def sample_template_with_placeholders(notify_db, notify_db_session): - return sample_template(notify_db, notify_db_session, content="Hello ((name))") + return sample_template(notify_db, notify_db_session, content="Hello ((name))\nYour thing is due soon") @pytest.fixture(scope='function') diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 115fa13b3..faf0bfdcf 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -553,7 +553,7 @@ def test_get_notification_by_id_returns_merged_template_content(notify_db, notification = json.loads(response.get_data(as_text=True))['data']['notification'] assert response.status_code == 200 - assert notification['body'] == 'Hello world' + assert notification['body'] == 'Hello world\nYour thing is due soon' assert 'subject' not in notification @@ -610,8 +610,8 @@ def test_get_notifications_for_service_returns_merged_template_content(notify_ap resp = json.loads(response.get_data(as_text=True)) assert len(resp['notifications']) == 2 - assert resp['notifications'][0]['body'] == 'Hello merged with first' - assert resp['notifications'][1]['body'] == 'Hello merged with second' + assert resp['notifications'][0]['body'] == 'Hello merged with first\nYour thing is due soon' + assert resp['notifications'][1]['body'] == 'Hello merged with second\nYour thing is due soon' def _create_auth_header_from_key(api_key): From aa12c885515be7f479e678db5600f0e291a398e8 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 8 Jul 2016 11:02:19 +0100 Subject: [PATCH 3/3] Add a test for sending email to provider MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We had a test like this for sending sms, but not email. This meant that, for example, we weren’t checking that the provider was getting passed the HTML and plain text versions of the email. --- tests/app/celery/test_provider_tasks.py | 51 +++++++++++++++++-- tests/app/conftest.py | 2 +- .../rest/test_send_notification.py | 2 +- tests/app/notifications/test_rest.py | 2 +- 4 files changed, 50 insertions(+), 7 deletions(-) diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index e3d000c17..2bf0a1902 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -80,10 +80,11 @@ def test_should_return_highest_priority_active_provider(notify_db, notify_db_ses def test_should_send_personalised_template_to_correct_sms_provider_and_persist( - notify_db, - notify_db_session, - sample_template_with_placeholders, - mocker): + notify_db, + notify_db_session, + sample_template_with_placeholders, + mocker +): db_notification = sample_notification(notify_db, notify_db_session, template=sample_template_with_placeholders, to_field="+447234123123", personalisation={"name": "Jo"}, status='created') @@ -114,6 +115,48 @@ def test_should_send_personalised_template_to_correct_sms_provider_and_persist( assert notification.personalisation == {"name": "Jo"} +def test_should_send_personalised_template_to_correct_email_provider_and_persist( + notify_db, + notify_db_session, + sample_email_template_with_placeholders, + mocker +): + db_notification = sample_notification( + notify_db=notify_db, notify_db_session=notify_db_session, + template=sample_email_template_with_placeholders, + to_field="jo.smith@example.com", + personalisation={'name': 'Jo'} + ) + + mocker.patch('app.aws_ses_client.send_email', return_value='reference') + mocker.patch('app.aws_ses_client.get_name', return_value="ses") + mocker.patch('app.statsd_client.incr') + mocker.patch('app.statsd_client.timing_with_dates') + mocker.patch('app.statsd_client.timing') + + send_email_to_provider( + db_notification.service_id, + db_notification.id + ) + + app.aws_ses_client.send_email.assert_called_once_with( + '"Sample service" ', + 'jo.smith@example.com', + 'Jo', + body='Hello Jo\nThis is an email from GOV.\u200bUK', + html_body=ANY, + reply_to_address=None + ) + assert '