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.

Sign up for GitHub

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

Post PR updates #79

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
942047c
Remove redundant constants
Charl1996 Nov 15, 2024
6104485
Add trailing slash
Charl1996 Nov 15, 2024
de02838
Update comment
Charl1996 Nov 15, 2024
082902b
Little refactor
Charl1996 Nov 15, 2024
6871553
Update tests
Charl1996 Nov 15, 2024
ca7285d
Fix domain_access response and add tests
Charl1996 Nov 18, 2024
5236671
Don't append mutable obj
Charl1996 Nov 18, 2024
13d513c
Update method docstring
Charl1996 Nov 18, 2024
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
3 changes: 0 additions & 3 deletions hq_superset/const.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,6 @@
CAN_READ_PERMISSION,
]

CAN_WRITE_PERMISSION = "can_write"


READ_ONLY_MENU_PERMISSIONS = {
"Chart": READ_ONLY_PERMISSIONS,
"Dataset": READ_ONLY_PERMISSIONS,
Expand Down
24 changes: 6 additions & 18 deletions hq_superset/utils.py
View file
Open in desktop
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 @@ -199,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.

5236671


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

@staticmethod
def _get_domain_access(domain):
Expand All @@ -228,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 •
edited
Loading

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.

ca7285d

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