feat: add GradeEventContextEnricher pipeline step for grade analytics#2543
feat: add GradeEventContextEnricher pipeline step for grade analytics#2543
Conversation
Introduces enterprise/filters/grades.py with GradeEventContextEnricher, a PipelineStep for the org.openedx.learning.grade.context.requested.v1 filter that enriches grade analytics event context with the learner's enterprise UUID. Adds openedx-filters as a dependency and unit tests covering both enrichment and no-op branches. ENT-11563 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
pwnage101
left a comment
There was a problem hiding this comment.
Aside from these changes, please also remember to bump the version of edx-enterprise.
Apologies if this wasn't clear, but these draft claude-generated PRs were meant to offer guidance and direction, not to be ready to go as-is.
| no enterprise course enrollment, the context is returned unchanged. | ||
| """ | ||
|
|
||
| def run_filter(self, context, user_id, course_id): # pylint: disable=arguments-differ |
There was a problem hiding this comment.
Add python type hints
| def run_filter(self, context, user_id, course_id): # pylint: disable=arguments-differ | |
| def run_filter(self, context: dict, user_id: int, course_id: str) -> dict[str, Any]: # pylint: disable=arguments-differ |
| Arguments: | ||
| context (dict): the event tracking context dict. | ||
| user_id (int): the ID of the user whose grade event is being emitted. | ||
| course_id (str or CourseKey): the course key for the grade event. |
There was a problem hiding this comment.
Per the filter definition, it's impossible for course_id to be a CourseKey: https://github.com/openedx/openedx-filters/blob/d4b09fdb850436bbcea638066c124a47b5025084/openedx_filters/learning/filters.py#L1584
Fixed docstring:
| course_id (str or CourseKey): the course key for the grade event. | |
| course_id (str): the course key for the grade event. |
| course_id (str or CourseKey): the course key for the grade event. | ||
|
|
||
| Returns: | ||
| dict: updated pipeline data with the enriched ``context`` dict. |
There was a problem hiding this comment.
Be more specific about the shape of the output dict.
| dict: updated pipeline data with the enriched ``context`` dict. | |
| dict: updated pipeline data with the enriched ``context`` dict. | |
| { | |
| "context": <enriched context>, | |
| "user_id": <unchanged>, | |
| "course_id": <unchanged>, | |
| } |
| ) | ||
| if uuids: | ||
| return {"context": {**context, "enterprise_uuid": str(uuids[0])}} | ||
| return {"context": context} |
There was a problem hiding this comment.
Missing passthrough values and should probably just utilize updated_context
| return {"context": context} | |
| return {"context": context, "user_id": user_id, "course_id": course_id} |
| str(course_id), | ||
| ) | ||
| if uuids: | ||
| return {"context": {**context, "enterprise_uuid": str(uuids[0])}} |
There was a problem hiding this comment.
- This lacks passthrough values, 2. probably shouldn't just be an early-return since that's less maintainable, and 3. leverages complicated destructuring instead of something more pythonic and something which allows us to retain one exit point of the function. Solve all 3 problems at once:
| return {"context": {**context, "enterprise_uuid": str(uuids[0])}} | |
| context = deepcopy(context) # create a copy to avoid altering the passed-in value. | |
| # Warning: Selecting the first element is not likely deterministic! | |
| context["enterprise_uuid"] = str(uuids[0]) |
| # via -r requirements/test-master.txt | ||
| openedx-events==10.5.0 | ||
| # via -r requirements/test-master.txt | ||
| openedx-filters==2.1.0 |
There was a problem hiding this comment.
❌ 3.3.0 now supports python 3.11. We should take this opportunity to deploy with the latest version.
| openedx-filters==2.1.0 | |
| openedx-filters==3.3.0 |
Ditto for the rest of the requirements changes in this PR.
| uuids = EnterpriseCourseEnrollment.get_enterprise_uuids_with_user_and_course( | ||
| str(user_id), | ||
| str(course_id), | ||
| ) |
There was a problem hiding this comment.
user_idcan stay anint. I looked at the function, it's okay.- course_id can be assumed to be a
str, that's promised by the type hint.
| uuids = EnterpriseCourseEnrollment.get_enterprise_uuids_with_user_and_course( | |
| str(user_id), | |
| str(course_id), | |
| ) | |
| uuids = EnterpriseCourseEnrollment.get_enterprise_uuids_with_user_and_course(user_id, course_id) |
| @patch( | ||
| "enterprise.filters.grades.EnterpriseCourseEnrollment" | ||
| ".get_enterprise_uuids_with_user_and_course" | ||
| ) |
There was a problem hiding this comment.
- Overly strict line length limits.
- Unnecessary patching of module at import (
enterprise.filters.grades) instead of source (enterprise.models).
| @patch( | |
| "enterprise.filters.grades.EnterpriseCourseEnrollment" | |
| ".get_enterprise_uuids_with_user_and_course" | |
| ) | |
| @patch("enterprise.models.EnterpriseCourseEnrollment.get_enterprise_uuids_with_user_and_course") |
Ditto for other tests.
| return GradeEventContextEnricher( | ||
| "org.openedx.learning.grade.context.requested.v1", | ||
| [], | ||
| ) |
There was a problem hiding this comment.
Use argument names to improve readability:
| return GradeEventContextEnricher( | |
| "org.openedx.learning.grade.context.requested.v1", | |
| [], | |
| ) | |
| return GradeEventContextEnricher( | |
| filter_type="org.openedx.learning.grade.context.requested.v1", | |
| running_pipeline=[], | |
| ) |
Introduces enterprise/filters/grades.py with GradeEventContextEnricher, a PipelineStep for the org.openedx.learning.grade.context.requested.v1 filter that enriches grade analytics event context with the learner's enterprise UUID. Adds openedx-filters as a dependency and unit tests covering both enrichment and no-op branches.
ENT-11563
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
Blocked by:
Blocks: