From 9402503d917a9b7ea8770c1ded716152683f1fdc Mon Sep 17 00:00:00 2001 From: chethan2106 Date: Thu, 30 Apr 2026 14:03:41 +0000 Subject: [PATCH 1/5] feat: Requesting to add Course Passing Grade in the Admin Portal Learner Progress report --- enterprise_data/api/v1/serializers.py | 6 +++ .../api/v1/views/enterprise_learner.py | 34 ++++++++++++-- .../api/v1/views/lpr_data_source_base.py | 1 + enterprise_data/renderers.py | 1 + enterprise_data/tests/api/v1/test_views.py | 44 +++++++++++++++++++ 5 files changed, 82 insertions(+), 4 deletions(-) diff --git a/enterprise_data/api/v1/serializers.py b/enterprise_data/api/v1/serializers.py index 904b6d12..cd1be3f1 100644 --- a/enterprise_data/api/v1/serializers.py +++ b/enterprise_data/api/v1/serializers.py @@ -29,6 +29,7 @@ class EnterpriseLearnerEnrollmentSerializer(serializers.ModelSerializer): total_learning_time_hours = serializers.SerializerMethodField() enterprise_flex_group_name = serializers.SerializerMethodField() enterprise_flex_group_uuid = serializers.SerializerMethodField() + course_passing_grade = serializers.SerializerMethodField() course_progress = serializers.SerializerMethodField() class Meta: @@ -51,6 +52,7 @@ class Meta: 'enterprise_customer_uuid', 'enterprise_sso_uid', 'created', 'course_api_url', 'total_learning_time_hours', 'is_subsidy', 'course_product_line', 'budget_id', 'enterprise_flex_group_name', 'enterprise_flex_group_uuid', + 'course_passing_grade', 'course_progress', ) @@ -72,6 +74,10 @@ def get_course_progress(self, obj): """Returns learner course progress from selected report data.""" return getattr(obj, 'course_progress', None) + def get_course_passing_grade(self, obj): + """Returns required passing grade for the course, if available.""" + return getattr(obj, 'course_passing_grade', None) + @cache_it() def _get_flex_groups(self, obj): """ diff --git a/enterprise_data/api/v1/views/enterprise_learner.py b/enterprise_data/api/v1/views/enterprise_learner.py index 2b3a002f..5d81375d 100644 --- a/enterprise_data/api/v1/views/enterprise_learner.py +++ b/enterprise_data/api/v1/views/enterprise_learner.py @@ -6,6 +6,7 @@ from logging import getLogger from uuid import UUID +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview # pylint: disable=import-error from rest_framework import filters, viewsets from rest_framework.decorators import action from rest_framework.exceptions import NotFound @@ -14,6 +15,7 @@ from django.conf import settings from django.core.paginator import Paginator +from django.db import connection from django.db.models import Count, Exists, Max, OuterRef, Prefetch, Q, Subquery, Value from django.db.models.fields import IntegerField from django.db.models.functions import Coalesce @@ -70,6 +72,7 @@ class EnterpriseLearnerEnrollmentViewSet(EnterpriseViewSetMixin, viewsets.ReadOn 'enterprise_customer_uuid', 'enterprise_sso_uid', 'created', 'course_api_url', 'total_learning_time_hours', 'is_subsidy', 'course_product_line', 'budget_id', 'enterprise_flex_group_name', 'enterprise_flex_group_uuid', + 'course_passing_grade', 'course_progress', ] @@ -96,14 +99,37 @@ def get_queryset(self): # TODO: Created a ticket ENT0-9531 to add the cache on this viewset - # Add a synthetic placeholder column so the serialized response shape - # always includes `course_progress`; real values are merged in later. + # Add synthetic placeholder columns so the serialized response shape + # always includes `course_progress` and `course_passing_grade`. + enrollments = self._get_enrollments_with_course_metadata(enterprise_customer_uuid) + + enrollments = self.apply_filters(enrollments) + + return enrollments + + def _get_enrollments_with_course_metadata(self, enterprise_customer_uuid): + """ + Build the enrollments queryset and include synthetic metadata fields. + + Adds ``course_progress`` as a placeholder and joins CourseOverview for + ``course_passing_grade`` when the CourseOverview table is present. + """ enrollments = EnterpriseLearnerEnrollment.objects.filter( enterprise_customer_uuid=enterprise_customer_uuid ).extra(select={'course_progress': 'NULL'}) - enrollments = self.apply_filters(enrollments) - return enrollments + course_overview_table = CourseOverview._meta.db_table + has_course_overview_table = course_overview_table in connection.introspection.table_names() + if not has_course_overview_table: + return enrollments.extra(select={'course_passing_grade': 'NULL'}) + + # Use an explicit INNER JOIN to fetch passing grade from CourseOverview. + enrollment_table = EnterpriseLearnerEnrollment._meta.db_table + return enrollments.extra( + tables=[course_overview_table], + where=[f'{course_overview_table}.id = {enrollment_table}.courserun_key'], + select={'course_passing_grade': f'{course_overview_table}.lowest_passing_grade'}, + ) def list(self, request, *args, **kwargs): """ diff --git a/enterprise_data/api/v1/views/lpr_data_source_base.py b/enterprise_data/api/v1/views/lpr_data_source_base.py index 80ab52d4..74afa86d 100644 --- a/enterprise_data/api/v1/views/lpr_data_source_base.py +++ b/enterprise_data/api/v1/views/lpr_data_source_base.py @@ -31,6 +31,7 @@ class LPRSerializerShapeMixin: 'enterprise_customer_uuid', 'enterprise_sso_uid', 'created', 'course_api_url', 'total_learning_time_hours', 'is_subsidy', 'course_product_line', 'budget_id', 'enterprise_flex_group_name', 'enterprise_flex_group_uuid', + 'course_passing_grade', 'course_progress', ) diff --git a/enterprise_data/renderers.py b/enterprise_data/renderers.py index 231f5dd4..b38c7a5e 100644 --- a/enterprise_data/renderers.py +++ b/enterprise_data/renderers.py @@ -28,6 +28,7 @@ class EnrollmentsCSVRenderer(CSVStreamingRenderer): 'user_country_code', 'user_username', 'user_first_name', 'user_last_name', 'enterprise_name', 'enterprise_customer_uuid', 'enterprise_sso_uid', 'created', 'course_api_url', 'total_learning_time_hours', 'is_subsidy', 'course_product_line', 'budget_id', 'enterprise_flex_group_name', 'enterprise_flex_group_uuid', + 'course_passing_grade', 'course_progress', ] diff --git a/enterprise_data/tests/api/v1/test_views.py b/enterprise_data/tests/api/v1/test_views.py index 78d627f1..22637aeb 100644 --- a/enterprise_data/tests/api/v1/test_views.py +++ b/enterprise_data/tests/api/v1/test_views.py @@ -3,7 +3,10 @@ """ import datetime +import importlib import os +import sys +import types from unittest import mock from uuid import UUID, uuid4 @@ -33,6 +36,47 @@ from enterprise_data_roles.models import EnterpriseDataFeatureRole, EnterpriseDataRoleAssignment +def _ensure_course_overview_import_for_tests(): + """ + Provide a minimal ``openedx...course_overviews.models`` shim for local + unit tests when full edx-platform Python dependencies are unavailable. + """ + try: + course_overview_models = importlib.import_module( + 'openedx.core.djangoapps.content.course_overviews.models' + ) + if getattr(course_overview_models, 'CourseOverview', None) is not None: + return + except ImportError: + pass + + module_names = [ + 'openedx', + 'openedx.core', + 'openedx.core.djangoapps', + 'openedx.core.djangoapps.content', + 'openedx.core.djangoapps.content.course_overviews', + ] + for module_name in module_names: + if module_name not in sys.modules: + sys.modules[module_name] = types.ModuleType(module_name) + + models_module_name = 'openedx.core.djangoapps.content.course_overviews.models' + models_module = types.ModuleType(models_module_name) + + class _Meta: + db_table = 'course_overviews_courseoverview' + + class DummyCourseOverview: + _meta = _Meta() + + models_module.CourseOverview = DummyCourseOverview + sys.modules[models_module_name] = models_module + + +_ensure_course_overview_import_for_tests() + + @ddt.ddt @mark.django_db class TestEnterpriseLearnerEnrollmentViewSet(JWTTestMixin, APITransactionTestCase): From 14cf0d852f7a268c2bb8970ffea106d816c94d8e Mon Sep 17 00:00:00 2001 From: chethan2106 Date: Mon, 4 May 2026 11:38:30 +0000 Subject: [PATCH 2/5] feat: import and test case fix --- .../api/v1/views/enterprise_learner.py | 9 ++++++- enterprise_data/tests/api/v1/test_views.py | 25 +++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/enterprise_data/api/v1/views/enterprise_learner.py b/enterprise_data/api/v1/views/enterprise_learner.py index 5d81375d..ab946f1e 100644 --- a/enterprise_data/api/v1/views/enterprise_learner.py +++ b/enterprise_data/api/v1/views/enterprise_learner.py @@ -6,7 +6,11 @@ from logging import getLogger from uuid import UUID -from openedx.core.djangoapps.content.course_overviews.models import CourseOverview # pylint: disable=import-error +try: + from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +except ImportError: + CourseOverview = None + from rest_framework import filters, viewsets from rest_framework.decorators import action from rest_framework.exceptions import NotFound @@ -118,6 +122,9 @@ def _get_enrollments_with_course_metadata(self, enterprise_customer_uuid): enterprise_customer_uuid=enterprise_customer_uuid ).extra(select={'course_progress': 'NULL'}) + if CourseOverview is None: + return enrollments.extra(select={'course_passing_grade': 'NULL'}) + course_overview_table = CourseOverview._meta.db_table has_course_overview_table = course_overview_table in connection.introspection.table_names() if not has_course_overview_table: diff --git a/enterprise_data/tests/api/v1/test_views.py b/enterprise_data/tests/api/v1/test_views.py index 22637aeb..c8f329b3 100644 --- a/enterprise_data/tests/api/v1/test_views.py +++ b/enterprise_data/tests/api/v1/test_views.py @@ -336,6 +336,31 @@ def test_stream_serialized_data_enriches_course_progress_from_snowflake(self, mo self.assertIn('course_progress', content) self.assertIn('0.87', content) + def test_course_passing_grade_field_in_response(self): + """Test that course_passing_grade field is included in the API response""" + enterprise_learner = EnterpriseLearnerFactory( + enterprise_customer_uuid=self.enterprise_id, + user_email='student@example.com', + ) + EnterpriseLearnerEnrollmentFactory( + enterprise_customer_uuid=self.enterprise_id, + is_consent_granted=True, + enterprise_user_id=enterprise_learner.enterprise_user_id, + user_email='student@example.com', + courserun_key='course-v1:edX+Demo+2024', + ) + + url = reverse('v1:enterprise-learner-enrollment-list', kwargs={'enterprise_id': self.enterprise_id}) + response = self.client.get(url) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + results = response.json()['results'] + self.assertEqual(len(results), 1) + # Verify course_passing_grade field is present in the response + self.assertIn('course_passing_grade', results[0]) + # The field will be null since CourseOverview is mocked in tests + self.assertIsNone(results[0]['course_passing_grade']) + @ddt.ddt @mark.django_db From 25520ae33ff96b91c41b86098d71f56ad8e1c3e9 Mon Sep 17 00:00:00 2001 From: chethan2106 Date: Mon, 4 May 2026 15:24:48 +0000 Subject: [PATCH 3/5] test: add test cases for course_passing_grade and course_progress fields --- .../tests/api/v1/test_serializers.py | 9 +++ enterprise_data/tests/api/v1/test_views.py | 81 +++++++++++++++++++ 2 files changed, 90 insertions(+) diff --git a/enterprise_data/tests/api/v1/test_serializers.py b/enterprise_data/tests/api/v1/test_serializers.py index b66dd8de..da39285f 100644 --- a/enterprise_data/tests/api/v1/test_serializers.py +++ b/enterprise_data/tests/api/v1/test_serializers.py @@ -48,6 +48,15 @@ def test_course_progress_field_present(self): serializer = EnterpriseLearnerEnrollmentSerializer(self.enrollment) assert serializer.data['course_progress'] == 0.42 + def test_course_passing_grade_field_present(self): + self.enrollment.course_passing_grade = 0.7 + serializer = EnterpriseLearnerEnrollmentSerializer(self.enrollment) + assert serializer.data['course_passing_grade'] == 0.7 + + def test_course_passing_grade_field_defaults_to_none(self): + serializer = EnterpriseLearnerEnrollmentSerializer(self.enrollment) + assert serializer.data['course_passing_grade'] is None + def test_csv_renderer_header_matches_serializer_field_order(self): """CSV header must exactly match serializer field order.""" serializer_fields = list(EnterpriseLearnerEnrollmentSerializer.Meta.fields) diff --git a/enterprise_data/tests/api/v1/test_views.py b/enterprise_data/tests/api/v1/test_views.py index c8f329b3..68ebc5f1 100644 --- a/enterprise_data/tests/api/v1/test_views.py +++ b/enterprise_data/tests/api/v1/test_views.py @@ -19,6 +19,7 @@ from django.utils import timezone from enterprise_data.api.v1.serializers import EnterpriseOfferSerializer +from enterprise_data.api.v1.views.enterprise_learner import EnterpriseLearnerEnrollmentViewSet from enterprise_data.models import EnterpriseLearnerEnrollment, EnterpriseOffer from enterprise_data.tests.factories import ( EnterpriseAdminLearnerProgressFactory, @@ -361,6 +362,86 @@ def test_course_passing_grade_field_in_response(self): # The field will be null since CourseOverview is mocked in tests self.assertIsNone(results[0]['course_passing_grade']) + @mock.patch('enterprise_data.api.v1.views.enterprise_learner.CourseOverview', None) + @mock.patch('enterprise_data.api.v1.views.enterprise_learner.EnterpriseLearnerEnrollment.objects.filter') + def test_get_enrollments_with_course_metadata_without_course_overview(self, mock_filter): + viewset = EnterpriseLearnerEnrollmentViewSet() + enrollments = mock.Mock() + with_progress = mock.Mock() + with_passing_grade = mock.Mock() + + mock_filter.return_value = enrollments + enrollments.extra.return_value = with_progress + with_progress.extra.return_value = with_passing_grade + + result = viewset._get_enrollments_with_course_metadata(self.enterprise_id) # pylint: disable=protected-access + + mock_filter.assert_called_once_with(enterprise_customer_uuid=self.enterprise_id) + enrollments.extra.assert_called_once_with(select={'course_progress': 'NULL'}) + with_progress.extra.assert_called_once_with(select={'course_passing_grade': 'NULL'}) + self.assertEqual(result, with_passing_grade) + + @mock.patch('enterprise_data.api.v1.views.enterprise_learner.EnterpriseLearnerEnrollment.objects.filter') + @mock.patch('enterprise_data.api.v1.views.enterprise_learner.connection.introspection.table_names') + @mock.patch('enterprise_data.api.v1.views.enterprise_learner.CourseOverview') + def test_get_enrollments_with_course_metadata_without_course_overview_table( + self, + mock_course_overview, + mock_table_names, + mock_filter, + ): + viewset = EnterpriseLearnerEnrollmentViewSet() + enrollments = mock.Mock() + with_progress = mock.Mock() + with_passing_grade = mock.Mock() + + mock_course_overview._meta.db_table = 'course_overviews_courseoverview' + mock_table_names.return_value = [] + mock_filter.return_value = enrollments + enrollments.extra.return_value = with_progress + with_progress.extra.return_value = with_passing_grade + + result = viewset._get_enrollments_with_course_metadata(self.enterprise_id) # pylint: disable=protected-access + + mock_filter.assert_called_once_with(enterprise_customer_uuid=self.enterprise_id) + enrollments.extra.assert_called_once_with(select={'course_progress': 'NULL'}) + mock_table_names.assert_called_once_with() + with_progress.extra.assert_called_once_with(select={'course_passing_grade': 'NULL'}) + self.assertEqual(result, with_passing_grade) + + @mock.patch('enterprise_data.api.v1.views.enterprise_learner.EnterpriseLearnerEnrollment.objects.filter') + @mock.patch('enterprise_data.api.v1.views.enterprise_learner.connection.introspection.table_names') + @mock.patch('enterprise_data.api.v1.views.enterprise_learner.CourseOverview') + def test_get_enrollments_with_course_metadata_with_course_overview_table( + self, + mock_course_overview, + mock_table_names, + mock_filter, + ): + viewset = EnterpriseLearnerEnrollmentViewSet() + enrollments = mock.Mock() + with_progress = mock.Mock() + with_joined_course_metadata = mock.Mock() + + mock_course_overview._meta.db_table = 'course_overviews_courseoverview' + mock_table_names.return_value = ['course_overviews_courseoverview'] + mock_filter.return_value = enrollments + enrollments.extra.return_value = with_progress + with_progress.extra.return_value = with_joined_course_metadata + + result = viewset._get_enrollments_with_course_metadata(self.enterprise_id) # pylint: disable=protected-access + enrollment_table = EnterpriseLearnerEnrollment._meta.db_table + + mock_filter.assert_called_once_with(enterprise_customer_uuid=self.enterprise_id) + enrollments.extra.assert_called_once_with(select={'course_progress': 'NULL'}) + mock_table_names.assert_called_once_with() + with_progress.extra.assert_called_once_with( + tables=['course_overviews_courseoverview'], + where=[f'course_overviews_courseoverview.id = {enrollment_table}.courserun_key'], + select={'course_passing_grade': 'course_overviews_courseoverview.lowest_passing_grade'}, + ) + self.assertEqual(result, with_joined_course_metadata) + @ddt.ddt @mark.django_db From 630747217cc8251bb9f18a46c57b8d99c7ec5875 Mon Sep 17 00:00:00 2001 From: chethan2106 Date: Tue, 5 May 2026 11:47:47 +0000 Subject: [PATCH 4/5] feat: Add Course Passing Grade in the Admin Portal Learner Progress Report --- .../api/v1/views/enterprise_learner.py | 101 ++++++++++++++---- .../api/v1/views/lpr_data_source_snowflake.py | 84 +++++++++++++-- enterprise_data/tests/api/v1/test_views.py | 62 +++++------ .../lpr/test_lpr_data_source_snowflake.py | 40 +++++-- 4 files changed, 210 insertions(+), 77 deletions(-) diff --git a/enterprise_data/api/v1/views/enterprise_learner.py b/enterprise_data/api/v1/views/enterprise_learner.py index ab946f1e..2d2473e8 100644 --- a/enterprise_data/api/v1/views/enterprise_learner.py +++ b/enterprise_data/api/v1/views/enterprise_learner.py @@ -26,6 +26,7 @@ from django.http import StreamingHttpResponse from django.utils import timezone +from enterprise_data import cache from enterprise_data.api.v1 import serializers from enterprise_data.clients import EnterpriseApiClient from enterprise_data.exceptions import EnterpriseApiClientException @@ -41,6 +42,7 @@ DEFAULT_LEARNER_CACHE_TIMEOUT = 60 * 10 +DEFAULT_COURSE_PASSING_GRADE_CACHE_TIMEOUT = 60 * 60 * 6 class EnterpriseLearnerEnrollmentViewSet(EnterpriseViewSetMixin, viewsets.ReadOnlyModelViewSet): @@ -115,28 +117,14 @@ def _get_enrollments_with_course_metadata(self, enterprise_customer_uuid): """ Build the enrollments queryset and include synthetic metadata fields. - Adds ``course_progress`` as a placeholder and joins CourseOverview for - ``course_passing_grade`` when the CourseOverview table is present. + Adds placeholders for fields enriched after serialization. Keeping the + base queryset ORM-only avoids a per-request CourseOverview join and + lets the enrichment path use a reusable cache. """ enrollments = EnterpriseLearnerEnrollment.objects.filter( enterprise_customer_uuid=enterprise_customer_uuid ).extra(select={'course_progress': 'NULL'}) - - if CourseOverview is None: - return enrollments.extra(select={'course_passing_grade': 'NULL'}) - - course_overview_table = CourseOverview._meta.db_table - has_course_overview_table = course_overview_table in connection.introspection.table_names() - if not has_course_overview_table: - return enrollments.extra(select={'course_passing_grade': 'NULL'}) - - # Use an explicit INNER JOIN to fetch passing grade from CourseOverview. - enrollment_table = EnterpriseLearnerEnrollment._meta.db_table - return enrollments.extra( - tables=[course_overview_table], - where=[f'{course_overview_table}.id = {enrollment_table}.courserun_key'], - select={'course_passing_grade': f'{course_overview_table}.lowest_passing_grade'}, - ) + return enrollments.extra(select={'course_passing_grade': 'NULL'}) def list(self, request, *args, **kwargs): """ @@ -156,13 +144,13 @@ def list(self, request, *args, **kwargs): def _enrich_course_progress_rows(self, rows): """ - Enrich serialized enrollment rows with ``course_progress`` fetched from - Snowflake's internal LPR table. + Enrich serialized enrollment rows with cached course metadata. Accepts a list-like collection of serialized row dicts and mutates each matching row in place. Silently skips enrichment on any error so the ORM-backed response is always returned intact. """ + self._enrich_course_passing_grade_rows(rows) try: if not rows: return rows @@ -170,7 +158,7 @@ def _enrich_course_progress_rows(self, rows): progress_map = SnowflakeCourseProgressSource().get_course_progress_map(enterprise_uuid, rows) for row in rows: key = ( - row.get('user_email', '').strip(), + row.get('user_email', '').strip().lower(), row.get('courserun_key', '').strip(), ) if key in progress_map: @@ -180,6 +168,77 @@ def _enrich_course_progress_rows(self, rows): LOGGER.warning('Could not enrich course_progress from Snowflake', exc_info=True) return rows + @staticmethod + def _course_passing_grade_cache_timeout(): + """Return the configurable TTL for CourseOverview passing-grade cache entries.""" + return getattr( + settings, + 'LPR_COURSE_PASSING_GRADE_CACHE_TIMEOUT', + DEFAULT_COURSE_PASSING_GRADE_CACHE_TIMEOUT, + ) + + @staticmethod + def _course_passing_grade_cache_key(courserun_key): + """Return the cache key for a CourseOverview passing grade.""" + return cache.get_key('lpr_course_passing_grade', courserun_key) + + @staticmethod + def _course_overview_table_exists(): + """Return whether CourseOverview can be queried in this runtime.""" + if CourseOverview is None: + return False + return CourseOverview._meta.db_table in connection.introspection.table_names() + + def _enrich_course_passing_grade_rows(self, rows): + """ + Enrich serialized enrollment rows with cached CourseOverview passing grades. + """ + try: + if not rows or not self._course_overview_table_exists(): + return rows + + courserun_keys = sorted({ + row.get('courserun_key', '').strip() + for row in rows + if row.get('courserun_key') + }) + if not courserun_keys: + return rows + + passing_grade_map = {} + missing_courserun_keys = [] + for courserun_key in courserun_keys: + cached_response = cache.get(self._course_passing_grade_cache_key(courserun_key)) + if cached_response.is_found: + passing_grade_map[courserun_key] = cached_response.value + else: + missing_courserun_keys.append(courserun_key) + + if missing_courserun_keys: + fetched_grades = dict( + CourseOverview.objects.filter(id__in=missing_courserun_keys).values_list( + 'id', + 'lowest_passing_grade', + ) + ) + for courserun_key in missing_courserun_keys: + passing_grade = fetched_grades.get(courserun_key) + passing_grade_map[courserun_key] = passing_grade + cache.set( + self._course_passing_grade_cache_key(courserun_key), + passing_grade, + timeout=self._course_passing_grade_cache_timeout(), + ) + + for row in rows: + courserun_key = row.get('courserun_key', '').strip() + if courserun_key in passing_grade_map: + row['course_passing_grade'] = passing_grade_map[courserun_key] + return rows + except Exception: # pylint: disable=broad-exception-caught + LOGGER.warning('Could not enrich course_passing_grade from CourseOverview', exc_info=True) + return rows + def _enrich_course_progress(self, response): """ Enrich each row in the paginated response with ``course_progress`` fetched diff --git a/enterprise_data/api/v1/views/lpr_data_source_snowflake.py b/enterprise_data/api/v1/views/lpr_data_source_snowflake.py index c6c16ba9..4f479586 100644 --- a/enterprise_data/api/v1/views/lpr_data_source_snowflake.py +++ b/enterprise_data/api/v1/views/lpr_data_source_snowflake.py @@ -26,8 +26,12 @@ from django.conf import settings +from enterprise_data import cache + LOGGER = getLogger(__name__) +DEFAULT_COURSE_PROGRESS_CACHE_TIMEOUT = 60 * 60 * 6 + try: import snowflake.connector as snowflake_connector except ImportError: # pragma: no cover - depends on runtime extras @@ -98,6 +102,33 @@ def _internal_table(): table = getattr(settings, 'LPR_SNOWFLAKE_INTERNAL_TABLE', 'LEARNER_PROGRESS_REPORT_INTERNAL') return f'{database}.{schema}.{table}' + @staticmethod + def _normalized_enterprise_uuid(enterprise_customer_uuid): + """Normalize enterprise UUIDs to the Snowflake comparison format.""" + return str(enterprise_customer_uuid).replace('-', '').lower() + + @staticmethod + def _normalized_row_key(user_email, courserun_key): + """Return a stable in-memory key for matching ORM rows to Snowflake rows.""" + return (str(user_email).strip().lower(), str(courserun_key).strip()) + + def _cache_key(self, enterprise_customer_uuid): + """Return the enterprise-scoped cache key for Snowflake course progress.""" + return cache.get_key( + 'lpr_course_progress', + self._internal_table(), + self._normalized_enterprise_uuid(enterprise_customer_uuid), + ) + + @staticmethod + def _cache_timeout(): + """Return the configurable TTL for Snowflake course progress cache entries.""" + return getattr( + settings, + 'LPR_COURSE_PROGRESS_CACHE_TIMEOUT', + DEFAULT_COURSE_PROGRESS_CACHE_TIMEOUT, + ) + # ------------------------------------------------------------------ # Public interface # ------------------------------------------------------------------ @@ -107,6 +138,11 @@ def get_course_progress_map(self, enterprise_customer_uuid, enrollments): Return a ``{(user_email, courserun_key): course_progress}`` mapping for all rows on the current page. + The Snowflake data refreshes roughly daily, so this method keeps an + enterprise-scoped cache of the full Snowflake result set and returns + only the rows requested by the current page. This avoids repeated + Snowflake queries while keeping the public API unchanged. + Args: enterprise_customer_uuid (str): The enterprise UUID to scope the query. enrollments (list[dict]): Serialized enrollment rows from the ORM page. @@ -117,31 +153,59 @@ def get_course_progress_map(self, enterprise_customer_uuid, enrollments): try/except so Snowflake unavailability never breaks the LPR response. """ pairs = [ - (row.get('user_email', ''), row.get('courserun_key', '')) + self._normalized_row_key(row.get('user_email', ''), row.get('courserun_key', '')) for row in enrollments if row.get('user_email') and row.get('courserun_key') ] if not pairs: return {} - table = self._internal_table() - normalized_uuid = str(enterprise_customer_uuid).replace('-', '').lower() + enterprise_progress_map = self._get_enterprise_course_progress_map(enterprise_customer_uuid) + return { + pair: enterprise_progress_map[pair] + for pair in pairs + if pair in enterprise_progress_map + } + + def _get_enterprise_course_progress_map(self, enterprise_customer_uuid): + """ + Return the full enterprise course progress map from cache or Snowflake. + """ + cache_key = self._cache_key(enterprise_customer_uuid) + cached_response = cache.get(cache_key) + if cached_response.is_found: + LOGGER.info( + '[course_progress] Cache hit for enterprise_uuid=%s', + enterprise_customer_uuid, + ) + return cached_response.value - # Build parameterised IN list for (user_email, courserun_key) pairs. - placeholders = ', '.join(['(%s, %s)'] * len(pairs)) - flat_params = [param for pair in pairs for param in pair] + LOGGER.info( + '[course_progress] Cache miss for enterprise_uuid=%s; fetching from Snowflake', + enterprise_customer_uuid, + ) + progress_map = self._fetch_enterprise_course_progress_map(enterprise_customer_uuid) + cache.set(cache_key, progress_map, timeout=self._cache_timeout()) + return progress_map + + def _fetch_enterprise_course_progress_map(self, enterprise_customer_uuid): + """ + Fetch all course progress rows for an enterprise from Snowflake. + """ + + table = self._internal_table() + normalized_uuid = self._normalized_enterprise_uuid(enterprise_customer_uuid) sql = ( f"SELECT USER_EMAIL, COURSERUN_KEY, COURSE_PROGRESS " f"FROM {table} " - f"WHERE LOWER(REPLACE(TO_VARCHAR(ENTERPRISE_CUSTOMER_UUID), '-', '')) = %s " - f" AND (USER_EMAIL, COURSERUN_KEY) IN ({placeholders})" + f"WHERE LOWER(REPLACE(TO_VARCHAR(ENTERPRISE_CUSTOMER_UUID), '-', '')) = %s" ) ctx = self._get_connection() cs = ctx.cursor() try: - cs.execute(sql, [normalized_uuid] + flat_params) + cs.execute(sql, [normalized_uuid]) rows = cs.fetchall() if not rows: LOGGER.warning( @@ -150,7 +214,7 @@ def get_course_progress_map(self, enterprise_customer_uuid, enrollments): enterprise_customer_uuid, ) return { - (row[0], row[1]): row[2] + self._normalized_row_key(row[0], row[1]): row[2] for row in rows } finally: diff --git a/enterprise_data/tests/api/v1/test_views.py b/enterprise_data/tests/api/v1/test_views.py index 68ebc5f1..f6109377 100644 --- a/enterprise_data/tests/api/v1/test_views.py +++ b/enterprise_data/tests/api/v1/test_views.py @@ -381,66 +381,52 @@ def test_get_enrollments_with_course_metadata_without_course_overview(self, mock with_progress.extra.assert_called_once_with(select={'course_passing_grade': 'NULL'}) self.assertEqual(result, with_passing_grade) - @mock.patch('enterprise_data.api.v1.views.enterprise_learner.EnterpriseLearnerEnrollment.objects.filter') - @mock.patch('enterprise_data.api.v1.views.enterprise_learner.connection.introspection.table_names') + @mock.patch('enterprise_data.api.v1.views.enterprise_learner.cache') @mock.patch('enterprise_data.api.v1.views.enterprise_learner.CourseOverview') - def test_get_enrollments_with_course_metadata_without_course_overview_table( + @mock.patch('enterprise_data.api.v1.views.enterprise_learner.connection.introspection.table_names') + def test_enrich_course_passing_grade_uses_cache( self, - mock_course_overview, mock_table_names, - mock_filter, + mock_course_overview, + mock_cache, ): viewset = EnterpriseLearnerEnrollmentViewSet() - enrollments = mock.Mock() - with_progress = mock.Mock() - with_passing_grade = mock.Mock() - mock_course_overview._meta.db_table = 'course_overviews_courseoverview' - mock_table_names.return_value = [] - mock_filter.return_value = enrollments - enrollments.extra.return_value = with_progress - with_progress.extra.return_value = with_passing_grade + mock_table_names.return_value = ['course_overviews_courseoverview'] + mock_cache.get.return_value = mock.Mock(is_found=True, value=0.7) + rows = [{'courserun_key': 'course-v1:edX+Demo+2024', 'course_passing_grade': None}] - result = viewset._get_enrollments_with_course_metadata(self.enterprise_id) # pylint: disable=protected-access + result = viewset._enrich_course_passing_grade_rows(rows) # pylint: disable=protected-access - mock_filter.assert_called_once_with(enterprise_customer_uuid=self.enterprise_id) - enrollments.extra.assert_called_once_with(select={'course_progress': 'NULL'}) + self.assertEqual(result[0]['course_passing_grade'], 0.7) mock_table_names.assert_called_once_with() - with_progress.extra.assert_called_once_with(select={'course_passing_grade': 'NULL'}) - self.assertEqual(result, with_passing_grade) + mock_course_overview.objects.filter.assert_not_called() + mock_cache.set.assert_not_called() - @mock.patch('enterprise_data.api.v1.views.enterprise_learner.EnterpriseLearnerEnrollment.objects.filter') + @mock.patch('enterprise_data.api.v1.views.enterprise_learner.cache') @mock.patch('enterprise_data.api.v1.views.enterprise_learner.connection.introspection.table_names') @mock.patch('enterprise_data.api.v1.views.enterprise_learner.CourseOverview') - def test_get_enrollments_with_course_metadata_with_course_overview_table( + def test_enrich_course_passing_grade_fetches_and_caches_missing_grade( self, mock_course_overview, mock_table_names, - mock_filter, + mock_cache, ): viewset = EnterpriseLearnerEnrollmentViewSet() - enrollments = mock.Mock() - with_progress = mock.Mock() - with_joined_course_metadata = mock.Mock() - mock_course_overview._meta.db_table = 'course_overviews_courseoverview' mock_table_names.return_value = ['course_overviews_courseoverview'] - mock_filter.return_value = enrollments - enrollments.extra.return_value = with_progress - with_progress.extra.return_value = with_joined_course_metadata + mock_cache.get.return_value = mock.Mock(is_found=False) + mock_course_overview.objects.filter.return_value.values_list.return_value = [ + ('course-v1:edX+Demo+2024', 0.7), + ] + rows = [{'courserun_key': 'course-v1:edX+Demo+2024', 'course_passing_grade': None}] - result = viewset._get_enrollments_with_course_metadata(self.enterprise_id) # pylint: disable=protected-access - enrollment_table = EnterpriseLearnerEnrollment._meta.db_table + result = viewset._enrich_course_passing_grade_rows(rows) # pylint: disable=protected-access - mock_filter.assert_called_once_with(enterprise_customer_uuid=self.enterprise_id) - enrollments.extra.assert_called_once_with(select={'course_progress': 'NULL'}) + self.assertEqual(result[0]['course_passing_grade'], 0.7) mock_table_names.assert_called_once_with() - with_progress.extra.assert_called_once_with( - tables=['course_overviews_courseoverview'], - where=[f'course_overviews_courseoverview.id = {enrollment_table}.courserun_key'], - select={'course_passing_grade': 'course_overviews_courseoverview.lowest_passing_grade'}, - ) - self.assertEqual(result, with_joined_course_metadata) + mock_course_overview.objects.filter.assert_called_once_with(id__in=['course-v1:edX+Demo+2024']) + mock_cache.set.assert_called_once() @ddt.ddt diff --git a/enterprise_data/tests/lpr/test_lpr_data_source_snowflake.py b/enterprise_data/tests/lpr/test_lpr_data_source_snowflake.py index 1846f042..0fa96843 100644 --- a/enterprise_data/tests/lpr/test_lpr_data_source_snowflake.py +++ b/enterprise_data/tests/lpr/test_lpr_data_source_snowflake.py @@ -77,10 +77,15 @@ def test_returns_empty_dict_when_no_pairs(self): assert _source().get_course_progress_map(ENTERPRISE_UUID, []) == {} assert _source().get_course_progress_map(ENTERPRISE_UUID, [{'user_email': '', 'courserun_key': ''}]) == {} + @patch('enterprise_data.api.v1.views.lpr_data_source_snowflake.cache') @patch.object(SnowflakeCourseProgressSource, '_get_connection') @patch.object(SnowflakeCourseProgressSource, '_internal_table', return_value=DEFAULT_TABLE) - def test_executes_expected_sql_and_params(self, _table, mock_conn_factory): - ctx, cursor = _mock_ctx_and_cursor(fetchall=[('alice@example.com', 'course-v1:Org+Course+Run', 0.8)]) + def test_executes_expected_sql_and_params(self, _table, mock_conn_factory, mock_cache): + mock_cache.get.return_value = MagicMock(is_found=False) + ctx, cursor = _mock_ctx_and_cursor(fetchall=[ + ('alice@example.com', 'course-v1:Org+Course+Run', 0.8), + ('carol@example.com', 'course-v1:Org+Other+Run', 0.4), + ]) mock_conn_factory.return_value = ctx enrollments = [ @@ -94,15 +99,32 @@ def test_executes_expected_sql_and_params(self, _table, mock_conn_factory): assert 'COURSE_PROGRESS' in sql assert 'USER_EMAIL, COURSERUN_KEY' in sql assert NORMALIZED_UUID == params[0] - assert params[1:] == [ - 'alice@example.com', 'course-v1:Org+Course+Run', - 'bob@example.com', 'course-v1:Org+Other+Run', - ] + assert len(params) == 1 + assert result == {('alice@example.com', 'course-v1:Org+Course+Run'): 0.8} + mock_cache.set.assert_called_once() + + @patch('enterprise_data.api.v1.views.lpr_data_source_snowflake.cache') + @patch.object(SnowflakeCourseProgressSource, '_get_connection') + def test_returns_results_from_enterprise_cache(self, mock_conn_factory, mock_cache): + mock_cache.get.return_value = MagicMock( + is_found=True, + value={('alice@example.com', 'course-v1:Org+Course+Run'): 0.8}, + ) + + result = _source().get_course_progress_map( + ENTERPRISE_UUID, + [{'user_email': 'Alice@Example.com', 'courserun_key': 'course-v1:Org+Course+Run'}], + ) + assert result == {('alice@example.com', 'course-v1:Org+Course+Run'): 0.8} + mock_conn_factory.assert_not_called() + mock_cache.set.assert_not_called() + @patch('enterprise_data.api.v1.views.lpr_data_source_snowflake.cache') @patch.object(SnowflakeCourseProgressSource, '_get_connection') @patch.object(SnowflakeCourseProgressSource, '_internal_table', return_value=DEFAULT_TABLE) - def test_cursor_and_connection_closed_on_success(self, _table, mock_conn_factory): + def test_cursor_and_connection_closed_on_success(self, _table, mock_conn_factory, mock_cache): + mock_cache.get.return_value = MagicMock(is_found=False) ctx, cursor = _mock_ctx_and_cursor() mock_conn_factory.return_value = ctx _source().get_course_progress_map( @@ -112,9 +134,11 @@ def test_cursor_and_connection_closed_on_success(self, _table, mock_conn_factory cursor.close.assert_called_once() ctx.close.assert_called_once() + @patch('enterprise_data.api.v1.views.lpr_data_source_snowflake.cache') @patch.object(SnowflakeCourseProgressSource, '_get_connection') @patch.object(SnowflakeCourseProgressSource, '_internal_table', return_value=DEFAULT_TABLE) - def test_cursor_and_connection_closed_on_execute_error(self, _table, mock_conn_factory): + def test_cursor_and_connection_closed_on_execute_error(self, _table, mock_conn_factory, mock_cache): + mock_cache.get.return_value = MagicMock(is_found=False) ctx, cursor = _mock_ctx_and_cursor() cursor.execute.side_effect = RuntimeError('Snowflake error') mock_conn_factory.return_value = ctx From bd6bc3b5bb7d35dce0d8922b92e0bea00aa976da Mon Sep 17 00:00:00 2001 From: chethan2106 Date: Tue, 5 May 2026 14:32:26 +0000 Subject: [PATCH 5/5] feat: update caching time and approach --- .../api/v1/views/lpr_data_source_snowflake.py | 3 +- .../lpr/test_lpr_data_source_snowflake.py | 29 ++++++++++++++++++- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/enterprise_data/api/v1/views/lpr_data_source_snowflake.py b/enterprise_data/api/v1/views/lpr_data_source_snowflake.py index 4f479586..dd16ba01 100644 --- a/enterprise_data/api/v1/views/lpr_data_source_snowflake.py +++ b/enterprise_data/api/v1/views/lpr_data_source_snowflake.py @@ -19,6 +19,7 @@ LPR_SNOWFLAKE_INTERNAL_TABLE = 'LEARNER_PROGRESS_REPORT_INTERNAL' LPR_SNOWFLAKE_WAREHOUSE = None (omitted from connection if not set) LPR_SNOWFLAKE_ROLE = None (omitted from connection if not set) + LPR_COURSE_PROGRESS_CACHE_TIMEOUT = 300 (5 minutes) """ from logging import getLogger @@ -30,7 +31,7 @@ LOGGER = getLogger(__name__) -DEFAULT_COURSE_PROGRESS_CACHE_TIMEOUT = 60 * 60 * 6 +DEFAULT_COURSE_PROGRESS_CACHE_TIMEOUT = 60 * 5 try: import snowflake.connector as snowflake_connector diff --git a/enterprise_data/tests/lpr/test_lpr_data_source_snowflake.py b/enterprise_data/tests/lpr/test_lpr_data_source_snowflake.py index 0fa96843..6df99282 100644 --- a/enterprise_data/tests/lpr/test_lpr_data_source_snowflake.py +++ b/enterprise_data/tests/lpr/test_lpr_data_source_snowflake.py @@ -5,7 +5,12 @@ import pytest -from enterprise_data.api.v1.views.lpr_data_source_snowflake import SnowflakeCourseProgressSource +from django.test import override_settings + +from enterprise_data.api.v1.views.lpr_data_source_snowflake import ( + DEFAULT_COURSE_PROGRESS_CACHE_TIMEOUT, + SnowflakeCourseProgressSource, +) ENTERPRISE_UUID = 'a1b2c3d4-e5f6-7890-abcd-ef1234567890' NORMALIZED_UUID = 'a1b2c3d4e5f67890abcdef1234567890' @@ -70,6 +75,28 @@ def test_connection_includes_optional_role_and_warehouse(self, mock_settings, mo assert kwargs['role'] == 'ANALYST' +class TestCacheConfiguration: + """Tests for enterprise-scoped course progress cache configuration.""" + + def test_default_cache_timeout_is_five_minutes(self): + assert DEFAULT_COURSE_PROGRESS_CACHE_TIMEOUT == 300 + assert _source()._cache_timeout() == 300 + + @override_settings(LPR_COURSE_PROGRESS_CACHE_TIMEOUT=120) + def test_cache_timeout_is_configurable(self): + assert _source()._cache_timeout() == 120 + + @patch('enterprise_data.api.v1.views.lpr_data_source_snowflake.cache.get_key', return_value='cache-key') + @patch.object(SnowflakeCourseProgressSource, '_internal_table', return_value=DEFAULT_TABLE) + def test_cache_key_scopes_by_table_and_normalized_enterprise(self, _table, mock_get_key): + assert _source()._cache_key(ENTERPRISE_UUID) == 'cache-key' + mock_get_key.assert_called_once_with( + 'lpr_course_progress', + DEFAULT_TABLE, + NORMALIZED_UUID, + ) + + class TestGetCourseProgressMap: """Tests for SQL execution, mapping, and cleanup behavior."""