From a39552f81ff145e90d598f483e6b8681701b586a Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 6 Jun 2016 09:18:42 +0100 Subject: [PATCH 1/6] add pip-accel to travis to cache builds better --- .travis.yml | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 681c35b5c..8d2a4b2c9 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,12 +1,16 @@ sudo: false language: python -cache: pip +cache: + pip: true + directories: + - ~/.pip-accel python: - '3.4' addons: postgresql: '9.3' install: -- pip install -r requirements_for_test.txt +- pip install pip-accel +- pip-accel install -r requirements_for_test.txt before_script: - psql -c 'create database test_notification_api;' -U postgres script: From fac5727ad78a8bf7ea67b452a64d3caa4d12dd8d Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 6 Jul 2016 16:42:49 +0100 Subject: [PATCH 2/6] Fix update script --- migrations/versions/0039_fix_notifications.py | 43 ++++++++++++------- 1 file changed, 27 insertions(+), 16 deletions(-) diff --git a/migrations/versions/0039_fix_notifications.py b/migrations/versions/0039_fix_notifications.py index 14c509c49..f8f9b6f6c 100644 --- a/migrations/versions/0039_fix_notifications.py +++ b/migrations/versions/0039_fix_notifications.py @@ -17,26 +17,37 @@ import sqlalchemy as sa def upgrade(): op.execute('update notifications set notification_type = (select cast(cast(template_type as text) as notification_type) from templates where templates.id= notifications.template_id)') conn = op.get_bind() - del_sql = "select n.id, results.* from (select 'failed' as stat_type, count(status) as count, notification_type, date(created_at) as day, service_id " \ - "from notifications where status in ('temporary-failure', 'permanent-failure', 'technical-failure') group by service_id, notification_type, date(created_at) " \ - "union select 'delivered' as stat_type, count(status) , notification_type, date(created_at) as day, service_id " \ - "from notifications where status in ('delivered') group by service_id, notification_type, date(created_at)) as results, " \ - "notification_statistics n " \ - "where n.day = results.day and n.service_id = results.service_id order by results.day;" + reset_counts = "update notification_statistics set emails_requested = 0, emails_delivered = 0, emails_failed=0," \ + "sms_requested = 0, sms_delivered = 0, sms_failed=0 where day > '2016-06-30'" + op.execute(reset_counts) + all_notifications = "select * from notifications where date(created_at) > '2016-06-30' order by created_at;" - results = conn.execute(del_sql) + results = conn.execute(all_notifications) res = results.fetchall() for x in res: - if x.stat_type == 'delivered' and x.notification_type == 'email': - op.execute("update notification_statistics set emails_delivered = {} where id = '{}'".format(x.count, x.id)) - if x.stat_type == 'failed' and x.notification_type == 'email': - op.execute("update notification_statistics set emails_failed = {} where id = '{}'".format(x.count, x.id)) - if x.stat_type == 'delivered' and x.notification_type == 'sms': - op.execute("update notification_statistics set sms_delivered = {} where id = '{}'".format(x.count, x.id)) - if x.stat_type == 'failed' and x.notification_type == 'sms': - op.execute("update notification_statistics set sms_failed = {} where id = '{}'".format(x.count, x.id)) - + print(' in loop {} {}'.format(x.notification_type, x.created_at)) + created = x.created_at.strftime("%Y-%m-%d") + if x.notification_type == 'email' and x.status == 'delivered': + sql = "update notification_statistics set emails_requested = emails_requested + 1, " \ + "emails_delivered = emails_delivered + 1 where day = date('{}') and service_id = '{}'".format(created, x.service_id) + if x.notification_type == 'sms' and x.status == 'delivered': + sql = "update notification_statistics set sms_requested = sms_requested + 1, " \ + "sms_delivered = sms_delivered + 1 where day = date('{}') and service_id = '{}'".format(created, x.service_id) + if x.notification_type == 'email' and x.status in ['technical-failure', 'temporary-failure', 'permanent-failure']: + sql = "update notification_statistics set emails_requested = emails_requested + 1, " \ + "emails_failed = emails_failed + 1 where day = date('{}') and service_id = '{}'".format(created, x.service_id) + if x.notification_type == 'sms' and x.status in ['technical-failure', 'temporary-failure', 'permanent-failure']: + sql = "update notification_statistics set sms_requested = sms_requested + 1, " \ + "sms_failed = sms_failed + 1 where day = date('{}') and service_id = '{}'".format(created, x.service_id) + if x.notification_type == 'email' and x.status in ['created', 'sending', 'pending']: + sql = "update notification_statistics set emails_requested = emails_requested + 1 " \ + " where day = date('{}') and service_id = '{}'".format(created, x.service_id) + if x.notification_type == 'sms' and x.status in ['created', 'sending', 'pending']: + sql = "update notification_statistics set sms_requested = sms_requested + 1 " \ + " where day = date('{}') and service_id = '{}'".format(created, x.service_id) + print(sql) + conn.execute(sql) def downgrade(): ### commands auto generated by Alembic - please adjust! ### From 824085ead8a16eabebfe9d41999610b8be44bec2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 5 Jul 2016 16:21:31 +0100 Subject: [PATCH 3/6] Bring in changes to template and CSV processing Functional changes: - adds the blue bar Performance changes - faster CSV processing Depends on: - [ ] https://github.com/alphagov/notifications-utils/pull/47 - [ ] https://github.com/alphagov/notifications-utils/pull/48 Also brings in some breaking changes, which do not affect utils (apart from a weird import). --- app/celery/provider_tasks.py | 28 ++++++++++++++++++---------- app/notifications/rest.py | 6 +++++- requirements.txt | 2 +- 3 files changed, 24 insertions(+), 12 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 8e6e4cb85..6e8654c5b 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -20,9 +20,8 @@ from notifications_utils.recipients import ( ) from app.dao.templates_dao import dao_get_template_by_id -from notifications_utils.template import ( - Template -) +from notifications_utils.template import Template +from notifications_utils.renderers import HTMLEmail, PlainTextEmail, SMSMessage from app.models import SMS_TYPE, EMAIL_TYPE, KEY_TYPE_TEST @@ -62,7 +61,7 @@ def send_sms_to_provider(self, service_id, notification_id): template = Template( template_model.__dict__, values={} if not notification.personalisation else notification.personalisation, - prefix=service.name + renderer=SMSMessage(prefix=service.name) ) try: if service.research_mode or notification.key_type == KEY_TYPE_TEST: @@ -129,9 +128,18 @@ def send_email_to_provider(self, service_id, notification_id): notification = get_notification_by_id(notification_id) if notification.status == 'created': try: - template = Template( - dao_get_template_by_id(notification.template_id, notification.template_version).__dict__, - values=notification.personalisation + template_dict = dao_get_template_by_id(notification.template_id, notification.template_version).__dict__ + + html_email = Template( + template_dict, + values=notification.personalisation, + renderer=HTMLEmail() + ) + + plain_text_email = Template( + template_dict, + values=notification.personalisation, + renderer=PlainTextEmail() ) if service.research_mode or notification.key_type == KEY_TYPE_TEST: @@ -145,9 +153,9 @@ def send_email_to_provider(self, service_id, notification_id): reference = provider.send_email( from_address, notification.to, - template.replaced_subject, - body=template.replaced_govuk_escaped, - html_body=template.as_HTML_email, + plain_text_email.replaced_subject, + body=plain_text_email.replaced, + html_body=html_email.replaced, reply_to_address=service.reply_to_email_address, ) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 5e549eb57..ed1e1ee92 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -240,7 +240,11 @@ def send_notification(notification_type): if errors: raise InvalidRequest(errors, status_code=400) - template_object = Template(template.__dict__, notification.get('personalisation', {})) + template_object = Template( + template.__dict__, + notification.get('personalisation', {}), + renderer=lambda content: content + ) if template_object.missing_data: message = 'Missing personalisation: {}'.format(", ".join(template_object.missing_data)) errors = {'template': [message]} diff --git a/requirements.txt b/requirements.txt index 13d504b91..442efe262 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@6.3.2#egg=notifications-utils==6.3.2 +git+https://github.com/alphagov/notifications-utils.git@8.1.0#egg=notifications-utils==8.1.0 From a8a556d02a2c8519c41bfac48339f082b181ce5b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 7 Jul 2016 15:59:50 +0100 Subject: [PATCH 4/6] 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 5/6] 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 6/6] 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 '