From dec869e430acb72500c69d00c9bafd18e9494367 Mon Sep 17 00:00:00 2001 From: mkurkar Date: Sun, 30 Nov 2025 15:39:01 +0300 Subject: [PATCH 01/10] fix: rating statistics very slow --- .../dashboard/settings/common_production.py | 7 ++ .../dashboard/statistics/courses.py | 79 +++++++++++++------ futurex_openedx_extensions/dashboard/views.py | 55 ++++++++++--- 3 files changed, 107 insertions(+), 34 deletions(-) diff --git a/futurex_openedx_extensions/dashboard/settings/common_production.py b/futurex_openedx_extensions/dashboard/settings/common_production.py index f4c26388..d404cc62 100644 --- a/futurex_openedx_extensions/dashboard/settings/common_production.py +++ b/futurex_openedx_extensions/dashboard/settings/common_production.py @@ -13,6 +13,13 @@ def plugin_settings(settings: Any) -> None: 60 * 60 * 2, # 2 hours ) + # Cache timeout for course ratings per tenant + settings.FX_CACHE_TIMEOUT_COURSES_RATINGS = getattr( + settings, + 'FX_CACHE_TIMEOUT_COURSES_RATINGS', + 60 * 15, # 15 minutes + ) + settings.FX_DISABLE_CONFIG_VALIDATIONS = getattr( settings, 'FX_DISABLE_CONFIG_VALIDATIONS', diff --git a/futurex_openedx_extensions/dashboard/statistics/courses.py b/futurex_openedx_extensions/dashboard/statistics/courses.py index 81c346ef..e3c44d41 100644 --- a/futurex_openedx_extensions/dashboard/statistics/courses.py +++ b/futurex_openedx_extensions/dashboard/statistics/courses.py @@ -11,14 +11,18 @@ from django.utils.timezone import now from futurex_openedx_extensions.dashboard.details.courses import annotate_courses_rating_queryset +from futurex_openedx_extensions.helpers.caching import cache_dict from futurex_openedx_extensions.helpers.constants import COURSE_STATUSES from futurex_openedx_extensions.helpers.extractors import get_valid_duration +from futurex_openedx_extensions.helpers.permissions import build_fx_permission_info from futurex_openedx_extensions.helpers.querysets import ( annotate_period, check_staff_exist_queryset, get_base_queryset_courses, ) +RATING_RANGE = range(1, 6) + def get_courses_count( fx_permission_info: dict, visible_filter: bool | None = True, active_filter: bool | None = None @@ -208,16 +212,32 @@ def get_courses_count_by_status( return q_set +def _cache_key_courses_ratings(tenant_id: int, visible_filter: bool | None, active_filter: bool | None) -> str: + """ + Generate cache key for get_courses_ratings + + :param tenant_id: Tenant ID + :type tenant_id: int + :param visible_filter: Value to filter courses on catalog visibility + :type visible_filter: bool | None + :param active_filter: Value to filter courses on active status + :type active_filter: bool | None + :return: Cache key string + :rtype: str + """ + return f'fx_courses_ratings_t{tenant_id}_v{visible_filter}_a{active_filter}' + + def get_courses_ratings( - fx_permission_info: dict, + tenant_id: int, visible_filter: bool | None = True, active_filter: bool | None = None, ) -> Dict[str, int]: """ - Get the average rating of courses in the given tenants + Get the average rating of courses for a single tenant. Results are cached per tenant. - :param fx_permission_info: Dictionary containing permission information - :type fx_permission_info: dict + :param tenant_id: Tenant ID to get ratings for + :type tenant_id: int :param visible_filter: Value to filter courses on catalog visibility. None means no filter :type visible_filter: bool | None :param active_filter: Value to filter courses on active status. None means no filter (according to dates) @@ -225,24 +245,37 @@ def get_courses_ratings( :return: Dictionary containing the total rating, courses count, and rating count per rating value :rtype: Dict[str, int] """ - q_set = get_base_queryset_courses( - fx_permission_info, visible_filter=visible_filter, active_filter=active_filter + @cache_dict( + timeout='FX_CACHE_TIMEOUT_COURSES_RATINGS', + key_generator_or_name=_cache_key_courses_ratings ) + def _get_ratings(t_id: int, v_filter: bool | None, a_filter: bool | None) -> Dict[str, int]: + """ + Inner function to compute ratings with caching + """ + fx_permission_info = build_fx_permission_info(t_id) + q_set = get_base_queryset_courses( + fx_permission_info, visible_filter=v_filter, active_filter=a_filter + ) - q_set = annotate_courses_rating_queryset(q_set).filter(rating_count__gt=0) - - q_set = q_set.annotate(**{ - f'course_rating_{rate_value}_count': Count( - 'feedbackcourse', - filter=Q(feedbackcourse__rating_content=rate_value) - ) for rate_value in range(1, 6) - }) - - return q_set.aggregate( - total_rating=Coalesce(Sum('rating_total'), 0), - courses_count=Coalesce(Count('id'), 0), - **{ - f'rating_{rate_value}_count': Coalesce(Sum(f'course_rating_{rate_value}_count'), 0) - for rate_value in range(1, 6) - } - ) + q_set = annotate_courses_rating_queryset(q_set).filter(rating_count__gt=0) + + # Annotate each rating level count (1-5 stars) + q_set = q_set.annotate(**{ + f'course_rating_{rate_value}_count': Count( + 'feedbackcourse', + filter=Q(feedbackcourse__rating_content=rate_value) + ) for rate_value in RATING_RANGE + }) + + # Aggregate total ratings and counts per rating level + return q_set.aggregate( + total_rating=Coalesce(Sum('rating_total'), 0), + courses_count=Coalesce(Count('id'), 0), + **{ + f'rating_{rate_value}_count': Coalesce(Sum(f'course_rating_{rate_value}_count'), 0) + for rate_value in RATING_RANGE + } + ) + + return _get_ratings(tenant_id, visible_filter, active_filter) diff --git a/futurex_openedx_extensions/dashboard/views.py b/futurex_openedx_extensions/dashboard/views.py index 42fa7843..64ceed7f 100644 --- a/futurex_openedx_extensions/dashboard/views.py +++ b/futurex_openedx_extensions/dashboard/views.py @@ -73,7 +73,7 @@ COURSE_STATUSES, FX_VIEW_DEFAULT_AUTH_CLASSES, ) -from futurex_openedx_extensions.helpers.converters import dict_to_hash, error_details_to_dictionary +from futurex_openedx_extensions.helpers.converters import dict_to_hash, error_details_to_dictionary, ids_string_to_list from futurex_openedx_extensions.helpers.course_categories import CourseCategories from futurex_openedx_extensions.helpers.exceptions import FXCodedException, FXExceptionCodes from futurex_openedx_extensions.helpers.export_mixins import ExportCSVMixin @@ -114,7 +114,9 @@ from futurex_openedx_extensions.helpers.upload import get_storage_dir, upload_file from futurex_openedx_extensions.helpers.users import get_user_by_key +# Constants default_auth_classes = FX_VIEW_DEFAULT_AUTH_CLASSES.copy() +RATING_RANGE = range(1, 6) # Ratings from 1 to 5 stars logger = logging.getLogger(__name__) @@ -1064,28 +1066,59 @@ def post(self, request: Any, *args: Any, **kwargs: Any) -> Response: @docs('GlobalRatingView.get') class GlobalRatingView(FXViewRoleInfoMixin, APIView): - """View to get the global rating""" + """View to get the global rating for a single tenant""" authentication_classes = default_auth_classes permission_classes = [FXHasTenantCourseAccess] fx_view_name = 'global_rating' fx_default_read_only_roles = ['staff', 'instructor', 'data_researcher', 'org_course_creator_group'] - fx_view_description = 'api/fx/statistics/v1/rating/: Get the global rating for courses' + fx_view_description = 'api/fx/statistics/v1/rating/: Get the global rating for courses in a single tenant' def get(self, request: Any, *args: Any, **kwargs: Any) -> JsonResponse: """ - GET /api/fx/statistics/v1/rating/?tenant_ids= + GET /api/fx/statistics/v1/rating/?tenant_ids= - (optional): a comma-separated list of the tenant IDs to get the information for. If not provided, - the API will assume the list of all accessible tenants by the user + (required): a single tenant ID to get the rating information for. + Multiple tenant IDs are not supported - only one tenant ID must be provided. """ - data_result = get_courses_ratings(fx_permission_info=self.fx_permission_info) + tenant_ids_string = request.GET.get('tenant_ids') + if not tenant_ids_string: + raise FXCodedException( + code=FXExceptionCodes.TENANT_ID_REQUIRED_AS_URL_ARG, + message='tenant_ids parameter is required' + ) + + try: + tenant_ids = ids_string_to_list(tenant_ids_string) + except ValueError as exc: + raise FXCodedException( + code=FXExceptionCodes.TENANT_NOT_FOUND, + message=f'Invalid tenant_ids provided: {str(exc)}' + ) from exc + + if len(tenant_ids) != 1: + raise FXCodedException( + code=FXExceptionCodes.TENANT_ID_REQUIRED_AS_URL_ARG, + message=f'Exactly one tenant ID is required, got {len(tenant_ids)}' + ) + + tenant_id = tenant_ids[0] + accessible_tenant_ids = self.fx_permission_info.get('view_allowed_tenant_ids_any_access', []) + if tenant_id not in accessible_tenant_ids: + raise PermissionDenied( + detail=json.dumps({ + 'reason': f'User does not have access to tenant {tenant_id}' + }) + ) + + data_result = get_courses_ratings(tenant_id=tenant_id) + rating_counts = {str(i): data_result[f'rating_{i}_count'] for i in RATING_RANGE} + total_count = sum(rating_counts.values()) + result = { 'total_rating': data_result['total_rating'], - 'total_count': sum(data_result[f'rating_{index}_count'] for index in range(1, 6)), + 'total_count': total_count, 'courses_count': data_result['courses_count'], - 'rating_counts': { - str(index): data_result[f'rating_{index}_count'] for index in range(1, 6) - }, + 'rating_counts': rating_counts, } return JsonResponse(result) From 8f8fd714ec008a41d8dca38b4ca42ad0a80c5aaf Mon Sep 17 00:00:00 2001 From: mkurkar Date: Sun, 30 Nov 2025 15:39:41 +0300 Subject: [PATCH 02/10] fix: optimize course ratings retrieval and add tenant ID handling --- .../dashboard/statistics/courses.py | 2 - test_utils/test_settings_common.py | 1 + .../test_statistics/test_courses.py | 4 +- tests/test_dashboard/test_views.py | 82 ++++++++++++++++++- 4 files changed, 82 insertions(+), 7 deletions(-) diff --git a/futurex_openedx_extensions/dashboard/statistics/courses.py b/futurex_openedx_extensions/dashboard/statistics/courses.py index e3c44d41..75dab204 100644 --- a/futurex_openedx_extensions/dashboard/statistics/courses.py +++ b/futurex_openedx_extensions/dashboard/statistics/courses.py @@ -260,7 +260,6 @@ def _get_ratings(t_id: int, v_filter: bool | None, a_filter: bool | None) -> Dic q_set = annotate_courses_rating_queryset(q_set).filter(rating_count__gt=0) - # Annotate each rating level count (1-5 stars) q_set = q_set.annotate(**{ f'course_rating_{rate_value}_count': Count( 'feedbackcourse', @@ -268,7 +267,6 @@ def _get_ratings(t_id: int, v_filter: bool | None, a_filter: bool | None) -> Dic ) for rate_value in RATING_RANGE }) - # Aggregate total ratings and counts per rating level return q_set.aggregate( total_rating=Coalesce(Sum('rating_total'), 0), courses_count=Coalesce(Count('id'), 0), diff --git a/test_utils/test_settings_common.py b/test_utils/test_settings_common.py index 8b214fe5..db262531 100644 --- a/test_utils/test_settings_common.py +++ b/test_utils/test_settings_common.py @@ -92,6 +92,7 @@ def root(*args): FX_CACHE_TIMEOUT_VIEW_ROLES = 60 * 31 # 31 minutes FX_CACHE_TIMEOUT_LIVE_STATISTICS_PER_TENANT = 60 * 60 * 3 # three hours FX_CACHE_TIMEOUT_CONFIG_ACCESS_CONTROL = 60 * 60 * 48 # 2 days +FX_CACHE_TIMEOUT_COURSES_RATINGS = 60 * 15 # 15 minutes FX_TASK_MINUTES_LIMIT = 6 # 6 minutes FX_MAX_PERIOD_CHUNKS_MAP = { 'day': 365 * 2, diff --git a/tests/test_dashboard/test_statistics/test_courses.py b/tests/test_dashboard/test_statistics/test_courses.py index f428a195..e95d8ec4 100644 --- a/tests/test_dashboard/test_statistics/test_courses.py +++ b/tests/test_dashboard/test_statistics/test_courses.py @@ -89,7 +89,7 @@ def test_get_courses_ratings(base_data, fx_permission_info): # pylint: disable= rating_content=rate, ) - result = courses.get_courses_ratings(fx_permission_info) + result = courses.get_courses_ratings(tenant_id=1) assert result['total_rating'] == 114 assert result['courses_count'] == 3 assert result['rating_1_count'] == 3 @@ -105,7 +105,7 @@ def test_get_courses_ratings_no_rating(base_data, fx_permission_info): # pylint expected_keys = ['total_rating', 'courses_count'] + [ f'rating_{i}_count' for i in range(1, 6) ] - result = courses.get_courses_ratings(fx_permission_info) + result = courses.get_courses_ratings(tenant_id=1) assert set(result.keys()) == set(expected_keys) assert all(result[key] is not None for key in expected_keys) assert all(result[key] == 0 for key in expected_keys) diff --git a/tests/test_dashboard/test_views.py b/tests/test_dashboard/test_views.py index d4670470..40c7173d 100644 --- a/tests/test_dashboard/test_views.py +++ b/tests/test_dashboard/test_views.py @@ -28,7 +28,7 @@ from opaque_keys.edx.locator import CourseLocator, LibraryLocator from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from rest_framework import status as http_status -from rest_framework.exceptions import ParseError +from rest_framework.exceptions import ParseError, PermissionDenied from rest_framework.response import Response from rest_framework.status import HTTP_400_BAD_REQUEST from rest_framework.test import APIRequestFactory, APITestCase @@ -36,6 +36,7 @@ from futurex_openedx_extensions.dashboard import serializers, urls, views from futurex_openedx_extensions.dashboard.views import ( + GlobalRatingView, LearnersEnrollmentView, ThemeConfigDraftView, ThemeConfigPublishView, @@ -1748,6 +1749,7 @@ def get_query_record(cls, scope, version, slug): ) +@pytest.mark.usefixtures('base_data') class TestGlobalRatingView(BaseTestViewMixin): """Tests for GlobalRatingView""" VIEW_NAME = 'fx_dashboard:statistics-rating' @@ -1793,10 +1795,11 @@ def test_success(self): with patch('futurex_openedx_extensions.dashboard.views.get_courses_ratings') as mocked_calc: mocked_calc.return_value = test_data - response = self.client.get(self.url) + response = self.client.get(f'{self.url}?tenant_ids=1') data = json.loads(response.content) self.assertEqual(response.status_code, http_status.HTTP_200_OK) self.assertEqual(data, expected_result) + mocked_calc.assert_called_once_with(tenant_id=1) def test_success_no_rating(self): """Verify that the view returns the correct response when there are no ratings""" @@ -1811,7 +1814,7 @@ def test_success_no_rating(self): 'rating_4_count': 0, 'rating_5_count': 0, } - response = self.client.get(self.url) + response = self.client.get(f'{self.url}?tenant_ids=1') data = json.loads(response.content) self.assertEqual(response.status_code, http_status.HTTP_200_OK) self.assertEqual(data, { @@ -1826,6 +1829,79 @@ def test_success_no_rating(self): '5': 0, }, }) + mocked_calc.assert_called_once_with(tenant_id=1) + + def test_missing_tenant_ids(self): + """Verify that the view returns 400 when tenant_ids parameter is missing""" + self.login_user(self.staff_user) + response = self.client.get(self.url) + self.assertEqual(response.status_code, http_status.HTTP_400_BAD_REQUEST) + data = json.loads(response.content) + self.assertIn('reason', data) + self.assertIn('tenant_ids parameter is required', data['reason']) + + def test_multiple_tenant_ids(self): + """Verify that the view returns 400 when multiple tenant_ids are provided""" + self.login_user(self.staff_user) + response = self.client.get(f'{self.url}?tenant_ids=1,2') + self.assertEqual(response.status_code, http_status.HTTP_400_BAD_REQUEST) + data = json.loads(response.content) + self.assertIn('reason', data) + self.assertIn('Exactly one tenant ID is required', data['reason']) + + def test_invalid_tenant_id_format(self): + """Verify that the view returns 403 when tenant_ids has invalid format""" + self.login_user(self.staff_user) + response = self.client.get(f'{self.url}?tenant_ids=invalid') + self.assertEqual(response.status_code, http_status.HTTP_403_FORBIDDEN) + data = json.loads(response.content) + self.assertIn('reason', data) + + def test_unauthorized_tenant_access(self): + """Verify that the view returns 403 when user doesn't have access to the tenant""" + self.login_user(self.staff_user) + response = self.client.get(f'{self.url}?tenant_ids=999') + self.assertEqual(response.status_code, http_status.HTTP_403_FORBIDDEN) + + def test_direct_call_invalid_tenant_format(self): + """Test ValueError handler when ids_string_to_list raises ValueError (bypassing middleware)""" + self.login_user(self.staff_user) + factory = APIRequestFactory() + request = factory.get(f'{self.url}?tenant_ids=invalid_format') + request.user = self.staff_user + request.fx_permission_info = { + 'user': self.staff_user, + 'view_allowed_tenant_ids_any_access': [1, 2], + } + + view = GlobalRatingView() + view.request = request + + with patch('futurex_openedx_extensions.dashboard.views.ids_string_to_list') as mock_ids: + mock_ids.side_effect = ValueError('Invalid format') + with self.assertRaises(FXCodedException) as context: + view.get(request) + self.assertEqual(context.exception.code, FXExceptionCodes.TENANT_NOT_FOUND.value) + self.assertIn('Invalid tenant_ids provided', str(context.exception)) + + def test_direct_call_unauthorized_tenant(self): + """Test PermissionDenied when tenant_id not in accessible list (bypassing middleware)""" + self.login_user(self.staff_user) + factory = APIRequestFactory() + request = factory.get(f'{self.url}?tenant_ids=999') + request.user = self.staff_user + request.fx_permission_info = { + 'user': self.staff_user, + 'view_allowed_tenant_ids_any_access': [1, 2], + } + + view = GlobalRatingView() + view.request = request + + with self.assertRaises(PermissionDenied) as context: + view.get(request) + error_detail = json.loads(context.exception.detail) + self.assertIn('User does not have access to tenant 999', error_detail['reason']) @ddt.ddt From cba9a7baa8d42973447488505ba902773caa8dc9 Mon Sep 17 00:00:00 2001 From: mkurkar Date: Sun, 30 Nov 2025 16:09:56 +0300 Subject: [PATCH 03/10] fix: update global rating statistics endpoint to require a single tenant ID and improve error handling --- .../dashboard/docs_src.py | 32 +++++++++++++++---- futurex_openedx_extensions/helpers/admin.py | 2 +- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/futurex_openedx_extensions/dashboard/docs_src.py b/futurex_openedx_extensions/dashboard/docs_src.py index caf5b142..9a262dfb 100644 --- a/futurex_openedx_extensions/dashboard/docs_src.py +++ b/futurex_openedx_extensions/dashboard/docs_src.py @@ -840,14 +840,22 @@ def get_optional_parameter(path: str) -> Any: }, 'GlobalRatingView.get': { - 'summary': 'Get global rating statistics for the tenants', - 'description': 'Get global rating statistics for the tenants. The response will include the average rating and' - ' the total number of ratings for the selected tenants, plus the number of ratings for each rating value from' - ' 0 to 5 (inclusive).\n' - '\n**Note:** the count includes only visible courses.\n' + 'summary': 'Get global rating statistics for a single tenant', + 'description': 'Get global rating statistics for a single tenant. The response will include the average rating' + ' and the total number of ratings for the tenant, plus the number of ratings for each rating value from' + ' 1 to 5 (inclusive).\n' + '\n**Important:** This endpoint requires exactly **one** tenant ID. Multiple tenant IDs are not supported.\n' + '\n**Note:** The count includes only visible courses.\n' f'{repeated_descriptions["visible_course_definition"]}', 'parameters': [ - common_parameters['tenant_ids'], + openapi.Parameter( + 'tenant_ids', + ParameterLocation.QUERY, + required=True, + type=openapi.TYPE_INTEGER, + description='A single tenant ID to get the rating information for. **Required.** Multiple tenant IDs' + ' are not supported - exactly one tenant ID must be provided.', + ), ], 'responses': responses( overrides={ @@ -881,8 +889,18 @@ def get_optional_parameter(path: str) -> Any: } } ), + 400: openapi.Schema( + type=openapi.TYPE_OBJECT, + properties={ + 'reason': openapi.Schema(type=openapi.TYPE_STRING), + }, + description='Bad Request - Missing tenant_ids parameter, invalid format, or multiple tenant IDs' + ' provided when only one is required.', + example={ + 'reason': 'tenant_ids parameter is required', + } + ), }, - remove=[400] ), }, diff --git a/futurex_openedx_extensions/helpers/admin.py b/futurex_openedx_extensions/helpers/admin.py index c4218552..da6e9eae 100644 --- a/futurex_openedx_extensions/helpers/admin.py +++ b/futurex_openedx_extensions/helpers/admin.py @@ -4,7 +4,7 @@ import re from typing import Any, List, Tuple -import yaml # type: ignore +import yaml # type: ignore[import-untyped] from common.djangoapps.student.admin import CourseAccessRoleForm from django import forms from django.conf import settings From 3ce71826f9eacd36a7f6705ce8893fee789f1646 Mon Sep 17 00:00:00 2001 From: mkurkar Date: Sun, 30 Nov 2025 16:14:14 +0300 Subject: [PATCH 04/10] fix: move RATING_RANGE constant to constants module for better organization --- futurex_openedx_extensions/dashboard/statistics/courses.py | 4 +--- futurex_openedx_extensions/dashboard/views.py | 2 +- futurex_openedx_extensions/helpers/constants.py | 1 + 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/futurex_openedx_extensions/dashboard/statistics/courses.py b/futurex_openedx_extensions/dashboard/statistics/courses.py index 75dab204..bad1ee5e 100644 --- a/futurex_openedx_extensions/dashboard/statistics/courses.py +++ b/futurex_openedx_extensions/dashboard/statistics/courses.py @@ -12,7 +12,7 @@ from futurex_openedx_extensions.dashboard.details.courses import annotate_courses_rating_queryset from futurex_openedx_extensions.helpers.caching import cache_dict -from futurex_openedx_extensions.helpers.constants import COURSE_STATUSES +from futurex_openedx_extensions.helpers.constants import COURSE_STATUSES, RATING_RANGE from futurex_openedx_extensions.helpers.extractors import get_valid_duration from futurex_openedx_extensions.helpers.permissions import build_fx_permission_info from futurex_openedx_extensions.helpers.querysets import ( @@ -21,8 +21,6 @@ get_base_queryset_courses, ) -RATING_RANGE = range(1, 6) - def get_courses_count( fx_permission_info: dict, visible_filter: bool | None = True, active_filter: bool | None = None diff --git a/futurex_openedx_extensions/dashboard/views.py b/futurex_openedx_extensions/dashboard/views.py index 64ceed7f..64b3960d 100644 --- a/futurex_openedx_extensions/dashboard/views.py +++ b/futurex_openedx_extensions/dashboard/views.py @@ -72,6 +72,7 @@ COURSE_STATUS_SELF_PREFIX, COURSE_STATUSES, FX_VIEW_DEFAULT_AUTH_CLASSES, + RATING_RANGE, ) from futurex_openedx_extensions.helpers.converters import dict_to_hash, error_details_to_dictionary, ids_string_to_list from futurex_openedx_extensions.helpers.course_categories import CourseCategories @@ -116,7 +117,6 @@ # Constants default_auth_classes = FX_VIEW_DEFAULT_AUTH_CLASSES.copy() -RATING_RANGE = range(1, 6) # Ratings from 1 to 5 stars logger = logging.getLogger(__name__) diff --git a/futurex_openedx_extensions/helpers/constants.py b/futurex_openedx_extensions/helpers/constants.py index 7dba5857..a304ce59 100644 --- a/futurex_openedx_extensions/helpers/constants.py +++ b/futurex_openedx_extensions/helpers/constants.py @@ -53,6 +53,7 @@ COURSE_STATUS_SELF_PREFIX = 'self_' +RATING_RANGE = range(1, 6) # Course ratings from 1 to 5 stars COURSE_CREATOR_ROLE_TENANT = 'org_course_creator_group' COURSE_CREATOR_ROLE_GLOBAL = 'course_creator_group' From 5d29236eac4994372f59347c9736d9530bbbdabd Mon Sep 17 00:00:00 2001 From: mkurkar Date: Sun, 30 Nov 2025 18:26:10 +0300 Subject: [PATCH 05/10] test: add caching tests for course ratings functionality --- .../test_statistics/test_courses.py | 224 +++++++++++++++++- 1 file changed, 223 insertions(+), 1 deletion(-) diff --git a/tests/test_dashboard/test_statistics/test_courses.py b/tests/test_dashboard/test_statistics/test_courses.py index e95d8ec4..83328b80 100644 --- a/tests/test_dashboard/test_statistics/test_courses.py +++ b/tests/test_dashboard/test_statistics/test_courses.py @@ -1,10 +1,13 @@ """Tests for courses statistics.""" -from datetime import datetime +from datetime import datetime, timedelta from unittest.mock import patch import pytest from common.djangoapps.student.models import CourseEnrollment +from django.conf import settings +from django.core.cache import cache from django.db.models import CharField, Value +from django.utils import timezone from eox_nelp.course_experience.models import FeedbackCourse from openedx.core.djangoapps.content.course_overviews.models import CourseOverview @@ -223,3 +226,222 @@ def _is_unusable_enrollment(_enrollment): date_from, date_to = date_to, date_from assert calculated_from == datetime.combine(date_from, datetime.min.time()) assert calculated_to == datetime.combine(date_to, datetime.max.time()) + + +@pytest.mark.django_db +def test_cache_key_courses_ratings(): + """Verify that cache key generation works correctly with different parameters.""" + key1 = courses._cache_key_courses_ratings(1, True, True) # pylint: disable=protected-access + key2 = courses._cache_key_courses_ratings(1, True, False) # pylint: disable=protected-access + key3 = courses._cache_key_courses_ratings(1, False, True) # pylint: disable=protected-access + key4 = courses._cache_key_courses_ratings(2, True, True) # pylint: disable=protected-access + key5 = courses._cache_key_courses_ratings(1, None, None) # pylint: disable=protected-access + keys = [key1, key2, key3, key4, key5] + assert len(keys) == len(set(keys)), 'Cache keys should be unique for different parameters' + + assert key1 == 'fx_courses_ratings_t1_vTrue_aTrue' + assert key2 == 'fx_courses_ratings_t1_vTrue_aFalse' + assert key3 == 'fx_courses_ratings_t1_vFalse_aTrue' + assert key4 == 'fx_courses_ratings_t2_vTrue_aTrue' + assert key5 == 'fx_courses_ratings_t1_vNone_aNone' + + +@pytest.mark.django_db +def test_get_courses_ratings_caching(base_data, cache_testing): # pylint: disable=unused-argument + """Verify that get_courses_ratings caches results correctly per tenant.""" + ratings = { + 'course-v1:ORG1+5+5': [3, 4, 5], + 'course-v1:ORG2+4+4': [1, 2, 3], + } + for course_id, rating in ratings.items(): + course = CourseOverview.objects.get(id=course_id) + for rate in rating: + FeedbackCourse.objects.create( + course_id=course, + rating_content=rate, + ) + + cache.clear() + expected_cache_key = courses._cache_key_courses_ratings(1, True, None) # pylint: disable=protected-access + + with patch.object(cache, 'set', wraps=cache.set) as mock_set: + result1 = courses.get_courses_ratings(tenant_id=1) + cache_keys_set = [mock_call[0][0] for mock_call in mock_set.call_args_list] + assert expected_cache_key in cache_keys_set, \ + f'Expected cache key {expected_cache_key} not found in {cache_keys_set}' + + with patch.object(cache, 'get', wraps=cache.get) as mock_get: + with patch.object(cache, 'set', wraps=cache.set) as mock_set: + result2 = courses.get_courses_ratings(tenant_id=1) + cache_keys_checked = [mock_call[0][0] for mock_call in mock_get.call_args_list] + assert expected_cache_key in cache_keys_checked + cache_keys_set = [mock_call[0][0] for mock_call in mock_set.call_args_list] + assert expected_cache_key not in cache_keys_set, 'Cache should not be set again for same parameters' + + assert result1 == result2 + + +@pytest.mark.django_db +def test_get_courses_ratings_cache_different_parameters(base_data, cache_testing): # pylint: disable=unused-argument + """Verify that different parameter combinations create separate cache entries.""" + ratings = { + 'course-v1:ORG1+5+5': [3, 4, 5], + } + for course_id, rating in ratings.items(): + course = CourseOverview.objects.get(id=course_id) + for rate in rating: + FeedbackCourse.objects.create( + course_id=course, + rating_content=rate, + ) + + cache.clear() + result1 = courses.get_courses_ratings(tenant_id=1, visible_filter=True, active_filter=None) + result2 = courses.get_courses_ratings(tenant_id=1, visible_filter=False, active_filter=None) + result3 = courses.get_courses_ratings(tenant_id=1, visible_filter=True, active_filter=True) + result4 = courses.get_courses_ratings(tenant_id=2, visible_filter=True, active_filter=None) + key1 = courses._cache_key_courses_ratings(1, True, None) # pylint: disable=protected-access + key2 = courses._cache_key_courses_ratings(1, False, None) # pylint: disable=protected-access + key3 = courses._cache_key_courses_ratings(1, True, True) # pylint: disable=protected-access + key4 = courses._cache_key_courses_ratings(2, True, None) # pylint: disable=protected-access + + assert cache.get(key1) is not None, 'Cache entry for combination 1 should exist' + assert cache.get(key2) is not None, 'Cache entry for combination 2 should exist' + assert cache.get(key3) is not None, 'Cache entry for combination 3 should exist' + assert cache.get(key4) is not None, 'Cache entry for combination 4 should exist' + assert cache.get(key1)['data'] == result1 + assert cache.get(key2)['data'] == result2 + assert cache.get(key3)['data'] == result3 + assert cache.get(key4)['data'] == result4 + + +@pytest.mark.django_db +def test_get_courses_ratings_cache_timeout(base_data, cache_testing): # pylint: disable=unused-argument + """Verify that the FX_CACHE_TIMEOUT_COURSES_RATINGS setting is used for cache timeout.""" + ratings = { + 'course-v1:ORG1+5+5': [3, 4], + } + for course_id, rating in ratings.items(): + course = CourseOverview.objects.get(id=course_id) + for rate in rating: + FeedbackCourse.objects.create( + course_id=course, + rating_content=rate, + ) + + cache.clear() + + expected_timeout = getattr(settings, 'FX_CACHE_TIMEOUT_COURSES_RATINGS', 900) + expected_cache_key = courses._cache_key_courses_ratings(1, True, None) # pylint: disable=protected-access + + with patch.object(cache, 'set', wraps=cache.set) as mock_set: + courses.get_courses_ratings(tenant_id=1) + courses_ratings_calls = [ + mock_call for mock_call in mock_set.call_args_list if mock_call[0][0] == expected_cache_key + ] + + assert len(courses_ratings_calls) > 0, \ + f'Cache key {expected_cache_key} was not set. Keys set: {[c[0][0] for c in mock_set.call_args_list]}' + call_args = courses_ratings_calls[0][0] + timeout = call_args[2] + + assert timeout == expected_timeout, f'Expected timeout {expected_timeout}, got {timeout}' + + +@pytest.mark.django_db +def test_get_courses_ratings_cache_per_tenant(base_data, cache_testing): # pylint: disable=unused-argument + """Verify that results are cached separately per tenant.""" + ratings_tenant1 = { + 'course-v1:ORG1+5+5': [5, 5, 5], + } + ratings_tenant2 = { + 'course-v1:ORG2+4+4': [1, 1, 1], + } + + for course_id, rating in {**ratings_tenant1, **ratings_tenant2}.items(): + course = CourseOverview.objects.get(id=course_id) + for rate in rating: + FeedbackCourse.objects.create( + course_id=course, + rating_content=rate, + ) + + cache.clear() + result_tenant1 = courses.get_courses_ratings(tenant_id=1) + result_tenant2 = courses.get_courses_ratings(tenant_id=2) + assert result_tenant1 != result_tenant2 + cache_key_1 = courses._cache_key_courses_ratings(1, True, None) # pylint: disable=protected-access + cache_key_2 = courses._cache_key_courses_ratings(2, True, None) # pylint: disable=protected-access + + cached_data_1 = cache.get(cache_key_1) + cached_data_2 = cache.get(cache_key_2) + + assert cached_data_1 is not None, 'Tenant 1 cache should exist' + assert cached_data_2 is not None, 'Tenant 2 cache should exist' + assert cached_data_1['data'] == result_tenant1 + assert cached_data_2['data'] == result_tenant2 + + +@pytest.mark.django_db +def test_get_courses_ratings_cache_expiry(base_data, cache_testing): # pylint: disable=unused-argument + """Verify that cached data includes proper expiry information.""" + ratings = { + 'course-v1:ORG1+5+5': [4, 5], + } + for course_id, rating in ratings.items(): + course = CourseOverview.objects.get(id=course_id) + for rate in rating: + FeedbackCourse.objects.create( + course_id=course, + rating_content=rate, + ) + + cache.clear() + + before_call = timezone.now() + courses.get_courses_ratings(tenant_id=1) + after_call = timezone.now() + + cache_key = courses._cache_key_courses_ratings(1, True, None) # pylint: disable=protected-access + cached_data = cache.get(cache_key) + + assert cached_data is not None + assert 'created_datetime' in cached_data + assert 'expiry_datetime' in cached_data + assert 'data' in cached_data + expected_timeout = getattr(settings, 'FX_CACHE_TIMEOUT_COURSES_RATINGS', 900) + assert before_call <= cached_data['created_datetime'] <= after_call + expected_min_expiry = before_call + timedelta(seconds=expected_timeout) + expected_max_expiry = after_call + timedelta(seconds=expected_timeout) + assert expected_min_expiry <= cached_data['expiry_datetime'] <= expected_max_expiry + + +@pytest.mark.django_db +def test_get_courses_ratings_subsequent_calls_use_cache(base_data, cache_testing): # pylint: disable=unused-argument + """Verify that subsequent calls with the same parameters return cached results without recomputation.""" + ratings = { + 'course-v1:ORG1+5+5': [3, 4, 5], + } + for course_id, rating in ratings.items(): + course = CourseOverview.objects.get(id=course_id) + for rate in rating: + FeedbackCourse.objects.create( + course_id=course, + rating_content=rate, + ) + + cache.clear() + original_get_base_queryset = courses.get_base_queryset_courses + + with patch('futurex_openedx_extensions.dashboard.statistics.courses.get_base_queryset_courses', + wraps=original_get_base_queryset) as mock_queryset: + result1 = courses.get_courses_ratings(tenant_id=1, visible_filter=True, active_filter=None) + first_call_count = mock_queryset.call_count + result2 = courses.get_courses_ratings(tenant_id=1, visible_filter=True, active_filter=None) + second_call_count = mock_queryset.call_count + result3 = courses.get_courses_ratings(tenant_id=1, visible_filter=True, active_filter=None) + third_call_count = mock_queryset.call_count + assert first_call_count == 1, 'First call should compute' + assert second_call_count == 1 + assert third_call_count == 1 + assert result1 == result2 == result3 From 02e4b0231de0e68438018f290643b7f0ca89793b Mon Sep 17 00:00:00 2001 From: mkurkar Date: Sun, 30 Nov 2025 19:05:09 +0300 Subject: [PATCH 06/10] fix: streamline tenant ID handling in GlobalRatingView and improve access control error responses --- futurex_openedx_extensions/dashboard/views.py | 29 +------ tests/test_dashboard/test_views.py | 79 +++---------------- 2 files changed, 14 insertions(+), 94 deletions(-) diff --git a/futurex_openedx_extensions/dashboard/views.py b/futurex_openedx_extensions/dashboard/views.py index 64b3960d..e37a4af5 100644 --- a/futurex_openedx_extensions/dashboard/views.py +++ b/futurex_openedx_extensions/dashboard/views.py @@ -1080,35 +1080,12 @@ def get(self, request: Any, *args: Any, **kwargs: Any) -> JsonResponse: (required): a single tenant ID to get the rating information for. Multiple tenant IDs are not supported - only one tenant ID must be provided. """ - tenant_ids_string = request.GET.get('tenant_ids') - if not tenant_ids_string: - raise FXCodedException( - code=FXExceptionCodes.TENANT_ID_REQUIRED_AS_URL_ARG, - message='tenant_ids parameter is required' - ) - - try: - tenant_ids = ids_string_to_list(tenant_ids_string) - except ValueError as exc: - raise FXCodedException( - code=FXExceptionCodes.TENANT_NOT_FOUND, - message=f'Invalid tenant_ids provided: {str(exc)}' - ) from exc - - if len(tenant_ids) != 1: - raise FXCodedException( - code=FXExceptionCodes.TENANT_ID_REQUIRED_AS_URL_ARG, - message=f'Exactly one tenant ID is required, got {len(tenant_ids)}' - ) + tenant_id = self.verify_one_tenant_id_provided(request) - tenant_id = tenant_ids[0] accessible_tenant_ids = self.fx_permission_info.get('view_allowed_tenant_ids_any_access', []) if tenant_id not in accessible_tenant_ids: - raise PermissionDenied( - detail=json.dumps({ - 'reason': f'User does not have access to tenant {tenant_id}' - }) - ) + error_detail = json.dumps({'reason': f'User does not have access to tenant {tenant_id}'}) + raise PermissionDenied(detail=error_detail) data_result = get_courses_ratings(tenant_id=tenant_id) rating_counts = {str(i): data_result[f'rating_{i}_count'] for i in RATING_RANGE} diff --git a/tests/test_dashboard/test_views.py b/tests/test_dashboard/test_views.py index 40c7173d..d44b39a3 100644 --- a/tests/test_dashboard/test_views.py +++ b/tests/test_dashboard/test_views.py @@ -6,7 +6,7 @@ import os from collections import OrderedDict from datetime import date -from unittest.mock import ANY, MagicMock, Mock, patch +from unittest.mock import ANY, MagicMock, Mock, PropertyMock, patch import ddt import pytest @@ -1831,77 +1831,20 @@ def test_success_no_rating(self): }) mocked_calc.assert_called_once_with(tenant_id=1) - def test_missing_tenant_ids(self): - """Verify that the view returns 400 when tenant_ids parameter is missing""" + def test_tenant_access_denied(self): + """Verify that the view returns 403 when user doesn't have access to the requested tenant""" self.login_user(self.staff_user) - response = self.client.get(self.url) - self.assertEqual(response.status_code, http_status.HTTP_400_BAD_REQUEST) - data = json.loads(response.content) - self.assertIn('reason', data) - self.assertIn('tenant_ids parameter is required', data['reason']) - - def test_multiple_tenant_ids(self): - """Verify that the view returns 400 when multiple tenant_ids are provided""" - self.login_user(self.staff_user) - response = self.client.get(f'{self.url}?tenant_ids=1,2') - self.assertEqual(response.status_code, http_status.HTTP_400_BAD_REQUEST) - data = json.loads(response.content) - self.assertIn('reason', data) - self.assertIn('Exactly one tenant ID is required', data['reason']) - - def test_invalid_tenant_id_format(self): - """Verify that the view returns 403 when tenant_ids has invalid format""" - self.login_user(self.staff_user) - response = self.client.get(f'{self.url}?tenant_ids=invalid') + with patch.object( + GlobalRatingView, + 'fx_permission_info', + new_callable=PropertyMock, + return_value={'view_allowed_tenant_ids_any_access': [1]} + ): + response = self.client.get(f'{self.url}?tenant_ids=2') self.assertEqual(response.status_code, http_status.HTTP_403_FORBIDDEN) data = json.loads(response.content) self.assertIn('reason', data) - - def test_unauthorized_tenant_access(self): - """Verify that the view returns 403 when user doesn't have access to the tenant""" - self.login_user(self.staff_user) - response = self.client.get(f'{self.url}?tenant_ids=999') - self.assertEqual(response.status_code, http_status.HTTP_403_FORBIDDEN) - - def test_direct_call_invalid_tenant_format(self): - """Test ValueError handler when ids_string_to_list raises ValueError (bypassing middleware)""" - self.login_user(self.staff_user) - factory = APIRequestFactory() - request = factory.get(f'{self.url}?tenant_ids=invalid_format') - request.user = self.staff_user - request.fx_permission_info = { - 'user': self.staff_user, - 'view_allowed_tenant_ids_any_access': [1, 2], - } - - view = GlobalRatingView() - view.request = request - - with patch('futurex_openedx_extensions.dashboard.views.ids_string_to_list') as mock_ids: - mock_ids.side_effect = ValueError('Invalid format') - with self.assertRaises(FXCodedException) as context: - view.get(request) - self.assertEqual(context.exception.code, FXExceptionCodes.TENANT_NOT_FOUND.value) - self.assertIn('Invalid tenant_ids provided', str(context.exception)) - - def test_direct_call_unauthorized_tenant(self): - """Test PermissionDenied when tenant_id not in accessible list (bypassing middleware)""" - self.login_user(self.staff_user) - factory = APIRequestFactory() - request = factory.get(f'{self.url}?tenant_ids=999') - request.user = self.staff_user - request.fx_permission_info = { - 'user': self.staff_user, - 'view_allowed_tenant_ids_any_access': [1, 2], - } - - view = GlobalRatingView() - view.request = request - - with self.assertRaises(PermissionDenied) as context: - view.get(request) - error_detail = json.loads(context.exception.detail) - self.assertIn('User does not have access to tenant 999', error_detail['reason']) + self.assertEqual(data['reason'], 'User does not have access to tenant 2') @ddt.ddt From 5b0bff5704e6d577de8dc67f9c353d704e3b052d Mon Sep 17 00:00:00 2001 From: mkurkar Date: Sun, 30 Nov 2025 19:16:16 +0300 Subject: [PATCH 07/10] fix: remove tenant access check from GlobalRatingView to simplify permission handling --- futurex_openedx_extensions/dashboard/views.py | 5 ----- tests/test_dashboard/test_views.py | 15 --------------- 2 files changed, 20 deletions(-) diff --git a/futurex_openedx_extensions/dashboard/views.py b/futurex_openedx_extensions/dashboard/views.py index e37a4af5..9a3e5731 100644 --- a/futurex_openedx_extensions/dashboard/views.py +++ b/futurex_openedx_extensions/dashboard/views.py @@ -1082,11 +1082,6 @@ def get(self, request: Any, *args: Any, **kwargs: Any) -> JsonResponse: """ tenant_id = self.verify_one_tenant_id_provided(request) - accessible_tenant_ids = self.fx_permission_info.get('view_allowed_tenant_ids_any_access', []) - if tenant_id not in accessible_tenant_ids: - error_detail = json.dumps({'reason': f'User does not have access to tenant {tenant_id}'}) - raise PermissionDenied(detail=error_detail) - data_result = get_courses_ratings(tenant_id=tenant_id) rating_counts = {str(i): data_result[f'rating_{i}_count'] for i in RATING_RANGE} total_count = sum(rating_counts.values()) diff --git a/tests/test_dashboard/test_views.py b/tests/test_dashboard/test_views.py index d44b39a3..1f034f6c 100644 --- a/tests/test_dashboard/test_views.py +++ b/tests/test_dashboard/test_views.py @@ -1831,21 +1831,6 @@ def test_success_no_rating(self): }) mocked_calc.assert_called_once_with(tenant_id=1) - def test_tenant_access_denied(self): - """Verify that the view returns 403 when user doesn't have access to the requested tenant""" - self.login_user(self.staff_user) - with patch.object( - GlobalRatingView, - 'fx_permission_info', - new_callable=PropertyMock, - return_value={'view_allowed_tenant_ids_any_access': [1]} - ): - response = self.client.get(f'{self.url}?tenant_ids=2') - self.assertEqual(response.status_code, http_status.HTTP_403_FORBIDDEN) - data = json.loads(response.content) - self.assertIn('reason', data) - self.assertEqual(data['reason'], 'User does not have access to tenant 2') - @ddt.ddt class TestMyRolesView(BaseTestViewMixin): From dfd4194ae313a6d70592783d04487f416ecee934 Mon Sep 17 00:00:00 2001 From: mkurkar Date: Sun, 30 Nov 2025 19:27:42 +0300 Subject: [PATCH 08/10] fix: refactor caching logic in get_courses_ratings and clean up imports in views --- .../dashboard/statistics/courses.py | 60 +++++++++---------- futurex_openedx_extensions/dashboard/views.py | 2 +- tests/test_dashboard/test_views.py | 5 +- 3 files changed, 32 insertions(+), 35 deletions(-) diff --git a/futurex_openedx_extensions/dashboard/statistics/courses.py b/futurex_openedx_extensions/dashboard/statistics/courses.py index bad1ee5e..eb077810 100644 --- a/futurex_openedx_extensions/dashboard/statistics/courses.py +++ b/futurex_openedx_extensions/dashboard/statistics/courses.py @@ -210,7 +210,11 @@ def get_courses_count_by_status( return q_set -def _cache_key_courses_ratings(tenant_id: int, visible_filter: bool | None, active_filter: bool | None) -> str: +def _cache_key_courses_ratings( + tenant_id: int, + visible_filter: bool | None = True, + active_filter: bool | None = None, +) -> str: """ Generate cache key for get_courses_ratings @@ -226,6 +230,10 @@ def _cache_key_courses_ratings(tenant_id: int, visible_filter: bool | None, acti return f'fx_courses_ratings_t{tenant_id}_v{visible_filter}_a{active_filter}' +@cache_dict( + timeout='FX_CACHE_TIMEOUT_COURSES_RATINGS', + key_generator_or_name=_cache_key_courses_ratings +) def get_courses_ratings( tenant_id: int, visible_filter: bool | None = True, @@ -243,35 +251,25 @@ def get_courses_ratings( :return: Dictionary containing the total rating, courses count, and rating count per rating value :rtype: Dict[str, int] """ - @cache_dict( - timeout='FX_CACHE_TIMEOUT_COURSES_RATINGS', - key_generator_or_name=_cache_key_courses_ratings + fx_permission_info = build_fx_permission_info(tenant_id) + q_set = get_base_queryset_courses( + fx_permission_info, visible_filter=visible_filter, active_filter=active_filter ) - def _get_ratings(t_id: int, v_filter: bool | None, a_filter: bool | None) -> Dict[str, int]: - """ - Inner function to compute ratings with caching - """ - fx_permission_info = build_fx_permission_info(t_id) - q_set = get_base_queryset_courses( - fx_permission_info, visible_filter=v_filter, active_filter=a_filter - ) - q_set = annotate_courses_rating_queryset(q_set).filter(rating_count__gt=0) - - q_set = q_set.annotate(**{ - f'course_rating_{rate_value}_count': Count( - 'feedbackcourse', - filter=Q(feedbackcourse__rating_content=rate_value) - ) for rate_value in RATING_RANGE - }) - - return q_set.aggregate( - total_rating=Coalesce(Sum('rating_total'), 0), - courses_count=Coalesce(Count('id'), 0), - **{ - f'rating_{rate_value}_count': Coalesce(Sum(f'course_rating_{rate_value}_count'), 0) - for rate_value in RATING_RANGE - } - ) - - return _get_ratings(tenant_id, visible_filter, active_filter) + q_set = annotate_courses_rating_queryset(q_set).filter(rating_count__gt=0) + + q_set = q_set.annotate(**{ + f'course_rating_{rate_value}_count': Count( + 'feedbackcourse', + filter=Q(feedbackcourse__rating_content=rate_value) + ) for rate_value in RATING_RANGE + }) + + return q_set.aggregate( + total_rating=Coalesce(Sum('rating_total'), 0), + courses_count=Coalesce(Count('id'), 0), + **{ + f'rating_{rate_value}_count': Coalesce(Sum(f'course_rating_{rate_value}_count'), 0) + for rate_value in RATING_RANGE + } + ) diff --git a/futurex_openedx_extensions/dashboard/views.py b/futurex_openedx_extensions/dashboard/views.py index 9a3e5731..a2a6873e 100644 --- a/futurex_openedx_extensions/dashboard/views.py +++ b/futurex_openedx_extensions/dashboard/views.py @@ -74,7 +74,7 @@ FX_VIEW_DEFAULT_AUTH_CLASSES, RATING_RANGE, ) -from futurex_openedx_extensions.helpers.converters import dict_to_hash, error_details_to_dictionary, ids_string_to_list +from futurex_openedx_extensions.helpers.converters import dict_to_hash, error_details_to_dictionary from futurex_openedx_extensions.helpers.course_categories import CourseCategories from futurex_openedx_extensions.helpers.exceptions import FXCodedException, FXExceptionCodes from futurex_openedx_extensions.helpers.export_mixins import ExportCSVMixin diff --git a/tests/test_dashboard/test_views.py b/tests/test_dashboard/test_views.py index 1f034f6c..27019baf 100644 --- a/tests/test_dashboard/test_views.py +++ b/tests/test_dashboard/test_views.py @@ -6,7 +6,7 @@ import os from collections import OrderedDict from datetime import date -from unittest.mock import ANY, MagicMock, Mock, PropertyMock, patch +from unittest.mock import ANY, MagicMock, Mock, patch import ddt import pytest @@ -28,7 +28,7 @@ from opaque_keys.edx.locator import CourseLocator, LibraryLocator from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from rest_framework import status as http_status -from rest_framework.exceptions import ParseError, PermissionDenied +from rest_framework.exceptions import ParseError from rest_framework.response import Response from rest_framework.status import HTTP_400_BAD_REQUEST from rest_framework.test import APIRequestFactory, APITestCase @@ -36,7 +36,6 @@ from futurex_openedx_extensions.dashboard import serializers, urls, views from futurex_openedx_extensions.dashboard.views import ( - GlobalRatingView, LearnersEnrollmentView, ThemeConfigDraftView, ThemeConfigPublishView, From 965e6339e55c99164ba80641be69836606c9b957 Mon Sep 17 00:00:00 2001 From: mkurkar Date: Mon, 1 Dec 2025 14:26:50 +0300 Subject: [PATCH 09/10] fix: remove unused imports and redundant caching tests in test_courses.py --- .../test_statistics/test_courses.py | 206 +----------------- 1 file changed, 1 insertion(+), 205 deletions(-) diff --git a/tests/test_dashboard/test_statistics/test_courses.py b/tests/test_dashboard/test_statistics/test_courses.py index 83328b80..0e305695 100644 --- a/tests/test_dashboard/test_statistics/test_courses.py +++ b/tests/test_dashboard/test_statistics/test_courses.py @@ -1,13 +1,10 @@ """Tests for courses statistics.""" -from datetime import datetime, timedelta +from datetime import datetime from unittest.mock import patch import pytest from common.djangoapps.student.models import CourseEnrollment -from django.conf import settings -from django.core.cache import cache from django.db.models import CharField, Value -from django.utils import timezone from eox_nelp.course_experience.models import FeedbackCourse from openedx.core.djangoapps.content.course_overviews.models import CourseOverview @@ -244,204 +241,3 @@ def test_cache_key_courses_ratings(): assert key3 == 'fx_courses_ratings_t1_vFalse_aTrue' assert key4 == 'fx_courses_ratings_t2_vTrue_aTrue' assert key5 == 'fx_courses_ratings_t1_vNone_aNone' - - -@pytest.mark.django_db -def test_get_courses_ratings_caching(base_data, cache_testing): # pylint: disable=unused-argument - """Verify that get_courses_ratings caches results correctly per tenant.""" - ratings = { - 'course-v1:ORG1+5+5': [3, 4, 5], - 'course-v1:ORG2+4+4': [1, 2, 3], - } - for course_id, rating in ratings.items(): - course = CourseOverview.objects.get(id=course_id) - for rate in rating: - FeedbackCourse.objects.create( - course_id=course, - rating_content=rate, - ) - - cache.clear() - expected_cache_key = courses._cache_key_courses_ratings(1, True, None) # pylint: disable=protected-access - - with patch.object(cache, 'set', wraps=cache.set) as mock_set: - result1 = courses.get_courses_ratings(tenant_id=1) - cache_keys_set = [mock_call[0][0] for mock_call in mock_set.call_args_list] - assert expected_cache_key in cache_keys_set, \ - f'Expected cache key {expected_cache_key} not found in {cache_keys_set}' - - with patch.object(cache, 'get', wraps=cache.get) as mock_get: - with patch.object(cache, 'set', wraps=cache.set) as mock_set: - result2 = courses.get_courses_ratings(tenant_id=1) - cache_keys_checked = [mock_call[0][0] for mock_call in mock_get.call_args_list] - assert expected_cache_key in cache_keys_checked - cache_keys_set = [mock_call[0][0] for mock_call in mock_set.call_args_list] - assert expected_cache_key not in cache_keys_set, 'Cache should not be set again for same parameters' - - assert result1 == result2 - - -@pytest.mark.django_db -def test_get_courses_ratings_cache_different_parameters(base_data, cache_testing): # pylint: disable=unused-argument - """Verify that different parameter combinations create separate cache entries.""" - ratings = { - 'course-v1:ORG1+5+5': [3, 4, 5], - } - for course_id, rating in ratings.items(): - course = CourseOverview.objects.get(id=course_id) - for rate in rating: - FeedbackCourse.objects.create( - course_id=course, - rating_content=rate, - ) - - cache.clear() - result1 = courses.get_courses_ratings(tenant_id=1, visible_filter=True, active_filter=None) - result2 = courses.get_courses_ratings(tenant_id=1, visible_filter=False, active_filter=None) - result3 = courses.get_courses_ratings(tenant_id=1, visible_filter=True, active_filter=True) - result4 = courses.get_courses_ratings(tenant_id=2, visible_filter=True, active_filter=None) - key1 = courses._cache_key_courses_ratings(1, True, None) # pylint: disable=protected-access - key2 = courses._cache_key_courses_ratings(1, False, None) # pylint: disable=protected-access - key3 = courses._cache_key_courses_ratings(1, True, True) # pylint: disable=protected-access - key4 = courses._cache_key_courses_ratings(2, True, None) # pylint: disable=protected-access - - assert cache.get(key1) is not None, 'Cache entry for combination 1 should exist' - assert cache.get(key2) is not None, 'Cache entry for combination 2 should exist' - assert cache.get(key3) is not None, 'Cache entry for combination 3 should exist' - assert cache.get(key4) is not None, 'Cache entry for combination 4 should exist' - assert cache.get(key1)['data'] == result1 - assert cache.get(key2)['data'] == result2 - assert cache.get(key3)['data'] == result3 - assert cache.get(key4)['data'] == result4 - - -@pytest.mark.django_db -def test_get_courses_ratings_cache_timeout(base_data, cache_testing): # pylint: disable=unused-argument - """Verify that the FX_CACHE_TIMEOUT_COURSES_RATINGS setting is used for cache timeout.""" - ratings = { - 'course-v1:ORG1+5+5': [3, 4], - } - for course_id, rating in ratings.items(): - course = CourseOverview.objects.get(id=course_id) - for rate in rating: - FeedbackCourse.objects.create( - course_id=course, - rating_content=rate, - ) - - cache.clear() - - expected_timeout = getattr(settings, 'FX_CACHE_TIMEOUT_COURSES_RATINGS', 900) - expected_cache_key = courses._cache_key_courses_ratings(1, True, None) # pylint: disable=protected-access - - with patch.object(cache, 'set', wraps=cache.set) as mock_set: - courses.get_courses_ratings(tenant_id=1) - courses_ratings_calls = [ - mock_call for mock_call in mock_set.call_args_list if mock_call[0][0] == expected_cache_key - ] - - assert len(courses_ratings_calls) > 0, \ - f'Cache key {expected_cache_key} was not set. Keys set: {[c[0][0] for c in mock_set.call_args_list]}' - call_args = courses_ratings_calls[0][0] - timeout = call_args[2] - - assert timeout == expected_timeout, f'Expected timeout {expected_timeout}, got {timeout}' - - -@pytest.mark.django_db -def test_get_courses_ratings_cache_per_tenant(base_data, cache_testing): # pylint: disable=unused-argument - """Verify that results are cached separately per tenant.""" - ratings_tenant1 = { - 'course-v1:ORG1+5+5': [5, 5, 5], - } - ratings_tenant2 = { - 'course-v1:ORG2+4+4': [1, 1, 1], - } - - for course_id, rating in {**ratings_tenant1, **ratings_tenant2}.items(): - course = CourseOverview.objects.get(id=course_id) - for rate in rating: - FeedbackCourse.objects.create( - course_id=course, - rating_content=rate, - ) - - cache.clear() - result_tenant1 = courses.get_courses_ratings(tenant_id=1) - result_tenant2 = courses.get_courses_ratings(tenant_id=2) - assert result_tenant1 != result_tenant2 - cache_key_1 = courses._cache_key_courses_ratings(1, True, None) # pylint: disable=protected-access - cache_key_2 = courses._cache_key_courses_ratings(2, True, None) # pylint: disable=protected-access - - cached_data_1 = cache.get(cache_key_1) - cached_data_2 = cache.get(cache_key_2) - - assert cached_data_1 is not None, 'Tenant 1 cache should exist' - assert cached_data_2 is not None, 'Tenant 2 cache should exist' - assert cached_data_1['data'] == result_tenant1 - assert cached_data_2['data'] == result_tenant2 - - -@pytest.mark.django_db -def test_get_courses_ratings_cache_expiry(base_data, cache_testing): # pylint: disable=unused-argument - """Verify that cached data includes proper expiry information.""" - ratings = { - 'course-v1:ORG1+5+5': [4, 5], - } - for course_id, rating in ratings.items(): - course = CourseOverview.objects.get(id=course_id) - for rate in rating: - FeedbackCourse.objects.create( - course_id=course, - rating_content=rate, - ) - - cache.clear() - - before_call = timezone.now() - courses.get_courses_ratings(tenant_id=1) - after_call = timezone.now() - - cache_key = courses._cache_key_courses_ratings(1, True, None) # pylint: disable=protected-access - cached_data = cache.get(cache_key) - - assert cached_data is not None - assert 'created_datetime' in cached_data - assert 'expiry_datetime' in cached_data - assert 'data' in cached_data - expected_timeout = getattr(settings, 'FX_CACHE_TIMEOUT_COURSES_RATINGS', 900) - assert before_call <= cached_data['created_datetime'] <= after_call - expected_min_expiry = before_call + timedelta(seconds=expected_timeout) - expected_max_expiry = after_call + timedelta(seconds=expected_timeout) - assert expected_min_expiry <= cached_data['expiry_datetime'] <= expected_max_expiry - - -@pytest.mark.django_db -def test_get_courses_ratings_subsequent_calls_use_cache(base_data, cache_testing): # pylint: disable=unused-argument - """Verify that subsequent calls with the same parameters return cached results without recomputation.""" - ratings = { - 'course-v1:ORG1+5+5': [3, 4, 5], - } - for course_id, rating in ratings.items(): - course = CourseOverview.objects.get(id=course_id) - for rate in rating: - FeedbackCourse.objects.create( - course_id=course, - rating_content=rate, - ) - - cache.clear() - original_get_base_queryset = courses.get_base_queryset_courses - - with patch('futurex_openedx_extensions.dashboard.statistics.courses.get_base_queryset_courses', - wraps=original_get_base_queryset) as mock_queryset: - result1 = courses.get_courses_ratings(tenant_id=1, visible_filter=True, active_filter=None) - first_call_count = mock_queryset.call_count - result2 = courses.get_courses_ratings(tenant_id=1, visible_filter=True, active_filter=None) - second_call_count = mock_queryset.call_count - result3 = courses.get_courses_ratings(tenant_id=1, visible_filter=True, active_filter=None) - third_call_count = mock_queryset.call_count - assert first_call_count == 1, 'First call should compute' - assert second_call_count == 1 - assert third_call_count == 1 - assert result1 == result2 == result3 From bf9230068a9e4527afd519bb2646ebf8ed292d01 Mon Sep 17 00:00:00 2001 From: shadinaif Date: Tue, 2 Dec 2025 09:24:18 +0300 Subject: [PATCH 10/10] chore: minior quality fixes --- .../dashboard/settings/common_production.py | 2 +- .../dashboard/statistics/courses.py | 7 ++++--- futurex_openedx_extensions/helpers/constants.py | 1 + test_utils/test_settings_common.py | 3 ++- tests/test_dashboard/test_apps.py | 1 + tests/test_dashboard/test_statistics/test_courses.py | 12 ++++++------ 6 files changed, 15 insertions(+), 11 deletions(-) diff --git a/futurex_openedx_extensions/dashboard/settings/common_production.py b/futurex_openedx_extensions/dashboard/settings/common_production.py index d404cc62..fe20637f 100644 --- a/futurex_openedx_extensions/dashboard/settings/common_production.py +++ b/futurex_openedx_extensions/dashboard/settings/common_production.py @@ -17,7 +17,7 @@ def plugin_settings(settings: Any) -> None: settings.FX_CACHE_TIMEOUT_COURSES_RATINGS = getattr( settings, 'FX_CACHE_TIMEOUT_COURSES_RATINGS', - 60 * 15, # 15 minutes + 60 * 60, # 1 hour ) settings.FX_DISABLE_CONFIG_VALIDATIONS = getattr( diff --git a/futurex_openedx_extensions/dashboard/statistics/courses.py b/futurex_openedx_extensions/dashboard/statistics/courses.py index eb077810..689db260 100644 --- a/futurex_openedx_extensions/dashboard/statistics/courses.py +++ b/futurex_openedx_extensions/dashboard/statistics/courses.py @@ -11,6 +11,7 @@ from django.utils.timezone import now from futurex_openedx_extensions.dashboard.details.courses import annotate_courses_rating_queryset +from futurex_openedx_extensions.helpers import constants as cs from futurex_openedx_extensions.helpers.caching import cache_dict from futurex_openedx_extensions.helpers.constants import COURSE_STATUSES, RATING_RANGE from futurex_openedx_extensions.helpers.extractors import get_valid_duration @@ -210,7 +211,7 @@ def get_courses_count_by_status( return q_set -def _cache_key_courses_ratings( +def cache_name_courses_rating( tenant_id: int, visible_filter: bool | None = True, active_filter: bool | None = None, @@ -227,12 +228,12 @@ def _cache_key_courses_ratings( :return: Cache key string :rtype: str """ - return f'fx_courses_ratings_t{tenant_id}_v{visible_filter}_a{active_filter}' + return f'{cs.CACHE_NAME_COURSES_RATINGS}_t{tenant_id}_v{visible_filter}_a{active_filter}' @cache_dict( timeout='FX_CACHE_TIMEOUT_COURSES_RATINGS', - key_generator_or_name=_cache_key_courses_ratings + key_generator_or_name=cache_name_courses_rating ) def get_courses_ratings( tenant_id: int, diff --git a/futurex_openedx_extensions/helpers/constants.py b/futurex_openedx_extensions/helpers/constants.py index a304ce59..35620013 100644 --- a/futurex_openedx_extensions/helpers/constants.py +++ b/futurex_openedx_extensions/helpers/constants.py @@ -10,6 +10,7 @@ CACHE_NAME_LIVE_STATISTICS_PER_TENANT = 'fx_live_statistics_per_tenant' CACHE_NAME_CONFIG_ACCESS_CONTROL = 'fx_config_access_control' CACHE_NAME_TENANT_READABLE_LMS_CONFIG = 'fx_config_tenant_lms_config' +CACHE_NAME_COURSES_RATINGS = 'fx_courses_ratings' CACHE_NAMES = { CACHE_NAME_ALL_COURSE_ORG_FILTER_LIST: { diff --git a/test_utils/test_settings_common.py b/test_utils/test_settings_common.py index db262531..befe3a53 100644 --- a/test_utils/test_settings_common.py +++ b/test_utils/test_settings_common.py @@ -92,7 +92,8 @@ def root(*args): FX_CACHE_TIMEOUT_VIEW_ROLES = 60 * 31 # 31 minutes FX_CACHE_TIMEOUT_LIVE_STATISTICS_PER_TENANT = 60 * 60 * 3 # three hours FX_CACHE_TIMEOUT_CONFIG_ACCESS_CONTROL = 60 * 60 * 48 # 2 days -FX_CACHE_TIMEOUT_COURSES_RATINGS = 60 * 15 # 15 minutes +FX_CACHE_TIMEOUT_COURSES_RATINGS = 60 * 2 # 2 hours + FX_TASK_MINUTES_LIMIT = 6 # 6 minutes FX_MAX_PERIOD_CHUNKS_MAP = { 'day': 365 * 2, diff --git a/tests/test_dashboard/test_apps.py b/tests/test_dashboard/test_apps.py index 88364abd..14463876 100644 --- a/tests/test_dashboard/test_apps.py +++ b/tests/test_dashboard/test_apps.py @@ -10,6 +10,7 @@ helpers_default_settings = [ ('FX_CACHE_TIMEOUT_LIVE_STATISTICS_PER_TENANT', 60 * 60 * 2), # 2 hours ('FX_ALLOWED_COURSE_LANGUAGE_CODES', ['en', 'ar', 'fr']), + ('FX_CACHE_TIMEOUT_COURSES_RATINGS', 60 * 60), # 1 hour ] diff --git a/tests/test_dashboard/test_statistics/test_courses.py b/tests/test_dashboard/test_statistics/test_courses.py index 0e305695..69d7a41d 100644 --- a/tests/test_dashboard/test_statistics/test_courses.py +++ b/tests/test_dashboard/test_statistics/test_courses.py @@ -226,13 +226,13 @@ def _is_unusable_enrollment(_enrollment): @pytest.mark.django_db -def test_cache_key_courses_ratings(): +def testcache_name_courses_rating(): """Verify that cache key generation works correctly with different parameters.""" - key1 = courses._cache_key_courses_ratings(1, True, True) # pylint: disable=protected-access - key2 = courses._cache_key_courses_ratings(1, True, False) # pylint: disable=protected-access - key3 = courses._cache_key_courses_ratings(1, False, True) # pylint: disable=protected-access - key4 = courses._cache_key_courses_ratings(2, True, True) # pylint: disable=protected-access - key5 = courses._cache_key_courses_ratings(1, None, None) # pylint: disable=protected-access + key1 = courses.cache_name_courses_rating(1, True, True) + key2 = courses.cache_name_courses_rating(1, True, False) + key3 = courses.cache_name_courses_rating(1, False, True) + key4 = courses.cache_name_courses_rating(2, True, True) + key5 = courses.cache_name_courses_rating(1, None, None) keys = [key1, key2, key3, key4, key5] assert len(keys) == len(set(keys)), 'Cache keys should be unique for different parameters'