From 7943b6819b5ba4efefe9b62d41c58ea279c8f528 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 5 Apr 2017 09:18:58 +0100 Subject: [PATCH 1/3] Revert "Revert "Bump utils to allow lists as placeholder values"" --- requirements.txt | 2 +- .../rest/test_send_notification.py | 36 +++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 73cf5b94b..783686de9 100644 --- a/requirements.txt +++ b/requirements.txt @@ -29,6 +29,6 @@ notifications-python-client>=3.1,<3.2 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@13.9.6#egg=notifications-utils==13.9.6 +git+https://github.com/alphagov/notifications-utils.git@14.0.1#egg=notifications-utils==14.0.1 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index 90e2c64e7..7c90f0c99 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -133,6 +133,42 @@ def test_send_notification_with_placeholders_replaced(notify_api, sample_email_t assert response_data['subject'] == 'Jo' +def test_send_notification_with_placeholders_replaced_with_list( + client, + sample_email_template_with_placeholders, + mocker +): + mocked = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') + + response = client.post( + path='/notifications/email', + data=json.dumps( + { + 'to': 'ok@ok.com', + 'template': str(sample_email_template_with_placeholders.id), + 'personalisation': { + 'name': ['Jo', 'John', 'Josephine'] + } + } + ), + headers=[ + ('Content-Type', 'application/json'), + create_authorization_header(service_id=sample_email_template_with_placeholders.service.id) + ] + ) + + response_data = json.loads(response.data)['data'] + assert response.status_code == 201 + assert response_data['body'] == ( + 'Hello \n\n' + '* Jo\n' + '* John\n' + '* Josephine\n' + 'This is an email from GOV.\u200BUK' + ) + assert response_data['subject'] == 'Jo, John and Josephine' + + def test_should_not_send_notification_for_archived_template(notify_api, sample_template): with notify_api.test_request_context(): with notify_api.test_client() as client: From 1802c84b8ad6c02fbe5e23c11ec69a43020f58a8 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 5 Apr 2017 09:28:17 +0100 Subject: [PATCH 2/3] Add handling for data types other than list/string Brings in: - [ ] https://github.com/alphagov/notifications-utils/pull/135 Also adds extra tests for: - the exact issue that we saw in production when #867 was deployed - what happens when `None` is passed as a placeholder value, because this should never get as far as the relevant bit of utils --- requirements.txt | 2 +- .../rest/test_send_notification.py | 47 ++++++++++++++----- 2 files changed, 36 insertions(+), 13 deletions(-) diff --git a/requirements.txt b/requirements.txt index 783686de9..6224a91e8 100644 --- a/requirements.txt +++ b/requirements.txt @@ -29,6 +29,6 @@ notifications-python-client>=3.1,<3.2 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@14.0.1#egg=notifications-utils==14.0.1 +git+https://github.com/alphagov/notifications-utils.git@14.0.2#egg=notifications-utils==14.0.2 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index 7c90f0c99..d6a4e9c31 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -133,10 +133,39 @@ def test_send_notification_with_placeholders_replaced(notify_api, sample_email_t assert response_data['subject'] == 'Jo' -def test_send_notification_with_placeholders_replaced_with_list( +@pytest.mark.parametrize('personalisation, expected_body, expected_subject', [ + ( + ['Jo', 'John', 'Josephine'], + ( + 'Hello \n\n' + '* Jo\n' + '* John\n' + '* Josephine\n' + 'This is an email from GOV.\u200BUK' + ), + 'Jo, John and Josephine', + ), + ( + 6, + ( + 'Hello 6\n' + 'This is an email from GOV.\u200BUK' + ), + '6', + ), + pytest.mark.xfail(( + None, + ('we consider None equivalent to missing personalisation'), + '', + )), +]) +def test_send_notification_with_placeholders_replaced_with_unusual_types( client, sample_email_template_with_placeholders, - mocker + mocker, + personalisation, + expected_body, + expected_subject, ): mocked = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') @@ -147,7 +176,7 @@ def test_send_notification_with_placeholders_replaced_with_list( 'to': 'ok@ok.com', 'template': str(sample_email_template_with_placeholders.id), 'personalisation': { - 'name': ['Jo', 'John', 'Josephine'] + 'name': personalisation } } ), @@ -157,16 +186,10 @@ def test_send_notification_with_placeholders_replaced_with_list( ] ) - response_data = json.loads(response.data)['data'] assert response.status_code == 201 - assert response_data['body'] == ( - 'Hello \n\n' - '* Jo\n' - '* John\n' - '* Josephine\n' - 'This is an email from GOV.\u200BUK' - ) - assert response_data['subject'] == 'Jo, John and Josephine' + response_data = json.loads(response.data)['data'] + assert response_data['body'] == expected_body + assert response_data['subject'] == expected_subject def test_should_not_send_notification_for_archived_template(notify_api, sample_template): From f98f220e74697ac22593c8d9d7c04d97dd6b83e5 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 5 Apr 2017 12:27:27 +0100 Subject: [PATCH 3/3] Update utils to fix letter output Includes: - [x] https://github.com/alphagov/notifications-utils/pull/136 - [ ] https://github.com/alphagov/notifications-utils/pull/131 --- requirements.txt | 2 +- tests/app/celery/test_tasks.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/requirements.txt b/requirements.txt index 6224a91e8..98b9ef1da 100644 --- a/requirements.txt +++ b/requirements.txt @@ -29,6 +29,6 @@ notifications-python-client>=3.1,<3.2 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@14.0.2#egg=notifications-utils==14.0.2 +git+https://github.com/alphagov/notifications-utils.git@15.0.0#egg=notifications-utils==15.0.0 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index eae030386..41c35b299 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1005,4 +1005,4 @@ def test_dvla_letter_template(sample_letter_notification): letter = LetterDVLATemplate(t, sample_letter_notification.personalisation, 12345) - assert str(letter) == "140|500|001||201703230012345|23032017||||||||||||A1|A2|A3|A4|A5|A6||A_POST||||||||Template subject|Dear Sir/Madam, Hello. Yours Truly, The Government." # noqa + assert str(letter) == "140|500|001||201703230012345|||||||||||||A1|A2|A3|A4|A5|A6||A_POST|||||||||23 March 2017

Template subjectDear Sir/Madam, Hello. Yours Truly, The Government." # noqa