fix: rating statistics very slow

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]
         ),
     },
 

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 * 60,  # 1 hour
+    )
+
     settings.FX_DISABLE_CONFIG_VALIDATIONS = getattr(
         settings,
         'FX_DISABLE_CONFIG_VALIDATIONS',

futurex_openedx_extensions/dashboard/statistics/courses.py

@@ -11,8 +11,11 @@
 from django.utils.timezone import now
 
 from futurex_openedx_extensions.dashboard.details.courses import annotate_courses_rating_queryset
-from futurex_openedx_extensions.helpers.constants import COURSE_STATUSES
+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
+from futurex_openedx_extensions.helpers.permissions import build_fx_permission_info
 from futurex_openedx_extensions.helpers.querysets import (
     annotate_period,
     check_staff_exist_queryset,
@@ -208,23 +211,48 @@ def get_courses_count_by_status(
     return q_set
 
 
+def cache_name_courses_rating(
+    tenant_id: int,
+    visible_filter: bool | None = True,
+    active_filter: bool | None = 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'{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_name_courses_rating
+)
 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]
     """
+    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
     )
@@ -235,14 +263,14 @@ def get_courses_ratings(
         f'course_rating_{rate_value}_count': Count(
             'feedbackcourse',
             filter=Q(feedbackcourse__rating_content=rate_value)
-        ) for rate_value in range(1, 6)
+        ) 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 range(1, 6)
+            for rate_value in RATING_RANGE
         }
     )

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
 from futurex_openedx_extensions.helpers.course_categories import CourseCategories
@@ -114,6 +115,7 @@
 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()
 logger = logging.getLogger(__name__)
 
@@ -1064,28 +1066,31 @@ 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_id = self.verify_one_tenant_id_provided(request)
+
+        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)

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

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: {
@@ -53,6 +54,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'

test_utils/test_settings_common.py

@@ -92,6 +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 * 2  # 2 hours
+
 FX_TASK_MINUTES_LIMIT = 6  # 6 minutes
 FX_MAX_PERIOD_CHUNKS_MAP = {
     'day': 365 * 2,

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
 ]
 
 

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)
@@ -223,3 +223,21 @@ 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 testcache_name_courses_rating():
+    """Verify that cache key generation works correctly with different parameters."""
+    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'
+
+    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'

tests/test_dashboard/test_views.py

@@ -1748,6 +1748,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 +1794,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 +1813,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 +1828,7 @@ def test_success_no_rating(self):
                 '5': 0,
             },
         })
+        mocked_calc.assert_called_once_with(tenant_id=1)
 
 
 @ddt.ddt