diff --git a/RELEASE.rst b/RELEASE.rst index 901dab13ba..bba3884e5a 100644 --- a/RELEASE.rst +++ b/RELEASE.rst @@ -1,6 +1,12 @@ Release Notes ============= +Version 1.142.0 +--------------- + +- add bare minimum upgrade product info to v3 enrollments endpoint (#3385) +- Add a test for number of queries in courses api (#3361) + Version 1.141.4 (Released March 12, 2026) --------------- diff --git a/cms/models.py b/cms/models.py index 81e6f1d279..354e8f6316 100644 --- a/cms/models.py +++ b/cms/models.py @@ -1370,7 +1370,7 @@ def _get_current_finaid(self, request): is_courseware_flexible_price_approved(self.product, request.user) and ecommerce_product ): - ecommerce_product = ecommerce_product.first() + ecommerce_product = ecommerce_product[0] discount = determine_courseware_flexible_price_discount( ecommerce_product, request.user @@ -1818,8 +1818,8 @@ def get_context(self, request, *args, **kwargs): product_page = self.get_parent_product_page() product = product_page.product context["product"] = ( - product.active_products.first() - if isinstance(product, Course) and product.active_products is not None + product.active_products[0] + if isinstance(product, Course) and product.active_products else None ) context["product_page"] = product_page.url diff --git a/cms/serializers.py b/cms/serializers.py index c1c00dd96d..21acf44ecb 100644 --- a/cms/serializers.py +++ b/cms/serializers.py @@ -204,24 +204,14 @@ def get_financial_assistance_form_url(self, instance): def get_current_price(self, instance) -> int | None: """Get the current price of the course product.""" - # Handle both QuerySet and prefetched list cases active_products = instance.product.active_products - if active_products is None: + if not active_products: return None - try: - # Convert to list and sort by price (descending) - products_list = ( - list(active_products.all()) - if hasattr(active_products, "all") - else list(active_products) - ) - relevant_product = ( - max(products_list, key=lambda p: p.price) if products_list else None - ) - except (AttributeError, TypeError): + # Only call max if there are products + relevant_product = max(active_products, key=lambda p: p.price) + except (ValueError, AttributeError, TypeError): relevant_product = None - return relevant_product.price if relevant_product else None @extend_schema_field(list) diff --git a/cms/serializers_test.py b/cms/serializers_test.py index 01ac76462b..d2ad422d29 100644 --- a/cms/serializers_test.py +++ b/cms/serializers_test.py @@ -514,10 +514,8 @@ def test_get_current_price_with_queryset_single_product( course_run = CourseRunFactory(course=course_page.product) product = ProductFactory(purchasable_object=course_run, price=Decimal("99.99")) - # Mock active_products to return a QuerySet-like object - mock_queryset = mocker.MagicMock() - mock_queryset.all.return_value = [product] - course_page.product.active_products = mock_queryset + # Mock active_products to return a list of products + course_page.product.active_products = [product] serializer = CoursePageSerializer() result = serializer.get_current_price(course_page) @@ -537,10 +535,8 @@ def test_get_current_price_with_queryset_multiple_products( product2 = ProductFactory(purchasable_object=course_run2, price=Decimal("100.00")) product3 = ProductFactory(purchasable_object=course_run3, price=Decimal("75.00")) - # Mock active_products to return a QuerySet-like object - mock_queryset = mocker.MagicMock() - mock_queryset.all.return_value = [product1, product2, product3] - course_page.product.active_products = mock_queryset + # Mock active_products to return a list of products + course_page.product.active_products = [product1, product2, product3] serializer = CoursePageSerializer() result = serializer.get_current_price(course_page) @@ -590,10 +586,8 @@ def test_get_current_price_with_empty_queryset(mocker, fully_configured_wagtail) """Test get_current_price with empty QuerySet""" course_page = CoursePageFactory() - # Mock active_products to return empty QuerySet - mock_queryset = mocker.MagicMock() - mock_queryset.all.return_value = [] - course_page.product.active_products = mock_queryset + # Mock active_products to return empty list + course_page.product.active_products = [] serializer = CoursePageSerializer() result = serializer.get_current_price(course_page) diff --git a/courses/models.py b/courses/models.py index 1e373f3c18..cf6887b3b1 100644 --- a/courses/models.py +++ b/courses/models.py @@ -951,13 +951,15 @@ def active_products(self): Gets active products for the first unexpired courserun for this course Returns: - - ProductsQuerySet + - list of active products """ relevant_run = self.first_unexpired_run - - return ( - relevant_run.products.filter(is_active=True).all() if relevant_run else None - ) + if relevant_run is None: + return [] + # Use prefetched products if available + if hasattr(relevant_run, "prefetched_products"): + return [p for p in relevant_run.prefetched_products if p.is_active] + return list(relevant_run.products.filter(is_active=True).all()) @cached_property def first_unexpired_run(self): @@ -1223,12 +1225,16 @@ def is_upgradable(self): Checks if the course can be upgraded A null value means that the upgrade window is always open """ + if hasattr(self, "prefetched_products"): + has_product = bool(self.prefetched_products) + else: + has_product = self.products.exists() return ( self.live is True and ( self.upgrade_deadline is None or (self.upgrade_deadline > now_in_utc()) ) - and self.products.count() > 0 + and has_product ) @cached_property diff --git a/courses/models_test.py b/courses/models_test.py index da4ff3707b..7580f37384 100644 --- a/courses/models_test.py +++ b/courses/models_test.py @@ -1076,7 +1076,7 @@ def test_active_products_for_expired_course_run(): course_run = CourseRunFactory.create(enrollment_end=now - timedelta(days=10)) ProductFactory.create(purchasable_object=course_run) - assert course_run.course.active_products is None + assert course_run.course.active_products == [] def test_related_programs(): diff --git a/courses/serializers/v3/courses.py b/courses/serializers/v3/courses.py index 25b9d7f9d7..2a9df79987 100644 --- a/courses/serializers/v3/courses.py +++ b/courses/serializers/v3/courses.py @@ -32,10 +32,46 @@ class CourseRunWithCourseSerializer(BaseCourseRunSerializer): """CourseRun serializer""" course = CourseSerializer(read_only=True) + upgrade_product_id = serializers.SerializerMethodField() + upgrade_product_price = serializers.SerializerMethodField() + upgrade_product_is_active = serializers.SerializerMethodField() + + def _get_upgrade_product(self, obj): + """Return the active upgrade product only if the run is currently upgradable.""" + if not obj.is_upgradable: + return None + + prefetched_products = getattr(obj, "prefetched_products", None) + if prefetched_products is not None: + return prefetched_products[0] if prefetched_products else None + + return ( + obj.products.filter(is_active=True).only("id", "price", "is_active").first() + ) + + @extend_schema_field(serializers.IntegerField(allow_null=True)) + def get_upgrade_product_id(self, obj): + product = self._get_upgrade_product(obj) + return product.id if product else None + + @extend_schema_field( + serializers.DecimalField(max_digits=7, decimal_places=2, allow_null=True) + ) + def get_upgrade_product_price(self, obj): + product = self._get_upgrade_product(obj) + return str(product.price) if product else None + + @extend_schema_field(serializers.BooleanField(allow_null=True)) + def get_upgrade_product_is_active(self, obj): + product = self._get_upgrade_product(obj) + return product.is_active if product else None class Meta(BaseCourseRunSerializer.Meta): fields = [ *BaseCourseRunSerializer.Meta.fields, + "upgrade_product_id", + "upgrade_product_price", + "upgrade_product_is_active", "course", ] diff --git a/courses/serializers/v3/courses_test.py b/courses/serializers/v3/courses_test.py index 40b0c8a4e2..b0fdd9e485 100644 --- a/courses/serializers/v3/courses_test.py +++ b/courses/serializers/v3/courses_test.py @@ -10,6 +10,7 @@ from courses.serializers.v3.courses import ( CourseRunEnrollmentSerializer, ) +from ecommerce.factories import ProductFactory pytestmark = [pytest.mark.django_db] @@ -57,3 +58,25 @@ def test_serializer_fields(self): } assert set(serialized_data.keys()) == expected_fields + + def test_serializer_includes_upgrade_fields_for_upgradable_run(self): + """Test serialization includes denormalized upgrade fields when product is eligible.""" + enrollment = CourseRunEnrollmentFactory.create() + product = ProductFactory.create(purchasable_object=enrollment.run) + + serialized_data = CourseRunEnrollmentSerializer(enrollment).data + + assert serialized_data["run"]["upgrade_product_id"] == product.id + assert serialized_data["run"]["upgrade_product_price"] == str(product.price) + assert serialized_data["run"]["upgrade_product_is_active"] is True + + def test_serializer_upgrade_fields_null_when_not_eligible(self): + """Test upgrade fields are null if run has no eligible upgrade product.""" + enrollment = CourseRunEnrollmentFactory.create(run__upgrade_deadline=None) + ProductFactory.create(purchasable_object=enrollment.run, is_active=False) + + serialized_data = CourseRunEnrollmentSerializer(enrollment).data + + assert serialized_data["run"]["upgrade_product_id"] is None + assert serialized_data["run"]["upgrade_product_price"] is None + assert serialized_data["run"]["upgrade_product_is_active"] is None diff --git a/courses/views/v2/views_test.py b/courses/views/v2/views_test.py index 529364951a..99dbc4c96d 100644 --- a/courses/views/v2/views_test.py +++ b/courses/views/v2/views_test.py @@ -73,6 +73,7 @@ num_queries_from_programs, ) from courses.views.v2 import Pagination, ProgramFilterSet +from ecommerce.factories import ProductFactory from ecommerce.models import Product from main import features from main.test_utils import assert_drf_json_equal, duplicate_queries_check @@ -2001,3 +2002,38 @@ def test_program_enrollment_destroy(user_drf_client, user): enrollment.refresh_from_db() assert enrollment.active is False assert enrollment.change_status == ENROLL_CHANGE_STATUS_UNENROLLED + + +@pytest.mark.django_db +def test_course_run_and_product_prefetch_optimized( + user_drf_client, django_assert_max_num_queries +): + """ + Verify that querying courses with multiple courseruns and products is optimized and does not result in N+1 queries. + """ + + course = CourseFactory() + num_courseruns = 8 + courseruns = [CourseRunFactory(course=course) for _ in range(num_courseruns)] + for run in courseruns: + ProductFactory( + purchasable_object=run, + ) + max_expected_queries = 21 + num_queries_before = len(connection.queries) + with django_assert_max_num_queries(max_expected_queries): + resp = user_drf_client.get(reverse("v2:courses_api-list")) + assert resp.status_code == 200 + data = resp.json()["results"] + assert len(data) == 1 + assert len(data[0]["courseruns"]) == num_courseruns + # Check that products are queried only once/twice + # not sure why there is a second query + queries_after = connection.queries[num_queries_before:] + + product_queries = [ + q for q in queries_after if 'FROM "ecommerce_product"' in q.get("sql", "") + ] + assert len(product_queries) == 2, ( + f"Expected 1 product query, got {len(product_queries)}: {[q['sql'] for q in product_queries]}" + ) diff --git a/courses/views/v3/__init__.py b/courses/views/v3/__init__.py index 2a1a1a1869..cd70e6bb28 100644 --- a/courses/views/v3/__init__.py +++ b/courses/views/v3/__init__.py @@ -4,7 +4,7 @@ import django_filters from django.conf import settings -from django.db.models import Q +from django.db.models import Prefetch, Q from django.shortcuts import get_object_or_404 from django_filters.rest_framework import DjangoFilterBackend from drf_spectacular.types import OpenApiTypes @@ -27,6 +27,7 @@ ProgramEnrollmentCreateSerializer, ProgramEnrollmentSerializer, ) +from ecommerce.models import Product from main import features @@ -91,7 +92,15 @@ class UserEnrollmentsApiViewSet( "run", "run__b2b_contract", ) - .prefetch_related("run__course", "run__enrollment_modes") + .prefetch_related( + "run__course", + "run__enrollment_modes", + Prefetch( + "run__products", + queryset=Product.objects.only("id", "price", "is_active"), + to_attr="prefetched_products", + ), + ) .prefetch("certificate", "grades") .order_by("id") ) diff --git a/courses/views/v3/views_test.py b/courses/views/v3/views_test.py index ca615b2085..765813609d 100644 --- a/courses/views/v3/views_test.py +++ b/courses/views/v3/views_test.py @@ -35,6 +35,11 @@ def test_user_enrollments_detail( resp = user_drf_client.get( reverse("v3:user_enrollments_api-detail", kwargs={"pk": enrollment.id}) ) + upgrade_product = ( + enrollment.run.products.filter(is_active=True).first() + if enrollment.run.is_upgradable + else None + ) assert resp.status_code == status.HTTP_200_OK assert resp.json() == { "id": enrollment.id, @@ -60,6 +65,13 @@ def test_user_enrollments_detail( else None, "enrollment_end": drf_datetime(enrollment.run.enrollment_end), "enrollment_modes": [], + "upgrade_product_id": upgrade_product.id if upgrade_product else None, + "upgrade_product_price": str(upgrade_product.price) + if upgrade_product + else None, + "upgrade_product_is_active": upgrade_product.is_active + if upgrade_product + else None, "enrollment_start": drf_datetime(enrollment.run.enrollment_start), "expiration_date": drf_datetime(enrollment.run.expiration_date), "course": { @@ -121,6 +133,13 @@ def test_user_enrollments_list( else None, "enrollment_end": drf_datetime(enrollment.run.enrollment_end), "enrollment_modes": [], + "upgrade_product_id": upgrade_product.id if upgrade_product else None, + "upgrade_product_price": str(upgrade_product.price) + if upgrade_product + else None, + "upgrade_product_is_active": upgrade_product.is_active + if upgrade_product + else None, "enrollment_start": drf_datetime(enrollment.run.enrollment_start), "expiration_date": drf_datetime(enrollment.run.expiration_date), "course": { @@ -149,6 +168,11 @@ def test_user_enrollments_list( "certificate": maybe_serialize_course_cert(enrollment.run, enrollment.user), } for enrollment in user_with_enrollments_and_certificates.run_enrollments + for upgrade_product in [ + enrollment.run.products.filter(is_active=True).first() + if enrollment.run.is_upgradable + else None + ] ] @@ -191,6 +215,15 @@ def test_user_enrollments_list_filter_org_id( else None, "enrollment_end": drf_datetime(enrollment.run.enrollment_end), "enrollment_modes": [], + "upgrade_product_id": upgrade_product.id + if upgrade_product + else None, + "upgrade_product_price": str(upgrade_product.price) + if upgrade_product + else None, + "upgrade_product_is_active": upgrade_product.is_active + if upgrade_product + else None, "enrollment_start": drf_datetime(enrollment.run.enrollment_start), "expiration_date": drf_datetime(enrollment.run.expiration_date), "course": { @@ -221,6 +254,11 @@ def test_user_enrollments_list_filter_org_id( ), } for enrollment in user_with_enrollments_and_certificates.run_enrollments + for upgrade_product in [ + enrollment.run.products.filter(is_active=True).first() + if enrollment.run.is_upgradable + else None + ] if enrollment.run in b2b_courses.course_runs_by_org_id[org.id] ] @@ -266,6 +304,13 @@ def test_user_enrollments_list_filter_exclude_b2b( else None, "enrollment_end": drf_datetime(enrollment.run.enrollment_end), "enrollment_modes": [], + "upgrade_product_id": upgrade_product.id if upgrade_product else None, + "upgrade_product_price": str(upgrade_product.price) + if upgrade_product + else None, + "upgrade_product_is_active": upgrade_product.is_active + if upgrade_product + else None, "enrollment_start": drf_datetime(enrollment.run.enrollment_start), "expiration_date": drf_datetime(enrollment.run.expiration_date), "course": { @@ -292,6 +337,11 @@ def test_user_enrollments_list_filter_exclude_b2b( "certificate": maybe_serialize_course_cert(enrollment.run, enrollment.user), } for enrollment in user_with_enrollments_and_certificates.run_enrollments + for upgrade_product in [ + enrollment.run.products.filter(is_active=True).first() + if enrollment.run.is_upgradable + else None + ] if enrollment.run not in b2b_courses.course_runs ] @@ -324,6 +374,13 @@ def test_user_enrollments_list_filter_exclude_b2b( else None, "enrollment_end": drf_datetime(enrollment.run.enrollment_end), "enrollment_modes": [], + "upgrade_product_id": upgrade_product.id if upgrade_product else None, + "upgrade_product_price": str(upgrade_product.price) + if upgrade_product + else None, + "upgrade_product_is_active": upgrade_product.is_active + if upgrade_product + else None, "enrollment_start": drf_datetime(enrollment.run.enrollment_start), "expiration_date": drf_datetime(enrollment.run.expiration_date), "course": { @@ -352,9 +409,25 @@ def test_user_enrollments_list_filter_exclude_b2b( "certificate": maybe_serialize_course_cert(enrollment.run, enrollment.user), } for enrollment in user_with_enrollments_and_certificates.run_enrollments + for upgrade_product in [ + enrollment.run.products.filter(is_active=True).first() + if enrollment.run.is_upgradable + else None + ] ] +def test_user_enrollments_list_query_count_guard( + user_drf_client, + user_with_enrollments_and_certificates: UserWithEnrollmentsAndCerts, + django_assert_max_num_queries, +): + """List endpoint should stay bounded in query count with denormalized upgrade fields.""" + with django_assert_max_num_queries(20): + resp = user_drf_client.get(reverse("v3:user_enrollments_api-list")) + assert resp.status_code == status.HTTP_200_OK + + def test_program_enrollments( user_drf_client, user_with_enrollments_and_certificates, diff --git a/main/settings.py b/main/settings.py index 8a7677f766..f892ca143b 100644 --- a/main/settings.py +++ b/main/settings.py @@ -37,7 +37,7 @@ from main.sentry import init_sentry from openapi.settings_spectacular import open_spectacular_settings -VERSION = "1.141.4" +VERSION = "1.142.0" log = logging.getLogger() diff --git a/openapi/specs/v0.yaml b/openapi/specs/v0.yaml index 45464a6ae8..d98da396b0 100644 --- a/openapi/specs/v0.yaml +++ b/openapi/specs/v0.yaml @@ -4421,6 +4421,20 @@ components: items: $ref: '#/components/schemas/EnrollmentMode' readOnly: true + upgrade_product_id: + type: integer + nullable: true + readOnly: true + upgrade_product_price: + type: string + format: decimal + pattern: ^-?\d{0,5}(?:\.\d{0,2})?$ + nullable: true + readOnly: true + upgrade_product_is_active: + type: boolean + nullable: true + readOnly: true course: allOf: - $ref: '#/components/schemas/CourseV3' @@ -4437,6 +4451,9 @@ components: - is_upgradable - run_tag - title + - upgrade_product_id + - upgrade_product_is_active + - upgrade_product_price CourseRunWithCourseV3Request: type: object description: CourseRun serializer diff --git a/openapi/specs/v1.yaml b/openapi/specs/v1.yaml index 1a33493603..a6346fb72e 100644 --- a/openapi/specs/v1.yaml +++ b/openapi/specs/v1.yaml @@ -4421,6 +4421,20 @@ components: items: $ref: '#/components/schemas/EnrollmentMode' readOnly: true + upgrade_product_id: + type: integer + nullable: true + readOnly: true + upgrade_product_price: + type: string + format: decimal + pattern: ^-?\d{0,5}(?:\.\d{0,2})?$ + nullable: true + readOnly: true + upgrade_product_is_active: + type: boolean + nullable: true + readOnly: true course: allOf: - $ref: '#/components/schemas/CourseV3' @@ -4437,6 +4451,9 @@ components: - is_upgradable - run_tag - title + - upgrade_product_id + - upgrade_product_is_active + - upgrade_product_price CourseRunWithCourseV3Request: type: object description: CourseRun serializer diff --git a/openapi/specs/v2.yaml b/openapi/specs/v2.yaml index 0e1fe09e34..6fbe89eab5 100644 --- a/openapi/specs/v2.yaml +++ b/openapi/specs/v2.yaml @@ -4421,6 +4421,20 @@ components: items: $ref: '#/components/schemas/EnrollmentMode' readOnly: true + upgrade_product_id: + type: integer + nullable: true + readOnly: true + upgrade_product_price: + type: string + format: decimal + pattern: ^-?\d{0,5}(?:\.\d{0,2})?$ + nullable: true + readOnly: true + upgrade_product_is_active: + type: boolean + nullable: true + readOnly: true course: allOf: - $ref: '#/components/schemas/CourseV3' @@ -4437,6 +4451,9 @@ components: - is_upgradable - run_tag - title + - upgrade_product_id + - upgrade_product_is_active + - upgrade_product_price CourseRunWithCourseV3Request: type: object description: CourseRun serializer