Skip to content

feat: add EnterpriseEnrollmentPostProcessor pipeline step for CourseEnrollmentStarted filter#2553

Draft
pwnage101 wants to merge 2 commits intomasterfrom
pwnage101/ENT-11570
Draft

feat: add EnterpriseEnrollmentPostProcessor pipeline step for CourseEnrollmentStarted filter#2553
pwnage101 wants to merge 2 commits intomasterfrom
pwnage101/ENT-11570

Conversation

@pwnage101
Copy link
Copy Markdown
Contributor

@pwnage101 pwnage101 commented Mar 5, 2026

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 22, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.93%. Comparing base (daa14a2) to head (cd354da).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2553      +/-   ##
==========================================
+ Coverage   85.91%   85.93%   +0.01%     
==========================================
  Files         250      251       +1     
  Lines       16604    16627      +23     
  Branches     1639     1640       +1     
==========================================
+ Hits        14266    14289      +23     
  Misses       2001     2001              
  Partials      337      337              
Flag Coverage Δ
unittests 85.93% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kiram15 kiram15 force-pushed the pwnage101/ENT-11570 branch from a35d99d to ceabb63 Compare April 23, 2026 20:39
@kiram15 kiram15 requested a review from Copilot April 23, 2026 20:49
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds an EnterpriseEnrollmentPostProcessor pipeline step to post enterprise enrollment + consent on course enrollment events, with accompanying unit tests.

Changes:

  • Introduces enterprise.filters.enrollment.EnterpriseEnrollmentPostProcessor pipeline step.
  • Adds unit tests covering enterprise vs non-enterprise paths and exception logging behavior.
  • Adds minimal tests/filters package init.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
enterprise/filters/enrollment.py New pipeline step that posts enterprise enrollment and consent, logging failures.
enterprise/filters/init.py Initializes enterprise.filters package.
tests/filters/test_enrollment.py New tests validating API client calls and exception logging.
tests/filters/init.py Initializes tests.filters package.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +28 to +39
def _make_openedx_modules():
"""
Build a minimal set of sys.modules entries for the openedx namespace.
"""
entries = {}
for name in (
"openedx",
"openedx.features",
"openedx.features.enterprise_support",
):
entries[name] = ModuleType(name)
return entries
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

These ModuleType stubs won't behave like packages during import because they lack __path__. As a result, from openedx.features.enterprise_support.api import ... can fail with ModuleNotFoundError: 'openedx' is not a package in environments where openedx isn't installed. Fix by marking the namespace modules as packages (e.g., setting __path__ = [] on each) and/or wiring submodules as attributes on their parents so the import machinery can resolve the hierarchy.

Copilot uses AI. Check for mistakes.
"""
user = UserFactory.build(username="regular-user")
course_key = MagicMock()
course_key.__str__ = lambda self: "course-v1:org+course+run"
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

Overriding __str__ with a lambda on a MagicMock is brittle because special methods are typically resolved on the type, not the instance, and mocking frameworks already provide __str__. Prefer configuring the mock’s __str__ as a mock return value (or just pass a real string/CourseKey) to make the test more reliable.

Suggested change
course_key.__str__ = lambda self: "course-v1:org+course+run"
course_key.__str__.return_value = "course-v1:org+course+run"

Copilot uses AI. Check for mistakes.
Comment on lines +32 to +36
enterprise_customer_users = EnterpriseCustomerUser.objects.filter(user=user)
if not enterprise_customer_users.exists():
return {'user': user, 'course_key': course_key, 'mode': mode}

enterprise_customer_user = enterprise_customer_users.first()
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

This does two DB queries (exists() and then first()). You can reduce to a single query by fetching the first row and checking for None (and optionally select_related('enterprise_customer') since you immediately access enterprise_customer.uuid). This will be more efficient on the enrollment path.

Suggested change
enterprise_customer_users = EnterpriseCustomerUser.objects.filter(user=user)
if not enterprise_customer_users.exists():
return {'user': user, 'course_key': course_key, 'mode': mode}
enterprise_customer_user = enterprise_customer_users.first()
enterprise_customer_user = (
EnterpriseCustomerUser.objects.select_related('enterprise_customer')
.filter(user=user)
.first()
)
if enterprise_customer_user is None:
return {'user': user, 'course_key': course_key, 'mode': mode}

Copilot uses AI. Check for mistakes.
@kiram15 kiram15 force-pushed the pwnage101/ENT-11570 branch from ceabb63 to cd354da Compare April 24, 2026 05:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants