From dcfff87cc056a6560e5fe2e0264c8a36a80b70fb Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 22 Jul 2021 14:38:45 +0100 Subject: [PATCH] Continue to remove "roles" terminology This renames the two functions we have to translate between UI and DB permissions, as well as some of their associated variables to make it clearer which kind of permission they contain. --- app/models/user.py | 6 +++--- app/notify_client/invite_api_client.py | 4 ++-- app/notify_client/user_api_client.py | 8 +++---- app/utils/user_permissions.py | 24 +++++++++++---------- tests/app/utils/test_user_permissions.py | 27 +++++++++++++----------- 5 files changed, 36 insertions(+), 33 deletions(-) diff --git a/app/models/user.py b/app/models/user.py index cd62db52a..95278abb6 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -18,7 +18,7 @@ from app.notify_client.user_api_client import user_api_client from app.utils.user import is_gov_user from app.utils.user_permissions import ( all_ui_permissions, - translate_permissions_from_db_to_admin_roles, + translate_permissions_from_db_to_ui, ) @@ -103,7 +103,7 @@ class User(JSONModel, UserMixin): them out later, we'll need to rework this function. """ self._permissions = { - service: translate_permissions_from_db_to_admin_roles(permissions) + service: translate_permissions_from_db_to_ui(permissions) for service, permissions in permissions_by_service.items() } @@ -505,7 +505,7 @@ class InvitedUser(JSONModel): self._permissions = permissions else: self._permissions = permissions.split(',') - self._permissions = translate_permissions_from_db_to_admin_roles(self.permissions) + self._permissions = translate_permissions_from_db_to_ui(self.permissions) @property def from_user(self): diff --git a/app/notify_client/invite_api_client.py b/app/notify_client/invite_api_client.py index b76d7d279..464c3ba9c 100644 --- a/app/notify_client/invite_api_client.py +++ b/app/notify_client/invite_api_client.py @@ -1,7 +1,7 @@ from app.notify_client import NotifyAdminAPIClient, _attach_current_user, cache from app.utils.user_permissions import ( all_ui_permissions, - translate_permissions_from_admin_roles_to_db, + translate_permissions_from_ui_to_db, ) @@ -23,7 +23,7 @@ class InviteApiClient(NotifyAdminAPIClient): 'service': service_id, 'email_address': email_address, 'from_user': invite_from_id, - 'permissions': ','.join(sorted(translate_permissions_from_admin_roles_to_db(permissions))), + 'permissions': ','.join(sorted(translate_permissions_from_ui_to_db(permissions))), 'auth_type': auth_type, 'invite_link_host': self.admin_url, 'folder_permissions': folder_permissions, diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index f89de2df9..379d4b9b7 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -1,9 +1,7 @@ from notifications_python_client.errors import HTTPError from app.notify_client import NotifyAdminAPIClient, cache -from app.utils.user_permissions import ( - translate_permissions_from_admin_roles_to_db, -) +from app.utils.user_permissions import translate_permissions_from_ui_to_db ALLOWED_ATTRIBUTES = { 'name', @@ -152,7 +150,7 @@ class UserApiClient(NotifyAdminAPIClient): # permissions passed in are the combined admin roles, not db permissions endpoint = '/service/{}/users/{}'.format(service_id, user_id) data = { - 'permissions': [{'permission': x} for x in translate_permissions_from_admin_roles_to_db(permissions)], + 'permissions': [{'permission': x} for x in translate_permissions_from_ui_to_db(permissions)], 'folder_permissions': folder_permissions, } @@ -168,7 +166,7 @@ class UserApiClient(NotifyAdminAPIClient): def set_user_permissions(self, user_id, service_id, permissions, folder_permissions=None): # permissions passed in are the combined admin roles, not db permissions data = { - 'permissions': [{'permission': x} for x in translate_permissions_from_admin_roles_to_db(permissions)], + 'permissions': [{'permission': x} for x in translate_permissions_from_ui_to_db(permissions)], } if folder_permissions is not None: diff --git a/app/utils/user_permissions.py b/app/utils/user_permissions.py index b9944d532..cddd87ca4 100644 --- a/app/utils/user_permissions.py +++ b/app/utils/user_permissions.py @@ -28,25 +28,27 @@ broadcast_permission_options = ( ) -def translate_permissions_from_db_to_admin_roles(permissions): +def translate_permissions_from_db_to_ui(db_permissions): """ - Given a list of database permissions, return a set of roles + Given a list of database permissions, return a set of UI permissions - A role is returned if all of its database permissions are in the permission list that is passed in. - Any permissions in the list that are not database permissions are also returned. + A UI permission is returned if all of its DB permissions are in the permission list that is passed in. + Any DB permissions in the list that are not known permissions are also returned. """ - unknown_database_permissions = {p for p in permissions if p not in all_db_permissions} + unknown_database_permissions = set(db_permissions) - all_db_permissions return { - admin_role for admin_role, db_role_list in permission_mappings.items() - if set(db_role_list) <= set(permissions) + ui_permission for ui_permission, db_permissions_for_ui_permission in permission_mappings.items() + if set(db_permissions_for_ui_permission) <= set(db_permissions) } | unknown_database_permissions -def translate_permissions_from_admin_roles_to_db(permissions): +def translate_permissions_from_ui_to_db(ui_permissions): """ - Given a list of admin roles (ie: checkboxes on a permissions edit page for example), return a set of db permissions + Given a list of UI permissions (ie: checkboxes on a permissions edit page), return a set of DB permissions - Looks them up in the roles dict, falling back to just passing through if they're not recognised. + Looks them up in the mapping, falling back to just passing through if they're not recognised. """ - return set(chain.from_iterable(permission_mappings.get(permission, [permission]) for permission in permissions)) + return set(chain.from_iterable( + permission_mappings.get(ui_permission, [ui_permission]) for ui_permission in ui_permissions + )) diff --git a/tests/app/utils/test_user_permissions.py b/tests/app/utils/test_user_permissions.py index 9ad639022..b40994597 100644 --- a/tests/app/utils/test_user_permissions.py +++ b/tests/app/utils/test_user_permissions.py @@ -1,12 +1,12 @@ import pytest from app.utils.user_permissions import ( - translate_permissions_from_admin_roles_to_db, - translate_permissions_from_db_to_admin_roles, + translate_permissions_from_db_to_ui, + translate_permissions_from_ui_to_db, ) -@pytest.mark.parametrize('db_roles,admin_roles', [ +@pytest.mark.parametrize('db_permissions,expected_ui_permissions', [ ( ['approve_broadcasts', 'reject_broadcasts', 'cancel_broadcasts'], {'approve_broadcasts'}, @@ -28,15 +28,18 @@ from app.utils.user_permissions import ( {'send_messages', 'manage_templates', 'some_unknown_permission'}, ), ]) -def test_translate_permissions_from_db_to_admin_roles( - db_roles, - admin_roles, +def test_translate_permissions_from_db_to_ui( + db_permissions, + expected_ui_permissions, ): - roles = translate_permissions_from_db_to_admin_roles(db_roles) - assert roles == admin_roles + ui_permissions = translate_permissions_from_db_to_ui(db_permissions) + assert ui_permissions == expected_ui_permissions -def test_translate_permissions_from_admin_roles_to_db(): - roles = ['send_messages', 'manage_templates', 'some_unknown_permission'] - db_perms = translate_permissions_from_admin_roles_to_db(roles) - assert db_perms == {'send_texts', 'send_emails', 'send_letters', 'manage_templates', 'some_unknown_permission'} +def test_translate_permissions_from_ui_to_db(): + ui_permissions = ['send_messages', 'manage_templates', 'some_unknown_permission'] + db_permissions = translate_permissions_from_ui_to_db(ui_permissions) + + assert db_permissions == { + 'send_texts', 'send_emails', 'send_letters', 'manage_templates', 'some_unknown_permission' + }