Set permissions with checkboxes, not yes/no inputs

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

@@ -107,4 +107,8 @@ td {
 
   }
 
+}
+
+.form-label {
+  margin-bottom: 5px;
 }

app/assets/stylesheets/components/yes-no.scss

@@ -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;
-        }
-
-      }
-
-    }
-
-  }
-
-}

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';

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)

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/<service_id>/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/<service_id>/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/<user_id>?&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)

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})

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
 ) %}
-    <label class="form-checkbox" for="{{ field.name }}">
+    <label class="{% if block %}block-label{% endif %}" for="{{ field.name }}">
+        {{ field()}}
         {{ field.label }}
         {% if hint %}
             <span class="form-hint">
@@ -18,9 +20,6 @@
                 {{ field.errors[0] }}
             </span>
         {% endif %}
-        {{ field(**{
-            'class': 'form-control form-control-{} textbox-highlight-textbox'.format(width) if highlight_tags else 'form-control form-control-{} {}'.format(width, 'textbox-right-aligned' if suffix else ''),
-            'data-module': 'highlight-tags' if highlight_tags else ''})}}
         {% if help_link and help_link_text %}
             <p class="textbox-help-link">
                 <a href='{{ help_link }}'>{{ help_link_text }}</a>

app/templates/components/yes-no.html

@@ -1,22 +0,0 @@
-{% macro yes_no(field, current_value=None) %}
-  <fieldset class='yes-no'>
-    <legend class='yes-no-label{% if field.errors %} error{% endif %}'>
-      {{ field.label }}
-       {% if field.errors %}
-        <span class="error-message">
-          {{ field.errors[0] }}
-        </span>
-      {% endif %}
-    </legend>
-    <div class='yes-no-fields inline'>
-      <label class='block-label'>
-        <input type='radio' name='{{ field.name }}' value='yes' {% if current_value == 'yes' %}checked{% endif %} />
-        Yes
-      </label>
-      <label class='block-label'>
-        <input type='radio' name='{{ field.name }}' value='no' {% if current_value == 'no' %}checked{% endif %} />
-        No
-      </label>
-    </div>
-  </fieldset>
-{% endmacro %}

app/templates/views/edit-user-permissions.html

@@ -1,5 +1,4 @@
 {% extends "withnav_template.html" %}
-{% from "components/yes-no.html" import yes_no %}
 {% from "components/textbox.html" import textbox %}
 {% from "components/page-footer.html" import page_footer %}
 
@@ -10,21 +9,17 @@ Manage users – GOV.UK Notify
 {% block maincolumn_content %}
 
   <h1 class="heading-large">
-    {{ user.name or user.email_localpart or "Add a new team member" }}
+    {{ user.name or user.email_localpart }}
   </h1>
 
+  <p>
+    {{ user.email_address }}
+  </p>
+
   <div class="grid-row">
     <form method="post" class="column-three-quarters">
 
-      <fieldset class='yes-no-wrapper'>
-        <legend class='heading-small'>
-          Permissions
-        </legend>
-        <span class="form-hint">All team members can see message history</span>
-        {{ yes_no(form.send_messages, form.send_messages.data) }}
-        {{ yes_no(form.manage_service, form.manage_service.data) }}
-        {{ yes_no(form.manage_api_keys, form.manage_api_keys.data) }}
-      </fieldset>
+      {% include 'views/manage-users/permissions.html' %}
 
       {{ page_footer(
         'Save',

app/templates/views/invite-user.html

@@ -1,5 +1,5 @@
 {% extends "withnav_template.html" %}
-{% from "components/yes-no.html" import yes_no %}
+{% from "components/checkbox.html" import checkbox %}
 {% from "components/textbox.html" import textbox %}
 {% from "components/page-footer.html" import page_footer %}
 
@@ -10,23 +10,15 @@ Manage users – GOV.UK Notify
 {% block maincolumn_content %}
 
   <h1 class="heading-large">
-    {{ user.name or user.email_localpart or "Invite a team member" }}
+    {{ "Invite a team member" }}
   </h1>
 
   <div class="grid-row">
     <form method="post" class="column-three-quarters">
 
-      {{ textbox(form.email_address, hint='You must use an email address from a central government organisation', width='1-1', safe_error_message=True) }}
+      {{ textbox(form.email_address, hint='Must be from a central government organisation', width='1-1', safe_error_message=True) }}
 
-      <fieldset class='yes-no-wrapper'>
-        <legend class='heading-small'>
-          Permissions
-        </legend>
-        <span class="form-hint">All team members can see message history</span>
-        {{ yes_no(form.send_messages, form.send_messages.data) }}
-        {{ yes_no(form.manage_service, form.manage_service.data) }}
-        {{ yes_no(form.manage_api_keys, form.manage_api_keys.data) }}
-      </fieldset>
+      {% include 'views/manage-users/permissions.html' %}
 
       {{ page_footer('Send invitation email') }}
 

app/templates/views/manage-users.html

@@ -4,7 +4,7 @@
 
 {% set table_options = {
   'field_headings': [
-    'Name', 'Send messages', 'Manage service', 'Manage API keys', hidden_field_heading('Link to change')
+    'Name', 'Send messages', 'Modify service', 'Access API keys', hidden_field_heading('Link to change')
   ],
   'field_headings_visible': True,
   'caption_visible': True

app/templates/views/manage-users/permissions.html

@@ -0,0 +1,21 @@
+{% from "components/checkbox.html" import checkbox %}
+
+<fieldset class="form-group">
+  <legend class="form-label">
+    Permissions
+  </legend>
+  {{ checkbox(form.send_messages, block=True) }}
+  {{ checkbox(form.manage_service, block=True) }}
+  {{ checkbox(form.manage_api_keys, block=True) }}
+</fieldset>
+
+<div class="form-group">
+  <p class="form-label">
+    All team members can see
+  </p>
+  <ul class="list list-bullet">
+    <li>templates</li>
+    <li>history of sent messages</li>
+    <li>who the other team members are</li>
+  </ul>
+</div>

app/templates/views/styleguide.html

@@ -9,7 +9,6 @@
 {% from "components/table.html" import mapping_table, list_table, row, field, text_field, boolean_field, right_aligned_field_heading %}
 {% from "components/textbox.html" import textbox %}
 {% from "components/file-upload.html" import file_upload %}
-{% from "components/yes-no.html" import yes_no %}
 {% from "components/api-key.html" import api_key %}
 
 {% block page_title %}
@@ -243,17 +242,6 @@
   <h2 class="heading-large">File upload</h2>
   {{ file_upload(form.file_upload) }}
 
-  <h2 class="heading-large">Yes/no</h2>
-  <div class="grid-row">
-    <div class='column-half'>
-      <fieldset class='yes-no-wrapper'>
-        {{ yes_no(form.manage_service, form.manage_service.data) }}
-        {{ yes_no(form.manage_templates, form.manage_templates.data) }}
-      </fieldset>
-    </div>
-  </div>
-
-
   <h2 class="heading-large">API key</h2>
 
   {{ api_key('d30512af92e1386d63b90e5973b49a10') }}

tests/app/main/views/test_manage_users.py

@@ -61,9 +61,9 @@ def test_edit_user_permissions(
             response = client.post(url_for(
                 'main.edit_user_permissions', service_id=service['id'], user_id=active_user_with_permissions.id
             ), data={'email_address': active_user_with_permissions.email_address,
-                     'send_messages': 'yes',
-                     'manage_service': 'yes',
-                     'manage_api_keys': 'yes'})
+                     'send_messages': 'y',
+                     'manage_service': 'y',
+                     'manage_api_keys': 'y'})
 
         assert response.status_code == 302
         assert response.location == url_for(
@@ -72,14 +72,17 @@ def test_edit_user_permissions(
         mock_set_user_permissions.assert_called_with(
             str(active_user_with_permissions.id),
             service['id'],
-            ['send_texts',
-             'send_emails',
-             'send_letters',
-             'manage_users',
-             'manage_templates',
-             'manage_settings',
-             'manage_api_keys',
-             'access_developer_docs'])
+            permissions={
+                'send_texts',
+                'send_emails',
+                'send_letters',
+                'manage_users',
+                'manage_templates',
+                'manage_settings',
+                'manage_api_keys',
+                'access_developer_docs'
+            }
+        )
 
 
 def test_edit_some_user_permissions(
@@ -102,9 +105,9 @@ def test_edit_some_user_permissions(
             response = client.post(url_for(
                 'main.edit_user_permissions', service_id=service_id, user_id=active_user_with_permissions.id
             ), data={'email_address': active_user_with_permissions.email_address,
-                     'send_messages': 'yes',
-                     'manage_service': 'no',
-                     'manage_api_keys': 'no'})
+                     'send_messages': 'y',
+                     'manage_service': '',
+                     'manage_api_keys': ''})
 
         assert response.status_code == 302
         assert response.location == url_for(
@@ -113,9 +116,12 @@ def test_edit_some_user_permissions(
         mock_set_user_permissions.assert_called_with(
             str(active_user_with_permissions.id),
             service_id,
-            ['send_texts',
-             'send_emails',
-             'send_letters'])
+            permissions={
+                'send_texts',
+                'send_emails',
+                'send_letters'
+            }
+        )
 
 
 def _mocks_for_test_manage_users(mocker, active_user_with_permissions, service):
@@ -161,9 +167,9 @@ def test_invite_user(
             response = client.post(
                 url_for('main.invite_user', service_id=service['id']),
                 data={'email_address': email_address,
-                      'send_messages': 'yes',
-                      'manage_service': 'yes',
-                      'manage_api_keys': 'yes'},
+                      'send_messages': 'y',
+                      'manage_service': 'y',
+                      'manage_api_keys': 'y'},
                 follow_redirects=True
             )
 
@@ -267,9 +273,9 @@ def test_user_cant_invite_themselves(
             response = client.post(
                 url_for('main.invite_user', service_id=service['id']),
                 data={'email_address': active_user_with_permissions.email_address,
-                      'send_messages': 'yes',
-                      'manage_service': 'yes',
-                      'manage_api_keys': 'yes'},
+                      'send_messages': 'y',
+                      'manage_service': 'y',
+                      'manage_api_keys': 'y'},
                 follow_redirects=True
             )