From 06108de0f7a3b5ea9a0383e2eeed73bc4a120dfe Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 27 Apr 2020 10:57:25 +0100 Subject: [PATCH] Allow international addresses in spreadsheets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For services with permission, they can now put international addresses into their spreadsheets without getting a postcode error. This also means they can start using address line 7 instead of postcode, since it doesn’t make sense to put a country in a field called ‘postcode’. But this will be undocumented to start with, because we’re not giving any real users the permission. It does now mean that the number of possible placeholders (7 + postcode) is greater than the number of allowed placeholders (7), so we have to account for that in the one-off address flow where we’re populating the placeholders automatically. We’re sticking with 6 + postcode here for backwards compatibility. --- app/main/forms.py | 15 ++- app/main/views/send.py | 41 +++++-- app/main/views/uploads.py | 2 +- app/models/contact_list.py | 2 +- app/templates/views/check/row-errors.html | 8 +- requirements-app.txt | 2 +- requirements.txt | 8 +- tests/app/main/views/test_send.py | 130 ++++++++++++++++++++-- tests/conftest.py | 19 ++++ 9 files changed, 197 insertions(+), 30 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index a4455a45b..74654cb7f 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -754,6 +754,10 @@ class SMSTemplateForm(BaseTemplateForm): class LetterAddressForm(StripWhitespaceForm): + def __init__(self, *args, allow_international_letters=False, **kwargs): + self.allow_international_letters = allow_international_letters + super().__init__(*args, **kwargs) + address = PostalAddressField( 'Address', validators=[DataRequired(message="Cannot be empty")] @@ -761,7 +765,10 @@ class LetterAddressForm(StripWhitespaceForm): def validate_address(self, field): - address = PostalAddress(field.data) + address = PostalAddress( + field.data, + allow_international_letters=self.allow_international_letters, + ) if not address.has_enough_lines: raise ValidationError( @@ -773,7 +780,11 @@ class LetterAddressForm(StripWhitespaceForm): f'Address must be no more than {PostalAddress.MAX_LINES} lines long' ) - if not address.postcode: + if not address.has_valid_last_line: + if self.allow_international_letters: + raise ValidationError( + f'Last line of the address must be a UK postcode or another country' + ) raise ValidationError( f'Last line of the address must be a real UK postcode' ) diff --git a/app/main/views/send.py b/app/main/views/send.py index 81f4eea49..ee4e0e5c5 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -18,7 +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 +from notifications_utils.postal_address import ( + PostalAddress, + address_lines_1_to_6_and_postcode_keys, +) from notifications_utils.recipients import RecipientCSV, first_column_headings from notifications_utils.sanitise_text import SanitiseASCII from xlrd.biffh import XLRDError @@ -59,6 +62,11 @@ from app.utils import ( user_has_permissions, ) +letter_address_columns = [ + column.replace('_', ' ') + for column in address_lines_1_to_6_and_postcode_keys +] + def get_example_csv_fields(column_headers, use_example_as_example, submitted_fields): if use_example_as_example: @@ -77,7 +85,7 @@ def get_example_csv_rows(template, use_example_as_example=True, submitted_fields (submitted_fields or {}).get( key, get_example_letter_address(key) if use_example_as_example else key ) - for key in first_column_headings['letter'] + for key in letter_address_columns ] }[template.template_type] + get_example_csv_fields( ( @@ -364,7 +372,10 @@ def send_one_off_letter_address(service_id, template_id): get_normalised_placeholders_from_session() ) - form = LetterAddressForm(address=current_session_address.normalised) + form = LetterAddressForm( + address=current_session_address.normalised, + allow_international_letters=current_service.has_permission('international_letters'), + ) if form.validate_on_submit(): session['placeholders'].update(PostalAddress(form.address.data).as_personalisation) @@ -376,7 +387,8 @@ def send_one_off_letter_address(service_id, template_id): if all_placeholders_in_session(placeholders): return get_notification_check_endpoint(service_id, template) - first_non_address_placeholder_index = len(first_column_headings['letter']) + first_non_address_placeholder_index = len(address_lines_1_to_6_and_postcode_keys) + return redirect(url_for( 'main.send_one_off_step', service_id=service_id, @@ -468,7 +480,7 @@ 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 template.template_type == 'letter': - if step_index < len(first_column_headings['letter']): + if step_index < len(address_lines_1_to_6_and_postcode_keys): return redirect(url_for( '.send_one_off_letter_address', service_id=service_id, @@ -665,7 +677,8 @@ def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_ [user.name, user.mobile_number, user.email_address] for user in Users(service_id) ) if current_service.trial_mode else None, remaining_messages=remaining_messages, - international_sms=current_service.has_permission('international_sms'), + allow_international_sms=current_service.has_permission('international_sms'), + allow_international_letters=current_service.has_permission('international_letters'), ) if request.args.get('from_test'): @@ -845,10 +858,12 @@ def go_to_dashboard_after_tour(service_id, example_template_id): def fields_to_fill_in(template, prefill_current_user=False): - recipient_columns = first_column_headings[template.template_type] + if 'letter' == template.template_type: + return letter_address_columns + list(template.placeholders) + + if not prefill_current_user: + return first_column_headings[template.template_type] + list(template.placeholders) - if 'letter' == template.template_type or not prefill_current_user: - return recipient_columns + list(template.placeholders) if template.template_type == 'sms': session['recipient'] = current_user.mobile_number session['placeholders']['phone number'] = current_user.mobile_number @@ -1126,8 +1141,14 @@ def get_sms_sender_from_session(): def get_spreadsheet_column_headings_from_template(template): column_headings = [] + if template.template_type == 'letter': + # We want to avoid showing `address line 7` for now + recipient_columns = letter_address_columns + else: + recipient_columns = first_column_headings[template.template_type] + for column_heading in ( - first_column_headings[template.template_type] + list(template.placeholders) + recipient_columns + list(template.placeholders) ): if column_heading not in Columns.from_keys(column_headings): column_headings.append(column_heading) diff --git a/app/main/views/uploads.py b/app/main/views/uploads.py index 1723ec004..5166d1ed8 100644 --- a/app/main/views/uploads.py +++ b/app/main/views/uploads.py @@ -363,7 +363,7 @@ def check_contact_list(service_id, upload_id): [user.name, user.mobile_number, user.email_address] for user in current_service.active_users ) if current_service.trial_mode else None, - international_sms=current_service.has_permission('international_sms'), + allow_international_sms=current_service.has_permission('international_sms'), max_initial_rows_shown=50, max_errors_shown=50, ) diff --git a/app/models/contact_list.py b/app/models/contact_list.py index 222d0108b..9d4dfaaa7 100644 --- a/app/models/contact_list.py +++ b/app/models/contact_list.py @@ -121,7 +121,7 @@ class ContactList(JSONModel): return RecipientCSV( self.contents, template=get_sample_template(self.template_type), - international_sms=True, + allow_international_sms=True, max_initial_rows_shown=50, ) diff --git a/app/templates/views/check/row-errors.html b/app/templates/views/check/row-errors.html index 9894a04d4..d738af271 100644 --- a/app/templates/views/check/row-errors.html +++ b/app/templates/views/check/row-errors.html @@ -84,8 +84,12 @@ Address must be at least {{ letter_min_address_lines }} lines long {% elif item.as_postal_address.has_too_many_lines %} Address must be no more than {{ letter_max_address_lines }} lines long - {% elif not item.as_postal_address.postcode %} - Last line of the address must be a real UK postcode + {% elif not item.as_postal_address.has_valid_last_line %} + {% if item.as_postal_address.allow_international_letters %} + Last line of the address must be a UK postcode or another country + {% else %} + Last line of the address must be a real UK postcode + {% endif %} {% endif %} {% endcall %} diff --git a/requirements-app.txt b/requirements-app.txt index df0c90148..8b0cffedf 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -24,5 +24,5 @@ WTForms==2.2.1 # Pinned because of breaking change in 2.3.0 awscli-cwlogs>=1.4,<1.5 itsdangerous==1.1.0 -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 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 b81e710cd..2e46df7cf 100644 --- a/requirements.txt +++ b/requirements.txt @@ -26,17 +26,17 @@ WTForms==2.2.1 # Pinned because of breaking change in 2.3.0 awscli-cwlogs>=1.4,<1.5 itsdangerous==1.1.0 -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 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.46 +awscli==1.18.48 bleach==3.1.4 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 docopt==0.6.2 diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index a5aaed2da..55c3369a0 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -310,6 +310,37 @@ def test_example_spreadsheet( ) +def test_example_spreadsheet_for_letters( + client_request, + mocker, + mock_get_service_letter_template_with_placeholders, + fake_uuid, +): + mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=1) + + page = client_request.get( + '.send_messages', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid + ) + + assert list(zip(*[ + [normalize_spaces(cell.text) for cell in page.select('tbody tr')[row].select('td')] + for row in (0, 1) + ])) == [ + ('1', '2'), + ('address line 1', 'A. Name'), + ('address line 2', '123 Example Street'), + ('address line 3', ''), + ('address line 4', ''), + ('address line 5', ''), + ('address line 6', ''), + ('postcode', 'XM4 5HQ'), + ('name', 'example'), + ('date', 'example'), + ] + + @pytest.mark.parametrize( "filename, acceptable_file", list(zip(test_spreadsheet_files, repeat(True))) + @@ -576,6 +607,7 @@ def test_upload_csv_file_with_bad_postal_address_shows_check_page_with_errors( address line 1, address line 3, address line 6, Firstname Lastname, 123 Example St., SW1A 1AA Firstname Lastname, 123 Example St., SW!A !AA + Firstname Lastname, 123 Example St., France , 123 Example St., SW!A !AA "1\n2\n3\n4\n5\n6\n7\n8" ''' @@ -594,7 +626,7 @@ def test_upload_csv_file_with_bad_postal_address_shows_check_page_with_errors( page.select_one('.banner-dangerous').text ) == ( 'There’s a problem with invalid.csv ' - 'You need to fix 3 addresses. ' + 'You need to fix 4 addresses. ' 'Skip to file contents' ) assert [ @@ -603,14 +635,69 @@ def test_upload_csv_file_with_bad_postal_address_shows_check_page_with_errors( '3 Last line of the address must be a real UK postcode', 'Firstname Lastname 123 Example St. SW!A !AA', - '4 Address must be at least 3 lines long', + '4 Last line of the address must be a real UK postcode', + 'Firstname Lastname 123 Example St. France', + + '5 Address must be at least 3 lines long', '123 Example St. SW!A !AA', - '5 Address must be no more than 7 lines long', + '6 Address must be no more than 7 lines long', '1 2 3 4 5 6 7 8', ] +def test_upload_csv_file_with_international_letters_permission_shows_appropriate_errors( + logged_in_client, + service_one, + mocker, + mock_get_service_letter_template, + mock_s3_upload, + mock_get_users_by_service, + mock_get_service_statistics, + mock_get_job_doesnt_exist, + mock_get_jobs, + fake_uuid, +): + service_one['permissions'] += ['international_letters'] + mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=9) + mocker.patch( + 'app.main.views.send.s3download', + return_value=''' + address line 1, address line 3, address line 6, + Firstname Lastname, 123 Example St., SW1A 1AA + Firstname Lastname, 123 Example St., France + Firstname Lastname, 123 Example St., SW!A !AA + Firstname Lastname, 123 Example St., Not France + ''' + ) + + response = logged_in_client.post( + url_for('main.send_messages', service_id=service_one['id'], template_id=fake_uuid), + data={'file': (BytesIO(''.encode('utf-8')), 'invalid.csv')}, + content_type='multipart/form-data', + follow_redirects=True + ) + + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert normalize_spaces( + page.select_one('.banner-dangerous').text + ) == ( + 'There’s a problem with invalid.csv ' + 'You need to fix 2 addresses. ' + 'Skip to file contents' + ) + assert [ + normalize_spaces(row.text) for row in page.select('tbody tr') + ] == [ + '4 Last line of the address must be a UK postcode or another country', + 'Firstname Lastname 123 Example St. SW!A !AA', + + '5 Last line of the address must be a UK postcode or another country', + 'Firstname Lastname 123 Example St. Not France', + ] + + @pytest.mark.parametrize('file_contents, expected_error,', [ ( """ @@ -2475,20 +2562,45 @@ def test_send_one_off_letter_address_populates_address_fields_in_session( assert session['placeholders'] == expected_placeholders -@pytest.mark.parametrize(['form_data', 'expected_error_message'], [ - ('', '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'), +@pytest.mark.parametrize('form_data, extra_permissions, expected_error_message', [ + ( + '', + [], + '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', + ), + ( + '\n'.join(['a', 'b', 'c', 'd', 'e', 'f', 'g']), + ['international_letters'], + 'Last line of the address must be a UK postcode or another country', + ), ]) def test_send_one_off_letter_address_rejects_bad_addresses( client_request, + service_one, fake_uuid, mock_get_service_letter_template, mock_template_preview, form_data, + extra_permissions, expected_error_message ): + service_one['permissions'] += extra_permissions + with client_request.session_transaction() as session: session['recipient'] = None session['placeholders'] = {} @@ -2643,7 +2755,7 @@ def test_upload_csvfile_with_international_validates( ) assert response.status_code == 200 - assert mock_recipients.call_args[1]['international_sms'] == should_allow_international + assert mock_recipients.call_args[1]['allow_international_sms'] == should_allow_international def test_job_from_contact_list_knows_where_its_come_from( diff --git a/tests/conftest.py b/tests/conftest.py index d67a138b9..15620aa71 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -878,6 +878,25 @@ def mock_get_service_letter_template(mocker): ) +@pytest.fixture(scope='function') +def mock_get_service_letter_template_with_placeholders(mocker): + def _get(service_id, template_id, version=None, postage='second'): + template = template_json( + service_id, + template_id, + name="Two week reminder", + type_="letter", + content="Hello ((name)) your thing is due on ((date))", + subject="Subject", + postage=postage, + ) + return {'data': template} + + return mocker.patch( + 'app.service_api_client.get_service_template', side_effect=_get + ) + + @pytest.fixture(scope='function') def mock_create_service_template(mocker, fake_uuid): def _create(name, type_, content, service, subject=None, process_type=None, parent_folder_id=None):