Merge pull request #4127 from alphagov/remove-org-users

Allow organisation team members to be removed

app/main/views/organisations.py

@@ -253,42 +253,28 @@ def invite_org_user(org_id):
     )
 
 
-@main.route("/organisations/<uuid:org_id>/users/<uuid:user_id>", methods=['GET', 'POST'])
+@main.route("/organisations/<uuid:org_id>/users/<uuid:user_id>", methods=['GET'])
 @user_has_permissions()
-def edit_user_org_permissions(org_id, user_id):
+def edit_organisation_user(org_id, user_id):
+    # The only action that can be done to an org user is to remove them from the org.
+    # This endpoint is used to get the ID of the user to delete without passing it as a
+    # query string, but it uses the template for all org team members in order to avoid
+    # having a page containing a single link.
     return render_template(
-        'views/organisations/organisation/users/user/index.html',
-        user=User.from_id(user_id)
+        'views/organisations/organisation/users/index.html',
+        users=current_organisation.team_members,
+        show_search_box=(len(current_organisation.team_members) > 7),
+        form=SearchUsersForm(),
+        user_to_remove=User.from_id(user_id)
     )
 
 
-@main.route("/organisations/<uuid:org_id>/users/<uuid:user_id>/delete", methods=['GET', 'POST'])
+@main.route("/organisations/<uuid:org_id>/users/<uuid:user_id>/delete", methods=['POST'])
 @user_has_permissions()
 def remove_user_from_organisation(org_id, user_id):
-    user = User.from_id(user_id)
-    if request.method == 'POST':
-        try:
-            organisations_client.remove_user_from_organisation(org_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_org_users',
-                    org_id=org_id))
-            else:
-                abort(500, e)
+    organisations_client.remove_user_from_organisation(org_id, user_id)
 
-        return redirect(url_for(
-            '.manage_org_users',
-            org_id=org_id
-        ))
-
-    flash('Are you sure you want to remove {}?'.format(user.name), 'remove')
-    return render_template(
-        'views/organisations/organisation/users/user/index.html',
-        user=user,
-    )
+    return redirect(url_for('.show_accounts_or_dashboard'))
 
 
 @main.route("/organisations/<uuid:org_id>/cancel-invited-user/<uuid:invited_user_id>", methods=['GET'])

app/navigation.py

@@ -341,7 +341,7 @@ class OrgNavigation(Navigation):
 
         },
         'team-members': {
-            'edit_user_org_permissions',
+            'edit_organisation_user',
             'invite_org_user',
             'manage_org_users',
             'remove_user_from_organisation',

app/notify_client/organisations_api_client.py

@@ -3,7 +3,7 @@ from itertools import chain
 from notifications_python_client.errors import HTTPError
 
 from app.extensions import redis_client
-from app.notify_client import NotifyAdminAPIClient, _attach_current_user, cache
+from app.notify_client import NotifyAdminAPIClient, cache
 
 
 class OrganisationsClient(NotifyAdminAPIClient):
@@ -76,10 +76,9 @@ class OrganisationsClient(NotifyAdminAPIClient):
     def get_organisation_services(self, org_id):
         return self.get(url="/organisations/{}/services".format(org_id))
 
+    @cache.delete('user-{user_id}')
     def remove_user_from_organisation(self, org_id, user_id):
-        endpoint = '/organisations/{}/users/{}'.format(org_id, user_id)
-        data = _attach_current_user({})
-        return self.delete(endpoint, data)
+        return self.delete(f'/organisations/{org_id}/users/{user_id}')
 
     def get_services_and_usage(self, org_id, year):
         return self.get(

app/templates/views/organisations/organisation/users/index.html

@@ -10,6 +10,15 @@
 
 {% block maincolumn_content %}
 
+  {% if user_to_remove %}
+    {{ banner(
+      'Are you sure you want to remove {}?'.format(user_to_remove.name),
+      type='dangerous',
+      delete_button='Yes, remove',
+      action=url_for('.remove_user_from_organisation', org_id=current_org.id, user_id=user_to_remove.id)
+    ) }}
+  {% endif %}
+
   <h1 class="heading-medium">
     Team members
   </h1>
@@ -44,7 +53,9 @@
           </div>
           <div class="govuk-grid-column-one-quarter">
             {% if user.status == 'pending' %}
-              <a class="govuk-link govuk-link--no-visited-state user-list-edit-link" href="{{ url_for('.cancel_invited_org_user', org_id=current_org.id, invited_user_id=user.id)}}">Cancel invitation</a>
+              <a class="govuk-link govuk-link--no-visited-state user-list-edit-link" href="{{ url_for('.cancel_invited_org_user', org_id=current_org.id, invited_user_id=user.id)}}">Cancel invitation<span class="govuk-visually-hidden"> for {{ user.email_address }}</span></a>
+            {% elif user.status != 'cancelled' %}
+              <a class="govuk-link govuk-link--destructive user-list-edit-link" href="{{ url_for('.edit_organisation_user', org_id=current_org.id, user_id=user.id)}}">Remove<span class="govuk-visually-hidden"> {{ user.name }} {{ user.email_address }}</span></a>
             {% endif %}
           </div>
         </div>

app/templates/views/organisations/organisation/users/user/index.html

@@ -1,31 +0,0 @@
-{% extends "withnav_template.html" %}
-{% from "components/page-header.html" import page_header %}
-{% from "components/page-footer.html" import page_footer %}
-{% from "components/form.html" import form_wrapper %}
-{% from "components/back-link/macro.njk" import govukBackLink %}
-
-{% block service_page_title %}
-  {{ user.name or user.email_localpart }}
-{% endblock %}
-
-{% block backLink %}
-  {{ govukBackLink({ "href": url_for('.manage_org_users', org_id=current_org.id) }) }}
-{% endblock %}
-
-{% block maincolumn_content %}
-
-  {{ page_header(user.name or user.email_localpart) }}
-
-  <p class="govuk-body">
-    {{ user.email_address }}
-  </p>
-
-  {% call form_wrapper(class="govuk-grid-column-three-quarters") %}
-      {{ page_footer(
-        'Save',
-        delete_link=url_for('.remove_user_from_organisation', org_id=current_org.id, user_id=user.id) if user or None,
-        delete_link_text='Remove user from organisation'
-      ) }}
-  {% endcall %}
-
-{% endblock %}

tests/app/main/views/organisations/test_organisation_invites.py

@@ -9,74 +9,6 @@ from app.models.user import InvitedOrgUser
 from tests.conftest import ORGANISATION_ID, normalize_spaces
 
 
-def test_view_team_members(
-    client_request,
-    mocker,
-    mock_get_organisation,
-    mock_get_users_for_organisation,
-    mock_get_invited_users_for_organisation,
-    fake_uuid
-):
-    page = client_request.get(
-        '.manage_org_users',
-        org_id=ORGANISATION_ID,
-    )
-
-    for i in range(0, 2):
-        assert normalize_spaces(
-            page.select('.user-list-item .heading-small')[i].text
-        ) == 'Test User {}'.format(i + 1)
-
-    assert normalize_spaces(
-        page.select('.user-list-edit-link')[0].text
-    ) == 'Cancel invitation'
-
-
-@pytest.mark.parametrize('number_of_users', (
-    pytest.param(7, marks=pytest.mark.xfail),
-    pytest.param(8),
-))
-def test_should_show_live_search_if_more_than_7_users(
-    client_request,
-    mocker,
-    mock_get_organisation,
-    active_user_with_permissions,
-    number_of_users,
-):
-    mocker.patch(
-        'app.models.user.OrganisationInvitedUsers.client_method',
-        return_value=[],
-    )
-    mocker.patch(
-        'app.models.user.OrganisationUsers.client_method',
-        return_value=[active_user_with_permissions] * number_of_users,
-    )
-
-    page = client_request.get(
-        '.manage_org_users',
-        org_id=ORGANISATION_ID,
-    )
-
-    assert page.select_one('div[data-module=live-search]')['data-targets'] == (
-        ".user-list-item"
-    )
-    assert len(page.select('.user-list-item')) == number_of_users
-
-    textbox = page.select_one('[data-module=autofocus] .govuk-input')
-    assert 'value' not in textbox
-    assert textbox['name'] == 'search'
-    # data-module=autofocus is set on a containing element so it
-    # shouldn’t also be set on the textbox itself
-    assert 'data-module' not in textbox
-    assert not page.select_one('[data-force-focus]')
-    assert textbox['class'] == [
-        'govuk-input', 'govuk-!-width-full',
-    ]
-    assert normalize_spaces(
-        page.select_one('label[for=search]').text
-    ) == 'Search by name or email address'
-
-
 def test_invite_org_user(
     client_request,
     mocker,
@@ -128,6 +60,34 @@ def test_invite_org_user_errors_when_same_email_as_inviter(
     assert 'You cannot send an invitation to yourself' in normalize_spaces(page.select_one('.govuk-error-message').text)
 
 
+def test_cancel_invited_org_user_cancels_user_invitations(
+    client_request,
+    mock_get_invites_for_organisation,
+    sample_org_invite,
+    mock_get_organisation,
+    mock_get_users_for_organisation,
+    mocker,
+):
+    mock_cancel = mocker.patch('app.org_invite_api_client.cancel_invited_user')
+    mocker.patch('app.org_invite_api_client.get_invited_user_for_org', return_value=sample_org_invite)
+
+    page = client_request.get(
+        'main.cancel_invited_org_user',
+        org_id=ORGANISATION_ID,
+        invited_user_id=sample_org_invite['id'],
+        _follow_redirects=True
+    )
+    assert normalize_spaces(page.h1.text) == 'Team members'
+    flash_banner = normalize_spaces(
+        page.find('div', class_='banner-default-with-tick').text
+    )
+    assert flash_banner == f"Invitation cancelled for {sample_org_invite['email_address']}"
+    mock_cancel.assert_called_once_with(
+        org_id=ORGANISATION_ID,
+        invited_user_id=sample_org_invite['id'],
+    )
+
+
 def test_accepted_invite_when_other_user_already_logged_in(
     client_request,
     mock_check_org_invite_token

tests/app/main/views/organisations/test_organisations.py

@@ -768,33 +768,148 @@ def test_organisation_trial_mode_services_doesnt_work_if_not_platform_admin(
     )
 
 
-def test_cancel_invited_org_user_cancels_user_invitations(
+def test_manage_org_users_shows_correct_link_next_to_each_user(
     client_request,
-    mock_get_invites_for_organisation,
-    sample_org_invite,
     mock_get_organisation,
     mock_get_users_for_organisation,
+    mock_get_invited_users_for_organisation,
+):
+    page = client_request.get(
+        '.manage_org_users',
+        org_id=ORGANISATION_ID,
+    )
+
+    # No banner confirming a user to be deleted shown
+    assert not page.select_one('.banner-dangerous')
+
+    users = page.find_all(class_='user-list-item')
+
+    # The first user is an invited user, so has the link to cancel the invitation.
+    # The second two users are active users, so have the link to be removed from the org
+    assert normalize_spaces(
+        users[0].text
+    ) == 'invited_user@test.gov.uk (invited) Cancel invitation for invited_user@test.gov.uk'
+    assert normalize_spaces(users[1].text) == 'Test User 1 test@gov.uk Remove Test User 1 test@gov.uk'
+    assert normalize_spaces(users[2].text) == 'Test User 2 testt@gov.uk Remove Test User 2 testt@gov.uk'
+
+    assert users[0].a['href'] == url_for(
+        '.cancel_invited_org_user',
+        org_id=ORGANISATION_ID,
+        invited_user_id='73616d70-6c65-4f6f-b267-5f696e766974'
+    )
+    assert users[1].a['href'] == url_for('.edit_organisation_user', org_id=ORGANISATION_ID, user_id='1234')
+    assert users[2].a['href'] == url_for('.edit_organisation_user', org_id=ORGANISATION_ID, user_id='5678')
+
+
+def test_manage_org_users_shows_no_link_for_cancelled_users(
+    client_request,
+    mock_get_organisation,
+    mock_get_users_for_organisation,
+    sample_org_invite,
     mocker,
 ):
-    mock_cancel = mocker.patch('app.org_invite_api_client.cancel_invited_user')
-    mocker.patch('app.org_invite_api_client.get_invited_user_for_org', return_value=sample_org_invite)
+    sample_org_invite['status'] = 'cancelled'
+    mocker.patch('app.models.user.OrganisationInvitedUsers.client_method', return_value=[sample_org_invite])
 
     page = client_request.get(
-        'main.cancel_invited_org_user',
+        '.manage_org_users',
         org_id=ORGANISATION_ID,
-        invited_user_id=sample_org_invite['id'],
-        _follow_redirects=True
     )
-    assert normalize_spaces(page.h1.text) == 'Team members'
-    flash_banner = normalize_spaces(
-        page.find('div', class_='banner-default-with-tick').text
+    users = page.find_all(class_='user-list-item')
+
+    assert normalize_spaces(users[0].text) == 'invited_user@test.gov.uk (cancelled invite)'
+    assert not users[0].a
+
+
+@pytest.mark.parametrize('number_of_users', (
+    pytest.param(7, marks=pytest.mark.xfail),
+    pytest.param(8),
+))
+def test_manage_org_users_should_show_live_search_if_more_than_7_users(
+    client_request,
+    mocker,
+    mock_get_organisation,
+    active_user_with_permissions,
+    number_of_users,
+):
+    mocker.patch(
+        'app.models.user.OrganisationInvitedUsers.client_method',
+        return_value=[],
     )
-    assert flash_banner == f"Invitation cancelled for {sample_org_invite['email_address']}"
-    mock_cancel.assert_called_once_with(
+    mocker.patch(
+        'app.models.user.OrganisationUsers.client_method',
+        return_value=[active_user_with_permissions] * number_of_users,
+    )
+
+    page = client_request.get(
+        '.manage_org_users',
         org_id=ORGANISATION_ID,
-        invited_user_id=sample_org_invite['id'],
     )
 
+    assert page.select_one('div[data-module=live-search]')['data-targets'] == (
+        ".user-list-item"
+    )
+    assert len(page.select('.user-list-item')) == number_of_users
+
+    textbox = page.select_one('[data-module=autofocus] .govuk-input')
+    assert 'value' not in textbox
+    assert textbox['name'] == 'search'
+    # data-module=autofocus is set on a containing element so it
+    # shouldn’t also be set on the textbox itself
+    assert 'data-module' not in textbox
+    assert not page.select_one('[data-force-focus]')
+    assert textbox['class'] == [
+        'govuk-input', 'govuk-!-width-full',
+    ]
+    assert normalize_spaces(
+        page.select_one('label[for=search]').text
+    ) == 'Search by name or email address'
+
+
+def test_edit_organisation_user_shows_the_delete_confirmation_banner(
+    client_request,
+    mock_get_organisation,
+    mock_get_invites_for_organisation,
+    mock_get_users_for_organisation,
+    active_user_with_permissions,
+):
+    page = client_request.get(
+        '.edit_organisation_user',
+        org_id=ORGANISATION_ID,
+        user_id=active_user_with_permissions['id']
+    )
+
+    assert normalize_spaces(page.h1) == 'Team members'
+
+    banner = page.select_one('.banner-dangerous')
+    assert "Are you sure you want to remove Test User?" in normalize_spaces(banner.contents[0])
+    assert banner.form.attrs['action'] == url_for(
+        'main.remove_user_from_organisation',
+        org_id=ORGANISATION_ID,
+        user_id=active_user_with_permissions['id']
+    )
+
+
+def test_remove_user_from_organisation_makes_api_request_to_remove_user(
+    client_request,
+    mocker,
+    mock_get_organisation,
+    fake_uuid,
+):
+    mock_remove_user = mocker.patch('app.organisations_client.remove_user_from_organisation')
+
+    client_request.post(
+        '.remove_user_from_organisation',
+        org_id=ORGANISATION_ID,
+        user_id=fake_uuid,
+        _expected_redirect=url_for(
+            'main.show_accounts_or_dashboard',
+            _external=True,
+        ),
+    )
+
+    mock_remove_user.assert_called_with(ORGANISATION_ID, fake_uuid)
+
 
 def test_organisation_settings_platform_admin_only(
     client_request,

tests/app/notify_client/test_organisation_client.py

@@ -229,3 +229,19 @@ def test_update_service_organisation_deletes_cache(mocker, fake_uuid):
         url='/organisations/{}/service'.format(fake_uuid),
         data=ANY
     )
+
+
+def test_remove_user_from_organisation_deletes_user_cache(mocker):
+    mock_redis_delete = mocker.patch('app.extensions.RedisClient.delete')
+    mock_delete = mocker.patch('app.notify_client.organisations_api_client.OrganisationsClient.delete')
+
+    org_id = 'abcd-1234'
+    user_id = 'efgh-5678'
+
+    organisations_client.remove_user_from_organisation(
+        org_id=org_id,
+        user_id=user_id,
+    )
+
+    assert mock_redis_delete.call_args_list == [call(f'user-{user_id}')]
+    mock_delete.assert_called_with(f'/organisations/{org_id}/users/{user_id}')

tests/app/test_navigation.py

@@ -101,6 +101,7 @@ EXCLUDED_ENDPOINTS = tuple(map(Navigation.get_endpoint_with_blueprint, {
     'edit_organisation_name',
     'edit_organisation_notes',
     'edit_organisation_type',
+    'edit_organisation_user',
     'edit_provider',
     'edit_service_billing_details',
     'edit_service_notes',
@@ -109,7 +110,6 @@ EXCLUDED_ENDPOINTS = tuple(map(Navigation.get_endpoint_with_blueprint, {
     'edit_template_postage',
     'edit_user_email',
     'edit_user_mobile_number',
-    'edit_user_org_permissions',
     'edit_user_permissions',
     'email_branding',
     'email_not_received',