From fd5dfdf4adfac3bbc6421b32f581738a7e4ca042 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 17 Aug 2017 10:30:51 +0100 Subject: [PATCH 1/5] Refactor form -> permissions function into helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This functionality was implemented twice, in two different ways. This commit factors it out into a helper method that can be reused in the two places. We chose to go with the more explicit implementation because it’s easier to understand. --- app/main/views/manage_users.py | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index e4a805859..1931fad5a 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -54,14 +54,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 +87,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 +141,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 From 726e91bebbef50c5591a2ac47d4a947a1792b277 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 17 Aug 2017 10:44:36 +0100 Subject: [PATCH 2/5] Make existing tests use ``client_request fixture MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit makes the existing tests around user permissions less verbose by using the new `client_request` fixture. This fixture takes care of: - setting up a service - asserting that the response is `200` It also tests that the page titles, some of which didn’t match with the `

`s, so this commit also fixes that mismatch. --- .../views/edit-user-permissions.html | 2 +- app/templates/views/manage-users.html | 2 +- tests/app/main/views/test_manage_users.py | 30 ++++++++----------- 3 files changed, 14 insertions(+), 20 deletions(-) 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/manage-users.html b/app/templates/views/manage-users.html index 555be8279..bf404bfc3 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 %} diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index f62e81423..60c257835 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -5,32 +5,29 @@ 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, + SERVICE_ONE_ID, +) def test_should_show_overview_page( - logged_in_client, + client_request, active_user_with_permissions, mocker, mock_get_invites_for_service, ): - 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'])) + 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' + app.user_api_client.get_users_for_service.assert_called_once_with(service_id=SERVICE_ONE_ID) def test_should_show_page_for_one_user( - logged_in_client, - active_user_with_permissions, - mocker, + client_request, ): - 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)) - - assert response.status_code == 200 + page = client_request.get('main.edit_user_permissions', service_id=SERVICE_ONE_ID, user_id=0) def test_edit_user_permissions( @@ -177,21 +174,18 @@ 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' From aac654cd3822343d2f608e6b40aaf444509b2b76 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 17 Aug 2017 10:49:21 +0100 Subject: [PATCH 3/5] Test that the right permissions in page This commit makes sure that the right permission choices are shown in these pages: - manage team page - invite a user page - edit permissions page This is in order to make changing these pages easier (see subsequent commits). --- app/templates/views/invite-user.html | 4 +- tests/app/main/views/test_manage_users.py | 68 +++++++++++++++++++++-- 2 files changed, 64 insertions(+), 8 deletions(-) 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/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 60c257835..a95f2b753 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -8,26 +8,80 @@ from tests.conftest import service_one as create_sample_service from tests.conftest import ( normalize_spaces, SERVICE_ONE_ID, + active_user_with_permissions, + active_user_view_permissions, ) +@pytest.mark.parametrize('user, expected_text', [ + ( + active_user_with_permissions, + ( + 'Test User (you) ' + 'Can Send messages Can Manage service Can Access API keys' + ), + ), + ( + active_user_view_permissions, + ( + 'Test User With Permissions (you) ' + 'Can’t Send messages Can’t Manage service Can’t Access API keys' + ), + ), +]) def test_should_show_overview_page( client_request, - active_user_with_permissions, mocker, mock_get_invites_for_service, + fake_uuid, + user, + expected_text, ): - mocker.patch('app.user_api_client.get_users_for_service', return_value=[active_user_with_permissions]) + 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 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_service', True), + ('manage_api_keys', True), + ] + ), + ( + 'main.invite_user', + {}, + [ + ('send_messages', False), + ('manage_service', False), + ('manage_api_keys', False), + ] + ), +]) def test_should_show_page_for_one_user( client_request, + endpoint, + extra_args, + expected_checkboxes, ): - page = client_request.get('main.edit_user_permissions', service_id=SERVICE_ONE_ID, user_id=0) + page = client_request.get(endpoint, service_id=SERVICE_ONE_ID, **extra_args) + checkboxes = page.select('input[type=checkbox]') + + assert len(checkboxes) == 3 + + 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( @@ -187,9 +241,11 @@ def test_manage_users_shows_invited_user( page = client_request.get('main.manage_users', service_id=SERVICE_ONE_ID) 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 Manage service Can Access API keys ' + 'Cancel invitation' + ) def test_manage_users_does_not_show_accepted_invite( From d591b9aeb9c55cf8b801d323456ae85116b23c35 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 17 Aug 2017 11:14:26 +0100 Subject: [PATCH 4/5] =?UTF-8?q?Add=20a=20fourth,=20=E2=80=98manage=20templ?= =?UTF-8?q?ates=E2=80=99=20permission?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We’ve seen from research (a long time ago) that the ‘manage service’ permission is too broad, and gives too much control to someone who only needs the ability to edit templates. In other words, editing content should be its own, separate permission, rather than being rolled up into manage service. Since this is already disaggregated on the API side, making this change just means changing the mapping on the admin side and adding an extra checkbox on the invite/edit page. Which is what this commit does. So for now, an existing user who has the manage service permission gets both manage service and manage templates (ie no change to what they can do). Newly invited users will get to choose if they have both, either, or neither. --- app/main/forms.py | 3 ++- app/main/views/manage_users.py | 3 ++- app/templates/views/manage-users.html | 12 ++++++++-- .../views/manage-users/permissions.html | 1 + tests/app/main/views/test_manage_users.py | 20 ++++++++++++---- tests/conftest.py | 23 +++++++++++++++++++ 6 files changed, 54 insertions(+), 8 deletions(-) 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 1931fad5a..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'] } diff --git a/app/templates/views/manage-users.html b/app/templates/views/manage-users.html index bf404bfc3..1b4b1fc19 100644 --- a/app/templates/views/manage-users.html +++ b/app/templates/views/manage-users.html @@ -52,7 +52,11 @@ '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( @@ -89,7 +93,11 @@ '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 a95f2b753..ce97036f0 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -10,6 +10,7 @@ from tests.conftest import ( SERVICE_ONE_ID, active_user_with_permissions, active_user_view_permissions, + active_user_manage_template_permission, ) @@ -18,14 +19,21 @@ from tests.conftest import ( active_user_with_permissions, ( 'Test User (you) ' - 'Can Send messages Can Manage service Can Access API keys' + '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 service Can’t Access API keys' + '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' ), ), ]) @@ -53,6 +61,7 @@ def test_should_show_overview_page( {'user_id': 0}, [ ('send_messages', True), + ('manage_templates', True), ('manage_service', True), ('manage_api_keys', True), ] @@ -62,6 +71,7 @@ def test_should_show_overview_page( {}, [ ('send_messages', False), + ('manage_templates', False), ('manage_service', False), ('manage_api_keys', False), ] @@ -76,7 +86,7 @@ def test_should_show_page_for_one_user( page = client_request.get(endpoint, service_id=SERVICE_ONE_ID, **extra_args) checkboxes = page.select('input[type=checkbox]') - assert len(checkboxes) == 3 + assert len(checkboxes) == 4 for index, expected in enumerate(expected_checkboxes): expected_input_name, expected_checked = expected @@ -96,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'}) @@ -192,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 @@ -243,7 +255,7 @@ def test_manage_users_shows_invited_user( assert page.h1.string.strip() == 'Team members' assert normalize_spaces(page.select('.user-list')[1].text) == ( 'invited_user@test.gov.uk ' - 'Can’t Send messages Can’t Manage service Can Access API keys ' + 'Can’t Send messages Can’t Edit templates Can’t Manage service Can Access API keys ' 'Cancel invitation' ) diff --git a/tests/conftest.py b/tests/conftest.py index 220c78180..67b7dffef 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -682,6 +682,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 From 0092c8bb33ee80dc2ede6f623869ece7986d46ee Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 17 Aug 2017 14:30:53 +0100 Subject: [PATCH 5/5] Change manage team layout to fit extra option MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We’ve moved from three to four permissions. Four permissions don’t fit in the exiting horizontal layout. This commit makes the permissions stack vertically instead. This approach has some downsides: - makes the permissions less easy to scan vertically - makes them take up a lot more space (and at lives services, most of them have somewhere around 15 team members) But I think for now it’s better than any horizontal alternative that I tried. --- app/assets/stylesheets/components/tick-cross.scss | 12 ++++++++---- app/assets/stylesheets/views/users.scss | 2 +- app/templates/views/manage-users.html | 4 ++-- 3 files changed, 11 insertions(+), 7 deletions(-) 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/templates/views/manage-users.html b/app/templates/views/manage-users.html index 1b4b1fc19..fb79b3b8a 100644 --- a/app/templates/views/manage-users.html +++ b/app/templates/views/manage-users.html @@ -37,7 +37,7 @@ {% for user in users %}

- {{ user.name }}  + {{ user.name }} {%- if user.email_address == current_user.email_address -%} (you) {% else %} @@ -84,7 +84,7 @@ {% for user in invited_users %}

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