Post PR updates #79
Post PR updates #79
Conversation
| "can_write": can_write, | ||
| "can_read": can_read, | ||
| }, roles | ||
| return can_read, can_write, roles |
There was a problem hiding this comment.
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.
hq_superset/utils.py
Outdated
| 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 |
There was a problem hiding this comment.
nit: what is an "edit" user? or "view only" user? in terms of CCA
hq_superset/utils.py
Outdated
| 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) |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
A test that returns a status code other than 200 would be cool, because it would have flagged the problem.
There was a problem hiding this comment.
oh, great catch @kaapstorm
There was a problem hiding this comment.
Will update!
This PR addresses the promised changes here and does a small refactor to simplify the code.