Merge pull request #3400 from alphagov/validate-postcode-templated-one-off

Add postcode validation check for one-off letters

app/main/forms.py

@@ -1,4 +1,3 @@
-import re
 import weakref
 from datetime import datetime, timedelta
 from itertools import chain
@@ -10,17 +9,13 @@ from flask_wtf import FlaskForm as Form
 from flask_wtf.file import FileAllowed
 from flask_wtf.file import FileField as FileField_wtf
 from notifications_utils.columns import Columns
-from notifications_utils.formatters import (
+from notifications_utils.formatters import strip_whitespace
-    normalise_whitespace_and_newlines,
+from notifications_utils.postal_address import PostalAddress
-    remove_whitespace_before_punctuation,
-    strip_whitespace,
-)
 from notifications_utils.recipients import (
     InvalidPhoneError,
     normalise_phone_number,
     validate_phone_number,
 )
-from notifications_utils.take import Take
 from wtforms import (
     BooleanField,
     DateField,
@@ -368,21 +363,10 @@ class StripWhitespaceStringField(StringField):
         super(StringField, self).__init__(label, **kwargs)
 
 
-class StripWhitespaceTextAreaField(TextAreaField):
+class PostalAddressField(TextAreaField):
     def process_formdata(self, valuelist):
         if valuelist:
-            self.data = Take(
+            self.data = PostalAddress(valuelist[0]).normalised
-                valuelist[0]
-            ).then(
-                remove_whitespace_before_punctuation
-            ).then(
-                normalise_whitespace_and_newlines
-            ).then(
-                # similar to normalise_multiple_newlines but taking everything down to one `\n` instead of two
-                lambda value: re.compile(r'\n{2,}').sub('\n', value)
-            ).then(
-                str.strip
-            )
 
 
 class OnOffField(RadioField):
@@ -765,40 +749,30 @@ class SMSTemplateForm(BaseTemplateForm):
 
 
 class LetterAddressForm(StripWhitespaceForm):
-    MIN_ADDRESS_LINES = 3
-    MAX_ADDRESS_LINES = 7
 
-    address = StripWhitespaceTextAreaField(
+    address = PostalAddressField(
         'Address',
         validators=[DataRequired(message="Cannot be empty")]
     )
 
     def validate_address(self, field):
-        lines = field.data.splitlines()
-        if len(lines) < self.MIN_ADDRESS_LINES:
-            raise ValidationError('Address must be at least 3 lines long')
-        if len(lines) > self.MAX_ADDRESS_LINES:
-            raise ValidationError('Address must be no more than 7 lines long')
 
-    @property
+        address = PostalAddress(field.data)
-    def as_address_lines_1_to_7_with_postcode(self):
-        lines = self.address.data.splitlines()
-        placeholders = {}
 
-        # set all placeholders to empty strings, or all_placeholders_in_session will always return false.
+        if not address.has_enough_lines:
-        # note that it must be `address line #` with spaces, not underscores or dashes
+            raise ValidationError(
-        for i in range(1, 7):
+                f'Address must be at least {PostalAddress.MIN_LINES} lines long'
-            placeholders[f'address line {i}'] = ''
+            )
 
-        # unroll the address into lines, and place into the session in the underlying placeholder names
+        if address.has_too_many_lines:
-        # postcode is required so make sure we put the last value in that
+            raise ValidationError(
-        # TODO: When postcode is no longer a required field, remove this special case and just use `address line #`
+                f'Address must be no more than {PostalAddress.MAX_LINES} lines long'
-        address_lines, last_address_line = lines[:-1], lines[-1]
+            )
-        for i, line in enumerate(address_lines, start=1):
-            placeholders[f'address line {i}'] = line
-        placeholders['postcode'] = last_address_line
 
-        return placeholders
+        if not address.postcode:
+            raise ValidationError(
+                f'Last line of the address must be a real UK postcode'
+            )
 
 
 class EmailTemplateForm(BaseTemplateForm):

app/main/views/send.py

@@ -18,6 +18,10 @@ from notifications_python_client.errors import HTTPError
 from notifications_utils import LETTER_MAX_PAGE_COUNT, SMS_CHAR_COUNT_LIMIT
 from notifications_utils.columns import Columns
 from notifications_utils.pdf import is_letter_too_long
+from notifications_utils.postal_address import (
+    PostalAddress,
+    address_lines_1_to_6_and_postcode_keys,
+)
 from notifications_utils.recipients import (
     RecipientCSV,
     first_column_headings,
@@ -335,18 +339,6 @@ def get_notification_check_endpoint(service_id, template):
     ))
 
 
-def get_letter_address_from_session():
-    """
-    Takes non-blank-string address values out of the placeholders, and returns a multi line string
-    """
-    keys = [f'address line {i}' for i in range(1, 7)] + ['postcode']
-    return '\n'.join([
-        session['placeholders'][key]
-        for key in keys
-        if session['placeholders'].get(key)
-    ])
-
-
 @main.route(
     "/services/<uuid:service_id>/send/<uuid:template_id>/one-off/address",
     methods=['GET', 'POST']
@@ -376,12 +368,14 @@ def send_one_off_letter_address(service_id, template_id):
         sms_sender=None
     )
 
-    current_session_address = get_letter_address_from_session()
+    current_session_address = PostalAddress.from_personalisation(
+        get_normalised_placeholders_from_session()
+    )
 
-    form = LetterAddressForm(address=current_session_address)
+    form = LetterAddressForm(address=current_session_address.normalised)
 
     if form.validate_on_submit():
-        session['placeholders'].update(form.as_address_lines_1_to_7_with_postcode)
+        session['placeholders'].update(PostalAddress(form.address.data).as_personalisation)
 
         placeholders = fields_to_fill_in(
             template,
@@ -482,7 +476,9 @@ def send_test_step(service_id, template_id, step_index):
         ))
 
     # if we're in a letter, we should show address block rather than "address line #" or "postcode"
-    if db_template['template_type'] == 'letter' and current_placeholder in first_column_headings['letter']:
+    if template.template_type == 'letter' and current_placeholder in (
+        Columns.from_keys(address_lines_1_to_6_and_postcode_keys)
+    ):
         return redirect(url_for('.send_one_off_letter_address', service_id=service_id, template_id=template_id))
 
     optional_placeholder = (current_placeholder in optional_address_columns)
@@ -878,10 +874,7 @@ def fields_to_fill_in(template, prefill_current_user=False):
 
 
 def get_normalised_placeholders_from_session():
-    return {
+    return Columns(session.get('placeholders', {}))
-        key: ''.join(value or [])
-        for key, value in session.get('placeholders', {}).items()
-    }
 
 
 def get_recipient_and_placeholders_from_session(template_type):

app/utils.py

@@ -23,6 +23,7 @@ from notifications_utils.formatters import (
     unescaped_formatted_list,
 )
 from notifications_utils.letter_timings import letter_can_be_cancelled
+from notifications_utils.postal_address import PostalAddress
 from notifications_utils.recipients import RecipientCSV
 from notifications_utils.take import Take
 from notifications_utils.template import (
@@ -658,6 +659,28 @@ LETTER_VALIDATION_MESSAGES = {
         'summary': (
             'Validation failed because the last line of the address is not a real UK postcode.'
         ),
+    },
+    'not-enough-address-lines': {
+        'title': 'There’s a problem with the address for this letter',
+        'detail': (
+            f'The address must be at least {PostalAddress.MIN_LINES} '
+            f'lines long.'
+        ),
+        'summary': (
+            f'Validation failed because the address must be at least '
+            f'{PostalAddress.MIN_LINES} lines long.'
+        ),
+    },
+    'too-many-address-lines': {
+        'title': 'There’s a problem with the address for this letter',
+        'detail': (
+            f'The address must be no more than {PostalAddress.MAX_LINES} '
+            f'lines long.'
+        ),
+        'summary': (
+            f'Validation failed because the address must be no more '
+            f'than {PostalAddress.MAX_LINES} lines long.'
+        ),
     }
 }
 

requirements-app.txt

@@ -22,5 +22,5 @@ notifications-python-client==5.5.1
 awscli-cwlogs>=1.4,<1.5
 itsdangerous==1.1.0
 
-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
 git+https://github.com/alphagov/govuk-frontend-jinja.git@v0.5.1-alpha#egg=govuk-frontend-jinja==0.5.1-alpha

requirements.txt

@@ -24,15 +24,15 @@ notifications-python-client==5.5.1
 awscli-cwlogs>=1.4,<1.5
 itsdangerous==1.1.0
 
-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
 git+https://github.com/alphagov/govuk-frontend-jinja.git@v0.5.1-alpha#egg=govuk-frontend-jinja==0.5.1-alpha
 
 ## The following requirements were added by pip freeze:
-awscli==1.18.33
+awscli==1.18.36
 bleach==3.1.4
 boto3==1.10.38
-botocore==1.15.33
+botocore==1.15.36
-certifi==2019.11.28
+certifi==2020.4.5.1
 chardet==3.0.4
 click==7.1.1
 colorama==0.4.3

tests/app/main/views/test_send.py

@@ -2115,7 +2115,7 @@ def test_send_test_works_as_letter_preview(
     assert response.get_data(as_text=True) == 'foo'
     assert mocked_preview.call_args[0][0].id == template_id
     assert type(mocked_preview.call_args[0][0]) == LetterImageTemplate
-    assert mocked_preview.call_args[0][0].values == {'address_line_1': 'Jo Lastname'}
+    assert mocked_preview.call_args[0][0].values == {'addressline1': 'Jo Lastname'}
     assert mocked_preview.call_args[0][1] == filetype
 
 
@@ -2204,35 +2204,38 @@ def test_send_one_off_letter_address_shows_form(
 
 @pytest.mark.parametrize(['form_data', 'expected_placeholders'], [
     # minimal
-    ('\n'.join(['a', 'b', 'c']), {
+    ('\n'.join(['a', 'b', 'sw1a1aa']), {
-        'address line 1': 'a',
+        'address_line_1': 'a',
-        'address line 2': 'b',
+        'address_line_2': 'b',
-        'address line 3': '',
+        'address_line_3': '',
-        'address line 4': '',
+        'address_line_4': '',
-        'address line 5': '',
+        'address_line_5': '',
-        'address line 6': '',
+        'address_line_6': '',
-        'postcode': 'c',
+        'address_line_7': 'SW1A 1AA',
+        'postcode': 'SW1A 1AA',
     }),
     # maximal
-    ('\n'.join(['a', 'b', 'c', 'd', 'e', 'f', 'g']), {
+    ('\n'.join(['a', 'b', 'c', 'd', 'e', 'f', 'sw1a1aa']), {
-        'address line 1': 'a',
+        'address_line_1': 'a',
-        'address line 2': 'b',
+        'address_line_2': 'b',
-        'address line 3': 'c',
+        'address_line_3': 'c',
-        'address line 4': 'd',
+        'address_line_4': 'd',
-        'address line 5': 'e',
+        'address_line_5': 'e',
-        'address line 6': 'f',
+        'address_line_6': 'f',
-        'postcode': 'g',
+        'address_line_7': 'SW1A 1AA',
+        'postcode': 'SW1A 1AA',
     }),
     # it ignores empty lines and strips whitespace from each line.
     # It also strips extra whitespace from the middle of lines.
-    ('\n  a\ta  \n\n\n      \n\n\n\nb  b   \r\nc', {
+    ('\n  a\ta  \n\n\n      \n\n\n\nb  b   \r\n sw1a1aa \n\n', {
-        'address line 1': 'a\ta',
+        'address_line_1': 'a\ta',
-        'address line 2': 'b b',
+        'address_line_2': 'b b',
-        'address line 3': '',
+        'address_line_3': '',
-        'address line 4': '',
+        'address_line_4': '',
-        'address line 5': '',
+        'address_line_5': '',
-        'address line 6': '',
+        'address_line_6': '',
-        'postcode': 'c',
+        'address_line_7': 'SW1A 1AA',
+        'postcode': 'SW1A 1AA',
     }),
 ])
 def test_send_one_off_letter_address_populates_address_fields_in_session(
@@ -2268,6 +2271,7 @@ def test_send_one_off_letter_address_populates_address_fields_in_session(
     ('', 'Cannot be empty'),
     ('a\n\n\n\nb', 'Address must be at least 3 lines long'),
     ('\n'.join(['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h']), 'Address must be no more than 7 lines long'),
+    ('\n'.join(['a', 'b', 'c', 'd', 'e', 'f', 'g']), 'Last line of the address must be a real UK postcode'),
 ])
 def test_send_one_off_letter_address_rejects_bad_addresses(
     client_request,
@@ -2306,7 +2310,7 @@ def test_send_one_off_letter_address_goes_to_next_placeholder(client_request, mo
         'main.send_one_off_letter_address',
         service_id=SERVICE_ONE_ID,
         template_id=template_data['id'],
-        _data={'address': 'a\nb\nc'},
+        _data={'address': 'a\nb\nSW1A 1AA'},
         # step 0-6 represent address line 1-6 and postcode. step 7 is the first non address placeholder
         _expected_redirect=url_for(
             'main.send_one_off_step',

tests/app/test_utils.py

@@ -507,6 +507,28 @@ def test_get_letter_validation_error_for_unknown_error():
             'Validation failed because the last line of the address is not a real UK postcode.'
         ),
     ),
+    (
+        'not-enough-address-lines',
+        None,
+        'There’s a problem with the address for this letter',
+        (
+            'The address must be at least 3 lines long.'
+        ),
+        (
+            'Validation failed because the address must be at least 3 lines long.'
+        ),
+    ),
+    (
+        'too-many-address-lines',
+        None,
+        'There’s a problem with the address for this letter',
+        (
+            'The address must be no more than 7 lines long.'
+        ),
+        (
+            'Validation failed because the address must be no more than 7 lines long.'
+        ),
+    ),
 ])
 def test_get_letter_validation_error_for_known_errors(
     client_request,