feat: replace enterprise support enrollment imports with filter call - #462
feat: replace enterprise support enrollment imports with filter call#462brobro10000 wants to merge 1 commit into
Conversation
2deee1b to
ad89114
Compare
Sibling of the openedx/openedx-platform PR of the same name, carrying the same call-site change to this deployable fork so it can go through stage/prod ahead of the upstream merge, per the enterprise plugin ticket runbook. Removes the direct openedx.features.enterprise_support imports and the _enterprise_course_enrollments_by_course_id helper from EnrollmentSupportListView, replacing them with a call to the new SupportEnrollmentDataRequested openedx-filter. Per pwnage101's review feedback on the sibling SupportContactContextRequested filter, run_filter now returns a tuple of all its inputs (enrollment_data, user) rather than a bare dict, matching the convention used by every other filter. No settings changes here: OPEN_EDX_FILTERS_CONFIG registration for the pipeline step lives in edx-enterprise's own plugin_settings() (enterprise/settings/common.py), per the ENT-11830 ownership handoff. ENT-11574
ad89114 to
2ddc176
Compare
| enterprise_course_enrollments = enterprise_enrollments_by_course_id.get(enrollment['course_id'], []) | ||
| enrollment['enterprise_course_enrollments'] = enterprise_course_enrollments | ||
| enterprise_enrollments_by_course_id, _ = SupportEnrollmentDataRequested.run_filter( | ||
| enrollment_data={}, user=user |
There was a problem hiding this comment.
❌ Hm, this is a big red flag that we're passing an empty dict to the filter. You should be passing something useful, like enrollments. Think of it like this: what would a non-enterprise implementer of a pipeline step for this filter need if it needed to augment the enrollments data, not just add a hard-coded "enterprise_course_enrollment" key?
| for enrollment in enrollments: | ||
| enrollment['enterprise_course_enrollments'] = enterprise_enrollments_by_course_id.get( | ||
| enrollment['course_id'], [] | ||
| ) |
There was a problem hiding this comment.
❌ delete this whole for-loop. You only needed this because you didn't rely on the filter to augment the enrollments values. Change the fundamental design of the filter to make in-place changes to enrollments, then remove this loop. Another litmus test for whether you've successfully de-enterprised the platform is whether the resulting file after your changes doesn't contain the word "enterprise" at all.
| for enrollment in enrollments: | |
| enrollment['enterprise_course_enrollments'] = enterprise_enrollments_by_course_id.get( | |
| enrollment['course_id'], [] | |
| ) |
ENT-11574
Sibling of the openedx/openedx-platform PR of the same name, carrying the same call-site
change to this deployable fork so it can go through stage/prod ahead of the upstream merge,
per the enterprise plugin ticket runbook. Devstack only boots this repo (not
openedx/openedx-platform), so this is also the branch used for local integration testing.
Companion PR to the already-open support-contact-tag PR set (openedx-filters#390,
edx-enterprise#2688, openedx-platform#39076, edx-platform#455) — this one covers the
other half of ENT-11574's acceptance criteria: enterprise enrollment data for the support
enrollment view.
Removes the direct
openedx.features.enterprise_supportimports and the_enterprise_course_enrollments_by_course_idhelper fromEnrollmentSupportListView,replacing them with a call to the new
SupportEnrollmentDataRequestedopenedx-filter. Nosettings changes in this PR:
OPEN_EDX_FILTERS_CONFIGregistration for the pipeline steplives entirely in edx-enterprise's own
plugin_settings()(enterprise/settings/common.py),per the
ENT-11830ownership handoff — already merged in this repo (#281) well before thisbranch existed.
lms/djangoapps/support/views/contact_us.pyis untouched — this PR is scoped to theenrollment filter only.
Related PRs
openedx-platform#39076, edx-platform#455 (support-contact-tag)
Merge order (per the enterprise plugin ticket runbook)
Merge this after local devstack testing and before the openedx/openedx-platform PR.
Auto-deploys to stage on merge — test in stage, then deploy to prod and confirm working, before
the openedx-platform PR is rebased and merged.
CI note
CI here is expected to be red on Django-app-boot-dependent jobs until openedx-filters and
edx-enterprise are released and this repo's requirements pin is bumped — CI installs the
published PyPI versions, which don't yet contain
SupportEnrollmentDataRequested/the newpipeline step. Not a bug in this diff — it self-resolves per the runbook's release ordering.
Testing
Same test change as the openedx-platform PR:
SupportEnrollmentDataRequested.run_filterismocked at the call site in
lms/djangoapps/support/tests/test_views.py; the pipeline-stepbehavior itself is covered by edx-enterprise's own test suite.
Local devstack integration testing (with the openedx-filters, edx-enterprise, and this branch
checked out together) is required before merging.