Merge pull request #332 from alphagov/make-permissions-checkboxes

Set permissions with checkboxes, not yes/no inputs

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,40 +24,52 @@ 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
 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
     )
@@ -68,27 +82,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(
@@ -105,26 +112,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
             )