feat: replace enterprise support contact-tag import with filter call - #455
feat: replace enterprise support contact-tag import with filter call#455brobro10000 wants to merge 1 commit into
Conversation
68d03c8 to
f47a2ac
Compare
brobro10000
changed the title
8b6d59a to
be4d946
Compare
| enterprise_customer = enterprise_api.enterprise_customer_for_request(request) | ||
| if enterprise_customer: | ||
| tags.append('enterprise_learner') | ||
| tags, _ = SupportContactContextRequested.run_filter(tags=tags, user=request.user) | ||
|
|
||
| context['tags'] = tags | ||
|
|
There was a problem hiding this comment.
Crap I just realized the naming and shape of this filter is completely wrong. The context is not being requested, the tags are being requested. Either:
- The name of this filter should be changed to SupportContactTagsContextRequested, or
- The filter call should come AFTER the
context['tags'] = tagsline and accept an entire context dictionary:context, _ = SupportContactContextRequested.run_filter(context=context, user=request.user)
I'm partial to the latter because it's more generally useful outside of enterprise.
This is unfortunately going to require patching both the openedx-filters and edx-enterprise related code, and re-releasing them, but now is the best time (before the filter is in-use).
There was a problem hiding this comment.
Also, I just realized since we're passing request.user, we don't even need to pass the user because it's available to the pipeline via crum, so the final call should look even simpler:
context = SupportContactContextRequested.run_filter(context=context)There was a problem hiding this comment.
Good catch — addressed. Moved the filter call after context['tags'] = tags and changed SupportContactContextRequested.run_filter to accept/return the whole context dict instead of a bare tags list (also dropped the unused user arg per your follow-up comment). Landed in openedx-filters #394 and edx-enterprise openedx#2691 (both merged and released — 3.11.0 / 8.11.0), and this PR's call site + pins are now updated to match.
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. Swaps the direct openedx.features.enterprise_support import in the support contact-us view for a call to the SupportContactContextRequested openedx-filter. Per pwnage101's review feedback, the filter call is made after context['tags'] = tags and operates on the whole page context dict rather than a bare tags list, since it's the context (not just the tags) that's conceptually being requested; the unused user argument is dropped entirely since pipeline steps that need it fetch it via crum internally. 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. Also bumps openedx-filters (3.10.0 -> 3.11.0, carrying the context-shape change above) and edx-enterprise (8.10.0 -> 8.11.0, carrying the matching SupportContactEnterpriseTagStep update) in requirements/constraints.txt and the compiled requirements files, now that both releases are published on PyPI. Verified locally against the real released packages (not just the mocked unit test) that SupportContactContextRequested.run_filter accepts and returns a bare context dict as expected. ENT-11574
be4d946 to
ce41075
Compare
pwnage101
left a comment
pwnage101
left a comment
There was a problem hiding this comment.
looking much better, just one nit to simplify the code a bit.
| if enterprise_customer: | ||
| tags.append('enterprise_learner') | ||
|
|
||
| context['tags'] = tags |
There was a problem hiding this comment.
Nit: you could delete this line and just do context['tags'] = ['LMS'] above on line 38.
2 participants
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.
Swaps the direct
openedx.features.enterprise_supportimport in the support contact-us viewfor a call to the
SupportContactContextRequestedopenedx-filter. No settings changes inthis PR:
OPEN_EDX_FILTERS_CONFIGregistration for the pipeline step lives entirely inedx-enterprise's own
plugin_settings()(enterprise/settings/common.py), per theENT-11830ownership handoff — already merged in this repo (#281) well before this branch existed.
(An earlier version of this PR incorrectly re-added
OPEN_EDX_FILTERS_CONFIGtolms/envs/common.py; that's been reverted.)lms/djangoapps/support/views/enrollments.pyis untouched — this PR is scoped to thecontact-tag filter only.
Update: filter shape corrected per review feedback
Per pwnage101's review feedback
(and the follow-up comment
on
user), the filter's original shape was wrong — it claimed to request the whole page"context" but only ever passed/returned a bare
tagslist.SupportContactContextRequested.run_filternow accepts/returns the entire
contextdict (the call is made aftercontext['tags'] = tags),and the unused
userargument was dropped entirely (pipeline steps fetch it viacruminternally, matching the convention used elsewhere). Acknowledged directly on the review thread.
Since the original shape (#390 / openedx#2688 below) had already merged and released, this required new
PRs in openedx-filters and edx-enterprise rather than amendments — both are now merged and
released (3.11.0 / 8.11.0), and this PR's call site and requirements pins are updated to match,
verified locally against the real released packages.
Related PRs
user): fix: change SupportContactContextRequested to accept/return full context openedx/openedx-filters#394Merge 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
Previously CI here was expected to be red until openedx-filters/edx-enterprise released — that
dependency chain has since resolved. Both packages are now released (openedx-filters 3.11.0,
edx-enterprise 8.11.0, carrying the shape-fix above) and this PR's requirements pins are bumped
to match. CI is green.
Testing
Same test change as the openedx-platform PR:
SupportContactContextRequested.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 — see handoff prompt for exact steps.