diff --git a/app/main/views/send.py b/app/main/views/send.py index 5572b7036..9c8060d31 100644 --- a/app/main/views/send.py +++ b/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 {} - 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( - '.view_template', service_id=service_id, template_id=template.id, **extra_args - ) + # only happens if generating a letter preview test + back_link = url_for('.send_test', service_id=service_id, template_id=template.id) 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 all_placeholders_in_session(template.placeholders) - ) - and back_link + not session.get('recipient') or + not all_placeholders_in_session(template.placeholders) ): 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') )) diff --git a/app/templates/views/jobs/job.html b/app/templates/views/jobs/job.html index 14aa297bf..9483766d4 100644 --- a/app/templates/views/jobs/job.html +++ b/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( - secondary_link=url_for('.view_template', service_id=current_service.id, template_id=template.id), - secondary_link_text='Back to {}'.format(template.name) - ) }} - {% endif %} + {{ page_footer( + secondary_link=url_for('.view_template', service_id=current_service.id, template_id=template.id), + secondary_link_text='Back to {}'.format(template.name) + ) }} {% endblock %} diff --git a/app/templates/views/notifications/check.html b/app/templates/views/notifications/check.html index 02656faae..fb336b631 100644 --- a/app/templates/views/notifications/check.html +++ b/app/templates/views/notifications/check.html @@ -28,9 +28,13 @@ {{ template|string }}
-
+ - {% if not error %} {% if template.template_type != 'letter' or not request.args.from_test %} diff --git a/app/templates/views/notifications/notification.html b/app/templates/views/notifications/notification.html index f4936fb53..e56b1066a 100644 --- a/app/templates/views/notifications/notification.html +++ b/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( - secondary_link=url_for('.view_template', service_id=current_service.id, template_id=template.id), - secondary_link_text='Back to {}'.format(template.name) - ) }} + {% if not help %} + {{ page_footer( + 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 %} diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index dce07f62e..e3f94686c 100644 --- a/tests/app/main/views/test_send.py +++ b/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, template_json +from tests import validate_route_permission 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): - checker = mocker.patch('app.main.views.send.get_check_messages_back_url', return_value='foo') +def test_check_messages_redirects_if_no_upload_data(logged_in_client, service_one): 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' - - -@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', - ) + assert response.location == url_for('main.choose_template', service_id=service_one['id'], _external=True) @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 )