fix: rating statistics very slow

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',

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,41 +212,68 @@ 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)
     :type active_filter: bool | None
     :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)
+
+        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)

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=<tenantIds>
+        GET /api/fx/statistics/v1/rating/?tenant_ids=<tenantId>
 
-        <tenantIds> (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
+        <tenantId> (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)

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,

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)

tests/test_dashboard/test_views.py

@@ -28,14 +28,15 @@
 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
 from rest_framework.utils.serializer_helpers import ReturnList
 
 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