Skip to content

feat: replace enterprise_support import with GradeEventContextRequested filter #38097

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
pwnage101 wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat: replace enterprise_support import with GradeEventContextRequested filter #38097

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b8cd931
feat: replace enterprise_support import with GradeEventContextRequest…
kiram15 May 26, 2026
30924b9
Merge remote-tracking branch 'origin/master' into pwnage101/ENT-11563
kiram15 May 28, 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
23 changes: 19 additions & 4 deletions lms/djangoapps/grades/events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
UserPersonalData,
)
from openedx_events.learning.signals import CCX_COURSE_PASSING_STATUS_UPDATED, COURSE_PASSING_STATUS_UPDATED
from openedx_filters.learning.filters import GradeEventContextRequested

from common.djangoapps.course_modes.models import CourseMode
from common.djangoapps.student.models import CourseEnrollment
Expand All @@ -27,7 +28,6 @@
)
from lms.djangoapps.grades.signals.signals import SCHEDULE_FOLLOW_UP_SEGMENT_EVENT_FOR_COURSE_PASSED_FIRST_TIME
from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
from openedx.features.enterprise_support.context import get_enterprise_event_context

log = getLogger(__name__)

Expand Down Expand Up @@ -166,10 +166,25 @@ def course_grade_passed_first_time(user_id, course_id):
"""
event_name = COURSE_GRADE_PASSED_FIRST_TIME_EVENT_TYPE
context = contexts.course_context_from_course_id(course_id)
context_enterprise = get_enterprise_event_context(user_id, course_id)
context.update(context_enterprise)
try:
filtered_context, _, _ = GradeEventContextRequested.run_filter(
context=context, user_id=user_id, course_id=course_id
)
except Exception: # pylint: disable=broad-except
Copy link
Copy Markdown
Contributor

@feanil feanil May 27, 2026

Choose a reason for hiding this comment

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

Why the broad except here? Can we narrow this down and define the exceptions we expect from processors of this filter? We do this with other filters where we are explicit about the exceptions we expect to come out of them: https://docs.openedx.org/projects/openedx-filters/en/latest/reference/filters.html#openedx_filters.learning.filters.AccountSettingsRenderStarted

Copy link
Copy Markdown
Contributor

@kiram15 kiram15 May 27, 2026

Choose a reason for hiding this comment

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

Since we intend all of this to be fail open behavior to catch all problems (misconfigured filter pipeline, runtime errors from downstream enrichment, etc) and still be able to see these problems while not effecting the original flow, I feel like enumerating every error that could go wrong for ultimately just a log line feels brittle and unmaintainable.

Copy link
Copy Markdown
Contributor Author

@pwnage101 pwnage101 May 28, 2026

Choose a reason for hiding this comment

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

Ideally we should only catch OpenEdxFilterException subclasses. For now I'm going to allow this because we've been running this in prod for weeks and just want the code to match, but next time lets stick to the strict interface of the filter.

log.exception(
'GradeEventContextRequested failed for user_id=%s course_id=%s',
user_id,
course_id,
)
raise
log.debug(
'GradeEventContextRequested succeeded for user_id=%s course_id=%s',
user_id,
course_id,
)

# TODO (AN-6134): remove this context manager
with tracker.get_tracker().context(event_name, context):
with tracker.get_tracker().context(event_name, filtered_context):
tracker.emit(
event_name,
{
Expand Down
81 changes: 80 additions & 1 deletion lms/djangoapps/grades/tests/test_events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"""

from unittest import mock
from unittest.mock import patch

from ccx_keys.locator import CCXLocator
from django.utils.timezone import now
Expand Down Expand Up @@ -34,7 +35,7 @@

class PersistentGradeEventsTest(OpenEdxEventsTestMixin, SharedModuleStoreTestCase):
"""
Tests for the Open edX Events associated with the persistant grade process through the update_or_create method.
Tests for the Open edX Events associated with the persistent grade process through the update_or_create method.

This class guarantees that the following events are sent during the user updates their grade, with
the exact Data Attributes as the event definition stated:
Expand Down Expand Up @@ -229,3 +230,81 @@
},
event_receiver.call_args.kwargs,
)


class GradeEventContextFilterTest(SharedModuleStoreTestCase):
"""
Tests that course_grade_passed_first_time invokes the GradeEventContextRequested
filter and uses the returned context directly.
"""

def setUp(self):
super().setUp()
self.user = UserFactory.create()
self.course = CourseFactory.create()

@patch('lms.djangoapps.grades.events.tracker')
@patch('lms.djangoapps.grades.events.contexts.course_context_from_course_id')
@patch('lms.djangoapps.grades.events.GradeEventContextRequested.run_filter')
def test_filter_called_with_context(
self,
mock_run_filter,
mock_course_context_from_course_id,
mock_tracker,
):
"""
course_grade_passed_first_time should call
GradeEventContextRequested.run_filter and use the returned context.
"""
original_context = {"course_id": str(self.course.id)}
filtered_context = {
"course_id": str(self.course.id),
"org": "test_org",
"enterprise_uuid": "abc-123",
}
mock_course_context_from_course_id.return_value = original_context
mock_run_filter.return_value = (filtered_context, self.user.id, self.course.id)

from lms.djangoapps.grades.events import course_grade_passed_first_time

course_grade_passed_first_time(self.user.id, self.course.id)

mock_run_filter.assert_called_once_with(
context=original_context,
user_id=self.user.id,
course_id=self.course.id,
)
mock_tracker.get_tracker.return_value.context.assert_called_once_with(
'edx.course.grade.passed.first_time',
filtered_context,
)

@patch('lms.djangoapps.grades.events.log')
@patch('lms.djangoapps.grades.events.tracker')
@patch('lms.djangoapps.grades.events.contexts.course_context_from_course_id')
@patch('lms.djangoapps.grades.events.GradeEventContextRequested.run_filter')
def test_filter_exception_is_logged_and_raised(
self,
mock_run_filter,
mock_course_context_from_course_id,
mock_tracker,
mock_log,
):
"""
If run_filter raises an exception, it should be logged and re-raised.
"""
original_context = {"course_id": str(self.course.id)}
mock_course_context_from_course_id.return_value = original_context
mock_run_filter.side_effect = Exception("boom")

from lms.djangoapps.grades.events import course_grade_passed_first_time

with self.assertRaisesRegex(Exception, "boom"):

Check failure on line 302 in lms/djangoapps/grades/tests/test_events.py

View workflow job for this annotation

GitHub Actions / Quality Others (ubuntu-24.04, 3.12, 20)

ruff (PT027) 

lms/djangoapps/grades/tests/test_events.py:302:14: PT027 Use `pytest.raises` instead of unittest-style `assertRaisesRegex`
  help: Replace `assertRaisesRegex` with `pytest.raises`
course_grade_passed_first_time(self.user.id, self.course.id)

mock_log.exception.assert_called_once_with(
'GradeEventContextRequested failed for user_id=%s course_id=%s',
self.user.id,
self.course.id,
)
mock_tracker.get_tracker.return_value.context.assert_not_called()
2 changes: 1 addition & 1 deletion requirements/constraints.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ django-stubs<6
# The team that owns this package will manually bump this package rather than having it pulled in automatically.
# This is to allow them to better control its deployment and to do it in a process that works better
# for them.
edx-enterprise==8.0.14
edx-enterprise==8.0.15

# Date: 2023-07-26
# Our legacy Sass code is incompatible with anything except this ancient libsass version.
Expand Down
2 changes: 1 addition & 1 deletion requirements/edx/base.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,7 @@ edx-drf-extensions==10.6.0
# enterprise-integrated-channels
# openedx-authz
# openedx-core
edx-enterprise==8.0.14
edx-enterprise==8.0.15
# via
# -c requirements/constraints.txt
# -r requirements/edx/kernel.in
Expand Down
2 changes: 1 addition & 1 deletion requirements/edx/development.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -756,7 +756,7 @@ edx-drf-extensions==10.6.0
# enterprise-integrated-channels
# openedx-authz
# openedx-core
edx-enterprise==8.0.14
edx-enterprise==8.0.15
# via
# -c requirements/constraints.txt
# -r requirements/edx/doc.txt
Expand Down
2 changes: 1 addition & 1 deletion requirements/edx/doc.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,7 +573,7 @@ edx-drf-extensions==10.6.0
# enterprise-integrated-channels
# openedx-authz
# openedx-core
edx-enterprise==8.0.14
edx-enterprise==8.0.15
# via
# -c requirements/constraints.txt
# -r requirements/edx/base.txt
Expand Down
2 changes: 1 addition & 1 deletion requirements/edx/testing.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,7 +589,7 @@ edx-drf-extensions==10.6.0
# enterprise-integrated-channels
# openedx-authz
# openedx-core
edx-enterprise==8.0.14
edx-enterprise==8.0.15
# via
# -c requirements/constraints.txt
# -r requirements/edx/base.txt
Expand Down
Loading