From 888821d1b42db2f4cfe0001c6d096b7d78ee0d25 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Mon, 27 Feb 2017 14:46:01 +0000
Subject: [PATCH] =?UTF-8?q?Don=E2=80=99t=20500=20when=20a=20CSV=20is=20mis?=
 =?UTF-8?q?sing=20rows?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

> When the CSV is missing the header row, we get an error and the user
> will see "Sorry, we are experiencing technical difficulties..."
>
> We should return a better error message for the user.

– https://www.pivotaltracker.com/story/show/140668615

This was caused by an attempt to access the `first_recipient` variable
before it was assigned. It would only be assigned when there was at
least one row in the file.

Fixing this means doing two things:
- defaulting `first_recipient` to be `None` before looking in the file
- adding an error message for when we can’t extract any rows out of the
  file (which is more nuanced than the file just being completely empty)

(There’s a nasty `sort` in the Jinja template because when there are no
rows in the file the order of the required column headers is not
deterministic.)
---
 app/main/views/send.py            |  3 +--
 app/templates/views/check.html    | 17 +++++++++++++++++
 tests/app/main/views/test_send.py | 18 +++++++++++++++++-
 3 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/app/main/views/send.py b/app/main/views/send.py
index cd0536b09..5a47a8173 100644
--- a/app/main/views/send.py
+++ b/app/main/views/send.py
@@ -249,8 +249,6 @@ def _check_messages(service_id, template_type, upload_id, letters_as_pdf=False):
     remaining_messages = (current_service['message_limit'] - sum(stat['requested'] for stat in statistics.values()))
 
     contents = s3download(service_id, upload_id)
-    if not contents:
-        flash('There was a problem reading your upload file')
 
     template = get_template(
         service_api_client.get_service_template(
@@ -295,6 +293,7 @@ def _check_messages(service_id, template_type, upload_id, letters_as_pdf=False):
         choose_time_form = ChooseTimeForm()
 
     with suppress(StopIteration):
+        first_recipient = None
         template.values = next(recipients.rows)
         first_recipient = template.values.get(
             Columns.make_key(recipients.recipient_column_headers[0]),
diff --git a/app/templates/views/check.html b/app/templates/views/check.html
index e3ee1709e..39a9cc6b1 100644
--- a/app/templates/views/check.html
+++ b/app/templates/views/check.html
@@ -35,6 +35,23 @@
       {% endcall %}
     </div>
 
+  {% elif not count_of_recipients %}
+
+    <div class="bottom-gutter">
+      {% call banner_wrapper(type='dangerous') %}
+        <h1 class='banner-title'>
+          Your file is missing some rows
+        </h1>
+        <p>
+          It needs at least one row of data, and {{ recipients.missing_column_headers | sort() | formatted_list(
+            prefix='a column called',
+            prefix_plural='columns called'
+          ) }}.
+        </p>
+        {{ skip_to_file_contents() }}
+      {% endcall %}
+    </div>
+
   {% elif not recipients.has_recipient_columns %}
 
     <div class="bottom-gutter">
diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py
index fc288bfff..d1857a2fe 100644
--- a/tests/app/main/views/test_send.py
+++ b/tests/app/main/views/test_send.py
@@ -138,7 +138,23 @@ def test_upload_csvfile_with_errors_shows_check_page_with_errors(
             'It doesn’t have a column called ‘name’. '
             'Skip to file contents'
         )
-    )
+    ),
+    (
+        "+447700900986",
+        (
+            'Your file is missing some rows '
+            'It needs at least one row of data, and columns called ‘name’ and ‘phone number’. '
+            'Skip to file contents'
+        )
+    ),
+    (
+        "",
+        (
+            'Your file is missing some rows '
+            'It needs at least one row of data, and columns called ‘name’ and ‘phone number’. '
+            'Skip to file contents'
+        )
+    ),
 ])
 def test_upload_csvfile_with_missing_columns_shows_error(
     logged_in_client,