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
7 changes: 7 additions & 0 deletions RELEASE.rst
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
Release Notes
=============

Version 1.144.3
---------------

- Update get_certificate_available to use the enrollment modes for determination (#3446)
- fix: don't send receipts for modules of a program (#3442)
- Don't sync edX course modes back from edX (#3443)

Version 1.144.2 (Released March 31, 2026)
---------------

Expand Down
4 changes: 4 additions & 0 deletions courses/factories.py
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,10 @@ class CourseRunFactory(DjangoModelFactory):
b2b_contract = None
is_source_run = False

enrollment_modes = factory.RelatedFactoryList(
EnrollmentModeFactory, size=1, mode_slug=EDX_ENROLLMENT_AUDIT_MODE
)

class Meta:
model = CourseRun

Expand Down
3 changes: 2 additions & 1 deletion courses/serializers/v2/courses.py
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,8 @@ def get_certificate_available(self, instance) -> bool:

return (
instance.first_unexpired_run is not None
and instance.first_unexpired_run.certificate_available_date is not None
and hasattr(instance, "verified_courserun_count")
and instance.verified_courserun_count > 0
Comment on lines 173 to +177

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 get_certificate_available method relies on the verified_courserun_count annotation, which is not present when CourseSerializer is used outside the CourseViewSet, causing incorrect results.
Severity: MEDIUM

Suggested Fix

The logic for get_certificate_available should not depend on an annotation that is only applied in a specific viewset. Instead, it should directly query the related courseruns to determine if any have a verified enrollment mode. This will make the serializer's logic self-contained and reliable across all its usage contexts.

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/v2/courses.py#L173-L177

Potential issue: The `get_certificate_available` method in `CourseSerializer` was
updated to check for a `verified_courserun_count` attribute on the `Course` instance.
This attribute is only annotated onto the queryset within the `CourseViewSet`. However,
`CourseSerializer` is also instantiated directly in other parts of the codebase, such as
in `cms/models.py`. In these contexts, the `Course` instance lacks the
`verified_courserun_count` annotation. The `hasattr` check will fail, causing the method
to silently and incorrectly return `False` for `certificate_available`, leading to
incorrect data in API endpoints that consume the serializer outside the viewset.

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

)

@extend_schema_field(str)
Expand Down
15 changes: 15 additions & 0 deletions courses/serializers/v2/courses_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
CourseFactory,
CourseRunEnrollmentFactory,
CourseRunFactory,
EnrollmentModeFactory,
ProgramFactory,
)
from courses.models import CourseRunEnrollment, CoursesTopic, Department
Expand All @@ -23,6 +24,7 @@
)
from courses.views.v2 import UserEnrollmentFilterSet
from main.test_utils import assert_drf_json_equal
from openedx.constants import EDX_ENROLLMENT_VERIFIED_MODE

pytestmark = [pytest.mark.django_db]

Expand Down Expand Up @@ -56,7 +58,13 @@ def test_serialize_course( # noqa: PLR0913
)
else:
courseRun1 = CourseRunFactory.create()
courseRun1.enrollment_modes.add(
EnrollmentModeFactory.create(mode_slug=EDX_ENROLLMENT_VERIFIED_MODE)
)
courseRun2 = CourseRunFactory.create(course=courseRun1.course)
courseRun2.enrollment_modes.add(
EnrollmentModeFactory.create(mode_slug=EDX_ENROLLMENT_VERIFIED_MODE)
)

if is_anonymous:
mock_context["request"].user = AnonymousUser()
Expand All @@ -78,6 +86,13 @@ def test_serialize_course( # noqa: PLR0913
run=courseRun1, **({} if is_anonymous else {"user": user})
)

# Fake out a "verified_courserun_count" 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(
enrollment_modes__mode_slug=EDX_ENROLLMENT_VERIFIED_MODE
).count()

data = CourseWithCourseRunsSerializer(instance=course, context=mock_context).data

assert_drf_json_equal(
Expand Down
3 changes: 1 addition & 2 deletions courses/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ def sync_courseruns_data():
"""
Task to sync titles and dates for course runs from edX.
"""
from courses.api import sync_course_mode, sync_course_runs
from courses.api import sync_course_runs

now = now_in_utc()
runs = (
Expand All @@ -33,7 +33,6 @@ def sync_courseruns_data():
)

# `sync_course_runs` logs internally so no need to capture/output the returned values
sync_course_mode(runs)
sync_course_runs(runs)


Expand Down
9 changes: 9 additions & 0 deletions courses/views/v2/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,15 @@ def get_queryset(self):
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
),
)
)

return queryset.order_by("title").distinct()

def get_serializer_context(self):
Expand Down
26 changes: 21 additions & 5 deletions ecommerce/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,9 @@
log = logging.getLogger(__name__)


def generate_checkout_payload(request, *, skip_discount_check=False): # noqa: PLR0911
def generate_checkout_payload( # noqa: PLR0911
request, *, skip_discount_check=False, skip_receipt=False
):
"""
Generate the checkout payload for the current basket.

Expand All @@ -90,6 +92,7 @@ def generate_checkout_payload(request, *, skip_discount_check=False): # noqa: P
- request: the incoming http request
Kwargs:
- skip_discount_check: skip checking discounts for validity (default False)
- skip_receipt: skip sending order receipt email (default False)
"""

from b2b.api import validate_basket_for_b2b_purchase # noqa: PLC0415
Expand Down Expand Up @@ -200,7 +203,10 @@ def generate_checkout_payload(request, *, skip_discount_check=False): # noqa: P
if total_price == 0:
with transaction.atomic():
fulfill_completed_order(
order, payment_data=ZERO_PAYMENT_DATA, basket=basket
order,
payment_data=ZERO_PAYMENT_DATA,
basket=basket,
skip_receipt=skip_receipt,
)

order.refresh_from_db()
Expand Down Expand Up @@ -329,9 +335,17 @@ def apply_user_discounts(request):
return


def fulfill_completed_order(order, payment_data, basket=None, already_enrolled=False): # noqa: FBT002
def fulfill_completed_order(
order,
payment_data,
basket=None,
already_enrolled=False, # noqa: FBT002
skip_receipt=False, # noqa: FBT002
):
order_flow = order.get_object_flow()
order_flow.fulfill(payment_data, already_enrolled=already_enrolled)
order_flow.fulfill(
payment_data, already_enrolled=already_enrolled, skip_receipt=skip_receipt
)
sync_hubspot_deal(order)

if basket and basket.compare_to_order(order):
Expand Down Expand Up @@ -1118,7 +1132,9 @@ def create_verified_program_course_run_enrollment(request, courserun, program):
msg = f"Basket for {request.user} is not zero-value"
raise VerifiedProgramInvalidBasketError(msg)

processed_order = generate_checkout_payload(request, skip_discount_check=True)
processed_order = generate_checkout_payload(
request, skip_discount_check=True, skip_receipt=True
)

if "no_checkout" not in processed_order:
# It didn't just clear the order so something went wrong
Expand Down
14 changes: 12 additions & 2 deletions ecommerce/api_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -735,11 +735,20 @@ def test_create_verified_program_discount():


def test_create_verified_program_course_run_enrollment(
mock_create_run_enrollments, mock_hubspot_order, bootstrapped_verified_program, user
mocker,
mock_create_run_enrollments,
mock_hubspot_order,
bootstrapped_verified_program,
user,
):
"""Test that creating a verified course run enrollment for a program works."""
"""Test that creating a verified course run enrollment for a program works
and does not send a receipt email.
"""

mock_cre_side_effect = mock_create_run_enrollments.side_effect
mock_send_receipt = mocker.patch(
"ecommerce.tasks.send_ecommerce_order_receipt.delay"
)

(program, _, _, courserun, _) = bootstrapped_verified_program

Expand All @@ -766,6 +775,7 @@ def test_create_verified_program_course_run_enrollment(
)

assert cr_enrollment.enrollment_mode == EDX_ENROLLMENT_VERIFIED_MODE
mock_send_receipt.assert_not_called()

mock_create_run_enrollments.side_effect = mock_cre_side_effect

Expand Down
5 changes: 3 additions & 2 deletions ecommerce/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -702,17 +702,18 @@ def create_enrollments(self):
source=OrderStatus.PENDING,
target=OrderStatus.FULFILLED,
)
def fulfill(self, payment_data, already_enrolled=False): # noqa: FBT002
def fulfill(self, payment_data, already_enrolled=False, skip_receipt=False): # noqa: FBT002
# record the transaction
self.create_transaction(payment_data)

# record all the courseruns in the order
self.create_enrollments()

# No email is required as this order is generated from management command
# Skip receipt emails for UAI orders
# Skip receipt emails for UAI orders and program-derived course run orders
if (
not already_enrolled
and not skip_receipt
and not is_uai_order(self.order)
and not is_contract_order(self.order)
):
Expand Down
29 changes: 29 additions & 0 deletions ecommerce/models_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -849,3 +849,32 @@ def test_process_transaction_line_hooks(mocker, user, user_drf_client):

assert mocked_create_run_enrollment.called
assert not mocked_create_program_enrollment.called


@pytest.mark.skip_nplusone_check
@pytest.mark.parametrize(
("skip_receipt", "email_sent"),
[(True, False), (False, True)],
)
def test_fulfill_skip_receipt(
mocker, django_capture_on_commit_callbacks, skip_receipt, email_sent
):
"""Test that fulfill respects the skip_receipt flag for sending receipt email."""
mocker.patch("courses.api.create_run_enrollments", autospec=True)
mock_send_receipt = mocker.patch(
"ecommerce.tasks.send_ecommerce_order_receipt.delay"
)

pending_order = OrderFactory.create(state=OrderStatus.PENDING)
order_flow = pending_order.get_object_flow()

with django_capture_on_commit_callbacks(execute=True):
order_flow.fulfill(
{"amount": 0, "data": {"reason": "No payment required"}},
skip_receipt=skip_receipt,
)

if email_sent:
mock_send_receipt.assert_called_once_with(pending_order.id)
else:
mock_send_receipt.assert_not_called()
2 changes: 1 addition & 1 deletion main/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
from main.sentry import init_sentry
from openapi.settings_spectacular import open_spectacular_settings

VERSION = "1.144.2"
VERSION = "1.144.3"

log = logging.getLogger()

Expand Down
Loading