From 5a52eafb84fa5ecb0527a48d894ac23ce15a5d0f Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 9 Aug 2018 16:29:51 +0100 Subject: [PATCH] =?UTF-8?q?Fix=20=E2=80=98Back=E2=80=99=20link=20from=20fi?= =?UTF-8?q?rst=20step=20of=20fix=20one=20off?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If you skip past the templates page (because you don’t have the edit permission) but then click back you end up in a loop which redirects you to the page you’re already on. This commit makes sure that you’re sent back a step further, so you don’t get stuck in that loop. --- app/main/views/send.py | 26 ++++++++-------- app/main/views/templates.py | 7 ++--- app/utils.py | 8 +++++ tests/app/main/views/test_send.py | 50 ++++++++++++++++++------------- 4 files changed, 53 insertions(+), 38 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index c8aecc77b..182684baf 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -49,6 +49,7 @@ from app.utils import ( get_errors_for_csv, get_help_argument, get_template, + should_skip_template_page, unicode_truncate, user_has_permissions, ) @@ -428,7 +429,7 @@ def send_test_step(service_id, template_id, step_index): help=get_help_argument(), )) - back_link = get_back_link(service_id, template_id, step_index) + back_link = get_back_link(service_id, template, step_index) template.values = get_recipient_and_placeholders_from_session(template.template_type) template.values[current_placeholder] = None @@ -762,35 +763,36 @@ def get_send_test_page_title(template_type, help_argument, entering_recipient, n return 'Personalise this message' -def get_back_link(service_id, template_id, step_index): +def get_back_link(service_id, template, step_index): 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, + template_id=template.id, help=get_help_argument() ) else: return None elif step_index == 0: - if current_user.has_permissions('view_activity'): - return url_for( - '.view_template', - service_id=service_id, - template_id=template_id, - ) - else: + if should_skip_template_page(template.template_type): return url_for( '.choose_template', service_id=service_id, ) + else: + return url_for( + '.view_template', + service_id=service_id, + template_id=template.id, + ) + else: return url_for( 'main.send_one_off_step', service_id=service_id, - template_id=template_id, + template_id=template.id, step_index=step_index - 1, ) @@ -818,7 +820,7 @@ def _check_notification(service_id, template_id, exception=None): sms_sender=sms_sender ) - back_link = get_back_link(service_id, template_id, len(fields_to_fill_in(template))) + back_link = get_back_link(service_id, template, len(fields_to_fill_in(template))) if ( not session.get('recipient') or diff --git a/app/main/views/templates.py b/app/main/views/templates.py index cacbf68ab..58ebcd524 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -29,6 +29,7 @@ from app.template_previews import TemplatePreview, get_page_count_for_letter from app.utils import ( email_or_sms_not_enabled, get_template, + should_skip_template_page, user_has_permissions, ) @@ -49,11 +50,7 @@ page_headings = { @user_has_permissions() def view_template(service_id, template_id): template = service_api_client.get_service_template(service_id, str(template_id))['data'] - if ( - current_user.has_permissions('send_messages') and - not current_user.has_permissions('manage_templates', 'manage_api_keys') and - template['template_type'] != 'letter' - ): + if should_skip_template_page(template['template_type']): return redirect(url_for( '.send_one_off', service_id=service_id, template_id=template_id )) diff --git a/app/utils.py b/app/utils.py index e180551cf..ac3251bf4 100644 --- a/app/utils.py +++ b/app/utils.py @@ -655,3 +655,11 @@ def guess_name_from_email_address(email_address): ).then( normalize_spaces ) + + +def should_skip_template_page(template_type): + return ( + current_user.has_permissions('send_messages') and + not current_user.has_permissions('manage_templates', 'manage_api_keys') and + template_type != 'letter' + ) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index c7002a2e0..7f4ddf5d7 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -1482,47 +1482,55 @@ def test_send_test_email_message_without_placeholders_redirects_to_check_page( assert page.select('h1')[0].text.strip() == 'Preview of ‘Two week reminder’' -@pytest.mark.parametrize('user, expected_back_link_endpoint, extra_args', ( - (active_user_with_permissions, 'main.view_template', {'template_id': unchanging_fake_uuid}), - (active_caseworking_user, 'main.choose_template', {}), +@pytest.mark.parametrize('permissions, expected_back_link_endpoint, extra_args', ( + ( + {'send_messages', 'manage_templates'}, + 'main.view_template', + {'template_id': unchanging_fake_uuid} + ), + ( + {'send_messages'}, + 'main.choose_template', + {}, + ), + ( + {'send_messages', 'view_activity'}, + 'main.choose_template', + {}, + ), )) def test_send_test_sms_message_with_placeholders_shows_first_field( - logged_in_client, - mocker, - service_one, + client_request, + active_user_with_permissions, mock_login, mock_get_service, mock_get_service_template_with_placeholders, mock_has_no_jobs, - fake_uuid, - user, + permissions, expected_back_link_endpoint, extra_args, ): - mocker.patch('app.user_api_client.get_user', return_value=user(fake_uuid)) + active_user_with_permissions._permissions[SERVICE_ONE_ID] = permissions + client_request.login(active_user_with_permissions) - with logged_in_client.session_transaction() as session: + with client_request.session_transaction() as session: assert 'placeholders' not in session - response = logged_in_client.get( - url_for( - 'main.send_test', - service_id=service_one['id'], - template_id=unchanging_fake_uuid, - ), - follow_redirects=True, + page = client_request.get( + 'main.send_test', + service_id=SERVICE_ONE_ID, + template_id=unchanging_fake_uuid, + _follow_redirects=True, ) - assert response.status_code == 200 - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') assert page.select('label')[0].text.strip() == 'name' assert page.select('input')[0]['name'] == 'placeholder_value' assert page.select('.page-footer-back-link')[0]['href'] == url_for( expected_back_link_endpoint, - service_id=service_one['id'], + service_id=SERVICE_ONE_ID, **extra_args ) - with logged_in_client.session_transaction() as session: + with client_request.session_transaction() as session: assert session['recipient'] == '07700 900762'