-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add learning hours count to the stats api #169
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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 | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we want to make it silent for the user, but noisy for us in the logs
Suggested change
|
||||||||||||||||||||
|
||||||||||||||||||||
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( | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ this is great, let's see the performance on production. We can cache it if we find it slowing down the dashboard |
||||||||||||||||||||
parse_course_effort( | ||||||||||||||||||||
entry.get('course_effort', settings.FX_DEFAULT_COURSE_EFFORT)) * entry.get('certificates_count', 0) | ||||||||||||||||||||
for entry in result | ||||||||||||||||||||
) |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
] | ||||||
|
||||||
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: | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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' | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this value was communicated with Nelc team
Suggested change
|
||||||
) |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. always remember, please: mocks are our hidden enemies in tests. We need to make them similar to the original as much as we can
Suggested change
|
||||||
|
||||||
class Meta: | ||||||
app_label = 'fake_models' | ||||||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -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'), | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. two more tests, please
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that's a good catch there. |
||||||||||
]) | ||||||||||
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}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ same default values for argument as in
get_certificates_count
. Thank you!