From 26f702ebce0523ac8930562f5910dd520e292a41 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 2 Apr 2020 17:21:27 +0100 Subject: [PATCH 1/3] Refactor to use PostalAddress helper from utils MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since we’re doing normalisation and line-count-checking of addresses in multiple places it makes sense for that code to be shared. Which is what happened here: https://github.com/alphagov/notifications-utils/pull/713 This commit refactors the admin code to make use of the new utils code. Note about placeholders: - they now go into the session as `address_line_1` instead of `address line 1` because this is the format the API uses, so should be considered canonical - they are now fetched from the session in a way that isn’t sensitive to case or underscores (using the `Columns` class) - the API doesn’t care about case or underscores vs spaces in placeholder names because it’s checking an instance of `Template` to see if all the required placeholders are present (see https://github.com/alphagov/notifications-api/blob/401c8e41d6a1e3b348b99eb0cc205bb420fce0c1/app/notifications/process_notifications.py#L40) --- app/main/forms.py | 59 ++++++++----------------------- app/main/views/send.py | 33 +++++++---------- requirements-app.txt | 2 +- requirements.txt | 8 ++--- tests/app/main/views/test_send.py | 41 +++++++++++---------- 5 files changed, 54 insertions(+), 89 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 850d5dee2..eca50ab26 100644 --- a/app/main/forms.py +++ b/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 ( - normalise_whitespace_and_newlines, - remove_whitespace_before_punctuation, - strip_whitespace, -) +from notifications_utils.formatters import strip_whitespace +from notifications_utils.postal_address import PostalAddress 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( - 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 - ) + self.data = PostalAddress(valuelist[0]).normalised class OnOffField(RadioField): @@ -765,40 +749,25 @@ 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 - def as_address_lines_1_to_7_with_postcode(self): - lines = self.address.data.splitlines() - placeholders = {} + address = PostalAddress(field.data) - # set all placeholders to empty strings, or all_placeholders_in_session will always return false. - # note that it must be `address line #` with spaces, not underscores or dashes - for i in range(1, 7): - placeholders[f'address line {i}'] = '' + if not address.has_enough_lines: + raise ValidationError( + f'Address must be at least {PostalAddress.MIN_LINES} lines long' + ) - # unroll the address into lines, and place into the session in the underlying placeholder names - # postcode is required so make sure we put the last value in that - # TODO: When postcode is no longer a required field, remove this special case and just use `address line #` - 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 address.has_too_many_lines: + raise ValidationError( + f'Address must be no more than {PostalAddress.MAX_LINES} lines long' + ) class EmailTemplateForm(BaseTemplateForm): diff --git a/app/main/views/send.py b/app/main/views/send.py index a4bcf08cf..5fa68d173 100644 --- a/app/main/views/send.py +++ b/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//send//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 { - key: ''.join(value or []) - for key, value in session.get('placeholders', {}).items() - } + return Columns(session.get('placeholders', {})) def get_recipient_and_placeholders_from_session(template_type): diff --git a/requirements-app.txt b/requirements-app.txt index 9967d53d3..0c386fd24 100644 --- a/requirements-app.txt +++ b/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 diff --git a/requirements.txt b/requirements.txt index 19e54be1e..5c110203d 100644 --- a/requirements.txt +++ b/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 -certifi==2019.11.28 +botocore==1.15.36 +certifi==2020.4.5.1 chardet==3.0.4 click==7.1.1 colorama==0.4.3 diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 84398c3e3..8547bec48 100644 --- a/tests/app/main/views/test_send.py +++ b/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 @@ -2205,33 +2205,36 @@ def test_send_one_off_letter_address_shows_form( @pytest.mark.parametrize(['form_data', 'expected_placeholders'], [ # minimal ('\n'.join(['a', 'b', 'c']), { - 'address line 1': 'a', - 'address line 2': 'b', - 'address line 3': '', - 'address line 4': '', - 'address line 5': '', - 'address line 6': '', + 'address_line_1': 'a', + 'address_line_2': 'b', + 'address_line_3': '', + 'address_line_4': '', + 'address_line_5': '', + 'address_line_6': '', + 'address_line_7': 'c', 'postcode': 'c', }), # maximal ('\n'.join(['a', 'b', 'c', 'd', 'e', 'f', 'g']), { - 'address line 1': 'a', - 'address line 2': 'b', - 'address line 3': 'c', - 'address line 4': 'd', - 'address line 5': 'e', - 'address line 6': 'f', + 'address_line_1': 'a', + 'address_line_2': 'b', + 'address_line_3': 'c', + 'address_line_4': 'd', + 'address_line_5': 'e', + 'address_line_6': 'f', + 'address_line_7': 'g', 'postcode': 'g', }), # 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', { - 'address line 1': 'a\ta', - 'address line 2': 'b b', - 'address line 3': '', - 'address line 4': '', - 'address line 5': '', - 'address line 6': '', + 'address_line_1': 'a\ta', + 'address_line_2': 'b b', + 'address_line_3': '', + 'address_line_4': '', + 'address_line_5': '', + 'address_line_6': '', + 'address_line_7': 'c', 'postcode': 'c', }), ]) From c646c16067dde34d491ed5813353b029dd6c00a7 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 2 Apr 2020 17:32:26 +0100 Subject: [PATCH 2/3] Add postcode validation check for one-off letters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We’re doing this everywhere else now, so this completes the story. It uses the same regex as elsewhere and the error messaging is consistent (but not uniform) with the other places. --- app/main/forms.py | 5 +++++ tests/app/main/views/test_send.py | 21 +++++++++++---------- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index eca50ab26..46592daf9 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -769,6 +769,11 @@ class LetterAddressForm(StripWhitespaceForm): f'Address must be no more than {PostalAddress.MAX_LINES} lines long' ) + if not address.postcode: + raise ValidationError( + f'Last line of the address must be a real UK postcode' + ) + class EmailTemplateForm(BaseTemplateForm): subject = TextAreaField( diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 8547bec48..a43a06420 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -2204,38 +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_2': 'b', 'address_line_3': '', 'address_line_4': '', 'address_line_5': '', 'address_line_6': '', - 'address_line_7': 'c', - '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_2': 'b', 'address_line_3': 'c', 'address_line_4': 'd', 'address_line_5': 'e', 'address_line_6': 'f', - 'address_line_7': 'g', - '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_2': 'b b', 'address_line_3': '', 'address_line_4': '', 'address_line_5': '', 'address_line_6': '', - 'address_line_7': 'c', - 'postcode': 'c', + 'address_line_7': 'SW1A 1AA', + 'postcode': 'SW1A 1AA', }), ]) def test_send_one_off_letter_address_populates_address_fields_in_session( @@ -2271,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, @@ -2309,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', From 54d7c6fcbda3f82de137676cbc4ea7acf449b89c Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 3 Apr 2020 14:13:44 +0100 Subject: [PATCH 3/3] Get ready to error for precompiled letters with bad addresses Template preview is going to start returning these errors; we need to be ready to handle them. --- app/utils.py | 23 +++++++++++++++++++++++ tests/app/test_utils.py | 22 ++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/app/utils.py b/app/utils.py index 8c1d737af..59f236101 100644 --- a/app/utils.py +++ b/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.' + ), } } diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index 58c1a3079..b91e473d6 100644 --- a/tests/app/test_utils.py +++ b/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,