From 00bb7a0ea05b2beed276da948d0686d3e6b370c7 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Wed, 22 May 2019 11:38:47 +0100 Subject: [PATCH 1/4] Add page to archive user Users can only be archived by Platform Admin from the user page (/users/). This removes them from all services and orgs and updates their details. --- app/main/views/find_users.py | 14 ++++++++- app/navigation.py | 4 +++ app/notify_client/user_api_client.py | 4 +++ .../views/find-users/user-information.html | 5 +++ tests/app/main/views/test_find_users.py | 31 +++++++++++++++++++ tests/app/notify_client/test_user_client.py | 1 + 6 files changed, 58 insertions(+), 1 deletion(-) diff --git a/app/main/views/find_users.py b/app/main/views/find_users.py index 2947b0a41..3016da327 100644 --- a/app/main/views/find_users.py +++ b/app/main/views/find_users.py @@ -1,4 +1,4 @@ -from flask import render_template, request +from flask import flash, redirect, render_template, request, url_for from flask_login import login_required from app import user_api_client @@ -37,3 +37,15 @@ def user_information(user_id): user=user, services=services, ) + + +@main.route("/users//archive", methods=['GET', 'POST']) +@login_required +@user_is_platform_admin +def archive_user(user_id): + if request.method == 'POST': + user_api_client.archive_user(user_id) + return redirect(url_for('.user_information', user_id=user_id)) + else: + flash('There\'s no way to reverse this! Are you sure you want to archive this user?', 'delete') + return user_information(user_id) diff --git a/app/navigation.py b/app/navigation.py index 623d7c7e7..82d73b849 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -80,6 +80,7 @@ class HeaderNavigation(Navigation): }, 'platform-admin': { 'add_organisation', + 'archive_user', 'clear_cache', 'create_email_branding', 'create_letter_branding', @@ -427,6 +428,7 @@ class MainNavigation(Navigation): 'add_service', 'agreement', 'archive_service', + 'archive_user', 'bat_phone', 'callbacks', 'cancel_invited_org_user', @@ -619,6 +621,7 @@ class CaseworkNavigation(Navigation): 'api_integration', 'api_keys', 'archive_service', + 'archive_user', 'bat_phone', 'branding_request', 'callbacks', @@ -902,6 +905,7 @@ class OrgNavigation(Navigation): 'api_integration', 'api_keys', 'archive_service', + 'archive_user', 'bat_phone', 'branding_request', 'callbacks', diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index f074e995f..091d8f1d5 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -65,6 +65,10 @@ class UserApiClient(NotifyAdminAPIClient): user_data = self.post(url, data=data) return user_data['data'] + @cache.delete('user-{user_id}') + def archive_user(self, user_id): + return self.post('/user/{}/archive'.format(user_id), data=None) + @cache.delete('user-{user_id}') def reset_failed_login_count(self, user_id): url = "/user/{}/reset-failed-login-count".format(user_id) diff --git a/app/templates/views/find-users/user-information.html b/app/templates/views/find-users/user-information.html index 7cfa7b8fd..5149d23a8 100644 --- a/app/templates/views/find-users/user-information.html +++ b/app/templates/views/find-users/user-information.html @@ -38,6 +38,11 @@ {{ user.failed_login_count }} failed login attempts

{% endif %} + + + Archive user + + {% endblock %} diff --git a/tests/app/main/views/test_find_users.py b/tests/app/main/views/test_find_users.py index 5af41fc83..225c85258 100644 --- a/tests/app/main/views/test_find_users.py +++ b/tests/app/main/views/test_find_users.py @@ -1,3 +1,4 @@ +from bs4 import BeautifulSoup from flask import url_for from lxml import html @@ -145,3 +146,33 @@ def test_user_information_page_displays_if_there_are_failed_login_attempts( document = html.fromstring(response.get_data(as_text=True)) assert document.xpath("//p/text()[normalize-space()='2 failed login attempts']") + + +def test_archive_user_prompts_for_confirmation( + logged_in_platform_admin_client, + api_user_active, + mock_get_organisations_and_services_for_user, +): + response = logged_in_platform_admin_client.get( + url_for('main.archive_user', user_id=api_user_active.id) + ) + + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert 'Are you sure you want to archive this user?' in page.find('div', class_='banner-dangerous').text + + +def test_archive_user_posts_to_user_client( + logged_in_platform_admin_client, + api_user_active, + mocker, +): + mock_user_client = mocker.patch('app.user_api_client.post') + + response = logged_in_platform_admin_client.post( + url_for('main.archive_user', user_id=api_user_active.id) + ) + + assert response.status_code == 302 + assert response.location == url_for('main.user_information', user_id=api_user_active.id, _external=True) + mock_user_client.assert_called_once_with('/user/{}/archive'.format(api_user_active.id), data=None) diff --git a/tests/app/notify_client/test_user_client.py b/tests/app/notify_client/test_user_client.py index 5210a0a3e..1ab9389b0 100644 --- a/tests/app/notify_client/test_user_client.py +++ b/tests/app/notify_client/test_user_client.py @@ -194,6 +194,7 @@ def test_returns_value_from_cache( (user_api_client, 'add_user_to_organisation', [sample_uuid(), user_id], {}), (user_api_client, 'set_user_permissions', [user_id, SERVICE_ONE_ID, []], {}), (user_api_client, 'activate_user', [api_user_pending(sample_uuid())['id']], {}), + (user_api_client, 'archive_user', [user_id], {}), (service_api_client, 'remove_user_from_service', [SERVICE_ONE_ID, user_id], {}), (service_api_client, 'create_service', ['', '', 0, False, user_id, sample_uuid()], {}), (invite_api_client, 'accept_invite', [SERVICE_ONE_ID, user_id], {}), From 73b4aa4d8590091672b422be731f071746ceee0d Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Wed, 22 May 2019 13:15:31 +0100 Subject: [PATCH 2/4] Refactor event handler code --- app/event_handlers.py | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-) diff --git a/app/event_handlers.py b/app/event_handlers.py index 6a6124513..59fa35703 100644 --- a/app/event_handlers.py +++ b/app/event_handlers.py @@ -4,12 +4,12 @@ from app import events_api_client def on_user_logged_in(_sender, user): - _send_event(event_type='sucessful_login', user_id=user.id) + _send_event('sucessful_login', user_id=user.id) def create_email_change_event(user_id, updated_by_id, original_email_address, new_email_address): _send_event( - event_type='update_user_email', + 'update_user_email', user_id=user_id, updated_by_id=updated_by_id, original_email_address=original_email_address, @@ -18,30 +18,18 @@ def create_email_change_event(user_id, updated_by_id, original_email_address, ne def create_mobile_number_change_event(user_id, updated_by_id, original_mobile_number, new_mobile_number): _send_event( - event_type='update_user_mobile_number', + 'update_user_mobile_number', user_id=user_id, updated_by_id=updated_by_id, original_mobile_number=original_mobile_number, new_mobile_number=new_mobile_number) -def _send_event(**kwargs): - if not kwargs.get('event_type'): - return - +def _send_event(event_type, **kwargs): event_data = _construct_event_data(request) - event_fields = ('user_id', - 'updated_by_id', - 'original_email_address', - 'new_email_address', - 'original_mobile_number', - 'new_mobile_number') + event_data.update(kwargs) - for field in event_fields: - if kwargs.get(field): - event_data[field] = kwargs[field] - - events_api_client.create_event(kwargs['event_type'], event_data) + events_api_client.create_event(event_type, event_data) def _construct_event_data(request): From f57f8641ada499632623ce5a7bb47ee2aa26a494 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Wed, 22 May 2019 14:05:19 +0100 Subject: [PATCH 3/4] Add an event if a user is archived This adds a new type of event, 'archive_user', which stores the id of the archived user and the id of the user who is doing the archiving. --- app/event_handlers.py | 7 +++++++ app/main/views/find_users.py | 5 ++++- tests/app/main/views/test_find_users.py | 23 +++++++++++++++++++++++ tests/app/test_event_handlers.py | 16 ++++++++++++++++ 4 files changed, 50 insertions(+), 1 deletion(-) diff --git a/app/event_handlers.py b/app/event_handlers.py index 59fa35703..166a694b9 100644 --- a/app/event_handlers.py +++ b/app/event_handlers.py @@ -25,6 +25,13 @@ def create_mobile_number_change_event(user_id, updated_by_id, original_mobile_nu new_mobile_number=new_mobile_number) +def create_archive_user_event(user_id, archived_by_id): + _send_event( + 'archive_user', + user_id=user_id, + archived_by_id=archived_by_id) + + def _send_event(event_type, **kwargs): event_data = _construct_event_data(request) event_data.update(kwargs) diff --git a/app/main/views/find_users.py b/app/main/views/find_users.py index 3016da327..3d04eb42e 100644 --- a/app/main/views/find_users.py +++ b/app/main/views/find_users.py @@ -1,7 +1,8 @@ from flask import flash, redirect, render_template, request, url_for -from flask_login import login_required +from flask_login import current_user, login_required from app import user_api_client +from app.event_handlers import create_archive_user_event from app.main import main from app.main.forms import SearchUsersByEmailForm from app.models.user import User @@ -45,6 +46,8 @@ def user_information(user_id): def archive_user(user_id): if request.method == 'POST': user_api_client.archive_user(user_id) + create_archive_user_event(str(user_id), current_user.id) + return redirect(url_for('.user_information', user_id=user_id)) else: flash('There\'s no way to reverse this! Are you sure you want to archive this user?', 'delete') diff --git a/tests/app/main/views/test_find_users.py b/tests/app/main/views/test_find_users.py index 225c85258..9d3993ad4 100644 --- a/tests/app/main/views/test_find_users.py +++ b/tests/app/main/views/test_find_users.py @@ -1,3 +1,4 @@ +import pytest from bs4 import BeautifulSoup from flask import url_for from lxml import html @@ -166,6 +167,7 @@ def test_archive_user_posts_to_user_client( logged_in_platform_admin_client, api_user_active, mocker, + mock_events, ): mock_user_client = mocker.patch('app.user_api_client.post') @@ -176,3 +178,24 @@ def test_archive_user_posts_to_user_client( assert response.status_code == 302 assert response.location == url_for('main.user_information', user_id=api_user_active.id, _external=True) mock_user_client.assert_called_once_with('/user/{}/archive'.format(api_user_active.id), data=None) + + assert mock_events.called + + +def test_archive_user_does_not_create_event_if_user_client_raises_exception( + logged_in_platform_admin_client, + api_user_active, + mocker, + mock_events, +): + mock_user_client = mocker.patch('app.user_api_client.post', side_effect=Exception()) + + with pytest.raises(Exception): + response = logged_in_platform_admin_client.post( + url_for('main.archive_user', user_id=api_user_active.id) + ) + + assert response.status_code == 500 + assert response.location == url_for('main.user_information', user_id=api_user_active.id, _external=True) + mock_user_client.assert_called_once_with('/user/{}/archive'.format(api_user_active.id), data=None) + assert not mock_events.called diff --git a/tests/app/test_event_handlers.py b/tests/app/test_event_handlers.py index ef72a038f..e132188af 100644 --- a/tests/app/test_event_handlers.py +++ b/tests/app/test_event_handlers.py @@ -2,6 +2,7 @@ import uuid from unittest.mock import ANY from app.event_handlers import ( + create_archive_user_event, create_email_change_event, create_mobile_number_change_event, on_user_logged_in, @@ -51,3 +52,18 @@ def test_create_mobile_number_change_event_calls_events_api(app_, mock_events): 'updated_by_id': updated_by_id, 'original_mobile_number': '07700900000', 'new_mobile_number': '07700900999'}) + + +def test_create_archive_user_event_calls_events_api(app_, mock_events): + user_id = str(uuid.uuid4()) + archived_by_id = str(uuid.uuid4()) + + with app_.test_request_context(): + create_archive_user_event(user_id, archived_by_id) + + mock_events.assert_called_with('archive_user', + {'browser_fingerprint': + {'browser': ANY, 'version': ANY, 'platform': ANY, 'user_agent_string': ''}, + 'ip_address': ANY, + 'user_id': user_id, + 'archived_by_id': archived_by_id}) From a6f5abbf7ef570fa305a6ec18d66f2070ba91529 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Wed, 5 Jun 2019 11:25:27 +0100 Subject: [PATCH 4/4] Only show archive user link for active users --- .../views/find-users/user-information.html | 12 +++-- tests/app/main/views/test_find_users.py | 46 ++++++++++++++++--- tests/conftest.py | 2 + 3 files changed, 49 insertions(+), 11 deletions(-) diff --git a/app/templates/views/find-users/user-information.html b/app/templates/views/find-users/user-information.html index 5149d23a8..1f89d2f89 100644 --- a/app/templates/views/find-users/user-information.html +++ b/app/templates/views/find-users/user-information.html @@ -38,11 +38,13 @@ {{ user.failed_login_count }} failed login attempts

{% endif %} - - - Archive user - - + {% if user.state == 'active' %} + + + Archive user + + + {% endif %} {% endblock %} diff --git a/tests/app/main/views/test_find_users.py b/tests/app/main/views/test_find_users.py index 9d3993ad4..703cae515 100644 --- a/tests/app/main/views/test_find_users.py +++ b/tests/app/main/views/test_find_users.py @@ -4,6 +4,7 @@ from flask import url_for from lxml import html from tests import user_json +from tests.conftest import normalize_spaces def test_find_users_by_email_page_loads_correctly(client_request, platform_admin_user): @@ -149,13 +150,46 @@ def test_user_information_page_displays_if_there_are_failed_login_attempts( assert document.xpath("//p/text()[normalize-space()='2 failed login attempts']") +def test_user_information_page_shows_archive_link_for_active_users( + logged_in_platform_admin_client, + api_user_active, + mock_get_organisations_and_services_for_user, +): + response = logged_in_platform_admin_client.get( + url_for('main.user_information', user_id=api_user_active['id']) + ) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + archive_url = url_for('main.archive_user', user_id=api_user_active['id']) + + link = page.find('a', {'href': archive_url}) + assert normalize_spaces(link.text) == 'Archive user' + + +def test_user_information_page_does_not_show_archive_link_for_inactive_users( + mocker, + client, + platform_admin_user, + mock_get_organisations_and_services_for_user, +): + inactive_user = user_json(state='inactive') + mocker.patch('app.user_api_client.get_user', side_effect=[platform_admin_user, inactive_user], autospec=True) + client.login(platform_admin_user) + response = client.get( + url_for('main.user_information', user_id=inactive_user['id']) + ) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + + archive_url = url_for('main.archive_user', user_id=inactive_user['id']) + assert not page.find('a', {'href': archive_url}) + + def test_archive_user_prompts_for_confirmation( logged_in_platform_admin_client, api_user_active, mock_get_organisations_and_services_for_user, ): response = logged_in_platform_admin_client.get( - url_for('main.archive_user', user_id=api_user_active.id) + url_for('main.archive_user', user_id=api_user_active['id']) ) assert response.status_code == 200 @@ -172,12 +206,12 @@ def test_archive_user_posts_to_user_client( mock_user_client = mocker.patch('app.user_api_client.post') response = logged_in_platform_admin_client.post( - url_for('main.archive_user', user_id=api_user_active.id) + url_for('main.archive_user', user_id=api_user_active['id']) ) assert response.status_code == 302 - assert response.location == url_for('main.user_information', user_id=api_user_active.id, _external=True) - mock_user_client.assert_called_once_with('/user/{}/archive'.format(api_user_active.id), data=None) + assert response.location == url_for('main.user_information', user_id=api_user_active['id'], _external=True) + mock_user_client.assert_called_once_with('/user/{}/archive'.format(api_user_active['id']), data=None) assert mock_events.called @@ -196,6 +230,6 @@ def test_archive_user_does_not_create_event_if_user_client_raises_exception( ) assert response.status_code == 500 - assert response.location == url_for('main.user_information', user_id=api_user_active.id, _external=True) - mock_user_client.assert_called_once_with('/user/{}/archive'.format(api_user_active.id), data=None) + assert response.location == url_for('main.user_information', user_id=api_user_active['id'], _external=True) + mock_user_client.assert_called_once_with('/user/{}/archive'.format(api_user_active['id']), data=None) assert not mock_events.called diff --git a/tests/conftest.py b/tests/conftest.py index 35fb3ebf1..4944606ef 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1117,6 +1117,7 @@ def platform_admin_user(fake_uuid): 'services': [], 'organisations': [], 'current_session_id': None, + 'logged_in_at': None, } return user_data @@ -1137,6 +1138,7 @@ def api_user_active(fake_uuid, email_address='test@user.gov.uk'): 'services': [], 'organisations': [], 'current_session_id': None, + 'logged_in_at': None, } return user_data