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

feat: ability to exclude global roles from GET-roles API #146

Merged
merged 1 commit into from
Nov 24, 2024
Merged

feat: ability to exclude global roles from GET-roles API #146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 13 additions & 15 deletions futurex_openedx_extensions/dashboard/serializers.py
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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