feat: add GradeEventContextRequested filter for grade analytics enrichment#333
Conversation
…hment Introduces a new GradeEventContextRequested filter (org.openedx.learning.grade.context.requested.v1) that allows pipeline steps to enrich the context dict emitted with grade analytics events. Adds unit tests confirming correct filter type and pipeline behavior. ENT-11563 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| dict: the context dict, possibly enriched by pipeline steps. | ||
| """ | ||
| data = super().run_pipeline(context=context, user_id=user_id, course_id=course_id) | ||
| return data.get("context") |
There was a problem hiding this comment.
It's unconventional for the return signature to differ from the arguments signature. Better would be:
| return data.get("context") | |
| return data.get("context"), data.get("user_id"), data.get("course_id") |
kiram15
force-pushed
the
pwnage101/ENT-11563
branch
2 times, most recently
from
29045b3 to
bb9efce
Compare
| filter_type = "org.openedx.learning.grade.context.requested.v1" | ||
|
|
||
| @classmethod | ||
| def run_filter(cls, context: dict, user_id: int, course_id: str) -> tuple[dict | None, int | None, str | None]: |
There was a problem hiding this comment.
the return type should be more strict by not letting non-None input values be converted to None.
| def run_filter(cls, context: dict, user_id: int, course_id: str) -> tuple[dict | None, int | None, str | None]: | |
| def run_filter(cls, context: dict, user_id: int, course_id: str) -> tuple[dict, int, str]: |
| course_id (str): The identifier of the course. | ||
|
|
||
| Returns: | ||
| tuple[dict | None, int | None, str | None]: |
There was a problem hiding this comment.
| tuple[dict | None, int | None, str | None]: | |
| tuple[dict, int, str]: |
There was a problem hiding this comment.
While we're in here, I always thought this was a bug but was too late to comment on it. Unless there's an exceptional reason, I think Input types should match output types.
def run_filter(cls, readonly_fields: set, user: Any) -> tuple[set, Any]:
kiram15
force-pushed
the
pwnage101/ENT-11563
branch
from
bb9efce to
d08c9f7
Compare
| str: The course identifier. | ||
| """ | ||
| data = super().run_pipeline(context=context, user_id=user_id, course_id=course_id) | ||
| return data.get("context"), data.get("user_id"), data.get("course_id") |
There was a problem hiding this comment.
As discussed, lets just make this strict:
| return data.get("context"), data.get("user_id"), data.get("course_id") | |
| return data["context"], data["user_id"], data["course_id"] |
That way, if the pipeline calling code can't tolerate None values from a buggy filter, then at least we catch the error sooner. Now the error (KeyError) happens closer to the root issue inside the pipeline itself, rather than after the entire pipeline has finished and returned to openedx-platform, potentially throwing a TypeError/AttributeError resulting from working with None values unexpectedly.
kiram15
force-pushed
the
pwnage101/ENT-11563
branch
from
d08c9f7 to
1849e0e
Compare
kiram15
force-pushed
the
pwnage101/ENT-11563
branch
from
18fbb54 to
f3686fb
Compare
|
✅ LGTM |
kiram15
force-pushed
the
pwnage101/ENT-11563
branch
from
1ddc9ba to
870eca9
Compare
| ], | ||
| include_package_data=True, | ||
| install_requires=load_requirements("requirements/base.in"), | ||
| python_requires=">=3.12", |
There was a problem hiding this comment.
We aren't using any python 3.12 specific functionality, and this allows edx-platform to be able to pick up the new openedx-filters versions that are right now incompatible with edx-platform's python version of 3.11.
kiram15
force-pushed
the
pwnage101/ENT-11563
branch
from
3f30ebe to
0cfde64
Compare
felipemontoya
left a comment
felipemontoya
left a comment
There was a problem hiding this comment.
This looks very clean and the accompanying platform PR is also clear and with good structure.
I think removing one more stitch in enterprise dependency while at the same time allowing others to enrich the grade analytics is completely welcomed.
|
|
||
| @classmethod | ||
| def run_filter(cls, readonly_fields: set, user: Any) -> tuple[Any, Any]: | ||
| def run_filter(cls, readonly_fields: set, user: Any) -> tuple[set, Any]: |
There was a problem hiding this comment.
[info] this is coming from the last PR that added this filter where we agreed to this after the PR was merged.
3 participants
Introduces a new GradeEventContextRequested filter (org.openedx.learning.grade.context.requested.v1) that allows pipeline steps to enrich the context dict emitted with grade analytics events. Adds unit tests confirming correct filter type and pipeline behavior.
ENT-11563
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
Blocks: