Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Post PR updates #79

Merged
merged 8 commits into from
Nov 20, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions hq_superset/const.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,6 @@
CAN_READ_PERMISSION,
]

CAN_WRITE_PERMISSION = "can_write"
CAN_EDIT_PERMISSION = "can_edit"
CAN_ADD_PERMISSION = "can_add"
CAN_DELETE_PERMISSIONS = "can_delete"


READ_ONLY_MENU_PERMISSIONS = {
"Chart": READ_ONLY_PERMISSIONS,
"Dataset": READ_ONLY_PERMISSIONS,
Expand Down
2 changes: 1 addition & 1 deletion hq_superset/hq_url.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,4 @@ def datasource_unsubscribe(domain, datasource_id):


def user_domain_roles(domain):
return f"a/{domain}/api/analytics-roles/v1"
return f"a/{domain}/api/analytics-roles/v1/"
5 changes: 1 addition & 4 deletions hq_superset/tests/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,4 @@ def _ensure_platform_roles_exist(self, sm):

@staticmethod
def _to_permissions_response(can_write, can_read, roles):
return {
"can_write": can_write,
"can_read": can_read,
}, roles
return can_read, can_write, roles
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method seems redundant, but it's providing a more clear structure to the expected results in the test.

For instance, instead of doing this in the test

  get_domain_access_mock.return_value = (False, False ,["sql_lab", "dataset_editor"])

I can do this

get_domain_access_mock.return_value = self._to_permissions_response(
    can_write=False,
    can_read=False,
    roles=["sql_lab", "dataset_editor"],
)

Using a method like this makes it more clear what each value means.

4 changes: 2 additions & 2 deletions hq_superset/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ def test_redirects_to_domain_select_after_login(self):
)
self.logout(client)

@patch.object(DomainSyncUtil, "_get_domain_access", return_value=({"can_write": True, "can_read": True}, []))
@patch.object(DomainSyncUtil, "_get_domain_access", return_value=(True, True, []))
def test_domain_select_works(self, *args):
client = self.app.test_client()
self.login(client)
Expand Down Expand Up @@ -135,7 +135,7 @@ def _do_assert(datasources):
_do_assert(self.oauth_mock.test2_datasources)
self.logout(client)

@patch.object(DomainSyncUtil, "_get_domain_access", return_value=({"can_write": True, "can_read": True}, []))
@patch.object(DomainSyncUtil, "_get_domain_access", return_value=(True, True, []))
def test_datasource_upload(self, *args):
client = self.app.test_client()
self.login(client)
Expand Down
4 changes: 2 additions & 2 deletions hq_superset/tests/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,8 @@ def get(self, url, token):
'a/test1/configurable_reports/data_sources/export/test1_ucr1/?format=csv': MockResponse(
TEST_UCR_CSV_V1, 200
),
'a/test1/api/analytics-roles/v1': MockResponse(self.user_domain_roles, 200),
'a/test2/api/analytics-roles/v1': MockResponse(self.user_domain_roles, 200),
'a/test1/api/analytics-roles/v1/': MockResponse(self.user_domain_roles, 200),
'a/test2/api/analytics-roles/v1/': MockResponse(self.user_domain_roles, 200),
}[url]


Expand Down
29 changes: 7 additions & 22 deletions hq_superset/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@
from superset.utils.database import get_or_create_db

from hq_superset.const import (
CAN_READ_PERMISSION,
CAN_WRITE_PERMISSION,
DOMAIN_PREFIX,
GAMMA_ROLE_NAME,
HQ_DATABASE_NAME,
Expand Down Expand Up @@ -135,10 +133,7 @@ def sync_domain_role(self, domain):
The user gets assigned at least 3 roles in order to function on any domain:
1. hq_user_role: gives access to superset platform
2. domain_schema_role: restricts user access to specific domain schema
3. domain_user_role: restricts access for particular user on domain in accordance with how the permissions
are defined on CommCare HQ.

Any additional roles defined on CommCare HQ will also be assigned to the user.
3. Either the Gamma role for "edit" users or the READ_ONLY_ROLE_NAME for "view only" user
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: what is an "edit" user? or "view only" user? in terms of CCA

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"""
hq_user_role = self._ensure_hq_user_role()
domain_schema_role = self._create_domain_role(domain)
Expand Down Expand Up @@ -202,19 +197,19 @@ def _ensure_domain_role_created(self, domain):
return self.sm.add_role(get_role_name_for_domain(domain))

def _get_additional_user_roles(self, domain):
domain_permissions, roles_names = self._get_domain_access(domain)
if self._user_has_no_access(domain_permissions):
can_read, can_write, platform_roles_names = self._get_domain_access(domain)
if not (can_read or can_write):
return []

if domain_permissions[CAN_WRITE_PERMISSION]:
if can_write:
user_role = GAMMA_ROLE_NAME
else:
self._ensure_read_only_role_exists()
user_role = READ_ONLY_ROLE_NAME

roles_names.append(user_role)
platform_roles_names.append(user_role)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest to avoid editing an mutable object. This can cause confusion since the original object is being edited here.
Rather make a new list using platform_roles_names and then append to it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

➕ 1️⃣

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


return self._get_platform_roles(roles_names)
return self._get_platform_roles(platform_roles_names)

@staticmethod
def _get_domain_access(domain):
Expand All @@ -231,17 +226,7 @@ def _get_domain_access(domain):
hq_permissions = response_data['permissions']
roles = response_data['roles'] or []

# Map between HQ and CCA
permissions = {
CAN_WRITE_PERMISSION: hq_permissions["can_edit"],
CAN_READ_PERMISSION: hq_permissions["can_view"],
}
return permissions, roles

@staticmethod
def _user_has_no_access(permissions: dict):
user_has_access = any([permissions[p] for p in permissions])
return not user_has_access
return hq_permissions["can_view"], hq_permissions["can_edit"], roles
Copy link
Contributor

@kaapstorm kaapstorm Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previous return on line 223 returns a dict and a list, but this returns two bools and a list. It looks like you need to update line 223.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test that returns a status code other than 200 would be cool, because it would have flagged the problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, great catch @kaapstorm

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaapstorm

Will update!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thank you.


def _get_platform_roles(self, roles_names):
platform_roles = []
Expand Down
Loading