From 6d56c74148b7dcdc83b9b34e28abe91e729576e0 Mon Sep 17 00:00:00 2001 From: Tehreem Sadat Date: Fri, 20 Dec 2024 22:17:46 +1300 Subject: [PATCH 1/2] feat: add learning hours count to the stats api --- .../dashboard/statistics/certificates.py | 59 +++++++++++++++++++ futurex_openedx_extensions/dashboard/views.py | 22 ++++++- .../helpers/settings/common_production.py | 7 +++ .../fake_models/models.py | 1 + test_utils/test_settings_common.py | 2 + tests/base_test_data.py | 9 +++ .../test_statistics/test_certificates.py | 44 ++++++++++++++ tests/test_dashboard/test_views.py | 4 +- 8 files changed, 145 insertions(+), 3 deletions(-) diff --git a/futurex_openedx_extensions/dashboard/statistics/certificates.py b/futurex_openedx_extensions/dashboard/statistics/certificates.py index 639b3c23..540353ac 100644 --- a/futurex_openedx_extensions/dashboard/statistics/certificates.py +++ b/futurex_openedx_extensions/dashboard/statistics/certificates.py @@ -3,6 +3,7 @@ from typing import Dict +from django.conf import settings from django.db.models import Count, OuterRef, Subquery from django.db.models.functions import Lower from lms.djangoapps.certificates.models import GeneratedCertificate @@ -41,3 +42,61 @@ def get_certificates_count( )).values('course_org').annotate(certificates_count=Count('id')).values_list('course_org', 'certificates_count')) return dict(result) + + +def get_learning_hours_count( + fx_permission_info: dict, visible_courses_filter: bool | None = True, active_courses_filter: bool | None = None +) -> int: + """ + Get the count of learning hours in the given tenants. The count is grouped by course_effort. Certificates + for admins, staff, and superusers are also included. + + :param fx_permission_info: Dictionary containing permission information + :type fx_permission_info: dict + :param visible_courses_filter: Value to filter courses on catalog visibility. None means no filter. + :type visible_courses_filter: bool | None + :param active_courses_filter: Value to filter courses on active status. None means no filter. + :type active_courses_filter: bool | None + :return: Count of certificates per organization + :rtype: Dict[str, int] + """ + + def parse_course_effort(effort: str) -> float: + """Parses course effort in HH:MM format and returns total hours as a float.""" + try: + parts = effort.split(':') + hours = int(parts[0]) + minutes = int(parts[1]) if len(parts) > 1 else 0 + + if minutes >= 60: + raise ValueError('Minutes cannot be 60 or more.') + + total_hours = hours + minutes / 60 + return round(total_hours, 1) + except (ValueError, IndexError): + return settings.FX_DEFAULT_COURSE_EFFORT + + result = list( + GeneratedCertificate.objects.filter( + status='downloadable', + course_id__in=get_base_queryset_courses( + fx_permission_info, + visible_filter=visible_courses_filter, + active_filter=active_courses_filter, + ), + ).annotate( + course_effort=Subquery( + CourseOverview.objects.filter( + id=OuterRef('course_id') + ).values('effort') + ) + ).annotate( + certificates_count=Count('id') + ).values('course_effort', 'certificates_count') + ) + + return sum( + parse_course_effort( + entry.get('course_effort', settings.FX_DEFAULT_COURSE_EFFORT)) * entry.get('certificates_count', 0) + for entry in result + ) diff --git a/futurex_openedx_extensions/dashboard/views.py b/futurex_openedx_extensions/dashboard/views.py index 9cff4929..0bc5e664 100644 --- a/futurex_openedx_extensions/dashboard/views.py +++ b/futurex_openedx_extensions/dashboard/views.py @@ -27,7 +27,10 @@ get_learners_enrollments_queryset, get_learners_queryset, ) -from futurex_openedx_extensions.dashboard.statistics.certificates import get_certificates_count +from futurex_openedx_extensions.dashboard.statistics.certificates import ( + get_certificates_count, + get_learning_hours_count, +) from futurex_openedx_extensions.dashboard.statistics.courses import ( get_courses_count, get_courses_count_by_status, @@ -84,14 +87,19 @@ class TotalCountsView(FXViewRoleInfoMixin, APIView): STAT_ENROLLMENTS = 'enrollments' STAT_HIDDEN_COURSES = 'hidden_courses' STAT_LEARNERS = 'learners' + STAT_LEARNING_HOURS = 'learning_hours' + + valid_stats = [ + STAT_CERTIFICATES, STAT_COURSES, STAT_ENROLLMENTS, STAT_HIDDEN_COURSES, STAT_LEARNERS, STAT_LEARNING_HOURS + ] - valid_stats = [STAT_CERTIFICATES, STAT_COURSES, STAT_ENROLLMENTS, STAT_HIDDEN_COURSES, STAT_LEARNERS] STAT_RESULT_KEYS = { STAT_CERTIFICATES: 'certificates_count', STAT_COURSES: 'courses_count', STAT_ENROLLMENTS: 'enrollments_count', STAT_HIDDEN_COURSES: 'hidden_courses_count', STAT_LEARNERS: 'learners_count', + STAT_LEARNING_HOURS: 'learning_hours_count', } authentication_classes = default_auth_classes @@ -127,6 +135,11 @@ def _get_learners_count_data(one_tenant_permission_info: dict, include_staff: bo """Get the count of learners for the given tenant""" return get_learners_count(one_tenant_permission_info, include_staff=include_staff) + @staticmethod + def _get_learning_hours_count_data(one_tenant_permission_info: dict) -> int: + """Get the count of learning_hours for the given tenant""" + return get_learning_hours_count(one_tenant_permission_info) + def _get_stat_count(self, stat: str, tenant_id: int, include_staff: bool) -> int: """Get the count of the given stat for the given tenant""" one_tenant_permission_info = get_tenant_limited_fx_permission_info(self.fx_permission_info, tenant_id) @@ -144,6 +157,11 @@ def _get_stat_count(self, stat: str, tenant_id: int, include_staff: bool) -> int if stat == self.STAT_HIDDEN_COURSES: return self._get_courses_count_data(one_tenant_permission_info, visible_filter=False) + if stat == self.STAT_LEARNING_HOURS: + return self._get_learning_hours_count_data( + one_tenant_permission_info, + ) + return self._get_learners_count_data(one_tenant_permission_info, include_staff) def get(self, request: Any, *args: Any, **kwargs: Any) -> Response | JsonResponse: diff --git a/futurex_openedx_extensions/helpers/settings/common_production.py b/futurex_openedx_extensions/helpers/settings/common_production.py index cf117e5d..e507b524 100644 --- a/futurex_openedx_extensions/helpers/settings/common_production.py +++ b/futurex_openedx_extensions/helpers/settings/common_production.py @@ -32,3 +32,10 @@ def plugin_settings(settings: Any) -> None: 'FX_DASHBOARD_STORAGE_DIR', 'fx_dashboard' ) + + # Default Course EFfort + settings.FX_DEFAULT_COURSE_EFFORT = getattr( + settings, + 'FX_DEFAULT_COURSE_EFFORT', + '50' + ) diff --git a/test_utils/edx_platform_mocks_shared/fake_models/models.py b/test_utils/edx_platform_mocks_shared/fake_models/models.py index 3c605dca..f4e41117 100644 --- a/test_utils/edx_platform_mocks_shared/fake_models/models.py +++ b/test_utils/edx_platform_mocks_shared/fake_models/models.py @@ -32,6 +32,7 @@ class CourseOverview(models.Model): self_paced = models.BooleanField(default=False) course_image_url = models.TextField() visible_to_staff_only = models.BooleanField(default=False) + effort = models.CharField(max_length=255) class Meta: app_label = 'fake_models' diff --git a/test_utils/test_settings_common.py b/test_utils/test_settings_common.py index ef621dea..18568cf3 100644 --- a/test_utils/test_settings_common.py +++ b/test_utils/test_settings_common.py @@ -100,3 +100,5 @@ def root(*args): NELC_DASHBOARD_BASE = 'dashboard.example.com' FX_DASHBOARD_STORAGE_DIR = 'test_dir' + +FX_DEFAULT_COURSE_EFFORT = 20 diff --git a/tests/base_test_data.py b/tests/base_test_data.py index 6f0f1bb2..b44690f7 100644 --- a/tests/base_test_data.py +++ b/tests/base_test_data.py @@ -316,6 +316,7 @@ 'enrollments_count': 26, 'hidden_courses_count': 0, 'learners_count': 16, + 'learning_hours_count': 14 * 20, }, '2': { 'certificates_count': 9, @@ -323,6 +324,7 @@ 'enrollments_count': 21, 'hidden_courses_count': 0, 'learners_count': 21, + 'learning_hours_count': 9 * 20, }, '3': { 'certificates_count': 0, @@ -330,6 +332,8 @@ 'enrollments_count': 4, 'hidden_courses_count': 0, 'learners_count': 6, + 'learning_hours_count': 0, + }, '7': { 'certificates_count': 7, @@ -337,6 +341,8 @@ 'enrollments_count': 14, 'hidden_courses_count': 0, 'learners_count': 17, + 'learning_hours_count': 7 * 20, + }, '8': { 'certificates_count': 2, @@ -344,11 +350,14 @@ 'enrollments_count': 7, 'hidden_courses_count': 0, 'learners_count': 9, + 'learning_hours_count': 2 * 20 + }, 'total_certificates_count': 32, 'total_courses_count': 23, 'total_enrollments_count': 72, 'total_hidden_courses_count': 0, 'total_learners_count': 69, + 'total_learning_hours_count': 640, 'limited_access': False, } diff --git a/tests/test_dashboard/test_statistics/test_certificates.py b/tests/test_dashboard/test_statistics/test_certificates.py index c5999cee..1291ecbd 100644 --- a/tests/test_dashboard/test_statistics/test_certificates.py +++ b/tests/test_dashboard/test_statistics/test_certificates.py @@ -1,6 +1,8 @@ """Tests for certificates statistics.""" import pytest +from django.test import override_settings from lms.djangoapps.certificates.models import GeneratedCertificate +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from futurex_openedx_extensions.dashboard.statistics import certificates from tests.fixture_helpers import get_tenants_orgs @@ -44,3 +46,45 @@ def test_get_certificates_count_not_downloadable(base_data, fx_permission_info): ).update(status=some_status_not_downloadable) result = certificates.get_certificates_count(fx_permission_info) assert result == {'org1': 3, 'org2': 10}, f'Wrong certificates result. expected: {result}' + + +@pytest.mark.django_db +@override_settings(FX_DEFAULT_COURSE_EFFORT=10) +@pytest.mark.parametrize('tenant_ids, expected_result', [ + ([1], 14 * 10), + ([2], 9 * 10), + ([3], 0 * 10), +]) +def test_get_learning_hours_count_for_default_course_effort( + base_data, fx_permission_info, tenant_ids, expected_result +): # pylint: disable=unused-argument + """Verify get_learning_hours_count function.""" + fx_permission_info['view_allowed_full_access_orgs'] = get_tenants_orgs(tenant_ids) + fx_permission_info['view_allowed_any_access_orgs'] = get_tenants_orgs(tenant_ids) + result = certificates.get_learning_hours_count(fx_permission_info) + assert result == expected_result, \ + f'Wrong learning hours count for tenant(s) {tenant_ids}. expected: {expected_result}, got: {result}' + + +@pytest.mark.django_db +@override_settings(FX_DEFAULT_COURSE_EFFORT=10) +@pytest.mark.parametrize('effort, expected_result, usecase', [ + # 2 is the certificate_count in tenant_id 8 + ('20:30', 20.5 * 2, 'Valid, proper HH:MM format with 2-digit minutes'), + ('20:05', 20.1 * 2, 'Valid, minutes as a single digit with leading zero'), + ('20:5', 20.1 * 2, 'Valid, minutes as a single digit without leading zero'), + ('5:5', 5.1 * 2, 'Valid, both hours and minutes are single digits'), + ('30', 30 * 2, 'Valid, only hours provided (no minutes)'), + ('5:120', 10 * 2, 'Invalid, minutes exceed 60, use default value'), + ('invalid', 10 * 2, 'Invalid, non-numeric value for hours, use default value'), + ('20:invalid', 10 * 2, 'Invalid, non-numeric value for minutes, use default value'), +]) +def test_get_learning_hours_count_for_different_course_effor_format( + base_data, fx_permission_info, effort, expected_result, usecase +): # pylint: disable=unused-argument + """Verify get_learning_hours_count function for different course format.""" + fx_permission_info['view_allowed_full_access_orgs'] = get_tenants_orgs([8]) + fx_permission_info['view_allowed_any_access_orgs'] = get_tenants_orgs([8]) + CourseOverview.objects.filter(id='course-v1:ORG8+1+1').update(effort=effort) + result = certificates.get_learning_hours_count(fx_permission_info) + assert result == expected_result, f'Wrong learning hours count usecase: {usecase}' diff --git a/tests/test_dashboard/test_views.py b/tests/test_dashboard/test_views.py index 46788c8c..ae620512 100644 --- a/tests/test_dashboard/test_views.py +++ b/tests/test_dashboard/test_views.py @@ -87,7 +87,9 @@ def test_invalid_stats(self): def test_all_stats(self): """Test get method""" self.login_user(self.staff_user) - response = self.client.get(self.url + '?stats=certificates,courses,hidden_courses,learners,enrollments') + response = self.client.get( + self.url + '?stats=certificates,courses,hidden_courses,learners,enrollments,learning_hours' + ) self.assertTrue(isinstance(response, JsonResponse)) self.assertEqual(response.status_code, http_status.HTTP_200_OK) self.assertDictEqual(json.loads(response.content), expected_statistics) From 088d1f3b141dfc6aacfa609256d0156b9e60ef2b Mon Sep 17 00:00:00 2001 From: Tehreem Sadat Date: Sat, 21 Dec 2024 03:20:21 +1300 Subject: [PATCH 2/2] Review changes --- .../dashboard/statistics/certificates.py | 27 ++++++++++++++++--- .../helpers/settings/common_production.py | 2 +- .../fake_models/models.py | 2 +- .../test_statistics/test_certificates.py | 4 +++ 4 files changed, 29 insertions(+), 6 deletions(-) diff --git a/futurex_openedx_extensions/dashboard/statistics/certificates.py b/futurex_openedx_extensions/dashboard/statistics/certificates.py index 540353ac..5b68afcf 100644 --- a/futurex_openedx_extensions/dashboard/statistics/certificates.py +++ b/futurex_openedx_extensions/dashboard/statistics/certificates.py @@ -1,6 +1,7 @@ """functions for getting statistics about certificates""" from __future__ import annotations +import logging from typing import Dict from django.conf import settings @@ -11,6 +12,8 @@ from futurex_openedx_extensions.helpers.querysets import get_base_queryset_courses +log = logging.getLogger(__name__) + def get_certificates_count( fx_permission_info: dict, visible_courses_filter: bool | None = True, active_courses_filter: bool | None = None @@ -61,19 +64,33 @@ def get_learning_hours_count( :rtype: Dict[str, int] """ - def parse_course_effort(effort: str) -> float: + def parse_course_effort(effort: str, course_id: str) -> float: """Parses course effort in HH:MM format and returns total hours as a float.""" try: + if not effort: + raise ValueError('Course effort is not set.') + parts = effort.split(':') hours = int(parts[0]) minutes = int(parts[1]) if len(parts) > 1 else 0 + if hours < 0 or minutes < 0: + raise ValueError('Hours and minutes must be non-negative values.') if minutes >= 60: raise ValueError('Minutes cannot be 60 or more.') total_hours = hours + minutes / 60 + + if total_hours < 0.5: + raise ValueError('course effort value is too small') + return round(total_hours, 1) - except (ValueError, IndexError): + + except (ValueError, IndexError) as exc: + log.exception( + 'Invalid course-effort for course %s. Assuming default value (%s hours). Error: %s', + course_id, settings.FX_DEFAULT_COURSE_EFFORT, str(exc) + ) return settings.FX_DEFAULT_COURSE_EFFORT result = list( @@ -92,11 +109,13 @@ def parse_course_effort(effort: str) -> float: ) ).annotate( certificates_count=Count('id') - ).values('course_effort', 'certificates_count') + ).values('course_effort', 'certificates_count', 'course_id') ) return sum( parse_course_effort( - entry.get('course_effort', settings.FX_DEFAULT_COURSE_EFFORT)) * entry.get('certificates_count', 0) + entry.get('course_effort', settings.FX_DEFAULT_COURSE_EFFORT), + entry.get('course_id') + ) * entry.get('certificates_count', 0) for entry in result ) diff --git a/futurex_openedx_extensions/helpers/settings/common_production.py b/futurex_openedx_extensions/helpers/settings/common_production.py index e507b524..d6053714 100644 --- a/futurex_openedx_extensions/helpers/settings/common_production.py +++ b/futurex_openedx_extensions/helpers/settings/common_production.py @@ -37,5 +37,5 @@ def plugin_settings(settings: Any) -> None: settings.FX_DEFAULT_COURSE_EFFORT = getattr( settings, 'FX_DEFAULT_COURSE_EFFORT', - '50' + '12', ) diff --git a/test_utils/edx_platform_mocks_shared/fake_models/models.py b/test_utils/edx_platform_mocks_shared/fake_models/models.py index f4e41117..17fdef75 100644 --- a/test_utils/edx_platform_mocks_shared/fake_models/models.py +++ b/test_utils/edx_platform_mocks_shared/fake_models/models.py @@ -32,7 +32,7 @@ class CourseOverview(models.Model): self_paced = models.BooleanField(default=False) course_image_url = models.TextField() visible_to_staff_only = models.BooleanField(default=False) - effort = models.CharField(max_length=255) + effort = models.TextField(null=True) class Meta: app_label = 'fake_models' diff --git a/tests/test_dashboard/test_statistics/test_certificates.py b/tests/test_dashboard/test_statistics/test_certificates.py index 1291ecbd..63ac443e 100644 --- a/tests/test_dashboard/test_statistics/test_certificates.py +++ b/tests/test_dashboard/test_statistics/test_certificates.py @@ -75,9 +75,13 @@ def test_get_learning_hours_count_for_default_course_effort( ('20:5', 20.1 * 2, 'Valid, minutes as a single digit without leading zero'), ('5:5', 5.1 * 2, 'Valid, both hours and minutes are single digits'), ('30', 30 * 2, 'Valid, only hours provided (no minutes)'), + ('0:30', 0.5 * 2, 'Valid, only min with 0 hours'), ('5:120', 10 * 2, 'Invalid, minutes exceed 60, use default value'), ('invalid', 10 * 2, 'Invalid, non-numeric value for hours, use default value'), ('20:invalid', 10 * 2, 'Invalid, non-numeric value for minutes, use default value'), + ('0:-55', 10 * 2, 'Invalid, neg min, use default value'), + ('-55', 10 * 2, 'Invalid, neg hour, use default value'), + ('0:10', 10 * 2, 'Invalid, course effort value is too small less then 0.5, use default value'), ]) def test_get_learning_hours_count_for_different_course_effor_format( base_data, fx_permission_info, effort, expected_result, usecase