Merge pull request #646 from alphagov/rearrange-csv-errors

Prioritise CSV errors to help you match placeholders to column headers
2026-07-17 21:20:32 -04:00 · 2016-06-06 12:21:13 +01:00
parent f71780ee27 a6833f4ad2
commit 3ddc1d171d
10 changed files with 167 additions and 101 deletions
--- 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,
--- a/app/templates/components/list.html
+++ 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="<span class='placeholder'>((",
+    after_each='))</span>',
+    separator=' '
+  ) }}
+{%- endmacro %}
--- 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 %}
+    {%- endif -%}
+  {%- endif %} {{ suffix }}
+{%- endmacro %}
--- 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 %}
+
+    <div class="bottom-gutter">
+      {% call banner_wrapper(type='dangerous') %}
+        <h1 class='banner-title'>
+          Your file needs to have a column called ‘{{ recipients.recipient_column_header }}’
+        </h1>
+        <p>
+          Your file has {{ formatted_list(
+            recipients.column_headers,
+            prefix='one column, called',
+            prefix_plural='columns called'
+          ) }}.
+        </p>
+      {% endcall %}
+    </div>
+
+  {% elif recipients.missing_column_headers %}
+
+    <div class="bottom-gutter">
+      {% call banner_wrapper(type='dangerous') %}
+        <h1 class='banner-title'>
+          The columns in your file need to match the double brackets in
+          your template
+        </h1>
+        <p>
+          Your file has {{ formatted_list(
+            recipients.column_headers,
+            prefix='one column, called',
+            prefix_plural='columns called'
+          ) }}.
+        </p>
+        <p>
+          It doesn’t have {{ formatted_list(
+            recipients.missing_column_headers,
+            conjunction='or',
+            prefix='a column called',
+            prefix_plural='columns called'
+          ) }}.
+        </p>
+      {% endcall %}
+    </div>
+
+  {% elif row_errors %}
+
+    <div class="bottom-gutter">
+      {% call banner_wrapper(type='dangerous') %}
+        {% if row_errors|length == 1 %}
+          <h1 class='banner-title'>
+            There is a problem with your data
+          </h1>
+          <p>
+            You need to {{ row_errors[0] }}
+          </p>
+        {% else %}
+          <h1 class='banner-title'>
+            There are some problems with your data
+          </h1>
+          <p>
+            You need to:
+          </p>
+          <ul class="list-bullet">
+            {% for error in row_errors %}
+              <li>{{ error }}</li>
+            {% endfor %}
+          </ul>
+        {% endif %}
+      {% endcall %}
+    </div>
+
+  {% elif not recipients.allowed_to_send_to %}
+
    <div class="bottom-gutter">
      {% call banner_wrapper(type='dangerous') %}
        <h1 class='banner-title'>
@@ -30,32 +103,9 @@
        </p>
      {% endcall %}
    </div>
-  {% elif errors %}
-    <div class="bottom-gutter">
-      {% call banner_wrapper(type='dangerous') %}
-        {% if errors|length == 1 %}
-          <h1 class='banner-title'>
-            There was a problem with {{ original_file_name }}
-          </h1>
-          <p>
-            You need to {{ errors[0] }}
-          </p>
-        {% else %}
-          <h1 class='banner-title'>
-            There were some problems with {{ original_file_name }}
-          </h1>
-          <p>
-            You need to:
-          </p>
-          <ul class="list-bullet">
-            {% for error in errors %}
-              <li>{{ error }}</li>
-            {% endfor %}
-          </ul>
-        {% endif %}
-      {% endcall %}
-    </div>
+
  {% elif count_of_recipients > (current_service.message_limit - statistics.get('emails_requested', 0) - statistics.get('sms_requested', 0)) %}
+
    <div class="bottom-gutter">
      {% call banner_wrapper(type='dangerous') %}
        <h1 class='banner-title'>
@@ -73,20 +123,23 @@
        {% endif %}
        <p>
          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 %}
        </p>
      {% endcall %}
    </div>
+
  {% else %}
+
    <h1 class="heading-large">
      Check and confirm
    </h1>
+
  {% endif %}

  {% if 'email' == template.template_type %}
@@ -123,19 +176,19 @@
    <form method="post" enctype="multipart/form-data" action="{{url_for('main.start_job', service_id=current_service.id, upload_id=upload_id)}}" class='page-footer'>
      <input type="hidden" name="csrf_token" value="{{ csrf_token() }}" />
      <input type="hidden" name="help" value="{{ '3' if request.args['help'] == '2' else 0 }}" />
-      <input type="submit" class="button" value="{{ send_button_text }}" />
+      <input type="submit" class="button" value="Send {{ count_of_recipients }} {{ message_count_label(count_of_recipients, template.template_type, suffix='') }}" />
      <a href="{{ back_link }}" class="page-footer-back-link">Back</a>
    </form>
  {% 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() %}
          <span class="table-field-error">
            <span class="table-field-error-label">{{ item['columns'][column].error }}</span>
@@ -161,13 +214,13 @@

  {% if count_of_displayed_recipients < count_of_recipients %}
    <p class="table-show-more-link">
-      {% if rows_have_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 %}
    </p>
-  {% elif rows_have_errors %}
+  {% elif row_errors and not recipients.missing_column_headers %}
    <p class="table-show-more-link">
      Only showing rows with errors
    </p>
--- 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') }}

+
+  <h2 class="heading-large">Formatted list</h2>
+
+  <p>
+    {{ formatted_list('A', prefix="one item called") }}
+  </p>
+
+  <p>
+    {{ formatted_list('AB', prefix_plural="two items called") }}
+  </p>
+
+  <p>
+    {{ formatted_list('ABC') }}
+  </p>
+
+  <p>
+    {{ formatted_list('ABCD', before_each='<strike>', after_each='</strike>', conjunction='or') }}
+  </p>
+
 {% endblock %}
--- 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 %}
-    <span class="placeholder">(({{ placeholder }}))</span>
-  {% endfor %}
-{% endmacro %}
+{% from "components/list.html" import list_of_placeholders %}

 {% block page_title %}
  GOV.UK Notify
@@ -21,8 +15,7 @@
  <div class="bottom-gutter">
    {% if template_change.placeholders_removed %}
      <p>
-        You removed
-        {{ list_of_placeholders(template_change.placeholders_removed) }}
+        You removed {{ list_of_placeholders(template_change.placeholders_removed) }}
      </p>
    {% endif %}
    {% if template_change.placeholders_added %}
--- 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:
--- 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
--- 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
--- 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,