From 7a5520558373a379e58281cc69e892840cf5458e Mon Sep 17 00:00:00 2001 From: cp-at-mit Date: Tue, 8 Sep 2026 15:00:10 -0400 Subject: [PATCH 1/3] ENABLE_MULTIPLE_CART_ITEMS=False should disable multi-product baskets even through APIs (#3909) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> --- ecommerce/tests/test_multiple_cart_items.py | 90 +++++++++++++++++++++ ecommerce/views/v0/__init__.py | 16 ++++ ecommerce/views/v0/views_test.py | 3 +- 3 files changed, 108 insertions(+), 1 deletion(-) diff --git a/ecommerce/tests/test_multiple_cart_items.py b/ecommerce/tests/test_multiple_cart_items.py index aec737d917..c735cbe1eb 100644 --- a/ecommerce/tests/test_multiple_cart_items.py +++ b/ecommerce/tests/test_multiple_cart_items.py @@ -115,6 +115,96 @@ def test_basket_items_count_endpoint(self, user_drf_client, user): # Should return count of distinct items, not total quantity assert response.data == 2 + @override_settings(ENABLE_MULTIPLE_CART_ITEMS=False) + def test_create_basket_from_product_single_item_mode_replaces( + self, user_drf_client, user + ): + """With flag disabled, create_basket_from_product replaces existing basket items""" + existing_product = ProductFactory.create() + new_product = ProductFactory.create() + + basket = BasketFactory.create(user=user) + BasketItemFactory.create(basket=basket, product=existing_product) + + assert basket.basket_items.count() == 1 + + response = user_drf_client.post( + f"/api/v0/baskets/create_from_product/{new_product.id}/", + ) + + assert response.status_code == status.HTTP_201_CREATED + basket.refresh_from_db() + assert basket.basket_items.count() == 1 + assert basket.basket_items.first().product == new_product + + @override_settings(ENABLE_MULTIPLE_CART_ITEMS=True) + def test_create_basket_from_product_multiple_items_mode_keeps_existing( + self, user_drf_client, user + ): + """With flag enabled, create_basket_from_product keeps existing basket items""" + existing_product = ProductFactory.create() + new_product = ProductFactory.create() + + basket = BasketFactory.create(user=user) + BasketItemFactory.create(basket=basket, product=existing_product) + + assert basket.basket_items.count() == 1 + + response = user_drf_client.post( + f"/api/v0/baskets/create_from_product/{new_product.id}/", + ) + + assert response.status_code == status.HTTP_201_CREATED + basket.refresh_from_db() + assert basket.basket_items.count() == 2 + + @override_settings(ENABLE_MULTIPLE_CART_ITEMS=False) + def test_create_basket_with_products_single_item_mode_replaces( + self, user_drf_client, user + ): + """With flag disabled, create_basket_with_products replaces existing basket items""" + existing_product = ProductFactory.create() + new_product = ProductFactory.create() + + basket = BasketFactory.create(user=user) + BasketItemFactory.create(basket=basket, product=existing_product) + + assert basket.basket_items.count() == 1 + + response = user_drf_client.post( + "/api/v0/baskets/create_with_products/", + data={"product_ids": [{"product_id": new_product.id, "quantity": 1}]}, + format="json", + ) + + assert response.status_code == status.HTTP_200_OK + basket.refresh_from_db() + assert basket.basket_items.count() == 1 + assert basket.basket_items.first().product == new_product + + @override_settings(ENABLE_MULTIPLE_CART_ITEMS=False) + def test_create_basket_with_products_single_item_mode_rejects_multiple( + self, + user_drf_client, + user, # noqa: ARG002 + ): + """With flag disabled, create_basket_with_products rejects multiple products""" + product1 = ProductFactory.create() + product2 = ProductFactory.create() + + response = user_drf_client.post( + "/api/v0/baskets/create_with_products/", + data={ + "product_ids": [ + {"product_id": product1.id, "quantity": 1}, + {"product_id": product2.id, "quantity": 1}, + ] + }, + format="json", + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + @override_settings(ENABLE_MULTIPLE_CART_ITEMS=False) def test_existing_basket_item_viewset_still_works(self, user_drf_client, user): """Test that the existing BasketItemViewSet still works regardless of feature flag""" diff --git a/ecommerce/views/v0/__init__.py b/ecommerce/views/v0/__init__.py index 7627743b69..08d6bc9479 100644 --- a/ecommerce/views/v0/__init__.py +++ b/ecommerce/views/v0/__init__.py @@ -5,6 +5,7 @@ import logging import django_filters +from django.conf import settings from django.contrib.auth import get_user_model from django.contrib.auth.mixins import LoginRequiredMixin from django.core.exceptions import ObjectDoesNotExist @@ -244,6 +245,11 @@ def _create_basket_from_product( with transaction.atomic(): basket = establish_basket_for_request(request, for_update=True) + if not getattr(settings, "ENABLE_MULTIPLE_CART_ITEMS", False): + basket.basket_items.all().delete() + # Don't clear discounts here — the read→delete→reapply logic below + # already preserves and re-checks them correctly. + # FUTURE: This is where the basket_add hook was called. (_, created) = BasketItem.objects.update_or_create( @@ -418,6 +424,16 @@ def create_basket_with_products(request): {"error": "Product not found"}, status=status.HTTP_404_NOT_FOUND ) + allow_multiple_items = getattr(settings, "ENABLE_MULTIPLE_CART_ITEMS", False) + if not allow_multiple_items: + if len(products) > 1: + return Response( + {"error": "Multiple cart items are not enabled."}, + status=status.HTTP_400_BAD_REQUEST, + ) + basket.basket_items.all().delete() + BasketDiscount.objects.filter(redeemed_basket=basket).delete() + try: for product, quantity in products: # FUTURE: this is where the basket_add hook is called diff --git a/ecommerce/views/v0/views_test.py b/ecommerce/views/v0/views_test.py index f396c648b9..48708fea36 100644 --- a/ecommerce/views/v0/views_test.py +++ b/ecommerce/views/v0/views_test.py @@ -11,7 +11,7 @@ import pytest import reversion from django.forms.models import model_to_dict -from django.test import Client +from django.test import Client, override_settings from django.urls import reverse from django.utils.dateparse import parse_datetime from mitol.common.utils.datetime import now_in_utc @@ -342,6 +342,7 @@ def test_add_basket_item(user_drf_client, user): @pytest.mark.parametrize("existing_basket", [True, False]) @pytest.mark.parametrize("add_discount", [True, False]) @pytest.mark.parametrize("bad_product", [True, False]) +@override_settings(ENABLE_MULTIPLE_CART_ITEMS=True) def test_create_basket_with_products( user, user_client, existing_basket, add_discount, bad_product ): From 3ec3dde9b0d6af35cb7635c5cc72ed78151f5d28 Mon Sep 17 00:00:00 2001 From: Chris Patti Date: Wed, 9 Sep 2026 11:53:01 -0400 Subject: [PATCH 2/3] Optimize course API pagination query (#3939) --- courses/serializers/v2/courses.py | 4 ++-- courses/serializers/v2/courses_test.py | 10 +++++----- courses/utils.py | 12 ++++++++++++ courses/views/internal/__init__.py | 20 ++++---------------- courses/views/v2/__init__.py | 18 ++++-------------- courses/views/v2/views_test.py | 12 ++++++++++-- drf_lint_baseline.json | 9 +++++++++ 7 files changed, 46 insertions(+), 39 deletions(-) diff --git a/courses/serializers/v2/courses.py b/courses/serializers/v2/courses.py index ff2ea4a448..0459010360 100644 --- a/courses/serializers/v2/courses.py +++ b/courses/serializers/v2/courses.py @@ -167,8 +167,8 @@ def get_certificate_available(self, instance) -> bool: return ( instance.first_unexpired_run is not None - and hasattr(instance, "verified_courserun_count") - and instance.verified_courserun_count > 0 + and hasattr(instance, "has_verified_courserun") + and instance.has_verified_courserun and not instance.certificates_disabled and has_live_certificate_page(instance) ) diff --git a/courses/serializers/v2/courses_test.py b/courses/serializers/v2/courses_test.py index 91e4e718a4..3db907ee74 100644 --- a/courses/serializers/v2/courses_test.py +++ b/courses/serializers/v2/courses_test.py @@ -84,12 +84,12 @@ def test_serialize_course( run=courseRun1, **({} if is_anonymous else {"user": user}) ) - # Fake out a "verified_courserun_count" attribute - this is an annotation that the + # Fake out a "has_verified_courserun" attribute - this is an annotation that the # viewset adds, but won't be here because we're just passing it in a straight # Course object. - course.verified_courserun_count = course.courseruns.filter( + course.has_verified_courserun = course.courseruns.filter( enrollment_modes__mode_slug=EDX_ENROLLMENT_VERIFIED_MODE - ).count() + ).exists() data = CourseWithCourseRunsSerializer(instance=course, context=mock_context).data @@ -138,9 +138,9 @@ def _course_eligible_for_certificate(): EnrollmentModeFactory.create(mode_slug=EDX_ENROLLMENT_VERIFIED_MODE) ) course = course_run.course - course.verified_courserun_count = course.courseruns.filter( + course.has_verified_courserun = course.courseruns.filter( enrollment_modes__mode_slug=EDX_ENROLLMENT_VERIFIED_MODE - ).count() + ).exists() return course diff --git a/courses/utils.py b/courses/utils.py index d99a649d52..0cbf2c8676 100644 --- a/courses/utils.py +++ b/courses/utils.py @@ -24,6 +24,18 @@ log = logging.getLogger(__name__) +def verified_courserun_exists(): + """Build an Exists() annotation for whether a course has a verified run.""" + from openedx.constants import EDX_ENROLLMENT_VERIFIED_MODE # noqa: PLC0415 + + return Exists( + CourseRun.objects.filter( + course_id=OuterRef("pk"), + enrollment_modes__mode_slug=EDX_ENROLLMENT_VERIFIED_MODE, + ) + ) + + def live_certificate_page_exists(): """ Build an Exists() subquery annotation for whether a live CertificatePage diff --git a/courses/views/internal/__init__.py b/courses/views/internal/__init__.py index 9fa8ee309d..11a447de2d 100644 --- a/courses/views/internal/__init__.py +++ b/courses/views/internal/__init__.py @@ -1,7 +1,7 @@ """Internal-only views for courses.""" from django.contrib.contenttypes.models import ContentType -from django.db.models import Count, Prefetch, Q +from django.db.models import Prefetch from prefetch import PrefetchOption from rest_framework import viewsets from rest_framework_api_key.permissions import HasAPIKey @@ -13,10 +13,9 @@ ) from courses.permissions import IsEtlUser from courses.serializers.internal import IngestibleCourseWithCourseRunsSerializer -from courses.utils import live_certificate_page_exists +from courses.utils import live_certificate_page_exists, verified_courserun_exists from courses.views.utils import Pagination from ecommerce.models import Product -from openedx.constants import EDX_ENROLLMENT_VERIFIED_MODE class IngestibleCourseViewSet(viewsets.ReadOnlyModelViewSet): @@ -76,19 +75,8 @@ def get_queryset(self): dated_runs_prefetch, ) queryset = queryset.annotate( - count_b2b_courseruns=Count("courseruns__b2b_contract__id") - ) - queryset = queryset.annotate(count_courseruns=Count("courseruns")) - queryset = queryset.annotate( - verified_courserun_count=Count( - "courseruns__enrollment_modes", - filter=Q( - courseruns__enrollment_modes__mode_slug=EDX_ENROLLMENT_VERIFIED_MODE - ), - ) - ) - queryset = queryset.annotate( - has_live_certificate_page=live_certificate_page_exists() + has_verified_courserun=verified_courserun_exists(), + has_live_certificate_page=live_certificate_page_exists(), ) queryset = queryset.prefetch( PrefetchOption( diff --git a/courses/views/v2/__init__.py b/courses/views/v2/__init__.py index 8b86785c0f..2c79098b24 100644 --- a/courses/views/v2/__init__.py +++ b/courses/views/v2/__init__.py @@ -8,7 +8,7 @@ import django_filters from django.conf import settings from django.contrib.contenttypes.models import ContentType -from django.db.models import Count, Prefetch, Q +from django.db.models import Prefetch, Q from django.http import JsonResponse from django.shortcuts import get_object_or_404 from django_filters.rest_framework import DjangoFilterBackend @@ -77,6 +77,7 @@ get_program_certificate_by_enrollment, get_unenrollable_courses, live_certificate_page_exists, + verified_courserun_exists, ) from ecommerce.api import create_verified_program_course_run_enrollment from ecommerce.models import Product @@ -464,19 +465,8 @@ def get_queryset(self): "departments", "in_programs", course_runs_prefetch ) queryset = queryset.annotate( - count_b2b_courseruns=Count("courseruns__b2b_contract__id") - ) - queryset = queryset.annotate(count_courseruns=Count("courseruns")) - queryset = queryset.annotate( - verified_courserun_count=Count( - "courseruns__enrollment_modes", - filter=Q( - courseruns__enrollment_modes__mode_slug=EDX_ENROLLMENT_VERIFIED_MODE - ), - ) - ) - queryset = queryset.annotate( - has_live_certificate_page=live_certificate_page_exists() + has_verified_courserun=verified_courserun_exists(), + has_live_certificate_page=live_certificate_page_exists(), ) queryset = queryset.prefetch_related( Prefetch( diff --git a/courses/views/v2/views_test.py b/courses/views/v2/views_test.py index 5b1092b488..19cc182308 100644 --- a/courses/views/v2/views_test.py +++ b/courses/views/v2/views_test.py @@ -15,7 +15,7 @@ from django.contrib.auth.models import AnonymousUser from django.contrib.contenttypes.models import ContentType from django.db import connection -from django.db.models import Q +from django.db.models import Exists, Q from django.test import RequestFactory from django.urls import reverse from faker import Faker @@ -77,7 +77,7 @@ num_queries_from_department, num_queries_from_programs, ) -from courses.views.v2 import Pagination, ProgramFilterSet +from courses.views.v2 import CourseViewSet, Pagination, ProgramFilterSet from ecommerce.factories import OrderFactory, ProductFactory from ecommerce.models import OrderStatus, Product from main import features @@ -259,6 +259,14 @@ def test_delete_program( assert resp.status_code == status.HTTP_405_METHOD_NOT_ALLOWED +def test_course_queryset_avoids_courserun_aggregate_annotations(): + """Course pagination should not aggregate over every related course run.""" + annotations = CourseViewSet().get_queryset().query.annotations + + assert isinstance(annotations["has_verified_courserun"], Exists) + assert {"count_b2b_courseruns", "count_courseruns"}.isdisjoint(annotations) + + @pytest.mark.skip_nplusone_check @pytest.mark.usefixtures("course_catalog_data") @pytest.mark.parametrize("course_catalog_course_count", [100], indirect=True) diff --git a/drf_lint_baseline.json b/drf_lint_baseline.json index 83dd5b7093..a71d4b4cf0 100644 --- a/drf_lint_baseline.json +++ b/drf_lint_baseline.json @@ -16,7 +16,16 @@ "courses/serializers/v1/programs.py:303:27:ORM001", "courses/serializers/v1/programs.py:318:16:ORM001", "courses/serializers/v1/programs.py:335:17:ORM001", + "courses/serializers/v2/courses.py:138:18:ORM004", + "courses/serializers/v2/courses.py:142:18:ORM003", + "courses/serializers/v2/courses.py:150:19:ORM004", + "courses/serializers/v2/courses.py:157:11:ORM003", + "courses/serializers/v2/courses.py:158:22:ORM003", + "courses/serializers/v2/courses.py:169:12:ORM003", + "courses/serializers/v2/courses.py:219:12:ORM007", + "courses/serializers/v2/courses.py:268:15:ORM004", "courses/serializers/v2/courses.py:276:17:ORM002", + "courses/serializers/v2/courses.py:297:19:ORM007", "courses/serializers/v2/departments.py:35:40:ORM002", "courses/serializers/v2/departments.py:49:42:ORM002", "courses/serializers/v2/programs.py:387:50:ORM002", From 90e2a77faaee39529b1f9449706de0b5ace56fa4 Mon Sep 17 00:00:00 2001 From: Doof Date: Wed, 9 Sep 2026 15:56:55 +0000 Subject: [PATCH 3/3] Release 1.165.3 --- RELEASE.rst | 6 ++++++ main/settings.py | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/RELEASE.rst b/RELEASE.rst index e5e5a557f3..6cc5913929 100644 --- a/RELEASE.rst +++ b/RELEASE.rst @@ -1,6 +1,12 @@ Release Notes ============= +Version 1.165.3 +--------------- + +- Optimize course API pagination query (#3939) +- ENABLE_MULTIPLE_CART_ITEMS=False should disable multi-product baskets even through APIs (#3909) + Version 1.165.2 --------------- diff --git a/main/settings.py b/main/settings.py index fd7b112b98..b00ef0bfc5 100644 --- a/main/settings.py +++ b/main/settings.py @@ -39,7 +39,7 @@ from main.sentry import init_sentry from openapi.settings_spectacular import open_spectacular_settings -VERSION = "1.165.2" +VERSION = "1.165.3" log = logging.getLogger()