From e366ad29ae286b56f0ce28ff09844d3a62b000fc Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 20 Apr 2020 15:18:40 +0100 Subject: [PATCH 1/4] Bump utils to 38.0.0 Brings in breaking change to how the `RecipientCSV` class should be instantiated. --- app/celery/tasks.py | 4 +--- requirements-app.txt | 2 +- requirements.txt | 6 +++--- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 4949b39b0..baed8d6ee 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -126,9 +126,7 @@ def get_recipient_csv_and_template_and_sender_id(job): template = db_template._as_utils_template() contents, meta_data = s3.get_job_and_metadata_from_s3(service_id=str(job.service_id), job_id=str(job.id)) - recipient_csv = RecipientCSV(file_data=contents, - template_type=template.template_type, - placeholders=template.placeholders) + recipient_csv = RecipientCSV(contents, template=template) return recipient_csv, template, meta_data.get("sender_id") diff --git a/requirements-app.txt b/requirements-app.txt index e2a8fd3be..eda2bd02a 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -26,6 +26,6 @@ notifications-python-client==5.5.1 # PaaS awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@37.3.0#egg=notifications-utils==37.3.0 +git+https://github.com/alphagov/notifications-utils.git@38.0.0#egg=notifications-utils==38.0.0 gds-metrics==0.2.0 diff --git a/requirements.txt b/requirements.txt index a7b247db9..1d31bb280 100644 --- a/requirements.txt +++ b/requirements.txt @@ -28,7 +28,7 @@ notifications-python-client==5.5.1 # PaaS awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@37.3.0#egg=notifications-utils==37.3.0 +git+https://github.com/alphagov/notifications-utils.git@38.0.0#egg=notifications-utils==38.0.0 gds-metrics==0.2.0 @@ -37,14 +37,14 @@ alembic==1.4.2 amqp==1.4.9 anyjson==0.3.3 attrs==19.3.0 -awscli==1.18.45 +awscli==1.18.46 bcrypt==3.1.7 billiard==3.3.0.23 bleach==3.1.4 blinker==1.4 boto==2.49.0 boto3==1.10.38 -botocore==1.15.45 +botocore==1.15.46 certifi==2020.4.5.1 chardet==3.0.4 click==7.1.1 From ba0d3305937ade88eaccfc534ce28a38e59c49ff Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 28 Apr 2020 11:16:11 +0100 Subject: [PATCH 2/4] Allow countries in last line of addresses For services that have permission to send international letters we should not reject letters that are addressed to another country. We should still reject letters that are badly-addressed. --- app/v2/notifications/post_notifications.py | 12 ++++- requirements-app.txt | 2 +- .../test_post_letter_notifications.py | 46 ++++++++++++++++++- 3 files changed, 55 insertions(+), 5 deletions(-) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 399940d44..acd37f1a8 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -36,6 +36,7 @@ from app.models import ( NOTIFICATION_SENDING, NOTIFICATION_DELIVERED, NOTIFICATION_PENDING_VIRUS_CHECK, + INTERNATIONAL_LETTERS, Notification) from app.notifications.process_letter_notifications import ( create_letter_notification @@ -345,7 +346,10 @@ def process_letter_notification(*, letter_data, api_key, template, reply_to_text template=template, reply_to_text=reply_to_text) - address = PostalAddress.from_personalisation(letter_data['personalisation']) + address = PostalAddress.from_personalisation( + letter_data['personalisation'], + allow_international_letters=api_key.service.has_permission(INTERNATIONAL_LETTERS), + ) if not address.has_enough_lines: raise ValidationError( @@ -357,7 +361,11 @@ def process_letter_notification(*, letter_data, api_key, template, reply_to_text message=f'Address must be no more than {PostalAddress.MAX_LINES} lines' ) - if not address.postcode: + if not address.has_valid_last_line: + if address.allow_international_letters: + raise ValidationError( + message=f'Last line of address must be a real UK postcode or another country' + ) raise ValidationError( message='Must be a real UK postcode' ) diff --git a/requirements-app.txt b/requirements-app.txt index eda2bd02a..6686f123d 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -26,6 +26,6 @@ notifications-python-client==5.5.1 # PaaS awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@38.0.0#egg=notifications-utils==38.0.0 +git+https://github.com/alphagov/notifications-utils.git@38.1.0#egg=notifications-utils==38.1.0 gds-metrics==0.2.0 diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py index 509e728a9..323688543 100644 --- a/tests/app/v2/notifications/test_post_letter_notifications.py +++ b/tests/app/v2/notifications/test_post_letter_notifications.py @@ -18,7 +18,8 @@ from app.models import ( NOTIFICATION_SENDING, NOTIFICATION_DELIVERED, NOTIFICATION_PENDING_VIRUS_CHECK, - SMS_TYPE + SMS_TYPE, + INTERNATIONAL_LETTERS, ) from app.schema_validation import validate from app.v2.errors import RateLimitError @@ -144,8 +145,49 @@ def test_post_letter_notification_formats_postcode( assert notification.personalisation["postcode"] == ' Sw1 1aa ' -def test_post_letter_notification_throws_error_for_bad_postcode( +def test_post_letter_notification_stores_country( client, notify_db_session, mocker +): + service = create_service(service_permissions=[LETTER_TYPE, INTERNATIONAL_LETTERS]) + template = create_template(service, template_type="letter") + mocker.patch('app.celery.tasks.letters_pdf_tasks.create_letters_pdf.apply_async') + data = { + 'template_id': str(template.id), + 'personalisation': { + 'address_line_1': 'Kaiser Wilhelm II', + 'address_line_2': 'Kronprinzenpalais', + 'address_line_5': ' deutschland ', + } + } + + resp_json = letter_request(client, data, service_id=service.id) + + assert validate(resp_json, post_letter_response) == resp_json + notification = Notification.query.one() + # In the personalisation we store what the client gives us + assert notification.personalisation["address_line_1"] == 'Kaiser Wilhelm II' + assert notification.personalisation["address_line_2"] == 'Kronprinzenpalais' + assert notification.personalisation["address_line_5"] == ' deutschland ' + # In the to field we store the whole address with the canonical country + assert notification.to == ( + 'Kaiser Wilhelm II\n' + 'Kronprinzenpalais\n' + 'Germany' + ) + + +@pytest.mark.parametrize('permissions, expected_error', ( + ( + [LETTER_TYPE], + 'Must be a real UK postcode', + ), + ( + [LETTER_TYPE, INTERNATIONAL_LETTERS], + 'Last line of address must be a real UK postcode or another country', + ), +)) +def test_post_letter_notification_throws_error_for_bad_postcode( + client, notify_db_session, mocker, permissions, expected_error ): service = create_service(service_permissions=[LETTER_TYPE]) template = create_template(service, template_type="letter", postage="first") From 0c1373eeb50855b3100931697acc6d5ee205f5b5 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 29 Apr 2020 15:51:02 +0100 Subject: [PATCH 3/4] Bump utils to 39.0.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `allow_international_letters` is a new, required argument, so the tests that make some `Row` objects need to provide that. There are now 8 possible address columns (7 plus postcode) so the tests need to expect that. But this won’t have any user-facing impact. --- requirements-app.txt | 2 +- requirements.txt | 10 +++++----- tests/app/celery/test_tasks.py | 5 ++++- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/requirements-app.txt b/requirements-app.txt index 6686f123d..448273766 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -26,6 +26,6 @@ notifications-python-client==5.5.1 # PaaS awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@38.1.0#egg=notifications-utils==38.1.0 +git+https://github.com/alphagov/notifications-utils.git@39.0.0#egg=notifications-utils==39.0.0 gds-metrics==0.2.0 diff --git a/requirements.txt b/requirements.txt index 1d31bb280..99d1149a4 100644 --- a/requirements.txt +++ b/requirements.txt @@ -28,7 +28,7 @@ notifications-python-client==5.5.1 # PaaS awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@38.0.0#egg=notifications-utils==38.0.0 +git+https://github.com/alphagov/notifications-utils.git@39.0.0#egg=notifications-utils==39.0.0 gds-metrics==0.2.0 @@ -37,17 +37,17 @@ alembic==1.4.2 amqp==1.4.9 anyjson==0.3.3 attrs==19.3.0 -awscli==1.18.46 +awscli==1.18.48 bcrypt==3.1.7 billiard==3.3.0.23 bleach==3.1.4 blinker==1.4 boto==2.49.0 boto3==1.10.38 -botocore==1.15.46 +botocore==1.15.48 certifi==2020.4.5.1 chardet==3.0.4 -click==7.1.1 +click==7.1.2 colorama==0.4.3 dnspython==1.16.0 docutils==0.15.2 @@ -74,7 +74,7 @@ pyrsistent==0.16.0 python-dateutil==2.8.1 python-editor==1.0.4 python-json-logger==0.1.11 -pytz==2019.3 +pytz==2020.1 PyYAML==5.3.1 redis==3.4.1 requests==2.23.0 diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 94baf03be..000a078b5 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -340,7 +340,7 @@ def test_should_process_letter_job(sample_letter_job, mocker): row_call = process_row_mock.mock_calls[0][1] assert row_call[0].index == 0 - assert row_call[0].recipient == ['A1', 'A2', 'A3', 'A4', None, None, 'A_POST'] + assert row_call[0].recipient == ['A1', 'A2', 'A3', 'A4', None, None, 'A_POST', None] assert row_call[0].personalisation == { 'addressline1': 'A1', 'addressline2': 'A2', @@ -407,6 +407,7 @@ def test_process_row_sends_letter_task(template_type, research_mode, expected_fu recipient_column_headers=['to'], placeholders={'foo'}, template=template, + allow_international_letters=True, ), template, job, @@ -449,6 +450,7 @@ def test_process_row_when_sender_id_is_provided(mocker, fake_uuid): recipient_column_headers=['to'], placeholders={'foo'}, template=template, + allow_international_letters=True, ), template, job, @@ -1418,6 +1420,7 @@ def test_get_letter_template_instance(mocker, sample_job): 'address line 5', 'address line 6', 'postcode', + 'address line 7', ] From 85fc601886b5fc255c9db801f893defee5b4ddf1 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 1 May 2020 14:26:20 +0100 Subject: [PATCH 4/4] Tell template preview to allow international letters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If a service has permission to send international letters then it should tell template preview, so that template preview knows what rule to apply when it’s validating the address of the letter. Depends on: - [ ] https://github.com/alphagov/notifications-template-preview/pull/445 --- app/celery/letters_pdf_tasks.py | 4 ++++ tests/app/celery/test_letters_pdf_tasks.py | 24 +++++++++++++++++++--- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 495426cba..0d749e057 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -40,6 +40,7 @@ from app.letters.utils import ( get_file_names_from_error_bucket, ) from app.models import ( + INTERNATIONAL_LETTERS, KEY_TYPE_TEST, NOTIFICATION_CREATED, NOTIFICATION_DELIVERED, @@ -227,6 +228,9 @@ def sanitise_letter(self, filename): kwargs={ 'notification_id': str(notification.id), 'filename': filename, + 'allow_international_letters': notification.service.has_permission( + INTERNATIONAL_LETTERS + ), }, queue=QueueNames.SANITISE_LETTERS, ) diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index a6e43c27b..6f4e1562a 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -31,8 +31,10 @@ from app.celery.letters_pdf_tasks import ( from app.config import QueueNames, TaskNames from app.letters.utils import ScanErrorType from app.models import ( + INTERNATIONAL_LETTERS, KEY_TYPE_NORMAL, KEY_TYPE_TEST, + LETTER_TYPE, Notification, NOTIFICATION_CREATED, NOTIFICATION_DELIVERED, @@ -42,7 +44,7 @@ from app.models import ( NOTIFICATION_VIRUS_SCAN_FAILED, ) -from tests.app.db import create_notification, create_letter_branding +from tests.app.db import create_notification, create_letter_branding, create_service from tests.conftest import set_config_values @@ -546,16 +548,32 @@ def test_move_invalid_letter_and_update_status_logs_error_and_sets_tech_failure_ ) -def test_sanitise_letter_calls_template_preview_sanitise_task(mocker, sample_letter_notification): +@pytest.mark.parametrize('permissions, expected_international_letters_allowed', ( + ([LETTER_TYPE], False), + ([LETTER_TYPE, INTERNATIONAL_LETTERS], True), +)) +def test_sanitise_letter_calls_template_preview_sanitise_task( + mocker, + sample_letter_notification, + permissions, + expected_international_letters_allowed, +): mock_celery = mocker.patch('app.celery.letters_pdf_tasks.notify_celery.send_task') filename = 'NOTIFY.{}'.format(sample_letter_notification.reference) + sample_letter_notification.service = create_service( + service_permissions=permissions + ) sample_letter_notification.status = NOTIFICATION_PENDING_VIRUS_CHECK sanitise_letter(filename) mock_celery.assert_called_once_with( name=TaskNames.SANITISE_LETTER, - kwargs={'notification_id': str(sample_letter_notification.id), 'filename': filename}, + kwargs={ + 'notification_id': str(sample_letter_notification.id), + 'filename': filename, + 'allow_international_letters': expected_international_letters_allowed, + }, queue=QueueNames.SANITISE_LETTERS, )