Skip to content

Commit

Permalink
fix: refactor code for enrollment api to make code cleaner
Browse files Browse the repository at this point in the history
  • Loading branch information
@tehreem-sadat
tehreem-sadat committed Dec 25, 2024
1 parent 6854d71 commit c6b9497
Show file tree
Hide file tree
Showing 4 changed files with 186 additions and 141 deletions.
45 changes: 10 additions & 35 deletions futurex_openedx_extensions/dashboard/details/learners.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,11 @@
from lms.djangoapps.certificates.models import GeneratedCertificate
from lms.djangoapps.courseware.models import StudentModule
from lms.djangoapps.grades.models import PersistentCourseGrade
from openedx.core.djangoapps.content.course_overviews.models import CourseOverview

from futurex_openedx_extensions.helpers.exceptions import FXCodedException, FXExceptionCodes
from futurex_openedx_extensions.helpers.extractors import get_partial_access_course_ids, verify_course_ids
from futurex_openedx_extensions.helpers.querysets import (
check_staff_exist_queryset,
get_base_queryset_courses,
get_course_search_queryset,
get_learners_search_queryset,
get_one_user_queryset,
get_permitted_learners_queryset,
Expand Down Expand Up @@ -286,43 +284,20 @@ def get_learners_enrollments_queryset( # pylint: disable=too-many-arguments
:return: List of dictionaries containing user and course details.
"""
accessible_users = get_permitted_learners_queryset(
queryset=get_learners_search_queryset(search_text=learner_search),
queryset=get_learners_search_queryset(
search_text=learner_search,
user_ids=user_ids,
usernames=usernames,
),
fx_permission_info=fx_permission_info,
include_staff=include_staff,
)
user_filter = Q()
if user_ids:
invalid_user_ids = [user_id for user_id in user_ids if not isinstance(user_id, int)]
if invalid_user_ids:
raise FXCodedException(
code=FXExceptionCodes.INVALID_INPUT,
message=f'Invalid user ids: {invalid_user_ids}',
)
user_filter |= Q(id__in=user_ids)
if usernames:
invalid_usernames = [username for username in usernames if not isinstance(username, str)]
if invalid_usernames:
raise FXCodedException(
code=FXExceptionCodes.INVALID_INPUT,
message=f'Invalid usernames: {invalid_usernames}',
)
user_filter |= Q(username__in=usernames)
accessible_users = accessible_users.filter(user_filter)

accessible_courses = CourseOverview.objects.filter(
Q(id__in=get_partial_access_course_ids(fx_permission_info)) |
Q(org__in=fx_permission_info['view_allowed_full_access_orgs'])
accessible_courses = get_course_search_queryset(
fx_permission_info=fx_permission_info,
search_text=course_search,
course_ids=course_ids,
)
if course_ids:
verify_course_ids(course_ids)
accessible_courses = accessible_courses.filter(id__in=course_ids)

course_search = (course_search or '').strip()
if course_search:
accessible_courses = accessible_courses.filter(
Q(display_name__icontains=course_search) |
Q(id__icontains=course_search),
)

queryset = CourseEnrollment.objects.filter(
is_active=True,
Expand Down
61 changes: 58 additions & 3 deletions futurex_openedx_extensions/helpers/querysets.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

from futurex_openedx_extensions.helpers.converters import get_allowed_roles
from futurex_openedx_extensions.helpers.exceptions import FXCodedException, FXExceptionCodes
from futurex_openedx_extensions.helpers.extractors import get_partial_access_course_ids
from futurex_openedx_extensions.helpers.extractors import get_partial_access_course_ids, verify_course_ids
from futurex_openedx_extensions.helpers.tenants import get_tenants_sites
from futurex_openedx_extensions.helpers.users import get_user_by_key
from futurex_openedx_extensions.upgrade.models_switch import CourseAccessRole, CourseEnrollment, UserSignupSource
Expand Down Expand Up @@ -208,11 +208,13 @@ def get_base_queryset_courses(
return q_set


def get_learners_search_queryset(
def get_learners_search_queryset( # pylint: disable=too-many-arguments
search_text: str | None = None,
superuser_filter: bool | None = False,
staff_filter: bool | None = False,
active_filter: bool | None = True
active_filter: bool | None = True,
user_ids: List[int] | None = None,
usernames: List[str] | None = None,
) -> QuerySet:
"""
Get the learners queryset for the given search text.
Expand Down Expand Up @@ -246,6 +248,59 @@ def get_learners_search_queryset(
Q(profile__name__icontains=search_text)
)

user_filter = Q()
if user_ids:
invalid_user_ids = [user_id for user_id in user_ids if not isinstance(user_id, int)]
if invalid_user_ids:
raise FXCodedException(
code=FXExceptionCodes.INVALID_INPUT,
message=f'Invalid user ids: {invalid_user_ids}',
)
user_filter |= Q(id__in=user_ids)
if usernames:
invalid_usernames = [username for username in usernames if not isinstance(username, str)]
if invalid_usernames:
raise FXCodedException(
code=FXExceptionCodes.INVALID_INPUT,
message=f'Invalid usernames: {invalid_usernames}',
)
user_filter |= Q(username__in=usernames)

queryset = queryset.filter(user_filter)

return queryset


def get_course_search_queryset(
fx_permission_info: dict,
search_text: str | None = None,
course_ids: List[str] | None = None,
) -> QuerySet:
"""
Get the courses queryset for the given search text.
:param fx_permission_info: Dictionary containing permission information
:type fx_permission_info: dict
:param search_text: Search text to filter the courses by
:type search_text: str | None
:return: QuerySet of courses
:rtype: QuerySet
"""
queryset = CourseOverview.objects.filter(
Q(id__in=get_partial_access_course_ids(fx_permission_info)) |
Q(org__in=fx_permission_info['view_allowed_full_access_orgs'])
)
if course_ids:
verify_course_ids(course_ids)
queryset = queryset.filter(id__in=course_ids)

course_search = (search_text or '').strip()
if course_search:
queryset = queryset.filter(
Q(display_name__icontains=course_search) |
Q(id__icontains=course_search),
)

return queryset


Expand Down
137 changes: 34 additions & 103 deletions tests/test_dashboard/test_details/test_details_learners.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,35 +224,53 @@ def test_get_learners_enrollments_queryset_annotations(
).count() == 0, 'only active enrollments should be filtered'


@pytest.mark.django_db
@pytest.mark.parametrize(
'course_search, learner_search, expected_count, usecase',
[
('', '', 25, 'no search'),
('Course 5', '', 8, 'only course search'),
('', 'user15', 2, 'only user search'),
('Course 5', 'user15', 1, 'both course and user search'),
],
)
def test_get_learners_enrollments_queryset_for_course_and_learner_search(
course_search, learner_search, expected_count, usecase, fx_permission_info,
):
"""Test get_learners_by_course_queryset result for accessible users and courses."""
fx_permission_info['view_allowed_full_access_orgs'] = ['org1', 'org2']
queryset = get_learners_enrollments_queryset(
fx_permission_info=fx_permission_info,
course_search=course_search,
learner_search=learner_search
)
assert queryset.count() == expected_count, f'unexpected enrollment queryset count for case: {usecase}'


@pytest.mark.django_db
@pytest.mark.parametrize(
'filter_user_ids, filter_course_ids, filter_usernames, expected_error, expected_count, usecase', [
(None, None, None, '', 6, 'No filter'),
(None, None, None, '', 25, 'No filter'),
([21], None, None, '', 3, 'only user_ids filter'),
(None, None, ['user21'], '', 3, 'only usernames filter'),
([21], None, ['user29'], '', 4, 'user_ids and usernames filter'),
([], ['course-v1:ORG1+5+5'], [], '', 2, 'only course_ids filter'),
([], ['course-v1:ORG1+5+5'], [], '', 3, 'only course_ids filter'),
([15, 29], ['course-v1:ORG1+5+5'], [], '', 1, 'both course_ids and user_ids filter'),
([15], ['course-v1:ORG1+5+5'], ['user21'], '', 2, 'course_ids, usernames and user_ids filter'),
(
[15], ['course-v1:ORG1+5+5'], ['user21', 'user15'], '', 2,
'course_ids, usernames and user_ids filter, with repeated user in user ids and usernames'
),
([15, 29, 'not-int', 0.2], [], [], 'Invalid user ids: [\'not-int\', 0.2]', 0, 'invalid user ids.'),
([15, 29], ['course-v1:ORG1+5+5', 'invalid'], [], 'Invalid course ID format: invalid', 0, 'invalid course_ids'),
([], [], [11], 'Invalid usernames: [11]', 0, 'invalid usernames'),
]
)
@patch('futurex_openedx_extensions.dashboard.details.learners.get_permitted_learners_queryset')
@patch('futurex_openedx_extensions.dashboard.details.learners.get_partial_access_course_ids')
def test_get_learners_enrollments_queryset_for_course_ids_and_user_ids_filter(
mocked_partial_course_ids, mocked_permitted_learners,
filter_user_ids, filter_course_ids, filter_usernames,
expected_error, expected_count, usecase, fx_permission_info,
): # pylint: disable=too-many-arguments, unused-argument
"""Test get_learners_by_course_queryset result for course_ids and user_ids filtration."""
fake_accessible_user_ids = [15, 21, 29]
fake_accessible_course_ids = [
'course-v1:ORG1+5+5', 'course-v1:ORG2+4+4', 'course-v1:ORG2+5+5', 'course-v1:ORG2+6+6', 'course-v1:ORG2+7+7'
]
mocked_permitted_learners.return_value = get_user_model().objects.filter(id__in=fake_accessible_user_ids)

def test_get_learners_enrollments_queryset_for_course_and_user_filters(
filter_user_ids, filter_course_ids, filter_usernames, expected_error, expected_count, usecase, fx_permission_info,
): # pylint: disable=too-many-arguments
"""Test get_learners_by_course_queryset result for user and course filters."""
fx_permission_info['view_allowed_full_access_orgs'] = ['org1', 'org2']
if expected_error:
with pytest.raises(FXCodedException) as exc_info:
queryset = get_learners_enrollments_queryset(
Expand All @@ -269,91 +287,4 @@ def test_get_learners_enrollments_queryset_for_course_ids_and_user_ids_filter(
user_ids=filter_user_ids,
usernames=filter_usernames,
)
expected_user_ids = filter_user_ids or fake_accessible_user_ids
if filter_usernames:
expected_user_ids.extend(
get_user_model().objects.filter(
username__in=filter_usernames
).values_list('id', flat=True)
)
expected_course_ids = filter_course_ids or fake_accessible_course_ids
assert queryset.count() == expected_count, f'unexpected enrollment queryset count for case: {usecase}'
assert not (
set(queryset.values_list('user_id', flat=True)) - set(expected_user_ids)
), f'unexpected enrollment user ids for case: {usecase}'
assert not (
{str(enrollment.course.id) for enrollment in queryset} - set(expected_course_ids)
), f'unexpected enrollment course ids for case: {usecase}'


@pytest.mark.django_db
@pytest.mark.parametrize(
'permitted_user_ids, limited_access_course_ids, full_access_orgs, expected_count, expected_course_ids, usecase',
[
(
[], [], ['org1', 'org2'], 0, [], 'no user is accessible'
),
(
[15, 21, 29], [], ['org1', 'org2'], 6,
[
'course-v1:ORG1+5+5', 'course-v1:ORG2+4+4', 'course-v1:ORG2+5+5',
'course-v1:ORG2+6+6', 'course-v1:ORG2+7+7'
],
'full access orgs',
),
(
[15, 21, 29], ['course-v1:ORG1+5+5'], [], 2,
['course-v1:ORG1+5+5'],
'course limited access',
),
(
[15, 21, 29], ['course-v1:ORG2+4+4'], ['org1'], 3,
['course-v1:ORG1+5+5', 'course-v1:ORG2+4+4'],
'full access orgs with course limited access',
),
],
)
@patch('futurex_openedx_extensions.dashboard.details.learners.get_permitted_learners_queryset')
@patch('futurex_openedx_extensions.dashboard.details.learners.get_partial_access_course_ids')
def test_get_learners_enrollments_queryset_for_accessible_courses_and_users(
mocked_partial_course_ids, mocked_permitted_learners,
permitted_user_ids, limited_access_course_ids, full_access_orgs,
expected_count, expected_course_ids, usecase, fx_permission_info,
): # pylint: disable=too-many-arguments
"""Test get_learners_by_course_queryset result for accessible users and courses."""
mocked_partial_course_ids.return_value = limited_access_course_ids
mocked_permitted_learners.return_value = get_user_model().objects.filter(id__in=permitted_user_ids)
fx_permission_info['view_allowed_full_access_orgs'] = full_access_orgs
queryset = get_learners_enrollments_queryset(
fx_permission_info=fx_permission_info,
)
assert queryset.count() == expected_count, f'unexpected enrollment queryset count for case: {usecase}'
assert not (
set(queryset.values_list('user_id', flat=True)) - set(permitted_user_ids)
), f'unexpected enrollment user ids for case: {usecase}'
assert not (
{str(enrollment.course.id) for enrollment in queryset} - set(expected_course_ids)
), f'unexpected enrollment course ids for case: {usecase}'


@pytest.mark.django_db
@pytest.mark.parametrize(
'course_search, learner_search, expected_count, usecase',
[
('', '', 25, 'no search'),
('Course 5', '', 8, 'only course name search'),
('', 'user15', 2, 'only user search'),
('Course 5', 'user15', 1, 'only user search'),
],
)
def test_get_learners_enrollments_queryset_for_course_and_learner_search(
course_search, learner_search, expected_count, usecase, fx_permission_info,
):
"""Test get_learners_by_course_queryset result for accessible users and courses."""
fx_permission_info['view_allowed_full_access_orgs'] = ['org1', 'org2']
queryset = get_learners_enrollments_queryset(
fx_permission_info=fx_permission_info,
course_search=course_search,
learner_search=learner_search
)
assert queryset.count() == expected_count, f'unexpected enrollment queryset count for case: {usecase}'
Loading

0 comments on commit c6b9497

Please sign in to comment.