diff --git a/app/main/views/send.py b/app/main/views/send.py index 7fba4f432..a850c6ac1 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', @@ -269,17 +256,15 @@ 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=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)) - 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'), - 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/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/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 71631c1d2..fbabfbbe3 100644 --- a/app/templates/views/check.html +++ b/app/templates/views/check.html @@ -6,14 +6,87 @@ {% from "components/placeholder.html" import placeholder %} {% 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 + {{ "Error" 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 {{ formatted_list( + recipients.column_headers, + prefix='one column, called', + prefix_plural='columns called' + ) }}. +

+

+ It doesn’t have {{ formatted_list( + recipients.missing_column_headers, + conjunction='or', + prefix='a column called', + prefix_plural='columns called' + ) }}. +

+ {% 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 +103,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 +123,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 %} @@ -123,19 +176,19 @@ {% 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 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 }} @@ -161,13 +214,13 @@ {% if count_of_displayed_recipients < count_of_recipients %} - {% elif rows_have_errors %} + {% elif row_errors and not recipients.missing_column_headers %} 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 %} 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,