From 072f8e0f87131471fa6cddb362a7b00a1a756808 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Mon, 15 Jan 2018 11:35:45 +0000
Subject: [PATCH] Make row in URL match displayed row
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Spreadsheets start at row 1 (the header row), and the values don’t start
until row 2. The row numbers in our URLs start at 0, which is a concept
that only makes sense to programmers.

It’s more predictable and consistent to make the number in the URL match
the row number displayed on the page when previewing the spreadsheet.
---
 app/main/views/send.py            | 13 ++++++++-----
 app/templates/views/check/ok.html |  4 ++--
 tests/app/main/views/test_send.py | 17 ++++++++++-------
 3 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/app/main/views/send.py b/app/main/views/send.py
index 5bd1ffc37..6ca253755 100644
--- a/app/main/views/send.py
+++ b/app/main/views/send.py
@@ -523,9 +523,12 @@ def _check_messages(service_id, template_type, upload_id, preview_row, letters_a
 
     count_of_recipients = len(list(recipients.rows))
 
-    if preview_row < count_of_recipients:
-        template.values = recipients[preview_row]
-    elif preview_row > 0:
+    if preview_row < 2:
+        abort(404)
+
+    if preview_row < count_of_recipients + 2:
+        template.values = recipients[preview_row - 2]
+    elif preview_row > 2:
         abort(404)
 
     session['upload_data']['notification_count'] = count_of_recipients
@@ -562,7 +565,7 @@ def _check_messages(service_id, template_type, upload_id, preview_row, letters_a
 @main.route("/services/<service_id>/<template_type>/check/<upload_id>/row-<int:row_index>", methods=['GET'])
 @login_required
 @user_has_permissions('send_texts', 'send_emails', 'send_letters')
-def check_messages(service_id, template_type, upload_id, row_index=0):
+def check_messages(service_id, template_type, upload_id, row_index=2):
 
     data = _check_messages(service_id, template_type, upload_id, row_index)
 
@@ -590,7 +593,7 @@ def check_messages(service_id, template_type, upload_id, row_index=0):
 @main.route("/services/<service_id>/<template_type>/check/<upload_id>/row-<int:row_index>.<filetype>", methods=['GET'])
 @login_required
 @user_has_permissions('send_texts', 'send_emails', 'send_letters')
-def check_messages_preview(service_id, template_type, upload_id, filetype, row_index=0):
+def check_messages_preview(service_id, template_type, upload_id, filetype, row_index=2):
     if filetype not in ('pdf', 'png'):
         abort(404)
 
diff --git a/app/templates/views/check/ok.html b/app/templates/views/check/ok.html
index 5724b3beb..aa9ca1cc8 100644
--- a/app/templates/views/check/ok.html
+++ b/app/templates/views/check/ok.html
@@ -60,10 +60,10 @@
       ) %}
         {% call index_field() %}
           <span>
-            {% if item.index == preview_row %}
+            {% if (item.index + 2) == preview_row %}
               {{ item.index + 2 }}
             {% else %}
-              <a href="{{ url_for('.check_messages', service_id=current_service.id, template_type=template.template_type, upload_id=upload_id, row_index=(item.index)) }}">{{ item.index + 2 }}</a>
+              <a href="{{ url_for('.check_messages', service_id=current_service.id, template_type=template.template_type, upload_id=upload_id, row_index=(item.index + 2)) }}">{{ item.index + 2 }}</a>
             {% endif %}
           </span>
         {% endcall %}
diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py
index 70fa17051..606f877c1 100644
--- a/tests/app/main/views/test_send.py
+++ b/tests/app/main/views/test_send.py
@@ -472,13 +472,13 @@ def test_upload_valid_csv_redirects_to_check_page(
         'Test Service: A, Template <em>content</em> with & entity',
     ),
     (
-        {'row_index': 0},
+        {'row_index': 2},
         None,
         'To: 07700900001',
         'Test Service: A, Template <em>content</em> with & entity',
     ),
     (
-        {'row_index': 2},
+        {'row_index': 4},
         True,
         'To: 07700900003',
         'Test Service: C, Template <em>content</em> with & entity',
@@ -524,7 +524,7 @@ def test_upload_valid_csv_shows_preview_and_table(
 
     if expected_link_in_first_row:
         assert page.select_one('.table-field-index a')['href'] == url_for(
-            'main.check_messages', service_id=SERVICE_ONE_ID, template_type='sms', upload_id=fake_uuid, row_index=0
+            'main.check_messages', service_id=SERVICE_ONE_ID, template_type='sms', upload_id=fake_uuid, row_index=2
         )
     else:
         assert not page.select_one('.table-field-index').select_one('a')
@@ -546,9 +546,12 @@ def test_upload_valid_csv_shows_preview_and_table(
 
 
 @pytest.mark.parametrize('row_index, expected_status', [
-    (0, 200),
+    (0, 404),
+    (1, 404),
     (2, 200),
-    (3, 404),
+    (3, 200),
+    (4, 200),
+    (5, 404),
 ])
 def test_404_for_previewing_a_row_out_of_range(
     client_request,
@@ -1480,11 +1483,11 @@ def test_can_start_letters_job(
         {'postcode': 'abc123', 'addressline1': '123 street'},
     ),
     (
-        {'row_index': 0},
+        {'row_index': 2},
         {'postcode': 'abc123', 'addressline1': '123 street'},
     ),
     (
-        {'row_index': 1},
+        {'row_index': 3},
         {'postcode': 'cba321', 'addressline1': '321 avenue'},
     ),
 ])