From db37a16eda4a26b8ca448aa583fb360e6b931bbc Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Fri, 7 Apr 2017 09:18:00 +0100
Subject: [PATCH] Fix broken journey when going back from check page
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

We’ve seen in letters usability testing that people get stuck in a
“no-man’s land” when trying to go back from the _Send yourself a test_
page.

This was broken for two reasons:
- we hadn’t considered that a letter template without placeholder still
  requires you to fill in
- we’ve changed subsequently made the _view template_ page the place
  where you do your actions, rather than the (old) page with all the
  templates shown

So this commit fixes it so that the back link always take you back to
the page you were previously on, and adds some more test cases so we
have all the scenarios accounted for.
---
 app/main/views/send.py            |  4 ++--
 tests/app/main/views/test_send.py | 39 +++++++++++++++++++++++++++----
 2 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/app/main/views/send.py b/app/main/views/send.py
index 7171e2b3a..9feee1257 100644
--- a/app/main/views/send.py
+++ b/app/main/views/send.py
@@ -236,13 +236,13 @@ def _check_messages(service_id, template_type, upload_id, letters_as_pdf=False):
 
     if request.args.get('from_test'):
         extra_args = {'help': 1} if request.args.get('help', '0') != '0' else {}
-        if len(template.placeholders):
+        if len(template.placeholders) or template.template_type == 'letter':
             back_link = url_for(
                 '.send_test', service_id=service_id, template_id=template.id, **extra_args
             )
         else:
             back_link = url_for(
-                '.choose_template', service_id=service_id, **extra_args
+                '.view_template', service_id=service_id, template_id=template.id, **extra_args
             )
         choose_time_form = None
     else:
diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py
index 31cffcb5f..f85470f4a 100644
--- a/tests/app/main/views/test_send.py
+++ b/tests/app/main/views/test_send.py
@@ -15,6 +15,11 @@ from app.main.views.send import get_check_messages_back_url
 
 from tests import validate_route_permission, template_json
 from tests.app.test_utils import normalize_spaces
+from tests.conftest import (
+    mock_get_service_template,
+    mock_get_service_template_with_placeholders,
+    mock_get_service_letter_template,
+)
 
 template_types = ['email', 'sms']
 
@@ -688,18 +693,41 @@ def test_route_invalid_permissions(
 
 
 @pytest.mark.parametrize(
-    'extra_args,expected_url',
+    'template_mock, extra_args, expected_url',
     [
         (
+            mock_get_service_template,
             dict(),
+            partial(url_for, '.send_messages')
+        ),
+        (
+            mock_get_service_template_with_placeholders,
+            dict(),
+            partial(url_for, '.send_messages')
+        ),
+        (
+            mock_get_service_template,
+            dict(from_test=True),
+            partial(url_for, '.view_template')
+        ),
+        (
+            mock_get_service_letter_template,  # No placeholders
+            dict(from_test=True),
             partial(url_for, '.send_test')
         ),
         (
-            dict(help='0'),
+            mock_get_service_template_with_placeholders,
+            dict(from_test=True),
             partial(url_for, '.send_test')
         ),
         (
-            dict(help='2'),
+            mock_get_service_template_with_placeholders,
+            dict(help='0', from_test=True),
+            partial(url_for, '.send_test')
+        ),
+        (
+            mock_get_service_template_with_placeholders,
+            dict(help='2', from_test=True),
             partial(url_for, '.send_test', help='1')
         )
     ]
@@ -711,14 +739,16 @@ def test_check_messages_back_link(
     mock_get_user_by_email,
     mock_get_users_by_service,
     mock_get_service,
-    mock_get_service_template_with_placeholders,
     mock_has_permissions,
     mock_get_detailed_service_for_today,
     mock_s3_download,
     fake_uuid,
+    mocker,
+    template_mock,
     extra_args,
     expected_url
 ):
+    template_mock(mocker)
     with logged_in_client.session_transaction() as session:
         session['upload_data'] = {'original_file_name': 'valid.csv',
                                   'template_id': fake_uuid,
@@ -729,7 +759,6 @@ def test_check_messages_back_link(
         service_id=fake_uuid,
         upload_id=fake_uuid,
         template_type='sms',
-        from_test=True,
         **extra_args
     ))
     assert response.status_code == 200