fix: change SupportEnrollmentDataRequested to accept/return full enrollments list - #396
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
6aa8e00 to
23abae8
Compare
c223c91 to
835f22f
Compare
| def run_filter(cls, enrollment_data: dict, user: Any) -> tuple[dict, Any]: | ||
| def run_filter(cls, enrollments: list, user: Any) -> tuple[list, Any]: |
There was a problem hiding this comment.
There's no reason to remove the "_data" suffix from the argument name. Conventionally, that signifies the value represents a serialized object, typically a dict, rather than an ORM object or other semantic type. I think the only change to the name should be to make it plural, and enhance the type: enrollments_data: list[dict]
There was a problem hiding this comment.
Fixed — renamed back to enrollments_data: list[dict], keeping the _data suffix convention. run_pipeline/return also use enrollments_data now.
| enrollment_data (dict): dict mapping course_id to list of enrollment records. | ||
| enrollments (list): list of enrollment record dicts, each keyed by course_id. |
There was a problem hiding this comment.
This should explain what keys to expect inside the serialized dict. openedx-filters is the plugin-facing interface, treating the platform as a black box. Plugin implementers (i.e. pipeline step implementers) should not necessarily need to spelunk platform code to understand what are the most essential keys available in the dict.
Therefore, at minimum you should document some of the most important dict keys, e.g. course_id, course_start/end, enrollment_start/end, course_modes, etc. Maybe document it in the form of a sample payload. Finally I'd recommend linking directly to the serialization code within this docstring for convenience.
There was a problem hiding this comment.
Fixed — the docstring now lists the expected keys (course_id, course_start, course_end, enrollment_start, enrollment_end, course_modes) and links to edx-platform's serialization code for the full shape.
There was a problem hiding this comment.
Updated with real, verified keys instead of guessed ones — checked openedx.core.djangoapps.enrollments.api.get_enrollments's own docstring (which has a sample payload) plus what EnrollmentSupportListView.get adds before calling this filter (verified_price, verified_upgrade_deadline, verification_deadline, order_number, source_system, manual_enrollment), and linked to the live serialization code.
There was a problem hiding this comment.
Restructured as a literal example payload (matching "sample payload") instead of prose — verified against get_enrollments's own doctest example plus what EnrollmentSupportListView.get adds before calling this filter (verified_price, verified_upgrade_deadline, verification_deadline, order_number, source_system, manual_enrollment).
df4080c to
29f964a
Compare
…llments list run_filter now takes the real enrollments list and returns it, instead of an empty placeholder dict, so pipeline steps can augment enrollment records in place instead of only appending a single hard-coded key. ENT-11574
29f964a to
c4dc8d9
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The breaking payload change must use a new filter major version or a coordinated major migration.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates the support enrollment filter to process complete enrollment records for downstream enrichment.
Changes:
- Replaces the placeholder mapping with an enrollment list.
- Updates tests and API documentation.
- Bumps the package version and changelog.
File summaries
| File | Description |
|---|---|
openedx_filters/learning/filters.py |
Changes the filter payload and return type. |
openedx_filters/learning/tests/test_filters.py |
Updates tests for the new payload. |
openedx_filters/__init__.py |
Bumps the version to 3.13.0. |
CHANGELOG.rst |
Documents the interface change. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Description
Addresses review feedback (and the follow-up) on the sibling edx-platform PR #462:
SupportEnrollmentDataRequested's original shape was a "red flag" — it passed an empty placeholder dict to the filter, which meant pipeline steps could only ever bolt on one hard-coded key, and forced the platform view to manually zip the filter's results back onto each enrollment record via a for-loop afterward.run_filternow takes the realenrollmentslist (already carrying every field the view has computed so far) and returns it back, so pipeline steps can augment any part of each enrollment record in place, not just append a fixed key.userstays as an explicit argument here (unlike the sibling contact filter's fix) — this is a support tool where staff look up other users' enrollments, socrum's current-request user would be the wrong value.Since the original shape (#393) already merged and released, this is a new PR rather than an amendment.
Companion PRs
ENT-11574