diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 3bfeffa09..1a45f3aed 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -123,41 +123,31 @@ 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=['POST']) @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) + 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) - 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 - )) - - 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..33d7d07ad 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -661,17 +661,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 +690,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', 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..7980362fd 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -879,63 +879,22 @@ def test_no_permission_manage_users_page( assert "Team members" not in resp_text -def test_get_remove_user_from_service( - logged_in_client, - active_user_with_permissions, - mock_get_users_by_service, - service_one, - 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) - - def test_remove_user_from_service( - logged_in_client, + 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.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( @@ -1157,6 +1116,29 @@ def test_edit_user_permissions_page_displays_redacted_mobile_number_and_change_l ) +def test_edit_user_permissions_with_delete_query_shows_banner( + client_request, + active_user_with_permissions, + mock_get_users_by_service, + mock_get_template_folders, + service_one +): + page = client_request.get( + 'main.edit_user_permissions', + service_id=service_one['id'], + user_id=active_user_with_permissions.id, + delete=1 + ) + + 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.remove_user_from_service', + service_id=service_one['id'], + user_id=active_user_with_permissions.id + ) + + def test_edit_user_mobile_number_page( client_request, active_user_with_permissions,