Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions RELEASE.rst
Original file line number Diff line number Diff line change
@@ -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)
---------------

Expand Down
6 changes: 3 additions & 3 deletions cms/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
18 changes: 4 additions & 14 deletions cms/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
18 changes: 6 additions & 12 deletions cms/serializers_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
18 changes: 12 additions & 6 deletions courses/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion courses/models_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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():
Expand Down
36 changes: 36 additions & 0 deletions courses/serializers/v3/courses.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment on lines +44 to +46
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The is_upgradable property incorrectly returns True for runs with only inactive products, causing the serializer to potentially expose inactive product data.
Severity: MEDIUM

Suggested Fix

Update the is_upgradable property in courses/models.py to check for active products specifically. When using prefetched products, use any(p.is_active for p in self.prefetched_products). For the database query fallback, use self.products.filter(is_active=True).exists(). Additionally, consider adding is_active=True to the prefetch query in courses/views/v3/__init__.py to prevent inactive products from being fetched in the first place.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: courses/serializers/v3/courses.py#L44-L46

Potential issue: The `is_upgradable` property on the `CourseRun` model can incorrectly
return `True` when a run only has inactive products. This is because the logic checks
for the existence of any product (`self.products.exists()` or
`bool(self.prefetched_products)`) without filtering for `is_active=True`. When this
occurs in an API view that uses a prefetch for all products (active and inactive), the
`CourseRunWithCourseSerializer`'s `_get_upgrade_product` method may return the first
product from the prefetched list, which could be an inactive one. This leads to the API
response incorrectly indicating that a run is upgradable and exposing details (ID,
price) of an inactive product.

Did we get this right? 👍 / 👎 to inform future reviews.


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",
]

Expand Down
23 changes: 23 additions & 0 deletions courses/serializers/v3/courses_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from courses.serializers.v3.courses import (
CourseRunEnrollmentSerializer,
)
from ecommerce.factories import ProductFactory

pytestmark = [pytest.mark.django_db]

Expand Down Expand Up @@ -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
36 changes: 36 additions & 0 deletions courses/views/v2/views_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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]}"
)
13 changes: 11 additions & 2 deletions courses/views/v3/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -27,6 +27,7 @@
ProgramEnrollmentCreateSerializer,
ProgramEnrollmentSerializer,
)
from ecommerce.models import Product
from main import features


Expand Down Expand Up @@ -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")
)
Expand Down
Loading
Loading