diff --git a/app/main/views/send.py b/app/main/views/send.py index ea681bc60..9c8060d31 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -345,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) @@ -494,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] @@ -577,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', @@ -613,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) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 4d63ada63..417d6e59c 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, @@ -1519,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'], @@ -1528,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', [ @@ -1604,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, @@ -1651,13 +1636,19 @@ def test_check_notification_shows_help( template_id=fake_uuid, help='2' ) - assert page.select('.banner-tour') + 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( @@ -1751,8 +1742,8 @@ def test_send_notification_redirects_to_view_page( '.view_notification', service_id=service_one['id'], notification_id=fake_uuid, - **extra_redirect_args, _external=True + **extra_redirect_args, )