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).
2025-12-09 22:53:27 -05:00 · 2019-03-14 17:31:51 +00:00
parent 37d12d3aa3
commit f7f9dd8530
5 changed files with 82 additions and 83 deletions
--- 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/<service_id>/users/<user_id>/delete", methods=['GET', 'POST'])
+@main.route("/services/<service_id>/users/<user_id>/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/<service_id>/users/<user_id>/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/<service_id>/users/<uuid:user_id>/edit-email", methods=['GET', 'POST'])
--- 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',
--- 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) %}
  <div
    class='banner{% if type %}-{{ type }}{% endif %}{% if with_tick %}-with-tick{% endif %}'
    {% if type == 'dangerous' %}
@@ -16,14 +18,14 @@
    </p>
    {% endif %}
    {% if delete_button %}
-      <form method='post'>
+      {% call form_wrapper(action=action) %}
        <input type="hidden" name="csrf_token" value="{{ csrf_token() }}" />
        <button type="submit" class="button" name="delete">{{ delete_button }}</button>
-      </form>
+      {% endcall %}
    {% endif %}
  </div>
 {% 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 %}
--- 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 %}

  <h1 class="heading-large">
    {{ 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'
        ) }}

--- 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(