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/dashboard/settings/common_production.py b/futurex_openedx_extensions/dashboard/settings/common_production.py index f4c26388..fe20637f 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 * 60, # 1 hour + ) + 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..689db260 100644 --- a/futurex_openedx_extensions/dashboard/statistics/courses.py +++ b/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,16 +211,40 @@ 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) @@ -225,6 +252,7 @@ def get_courses_ratings( :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,7 +263,7 @@ 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( @@ -243,6 +271,6 @@ def get_courses_ratings( 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 } ) diff --git a/futurex_openedx_extensions/dashboard/views.py b/futurex_openedx_extensions/dashboard/views.py index 42fa7843..a2a6873e 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 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= + 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_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) 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 diff --git a/futurex_openedx_extensions/helpers/constants.py b/futurex_openedx_extensions/helpers/constants.py index 7dba5857..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: { @@ -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' diff --git a/test_utils/test_settings_common.py b/test_utils/test_settings_common.py index 8b214fe5..befe3a53 100644 --- a/test_utils/test_settings_common.py +++ b/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, 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 f428a195..69d7a41d 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) @@ -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' diff --git a/tests/test_dashboard/test_views.py b/tests/test_dashboard/test_views.py index d4670470..27019baf 100644 --- a/tests/test_dashboard/test_views.py +++ b/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