diff --git a/app/assets/stylesheets/components/tick-cross.scss b/app/assets/stylesheets/components/tick-cross.scss index 74fc461ab..3601b0ad4 100644 --- a/app/assets/stylesheets/components/tick-cross.scss +++ b/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; - padding: 1px 0 0 25px; + background-position: 0 6px; + 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; } } diff --git a/app/assets/stylesheets/views/users.scss b/app/assets/stylesheets/views/users.scss index b04cd671d..57b3a2da4 100644 --- a/app/assets/stylesheets/views/users.scss +++ b/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 { diff --git a/app/main/forms.py b/app/main/forms.py index 45ba28782..acdfb5722 100644 --- a/app/main/forms.py +++ b/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") diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index e4a805859..8f45b80fc 100644 --- a/app/main/views/manage_users.py +++ b/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. - # 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) + permissions = ','.join(sorted(get_permissions_from_form(form))) 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 for role, permissions in roles.items() if form[role].data - )) | {'view_activity'} + permissions=set(get_permissions_from_form(form)), ) 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 diff --git a/app/templates/views/edit-user-permissions.html b/app/templates/views/edit-user-permissions.html index 93df5a411..eef384425 100644 --- a/app/templates/views/edit-user-permissions.html +++ b/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 %} diff --git a/app/templates/views/invite-user.html b/app/templates/views/invite-user.html index 34f7adb4a..c0f41fa4e 100644 --- a/app/templates/views/invite-user.html +++ b/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 %}

- {{ "Invite a team member" }} + Invite a team member

diff --git a/app/templates/views/manage-users.html b/app/templates/views/manage-users.html index 555be8279..fb79b3b8a 100644 --- a/app/templates/views/manage-users.html +++ b/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 %}

- {{ user.name }}  + {{ user.name }} {%- 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 %}

- {{ user.email_address }} + {{ user.email_address }}

    @@ -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( diff --git a/app/templates/views/manage-users/permissions.html b/app/templates/views/manage-users/permissions.html index 42824f67d..8427a1b2e 100644 --- a/app/templates/views/manage-users/permissions.html +++ b/app/templates/views/manage-users/permissions.html @@ -5,6 +5,7 @@ Permissions {{ checkbox(form.send_messages) }} + {{ checkbox(form.manage_templates) }} {{ checkbox(form.manage_service) }} {{ checkbox(form.manage_api_keys) }} diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index f62e81423..ce97036f0 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/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 - - -def test_should_show_overview_page( - logged_in_client, +from tests.conftest import ( + normalize_spaces, + SERVICE_ONE_ID, 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=[active_user_with_permissions]) - response = logged_in_client.get(url_for('main.manage_users', service_id=service['id'])) + mocker.patch('app.user_api_client.get_users_for_service', return_value=[user(fake_uuid)]) + page = client_request.get('main.manage_users', service_id=SERVICE_ONE_ID) - assert 'Team members' in response.get_data(as_text=True) - assert response.status_code == 200 - app.user_api_client.get_users_for_service.assert_called_once_with(service_id=service['id']) + assert normalize_spaces(page.select_one('h1').text) == 'Team members' + assert normalize_spaces(page.select_one('.user-list-item').text) == ( + 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, - active_user_with_permissions, - mocker, + client_request, + endpoint, + extra_args, + expected_checkboxes, ): - service = create_sample_service(active_user_with_permissions) - response = logged_in_client.get(url_for('main.edit_user_permissions', service_id=service['id'], user_id=0)) + page = client_request.get(endpoint, service_id=SERVICE_ONE_ID, **extra_args) + 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 invited_users_list.find_all('h3')[0].text.strip() == 'invited_user@test.gov.uk' - assert invited_users_list.find_all('a')[0].text.strip() == 'Cancel invitation' + assert normalize_spaces(page.select('.user-list')[1].text) == ( + 'invited_user@test.gov.uk ' + '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( diff --git a/tests/conftest.py b/tests/conftest.py index 1045cec03..1004022dd 100644 --- a/tests/conftest.py +++ b/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