From 34171f303858bd3eb8da3d6a14f30b5e187fda81 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Fri, 3 May 2019 13:07:00 +0100
Subject: [PATCH] Fix order of placeholders in the tour

Doing a lookup with `step_index - 1` means that on step `0` we were
looking up `placeholders[-1]`, ie we were making people fill in the last
placeholder first.

Fixing this reintroduces the bug fixed by this pull request:
https://github.com/alphagov/notifications-admin/pull/2551

So this commit also re-fixes that bug but in a different way.
---
 app/main/views/send.py            |  8 +--
 tests/app/main/views/test_send.py | 90 ++++++++++++++++++++++++++++++-
 tests/conftest.py                 | 14 +++++
 3 files changed, 105 insertions(+), 7 deletions(-)

diff --git a/app/main/views/send.py b/app/main/views/send.py
index 98ec4fc3a..1c2eb352f 100644
--- a/app/main/views/send.py
+++ b/app/main/views/send.py
@@ -386,11 +386,7 @@ def send_test_step(service_id, template_id, step_index):
     )
 
     try:
-        if request.endpoint == 'main.send_test_step':
-            current_placeholder = placeholders[step_index - 1]
-        else:
-            current_placeholder = placeholders[step_index]
-
+        current_placeholder = placeholders[step_index]
     except IndexError:
         if all_placeholders_in_session(placeholders):
             return get_notification_check_endpoint(service_id, template)
@@ -812,7 +808,7 @@ def get_back_link(service_id, template, step_index):
                 service_id=service_id,
                 template_id=template.id,
             )
-    elif is_current_user_the_recipient() and step_index > 1:
+    elif is_current_user_the_recipient() and step_index >= 1:
         return url_for(
             'main.send_test_step',
             service_id=service_id,
diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py
index 96aca6e00..331d667b2 100644
--- a/tests/app/main/views/test_send.py
+++ b/tests/app/main/views/test_send.py
@@ -1123,6 +1123,69 @@ def test_send_one_off_or_test_has_correct_page_titles(
     assert (len(page.select('.banner-tour')) == 1) == tour_shown
 
 
+@pytest.mark.parametrize('endpoint, step_index, prefilled, expected_field_label', [
+    (
+        'main.send_test_step',
+        0,
+        {'phone number': '07900900123'},
+        'one',
+    ),
+    (
+        'main.send_one_off_step',
+        0,
+        {},
+        'phone number',
+    ),
+    (
+        'main.send_test_step',
+        1,
+        {'phone number': '07900900123', 'one': 'foo'},
+        'two',
+    ),
+    (
+        'main.send_one_off_step',
+        1,
+        {'phone number': '07900900123'},
+        'one',
+    ),
+    (
+        'main.send_test_step',
+        2,
+        {'phone number': '07900900123', 'one': 'foo', 'two': 'foo'},
+        'three',
+    ),
+    (
+        'main.send_one_off_step',
+        2,
+        {'phone number': '07900900123', 'one': 'one'},
+        'two',
+    ),
+])
+def test_send_one_off_or_test_shows_placeholders_in_correct_order(
+    client_request,
+    fake_uuid,
+    mock_has_no_jobs,
+    mock_get_service_template_with_multiple_placeholders,
+    endpoint,
+    step_index,
+    prefilled,
+    expected_field_label,
+):
+    with client_request.session_transaction() as session:
+        session['recipient'] = None
+        session['placeholders'] = prefilled
+        session['send_test_letter_page_count'] = None
+
+    page = client_request.get(
+        endpoint,
+        service_id=SERVICE_ONE_ID,
+        template_id=fake_uuid,
+        step_index=step_index,
+    )
+
+    assert normalize_spaces(page.select_one('label').text) == expected_field_label
+
+
 @pytest.mark.parametrize('user, template_mock, expected_link_text, expected_link_url', [
     (
         active_user_with_permissions,
@@ -1588,6 +1651,31 @@ def test_send_test_sms_message_with_placeholders_shows_first_field(
         assert session['recipient'] == '07700 900762'
 
 
+def test_send_test_sms_message_back_link_with_multiple_placeholders(
+    client_request,
+    mock_get_service_template_with_multiple_placeholders,
+    mock_has_no_jobs,
+):
+    with client_request.session_transaction() as session:
+        session['recipient'] = '07900900123'
+        session['placeholders'] = {'phone number': '07900900123', 'one': 'bar'}
+        session['send_test_letter_page_count'] = None
+
+    page = client_request.get(
+        'main.send_test_step',
+        service_id=SERVICE_ONE_ID,
+        template_id=unchanging_fake_uuid,
+        step_index=2,
+    )
+
+    assert page.select_one('.govuk-back-link')['href'] == url_for(
+        'main.send_test_step',
+        service_id=SERVICE_ONE_ID,
+        template_id=unchanging_fake_uuid,
+        step_index=1,
+    )
+
+
 def test_send_test_letter_clears_previous_page_cache(
     logged_in_platform_admin_client,
     mocker,
@@ -1721,7 +1809,7 @@ def test_send_test_indicates_optional_address_columns(
     )
 
     assert normalize_spaces(page.select('label')[0].text) == (
-        'address line 3 '
+        'address line 4 '  # step_index is 0-indexed so step 3 is address line 4
         'Optional'
     )
     assert page.select('.govuk-back-link')[0]['href'] == url_for(
diff --git a/tests/conftest.py b/tests/conftest.py
index a6a8db96c..b46b7d3b8 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -845,6 +845,20 @@ def mock_get_service_template_with_placeholders(mocker):
     )
 
 
+@pytest.fixture(scope='function')
+def mock_get_service_template_with_multiple_placeholders(mocker):
+    def _get(service_id, template_id, version=None):
+        template = template_json(
+            service_id, template_id, "Two week reminder", "sms", "((one)) ((two)) ((three))"
+        )
+        return {'data': template}
+
+    return mocker.patch(
+        'app.service_api_client.get_service_template',
+        side_effect=_get
+    )
+
+
 @pytest.fixture(scope='function')
 def mock_get_service_template_with_placeholders_same_as_recipient(mocker):
     def _get(service_id, template_id, version=None):