From c138a4a5e065b88a3221e6119de968079d089bd2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 22 Mar 2016 13:18:06 +0000 Subject: [PATCH] Set permissions with checkboxes, not yes/no inputs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The yes/no pattern didn’t work too well, because: - it didn’t read naturally as a question and answer - often users left them completely unclicked if they didn’t want to set the permission (rather than clicking no) This commit changes both the invite and edit user pages to use checkboxes to set permissions. If also rewords these pages to read more naturally, and explain what the permissions mean. This meant changing some of the view logic around invites and persmissions, and I ended up refactoring a bunch of it because I found it hard to understand what was going on. --- app/assets/stylesheets/app.scss | 4 + app/assets/stylesheets/components/yes-no.scss | 42 -------- app/assets/stylesheets/main.scss | 1 - app/main/forms.py | 27 ++--- app/main/views/manage_users.py | 98 ++++++++----------- app/main/views/styleguide.py | 4 - app/templates/components/checkbox.html | 9 +- app/templates/components/yes-no.html | 22 ----- .../views/edit-user-permissions.html | 17 ++-- app/templates/views/invite-user.html | 16 +-- app/templates/views/manage-users.html | 2 +- .../views/manage-users/permissions.html | 21 ++++ app/templates/views/styleguide.html | 12 --- tests/app/main/views/test_manage_users.py | 52 +++++----- 14 files changed, 116 insertions(+), 211 deletions(-) delete mode 100644 app/assets/stylesheets/components/yes-no.scss delete mode 100644 app/templates/components/yes-no.html create mode 100644 app/templates/views/manage-users/permissions.html diff --git a/app/assets/stylesheets/app.scss b/app/assets/stylesheets/app.scss index f1f9c8fc7..f8ae0c7de 100644 --- a/app/assets/stylesheets/app.scss +++ b/app/assets/stylesheets/app.scss @@ -107,4 +107,8 @@ td { } +} + +.form-label { + margin-bottom: 5px; } \ No newline at end of file diff --git a/app/assets/stylesheets/components/yes-no.scss b/app/assets/stylesheets/components/yes-no.scss deleted file mode 100644 index 9dd2101b7..000000000 --- a/app/assets/stylesheets/components/yes-no.scss +++ /dev/null @@ -1,42 +0,0 @@ -.yes-no-wrapper { - border-bottom: 1px solid $border-colour; - margin: 0 0 $gutter 0; -} - -.yes-no { - - border-top: 1px solid $border-colour; - padding: 10px 0; - - &-label { - - padding-top: 19px; - float: left; - - &.error { - padding-top: 5px; - } - - } - - &-fields { - - text-align: right; - - .block-label { - - @include media(tablet) { - - margin-bottom: 0; - - &:last-child { - margin-right: 0; - } - - } - - } - - } - -} diff --git a/app/assets/stylesheets/main.scss b/app/assets/stylesheets/main.scss index 74db12573..67e7ab9d2 100644 --- a/app/assets/stylesheets/main.scss +++ b/app/assets/stylesheets/main.scss @@ -47,7 +47,6 @@ $path: '/static/images/'; @import 'components/browse-list'; @import 'components/email-message'; @import 'components/api-key'; -@import 'components/yes-no'; @import "components/_previous-next-navigation"; @import 'views/job'; diff --git a/app/main/forms.py b/app/main/forms.py index 2c36d8217..3e50ee657 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -96,31 +96,16 @@ class RegisterUserFromInviteForm(Form): email_address = HiddenField('email_address') -# WTF forms does not give a handy way to customise error messages for -# radio button fields so just overriding the default here for use -# in permissions form. -class CustomRadioField(RadioField): +class PermissionsForm(Form): - def pre_validate(self, form): - for v, _ in self.choices: - if self.data == v: - break - else: - raise ValueError(self.gettext('Choose yes or no')) + send_messages = BooleanField("Send messages from existing templates") + manage_service = BooleanField("Modify this service, its team, and its templates") + manage_api_keys = BooleanField("Create and revoke API keys") -class PermisisonsForm(Form): +class InviteUserForm(PermissionsForm): - # TODO fix this Radio field so we are not having to test for yes or no rather - # use operator equality. - send_messages = CustomRadioField("Send messages", choices=[('yes', 'yes'), ('no', 'no')]) - manage_service = CustomRadioField("Manage service", choices=[('yes', 'yes'), ('no', 'no')]) - manage_api_keys = CustomRadioField("Manage API keys", choices=[('yes', 'yes'), ('no', 'no')]) - - -class InviteUserForm(PermisisonsForm): - - email_address = email_address('Their email address') + email_address = email_address('Email address') def __init__(self, invalid_email_address, *args, **kwargs): super(InviteUserForm, self).__init__(*args, **kwargs) diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index cb77fb625..3d43bcec2 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -1,3 +1,5 @@ +from itertools import chain +from collections import OrderedDict from flask import ( request, render_template, @@ -14,7 +16,7 @@ from flask_login import ( from app.main import main from app.main.forms import ( InviteUserForm, - PermisisonsForm + PermissionsForm ) from app.main.dao.services_dao import get_service_by_id from app import user_api_client @@ -22,41 +24,53 @@ from app import invite_api_client from app.utils import user_has_permissions +roles = { + 'send_messages': ['send_texts', 'send_emails', 'send_letters'], + 'manage_service': ['manage_users', 'manage_templates', 'manage_settings'], + 'manage_api_keys': ['manage_api_keys', 'access_developer_docs'] +} + + @main.route("/services//users") @login_required @user_has_permissions('manage_users', admin_override=True) def manage_users(service_id): - users = user_api_client.get_users_for_service(service_id=service_id) - invited_users = invite_api_client.get_invites_for_service(service_id=service_id) - filtered_invites = [] - for invite in invited_users: - if invite.status != 'accepted': - filtered_invites.append(invite) - return render_template('views/manage-users.html', - service_id=service_id, - users=users, - current_user=current_user, - invited_users=filtered_invites) + return render_template( + 'views/manage-users.html', + service_id=service_id, + users=user_api_client.get_users_for_service(service_id=service_id), + current_user=current_user, + invited_users=[ + invite for invite in invite_api_client.get_invites_for_service(service_id=service_id) + if invite.status != 'accepted' + ] + ) @main.route("/services//users/invite", methods=['GET', 'POST']) @login_required @user_has_permissions('manage_users', admin_override=True) def invite_user(service_id): - service = get_service_by_id(service_id) + get_service_by_id(service_id) + + form = InviteUserForm(invalid_email_address=current_user.email_address) - form = InviteUserForm(current_user.email_address) if form.validate_on_submit(): email_address = form.email_address.data - permissions = _get_permissions(request.form) - invited_user = invite_api_client.create_invite(current_user.id, service_id, email_address, permissions) + invited_user = invite_api_client.create_invite( + current_user.id, + service_id, + email_address, + ','.join( + role for role in roles.keys() if request.form.get(role) == 'y' + ) + ) flash('Invite sent to {}'.format(invited_user.email_address), 'default_with_tick') return redirect(url_for('.manage_users', service_id=service_id)) return render_template( 'views/invite-user.html', - user=None, service_id=service_id, form=form ) @@ -69,27 +83,20 @@ def edit_user_permissions(service_id, user_id): # TODO we should probably using the service id here in the get user # call as well. eg. /user/?&service_id=service_id user = user_api_client.get_user(user_id) - service = get_service_by_id(service_id) + 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 = PermisisonsForm(**{ - 'send_messages': 'yes' if user.has_permissions( - permissions=['send_texts', 'send_emails', 'send_letters']) else 'no', - 'manage_service': 'yes' if user.has_permissions( - permissions=['manage_users', 'manage_templates', 'manage_settings']) else 'no', - 'manage_api_keys': 'yes' if user.has_permissions( - permissions=['manage_api_keys', 'access_developer_docs']) else 'no' - }) + form = PermissionsForm(**{ + role: user.has_permissions(permissions=permissions) for role, permissions in roles.items() + }) if form.validate_on_submit(): - permissions = [] - permissions.extend( - _convert_role_to_permissions('send_messages') if form.send_messages.data == 'yes' else []) - permissions.extend( - _convert_role_to_permissions('manage_service') if form.manage_service.data == 'yes' else []) - permissions.extend( - _convert_role_to_permissions('manage_api_keys') if form.manage_api_keys.data == 'yes' else []) - user_api_client.set_user_permissions(user_id, service_id, permissions) + user_api_client.set_user_permissions( + user_id, service_id, + permissions=set(chain.from_iterable( + permissions for role, permissions in roles.items() if form[role].data + )) + ) return redirect(url_for('.manage_users', service_id=service_id)) return render_template( @@ -106,26 +113,3 @@ def cancel_invited_user(service_id, invited_user_id): invite_api_client.cancel_invited_user(service_id=service_id, invited_user_id=invited_user_id) return redirect(url_for('main.manage_users', service_id=service_id)) - - -def _convert_role_to_permissions(role): - if role == 'send_messages': - return ['send_texts', 'send_emails', 'send_letters'] - elif role == 'manage_service': - return ['manage_users', 'manage_templates', 'manage_settings'] - elif role == 'manage_api_keys': - return ['manage_api_keys', 'access_developer_docs'] - return [] - - -# TODO replace with method which converts each 'role' into the list -# of permissions like the method above :) -def _get_permissions(form): - permissions = [] - if form.get('send_messages') and form['send_messages'] == 'yes': - permissions.append('send_messages') - if form.get('manage_service') and form['manage_service'] == 'yes': - permissions.append('manage_service') - if form.get('manage_api_keys') and form['manage_api_keys'] == 'yes': - permissions.append('manage_api_keys') - return ','.join(permissions) diff --git a/app/main/views/styleguide.py b/app/main/views/styleguide.py index 7d6186aa1..eed30b20e 100644 --- a/app/main/views/styleguide.py +++ b/app/main/views/styleguide.py @@ -1,7 +1,6 @@ from flask import render_template, current_app, abort from flask_wtf import Form from wtforms import StringField, PasswordField, TextAreaField, FileField, validators -from app.main.forms import CustomRadioField from utils.template import Template from app.main import main @@ -18,14 +17,11 @@ def styleguide(): code = StringField('Enter code') message = TextAreaField(u'Message') file_upload = FileField('Upload a CSV file to add your recipients’ details') - manage_service = CustomRadioField('Manage service', choices=[('yes', 'yes'), ('no', 'no')]) - manage_templates = CustomRadioField('Manage templates', choices=[('yes', 'yes'), ('no', 'no')]) sms = "Your vehicle tax for ((registration number)) is due on ((date)). Renew online at www.gov.uk/vehicle-tax" form = FormExamples() form.message.data = sms - form.manage_service.data = 'yes' form.validate() template = Template({'content': sms}) diff --git a/app/templates/components/checkbox.html b/app/templates/components/checkbox.html index e816f8aeb..5ae251520 100644 --- a/app/templates/components/checkbox.html +++ b/app/templates/components/checkbox.html @@ -4,9 +4,11 @@ help_link=None, help_link_text=None, width='2-3', - suffix=None + suffix=None, + block=False ) %} -