From 8f8c2501245288360a01dd94fb9ea173d36dc71b Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 22 May 2020 12:25:44 +0100 Subject: [PATCH 1/2] Handle session expiring during service name change --- app/main/views/service_settings.py | 4 ++ tests/__init__.py | 7 +++- tests/app/main/views/test_service_settings.py | 40 ++++++++++++++----- 3 files changed, 41 insertions(+), 10 deletions(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index bb9cb1044..02e036668 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -112,6 +112,10 @@ def service_name_change(service_id): @main.route("/services//service-settings/name/confirm", methods=['GET', 'POST']) @user_has_permissions('manage_service') def service_name_change_confirm(service_id): + if 'service_name_change' not in session: + flash("Session expired. Try again", 'error') + return redirect(url_for('main.service_name_change', service_id=service_id)) + # Validate password for form def _check_password(pwd): return user_api_client.verify_password(current_user.id, pwd) diff --git a/tests/__init__.py b/tests/__init__.py index f1a8d6626..14ebdd8ac 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -537,7 +537,8 @@ def validate_route_permission(mocker, route, permissions, usr, - service): + service, + session=None): usr['permissions'][str(service['id'])] = permissions usr['services'] = [service['id']] mocker.patch( @@ -556,6 +557,10 @@ def validate_route_permission(mocker, with app_.test_request_context(): with app_.test_client() as client: client.login(usr) + if session: + with client.session_transaction() as session_: + for k, v in session.items(): + session_[k] = v resp = None if method == 'GET': resp = client.get(route) diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 00126e9e8..51953cee1 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -644,6 +644,9 @@ def test_should_not_allow_duplicate_names( def test_should_show_service_name_confirmation( client_request, ): + service_new_name = 'New Name' + with client_request.session_transaction() as session: + session['service_name_change'] = service_new_name page = client_request.get( 'main.service_name_change_confirm', service_id=SERVICE_ONE_ID, @@ -703,6 +706,20 @@ def test_should_raise_duplicate_name_handled( assert mock_verify_password.called +def test_service_name_change_confirm_handles_expired_session( + client_request, mock_verify_password, mock_update_service +): + page = client_request.post( + 'main.service_name_change_confirm', + service_id=SERVICE_ONE_ID, + _follow_redirects=True + ) + mock_verify_password.assert_not_called() + mock_update_service.assert_not_called() + + assert page.find('div', 'banner-dangerous').text.strip() == "Session expired. Try again" + + @pytest.mark.parametrize('volumes, consent_to_research, expected_estimated_volumes_item', [ ((0, 0, 0), None, 'Tell us how many messages you expect to send Not completed'), ((1, 0, 0), None, 'Tell us how many messages you expect to send Not completed'), @@ -1881,7 +1898,9 @@ def test_route_permissions( url_for(route, service_id=service_one['id']), ['manage_service'], api_user_active, - service_one) + service_one, + session={'service_name_change': "New Service Name"} + ) @pytest.mark.parametrize('route', [ @@ -1936,14 +1955,17 @@ def test_route_for_platform_admin( mock_get_service_templates, mock_get_invites_for_service, ): - validate_route_permission(mocker, - app_, - "GET", - 200, - url_for(route, service_id=service_one['id']), - [], - platform_admin_user, - service_one) + validate_route_permission( + mocker, + app_, + "GET", + 200, + url_for(route, service_id=service_one['id']), + [], + platform_admin_user, + service_one, + session={'service_name_change': "New Service Name"} + ) def test_and_more_hint_appears_on_settings_with_more_than_just_a_single_sender( From f997cc28016b661a609591fc7577c6ec3bfb8692 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 22 May 2020 15:36:08 +0100 Subject: [PATCH 2/2] Improve the error message following content review. 'Session expired' or similar makes it sound like a new error. It could confuse the user and make them think the sign in didn't work and that their session has expired again. So we went with: The change you made was not saved. Please try again. --- app/main/views/service_settings.py | 2 +- tests/app/main/views/test_service_settings.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 02e036668..853eeee58 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -113,7 +113,7 @@ def service_name_change(service_id): @user_has_permissions('manage_service') def service_name_change_confirm(service_id): if 'service_name_change' not in session: - flash("Session expired. Try again", 'error') + flash("The change you made was not saved. Please try again.", 'error') return redirect(url_for('main.service_name_change', service_id=service_id)) # Validate password for form diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 51953cee1..f7e93d46b 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -717,7 +717,7 @@ def test_service_name_change_confirm_handles_expired_session( mock_verify_password.assert_not_called() mock_update_service.assert_not_called() - assert page.find('div', 'banner-dangerous').text.strip() == "Session expired. Try again" + assert page.find('div', 'banner-dangerous').text.strip() == "The change you made was not saved. Please try again." @pytest.mark.parametrize('volumes, consent_to_research, expected_estimated_volumes_item', [