From f5649d72c94b3333bf4e342b48721cb90aa321a7 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Sun, 19 Apr 2020 22:46:13 +0100 Subject: [PATCH] Explain 3 required address columns Our rules about address columns are relaxing, so that none of them are mandatory any more. Instead you just need any 3 of the 7 to make a valid address. This commit updates our error messaging to reflect that. --- app/main/views/send.py | 11 ++--------- app/main/views/uploads.py | 3 ++- app/models/contact_list.py | 3 ++- app/templates/views/check/column-errors.html | 15 ++++++++------- app/utils.py | 11 ++++++++++- requirements-app.txt | 2 +- requirements.txt | 6 +++--- tests/app/main/views/test_send.py | 10 +++++++--- 8 files changed, 35 insertions(+), 26 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index e166ccfea..58f569815 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -19,13 +19,8 @@ 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.recipients import ( - RecipientCSV, - first_column_headings, - optional_address_columns, -) +from notifications_utils.recipients import RecipientCSV, first_column_headings from notifications_utils.sanitise_text import SanitiseASCII -from orderedset import OrderedSet from xlrd.biffh import XLRDError from xlrd.xldate import XLDateError @@ -663,9 +658,7 @@ def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_ ) recipients = RecipientCSV( contents, - template_type=template.template_type, template=template, - placeholders=template.placeholders, max_initial_rows_shown=50, max_errors_shown=50, whitelist=itertools.chain.from_iterable( @@ -711,7 +704,7 @@ def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_ current_service.trial_mode, template.template_type == 'letter', )), - required_recipient_columns=OrderedSet(recipients.recipient_column_headers) - optional_address_columns, + first_recipient_column=recipients.recipient_column_headers[0], preview_row=preview_row, sent_previously=job_api_client.has_sent_previously( service_id, template.id, db_template['version'], request.args.get('original_file_name', '') diff --git a/app/main/views/uploads.py b/app/main/views/uploads.py index 20cc47f64..1723ec004 100644 --- a/app/main/views/uploads.py +++ b/app/main/views/uploads.py @@ -43,6 +43,7 @@ from app.utils import ( generate_previous_dict, get_errors_for_csv, get_letter_validation_error, + get_sample_template, get_template, unicode_truncate, user_has_permissions, @@ -357,7 +358,7 @@ def check_contact_list(service_id, upload_id): recipients = RecipientCSV( contents, - template_type=template_type or 'sms', + template=get_sample_template(template_type or 'sms'), whitelist=itertools.chain.from_iterable( [user.name, user.mobile_number, user.email_address] for user in current_service.active_users diff --git a/app/models/contact_list.py b/app/models/contact_list.py index 95cfdfdf2..222d0108b 100644 --- a/app/models/contact_list.py +++ b/app/models/contact_list.py @@ -14,6 +14,7 @@ from app.s3_client.s3_csv_client import ( s3upload, set_metadata_on_csv_upload, ) +from app.utils import get_sample_template class ContactList(JSONModel): @@ -119,7 +120,7 @@ class ContactList(JSONModel): def recipients(self): return RecipientCSV( self.contents, - template_type=self.template_type, + template=get_sample_template(self.template_type), international_sms=True, max_initial_rows_shown=50, ) diff --git a/app/templates/views/check/column-errors.html b/app/templates/views/check/column-errors.html index b9e641594..c7e98b311 100644 --- a/app/templates/views/check/column-errors.html +++ b/app/templates/views/check/column-errors.html @@ -58,15 +58,16 @@

There’s a problem with your column names

+ {% if template.template_type == 'letter' %}

- 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/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 17b08111b..df0c90148 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@37.3.0#egg=notifications-utils==37.3.0 +git+https://github.com/alphagov/notifications-utils.git@38.0.0#egg=notifications-utils==38.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 554278870..b81e710cd 100644 --- a/requirements.txt +++ b/requirements.txt @@ -26,14 +26,14 @@ 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@37.3.0#egg=notifications-utils==37.3.0 +git+https://github.com/alphagov/notifications-utils.git@38.0.0#egg=notifications-utils==38.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.43 +awscli==1.18.46 bleach==3.1.4 boto3==1.10.38 -botocore==1.15.43 +botocore==1.15.46 certifi==2020.4.5.1 chardet==3.0.4 click==7.1.1 diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index bdb25c5e2..fd69ad8a9 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 ( @@ -2575,7 +2576,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( @@ -3381,7 +3384,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 +3473,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' )