From 2ff6cf049f9e4e8be847c8d188cb75298a83583d Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 3 Jun 2016 13:52:15 +0100 Subject: [PATCH 1/5] 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, From 3ace856d74058d6f0a488929f196ebb3ea349c44 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 3 Jun 2016 14:38:10 +0100 Subject: [PATCH 2/5] Only show row-level errors if all columns are OK MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Row-level errors are: - bad phone number/email address - missing data I think it’s distracting to show these on the page if there’s something more fundamentally wrong with the file, eg placeholders don’t match. So this commit makes sure that these error messages are only displayed when the top-level error says ‘There is a problem with your data’ --- app/main/views/send.py | 2 +- app/templates/views/check.html | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 237e73337..e7a8a9447 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -275,7 +275,7 @@ def check_messages(service_id, template_type, upload_id): count_of_recipients=session['upload_data']['notification_count'], count_of_displayed_recipients=( len(list(recipients.initial_annotated_rows_with_errors)) - if any(recipients.rows_with_errors) else + if any(recipients.rows_with_errors) and not recipients.missing_column_headers else len(list(recipients.initial_annotated_rows)) ), original_file_name=session['upload_data'].get('original_file_name'), diff --git a/app/templates/views/check.html b/app/templates/views/check.html index 1b55742e5..1eec4a52c 100644 --- a/app/templates/views/check.html +++ b/app/templates/views/check.html @@ -191,13 +191,13 @@ {% endif %} {% call(item, row_number) list_table( - recipients.initial_annotated_rows_with_errors if row_errors else recipients.initial_annotated_rows, + recipients.initial_annotated_rows_with_errors if row_errors and not recipients.missing_column_headers else recipients.initial_annotated_rows, caption=original_file_name, field_headings=['1'] + recipients.column_headers ) %} {{ index_field(item.index + 2) }} {% for column in recipients.column_headers %} - {% if item['columns'][column].error %} + {% if item['columns'][column].error and not recipients.missing_column_headers %} {% call field() %} {{ item['columns'][column].error }} @@ -223,13 +223,13 @@ {% if count_of_displayed_recipients < count_of_recipients %}

- {% if row_errors %} + {% if row_errors and not recipients.missing_column_headers %} Only showing the first {{ count_of_displayed_recipients }} rows with errors {% else %} Only showing the first {{ count_of_displayed_recipients }} rows {% endif %}

- {% elif row_errors %} + {% elif row_errors and not recipients.missing_column_headers %}

Only showing rows with errors

From 9332f57f55ca4c8bb9c28eb218d62933c3a9c3c8 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 3 Jun 2016 14:30:17 +0100 Subject: [PATCH 3/5] Make components for nicely formatted lists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We have a couple of places now where we want nice lists made from `list`s, eg - ‘name’, ‘date’ and ‘phone number’ - ((firstname)) ((lastname)) or ((date)) This commit adds a more generic component for doing this, which can handle: - 1, 2, and n items - comma (or other character) separated lists - a conjunction between the last and one-before-last item - characters to be inserted before and after each item, eg an opening and closing HTML tag It also pulls the `list_of_placeholders` component from the breaking change page, and makes it use the `formatted_list` component under the hood. --- app/templates/components/list.html | 37 +++++++++++++++++++ app/templates/views/check.html | 34 ++++++----------- app/templates/views/styleguide.html | 20 ++++++++++ .../views/templates/breaking-change.html | 11 +----- 4 files changed, 71 insertions(+), 31 deletions(-) create mode 100644 app/templates/components/list.html diff --git a/app/templates/components/list.html b/app/templates/components/list.html new file mode 100644 index 000000000..26eb8e6ee --- /dev/null +++ b/app/templates/components/list.html @@ -0,0 +1,37 @@ +{% macro formatted_list( + items, + conjunction='and', + before_each='‘', + after_each='’', + separator=', ', + prefix='', + prefix_plural='' +) %} + {% if items|length == 1 %} + {{ prefix }} {{ before_each|safe }}{{ (items|list)[0] }}{{ after_each|safe }} + {% elif items %} + {{ prefix_plural }} + {% for item in (items|list)[0:-1] -%} + {{ before_each|safe -}} + {{ item -}} + {{ after_each|safe -}} + {% if not loop.last -%} + {{ separator -}} + {% endif -%} + {% endfor %} + {{ conjunction }} + {{ before_each|safe -}} + {{ (items|list)[-1] -}} + {{ after_each|safe }} + {%- endif %} +{%- endmacro %} + + +{% macro list_of_placeholders(placeholders) %} + {{ formatted_list( + placeholders, + before_each="((", + after_each='))', + separator=' ' + ) }} +{%- endmacro %} diff --git a/app/templates/views/check.html b/app/templates/views/check.html index 1eec4a52c..e36d913fb 100644 --- a/app/templates/views/check.html +++ b/app/templates/views/check.html @@ -6,20 +6,7 @@ {% from "components/placeholder.html" import placeholder %} {% 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 %} +{% from "components/list.html" import formatted_list %} {% block page_title %} {{ page_heading if errors else "Check and confirm" }} – GOV.UK Notify @@ -53,16 +40,19 @@ your template

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

- 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') }}. + It doesn’t have {{ formatted_list( + recipients.missing_column_headers, + conjunction='or', + prefix='a column called', + prefix_plural='columns called' + ) }}.

{% endcall %} diff --git a/app/templates/views/styleguide.html b/app/templates/views/styleguide.html index 59859b151..5a5ce64e4 100644 --- a/app/templates/views/styleguide.html +++ b/app/templates/views/styleguide.html @@ -10,6 +10,7 @@ {% from "components/textbox.html" import textbox %} {% from "components/file-upload.html" import file_upload %} {% from "components/api-key.html" import api_key %} +{% from "components/list.html" import formatted_list %} {% block page_title %} Styleguide – GOV.UK Notify @@ -230,4 +231,23 @@ {{ api_key('d30512af92e1386d63b90e5973b49a10') }} + +

Formatted list

+ +

+ {{ formatted_list('A', prefix="one item called") }} +

+ +

+ {{ formatted_list('AB', prefix_plural="two items called") }} +

+ +

+ {{ formatted_list('ABC') }} +

+ +

+ {{ formatted_list('ABCD', before_each='', after_each='', conjunction='or') }} +

+ {% endblock %} diff --git a/app/templates/views/templates/breaking-change.html b/app/templates/views/templates/breaking-change.html index 33ef94231..27eeafc8b 100644 --- a/app/templates/views/templates/breaking-change.html +++ b/app/templates/views/templates/breaking-change.html @@ -2,13 +2,7 @@ {% from "components/banner.html" import banner_wrapper %} {% from "components/page-footer.html" import page_footer %} {% from "components/table.html" import list_table, text_field, index_field, index_field_heading %} - -{% macro list_of_placeholders(placeholders) %} - {% for placeholder in placeholders %} - {% if loop.last and loop.length > 1 %} and {% endif %} - (({{ placeholder }})) - {% endfor %} -{% endmacro %} +{% from "components/list.html" import list_of_placeholders %} {% block page_title %} GOV.UK Notify @@ -21,8 +15,7 @@
{% if template_change.placeholders_removed %}

- You removed - {{ list_of_placeholders(template_change.placeholders_removed) }} + You removed {{ list_of_placeholders(template_change.placeholders_removed) }}

{% endif %} {% if template_change.placeholders_added %} From 4ac1a03a8931e50c015eee6d4e77bda9b2fe3037 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 3 Jun 2016 14:48:21 +0100 Subject: [PATCH 4/5] Use message_count_label component MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For the button on the check page, we need to be able to say ‘1 text message’ or ‘55 emails’. We already have the logic to do this on the dashboard (101 text messages sent), and it’s already in a component. So this commit makes the check page use the same component. --- app/main/views/send.py | 14 -------------- app/templates/components/message-count-label.html | 8 ++++---- app/templates/views/check.html | 3 ++- 3 files changed, 6 insertions(+), 19 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index e7a8a9447..b4379973e 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -31,19 +31,6 @@ from app import job_api_client, service_api_client, current_service, user_api_cl from app.utils import user_has_permissions, get_errors_for_csv, Spreadsheet -def get_send_button_text(template_type, number_of_messages): - if 1 == number_of_messages: - return { - 'email': 'Send 1 email', - 'sms': 'Send 1 text message' - }[template_type] - else: - return { - 'email': 'Send {} emails', - 'sms': 'Send {} text messages' - }[template_type].format(number_of_messages) - - def get_page_headings(template_type): return { 'email': 'Email templates', @@ -279,7 +266,6 @@ def check_messages(service_id, template_type, upload_id): len(list(recipients.initial_annotated_rows)) ), original_file_name=session['upload_data'].get('original_file_name'), - send_button_text=get_send_button_text(template.template_type, session['upload_data']['notification_count']), upload_id=upload_id, form=CsvUploadForm(), statistics=statistics, diff --git a/app/templates/components/message-count-label.html b/app/templates/components/message-count-label.html index 532a711d5..a4d59afed 100644 --- a/app/templates/components/message-count-label.html +++ b/app/templates/components/message-count-label.html @@ -1,4 +1,4 @@ -{% macro message_count_label(count, template_type) -%} +{% macro message_count_label(count, template_type, suffix='sent') -%} {%- if template_type == 'sms' -%} {%- if count == 1 -%} text message @@ -10,6 +10,6 @@ email {%- else -%} emails - {%- endif -%} - {%- endif %} sent -{%- endmacro %} \ No newline at end of file + {%- endif -%} + {%- endif %} {{ suffix }} +{%- endmacro %} diff --git a/app/templates/views/check.html b/app/templates/views/check.html index e36d913fb..1f95a4d9e 100644 --- a/app/templates/views/check.html +++ b/app/templates/views/check.html @@ -7,6 +7,7 @@ {% from "components/file-upload.html" import file_upload %} {% from "components/page-footer.html" import page_footer %} {% from "components/list.html" import formatted_list %} +{% from "components/message-count-label.html" import message_count_label %} {% block page_title %} {{ page_heading if errors else "Check and confirm" }} – GOV.UK Notify @@ -175,7 +176,7 @@
- + Back
{% endif %} From a6833f4ad2eae47c13bf1ad35c9c698497332785 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 3 Jun 2016 14:48:34 +0100 Subject: [PATCH 5/5] Change page `` on check page It was no longer accurate. --- app/main/views/send.py | 1 - app/templates/views/check.html | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index b4379973e..a850c6ac1 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -256,7 +256,6 @@ def check_messages(service_id, template_type, upload_id): recipients=recipients, first_recipient=first_recipient, template=template, - page_heading=get_page_headings(template.template_type), errors=recipients.has_errors, row_errors=get_errors_for_csv(recipients, template.template_type), count_of_recipients=session['upload_data']['notification_count'], diff --git a/app/templates/views/check.html b/app/templates/views/check.html index 1f95a4d9e..fbabfbbe3 100644 --- a/app/templates/views/check.html +++ b/app/templates/views/check.html @@ -10,7 +10,7 @@ {% from "components/message-count-label.html" import message_count_label %} {% block page_title %} - {{ page_heading if errors else "Check and confirm" }} – GOV.UK Notify + {{ "Error" if errors else "Check and confirm" }} – GOV.UK Notify {% endblock %} {% block maincolumn_content %}