From 646ba6e8c332cfc139bd11c10cd3d640a2d16f04 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 6 Aug 2018 11:10:37 +0100 Subject: [PATCH] =?UTF-8?q?Add=20a=20=E2=80=98See=20dashboard=E2=80=99=20p?= =?UTF-8?q?ermission?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Our research and prototyping around ‘basic view’ found that: - a lot of users who send messages rarely or never look at the dashboard (yet it’s the first page they see when they sign in) - team managers like the idea of taking away things that users don’t need in order to make the interface simpler We’ve disentangled the simpler way of sending messages from being part of ‘basic view’. This means we can give managers the option of taking away the dashboard as an independent choice, not something that’s wrapped up in a separate ‘view’. I think that this checkbox is a more straightforward proposition than ‘basic view’ ever was (despite all the work we did to explain it and develop the nested checkbox pattern). In research users would often explain the feature back to us as being about hiding the dashboard – we should try to make Notify operate in terms of concepts that come naturally to people wherever possible. --- app/main/forms.py | 56 +-- app/main/views/manage_users.py | 37 +- app/notify_client/models.py | 2 +- app/templates/main_nav.html | 21 +- app/templates/views/manage-users.html | 49 +-- .../views/manage-users/permissions.html | 60 +-- tests/app/main/views/test_manage_users.py | 354 +++++++----------- tests/app/main/views/test_templates.py | 3 +- tests/app/test_navigation.py | 4 +- tests/conftest.py | 21 ++ 10 files changed, 220 insertions(+), 387 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index c6b2837cf..d9527b21e 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -256,13 +256,13 @@ class RegisterUserFromOrgInviteForm(StripWhitespaceForm): auth_type = HiddenField('auth_type', validators=[DataRequired()]) -class AbstractPermissionsForm(StripWhitespaceForm): +class PermissionsForm(StripWhitespaceForm): - view_activity = HiddenField("View activity") - send_messages = BooleanField("Send messages from existing templates") + view_activity = BooleanField("See dashboard and reports") + send_messages = BooleanField("Send messages") manage_templates = BooleanField("Add and edit templates") manage_service = BooleanField("Manage this service and its team") - manage_api_keys = BooleanField("Create and revoke API keys") + manage_api_keys = BooleanField("Manage API keys") login_authentication = RadioField( 'Sign in using', @@ -277,38 +277,18 @@ class AbstractPermissionsForm(StripWhitespaceForm): def permissions(self): return {role for role in roles.keys() if self[role].data is True} - -class AdminPermissionsForm(AbstractPermissionsForm): - - def process(self, *args, **kwargs): - super().process(*args, **kwargs) - # view_activity is a default role to be added to all users. - self.view_activity.data = True + @classmethod + def from_user(cls, user, service_id): + return cls( + **{ + role: user.has_permission_for_service(service_id, role) + for role in roles.keys() + }, + login_authentication=user.auth_type + ) -class CaseworkingPermissionsForm(AbstractPermissionsForm): - - def process(self, *args, **kwargs): - super().process(*args, **kwargs) - if self.user_type.data == 'admin': - self.view_activity.data = True - elif self.user_type.data == 'caseworker': - self.view_activity.data = False - self.manage_templates.data = False - self.manage_service.data = False - self.manage_api_keys.data = False - self.send_messages.data = True - - user_type = RadioField( - 'User type', - choices=[ - ('caseworker', 'Basic view'), - ('admin', 'Admin view'), - ], - ) - - -class AbstractInviteUserForm(StripWhitespaceForm): +class InviteUserForm(PermissionsForm): email_address = email_address(gov_user=False) def __init__(self, invalid_email_address, *args, **kwargs): @@ -320,14 +300,6 @@ class AbstractInviteUserForm(StripWhitespaceForm): raise ValidationError("You can’t send an invitation to yourself") -class AdminInviteUserForm(AbstractInviteUserForm, AdminPermissionsForm): - pass - - -class CaseworkingInviteUserForm(AbstractInviteUserForm, CaseworkingPermissionsForm): - pass - - class InviteOrgUserForm(StripWhitespaceForm): email_address = email_address(gov_user=False) diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 7c65a4157..a2aa715c7 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -1,5 +1,3 @@ -from functools import partial - from flask import abort, flash, redirect, render_template, request, url_for from flask_login import current_user, login_required from notifications_python_client.errors import HTTPError @@ -11,14 +9,7 @@ from app import ( user_api_client, ) from app.main import main -from app.main.forms import ( - AdminInviteUserForm, - AdminPermissionsForm, - CaseworkingInviteUserForm, - CaseworkingPermissionsForm, - SearchUsersForm, -) -from app.notify_client.models import roles +from app.main.forms import InviteUserForm, PermissionsForm, SearchUsersForm from app.utils import user_has_permissions @@ -48,12 +39,7 @@ def manage_users(service_id): @user_has_permissions('manage_service') def invite_user(service_id): - if current_service.has_permission('caseworking'): - form = CaseworkingInviteUserForm - else: - form = AdminInviteUserForm - - form = form(invalid_email_address=current_user.email_address) + form = InviteUserForm(invalid_email_address=current_user.email_address) service_has_email_auth = current_service.has_permission('email_auth') if not service_has_email_auth: @@ -89,18 +75,7 @@ def edit_user_permissions(service_id, user_id): user = user_api_client.get_user(user_id) user_has_no_mobile_number = user.mobile_number is None - if current_service.has_permission('caseworking'): - form = partial( - CaseworkingPermissionsForm, - user_type='admin' if user.has_permission_for_service(service_id, 'view_activity') else 'caseworker', - ) - else: - form = AdminPermissionsForm - - form = form( - **{role: user.has_permission_for_service(service_id, role) for role in roles.keys()}, - login_authentication=user.auth_type - ) + form = PermissionsForm.from_user(user, service_id) if form.validate_on_submit(): user_api_client.set_user_permissions( @@ -125,11 +100,7 @@ def edit_user_permissions(service_id, user_id): @user_has_permissions('manage_service') def remove_user_from_service(service_id, user_id): user = user_api_client.get_user(user_id) - # Need to make the email address read only, or a disabled field? - # Do it through the template or the form class? - form = AdminPermissionsForm(**{ - role: user.has_permission_for_service(service_id, role) for role in roles.keys() - }) + form = PermissionsForm.from_user(user, service_id) if request.method == 'POST': try: diff --git a/app/notify_client/models.py b/app/notify_client/models.py index a6c8c1840..f91bb4c41 100644 --- a/app/notify_client/models.py +++ b/app/notify_client/models.py @@ -197,7 +197,7 @@ class InvitedUser(object): return set(self.permissions) > set(permissions) def has_permission_for_service(self, service_id, permission): - if self.status == 'cancelled' and permission != 'view_activity': + if self.status == 'cancelled': return False return self.service == service_id and permission in self.permissions diff --git a/app/templates/main_nav.html b/app/templates/main_nav.html index 3dea9b82d..2259a982d 100644 --- a/app/templates/main_nav.html +++ b/app/templates/main_nav.html @@ -1,10 +1,18 @@ {% if help %} {% include 'partials/tour.html' %} -{% elif current_user.has_permissions('view_activity') %} +{% else %} -{% else %} - {% endif %} diff --git a/app/templates/views/manage-users.html b/app/templates/views/manage-users.html index 2bd5fa562..aa1d32aa8 100644 --- a/app/templates/views/manage-users.html +++ b/app/templates/views/manage-users.html @@ -63,35 +63,26 @@