Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
0a1edae
Default keep_failed_enrollments to feature flag in enrollments
cp-at-mit Jan 29, 2026
7a5a66e
Default keep_failed_enrollments to feature flag
cp-at-mit Jan 30, 2026
a26e8fe
Use settings.FEATURES for IGNORE_EDX_FAILURES
cp-at-mit Jan 30, 2026
6c8bdf5
Log and optionally ignore edX sync failures
cp-at-mit Jan 30, 2026
38d583d
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jan 30, 2026
7a1d82a
Import is_enabled and adjust feature-flag tests
cp-at-mit Jan 31, 2026
1f66eca
Merge branch 'ignore-edx-failures-variable-usage' of https://github.c…
cp-at-mit Jan 31, 2026
1fe76a2
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jan 31, 2026
44e4db5
Fix tests
cp-at-mit Feb 2, 2026
e340841
fix tests
cp-at-mit Feb 2, 2026
de93314
Fix test
cp-at-mit Feb 2, 2026
8997b2c
fix mock
cp-at-mit Feb 2, 2026
0067a49
lint
cp-at-mit Feb 2, 2026
255f3b6
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Feb 2, 2026
53a3289
lint
cp-at-mit Feb 2, 2026
82590df
Merge branch 'ignore-edx-failures-variable-usage' of https://github.c…
cp-at-mit Feb 2, 2026
b0a04e8
lint
cp-at-mit Feb 2, 2026
8fb7877
ruff
cp-at-mit Feb 2, 2026
06f9947
Merge branch 'main' into ignore-edx-failures-variable-usage
cp-at-mit Feb 3, 2026
f73aab5
Fix enrollment
cp-at-mit Feb 3, 2026
f9bdf48
Suppress PLR0913 lint warning on test func
cp-at-mit Feb 4, 2026
938c533
Merge branch 'main' into ignore-edx-failures-variable-usage
cp-at-mit Feb 9, 2026
212260f
Merge branch 'main' into ignore-edx-failures-variable-usage
cp-at-mit Feb 9, 2026
e0b0feb
Fix merge
cp-at-mit Feb 9, 2026
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
16 changes: 14 additions & 2 deletions courses/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ def create_run_enrollments( # noqa: C901
runs,
*,
change_status=None,
keep_failed_enrollments=False,
keep_failed_enrollments=None,
mode=EDX_DEFAULT_ENROLLMENT_MODE,
):
"""
Expand All @@ -156,13 +156,19 @@ def create_run_enrollments( # noqa: C901
change_status (str): The status of the enrollment
keep_failed_enrollments: (boolean): If True, keeps the local enrollment record
in the database even if the enrollment fails in edX.
If None, defaults to the value of the IGNORE_EDX_FAILURES feature flag.
mode (str): The course mode

Returns:
(list of CourseRunEnrollment, bool): A list of enrollment objects that were successfully
created in mitxonline, paired with a boolean indicating whether or not the edX enrollment API call was successful
for all of the given course runs
"""
if keep_failed_enrollments is None:
keep_failed_enrollments = settings.FEATURES.get(
features.IGNORE_EDX_FAILURES, False
)

successful_enrollments = []

def send_enrollment_emails():
Expand Down Expand Up @@ -328,7 +334,7 @@ def downgrade_learner(enrollment):
def deactivate_run_enrollment(
run_enrollment,
change_status,
keep_failed_enrollments=False, # noqa: FBT002
keep_failed_enrollments=None,
):
"""
Helper method to deactivate a CourseRunEnrollment
Expand All @@ -338,13 +344,19 @@ def deactivate_run_enrollment(
change_status (str): The change status to set on the enrollment when deactivating
keep_failed_enrollments: (boolean): If True, keeps the local enrollment record
in the database even if the enrollment fails in edX.
If None, defaults to the value of the IGNORE_EDX_FAILURES feature flag.

Returns:
CourseRunEnrollment: The deactivated enrollment
"""
from ecommerce.models import Line # noqa: PLC0415
from hubspot_sync.task_helpers import sync_hubspot_line_by_line_id # noqa: PLC0415

if keep_failed_enrollments is None:
keep_failed_enrollments = settings.FEATURES.get(
features.IGNORE_EDX_FAILURES, False
)

try:
unenroll_edx_course_run(run_enrollment)
except Exception: # pylint: disable=broad-except
Expand Down
122 changes: 122 additions & 0 deletions courses/api_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import pytest
import responses
import reversion
from django.conf import settings
from django.core.exceptions import ValidationError
from django.test import RequestFactory
from edx_api.course_detail import CourseDetail, CourseMode, CourseModes
Expand Down Expand Up @@ -2628,3 +2629,124 @@ def test_program_certificate_verifiable_credentials_signing_payload(
}

assert payload == expected_payload


@pytest.mark.parametrize(
"keep_failed_enrollments,flag_enabled,expected_behavior", # noqa: PT006
[
(
None,
True,
True,
), # Feature flag enabled, parameter None -> keep failed enrollments
(
None,
False,
False,
), # Feature flag disabled, parameter None -> don't keep failed enrollments
(True, False, True), # Explicit True overrides feature flag being False
(False, True, False), # Explicit False overrides feature flag being True
],
)
def test_create_run_enrollments_feature_flag(
mocker,
user,
keep_failed_enrollments,
flag_enabled,
expected_behavior,
):
"""
Test that create_run_enrollments respects the IGNORE_EDX_FAILURES feature flag
when keep_failed_enrollments parameter is None.
"""

num_runs = 2
runs = CourseRunFactory.create_batch(num_runs)

# Mock the feature flag
mocker.patch.dict(
settings.FEATURES,
{"IGNORE_EDX_FAILURES": flag_enabled},
)

# Mock the edX enrollment to fail
exception_cls = EdxApiEnrollErrorException
inner_exception = MockHttpError()
mocker.patch(
"courses.api.enroll_in_edx_course_runs",
side_effect=exception_cls(user, runs[0], inner_exception),
)
mocker.patch("courses.api.log.exception")
mocker.patch("courses.api.mail_api.send_course_run_enrollment_email")

successful_enrollments, edx_request_success = create_run_enrollments(
user,
runs,
keep_failed_enrollments=keep_failed_enrollments,
)

# Verify behavior based on expected_behavior flag
expected_enrollments = num_runs if expected_behavior else 0
assert len(successful_enrollments) == expected_enrollments
assert edx_request_success is False


@pytest.mark.parametrize(
"keep_failed_enrollments,flag_enabled,expected_behavior", # noqa: PT006
[
(
None,
True,
True,
), # Feature flag enabled, parameter None -> keep failed enrollments
(
None,
False,
False,
), # Feature flag disabled, parameter None -> don't keep failed enrollments
(True, False, True), # Explicit True overrides feature flag being False
(False, True, False), # Explicit False overrides feature flag being True
],
)
def test_deactivate_run_enrollment_feature_flag(
mocker,
keep_failed_enrollments,
flag_enabled,
expected_behavior,
):
"""
Test that deactivate_run_enrollment respects the IGNORE_EDX_FAILURES feature flag
when keep_failed_enrollments parameter is None.
"""

# Create test enrollment
enrollment = CourseRunEnrollmentFactory()

# Mock the feature flag
mocker.patch.dict(
settings.FEATURES,
{"IGNORE_EDX_FAILURES": flag_enabled},
)

# Mock the unenroll to fail
mocker.patch(
"courses.api.unenroll_edx_course_run",
side_effect=Exception("edX API failure"),
)
mocker.patch("courses.api.log.exception")

result = deactivate_run_enrollment(
enrollment,
change_status=ENROLL_CHANGE_STATUS_UNENROLLED,
keep_failed_enrollments=keep_failed_enrollments,
)

# Verify behavior based on expected_behavior flag
# When flag is enabled (expected_behavior=True), enrollment should be deactivated despite failure
# When flag is disabled (expected_behavior=False), function returns None
if expected_behavior:
assert result is not None
assert not result.active
assert result.change_status == ENROLL_CHANGE_STATUS_UNENROLLED
else:
assert result is None
6 changes: 4 additions & 2 deletions courses/serializers/v1/courses.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
from __future__ import annotations

from django.conf import settings
from drf_spectacular.utils import (
extend_schema_field,
extend_schema_serializer,
inline_serializer,
)
from mitol.olposthog.features import is_enabled
from rest_framework import serializers
from rest_framework.exceptions import ValidationError

Expand Down Expand Up @@ -181,7 +181,9 @@ def create(self, validated_data):
successful_enrollments, _ = create_run_enrollments(
user,
[run],
keep_failed_enrollments=is_enabled(features.IGNORE_EDX_FAILURES),
keep_failed_enrollments=settings.FEATURES.get(
features.IGNORE_EDX_FAILURES, False
),
)
return successful_enrollments

Expand Down
6 changes: 4 additions & 2 deletions courses/serializers/v2/courses.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@

import logging

from django.conf import settings
from drf_spectacular.utils import extend_schema_field, extend_schema_serializer
from mitol.olposthog.features import is_enabled
from rest_framework import serializers
from rest_framework.exceptions import ValidationError

Expand Down Expand Up @@ -328,7 +328,9 @@ def create(self, validated_data):
successful_enrollments, _ = create_run_enrollments(
user,
[run],
keep_failed_enrollments=is_enabled(features.IGNORE_EDX_FAILURES),
keep_failed_enrollments=settings.FEATURES.get(
features.IGNORE_EDX_FAILURES, False
),
)
return successful_enrollments

Expand Down
13 changes: 10 additions & 3 deletions courses/views/v1/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from typing import Tuple, Union # noqa: UP035

import django_filters
from django.conf import settings
from django.contrib.auth.models import User
from django.contrib.contenttypes.models import ContentType
from django.db import transaction
Expand Down Expand Up @@ -344,7 +345,9 @@ def post(self, request):
_, edx_request_success = create_run_enrollments(
user=user,
runs=[run],
keep_failed_enrollments=is_enabled(features.IGNORE_EDX_FAILURES),
keep_failed_enrollments=settings.FEATURES.get(
features.IGNORE_EDX_FAILURES, False
),
)

def respond(data, status=True): # noqa: FBT002
Expand All @@ -355,7 +358,9 @@ def respond(data, status=True): # noqa: FBT002
return Response("Ok" if status else "Fail")
return HttpResponseRedirect(data)

if edx_request_success or is_enabled(features.IGNORE_EDX_FAILURES):
if edx_request_success or settings.FEATURES.get(
features.IGNORE_EDX_FAILURES, False
):
resp = respond(reverse("user-dashboard"))
cookie_value = {
"type": USER_MSG_TYPE_ENROLLED,
Expand Down Expand Up @@ -441,7 +446,9 @@ def destroy(self, request, *args, **kwargs): # noqa: ARG002
deactivated_enrollment = deactivate_run_enrollment(
enrollment,
change_status=ENROLL_CHANGE_STATUS_UNENROLLED,
keep_failed_enrollments=is_enabled(features.IGNORE_EDX_FAILURES),
keep_failed_enrollments=settings.FEATURES.get(
features.IGNORE_EDX_FAILURES, False
),
)
if deactivated_enrollment is None:
return Response(status=status.HTTP_400_BAD_REQUEST)
Expand Down
21 changes: 18 additions & 3 deletions courses/views/v2/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@
Course API Views version 2
"""

import contextlib
import logging

import django_filters
from django.conf import settings
from django.db.models import Count, Prefetch, Q
from django.http import JsonResponse
from django.shortcuts import get_object_or_404
Expand Down Expand Up @@ -70,6 +71,8 @@
from openedx.api import sync_enrollments_with_edx
from openedx.constants import EDX_ENROLLMENT_AUDIT_MODE, EDX_ENROLLMENT_VERIFIED_MODE

log = logging.getLogger(__name__)


class Pagination(PageNumberPagination):
"""Paginator class for infinite loading"""
Expand Down Expand Up @@ -513,8 +516,18 @@ def get_serializer_context(self):
def list(self, request, *args, **kwargs):
"""List user enrollments with optional sync."""
if is_enabled(features.SYNC_ON_DASHBOARD_LOAD):
with contextlib.suppress(Exception):
ignore_edx_failures = settings.FEATURES.get(
features.IGNORE_EDX_FAILURES, False
)
try:
sync_enrollments_with_edx(self.request.user)
except Exception: # pylint: disable=broad-except
log.exception(
"Failed to sync enrollments with edX for user: %s",
self.request.user.id,
)
if not ignore_edx_failures:
raise
Comment on lines +520 to +530
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: Inconsistent feature flag mechanisms are used. Enabling SYNC_ON_DASHBOARD_LOAD via PostHog without setting IGNORE_EDX_FAILURES in Django settings will cause dashboard load failures if edX sync fails.
Severity: HIGH

Suggested Fix

Unify the feature flag checks. Either check both flags using the same mechanism (e.g., both via PostHog or both via Django settings) or ensure the IGNORE_EDX_FAILURES flag is also checked via PostHog alongside SYNC_ON_DASHBOARD_LOAD. Additionally, add a test case to cover the scenario where sync is enabled, the ignore flag is disabled, and an error occurs.

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/views/v2/__init__.py#L516-L530

Potential issue: The code uses two different feature flag systems within the same
function. The `SYNC_ON_DASHBOARD_LOAD` feature is checked using PostHog's `is_enabled`
function, while the `IGNORE_EDX_FAILURES` feature is checked via Django's
`settings.FEATURES`. If the sync feature is enabled for a user via PostHog, but the
`IGNORE_EDX_FAILURES` setting is not explicitly configured to `True` in Django settings
(defaulting to `False`), any exception during the `sync_enrollments_with_edx` call will
be re-raised. This will cause the user's dashboard to fail to load whenever the edX API
is unavailable or returns an error.

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

return super().list(request, *args, **kwargs)

@extend_schema(
Expand All @@ -535,7 +548,9 @@ def destroy(self, request, *args, **kwargs): # noqa: ARG002
deactivated_enrollment = deactivate_run_enrollment(
enrollment,
change_status=ENROLL_CHANGE_STATUS_UNENROLLED,
keep_failed_enrollments=is_enabled(features.IGNORE_EDX_FAILURES),
keep_failed_enrollments=settings.FEATURES.get(
features.IGNORE_EDX_FAILURES, False
),
)
if deactivated_enrollment is None:
return Response(status=status.HTTP_400_BAD_REQUEST)
Expand Down
Loading