Merge pull request #2793 from alphagov/refactor-postcode-validation

Allow any three lines in the address for templated letters
2026-08-24 00:06:16 -04:00 · 2020-04-14 12:51:22 +01:00
parent e2985161bb 36e61272c5
commit f90c475126
8 changed files with 64 additions and 73 deletions
--- a/app/celery/tasks.py
+++ b/app/celery/tasks.py
@@ -3,10 +3,9 @@ from datetime import datetime
 from collections import namedtuple, defaultdict

 from flask import current_app
-from notifications_utils.recipients import (
-    format_postcode_for_printing,
-    RecipientCSV
-)
+from notifications_utils.columns import Columns
+from notifications_utils.postal_address import PostalAddress
+from notifications_utils.recipients import RecipientCSV
 from notifications_utils.statsd_decorators import statsd
 from notifications_utils.timezones import convert_utc_to_bst
 from requests import (
@@ -344,11 +343,9 @@ def save_letter(
    notification = encryption.decrypt(encrypted_notification)

    # we store the recipient as just the first item of the person's address
-    recipient = notification['personalisation']['addressline1']
-
-    notification['personalisation']['postcode'] = format_postcode_for_printing(
-        notification['personalisation']['postcode']
-    )
+    recipient = PostalAddress.from_personalisation(
+        Columns(notification['personalisation'])
+    ).normalised_lines[0]

    service = dao_fetch_service_by_id(service_id)
    template = dao_get_template_by_id(notification['template'], version=notification['template_version'])
--- a/app/schema_validation/definitions.py
+++ b/app/schema_validation/definitions.py
@@ -27,29 +27,6 @@ personalisation = {
 }


-letter_personalisation = dict(
-    personalisation,
-    properties={
-        "address_line_1": {
-            "type": "string",
-            "minLength": 1,
-            "validationMessage": "address_line_1 is required"
-        },
-        "address_line_2": {
-            "type": "string",
-            "minLength": 1,
-            "validationMessage": "address_line_2 is required"
-        },
-        "postcode": {
-            "type": "string",
-            "minLength": 1,
-            "validationMessage": "postcode is required"
-        },
-    },
-    required=["address_line_1", "address_line_2", "postcode"],
-)
-
-
 https_url = {
    "type": "string",
    "format": "uri",
--- a/app/v2/notifications/notification_schemas.py
+++ b/app/v2/notifications/notification_schemas.py
@@ -4,7 +4,7 @@ from app.models import (
    NOTIFICATION_STATUS_LETTER_RECEIVED,
    TEMPLATE_TYPES
 )
-from app.schema_validation.definitions import (uuid, personalisation, letter_personalisation)
+from app.schema_validation.definitions import (uuid, personalisation)


 template = {
@@ -226,7 +226,7 @@ post_letter_request = {
    "properties": {
        "reference": {"type": "string"},
        "template_id": uuid,
-        "personalisation": letter_personalisation
+        "personalisation": personalisation
    },
    "required": ["template_id", "personalisation"],
    "additionalProperties": False
--- a/app/v2/notifications/post_notifications.py
+++ b/app/v2/notifications/post_notifications.py
@@ -5,9 +5,8 @@ from datetime import datetime

 from boto.exception import SQSError
 from flask import request, jsonify, current_app, abort
-from notifications_utils.recipients import (
-    format_postcode_for_printing, is_a_real_uk_postcode, try_validate_and_format_phone_number
-)
+from notifications_utils.postal_address import PostalAddress
+from notifications_utils.recipients import try_validate_and_format_phone_number
 from notifications_utils.template import WithSubjectTemplate

 from app import (
@@ -349,9 +348,22 @@ def process_letter_notification(*, letter_data, api_key, template, reply_to_text
                                                        template=template,
                                                        reply_to_text=reply_to_text)

-    postcode = letter_data['personalisation']['postcode']
-    if not is_a_real_uk_postcode(postcode):
-        raise ValidationError(message='Must be a real UK postcode')
+    address = PostalAddress.from_personalisation(letter_data['personalisation'])
+
+    if not address.has_enough_lines:
+        raise ValidationError(
+            message=f'Address must be at least {PostalAddress.MIN_LINES} lines'
+        )
+
+    if address.has_too_many_lines:
+        raise ValidationError(
+            message=f'Address must be no more than {PostalAddress.MAX_LINES} lines'
+        )
+
+    if not address.postcode:
+        raise ValidationError(
+            message='Must be a real UK postcode'
+        )

    test_key = api_key.key_type == KEY_TYPE_TEST

@@ -359,8 +371,6 @@ def process_letter_notification(*, letter_data, api_key, template, reply_to_text
    status = NOTIFICATION_CREATED if not test_key else NOTIFICATION_SENDING
    queue = QueueNames.CREATE_LETTERS_PDF if not test_key else QueueNames.RESEARCH_MODE

-    letter_data['personalisation']['postcode'] = format_postcode_for_printing(postcode)
-
    notification = create_letter_notification(letter_data=letter_data,
                                              template=template,
                                              api_key=api_key,
--- a/requirements-app.txt
+++ b/requirements-app.txt
@@ -27,4 +27,4 @@ notifications-python-client==5.5.1
 awscli-cwlogs>=1.4,<1.5


-git+https://github.com/alphagov/notifications-utils.git@36.9.3#egg=notifications-utils==36.9.3
+git+https://github.com/alphagov/notifications-utils.git@36.12.0#egg=notifications-utils==36.12.0
--- a/requirements.txt
+++ b/requirements.txt
@@ -29,21 +29,21 @@ notifications-python-client==5.5.1
 awscli-cwlogs>=1.4,<1.5


-git+https://github.com/alphagov/notifications-utils.git@36.9.3#egg=notifications-utils==36.9.3
+git+https://github.com/alphagov/notifications-utils.git@36.12.0#egg=notifications-utils==36.12.0

 ## The following requirements were added by pip freeze:
 alembic==1.4.2
 amqp==1.4.9
 anyjson==0.3.3
 attrs==19.3.0
-awscli==1.18.32
+awscli==1.18.37
 bcrypt==3.1.7
 billiard==3.3.0.23
 bleach==3.1.4
 boto==2.49.0
 boto3==1.10.38
-botocore==1.15.32
-certifi==2019.11.28
+botocore==1.15.37
+certifi==2020.4.5.1
 chardet==3.0.4
 click==7.1.1
 colorama==0.4.3
@@ -81,5 +81,5 @@ smartypants==2.0.1
 statsd==3.3.0
 urllib3==1.25.8
 webencodings==0.5.1
-Werkzeug==1.0.0
+Werkzeug==1.0.1
 zipp==3.1.0
--- a/tests/app/celery/test_tasks.py
+++ b/tests/app/celery/test_tasks.py
@@ -950,7 +950,26 @@ def test_save_sms_does_not_send_duplicate_and_does_not_put_in_retry_queue(sample
    assert not retry.called


-def test_save_letter_saves_letter_to_database(mocker, notify_db_session):
+@pytest.mark.parametrize('personalisation', (
+    {
+        'addressline1': 'Foo',
+        'addressline2': 'Bar',
+        'addressline3': 'Baz',
+        'addressline4': 'Wibble',
+        'addressline5': 'Wobble',
+        'addressline6': 'Wubble',
+        'postcode': 'SE1 2SA',
+    },
+    {
+        # The address isn’t normalised when we store it in the
+        # `personalisation` column, but the first line is normalised for
+        # Storing in the `to` column
+        'addressline2': '    Foo    ',
+        'addressline4': 'Bar',
+        'addressline6': 'se12sa',
+    },
+))
+def test_save_letter_saves_letter_to_database(mocker, notify_db_session, personalisation):
    service = create_service()
    contact_block = create_letter_contact(service=service, contact_block="Address contact", is_default=True)
    template = create_template(service=service, template_type=LETTER_TYPE, reply_to=contact_block.id)
@@ -959,18 +978,9 @@ def test_save_letter_saves_letter_to_database(mocker, notify_db_session):
    mocker.patch('app.celery.tasks.create_random_identifier', return_value="this-is-random-in-real-life")
    mocker.patch('app.celery.tasks.letters_pdf_tasks.create_letters_pdf.apply_async')

-    personalisation = {
-        'addressline1': 'Foo',
-        'addressline2': 'Bar',
-        'addressline3': 'Baz',
-        'addressline4': 'Wibble',
-        'addressline5': 'Wobble',
-        'addressline6': 'Wubble',
-        'postcode': 'SE1 2SA',
-    }
    notification_json = _notification_json(
        template=job.template,
-        to='Foo',
+        to='This is ignored for letters',
        personalisation=personalisation,
        job_id=job.id,
        row_number=1
@@ -1048,7 +1058,7 @@ def test_save_letter_saves_letter_to_database_with_formatted_postcode(mocker, no

    notification_db = Notification.query.one()
    assert notification_db.id == notification_id
-    assert notification_db.personalisation["postcode"] == "SE16 4SA"
+    assert notification_db.personalisation["postcode"] == "se1 64sa"


 def test_save_letter_saves_letter_to_database_right_reply_to(mocker, notify_db_session):
--- a/tests/app/v2/notifications/test_post_letter_notifications.py
+++ b/tests/app/v2/notifications/test_post_letter_notifications.py
@@ -139,7 +139,9 @@ def test_post_letter_notification_formats_postcode(

    assert validate(resp_json, post_letter_response) == resp_json
    notification = Notification.query.one()
-    assert notification.personalisation["postcode"] == "SW1 1AA"
+    # We store what the client gives us, and only reformat it when
+    # generating the PDF
+    assert notification.personalisation["postcode"] == '  Sw1  1aa   '


 def test_post_letter_notification_throws_error_for_bad_postcode(
@@ -268,9 +270,7 @@ def test_post_letter_notification_returns_400_for_empty_personalisation(
    assert error_json['status_code'] == 400
    assert all([e['error'] == 'ValidationError' for e in error_json['errors']])
    assert set([e['message'] for e in error_json['errors']]) == {
-        'personalisation address_line_1 is required',
-        'personalisation address_line_2 is required',
-        'personalisation postcode is required'
+        'Address must be at least 3 lines',
    }


@@ -342,15 +342,12 @@ def test_notification_returns_400_if_address_doesnt_have_underscores(
    error_json = letter_request(client, data, service_id=sample_letter_template.service_id, _expected_status=400)

    assert error_json['status_code'] == 400
-    assert len(error_json['errors']) == 2
-    assert {
-        'error': 'ValidationError',
-        'message': 'personalisation address_line_1 is a required property'
-    } in error_json['errors']
-    assert {
-        'error': 'ValidationError',
-        'message': 'personalisation address_line_2 is a required property'
-    } in error_json['errors']
+    assert error_json['errors'] == [
+        {
+            'error': 'ValidationError',
+            'message': 'Address must be at least 3 lines'
+        }
+    ]


 def test_returns_a_429_limit_exceeded_if_rate_limit_exceeded(