diff --git a/futurex_openedx_extensions/__init__.py b/futurex_openedx_extensions/__init__.py index f03c55a1..6378bdfb 100644 --- a/futurex_openedx_extensions/__init__.py +++ b/futurex_openedx_extensions/__init__.py @@ -1,3 +1,3 @@ """One-line description for README and other doc files.""" -__version__ = '0.9.13' +__version__ = '0.9.14' diff --git a/futurex_openedx_extensions/dashboard/details/courses.py b/futurex_openedx_extensions/dashboard/details/courses.py index 30cdf574..5fa548a8 100644 --- a/futurex_openedx_extensions/dashboard/details/courses.py +++ b/futurex_openedx_extensions/dashboard/details/courses.py @@ -1,6 +1,7 @@ """Courses details collectors""" from __future__ import annotations +from common.djangoapps.student.models import CourseEnrollment from completion.models import BlockCompletion from django.contrib.auth import get_user_model from django.db.models import ( @@ -29,6 +30,7 @@ check_staff_exist_queryset, get_base_queryset_courses, get_one_user_queryset, + update_removable_annotations, ) @@ -62,6 +64,8 @@ def annotate_courses_rating_queryset( ), 0), ) + update_removable_annotations(queryset, removable=['rating_count', 'rating_total']) + return queryset @@ -104,30 +108,36 @@ def get_courses_queryset( if include_staff: is_staff_queryset = Q(Value(False, output_field=BooleanField())) else: - is_staff_queryset = check_staff_exist_queryset('courseenrollment__user_id', 'org', 'id') + is_staff_queryset = check_staff_exist_queryset( + ref_user_id='user_id', ref_org='course__org', ref_course_id='course_id', + ) queryset = queryset.annotate( - enrolled_count=Count( - 'courseenrollment', - filter=( - Q(courseenrollment__is_active=True) & - Q(courseenrollment__user__is_active=True) & - Q(courseenrollment__user__is_staff=False) & - Q(courseenrollment__user__is_superuser=False) & - ~is_staff_queryset - ), - ) + enrolled_count=Coalesce(Subquery( + CourseEnrollment.objects.filter( + course_id=OuterRef('id'), + is_active=True, + user__is_active=True, + user__is_staff=False, + user__is_superuser=False, + ).filter( + ~is_staff_queryset, + ).values('course_id').annotate(count=Count('id')).values('count'), + output_field=IntegerField(), + ), 0) ).annotate( - active_count=Count( - 'courseenrollment', - filter=( - Q(courseenrollment__is_active=True) & - Q(courseenrollment__user__is_active=True) & - Q(courseenrollment__user__is_staff=False) & - Q(courseenrollment__user__is_superuser=False) & - ~is_staff_queryset - ), - ) + active_count=Coalesce(Subquery( + CourseEnrollment.objects.filter( + course_id=OuterRef('id'), + is_active=True, + user__is_active=True, + user__is_staff=False, + user__is_superuser=False, + ).filter( + ~is_staff_queryset, + ).values('course_id').annotate(count=Count('id')).values('count'), + output_field=IntegerField(), + ), 0) ).annotate( certificates_count=Coalesce(Subquery( GeneratedCertificate.objects.filter( @@ -144,6 +154,10 @@ def get_courses_queryset( ) ) + update_removable_annotations(queryset, removable=[ + 'enrolled_count', 'active_count', 'certificates_count', 'completion_rate', + ]) + return queryset @@ -216,4 +230,6 @@ def get_learner_courses_info_queryset( ) ) + update_removable_annotations(queryset, removable=['related_user_id', 'enrollment_date', 'last_activity']) + return queryset diff --git a/futurex_openedx_extensions/dashboard/details/learners.py b/futurex_openedx_extensions/dashboard/details/learners.py index 78bb666c..96a7238c 100644 --- a/futurex_openedx_extensions/dashboard/details/learners.py +++ b/futurex_openedx_extensions/dashboard/details/learners.py @@ -3,8 +3,10 @@ from datetime import timedelta +from common.djangoapps.student.models import CourseEnrollment from django.contrib.auth import get_user_model -from django.db.models import BooleanField, Case, Count, Exists, OuterRef, Q, Subquery, Value, When +from django.db.models import BooleanField, Case, Count, Exists, IntegerField, OuterRef, Q, Subquery, Value, When +from django.db.models.functions import Coalesce from django.db.models.query import QuerySet from django.utils import timezone from lms.djangoapps.certificates.models import GeneratedCertificate @@ -17,6 +19,7 @@ get_learners_search_queryset, get_one_user_queryset, get_permitted_learners_queryset, + update_removable_annotations, ) @@ -25,7 +28,7 @@ def get_courses_count_for_learner_queryset( visible_courses_filter: bool | None = True, active_courses_filter: bool | None = None, include_staff: bool = False, -) -> Count: +) -> Coalesce: """ Annotate the given queryset with the courses count for the learner. @@ -37,36 +40,37 @@ def get_courses_count_for_learner_queryset( :type active_courses_filter: bool | None :param include_staff: flag to include staff users :type include_staff: bool - :return: Count of learners - :rtype: Count + :return: Count of enrolled courses + :rtype: Coalesce """ if not include_staff: is_staff_queryset = check_staff_exist_queryset( - ref_user_id='id', ref_org='courseenrollment__course__org', ref_course_id='courseenrollment__course_id', + ref_user_id='user_id', ref_org='course__org', ref_course_id='course_id', ) else: is_staff_queryset = Q(Value(False, output_field=BooleanField())) - return Count( - 'courseenrollment', - filter=( - Q(courseenrollment__course_id__in=get_base_queryset_courses( + return Coalesce(Subquery( + CourseEnrollment.objects.filter( + user_id=OuterRef('id'), + course_id__in=get_base_queryset_courses( fx_permission_info, visible_filter=visible_courses_filter, active_filter=active_courses_filter, - )) & - Q(courseenrollment__is_active=True) & - ~is_staff_queryset - ), - distinct=True - ) + ), + is_active=True, + ).filter( + ~is_staff_queryset, + ).values('user_id').annotate(count=Count('id')).values('count'), + output_field=IntegerField(), + ), 0) def get_certificates_count_for_learner_queryset( fx_permission_info: dict, visible_courses_filter: bool | None = True, active_courses_filter: bool | None = None, -) -> Count: +) -> Coalesce: """ Annotate the given queryset with the certificate counts. @@ -76,23 +80,23 @@ def get_certificates_count_for_learner_queryset( :type visible_courses_filter: bool | None :param active_courses_filter: Value to filter courses on active status. None means no filter. :type active_courses_filter: bool | None - :return: QuerySet of learners - :rtype: QuerySet + :return: Count of certificates + :rtype: Coalesce """ - return Count( - 'generatedcertificate', - filter=( - Q(generatedcertificate__course_id__in=Subquery( + return Coalesce(Subquery( + GeneratedCertificate.objects.filter( + user_id=OuterRef('id'), + course_id__in=Subquery( get_base_queryset_courses( fx_permission_info, visible_filter=visible_courses_filter, active_filter=active_courses_filter ).values_list('id', flat=True) - )) & - Q(generatedcertificate__status='downloadable') - ), - distinct=True - ) + ), + status='downloadable', + ).values('user_id').annotate(count=Count('id')).values('count'), + output_field=IntegerField(), + ), 0) def get_learners_queryset( @@ -141,6 +145,8 @@ def get_learners_queryset( ) ).select_related('profile', 'extrainfo').order_by('id') + update_removable_annotations(queryset, removable=['courses_count', 'certificates_count']) + return queryset @@ -202,6 +208,8 @@ def get_learners_by_course_queryset( ) ).select_related('profile').order_by('id') + update_removable_annotations(queryset, removable=['certificate_available', 'course_score', 'active_in_course']) + return queryset @@ -249,4 +257,6 @@ def get_learner_info_queryset( ) ).select_related('profile') + update_removable_annotations(queryset, removable=['courses_count', 'certificates_count']) + return queryset diff --git a/futurex_openedx_extensions/helpers/apps.py b/futurex_openedx_extensions/helpers/apps.py index 9c084d05..d87b389a 100644 --- a/futurex_openedx_extensions/helpers/apps.py +++ b/futurex_openedx_extensions/helpers/apps.py @@ -29,4 +29,6 @@ class HelpersConfig(AppConfig): def ready(self) -> None: """Connect handlers to send notifications about discussions.""" + from futurex_openedx_extensions.helpers import \ + monkey_patches # pylint: disable=unused-import, import-outside-toplevel from futurex_openedx_extensions.helpers import signals # pylint: disable=unused-import, import-outside-toplevel diff --git a/futurex_openedx_extensions/helpers/exceptions.py b/futurex_openedx_extensions/helpers/exceptions.py index 10dc5e8b..e59d6461 100644 --- a/futurex_openedx_extensions/helpers/exceptions.py +++ b/futurex_openedx_extensions/helpers/exceptions.py @@ -38,6 +38,8 @@ class FXExceptionCodes(Enum): SERIALIZER_FILED_NAME_DOES_NOT_EXIST = 7001 + QUERY_SET_BAD_OPERATION = 8001 + class FXCodedException(Exception): """Exception with a code.""" diff --git a/futurex_openedx_extensions/helpers/monkey_patches.py b/futurex_openedx_extensions/helpers/monkey_patches.py new file mode 100644 index 00000000..d84436b5 --- /dev/null +++ b/futurex_openedx_extensions/helpers/monkey_patches.py @@ -0,0 +1,21 @@ +"""Monkey patches defined here.""" +from __future__ import annotations + +from typing import Any + +from django.db.models.query import QuerySet + +original_queryset_chain = QuerySet._chain # pylint: disable=protected-access + + +def customized_queryset_chain(self: Any, **kwargs: Any) -> QuerySet: + """Customized queryset chain method for the QuerySet class.""" + result = original_queryset_chain(self, **kwargs) + + if hasattr(self, 'removable_annotations'): + result.removable_annotations = self.removable_annotations.copy() + + return result + + +QuerySet._chain = customized_queryset_chain # pylint: disable=protected-access diff --git a/futurex_openedx_extensions/helpers/pagination.py b/futurex_openedx_extensions/helpers/pagination.py index 449c1330..7eb0bd3d 100644 --- a/futurex_openedx_extensions/helpers/pagination.py +++ b/futurex_openedx_extensions/helpers/pagination.py @@ -1,9 +1,33 @@ """Pagination helpers and classes for the API views.""" +from django.core.paginator import Paginator +from django.db.models.query import QuerySet +from django.utils.functional import cached_property from rest_framework.pagination import PageNumberPagination +from futurex_openedx_extensions.helpers.querysets import verify_queryset_removable_annotations + + +class DefaultPaginator(Paginator): + """Default paginator settings for the API views.""" + @cached_property + def count(self) -> int: + """Return the total number of objects, across all pages.""" + if isinstance(self.object_list, QuerySet) and hasattr(self.object_list, 'removable_annotations'): + verify_queryset_removable_annotations(self.object_list) + + clone = self.object_list._chain() # pylint: disable=protected-access + for key in self.object_list.removable_annotations: + clone.query.annotations.pop(key, None) + + return clone.count() + + return super().count + class DefaultPagination(PageNumberPagination): """Default pagination settings for the API views.""" page_size: int = 20 page_size_query_param: str = 'page_size' max_page_size: int = 100 + + django_paginator_class = DefaultPaginator diff --git a/futurex_openedx_extensions/helpers/querysets.py b/futurex_openedx_extensions/helpers/querysets.py index fed2d6fb..51056f97 100644 --- a/futurex_openedx_extensions/helpers/querysets.py +++ b/futurex_openedx_extensions/helpers/querysets.py @@ -5,7 +5,7 @@ from common.djangoapps.student.models import CourseAccessRole, CourseEnrollment, UserSignupSource from django.contrib.auth import get_user_model -from django.db.models import BooleanField, Case, Exists, OuterRef, Q, Value, When +from django.db.models import BooleanField, Case, Count, Exists, OuterRef, Q, Value, When from django.db.models.query import QuerySet from django.utils.timezone import now from opaque_keys.edx.django.models import CourseKeyField @@ -18,6 +18,64 @@ from futurex_openedx_extensions.helpers.users import get_user_by_key +def verify_queryset_removable_annotations(queryset: QuerySet) -> None: + """ + Verify that the queryset has the removable annotations set. + + :param queryset: QuerySet to verify + :type queryset: QuerySet + """ + if not hasattr(queryset, 'removable_annotations'): + return + + for key in queryset.removable_annotations: + if isinstance(queryset.query.annotations.get(key, None), Count): + raise FXCodedException( + code=FXExceptionCodes.QUERY_SET_BAD_OPERATION, + message=( + f'Cannot set annotation `{key}` of type `Count` as removable. You must unset it from the ' + f'removable annotations list, or replace the `Count` annotation with `Subquery`.' + ), + ) + + +def update_removable_annotations( + queryset: QuerySet, + removable: set | List[str] | None = None, + not_removable: set | List[str] | None = None, +) -> None: + """ + Update the removable annotations on the given queryset. + + :param queryset: QuerySet to update + :type queryset: QuerySet + :param removable: Set of annotations to add to the removable annotations + :type removable: set(str) | List[str] | None + :param not_removable: Set of annotations to remove from the removable annotations + :type not_removable: set(str) | List[str] | None + """ + removable_annotations = queryset.removable_annotations if hasattr(queryset, 'removable_annotations') else set() + removable_annotations = (removable_annotations | set(removable or [])) - set(not_removable or []) + + if not removable_annotations and hasattr(queryset, 'removable_annotations'): + del queryset.removable_annotations + + elif removable_annotations: + queryset.removable_annotations = removable_annotations + verify_queryset_removable_annotations(queryset) + + +def clear_removable_annotations(queryset: QuerySet) -> None: + """ + Clear the removable annotations on the given queryset. + + :param queryset: QuerySet to clear the removable annotations from + :type queryset: QuerySet + """ + if hasattr(queryset, 'removable_annotations'): + del queryset.removable_annotations + + def check_staff_exist_queryset( ref_user_id: str | Value, ref_org: str | Value | List | None, @@ -137,11 +195,15 @@ def get_base_queryset_courses( ), ) + update_removable_annotations(q_set, removable=['course_is_active', 'course_is_visible']) + if active_filter is not None: q_set = q_set.filter(course_is_active=active_filter) + update_removable_annotations(q_set, not_removable=['course_is_active']) if visible_filter is not None: q_set = q_set.filter(course_is_visible=visible_filter) + update_removable_annotations(q_set, not_removable=['course_is_visible']) return q_set diff --git a/tests/test_helpers/test_monkey_patches.py b/tests/test_helpers/test_monkey_patches.py new file mode 100644 index 00000000..547afcf8 --- /dev/null +++ b/tests/test_helpers/test_monkey_patches.py @@ -0,0 +1,44 @@ +"""Tests for monkey patches""" +from unittest.mock import MagicMock, patch + +from django.db.models.query import QuerySet + +from futurex_openedx_extensions.helpers.monkey_patches import customized_queryset_chain, original_queryset_chain + + +def test_queryset_chain(): + """Verify that the original_queryset_chain is correctly defined.""" + assert QuerySet._chain == customized_queryset_chain # pylint: disable=protected-access, comparison-with-callable + assert original_queryset_chain is not customized_queryset_chain + + +@patch('futurex_openedx_extensions.helpers.monkey_patches.original_queryset_chain') +def test_customized_queryset_chain_has_attribute(mock_original_queryset_chain): + """Verify that the customized_queryset_chain is correctly defined.""" + mock_original_queryset_chain.return_value = MagicMock(spec=QuerySet) + del mock_original_queryset_chain.return_value.removable_annotations + + mock_queryset = MagicMock(spec=QuerySet) + mock_queryset.removable_annotations = {'annotation1'} + mock_queryset._chain.return_value = mock_queryset # pylint: disable=protected-access + + result = customized_queryset_chain(mock_queryset) + assert result.removable_annotations == mock_queryset.removable_annotations + mock_original_queryset_chain.assert_called_once_with(mock_queryset) + + +@patch('futurex_openedx_extensions.helpers.monkey_patches.original_queryset_chain') +def test_customized_queryset_chain_no_attribute(mock_original_queryset_chain): + """Verify that the customized_queryset_chain is correctly defined.""" + mock_original_queryset_chain.return_value = MagicMock(spec=QuerySet) + delattr( # pylint: disable=literal-used-as-attribute + mock_original_queryset_chain.return_value, 'removable_annotations', + ) + + mock_queryset = MagicMock(spec=QuerySet) + del mock_queryset.removable_annotations + mock_queryset._chain.return_value = mock_queryset # pylint: disable=protected-access + + result = customized_queryset_chain(mock_queryset) + assert not hasattr(result, 'removable_annotations') + mock_original_queryset_chain.assert_called_once_with(mock_queryset) diff --git a/tests/test_helpers/test_pagination.py b/tests/test_helpers/test_pagination.py index 22f78c06..2fc2f705 100644 --- a/tests/test_helpers/test_pagination.py +++ b/tests/test_helpers/test_pagination.py @@ -1,7 +1,10 @@ """Tests for pagination helpers""" +from unittest.mock import MagicMock, PropertyMock, patch + +from django.db.models import QuerySet from rest_framework.pagination import PageNumberPagination -from futurex_openedx_extensions.helpers.pagination import DefaultPagination +from futurex_openedx_extensions.helpers.pagination import DefaultPagination, DefaultPaginator def test_default_pagination(): @@ -10,3 +13,55 @@ def test_default_pagination(): assert DefaultPagination.page_size == 20 assert DefaultPagination.page_size_query_param == 'page_size' assert DefaultPagination.max_page_size == 100 + assert DefaultPagination.django_paginator_class == DefaultPaginator + + +@patch('futurex_openedx_extensions.helpers.pagination.Paginator.count', new_callable=PropertyMock) +@patch('futurex_openedx_extensions.helpers.pagination.verify_queryset_removable_annotations') +def test_count_with_query_set_and_removable_annotations(mock_verify, mock_super_count): + """Verify that the count property is correctly defined.""" + mock_super_count.return_value = 'should not be reached' + + mock_queryset = MagicMock(spec=QuerySet) + mock_queryset.removable_annotations = {'annotation1'} + mock_queryset._chain.return_value = mock_queryset # pylint: disable=protected-access + mock_queryset.query.annotations = {'annotation1': None, 'annotation2': None} + mock_queryset.count.return_value = 5 + + paginator = DefaultPaginator(mock_queryset, per_page=10) + + assert paginator.count == 5 + assert 'annotation1' not in mock_queryset.query.annotations + mock_super_count.assert_not_called() + mock_verify.assert_called_once_with(mock_queryset) + + +@patch('futurex_openedx_extensions.helpers.pagination.Paginator.count', new_callable=PropertyMock) +@patch('futurex_openedx_extensions.helpers.pagination.verify_queryset_removable_annotations') +def test_count_with_query_set_no_removable_annotations(mock_verify, mock_super_count): + """Verify that the count property is correctly defined.""" + mock_super_count.return_value = 44 + + mock_queryset = MagicMock(spec=QuerySet) + del mock_queryset.removable_annotations + mock_queryset.count.return_value = 'should not be reached' + mock_queryset.query.annotations = {'annotation1': None, 'annotation2': None} + + paginator = DefaultPaginator(mock_queryset, per_page=10) + + assert paginator.count == mock_super_count.return_value + assert mock_queryset.query.annotations == {'annotation1': None, 'annotation2': None} + mock_verify.assert_not_called() + + +@patch('futurex_openedx_extensions.helpers.pagination.Paginator.count', new_callable=PropertyMock) +@patch('futurex_openedx_extensions.helpers.pagination.verify_queryset_removable_annotations') +def test_count_with_not_query_set(mock_verify, mock_super_count): + """Verify that the count property is correctly defined.""" + mock_super_count.return_value = 44 + + object_list = 'not a queryset' + paginator = DefaultPaginator(object_list, per_page=10) + + assert paginator.count == mock_super_count.return_value + mock_verify.assert_not_called() diff --git a/tests/test_helpers/test_querysets.py b/tests/test_helpers/test_querysets.py index b9a6b7f0..3a60140f 100644 --- a/tests/test_helpers/test_querysets.py +++ b/tests/test_helpers/test_querysets.py @@ -1,10 +1,14 @@ """Tests for querysets helpers""" +from unittest.mock import Mock, patch + import pytest from common.djangoapps.student.models import CourseAccessRole, UserProfile from django.contrib.auth import get_user_model +from django.db.models import Count from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from futurex_openedx_extensions.helpers import querysets +from futurex_openedx_extensions.helpers.exceptions import FXCodedException, FXExceptionCodes from tests.fixture_helpers import get_tenants_orgs @@ -217,3 +221,80 @@ def test_get_permitted_learners_queryset( result = querysets.get_permitted_learners_queryset(queryset, fx_permission_info, include_staff=True) assert result.count() == expected_with_staff + + +@pytest.mark.parametrize('original, removable, not_removable, expected_result', [ + (None, None, None, None), + (None, {'f1', 'f2'}, None, {'f1', 'f2'}), + (None, None, {'f1', 'f2'}, None), + ({'f1', 'f2'}, None, None, {'f1', 'f2'}), + ({'f1', 'f2'}, {'f1'}, None, {'f1', 'f2'}), + ({'f1', 'f2'}, None, {'f1'}, {'f2'}), + ({'f1', 'f2'}, None, {'f1', 'f3'}, {'f2'}), + ({'f1', 'f2'}, {'f1'}, {'f1', 'f2'}, None), +]) +def test_update_removable_annotations(original, removable, not_removable, expected_result): + """Verify update_removable_annotations function.""" + queryset = Mock() + if original is None: + del queryset.removable_annotations + else: + queryset.removable_annotations = original + + with patch('futurex_openedx_extensions.helpers.querysets.verify_queryset_removable_annotations') as mock_verify: + querysets.update_removable_annotations(queryset, removable, not_removable) + + if expected_result is None: + assert not hasattr(queryset, 'removable_annotations') + mock_verify.assert_not_called() + else: + assert queryset.removable_annotations == expected_result + mock_verify.assert_called_once_with(queryset) + + +def test_verify_queryset_removable_annotations(): + """Verify verify_queryset_removable_annotations will raise an error for annotations with type Count""" + queryset = Mock(removable_annotations={'f1'}, query=Mock()) + queryset.query.annotations = { + 'f1': 'not Count', + 'f2': Mock(spec=Count), + } + querysets.verify_queryset_removable_annotations(queryset) + + queryset.removable_annotations.add('f2') + with pytest.raises(FXCodedException) as exc_info: + querysets.verify_queryset_removable_annotations(queryset) + assert exc_info.value.code == FXExceptionCodes.QUERY_SET_BAD_OPERATION.value + assert str(exc_info.value) == ( + 'Cannot set annotation `f2` of type `Count` as removable. You must unset it from the ' + 'removable annotations list, or replace the `Count` annotation with `Subquery`.' + ) + + +def test_verify_queryset_removable_annotations_no_removable(): + """Verify verify_queryset_removable_annotations will not raise an error if there are no removable annotations""" + queryset = Mock(query=Mock()) + del queryset.removable_annotations + queryset.query.annotations['f1']: Mock(spec=Count) + querysets.verify_queryset_removable_annotations(queryset) + + +def test_verify_queryset_removable_annotations_removable_does_not_exist(): + """Verify verify_queryset_removable_annotations will not raise an error if a removable annotation does not exist""" + queryset = Mock(removable_annotations={'f3'}, query=Mock()) + queryset.query.annotations = { + 'f1': 'not Count', + 'f2': Mock(spec=Count), + } + querysets.verify_queryset_removable_annotations(queryset) + + +def test_clear_removable_annotations(): + """Verify clear_removable_annotations function.""" + queryset = Mock(removable_annotations={'f1', 'f2'}) + querysets.clear_removable_annotations(queryset) + + assert not hasattr(queryset, 'removable_annotations') + + querysets.clear_removable_annotations(queryset) + assert not hasattr(queryset, 'removable_annotations')