- Your file needs {{ ( - recipients.missing_column_headers - if template.template_type == 'letter' else required_recipient_columns - ) | formatted_list( - prefix='a column called', - prefix_plural='columns called' - ) }}. + Your file needs at least 3 address columns, for example ‘address line 1’, + ‘address line 2’ and ‘address line 3’.
+ {% else %} ++ Your file needs a column called ‘{{ first_recipient_column }}’. +
+ {% endif %}Right now it has {{ recipients.column_headers | formatted_list( prefix='one column, called ', diff --git a/app/templates/views/check/row-errors.html b/app/templates/views/check/row-errors.html index 3ddc43b29..d738af271 100644 --- a/app/templates/views/check/row-errors.html +++ b/app/templates/views/check/row-errors.html @@ -80,6 +80,16 @@ No content for this message {% elif item.message_too_long %} Message is too long + {% elif not item.as_postal_address.has_enough_lines %} + 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.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/app/utils.py b/app/utils.py index d4e92c6cd..1c8b69eb3 100644 --- a/app/utils.py +++ b/app/utils.py @@ -151,6 +151,15 @@ def get_errors_for_csv(recipients, template_type): return errors +def get_sample_template(template_type): + if template_type == 'email': + return EmailPreviewTemplate({'content': 'any', 'subject': '', 'template_type': 'email'}) + if template_type == 'sms': + return SMSPreviewTemplate({'content': 'any', 'template_type': 'sms'}) + if template_type == 'letter': + return LetterImageTemplate({'content': 'any', 'subject': '', 'template_type': 'letter'}) + + def generate_notifications_csv(**kwargs): from app import notification_api_client from app.s3_client.s3_csv_client import s3download @@ -161,7 +170,7 @@ def generate_notifications_csv(**kwargs): original_file_contents = s3download(kwargs['service_id'], kwargs['job_id']) original_upload = RecipientCSV( original_file_contents, - template_type=kwargs['template_type'], + template=get_sample_template(kwargs['template_type']), ) original_column_headers = original_upload.column_headers fieldnames = ['Row number'] + original_column_headers + ['Template', 'Type', 'Job', 'Status', 'Time'] diff --git a/requirements-app.txt b/requirements-app.txt index ccdd43377..9992846f5 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -23,5 +23,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@37.3.0#egg=notifications-utils==37.3.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 6accb0ad5..f17dd12fd 100644 --- a/requirements.txt +++ b/requirements.txt @@ -25,7 +25,7 @@ notifications-python-client==5.5.1 awscli-cwlogs>=1.4,<1.5 itsdangerous==1.1.0 -git+https://github.com/alphagov/notifications-utils.git@37.3.0#egg=notifications-utils==37.3.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: diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 876cac5bd..3e1ac453f 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -19,6 +19,7 @@ from notifications_utils.recipients import RecipientCSV from notifications_utils.template import ( LetterImageTemplate, LetterPreviewTemplate, + SMSPreviewTemplate, ) from xlrd.biffh import XLRDError from xlrd.xldate import ( @@ -309,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))) + @@ -556,6 +588,116 @@ def test_upload_csv_file_with_very_long_placeholder_shows_check_page_with_errors assert page.select('tbody tr td')[1]['colspan'] == '2' +def test_upload_csv_file_with_bad_postal_address_shows_check_page_with_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, +): + 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., SW!A !AA + Firstname Lastname, 123 Example St., France + , 123 Example St., SW!A !AA + "1\n2\n3\n4\n5\n6\n7\n8" + ''' + ) + + 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 4 addresses. ' + 'Skip to file contents' + ) + assert [ + normalize_spaces(row.text) for row in page.select('tbody tr') + ] == [ + '3 Last line of the address must be a real UK postcode', + 'Firstname Lastname 123 Example St. SW!A !AA', + + '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', + + '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,', [ ( """ @@ -2420,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'] = {} @@ -2575,7 +2742,9 @@ def test_upload_csvfile_with_international_validates( mocker.patch('app.main.views.send.s3download', return_value='') mock_recipients = mocker.patch( 'app.main.views.send.RecipientCSV', - return_value=RecipientCSV("", template_type="sms"), + return_value=RecipientCSV("", template=SMSPreviewTemplate( + {'content': 'foo', 'template_type': 'sms'} + )), ) response = logged_in_client.post( @@ -2586,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( @@ -3381,7 +3550,7 @@ def test_check_messages_shows_data_errors_before_trial_mode_errors_for_letters( assert normalize_spaces(page.select_one('.banner-dangerous').text) == ( 'There’s a problem with example.xlsx ' - 'You need to enter missing data in 2 rows. ' + 'You need to fix 2 addresses. ' 'Skip to file contents' ) assert not page.select('.table-field-index a') @@ -3470,7 +3639,8 @@ def test_check_messages_column_error_doesnt_show_optional_columns( assert normalize_spaces(page.select_one('.banner-dangerous').text) == ( 'There’s a problem with your column names ' - 'Your file needs a column called ‘postcode’. ' + 'Your file needs at least 3 address columns, for example ‘address line 1’, ' + '‘address line 2’ and ‘address line 3’. ' 'Right now it has columns called ‘address_line_1’, ‘address_line_2’ and ‘foo’. ' 'Skip to file contents' ) 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):