From 389638244c990383204c582c9a49ef7785c0edda Mon Sep 17 00:00:00 2001 From: David McDonald Date: Mon, 5 Oct 2020 11:55:15 +0100 Subject: [PATCH] Remove old tour and help arguments from sending flow We no longer need the `start_tour` page as this has been replaced with the new `begin_tour` page. We also no longer need to handle the `help` argument in the `send_test_step` or `send_one_off_step` as these no longer are responsible for the tour and don't need to show the help text. Worth pointing out, the new tour joins into the send one off flow. When doing a GET `check_tour_notification`, and submitting the form shown on this page you are POSTed to `send_notification` with `help=3`. Also for general sending of one off notifications, the POST to `send_notification` is done with `help=0` which is a bit of a hack to make sure that we don't show a back link on the `view_notification` page for when someone gets there having just sent a one off notification. This use of `help=0` may be a candidate for a refactor in the future as it feels like a bit of a hacky way of doing things and is therefore not as clear to developers what is going on. Also removes the help argument from the csv routes used here. There is no reason that we need to ever show help for CSVs and this is leftover code from when we used to do the tour that way. --- app/main/views/send.py | 45 +------ app/main/views/templates.py | 21 ---- app/navigation.py | 4 - app/templates/views/check/ok.html | 1 - tests/app/main/views/test_send.py | 160 +------------------------ tests/app/main/views/test_templates.py | 50 -------- tests/app/main/views/test_tour.py | 8 ++ 7 files changed, 17 insertions(+), 272 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 5769d78d0..308295234 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -54,7 +54,6 @@ from app.utils import ( PermanentRedirect, Spreadsheet, get_errors_for_csv, - get_help_argument, get_template, should_skip_template_page, unicode_truncate, @@ -321,7 +320,6 @@ def send_test(service_id, template_id): service_id=service_id, template_id=template_id, step_index=0, - help=get_help_argument(), )) @@ -330,8 +328,6 @@ 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 )) @@ -394,14 +390,12 @@ def send_one_off_letter_address(service_id, template_id): 'views/send-one-off-letter-address.html', page_title=get_send_test_page_title( template_type='letter', - help_argument=None, entering_recipient=True, name=template.name, ), template=template, form=form, back_link=get_back_link(service_id, template, 0), - help=False, link_to_upload=True, ) @@ -487,7 +481,6 @@ def send_test_step(service_id, template_id, step_index): service_id=service_id, template_id=template_id, step_index=step_index + 1, - help=get_help_argument(), )) form = get_placeholder_form_instance( @@ -518,7 +511,6 @@ def send_test_step(service_id, template_id, step_index): service_id=service_id, template_id=template_id, step_index=step_index + 1, - help=get_help_argument(), )) back_link = get_back_link(service_id, template, step_index, placeholders) @@ -530,7 +522,6 @@ def send_test_step(service_id, template_id, step_index): 'views/send-test.html', page_title=get_send_test_page_title( template.template_type, - get_help_argument(), entering_recipient=not session['recipient'], name=template.name, ), @@ -538,7 +529,6 @@ def send_test_step(service_id, template_id, step_index): form=form, skip_link=get_skip_link(step_index, template), back_link=back_link, - help=get_help_argument(), link_to_upload=( request.endpoint == 'main.send_one_off_step' and step_index == 0 @@ -707,7 +697,6 @@ def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_ remaining_messages=remaining_messages, choose_time_form=choose_time_form, back_link=back_link, - help=get_help_argument(), trying_to_send_letters_in_trial_mode=all(( current_service.trial_mode, template.template_type == 'letter', @@ -834,7 +823,6 @@ def start_job(service_id, upload_id): 'main.view_job', job_id=upload_id, service_id=service_id, - help=request.form.get('help'), just_sent='yes', ) ) @@ -891,9 +879,7 @@ def all_placeholders_in_session(placeholders): ) -def get_send_test_page_title(template_type, help_argument, entering_recipient, name=None): - if help_argument: - return 'Example text message' +def get_send_test_page_title(template_type, entering_recipient, name=None): if entering_recipient: return 'Send ‘{}’'.format(name) return 'Personalise this message' @@ -908,31 +894,7 @@ def is_current_user_the_recipient(): def get_back_link(service_id, template, step_index, placeholders=None): - if get_help_argument(): - # 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: - if step_index == 0: - return url_for( - 'main.start_tour', - service_id=service_id, - template_id=template.id, - ) - elif step_index > 0: - return url_for( - 'main.send_test_step', - service_id=service_id, - template_id=template.id, - step_index=step_index - 1, - help=2, - ) - elif step_index == 0: + if step_index == 0: if should_skip_template_page(template.template_type): return url_for( '.choose_template', @@ -1046,7 +1008,6 @@ def _check_notification(service_id, template_id, exception=None): return dict( template=template, back_link=back_link, - help=get_help_argument(), letter_too_long=is_letter_too_long(page_count), letter_max_pages=LETTER_MAX_PAGE_COUNT, page_count=page_count, @@ -1116,6 +1077,8 @@ def send_notification(service_id, template_id): '.view_notification', service_id=service_id, notification_id=noti['id'], + # used to show the final step of the tour (help=3) or not show + # a back link on a just sent one off notification (help=0) help=request.args.get('help') )) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 8e6ddb164..f7f3936d5 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -83,27 +83,6 @@ def view_template(service_id, template_id): ) -@main.route("/services//start-tour/") -@user_has_permissions('view_activity') -def start_tour(service_id, template_id): - - template = current_service.get_template(template_id) - - if template['template_type'] != 'sms': - abort(404) - - return render_template( - 'views/templates/start-tour.html', - template=get_template( - template, - current_service, - show_recipient=True, - ), - help='1', - continue_link=url_for('.send_test', service_id=current_service.id, template_id=template['id'], help=2) - ) - - @main.route("/services//templates/all", methods=['GET', 'POST']) @main.route("/services//templates", methods=['GET', 'POST']) @main.route("/services//templates/folders/", methods=['GET', 'POST']) diff --git a/app/navigation.py b/app/navigation.py index c0e23f65a..ab37a72a9 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -322,7 +322,6 @@ class HeaderNavigation(Navigation): 'show_accounts_or_dashboard', 'sign_out', 'start_job', - 'start_tour', 'styleguide', 'submit_request_to_go_live', 'template_history', @@ -665,7 +664,6 @@ class MainNavigation(Navigation): 'sign_in', 'sign_out', 'start_job', - 'start_tour', 'styleguide', 'support', 'support_public', @@ -978,7 +976,6 @@ class CaseworkNavigation(Navigation): 'sign_in', 'sign_out', 'start_job', - 'start_tour', 'styleguide', 'submit_request_to_go_live', 'support', @@ -1293,7 +1290,6 @@ class OrgNavigation(Navigation): 'sign_in', 'sign_out', 'start_job', - 'start_tour', 'styleguide', 'submit_request_to_go_live', 'support', diff --git a/app/templates/views/check/ok.html b/app/templates/views/check/ok.html index a2fe13ab2..0eed56fd8 100644 --- a/app/templates/views/check/ok.html +++ b/app/templates/views/check/ok.html @@ -35,7 +35,6 @@
- {% if choose_time_form and template.template_type != 'letter' %} {{ radio_select( diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 9d6212978..ba9ecbcdf 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -1381,54 +1381,36 @@ def test_send_one_off_does_not_send_without_the_correct_permissions( create_active_user_with_permissions(), create_active_caseworking_user(), )) -@pytest.mark.parametrize('template_type, partial_url, expected_h1, tour_shown', [ +@pytest.mark.parametrize('template_type, partial_url, expected_h1', [ ( 'sms', partial(url_for, 'main.send_test'), 'Personalise this message', - False, ), ( 'sms', partial(url_for, 'main.send_one_off'), 'Send ‘Two week reminder’', - False, - ), - ( - 'sms', - partial(url_for, 'main.send_test', help=1), - 'Example text message', - True, - ), - ( - 'email', - partial(url_for, 'main.send_test', help=1), - 'Example text message', - True, ), ( 'email', partial(url_for, 'main.send_test'), 'Personalise this message', - False, ), ( 'email', partial(url_for, 'main.send_one_off'), 'Send ‘Two week reminder’', - False, ), ( 'letter', partial(url_for, 'main.send_test'), 'Send ‘Two week reminder’', - False, ), ( 'letter', partial(url_for, 'main.send_one_off'), 'Send ‘Two week reminder’', - False, ), ]) def test_send_one_off_or_test_has_correct_page_titles( @@ -1441,7 +1423,6 @@ def test_send_one_off_or_test_has_correct_page_titles( template_type, partial_url, expected_h1, - tour_shown, user, ): mocker.patch('app.user_api_client.get_user', return_value=user) @@ -1458,7 +1439,7 @@ def test_send_one_off_or_test_has_correct_page_titles( assert response.status_code == 200 assert page.h1.text.strip() == expected_h1 - assert (len(page.select('.banner-tour')) == 1) == tour_shown + assert len(page.select('.banner-tour')) == 0 @pytest.mark.parametrize('endpoint, step_index, prefilled, expected_field_label', [ @@ -1773,32 +1754,6 @@ def test_no_link_to_use_existing_list_for_service_without_lists( ] -@pytest.mark.parametrize('user', ( - create_active_user_with_permissions(), - create_active_caseworking_user(), -)) -def test_link_to_upload_not_offered_in_tour( - client_request, - fake_uuid, - mock_get_service_template, - user, -): - client_request.login(user) - - page = client_request.get( - 'main.send_test', - service_id=SERVICE_ONE_ID, - template_id=fake_uuid, - help=1, - _follow_redirects=True, - ) - - # We’re in the tour… - assert page.select('.banner-tour') - # …but first link on the page is ‘Back’, so not preceeded by ‘Upload’ - assert page.select_one('main a').text == 'Back' - - @pytest.mark.parametrize('user', ( create_active_user_with_permissions(), create_active_caseworking_user(), @@ -1974,7 +1929,7 @@ def test_send_test_redirects_to_start_if_index_out_of_bounds_and_some_placeholde ('main.send_test', 'main.send_test_step'), ('main.send_one_off', 'main.send_one_off_step'), ]) -def test_send_test_sms_message_redirects_with_help_argument( +def test_send_test_sms_message_redirects( client_request, mocker, service_one, @@ -1991,14 +1946,12 @@ def test_send_test_sms_message_redirects_with_help_argument( endpoint, service_id=SERVICE_ONE_ID, template_id=fake_uuid, - help=1, _expected_status=302, _expected_response=url_for( expected_redirect, service_id=SERVICE_ONE_ID, template_id=fake_uuid, step_index=0, - help=1, _external=True, ) ) @@ -2112,49 +2065,6 @@ def test_send_test_sms_message_back_link_with_multiple_placeholders( ) -@pytest.mark.parametrize('step_index, expected_back_link', ( - (0, partial( - url_for, - 'main.start_tour', - )), - (1, partial( - url_for, - 'main.send_test_step', - step_index=0, - help=2, - )), - (2, partial( - url_for, - 'main.send_test_step', - step_index=1, - help=2, - )) -)) -def test_send_test_sms_message_back_link_in_tour( - client_request, - mock_get_service_template_with_multiple_placeholders, - mock_has_no_jobs, - step_index, - expected_back_link, -): - with client_request.session_transaction() as session: - session['recipient'] = '07900900123' - session['placeholders'] = {'phone number': '07900900123', 'one': 'bar'} - - page = client_request.get( - 'main.send_test_step', - service_id=SERVICE_ONE_ID, - template_id=unchanging_fake_uuid, - step_index=step_index, - help=2, - ) - - assert page.select_one('.govuk-back-link')['href'] == expected_back_link( - service_id=SERVICE_ONE_ID, - template_id=unchanging_fake_uuid, - ) - - def test_send_test_letter_redirects_to_right_url( platform_admin_client, fake_uuid, @@ -3972,37 +3882,6 @@ 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, @@ -4031,6 +3910,8 @@ def test_check_notification_shows_preview( # assert tour not visible assert not page.select('.banner-tour') + + # post to send_notification with help=0 to ensure no back link is then shown assert page.form.attrs['action'] == url_for( 'main.send_notification', service_id=service_one['id'], @@ -4039,37 +3920,6 @@ def test_check_notification_shows_preview( ) -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('.govuk-back-link')['href'] == url_for( - 'main.send_test', - service_id=service_one['id'], - template_id=fake_uuid, - help='2' - ) - - def test_check_notification_shows_back_link( mocker, client_request, diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index e658c2804..a69c1a58e 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -2209,56 +2209,6 @@ def test_should_create_sms_or_broadcast_template_without_downgrading_unicode_cha ) -def test_should_show_template_as_first_page_of_tour( - client_request, - mock_get_service_template, - service_one, - fake_uuid, -): - - page = client_request.get( - 'main.start_tour', - service_id=SERVICE_ONE_ID, - template_id=fake_uuid, - ) - - assert normalize_spaces( - page.select('.banner-tour .heading-medium')[0].text - ) == ( - 'Try sending yourself this example' - ) - - assert normalize_spaces( - page.select('.sms-message-wrapper')[0].text - ) == ( - 'service one: Template content with & entity' - ) - - assert page.select('a.govuk-button')[0]['href'] == url_for( - '.send_test', service_id=SERVICE_ONE_ID, template_id=fake_uuid, help=2 - ) - - -@pytest.mark.parametrize('template_type', ['email', 'letter']) -def test_cant_see_email_template_in_tour( - client_request, - fake_uuid, - mocker, - template_type, -): - mocker.patch( - 'app.service_api_client.get_service_template', - return_value={'data': create_template(template_type=template_type)} - ) - - client_request.get( - 'main.start_tour', - service_id=SERVICE_ONE_ID, - template_id=fake_uuid, - _expected_status=404, - ) - - def test_should_show_message_before_redacting_template( client_request, mock_get_service_template, diff --git a/tests/app/main/views/test_tour.py b/tests/app/main/views/test_tour.py index 39a89364e..58a136598 100644 --- a/tests/app/main/views/test_tour.py +++ b/tests/app/main/views/test_tour.py @@ -536,6 +536,14 @@ def test_should_200_for_check_tour_notification( 'service one: hello hi howdy' ) + # post to send_notification keeps help argument + assert page.form.attrs['action'] == url_for( + 'main.send_notification', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + help='3' + ) + def test_back_link_from_check_tour_notification_points_to_last_tour_step( client_request,