From d31c244363c80e4b3b58f50db292f634d833650e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 9 Apr 2020 16:25:48 +0100 Subject: [PATCH] =?UTF-8?q?Don=E2=80=99t=20send=20users=20back=20to=20a=20?= =?UTF-8?q?page=20that=20will=20send=20them=20forward=20again?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If you’re on a page with a normal placeholder and the previous placeholder is one that’s also in the address block then going back to the previous page will send you immediately forward to the current page again. This commit makes the back link a bit smarter by skipping over pages where it can see that they relate to a placeholder from the address block. If it gets all the way to the start of the list of placeholders without finding any non-address ones then it will default to generating a link that will redirect the user to filling in the address block again. --- app/main/views/send.py | 31 ++++++++++++++++++++++--------- tests/app/main/views/test_send.py | 14 ++++++++++++++ 2 files changed, 36 insertions(+), 9 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index f4aa8e4ee..494a3a1b4 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -519,7 +519,7 @@ def send_test_step(service_id, template_id, step_index): help=get_help_argument(), )) - back_link = get_back_link(service_id, template, step_index) + back_link = get_back_link(service_id, template, step_index, placeholders) template.values = get_recipient_and_placeholders_from_session(template.template_type) template.values[current_placeholder] = None @@ -915,7 +915,7 @@ def is_current_user_the_recipient(): return session['recipient'] == current_user.email_address -def get_back_link(service_id, template, step_index): +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': @@ -967,13 +967,26 @@ def get_back_link(service_id, template, step_index): step_index=0, ) - else: - return url_for( - 'main.send_one_off_step', - service_id=service_id, - template_id=template.id, - step_index=step_index - 1, - ) + if template.template_type == 'letter' and placeholders: + # Make sure we’re not redirecting users to a page which will + # just redirect them forwards again + back_link_destination_step_index = next(( + index + for index, placeholder in reversed( + list(enumerate(placeholders[:step_index])) + ) + if placeholder not in Columns( + PostalAddress('').as_personalisation + ) + ), 1) + return get_back_link(service_id, template, back_link_destination_step_index + 1) + + return url_for( + 'main.send_one_off_step', + service_id=service_id, + template_id=template.id, + step_index=step_index - 1, + ) @main.route("/services//template//notification/check", methods=['GET']) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 1e2af293f..ddafa5cc4 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -2061,6 +2061,7 @@ def test_send_one_off_letter_copes_with_placeholder_from_address_block( mocker, fake_uuid, mock_template_preview, + no_letter_contact_blocks, placeholder, ): mocker.patch( @@ -2107,6 +2108,19 @@ def test_send_one_off_letter_copes_with_placeholder_from_address_block( 'postcode': 'SW1A 1AA', } + back_link = page.select_one('.govuk-back-link')['href'] + assert back_link == url_for( + 'main.send_one_off_step', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + step_index=1, + ) + previous_page = client_request.get_url(back_link, _follow_redirects=True) + + # We’ve skipped past the address placeholder and gone back to the + # address block + assert normalize_spaces(previous_page.select_one('form label').text) == 'Address' + def test_send_test_sms_message_puts_submitted_data_in_session( client_request,