From f3689cc11323d0365b889cdf5ea44521b3a846b1 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Wed, 23 Mar 2016 10:46:31 +0000 Subject: [PATCH] Functionality added and all tests working. Update correct use of permissions form. --- app/__init__.py | 6 +++ app/main/views/manage_users.py | 44 +++++++++++++++++- app/notify_client/api_client.py | 9 ++++ app/templates/flash_messages.html | 2 +- .../views/edit-user-permissions.html | 10 ++--- app/templates/views/manage-users.html | 2 +- tests/app/main/views/test_manage_users.py | 45 +++++++++++++++++++ tests/conftest.py | 5 +++ 8 files changed, 115 insertions(+), 8 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index d17998fc6..f835b5828 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -200,6 +200,12 @@ def register_errorhandlers(application): def handle_no_permissions(error): return _error_response(401) + @application.errorhandler(500) + def handle_exception(error): + if current_app.config.get('DEBUG', None): + raise error + return _error_response(500) + @application.errorhandler(Exception) def handle_bad_request(error): # We want the Flask in browser stacktrace diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 8c2736915..4f38751b6 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -5,7 +5,8 @@ from flask import ( render_template, redirect, url_for, - flash + flash, + abort ) from flask_login import ( @@ -13,6 +14,7 @@ from flask_login import ( current_user ) +from notifications_python_client import HTTPError from app.main import main from app.main.forms import ( InviteUserForm, @@ -20,6 +22,7 @@ from app.main.forms import ( ) from app.main.dao.services_dao import get_service_by_id from app import user_api_client +from app import service_api_client from app import invite_api_client from app.utils import user_has_permissions @@ -106,6 +109,45 @@ def edit_user_permissions(service_id, user_id): ) +@main.route("/services//users//delete", methods=['GET', 'POST']) +@login_required +@user_has_permissions('manage_users', admin_override=True) +def remove_user_from_service(service_id, user_id): + user = user_api_client.get_user(user_id) + service = get_service_by_id(service_id) + # Need to make the email address read only, or a disabled field? + # Do it through the template or the form class? + form = PermissionsForm(**{ + role: user.has_permissions(permissions=permissions) for role, permissions in roles.items() + }) + + 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 + )) + + flash('Are you sure you want to remove {}?'.format(user.name), 'remove') + return render_template( + 'views/edit-user-permissions.html', + user=user, + form=form, + service_id=service_id + ) + + @main.route("/services//cancel-invited-user/", methods=['GET']) @user_has_permissions('manage_users', admin_override=True) def cancel_invited_user(service_id, invited_user_id): diff --git a/app/notify_client/api_client.py b/app/notify_client/api_client.py index 6f703e622..76c62a796 100644 --- a/app/notify_client/api_client.py +++ b/app/notify_client/api_client.py @@ -70,6 +70,15 @@ class ServiceAPIClient(NotificationsAPIClient): endpoint = "/service/{0}".format(service_id) return self.post(endpoint, data) + def remove_user_from_service(self, service_id, user_id): + """ + Remove a user from a service + """ + endpoint = '/service/{service_id}/users/{user_id}'.format( + service_id=service_id, + user_id=user_id) + return self.delete(endpoint) + def create_service_template(self, name, type_, content, service_id, subject=None): """ Create a service template. diff --git a/app/templates/flash_messages.html b/app/templates/flash_messages.html index bf109421e..283689d8b 100644 --- a/app/templates/flash_messages.html +++ b/app/templates/flash_messages.html @@ -5,7 +5,7 @@ {{ banner( message, 'default' if ((category == 'default') or (category == 'default_with_tick')) else 'dangerous', - delete_button="Yes, delete" if 'delete' == category else None, + delete_button="Yes, {}".format(category) if category in ['delete', 'remove'] else None, with_tick=True if category == 'default_with_tick' else False )}} {% endfor %} diff --git a/app/templates/views/edit-user-permissions.html b/app/templates/views/edit-user-permissions.html index 979e6c2a1..315414122 100644 --- a/app/templates/views/edit-user-permissions.html +++ b/app/templates/views/edit-user-permissions.html @@ -21,11 +21,11 @@ Manage users – GOV.UK Notify {% include 'views/manage-users/permissions.html' %} - {{ page_footer( - 'Save', - back_link=url_for('.manage_users', service_id=service_id), - back_link_text='Cancel' - ) }} + {{ page_footer( + 'Save', + delete_link=url_for('.remove_user_from_service', service_id=service_id, user_id=user.id) if user or None, + delete_link_text='Remove user from service' + ) }} diff --git a/app/templates/views/manage-users.html b/app/templates/views/manage-users.html index 5677e5578..0a7a5ada9 100644 --- a/app/templates/views/manage-users.html +++ b/app/templates/views/manage-users.html @@ -45,7 +45,7 @@ Manage users – GOV.UK Notify {% call field(align='right') %} {% if current_user.has_permissions(['manage_users']) %} {% if current_user.id != item.id %} - Edit permission + Edit {% endif %} {% endif %} {% endcall %} diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index a7a12cd9b..e51e7e602 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -302,3 +302,48 @@ def test_no_permission_manage_users_page(app_, assert url_for('.invite_user', service_id=service_one['id']) not in resp_text assert "Edit permission" not in resp_text assert "Manage team" not in resp_text + + +def test_get_remove_user_from_service(app_, + api_user_active, + mock_login, + mock_get_user_by_email, + mock_get_service, + mock_get_users_by_service, + mock_get_user, + mock_has_permissions): + with app_.test_request_context(): + with app_.test_client() as client: + service = mock_get_service("12345")['data'] + client.login(api_user_active) + response = client.get( + url_for( + 'main.remove_user_from_service', + service_id=service['id'], + user_id=api_user_active.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) + + +def test_remove_user_from_service(app_, + api_user_active, + mock_login, + mock_get_user_by_email, + mock_get_service, + mock_get_users_by_service, + mock_get_user, + mock_has_permissions, + mock_remove_user_from_service): + with app_.test_request_context(): + with app_.test_client() as client: + service = mock_get_service("12345")['data'] + client.login(api_user_active) + response = client.post( + url_for( + 'main.remove_user_from_service', + service_id=service['id'], + user_id=api_user_active.id)) + assert response.status_code == 302 + assert response.location == url_for( + 'main.manage_users', service_id=service['id'], _external=True) diff --git a/tests/conftest.py b/tests/conftest.py index d93a14ebf..155213833 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -757,3 +757,8 @@ def mock_add_user_to_service(mocker, service_one, api_user_active): @pytest.fixture(scope='function') def mock_set_user_permissions(mocker): return mocker.patch('app.user_api_client.set_user_permissions', return_value=None) + + +@pytest.fixture(scope='function') +def mock_remove_user_from_service(mocker): + return mocker.patch('app.service_api_client.remove_user_from_service', return_value=None)