Merge pull request #1345 from alphagov/help-i-need-somebody

help (aka tour) now works correctly throughout the notification flow

app/main/views/send.py

@@ -183,6 +183,8 @@ def get_notification_check_endpoint(service_id, template):
             'main.check_notification',
             service_id=service_id,
             template_id=template.id,
+            # at check phase we should move to help stage 2 ("the template pulls in the data you provide")
+            help='2' if 'help' in request.args else None
         ))
 
 
@@ -343,7 +345,7 @@ def _check_messages(service_id, template_type, upload_id, letters_as_pdf=False):
         # NOTE: this is a 301 MOVED PERMANENTLY (httpstatus.es/301), so the browser will cache this redirect, and it'll
         # *always* happen for that browser. _check_messages is only used by endpoints that contain `upload_id`, which
         # is a one-time-use id (that ties to a given file in S3 that is already deleted if it's not in the session)
-        raise RequestRedirect(get_check_messages_back_url(service_id, template_type))
+        raise RequestRedirect(url_for('main.choose_template', service_id=service_id))
 
     users = user_api_client.get_users_for_service(service_id=service_id)
 
@@ -381,15 +383,8 @@ 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 {}
+        # only happens if generating a letter preview test
-        if len(template.placeholders) or template.template_type == 'letter':
+        back_link = url_for('.send_test', service_id=service_id, template_id=template.id)
-            back_link = url_for(
-                '.send_test', service_id=service_id, template_id=template.id, **extra_args
-            )
-        else:
-            back_link = url_for(
-                '.view_template', service_id=service_id, template_id=template.id, **extra_args
-            )
         choose_time_form = None
     else:
         back_link = url_for('.send_messages', service_id=service_id, template_id=template.id)
@@ -499,19 +494,6 @@ def go_to_dashboard_after_tour(service_id, example_template_id):
     )
 
 
-def get_check_messages_back_url(service_id, template_type):
-    if get_help_argument():
-        # if the user is on the introductory tour, then they should be redirected back to the beginning of the tour -
-        # but to do that we need to find the template_id of the example template. That template *should* be the only
-        # template for that service, but it's possible they've opened another tab and deleted it for example. In that
-        # case we should just redirect back to the main page as they clearly know what they're doing.
-        templates = service_api_client.get_service_templates(service_id)['data']
-        if len(templates) == 1:
-            return url_for('.send_test', service_id=service_id, template_id=templates[0]['id'], help=1)
-
-    return url_for('main.choose_template', service_id=service_id)
-
-
 def fields_to_fill_in(template, prefill_current_user=False):
 
     recipient_columns = first_column_headings[template.template_type]
@@ -582,7 +564,16 @@ def get_send_test_page_title(template_type, help_argument):
 
 def get_back_link(service_id, template_id, step_index):
     if get_help_argument():
-        return None
+        # if we're on the check page, redirect back to the beginning. anywhere else, don't return the back link
+        if request.endpoint == 'main.check_notification':
+            return url_for(
+                'main.send_test',
+                service_id=service_id,
+                template_id=template_id,
+                help=get_help_argument()
+            )
+        else:
+            return None
     elif step_index == 0:
         return url_for(
             '.view_template',
@@ -618,11 +609,8 @@ def _check_notification(service_id, template_id, exception=None):
     back_link = get_back_link(service_id, template_id, 0)
 
     if (
-        (
+        not session.get('recipient') or
-            not session.get('recipient') or
+        not all_placeholders_in_session(template.placeholders)
-            not all_placeholders_in_session(template.placeholders)
-        )
-        and back_link
     ):
         return redirect(back_link)
 
@@ -689,5 +677,6 @@ def send_notification(service_id, template_id):
     return redirect(url_for(
         '.view_notification',
         service_id=service_id,
-        notification_id=noti['id']
+        notification_id=noti['id'],
+        help=request.args.get('help')
     ))

app/templates/views/jobs/job.html

@@ -19,11 +19,9 @@
     {{ ajax_block(partials, updates_url, 'counts', finished=finished) }}
     {{ ajax_block(partials, updates_url, 'notifications', finished=finished) }}
 
-    {% if not help %}
+    {{ page_footer(
-      {{ page_footer(
+      secondary_link=url_for('.view_template', service_id=current_service.id, template_id=template.id),
-        secondary_link=url_for('.view_template', service_id=current_service.id, template_id=template.id),
+      secondary_link_text='Back to {}'.format(template.name)
-        secondary_link_text='Back to {}'.format(template.name)
+    ) }}
-      ) }}
-    {% endif %}
 
 {% endblock %}

app/templates/views/notifications/check.html

@@ -28,9 +28,13 @@
   {{ template|string }}
 
   <div class="bottom-gutter-3-2">
-    <form method="post" enctype="multipart/form-data" action="{{url_for('main.send_notification', service_id=current_service.id, template_id=template.id)}}" class='page-footer'>
+    <form method="post" enctype="multipart/form-data" action="{{url_for(
+        'main.send_notification',
+        service_id=current_service.id,
+        template_id=template.id,
+        help='3' if help else 0
+      )}}" class='page-footer'>
       <input type="hidden" name="csrf_token" value="{{ csrf_token() }}" />
-      <input type="hidden" name="help" value="{{ '3' if help else 0 }}" />
       {% if not error %}
         {% if template.template_type != 'letter' or not request.args.from_test %}
         <input type="submit" class="button" value="Send 1 {{ message_count_label(1, template.template_type, suffix='') }}" />

app/templates/views/notifications/notification.html

@@ -19,9 +19,11 @@
     {{ ajax_block(partials, updates_url, 'counts', finished=finished) }}
     {{ ajax_block(partials, updates_url, 'notifications', finished=finished) }}
 
-    {{ page_footer(
+    {% if not help %}
-      secondary_link=url_for('.view_template', service_id=current_service.id, template_id=template.id),
+      {{ page_footer(
-      secondary_link_text='Back to {}'.format(template.name)
+        secondary_link=url_for('.view_template', service_id=current_service.id, template_id=template.id),
-    ) }}
+        secondary_link_text='Back to {}'.format(template.name)
+      ) }}
+    {% endif %}
 
 {% endblock %}

tests/app/main/views/test_send.py

@@ -14,9 +14,7 @@ from notifications_python_client.errors import HTTPError
 from notifications_utils.template import LetterPreviewTemplate, LetterImageTemplate
 from notifications_utils.recipients import RecipientCSV
 
-from app.main.views.send import get_check_messages_back_url
+from tests import validate_route_permission
-
-from tests import validate_route_permission, template_json
 from tests.app.test_utils import normalize_spaces
 from tests.conftest import (
     mock_get_service_template,
@@ -1288,11 +1286,6 @@ def test_route_invalid_permissions(
             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),
@@ -1302,16 +1295,6 @@ def test_route_invalid_permissions(
             mock_get_service_template_with_placeholders,
             dict(from_test=True),
             partial(url_for, '.send_test')
-        ),
-        (
-            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')
         )
     ]
 )
@@ -1534,8 +1517,7 @@ def test_non_ascii_characters_in_letter_recipients_file_shows_error(
     assert page.find('span', class_='table-field-error-label').text == u'Can’t include П, е, т or я'
 
 
-def test_check_messages_redirects_if_no_upload_data(logged_in_client, service_one, mocker):
+def test_check_messages_redirects_if_no_upload_data(logged_in_client, service_one):
-    checker = mocker.patch('app.main.views.send.get_check_messages_back_url', return_value='foo')
     response = logged_in_client.get(url_for(
         'main.check_messages',
         service_id=service_one['id'],
@@ -1543,51 +1525,8 @@ def test_check_messages_redirects_if_no_upload_data(logged_in_client, service_on
         upload_id='baz'
     ))
 
-    checker.assert_called_once_with(service_one['id'], 'bar')
     assert response.status_code == 301
-    assert response.location == 'http://localhost/foo'
+    assert response.location == url_for('main.choose_template', service_id=service_one['id'], _external=True)
-
-
-@pytest.mark.parametrize('template_type', ['sms', 'email'])
-def test_get_check_messages_back_url_returns_to_correct_select_template(client, mocker, template_type):
-    mocker.patch('app.main.views.send.get_help_argument', return_value=False)
-
-    assert get_check_messages_back_url('1234', template_type) == url_for(
-        'main.choose_template',
-        service_id='1234'
-    )
-
-
-def test_check_messages_back_from_help_goes_to_start_of_help(client, service_one, mocker):
-    mocker.patch('app.main.views.send.get_help_argument', return_value=True)
-    mocker.patch('app.service_api_client.get_service_templates', lambda service_id: {
-        'data': [template_json(service_one['id'], '111', type_='sms')]
-    })
-    assert get_check_messages_back_url(service_one['id'], 'sms') == url_for(
-        'main.send_test',
-        service_id=service_one['id'],
-        template_id='111',
-        help='1'
-    )
-
-
-@pytest.mark.parametrize('templates', [
-    [],
-    [
-        template_json('000', '111', type_='sms'),
-        template_json('000', '222', type_='sms')
-    ]
-], ids=['no_templates', 'two_templates'])
-def test_check_messages_back_from_help_handles_unexpected_templates(client, mocker, templates):
-    mocker.patch('app.main.views.send.get_help_argument', return_value=True)
-    mocker.patch('app.service_api_client.get_service_templates', lambda service_id: {
-        'data': templates
-    })
-
-    assert get_check_messages_back_url('1234', 'sms') == url_for(
-        'main.choose_template',
-        service_id='1234',
-    )
 
 
 @pytest.mark.parametrize('existing_session_items', [
@@ -1619,6 +1558,37 @@ def test_check_notification_redirects_if_session_not_populated(
     )
 
 
+@pytest.mark.parametrize('existing_session_items', [
+    {},
+    {'recipient': '07700900001'},
+    {'name': 'Jo'}
+])
+def test_check_notification_redirects_with_help_if_session_not_populated(
+    logged_in_client,
+    service_one,
+    fake_uuid,
+    existing_session_items,
+    mock_get_service_template_with_placeholders
+):
+    with logged_in_client.session_transaction() as session:
+        session.update(existing_session_items)
+
+    resp = logged_in_client.get(url_for(
+        'main.check_notification',
+        service_id=service_one['id'],
+        template_id=fake_uuid,
+        help='2'
+    ))
+
+    assert resp.location == url_for(
+        'main.send_test',
+        service_id=service_one['id'],
+        template_id=fake_uuid,
+        help='2',
+        _external=True
+    )
+
+
 def test_check_notification_shows_preview(
     client_request,
     service_one,
@@ -1640,6 +1610,46 @@ def test_check_notification_shows_preview(
         page.findAll('a', {'class': 'page-footer-back-link'})[0]['href']
     ) == url_for('main.view_template', service_id=service_one['id'], template_id=fake_uuid)
 
+    # assert tour not visible
+    assert not page.select('.banner-tour')
+    assert page.form.attrs['action'] == url_for(
+        'main.send_notification',
+        service_id=service_one['id'],
+        template_id=fake_uuid,
+        help='0'
+    )
+
+
+def test_check_notification_shows_help(
+    client_request,
+    service_one,
+    fake_uuid,
+    mock_get_service_template
+):
+    with client_request.session_transaction() as session:
+        session['recipient'] = '07700900001'
+        session['placeholders'] = {}
+
+    page = client_request.get(
+        'main.check_notification',
+        service_id=service_one['id'],
+        template_id=fake_uuid,
+        help='2'
+    )
+    assert page.select_one('.banner-tour')
+    assert page.form.attrs['action'] == url_for(
+        'main.send_notification',
+        service_id=service_one['id'],
+        template_id=fake_uuid,
+        help='3'
+    )
+    assert page.select_one('.page-footer-back-link')['href'] == url_for(
+        'main.send_test',
+        service_id=service_one['id'],
+        template_id=fake_uuid,
+        help='2'
+    )
+
 
 def test_send_notification_submits_data(
     client_request,
@@ -1707,18 +1717,24 @@ def test_send_notification_redirects_if_missing_data(
     )
 
 
+@pytest.mark.parametrize('extra_args, extra_redirect_args', [
+    ({}, {}),
+    ({'help': '3'}, {'help': '3'})
+])
 def test_send_notification_redirects_to_view_page(
     logged_in_client,
     service_one,
     fake_uuid,
     mock_send_notification,
+    extra_args,
+    extra_redirect_args
 ):
     with logged_in_client.session_transaction() as session:
         session['recipient'] = '07700900001'
         session['placeholders'] = {'a': 'b'}
 
     resp = logged_in_client.post(
-        url_for('main.send_notification', service_id=service_one['id'], template_id=fake_uuid)
+        url_for('main.send_notification', service_id=service_one['id'], template_id=fake_uuid, **extra_args)
     )
 
     assert resp.status_code == 302
@@ -1726,7 +1742,8 @@ def test_send_notification_redirects_to_view_page(
         '.view_notification',
         service_id=service_one['id'],
         notification_id=fake_uuid,
-        _external=True
+        _external=True,
+        **extra_redirect_args
     )