Skip to content

Commit

Permalink
feat: ability to exclude global roles from GET-roles API
Browse files Browse the repository at this point in the history
  • Loading branch information
@shadinaif
shadinaif committed Nov 24, 2024
1 parent d6b2503 commit 0841841
Show file tree
Hide file tree
Showing 5 changed files with 88 additions and 78 deletions.
28 changes: 13 additions & 15 deletions futurex_openedx_extensions/dashboard/serializers.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -682,20 +682,18 @@ def parse_query_params(query_params: dict[str, Any]) -> dict[str, Any]:
else:
result['active_filter'] = None

exclude_tenant_roles = False
exclude_course_roles = False
if query_params.get('exclude_tenant_roles') is not None:
exclude_tenant_roles = query_params['exclude_tenant_roles'] == '1'

if not exclude_tenant_roles and query_params.get('exclude_course_roles') is not None:
exclude_course_roles = query_params['exclude_course_roles'] == '1'

if exclude_tenant_roles:
result['exclude_role_type'] = RoleType.ORG_WIDE
elif exclude_course_roles:
result['exclude_role_type'] = RoleType.COURSE_SPECIFIC
else:
result['exclude_role_type'] = None
excluded_role_types = query_params.get('excluded_role_types', '').split(',') \
if query_params.get('excluded_role_types') else []

result['excluded_role_types'] = []
if 'global' in excluded_role_types:
result['excluded_role_types'].append(RoleType.GLOBAL)

if 'tenant' in excluded_role_types:
result['excluded_role_types'].append(RoleType.ORG_WIDE)

if 'course' in excluded_role_types:
result['excluded_role_types'].append(RoleType.COURSE_SPECIFIC)

return result

Expand Down Expand Up @@ -748,7 +746,7 @@ def construct_roles_data(self, users: list[get_user_model]) -> None:
roles_filter=self.query_params['roles_filter'],
active_filter=self.query_params['active_filter'],
course_ids_filter=self.query_params['course_ids_filter'],
exclude_role_type=self.query_params['exclude_role_type'],
excluded_role_types=self.query_params['excluded_role_types'],
)

for record in records or []:
Expand Down
2 changes: 1 addition & 1 deletion futurex_openedx_extensions/dashboard/views.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -523,7 +523,7 @@ def get_queryset(self) -> QuerySet:
roles_filter=dummy_serializers.query_params['roles_filter'],
active_filter=dummy_serializers.query_params['active_filter'],
course_ids_filter=dummy_serializers.query_params['course_ids_filter'],
exclude_role_type=dummy_serializers.query_params['exclude_role_type'],
excluded_role_types=dummy_serializers.query_params['excluded_role_types'],
).values('user_id').distinct().order_by()
).select_related('profile').order_by('id')
except ValueError as exc:
Expand Down
35 changes: 21 additions & 14 deletions futurex_openedx_extensions/helpers/roles.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -555,6 +555,7 @@ class RoleType(Enum):
"""Role types."""
ORG_WIDE = 'org_wide'
COURSE_SPECIFIC = 'course_specific'
GLOBAL = 'global'


def get_course_access_roles_queryset( # pylint: disable=too-many-arguments, too-many-branches, too-many-locals
Expand All @@ -565,7 +566,7 @@ def get_course_access_roles_queryset( # pylint: disable=too-many-arguments, too
roles_filter: list[str] | None = None,
active_filter: bool | None = None,
course_ids_filter: list[str] | None = None,
exclude_role_type: RoleType | None = None,
excluded_role_types: list[RoleType | str] | None = None,
) -> QuerySet:
"""
Get the course access roles queryset.
Expand All @@ -585,18 +586,21 @@ def get_course_access_roles_queryset( # pylint: disable=too-many-arguments, too
:type active_filter: bool
:param course_ids_filter: The course IDs to filter on. None for no filter
:type course_ids_filter: list
:param exclude_role_type: The role type to exclude. None for no filter
:type exclude_role_type: RoleType
:param excluded_role_types: The role types to exclude. None of empty list for no exclusion
:type excluded_role_types: list of RoleType
:return: The roles for the users queryset
:rtype: QuerySet
"""
if exclude_role_type is not None and not isinstance(exclude_role_type, RoleType):
try:
exclude_role_type = RoleType(exclude_role_type)
except ValueError as exc:
if exclude_role_type == '':
exclude_role_type = 'EmptyString'
raise TypeError(f'Invalid exclude_role_type: {exclude_role_type}') from exc
excluded_role_types = excluded_role_types or []
if excluded_role_types:
for item_index, role_type in enumerate(excluded_role_types, start=0):
if not isinstance(role_type, RoleType):
try:
excluded_role_types[item_index] = RoleType(role_type)
except ValueError as exc:
if role_type == '':
role_type = 'EmptyString'
raise TypeError(f'Invalid excluded_role_types: {role_type}') from exc

if course_ids_filter:
verify_course_ids(course_ids_filter)
Expand Down Expand Up @@ -665,11 +669,14 @@ def get_course_access_roles_queryset( # pylint: disable=too-many-arguments, too
)
)

if exclude_role_type == RoleType.ORG_WIDE:
queryset = queryset.exclude(course_id=CourseKeyField.Empty)
if RoleType.ORG_WIDE in excluded_role_types:
queryset = queryset.exclude(~Q(org='') & Q(course_id=CourseKeyField.Empty))

if RoleType.COURSE_SPECIFIC in excluded_role_types:
queryset = queryset.exclude(~Q(org='') & ~Q(course_id=CourseKeyField.Empty))

elif exclude_role_type == RoleType.COURSE_SPECIFIC:
queryset = queryset.filter(course_id=CourseKeyField.Empty)
if RoleType.GLOBAL in excluded_role_types:
queryset = queryset.exclude(org='', course_id=CourseKeyField.Empty)

return queryset

Expand Down
55 changes: 29 additions & 26 deletions tests/test_dashboard/test_serializers.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -844,34 +844,37 @@ def test_user_roles_serializer_parse_query_params_defaults(
'course_ids_filter': [],
'roles_filter': [],
'active_filter': None,
'exclude_role_type': None,
'excluded_role_types': [],
}


@pytest.mark.parametrize('exclude_tenant_roles, exclude_course_roles, exclude_role_type', [
(None, None, None),
('0', None, None),
('1', None, RoleType.ORG_WIDE),
(None, '0', None),
('0', '0', None),
('1', '0', RoleType.ORG_WIDE),
(None, '1', RoleType.COURSE_SPECIFIC),
('0', '1', RoleType.COURSE_SPECIFIC),
('1', '1', RoleType.ORG_WIDE),
@pytest.mark.parametrize('excluded_role_types, result_excluded_role_types', [
(None, []),
('', []),
('global', [RoleType.GLOBAL]),
('invalid,entry', []),
('tenant', [RoleType.ORG_WIDE]),
('course', [RoleType.COURSE_SPECIFIC]),
('global,course', [RoleType.COURSE_SPECIFIC, RoleType.GLOBAL]),
('tenant,course', [RoleType.ORG_WIDE, RoleType.COURSE_SPECIFIC]),
('global,tenant,course', [RoleType.ORG_WIDE, RoleType.COURSE_SPECIFIC, RoleType.GLOBAL]),
])
def test_user_roles_serializer_parse_query_params_values(exclude_tenant_roles, exclude_course_roles, exclude_role_type):
def test_user_roles_serializer_parse_query_params_values(excluded_role_types, result_excluded_role_types):
"""Verify that the parse_query_params method correctly parses the query parameters."""
assert UserRolesSerializer.parse_query_params({
'search_text': 'user99',
'only_course_ids': 'course-v1:ORG99+88+88,another-course',
'only_roles': 'staff,instructor',
'active_users_filter': '1',
'exclude_tenant_roles': exclude_tenant_roles,
'exclude_course_roles': exclude_course_roles,
}) == {
'search_text': 'user99',
'course_ids_filter': ['course-v1:ORG99+88+88', 'another-course'],
'roles_filter': ['staff', 'instructor'],
'active_filter': True,
'exclude_role_type': exclude_role_type,
}
assert not DeepDiff(
UserRolesSerializer.parse_query_params({
'search_text': 'user99',
'only_course_ids': 'course-v1:ORG99+88+88,another-course',
'only_roles': 'staff,instructor',
'active_users_filter': '1',
'excluded_role_types': excluded_role_types,
}),
{
'search_text': 'user99',
'course_ids_filter': ['course-v1:ORG99+88+88', 'another-course'],
'roles_filter': ['staff', 'instructor'],
'active_filter': True,
'excluded_role_types': result_excluded_role_types,
},
ignore_order=True,
)
46 changes: 24 additions & 22 deletions tests/test_helpers/test_roles.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -803,22 +803,23 @@ def test_role_types():
"""Verify that the role types are as expected."""
assert RoleType.ORG_WIDE == RoleType('org_wide')
assert RoleType.COURSE_SPECIFIC == RoleType('course_specific')
assert len(RoleType) == 2, 'Unexpected number of role types, if this class is updated, then the logic of ' \
assert RoleType.GLOBAL == RoleType('global')
assert len(RoleType) == 3, 'Unexpected number of role types, if this class is updated, then the logic of ' \
'get_course_access_roles_queryset should be updated as well'


@pytest.mark.parametrize('bad_role_type, expected_error_message', [
('bad_name', 'Invalid exclude_role_type: bad_name'),
('', 'Invalid exclude_role_type: EmptyString'),
(['not string and not RoleType'], 'Invalid exclude_role_type: [\'not string and not RoleType\']'),
('bad_name', 'Invalid excluded_role_types: bad_name'),
('', 'Invalid excluded_role_types: EmptyString'),
(['not string and not RoleType'], 'Invalid excluded_role_types: [\'not string and not RoleType\']'),
])
def test_get_roles_for_users_queryset_bad_exclude(bad_role_type, expected_error_message):
"""Verify that get_roles_for_users_queryset raises an error if the exclude parameter is invalid."""
with pytest.raises(TypeError) as exc_info:
get_course_access_roles_queryset(
orgs_filter=['org1', 'org2'],
remove_redundant=False,
exclude_role_type=bad_role_type,
excluded_role_types=[bad_role_type],
)
assert str(exc_info.value) == expected_error_message

Expand Down Expand Up @@ -925,7 +926,7 @@ def test_get_roles_for_users_queryset_roles_filter(base_data): # pylint: disabl


@pytest.mark.django_db
@pytest.mark.parametrize('course_ids, remove_redundant, exclude_role_type, expected_result', [
@pytest.mark.parametrize('course_ids, remove_redundant, excluded_role_types, expected_result', [
([], False, None, get_test_data_dict()),
([], True, None, {
'user3': {
Expand Down Expand Up @@ -970,7 +971,7 @@ def test_get_roles_for_users_queryset_roles_filter(base_data): # pylint: disabl
},
'user48': {'org4': {'None': ['instructor']}},
}),
([], False, RoleType.ORG_WIDE, {
([], False, [RoleType.ORG_WIDE], {
'user3': {
'org1': {
'course-v1:ORG1+3+3': ['staff', 'instructor'],
Expand Down Expand Up @@ -1000,7 +1001,7 @@ def test_get_roles_for_users_queryset_roles_filter(base_data): # pylint: disabl
}
},
}),
([], True, RoleType.ORG_WIDE, {
([], True, [RoleType.ORG_WIDE], {
'user3': {
'org1': {
'course-v1:ORG1+3+3': ['instructor'],
Expand Down Expand Up @@ -1028,8 +1029,8 @@ def test_get_roles_for_users_queryset_roles_filter(base_data): # pylint: disabl
}
},
}),
([], False, RoleType.COURSE_SPECIFIC, get_test_data_dict_without_course_roles()),
([], True, RoleType.COURSE_SPECIFIC, get_test_data_dict_without_course_roles()),
([], False, [RoleType.COURSE_SPECIFIC], get_test_data_dict_without_course_roles()),
([], True, [RoleType.COURSE_SPECIFIC], get_test_data_dict_without_course_roles()),
(['course-v1:ORG3+2+2'], False, None, {
'user9': {
'org3': {
Expand Down Expand Up @@ -1079,7 +1080,7 @@ def test_get_roles_for_users_queryset_roles_filter(base_data): # pylint: disabl
}
},
}),
(['course-v1:ORG3+2+2'], False, RoleType.ORG_WIDE, {
(['course-v1:ORG3+2+2'], False, [RoleType.ORG_WIDE], {
'user9': {
'org3': {
'course-v1:ORG3+2+2': ['data_researcher'],
Expand All @@ -1091,37 +1092,38 @@ def test_get_roles_for_users_queryset_roles_filter(base_data): # pylint: disabl
}
},
}),
(['course-v1:ORG3+2+2'], True, RoleType.ORG_WIDE, {
(['course-v1:ORG3+2+2'], True, [RoleType.ORG_WIDE], {
'user11': {
'org3': {
'course-v1:ORG3+2+2': ['instructor'],
}
},
}),
(['course-v1:ORG3+2+2'], False, RoleType.COURSE_SPECIFIC, get_test_data_dict_without_course_roles_org3()),
(['course-v1:ORG3+2+2'], True, RoleType.COURSE_SPECIFIC, get_test_data_dict_without_course_roles_org3()),
(['course-v1:ORG3+2+2'], False, [RoleType.COURSE_SPECIFIC], get_test_data_dict_without_course_roles_org3()),
(['course-v1:ORG3+2+2'], True, [RoleType.COURSE_SPECIFIC], get_test_data_dict_without_course_roles_org3()),
])
def test_get_roles_for_users_queryset(
base_data, course_ids, remove_redundant, exclude_role_type, expected_result,
base_data, course_ids, remove_redundant, excluded_role_types, expected_result,
): # pylint: disable=unused-argument
"""Verify that get_roles_for_users_queryset returns the expected queryset."""
result = get_course_access_roles_queryset(
orgs_filter=get_all_orgs(),
course_ids_filter=course_ids,
remove_redundant=remove_redundant,
exclude_role_type=exclude_role_type,
excluded_role_types=excluded_role_types,
)
assert roles_records_to_dict(result) == expected_result


@pytest.mark.django_db
@pytest.mark.parametrize('remove_redundant, exclude_role_type, expected_result', [
@pytest.mark.parametrize('remove_redundant, excluded_role_types, expected_result', [
(False, None, {'user62': {'': {'None': ['course_creator_group']}}}),
(False, RoleType.COURSE_SPECIFIC, {'user62': {'': {'None': ['course_creator_group']}}}),
(False, RoleType.ORG_WIDE, {}),
(False, [RoleType.COURSE_SPECIFIC], {'user62': {'': {'None': ['course_creator_group']}}}),
(False, [RoleType.ORG_WIDE], {'user62': {'': {'None': ['course_creator_group']}}}),
(False, [RoleType.GLOBAL], {}),
])
def test_get_roles_for_users_queryset_global(
base_data, remove_redundant, exclude_role_type, expected_result,
base_data, remove_redundant, excluded_role_types, expected_result,
): # pylint: disable=unused-argument
"""Verify that get_roles_for_users_queryset removes global roles when excluding org roles."""
user62 = get_user_model().objects.get(username='user62')
Expand All @@ -1134,10 +1136,10 @@ def test_get_roles_for_users_queryset_global(
result = get_course_access_roles_queryset(
orgs_filter=test_orgs,
remove_redundant=remove_redundant,
exclude_role_type=exclude_role_type,
excluded_role_types=excluded_role_types,
users=[user62],
)
assert roles_records_to_dict(result) == expected_result
assert not DeepDiff(roles_records_to_dict(result), expected_result, ignore_order=True)


@pytest.mark.django_db
Expand Down

0 comments on commit 0841841

Please sign in to comment.