From a4424e999fbb9166150942ff0d12590c5a976246 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Fri, 2 Mar 2018 10:45:29 +0000
Subject: [PATCH] Catch duplicate recipient columns in spreadsheets
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If someone has duplicate recipient columns in their file we don’t know
which one to use. This commit adds an error message which should help
them fix the duplication.

This commit doesn’t go to the extra effort to actually show the
correct values for duplication in the preview. Don’t think it’s worth
the effort/complexity for how infrequently we’ve seen this error.

Depends on:
- [ ] https://github.com/alphagov/notifications-utils/pull/376
---
 app/main/views/send.py                       |  1 +
 app/templates/views/check/column-errors.html | 21 ++++++++-
 requirements.txt                             |  2 +-
 tests/app/main/views/test_send.py            | 45 ++++++++++++++++++++
 4 files changed, 66 insertions(+), 3 deletions(-)

diff --git a/app/main/views/send.py b/app/main/views/send.py
index 36a43b9c0..f2a7748cb 100644
--- a/app/main/views/send.py
+++ b/app/main/views/send.py
@@ -581,6 +581,7 @@ def check_messages(service_id, template_type, upload_id, row_index=2):
         data['recipients'].too_many_rows or
         not data['count_of_recipients'] or
         not data['recipients'].has_recipient_columns or
+        data['recipients'].duplicate_recipient_column_headers or
         data['recipients'].missing_column_headers
     ):
         return render_template('views/check/column-errors.html', **data)
diff --git a/app/templates/views/check/column-errors.html b/app/templates/views/check/column-errors.html
index 4fec14c21..cdfe988b1 100644
--- a/app/templates/views/check/column-errors.html
+++ b/app/templates/views/check/column-errors.html
@@ -69,6 +69,21 @@
           ) }}.
         </p>
 
+      {% elif recipients.duplicate_recipient_column_headers %}
+
+        <h1 class='banner-title' data-module="track-error" data-error-type="Duplicate recipient columns" data-error-label="{{ upload_id }}">
+          Your file has more than one column called {{ (
+            recipients.duplicate_recipient_column_headers
+          ) | formatted_list(
+            conjunction='or',
+            prefix='',
+            prefix_plural=''
+          ) }}
+        </h1>
+        <p>
+          Delete or rename one of these columns and try again.
+        </p>
+
       {% elif recipients.missing_column_headers %}
 
         <h1 class='banner-title' data-module="track-error" data-error-type="Missing placeholder columns" data-error-label="{{ upload_id }}">
@@ -128,6 +143,8 @@
 
   {% if not request.args.from_test %}
 
+    {% set column_headers = recipients._raw_column_headers if recipients.duplicate_recipient_column_headers else recipients.column_headers %}
+
     <h2 class="heading-medium" id="{{ file_contents_header_id }}">{{ original_file_name }}</h2>
 
     <div class="fullscreen-content" data-module="fullscreen-table">
@@ -137,14 +154,14 @@
         caption_visible=False,
         field_headings=[
           '<span class="visually-hidden">Row in file</span><span aria-hidden="true">1</span>'|safe
-        ] + recipients.column_headers
+        ] + column_headers
       ) %}
         {% call index_field() %}
           <span>
             {{ item.index + 2 }}
           </span>
         {% endcall %}
-        {% for column in recipients.column_headers %}
+        {% for column in column_headers %}
           {% if item['columns'][column].error and not recipients.missing_column_headers %}
             {% call field() %}
               <span>
diff --git a/requirements.txt b/requirements.txt
index 31dba0fc9..3cb8d632a 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -18,4 +18,4 @@ notifications-python-client==4.7.2
 # PaaS
 awscli-cwlogs>=1.4,<1.5
 
-git+https://github.com/alphagov/notifications-utils.git@23.8.0#egg=notifications-utils==23.8.0
+git+https://github.com/alphagov/notifications-utils.git@23.8.1#egg=notifications-utils==23.8.1
diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py
index 3554a115d..c349c6d8d 100644
--- a/tests/app/main/views/test_send.py
+++ b/tests/app/main/views/test_send.py
@@ -421,6 +421,17 @@ def test_upload_csvfile_with_errors_shows_check_page_with_errors(
             'Skip to file contents'
         )
     ),
+    (
+        """
+            phone number, phone number, PHONE_NUMBER
+            +447700900111,+447700900222,+447700900333,
+        """,
+        (
+            'Your file has more than one column called ‘phone number’ or ‘PHONE_NUMBER’ '
+            'Delete or rename one of these columns and try again. '
+            'Skip to file contents'
+        )
+    ),
     (
         """
             phone number, name
@@ -614,6 +625,40 @@ def test_upload_valid_csv_shows_preview_and_table(
         assert normalize_spaces(str(page.select('table tbody td')[index + 1])) == cell
 
 
+def test_show_all_columns_if_there_are_duplicate_recipient_columns(
+    client_request,
+    mocker,
+    mock_get_live_service,
+    mock_get_service_template_with_placeholders,
+    mock_get_users_by_service,
+    mock_get_detailed_service_for_today,
+    fake_uuid,
+):
+
+    with client_request.session_transaction() as session:
+        session['upload_data'] = {'template_id': fake_uuid}
+
+    mocker.patch('app.main.views.send.s3download', return_value="""
+        phone number, phone_number, PHONENUMBER
+        07700900001,  07700900002,  07700900003
+    """)
+
+    page = client_request.get(
+        'main.check_messages',
+        service_id=SERVICE_ONE_ID,
+        template_type='sms',
+        upload_id=fake_uuid,
+        _test_page_title=False,
+    )
+
+    assert normalize_spaces(page.select_one('thead').text) == (
+        'Row in file1 phone number phone_number PHONENUMBER'
+    )
+    assert normalize_spaces(page.select_one('tbody').text) == (
+        '2 07700900003 07700900003 07700900003'
+    )
+
+
 @pytest.mark.parametrize('row_index, expected_status', [
     (0, 404),
     (1, 404),