Change order of errors for bad CSV files

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.
2026-02-22 11:24:54 -05:00 · 2016-06-03 13:52:15 +01:00
parent 2c17261795
commit 2ff6cf049f
6 changed files with 109 additions and 69 deletions
--- 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))
--- 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 %}
+
+    <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 columns called {{ list_of_columns(recipients.column_headers) }}.
+        </p>
+        <p>
+          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') }}.
+        </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 +112,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 +132,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 %}
@@ -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 %}
    <p class="table-show-more-link">
-      {% 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 %}
    </p>
-  {% elif rows_have_errors %}
+  {% elif row_errors %}
    <p class="table-show-more-link">
      Only showing rows with errors
    </p>
--- 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,