From 5b658d924c491b23b1a1f2319bc027bb8ce0cc98 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Wed, 5 Jan 2022 14:53:21 +0000 Subject: [PATCH 1/6] Tweak and add test for OrganisationsClient.remove_user_from_organisation This stops adding the current user to the data sent to the API when removing a user from an organisation. API only needs to know the organisation_id and the id of the user we want to remove from the organisation, so we don't need to pass through the id of the current user too. The other change made is to clear the user cache of the user who has been removed from the org. We don't need to clear any of the organisation caches, since these values don't contain lists of users for the orgs. --- app/notify_client/organisations_api_client.py | 7 +++---- .../notify_client/test_organisation_client.py | 16 ++++++++++++++++ 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/app/notify_client/organisations_api_client.py b/app/notify_client/organisations_api_client.py index 467ab6791..c073ba9b2 100644 --- a/app/notify_client/organisations_api_client.py +++ b/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( diff --git a/tests/app/notify_client/test_organisation_client.py b/tests/app/notify_client/test_organisation_client.py index aba5fa5c1..92b0e7167 100644 --- a/tests/app/notify_client/test_organisation_client.py +++ b/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}') From d166c6382c10334e02bdc2d79443c487be6a8235 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Tue, 11 Jan 2022 14:16:37 +0000 Subject: [PATCH 2/6] Add link next to org team members allowing them to be removed This adds a link next to the organisation team members which lets them be removed from the organisation. Service team members have their own page and the link to remove them appears there. For organisation team members, we don't currently have any other information we want to show or any other actions to perform. As a result, this change uses the 'Team members' page to show the confirmation banner. The endpoint called 'edit_user_org_permissions' was renamed to 'edit_organisation_user' and some of the existing code around deleting org users (which didn't work) was changed. --- app/main/views/organisations.py | 42 ++++------ app/navigation.py | 2 +- .../organisation/users/index.html | 11 +++ .../views/organisations/test_organisations.py | 76 +++++++++++++++++++ tests/app/test_navigation.py | 2 +- 5 files changed, 103 insertions(+), 30 deletions(-) diff --git a/app/main/views/organisations.py b/app/main/views/organisations.py index 058b543cf..edcc60f98 100644 --- a/app/main/views/organisations.py +++ b/app/main/views/organisations.py @@ -253,42 +253,28 @@ def invite_org_user(org_id): ) -@main.route("/organisations//users/", methods=['GET', 'POST']) +@main.route("/organisations//users/", 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//users//delete", methods=['GET', 'POST']) +@main.route("/organisations//users//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//cancel-invited-user/", methods=['GET']) diff --git a/app/navigation.py b/app/navigation.py index 7bed95211..8e2710fa2 100644 --- a/app/navigation.py +++ b/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', diff --git a/app/templates/views/organisations/organisation/users/index.html b/app/templates/views/organisations/organisation/users/index.html index fd466eb34..56fa4fd82 100644 --- a/app/templates/views/organisations/organisation/users/index.html +++ b/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 %} +

Team members

@@ -45,6 +54,8 @@
{% if user.status == 'pending' %} Cancel invitation + {% else %} + Remove {{ user.name }} {{ user.email_address }} {% endif %}
diff --git a/tests/app/main/views/organisations/test_organisations.py b/tests/app/main/views/organisations/test_organisations.py index 7cb8e40c8..da67bb313 100644 --- a/tests/app/main/views/organisations/test_organisations.py +++ b/tests/app/main/views/organisations/test_organisations.py @@ -768,6 +768,82 @@ def test_organisation_trial_mode_services_doesnt_work_if_not_platform_admin( ) +def test_manage_org_users_shows_correct_link_next_to_each_user( + client_request, + 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' + 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_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_cancel_invited_org_user_cancels_user_invitations( client_request, mock_get_invites_for_organisation, diff --git a/tests/app/test_navigation.py b/tests/app/test_navigation.py index 5052413dd..54dd81dc3 100644 --- a/tests/app/test_navigation.py +++ b/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', From de2afb2f1a8b152d5e7e44710fb401237f712bbc Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Tue, 11 Jan 2022 15:59:52 +0000 Subject: [PATCH 3/6] Add hidden text next to the 'Cancel invitation' link This matches what we do on the service team members page. --- .../views/organisations/organisation/users/index.html | 2 +- tests/app/main/views/organisations/test_organisations.py | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app/templates/views/organisations/organisation/users/index.html b/app/templates/views/organisations/organisation/users/index.html index 56fa4fd82..9880bf2d7 100644 --- a/app/templates/views/organisations/organisation/users/index.html +++ b/app/templates/views/organisations/organisation/users/index.html @@ -53,7 +53,7 @@
{% if user.status == 'pending' %} - Cancel invitation + Cancel invitation for {{ user.email_address }} {% else %} Remove {{ user.name }} {{ user.email_address }} {% endif %} diff --git a/tests/app/main/views/organisations/test_organisations.py b/tests/app/main/views/organisations/test_organisations.py index da67bb313..6d60ede22 100644 --- a/tests/app/main/views/organisations/test_organisations.py +++ b/tests/app/main/views/organisations/test_organisations.py @@ -786,7 +786,9 @@ def test_manage_org_users_shows_correct_link_next_to_each_user( # 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' + 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' From 5bf0e8638eb0080dafe9c3ccdabe814217c61cf1 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Tue, 11 Jan 2022 14:25:25 +0000 Subject: [PATCH 4/6] Remove two unused org templates One of these was empty. The other could not be viewed without giving a `500` status code and isn't needed or linked to from anywhere. --- .../organisation/users/user/index.html | 31 ------------------- .../users/user/remove-org-user.html | 0 2 files changed, 31 deletions(-) delete mode 100644 app/templates/views/organisations/organisation/users/user/index.html delete mode 100644 app/templates/views/organisations/organisation/users/user/remove-org-user.html diff --git a/app/templates/views/organisations/organisation/users/user/index.html b/app/templates/views/organisations/organisation/users/user/index.html deleted file mode 100644 index 8c6018538..000000000 --- a/app/templates/views/organisations/organisation/users/user/index.html +++ /dev/null @@ -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) }} - -

- {{ user.email_address }} -

- - {% 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 %} diff --git a/app/templates/views/organisations/organisation/users/user/remove-org-user.html b/app/templates/views/organisations/organisation/users/user/remove-org-user.html deleted file mode 100644 index e69de29bb..000000000 From daed4a7f7f1ecec59e819d45811b6636d133a051 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Tue, 11 Jan 2022 14:38:58 +0000 Subject: [PATCH 5/6] Move org tests between files This deletes the `test_view_team_members` test since it is now duplicated by a new test. It also moves a test relating to org invites to test_organisation_invites.py and one that wasn't related to invites from that file to test_organisations.py. --- .../test_organisation_invites.py | 96 ++++++------------- .../views/organisations/test_organisations.py | 73 ++++++++------ 2 files changed, 73 insertions(+), 96 deletions(-) diff --git a/tests/app/main/views/organisations/test_organisation_invites.py b/tests/app/main/views/organisations/test_organisation_invites.py index ce8cfbb5b..dc497c6b7 100644 --- a/tests/app/main/views/organisations/test_organisation_invites.py +++ b/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 diff --git a/tests/app/main/views/organisations/test_organisations.py b/tests/app/main/views/organisations/test_organisations.py index 6d60ede22..7b2e06ceb 100644 --- a/tests/app/main/views/organisations/test_organisations.py +++ b/tests/app/main/views/organisations/test_organisations.py @@ -801,6 +801,51 @@ def test_manage_org_users_shows_correct_link_next_to_each_user( assert users[2].a['href'] == url_for('.edit_organisation_user', org_id=ORGANISATION_ID, user_id='5678') +@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=[], + ) + 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_edit_organisation_user_shows_the_delete_confirmation_banner( client_request, mock_get_organisation, @@ -846,34 +891,6 @@ def test_remove_user_from_organisation_makes_api_request_to_remove_user( mock_remove_user.assert_called_with(ORGANISATION_ID, fake_uuid) -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_organisation_settings_platform_admin_only( client_request, mock_get_organisation, From f3baacfb563c50b8282a603816216581efbf73f8 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Wed, 12 Jan 2022 11:49:58 +0000 Subject: [PATCH 6/6] Stop showing link for cancelled org users --- .../organisation/users/index.html | 2 +- .../views/organisations/test_organisations.py | 20 +++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/app/templates/views/organisations/organisation/users/index.html b/app/templates/views/organisations/organisation/users/index.html index 9880bf2d7..80d5db14a 100644 --- a/app/templates/views/organisations/organisation/users/index.html +++ b/app/templates/views/organisations/organisation/users/index.html @@ -54,7 +54,7 @@
{% if user.status == 'pending' %} Cancel invitation for {{ user.email_address }} - {% else %} + {% elif user.status != 'cancelled' %} Remove {{ user.name }} {{ user.email_address }} {% endif %}
diff --git a/tests/app/main/views/organisations/test_organisations.py b/tests/app/main/views/organisations/test_organisations.py index 7b2e06ceb..8ad264489 100644 --- a/tests/app/main/views/organisations/test_organisations.py +++ b/tests/app/main/views/organisations/test_organisations.py @@ -801,6 +801,26 @@ def test_manage_org_users_shows_correct_link_next_to_each_user( 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, +): + sample_org_invite['status'] = 'cancelled' + mocker.patch('app.models.user.OrganisationInvitedUsers.client_method', return_value=[sample_org_invite]) + + page = client_request.get( + '.manage_org_users', + org_id=ORGANISATION_ID, + ) + 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),