From 2ff6cf049f9e4e8be847c8d188cb75298a83583d Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 3 Jun 2016 13:52:15 +0100 Subject: [PATCH] Change order of errors for bad CSV files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit rearranges the CSV errors (again) to make them geared towards teaching you how to match placeholders to the column headers. So the order of errors now is: 1. No phone number/email column 2. Column headers don’t match placeholders 3. Missing or bad data 4. Trial mode 5. Daily limit reached This depends on: - [x] https://github.com/alphagov/notifications-utils/pull/39 for 1. --- app/main/views/send.py | 4 +- app/templates/views/check.html | 124 +++++++++++++++++++------- app/utils.py | 14 --- requirements.txt | 2 +- tests/app/main/test_errors_for_csv.py | 28 +++--- tests/app/main/views/test_send.py | 6 +- 6 files changed, 109 insertions(+), 69 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 7fba4f432..237e73337 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -270,8 +270,8 @@ def check_messages(service_id, template_type, upload_id): first_recipient=first_recipient, template=template, page_heading=get_page_headings(template.template_type), - errors=get_errors_for_csv(recipients, template.template_type), - rows_have_errors=any(recipients.rows_with_errors), + errors=recipients.has_errors, + row_errors=get_errors_for_csv(recipients, template.template_type), count_of_recipients=session['upload_data']['notification_count'], count_of_displayed_recipients=( len(list(recipients.initial_annotated_rows_with_errors)) diff --git a/app/templates/views/check.html b/app/templates/views/check.html index 71631c1d2..1b55742e5 100644 --- a/app/templates/views/check.html +++ b/app/templates/views/check.html @@ -7,13 +7,95 @@ {% from "components/file-upload.html" import file_upload %} {% from "components/page-footer.html" import page_footer %} +{% macro list_of_columns(columns, conjunction='and') %} + {% if columns|length == 1 %} + ‘{{ (columns|list)[0] }}’ + {% else %} + {{ + "‘{}’ {} ‘{}’".format( + "’, ‘".join((columns|list)[0:-1]), + conjunction, + (columns|list)[-1] + ) + }} + {%- endif %} +{%- endmacro %} + {% block page_title %} {{ page_heading if errors else "Check and confirm" }} – GOV.UK Notify {% endblock %} {% block maincolumn_content %} - {% if not recipients.allowed_to_send_to and not recipients.missing_column_headers %} + {% if not recipients.has_recipient_column %} + +
+ {% call banner_wrapper(type='dangerous') %} +

+ Your file needs to have a column called ‘{{ recipients.recipient_column_header }}’ +

+

+ Your file has {{ formatted_list( + recipients.column_headers, + prefix='one column, called', + prefix_plural='columns called' + ) }}. +

+ {% endcall %} +
+ + {% elif recipients.missing_column_headers %} + +
+ {% call banner_wrapper(type='dangerous') %} +

+ The columns in your file need to match the double brackets in + your template +

+

+ Your file has columns called {{ list_of_columns(recipients.column_headers) }}. +

+

+ It doesn’t have + {% if recipients.missing_column_headers|length == 1 %} + a column + {% else %} + columns + {% endif %} + called {{ list_of_columns(recipients.missing_column_headers, 'or') }}. +

+ {% endcall %} +
+ + {% elif row_errors %} + +
+ {% call banner_wrapper(type='dangerous') %} + {% if row_errors|length == 1 %} +

+ There is a problem with your data +

+

+ You need to {{ row_errors[0] }} +

+ {% else %} +

+ There are some problems with your data +

+

+ You need to: +

+ + {% endif %} + {% endcall %} +
+ + {% elif not recipients.allowed_to_send_to %} +
{% call banner_wrapper(type='dangerous') %}

@@ -30,32 +112,9 @@

{% endcall %}

- {% elif errors %} -
- {% call banner_wrapper(type='dangerous') %} - {% if errors|length == 1 %} -

- There was a problem with {{ original_file_name }} -

-

- You need to {{ errors[0] }} -

- {% else %} -

- There were some problems with {{ original_file_name }} -

-

- You need to: -

- - {% endif %} - {% endcall %} -
+ {% elif count_of_recipients > (current_service.message_limit - statistics.get('emails_requested', 0) - statistics.get('sms_requested', 0)) %} +
{% call banner_wrapper(type='dangerous') %}

@@ -73,20 +132,23 @@ {% endif %}

You can still send - {{ current_service.message_limit - statistics.emails_requested - statistics.sms_requested }} + {{ current_service.message_limit - statistics.get('emails_requested', 0) - statistics.get('sms_requested', 0) }} messages today, but ‘{{ original_file_name }}’ contains {{ count_of_recipients }} {{ recipients.recipient_column_header }} {%- if count_of_recipients != 1 -%} {{ 'es' if 'email address' == recipients.recipient_column_header else 's' }} - {%- endif -%}. + {%- endif %}

{% endcall %}

+ {% else %} +

Check and confirm

+ {% endif %} {% if 'email' == template.template_type %} @@ -129,7 +191,7 @@ {% endif %} {% call(item, row_number) list_table( - recipients.initial_annotated_rows_with_errors if rows_have_errors else recipients.initial_annotated_rows, + recipients.initial_annotated_rows_with_errors if row_errors else recipients.initial_annotated_rows, caption=original_file_name, field_headings=['1'] + recipients.column_headers ) %} @@ -161,13 +223,13 @@ {% if count_of_displayed_recipients < count_of_recipients %}

- {% if rows_have_errors %} + {% if row_errors %} Only showing the first {{ count_of_displayed_recipients }} rows with errors {% else %} Only showing the first {{ count_of_displayed_recipients }} rows {% endif %}

- {% elif rows_have_errors %} + {% elif row_errors %}

Only showing rows with errors

diff --git a/app/utils.py b/app/utils.py index e24fcef7e..052c52981 100644 --- a/app/utils.py +++ b/app/utils.py @@ -55,20 +55,6 @@ def get_errors_for_csv(recipients, template_type): errors = [] - missing_column_headers = list(recipients.missing_column_headers) - - if len(missing_column_headers) == 1: - errors.append("add a column called ‘{}’".format("".join(missing_column_headers))) - elif len(missing_column_headers) == 2: - errors.append("add 2 columns, ‘{}’".format("’ and ‘".join(missing_column_headers))) - elif len(missing_column_headers) > 2: - errors.append( - "add columns called ‘{}’, and ‘{}’".format( - "’, ‘".join(missing_column_headers[0:-1]), - missing_column_headers[-1] - ) - ) - if recipients.rows_with_bad_recipients: number_of_bad_recipients = len(list(recipients.rows_with_bad_recipients)) if 'sms' == template_type: diff --git a/requirements.txt b/requirements.txt index b04c2c4ad..c33f0f1b0 100644 --- a/requirements.txt +++ b/requirements.txt @@ -18,4 +18,4 @@ pytz==2016.4 git+https://github.com/alphagov/notifications-python-client.git@1.0.0#egg=notifications-python-client==1.0.0 -git+https://github.com/alphagov/notifications-utils.git@5.4.0#egg=notifications-utils==5.4.0 +git+https://github.com/alphagov/notifications-utils.git@6.1.0#egg=notifications-utils==6.1.0 diff --git a/tests/app/main/test_errors_for_csv.py b/tests/app/main/test_errors_for_csv.py index 3ea2cf6f4..b61be13bc 100644 --- a/tests/app/main/test_errors_for_csv.py +++ b/tests/app/main/test_errors_for_csv.py @@ -8,7 +8,6 @@ from app.utils import get_errors_for_csv MockRecipients = namedtuple( 'RecipientCSV', [ - 'missing_column_headers', 'rows_with_bad_recipients', 'rows_with_missing_data' ] @@ -16,52 +15,45 @@ MockRecipients = namedtuple( @pytest.mark.parametrize( - "missing_column_headers,rows_with_bad_recipients,rows_with_missing_data,template_type,expected_errors", + "rows_with_bad_recipients,rows_with_missing_data,template_type,expected_errors", [ ( - [], [], [], + [], [], 'sms', [] ), ( - [], {2}, [], + {2}, [], 'sms', ['fix 1 phone number'] ), ( - [], {2, 4, 6}, [], + {2, 4, 6}, [], 'sms', ['fix 3 phone numbers'] ), ( - [], {1}, [], + {1}, [], 'email', ['fix 1 email address'] ), ( - [], {2, 4, 6}, [], + {2, 4, 6}, [], 'email', ['fix 3 email addresses'] ), ( - ['name'], {2}, {3}, + {2}, {3}, 'sms', [ - 'add a column called ‘name’', 'fix 1 phone number', 'enter missing data in 1 row' ] ), ( - ['name', 'date'], [], [], - 'sms', - ['add 2 columns, ‘name’ and ‘date’'] - ), - ( - ['name', 'date', 'time'], {2, 4, 6, 8}, {3, 6, 9, 12}, + {2, 4, 6, 8}, {3, 6, 9, 12}, 'sms', [ - 'add columns called ‘name’, ‘date’, and ‘time’', 'fix 4 phone numbers', 'enter missing data in 4 rows' ] @@ -69,11 +61,11 @@ MockRecipients = namedtuple( ] ) def test_get_errors_for_csv( - missing_column_headers, rows_with_bad_recipients, rows_with_missing_data, + rows_with_bad_recipients, rows_with_missing_data, template_type, expected_errors ): assert get_errors_for_csv( - MockRecipients(missing_column_headers, rows_with_bad_recipients, rows_with_missing_data), + MockRecipients(rows_with_bad_recipients, rows_with_missing_data), template_type ) == expected_errors diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 827502b26..c4bedf428 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -107,7 +107,7 @@ def test_upload_csvfile_with_errors_shows_check_page_with_errors( for response in [initial_upload, reupload]: assert response.status_code == 200 content = response.get_data(as_text=True) - assert 'There was a problem with invalid.csv' in content + assert 'There is a problem with your data' in content assert '+447700900986' in content assert 'Missing' in content assert 'Re-upload your file' in content @@ -314,7 +314,7 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers( assert '07700 900701' in content assert '07700 900749' in content assert '07700 900750' not in content - assert 'Only showing the first 50 rows with errors' in content + assert 'Only showing the first 50 rows' in content def test_create_job_should_call_api( @@ -391,7 +391,7 @@ def test_check_messages_should_revalidate_file_when_uploading_file( follow_redirects=True ) assert response.status_code == 200 - assert 'There was a problem with invalid.csv' in response.get_data(as_text=True) + assert 'There is a problem with your data' in response.get_data(as_text=True) def test_route_permissions(mocker,