Merge pull request #1435 from alphagov/manage-templates-permission

Separate ‘manage service’ and ‘manage templates’ permissions

app/assets/stylesheets/components/tick-cross.scss

@@ -4,8 +4,8 @@
   display: inline-block;
   background-size: 19px 19px;
   background-repeat: no-repeat;
-  background-position: 0 0;
+  background-position: 0 6px;
-  padding: 1px 0 0 25px;
+  padding: 6px 0 5px 25px;
 
   @include ie-lte(8) {
     background-position: 0 3px;
@@ -31,6 +31,7 @@
     @extend %tick-cross;
     color: $secondary-text-colour;
     background-image: file-url('cross-grey.png');
+    box-shadow: inset 20px 0 0 0 rgba(255, 255, 255, 0.6);
 
     @include ie-lte(8) {
       background-image: file-url('cross-grey-16px.png');
@@ -42,21 +43,24 @@
 
     @extend %grid-row;
     margin-top: 5px;
+    position: relative;
 
     &-permissions {
 
       @include grid-column(3/4);
 
       li {
-        display: inline-block;
+        display: block;
         margin-right: 0.5em;
       }
 
     }
 
     &-edit-link {
-      @include grid-column(1/4);
       text-align: right;
+      position: absolute;
+      top: -1.6em;
+      right: -135px;
     }
 
   }

app/assets/stylesheets/views/users.scss

@@ -5,7 +5,7 @@
 
   &-item {
 
-    padding: $gutter-half 0;
+    padding: $gutter-half 150px $gutter-half 0;
     border-top: 1px solid $border-colour;
 
     &:last-child {

app/main/forms.py

@@ -171,7 +171,8 @@ class RegisterUserFromInviteForm(Form):
 
 class PermissionsForm(Form):
     send_messages = BooleanField("Send messages from existing templates")
-    manage_service = BooleanField("Modify this service, its team, and its templates")
+    manage_templates = BooleanField("Add and edit templates")
+    manage_service = BooleanField("Modify this service and its team")
     manage_api_keys = BooleanField("Create and revoke API keys")
 
 

app/main/views/manage_users.py

@@ -25,7 +25,8 @@ 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_templates': ['manage_templates'],
+    'manage_service': ['manage_users', 'manage_settings'],
     'manage_api_keys': ['manage_api_keys']
 }
 
@@ -54,14 +55,7 @@ def invite_user(service_id):
 
     if form.validate_on_submit():
         email_address = form.email_address.data
-        # view_activity is a default role to be added to all users.
+        permissions = ','.join(sorted(get_permissions_from_form(form)))
-        # All users will have at minimum view_activity to allow users to see notifications,
-        # templates, team members but no update privileges
-        selected_permissions = [permissions for role, permissions in roles.items() if request.form.get(role) == 'y']
-        selected_permissions = list(chain.from_iterable(selected_permissions))
-        selected_permissions.append('view_activity')
-        selected_permissions.sort()
-        permissions = ','.join(selected_permissions)
         invited_user = invite_api_client.create_invite(
             current_user.id,
             service_id,
@@ -94,9 +88,7 @@ def edit_user_permissions(service_id, user_id):
     if form.validate_on_submit():
         user_api_client.set_user_permissions(
             user_id, service_id,
-            permissions=set(chain.from_iterable(
+            permissions=set(get_permissions_from_form(form)),
-                permissions for role, permissions in roles.items() if form[role].data
-            )) | {'view_activity'}
         )
         return redirect(url_for('.manage_users', service_id=service_id))
 
@@ -150,3 +142,17 @@ 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 get_permissions_from_form(form):
+    # view_activity is a default role to be added to all users.
+    # All users will have at minimum view_activity to allow users to see notifications,
+    # templates, team members but no update privileges
+    selected_permissions = [
+        permissions
+        for role, permissions in roles.items()
+        if form[role].data is True
+    ]
+    selected_permissions = list(chain.from_iterable(selected_permissions))
+    selected_permissions.append('view_activity')
+    return selected_permissions

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

@@ -3,7 +3,7 @@
 {% from "components/page-footer.html" import page_footer %}
 
 {% block service_page_title %}
-Manage users
+  {{ user.name or user.email_localpart }}
 {% endblock %}
 
 {% block maincolumn_content %}

app/templates/views/invite-user.html

@@ -4,13 +4,13 @@
 {% from "components/page-footer.html" import page_footer %}
 
 {% block service_page_title %}
-Manage users
+  Invite a team member
 {% endblock %}
 
 {% block maincolumn_content %}
 
   <h1 class="heading-large">
-    {{ "Invite a team member" }}
+    Invite a team member
   </h1>
 
   <div class="grid-row">

app/templates/views/manage-users.html

@@ -12,7 +12,7 @@
 } %}
 
 {% block service_page_title %}
-Manage users
+  Team members
 {% endblock %}
 
 {% block maincolumn_content %}
@@ -37,7 +37,7 @@ Manage users
     {% for user in users %}
       <div class="user-list-item">
         <h3>
-          {{ user.name }}&ensp;<span class="hint">
+          <span class="heading-small">{{ user.name }}</span>&ensp;<span class="hint">
             {%- if user.email_address == current_user.email_address -%}
               (you)
             {% else %}
@@ -52,7 +52,11 @@ Manage users
               'Send messages'
             ) }}
             {{ tick_cross(
-              user.has_permissions(permissions=['manage_users', 'manage_templates', 'manage_settings']),
+              user.has_permissions(permissions=['manage_templates']),
+              'Manage templates'
+            ) }}
+            {{ tick_cross(
+              user.has_permissions(permissions=['manage_users', 'manage_settings']),
               'Manage service'
             ) }}
             {{ tick_cross(
@@ -80,7 +84,7 @@ Manage users
       {% for user in invited_users %}
         <div class="user-list-item">
           <h3>
-            {{ user.email_address }}
+            <span style="font-weight: bold">{{ user.email_address }}</span>
           </h3>
           <ul class="tick-cross-list">
             <div class="tick-cross-list-permissions">
@@ -89,7 +93,11 @@ Manage users
                 'Send messages'
               ) }}
               {{ tick_cross(
-                user.has_permissions(permissions=['manage_users', 'manage_templates', 'manage_settings']),
+                user.has_permissions(permissions=['manage_templates']),
+                'Edit templates'
+              ) }}
+              {{ tick_cross(
+                user.has_permissions(permissions=['manage_users', 'manage_settings']),
                 'Manage service'
               ) }}
               {{ tick_cross(

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

@@ -5,6 +5,7 @@
     Permissions
   </legend>
   {{ checkbox(form.send_messages) }}
+  {{ checkbox(form.manage_templates) }}
   {{ checkbox(form.manage_service) }}
   {{ checkbox(form.manage_api_keys) }}
 </fieldset>

tests/app/main/views/test_manage_users.py

@@ -5,32 +5,93 @@ import app
 from app.notify_client.models import InvitedUser
 from app.utils import is_gov_user
 from tests.conftest import service_one as create_sample_service
-
+from tests.conftest import (
-
+    normalize_spaces,
-def test_should_show_overview_page(
+    SERVICE_ONE_ID,
-    logged_in_client,
     active_user_with_permissions,
+    active_user_view_permissions,
+    active_user_manage_template_permission,
+)
+
+
+@pytest.mark.parametrize('user, expected_text', [
+    (
+        active_user_with_permissions,
+        (
+            'Test User (you) '
+            'Can Send messages Can Manage templates Can Manage service Can Access API keys'
+        ),
+    ),
+    (
+        active_user_view_permissions,
+        (
+            'Test User With Permissions (you) '
+            'Can’t Send messages Can’t Manage templates Can’t Manage service Can’t Access API keys'
+        ),
+    ),
+    (
+        active_user_manage_template_permission,
+        (
+            'Test User With Permissions (you) '
+            'Can’t Send messages Can Manage templates Can’t Manage service Can’t Access API keys'
+        ),
+    ),
+])
+def test_should_show_overview_page(
+    client_request,
     mocker,
     mock_get_invites_for_service,
+    fake_uuid,
+    user,
+    expected_text,
 ):
-    service = create_sample_service(active_user_with_permissions)
+    mocker.patch('app.user_api_client.get_users_for_service', return_value=[user(fake_uuid)])
-    mocker.patch('app.user_api_client.get_users_for_service', return_value=[active_user_with_permissions])
+    page = client_request.get('main.manage_users', service_id=SERVICE_ONE_ID)
-    response = logged_in_client.get(url_for('main.manage_users', service_id=service['id']))
 
-    assert 'Team members' in response.get_data(as_text=True)
+    assert normalize_spaces(page.select_one('h1').text) == 'Team members'
-    assert response.status_code == 200
+    assert normalize_spaces(page.select_one('.user-list-item').text) == (
-    app.user_api_client.get_users_for_service.assert_called_once_with(service_id=service['id'])
+        expected_text
+    )
+    app.user_api_client.get_users_for_service.assert_called_once_with(service_id=SERVICE_ONE_ID)
 
 
+@pytest.mark.parametrize('endpoint, extra_args, expected_checkboxes', [
+    (
+        'main.edit_user_permissions',
+        {'user_id': 0},
+        [
+            ('send_messages', True),
+            ('manage_templates', True),
+            ('manage_service', True),
+            ('manage_api_keys', True),
+        ]
+    ),
+    (
+        'main.invite_user',
+        {},
+        [
+            ('send_messages', False),
+            ('manage_templates', False),
+            ('manage_service', False),
+            ('manage_api_keys', False),
+        ]
+    ),
+])
 def test_should_show_page_for_one_user(
-    logged_in_client,
+    client_request,
-    active_user_with_permissions,
+    endpoint,
-    mocker,
+    extra_args,
+    expected_checkboxes,
 ):
-    service = create_sample_service(active_user_with_permissions)
+    page = client_request.get(endpoint, service_id=SERVICE_ONE_ID, **extra_args)
-    response = logged_in_client.get(url_for('main.edit_user_permissions', service_id=service['id'], user_id=0))
+    checkboxes = page.select('input[type=checkbox]')
 
-    assert response.status_code == 200
+    assert len(checkboxes) == 4
+
+    for index, expected in enumerate(expected_checkboxes):
+        expected_input_name, expected_checked = expected
+        assert checkboxes[index]['name'] == expected_input_name
+        assert checkboxes[index].has_attr('checked') == expected_checked
 
 
 def test_edit_user_permissions(
@@ -45,6 +106,7 @@ def test_edit_user_permissions(
         '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': 'y',
+             'manage_templates': 'y',
              'manage_service': 'y',
              'manage_api_keys': 'y'})
 
@@ -141,6 +203,7 @@ def test_invite_user(
         url_for('main.invite_user', service_id=service['id']),
         data={'email_address': email_address,
               'send_messages': 'y',
+              'manage_templates': 'y',
               'manage_service': 'y',
               'manage_api_keys': 'y'},
         follow_redirects=True
@@ -177,25 +240,24 @@ def test_cancel_invited_user_cancels_user_invitations(
 
 
 def test_manage_users_shows_invited_user(
-    logged_in_client,
+    client_request,
     mocker,
     active_user_with_permissions,
     sample_invite,
 ):
-    service = create_sample_service(active_user_with_permissions)
     data = [InvitedUser(**sample_invite)]
 
     mocker.patch('app.invite_api_client.get_invites_for_service', return_value=data)
     mocker.patch('app.user_api_client.get_users_for_service', return_value=[active_user_with_permissions])
 
-    response = logged_in_client.get(url_for('main.manage_users', service_id=service['id']))
+    page = client_request.get('main.manage_users', service_id=SERVICE_ONE_ID)
 
-    assert response.status_code == 200
-    page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
     assert page.h1.string.strip() == 'Team members'
-    invited_users_list = page.find_all('div', {'class': 'user-list'})[1]
+    assert normalize_spaces(page.select('.user-list')[1].text) == (
-    assert invited_users_list.find_all('h3')[0].text.strip() == 'invited_user@test.gov.uk'
+        'invited_user@test.gov.uk '
-    assert invited_users_list.find_all('a')[0].text.strip() == 'Cancel invitation'
+        'Can’t Send messages Can’t Edit templates Can’t Manage service Can Access API keys '
+        'Cancel invitation'
+    )
 
 
 def test_manage_users_does_not_show_accepted_invite(

tests/conftest.py

@@ -680,6 +680,29 @@ def active_user_view_permissions(fake_uuid):
     return user
 
 
+@pytest.fixture
+def active_user_manage_template_permission(fake_uuid):
+    from app.notify_client.user_api_client import User
+
+    user_data = {
+        'id': fake_uuid,
+        'name': 'Test User With Permissions',
+        'password': 'somepassword',
+        'password_changed_at': str(datetime.utcnow()),
+        'email_address': 'test@user.gov.uk',
+        'mobile_number': '07700 900762',
+        'state': 'active',
+        'failed_login_count': 0,
+        'permissions': {SERVICE_ONE_ID: [
+            'manage_templates',
+            'view_activity',
+        ]},
+        'platform_admin': False
+    }
+    user = User(user_data)
+    return user
+
+
 @pytest.fixture(scope='function')
 def api_user_locked(fake_uuid):
     from app.notify_client.user_api_client import User