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):