From ffdcda02bf52b30d642a27e386b3baf9877a2eb3 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Tue, 21 Apr 2020 17:40:47 +0100 Subject: [PATCH] Show error when user cannot be archived A user can't be archived if they are the only member of their service with `manage_settings` permission. `notifications-api` returns a `400` and an error message if that is the case, however this PR to remove the `400` error handler https://github.com/alphagov/notifications-admin/pull/3320 stopped the error message from showing. This meant that instead of seeing a message about why a user couldn't be archived, we would just show a `500` error page instead. This change checks the response from `notifications-api` and shows an error banner with a message if the user can't be archived. --- app/main/views/find_users.py | 9 ++++++- tests/app/main/views/test_find_users.py | 34 +++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/app/main/views/find_users.py b/app/main/views/find_users.py index 5f7fd57b7..10850cf9f 100644 --- a/app/main/views/find_users.py +++ b/app/main/views/find_users.py @@ -1,5 +1,6 @@ from flask import flash, redirect, render_template, request, url_for from flask_login import current_user +from notifications_python_client.errors import HTTPError from app import user_api_client from app.event_handlers import create_archive_user_event @@ -36,7 +37,13 @@ def user_information(user_id): @user_is_platform_admin def archive_user(user_id): if request.method == 'POST': - user_api_client.archive_user(user_id) + try: + user_api_client.archive_user(user_id) + except HTTPError as e: + if e.status_code == 400 and 'manage_settings' in e.message: + flash('User can’t be removed from a service - ' + 'check all services have another team member with manage_settings') + return redirect(url_for('main.user_information', user_id=user_id)) create_archive_user_event(str(user_id), current_user.id) return redirect(url_for('.user_information', user_id=user_id)) diff --git a/tests/app/main/views/test_find_users.py b/tests/app/main/views/test_find_users.py index ffa2d2415..7e8fce81a 100644 --- a/tests/app/main/views/test_find_users.py +++ b/tests/app/main/views/test_find_users.py @@ -4,6 +4,7 @@ import pytest from bs4 import BeautifulSoup from flask import url_for from lxml import html +from notifications_python_client.errors import HTTPError from tests import user_json from tests.conftest import normalize_spaces @@ -221,6 +222,39 @@ def test_archive_user_posts_to_user_client( assert mock_events.called +def test_archive_user_shows_error_message_if_user_cannot_be_archived( + platform_admin_client, + api_user_active, + mocker, + mock_get_non_empty_organisations_and_services_for_user, +): + mocker.patch( + 'app.user_api_client.post', + side_effect=HTTPError( + response=mocker.Mock( + status_code=400, + json={'result': 'error', + 'message': 'User can’t be removed from a service - check all services have another ' + 'team member with manage_settings'} + ), + message='User can’t be removed from a service - check all services have another team member ' + 'with manage_settings' + ) + ) + + response = platform_admin_client.post( + url_for('main.archive_user', user_id=api_user_active['id']), + follow_redirects=True + ) + + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + + assert normalize_spaces(page.find('h1').text) == 'Platform admin user' + assert normalize_spaces( + page.select_one('.banner-dangerous').text + ) == 'User can’t be removed from a service - check all services have another team member with manage_settings' + + def test_archive_user_does_not_create_event_if_user_client_raises_exception( platform_admin_client, api_user_active,