From f7f9dd8530acb3cb640ac2bc4ebf5fe647a5339b Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 14 Mar 2019 17:31:51 +0000 Subject: [PATCH] fix user permissions save button sometimes deleting when you hit the delete button, it flashes the delete button and takes you to the `/service/../user/../delete` url. If you then click the save button, it would make a POST to the delete URL... and delete the user. now the page stays on the edit url, but adds a `?delete=yes` query string. The dangerous flash banner now has an action field which defines where the browser will make the POST to (which remains at /delete). --- app/main/views/manage_users.py | 54 ++++++------- app/navigation.py | 12 ++- app/templates/components/banner.html | 12 +-- .../views/edit-user-permissions.html | 11 ++- tests/app/main/views/test_manage_users.py | 76 +++++++------------ 5 files changed, 82 insertions(+), 83 deletions(-) diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 3bfeffa09..a0479d0b0 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -123,41 +123,43 @@ def edit_user_permissions(service_id, user_id): user=user, form=form, service_has_email_auth=service_has_email_auth, - mobile_number=mobile_number + mobile_number=mobile_number, + delete=request.args.get('delete'), ) -@main.route("/services//users//delete", methods=['GET', 'POST']) +@main.route("/services//users//delete", methods=['GET']) @login_required @user_has_permissions('manage_service') def remove_user_from_service(service_id, user_id): - user = current_service.get_team_member(user_id) - form = PermissionsForm.from_user(user, service_id) + return redirect(url_for( + '.edit_user_permissions', + service_id=service_id, + user_id=user_id, + delete='yes' + )) - if request.method == 'POST': - try: - service_api_client.remove_user_from_service(service_id, user_id) - except HTTPError as e: - msg = "You cannot remove the only user for a service" - if e.status_code == 400 and msg in e.message: - flash(msg, 'info') - return redirect(url_for( - '.manage_users', - service_id=service_id)) - else: - abort(500, e) - return redirect(url_for( - '.manage_users', - service_id=service_id - )) +@main.route("/services//users//delete", methods=['POST']) +@login_required +@user_has_permissions('manage_service') +def confirm_remove_user_from_service(service_id, user_id): + try: + service_api_client.remove_user_from_service(service_id, user_id) + except HTTPError as e: + msg = "You cannot remove the only user for a service" + if e.status_code == 400 and msg in e.message: + flash(msg, 'info') + return redirect(url_for( + '.manage_users', + service_id=service_id)) + else: + abort(500, e) - flash('Are you sure you want to remove {}?'.format(user.name), 'remove') - return render_template( - 'views/edit-user-permissions.html', - user=user, - form=form - ) + return redirect(url_for( + '.manage_users', + service_id=service_id + )) @main.route("/services//users//edit-email", methods=['GET', 'POST']) diff --git a/app/navigation.py b/app/navigation.py index 11baad508..ef8984176 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -212,6 +212,7 @@ class HeaderNavigation(Navigation): 'register_from_org_invite', 'registration_continue', 'remove_user_from_organisation', + 'confirm_remove_user_from_service', 'remove_user_from_service', 'request_letter_branding', 'request_to_go_live', @@ -341,6 +342,7 @@ class MainNavigation(Navigation): 'team-members': { 'confirm_edit_user_email', 'confirm_edit_user_mobile_number', + 'confirm_remove_user_from_service', 'edit_user_email', 'edit_user_mobile_number', 'edit_user_permissions', @@ -617,6 +619,7 @@ class CaseworkNavigation(Navigation): 'confirm_edit_user_email', 'confirm_edit_user_mobile_number', 'confirm_redact_template', + 'confirm_remove_user_from_service', 'conversation', 'conversation_reply', 'conversation_reply_with_template', @@ -661,17 +664,17 @@ class CaseworkNavigation(Navigation): 'get_notifications_as_json', 'go_to_dashboard_after_tour', 'inbound_sms_admin', - 'inbox', 'inbox_download', 'inbox_updates', + 'inbox', 'index', 'information_risk_management', 'information_security', 'integration_testing', 'invite_org_user', 'invite_user', - 'letter_branding', 'letter_branding_preview_image', + 'letter_branding', 'letter_template', 'link_service_to_organisation', 'live_services', @@ -690,19 +693,19 @@ class CaseworkNavigation(Navigation): 'organisation_preview_letter_branding', 'organisation_settings', 'organisations', - 'platform_admin', 'platform_admin_letter_validation_preview', 'platform_admin_list_complaints', 'platform_admin_returned_letters', + 'platform_admin', 'pricing', 'privacy', 'public_agreement', 'public_download_agreement', 'received_text_messages_callback', 'redact_template', - 'register', 'register_from_invite', 'register_from_org_invite', + 'register', 'registration_continue', 'remove_user_from_organisation', 'remove_user_from_service', @@ -952,6 +955,7 @@ class OrgNavigation(Navigation): 'register_from_invite', 'register_from_org_invite', 'registration_continue', + 'confirm_remove_user_from_service', 'remove_user_from_service', 'request_letter_branding', 'request_to_go_live', diff --git a/app/templates/components/banner.html b/app/templates/components/banner.html index c352b247a..3b30cf1cb 100644 --- a/app/templates/components/banner.html +++ b/app/templates/components/banner.html @@ -1,4 +1,6 @@ -{% macro banner(body, type=None, with_tick=False, delete_button=None, subhead=None, context=None) %} +{% from "components/form.html" import form_wrapper %} + +{% macro banner(body, type=None, with_tick=False, delete_button=None, subhead=None, context=None, action=None) %}
{% endif %} {% if delete_button %} -
+ {% call form_wrapper(action=action) %} -
+ {% endcall %} {% endif %}
{% endmacro %} -{% macro banner_wrapper(type=None, with_tick=False, delete_button=None, subhead=None) %} - {{ banner(caller()|safe, type=type, with_tick=with_tick, delete_button=delete_button, subhead=subhead) }} +{% macro banner_wrapper(type=None, with_tick=False, delete_button=None, subhead=None, action=None) %} + {{ banner(caller()|safe, type=type, with_tick=with_tick, delete_button=delete_button, subhead=subhead, action=action) }} {% endmacro %} diff --git a/app/templates/views/edit-user-permissions.html b/app/templates/views/edit-user-permissions.html index 17a4ef88f..ebc307aae 100644 --- a/app/templates/views/edit-user-permissions.html +++ b/app/templates/views/edit-user-permissions.html @@ -2,12 +2,21 @@ {% from "components/textbox.html" import textbox %} {% from "components/page-footer.html" import page_footer %} {% from "components/form.html" import form_wrapper %} +{% from "components/banner.html" import banner %} {% block service_page_title %} {{ user.name or user.email_localpart }} {% endblock %} {% block maincolumn_content %} + {% if delete %} + {{ banner( + 'Are you sure you want to remove {}?'.format(user.name), + type='dangerous', + delete_button='Yes, remove', + action=url_for('.remove_user_from_service', service_id=current_service.id, user_id=user.id) + ) }} + {% endif %}

{{ user.name or user.email_localpart }} @@ -28,7 +37,7 @@ {{ page_footer( 'Save', - delete_link=url_for('.remove_user_from_service', service_id=current_service.id, user_id=user.id) if user or None, + delete_link=url_for('.edit_user_permissions', service_id=current_service.id, user_id=user.id, delete='yes'), delete_link_text='Remove user from service' ) }} diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 454ace2d1..d5eef75a1 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -879,63 +879,45 @@ def test_no_permission_manage_users_page( assert "Team members" not in resp_text -def test_get_remove_user_from_service( - logged_in_client, +def test_remove_user_from_service_redirects( + client_request, active_user_with_permissions, - mock_get_users_by_service, service_one, + mock_get_users_by_service, + mock_get_template_folders, mocker, ): - response = logged_in_client.get( - url_for( - 'main.remove_user_from_service', - service_id=service_one['id'], - user_id=active_user_with_permissions.id)) - assert response.status_code == 200 - assert "Are you sure you want to remove" in response.get_data(as_text=True) - assert "Remove user from service" in response.get_data(as_text=True) + page = client_request.get( + 'main.remove_user_from_service', + service_id=service_one['id'], + user_id=active_user_with_permissions.id, + _follow_redirects=True + ) + banner = page.find('div', class_='banner-dangerous') + assert banner.contents[0].strip() == "Are you sure you want to remove Test User?" + assert banner.form.attrs['action'] == url_for( + 'main.confirm_remove_user_from_service', + service_id=service_one['id'], + user_id=active_user_with_permissions.id + ) -def test_remove_user_from_service( - logged_in_client, +def test_confirm_remove_user_from_service( + client_request, active_user_with_permissions, service_one, - mocker, - mock_get_users_by_service, - mock_get_user, mock_remove_user_from_service, ): - response = logged_in_client.post( - url_for( - 'main.remove_user_from_service', - service_id=service_one['id'], - user_id=active_user_with_permissions.id)) - assert response.status_code == 302 - assert response.location == url_for( - 'main.manage_users', service_id=service_one['id'], _external=True) - mock_remove_user_from_service.assert_called_once_with(service_one['id'], - str(active_user_with_permissions.id)) - - -def test_can_remove_user_from_service_as_platform_admin( - logged_in_client, - service_one, - platform_admin_user, - active_user_with_permissions, - mock_get_users_by_service, - mock_remove_user_from_service, - mocker, -): - response = logged_in_client.post( - url_for( - 'main.remove_user_from_service', - service_id=service_one['id'], - user_id=active_user_with_permissions.id)) - assert response.status_code == 302 - assert response.location == url_for( - 'main.manage_users', service_id=service_one['id'], _external=True) - mock_remove_user_from_service.assert_called_once_with(service_one['id'], - str(active_user_with_permissions.id)) + client_request.post( + 'main.confirm_remove_user_from_service', + service_id=service_one['id'], + user_id=active_user_with_permissions.id, + _expected_redirect=url_for('main.manage_users', service_id=service_one['id'], _external=True) + ) + mock_remove_user_from_service.assert_called_once_with( + service_one['id'], + str(active_user_with_permissions.id) + ) def test_can_invite_user_as_platform_admin(