From 4aaf47aaa1a67f2c6db20d59cb84735882f908c5 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Mon, 5 Oct 2020 16:16:11 +0100 Subject: [PATCH] When sending to yourself use 'one-off' rather than 'test' routes There is no real reason to have to support both 'one-off' steps and also 'test' steps when sending a one off notification. It's a lot of complex code, just to now set the one of the placeholders in the session. We make our code much simpler but no longer using the 'test' routes but instead adding a new endpoint to set the notification recipient when sending to yourself before continuing on with the rest of the 'one-off' flow. After this is deployed for a day then we can completely remove the 'test' routes and this will help remove a lot of code complexity. --- app/main/views/send.py | 40 ++++++++++++----- app/navigation.py | 4 ++ tests/app/main/views/test_send.py | 73 +++++++++++++++++++++++++++++-- 3 files changed, 102 insertions(+), 15 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 308295234..ae4623101 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -885,14 +885,6 @@ def get_send_test_page_title(template_type, entering_recipient, name=None): return 'Personalise this message' -def is_current_user_the_recipient(): - if 'recipient' not in session: - return False - if hasattr(current_user, 'mobile_number'): - return session['recipient'] in {current_user.email_address, current_user.mobile_number} - return session['recipient'] == current_user.email_address - - def get_back_link(service_id, template, step_index, placeholders=None): if step_index == 0: if should_skip_template_page(template.template_type): @@ -906,14 +898,14 @@ def get_back_link(service_id, template, step_index, placeholders=None): service_id=service_id, template_id=template.id, ) - elif is_current_user_the_recipient() and step_index > 1: + elif request.endpoint == 'main.send_test_step' and step_index > 1: return url_for( 'main.send_test_step', service_id=service_id, template_id=template.id, step_index=step_index - 1, ) - elif is_current_user_the_recipient() and step_index == 1: + elif request.endpoint == 'main.send_test_step' and step_index == 1: return url_for( 'main.send_one_off_step', service_id=service_id, @@ -947,16 +939,40 @@ def get_skip_link(step_index, template): if ( request.endpoint == 'main.send_one_off_step' and step_index == 0 - and template.template_type != 'letter' + and template.template_type in ("sms", "email") and not (template.template_type == 'sms' and current_user.mobile_number is None) and current_user.has_permissions('manage_templates', 'manage_service') ): return ( 'Use my {}'.format(first_column_headings[template.template_type][0]), - url_for('.send_test', service_id=current_service.id, template_id=template.id), + url_for('.send_one_off_to_myself', service_id=current_service.id, template_id=template.id), ) +@main.route("/services//template//one-off/send-to-myself", methods=['GET']) +@user_has_permissions('send_messages', restrict_admin_usage=True) +def send_one_off_to_myself(service_id, template_id): + db_template = current_service.get_template_with_user_permission_or_403(template_id, current_user) + + if db_template['template_type'] not in ("sms", "email"): + abort(404) + + # We aren't concerned with creating the exact template (for example adding recipient and sender names) + # we just want to create enough to use `fields_to_fill_in` + template = get_template( + db_template, + current_service, + ) + fields_to_fill_in(template, prefill_current_user=True) + + return redirect(url_for( + 'main.send_one_off_step', + service_id=service_id, + template_id=template_id, + step_index=1, + )) + + @main.route("/services//template//notification/check", methods=['GET']) @user_has_permissions('send_messages', restrict_admin_usage=True) def check_notification(service_id, template_id): diff --git a/app/navigation.py b/app/navigation.py index ab37a72a9..05eb34dea 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -265,6 +265,7 @@ class HeaderNavigation(Navigation): 'send_one_off', 'send_one_off_letter_address', 'send_one_off_step', + 'send_one_off_to_myself', 'send_test', 'no_cookie.send_test_preview', 'send_test_step', @@ -414,6 +415,7 @@ class MainNavigation(Navigation): 'send_one_off', 'send_one_off_letter_address', 'send_one_off_step', + 'send_one_off_to_myself', 'send_test', 'no_cookie.send_test_preview', 'send_test_step', @@ -726,6 +728,7 @@ class CaseworkNavigation(Navigation): 'send_one_off', 'send_one_off_letter_address', 'send_one_off_step', + 'send_one_off_to_myself', 'send_test', 'send_test_step', }, @@ -1232,6 +1235,7 @@ class OrgNavigation(Navigation): 'send_one_off', 'send_one_off_letter_address', 'send_one_off_step', + 'send_one_off_to_myself', 'send_test', 'no_cookie.send_test_preview', 'send_test_step', diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index ba9ecbcdf..06be4d626 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -1504,13 +1504,13 @@ def test_send_one_off_or_test_shows_placeholders_in_correct_order( create_active_user_with_permissions(), 'sms', 'Use my phone number', - partial(url_for, 'main.send_test') + partial(url_for, 'main.send_one_off_to_myself') ), ( create_active_user_with_permissions(), 'email', 'Use my email address', - partial(url_for, 'main.send_test') + partial(url_for, 'main.send_one_off_to_myself') ), ( create_active_user_with_permissions(), @@ -1720,7 +1720,7 @@ def test_send_one_off_has_link_to_use_existing_list( ( 'Use my phone number', url_for( - 'main.send_test', + 'main.send_one_off_to_myself', service_id=SERVICE_ONE_ID, template_id=fake_uuid, ), @@ -4506,3 +4506,70 @@ def test_send_from_contact_list( mock_set_metadata.assert_called_once_with( SERVICE_ONE_ID, new_uuid, example_key='example value' ) + + +def test_send_to_myself_sets_placeholder_and_redirects_for_email( + mocker, client_request, fake_uuid, mock_get_service_email_template +): + with client_request.session_transaction() as session: + session['recipient'] = None + session['placeholders'] = {} + + client_request.get( + 'main.send_one_off_to_myself', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + _expected_status=302, + _expected_url=url_for( + 'main.send_one_off_step', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + step_index=1, + _external=True, + ) + ) + + with client_request.session_transaction() as session: + assert session['recipient'] == 'test@user.gov.uk' + assert session['placeholders'] == {'email address': 'test@user.gov.uk'} + + +def test_send_to_myself_sets_placeholder_and_redirects_for_sms( + mocker, client_request, fake_uuid, mock_get_service_template +): + with client_request.session_transaction() as session: + session['recipient'] = None + session['placeholders'] = {} + + client_request.get( + 'main.send_one_off_to_myself', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + _expected_status=302, + _expected_url=url_for( + 'main.send_one_off_step', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + step_index=1, + _external=True, + ) + ) + + with client_request.session_transaction() as session: + assert session['recipient'] == '07700 900762' + assert session['placeholders'] == {'phone number': '07700 900762'} + + +def test_send_to_myself_404s_for_letter( + mocker, client_request, fake_uuid, mock_get_service_letter_template +): + with client_request.session_transaction() as session: + session['recipient'] = None + session['placeholders'] = {} + + client_request.get( + 'main.send_one_off_to_myself', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + _expected_status=404, + )