feat: replace enterprise_support imports in Mako templates with pluggable overrides - #39162
Conversation
0500e32 to
29085f7
Compare
| if not settings.ORDER_HISTORY_MICROFRONTEND_URL: | ||
| # There is no order history microfrontend to send anyone to. | ||
| return False |
There was a problem hiding this comment.
This check is the only substantial difference from the original edx-platform PR since conditionally rendering the order history button based on this setting seems to be an upstream addition to the template so I just pulled it up into the python API here.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The logo refactor breaks the existing Stanford theme override, and progress-template integration lacks coverage.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Replaces direct enterprise dependencies in LMS Mako templates with pluggable branding APIs.
Changes:
- Adds overridable learner display-name and portal-link helpers.
- Migrates header, dropdown, and progress templates.
- Adds helper and template tests.
| File | Description |
|---|---|
lms/djangoapps/branding/api.py |
Adds pluggable branding helpers. |
lms/djangoapps/branding/tests/test_api.py |
Tests helpers and header rendering. |
lms/templates/user_dropdown.html |
Uses the display-name helper. |
lms/templates/header/user_dropdown.html |
Uses dashboard, name, and order-history helpers. |
lms/templates/header/navbar-logo-header.html |
Uses the header-logo helper. |
lms/templates/courseware/progress.html |
Uses the display-name helper. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
brobro10000
left a comment
brobro10000
left a comment
There was a problem hiding this comment.
Matches the already-merged edx#486 (ENT-12353) — same hook names (OVERRIDE_GET_LEARNER_DISPLAY_USERNAME, OVERRIDE_GET_ENTERPRISE_LEARNER_PORTAL_LINK), so it'll connect to the already-released enterprise override correctly. Blocking on one thing, see inline comment: this PR doesn't bump the edx-enterprise pin anywhere.
|
|
||
|
|
||
| @pluggable_override('OVERRIDE_GET_LEARNER_DISPLAY_USERNAME') | ||
| def get_learner_display_username(user: AbstractBaseUser) -> str: |
There was a problem hiding this comment.
This hook needs edx-enterprise>=8.17.0 to do anything — that's the release containing openedx/edx-enterprise#2697's enterprise_learner_generic_name, registered under this exact setting name. This PR doesn't touch pyproject.toml / requirements/*.txt / uv.lock anywhere, unlike #39161's 8.14.0→8.15.0 bump for the sibling ticket. Please bump the pin to 8.17.0 here too, or this merges and silently no-ops in production — the same failure mode edx#486 almost shipped with.
brobro10000
left a comment
brobro10000
left a comment
There was a problem hiding this comment.
Approving to unblock b/c I know you will handle it before it is merged.
…able overrides Removes enterprise imports from several Mako templates. Replacement values provided as pluggable overrides in lms/djangoapps/branding/api.py. - get_learner_display_username(user) - an appropriate display username used throughout the interface. - get_enterprise_learner_portal_link() - link to the enterprise learner portal for the given user, if one exists. ENT-12353
Dropping the navigation_logo named block was an accident which likely broke comprehensive themes (e.g. stanford-style) which may declare an override. ENT-12353
4ff1aed to
865f3b7
Compare
Note: This PR is the upstream version of edx#486
Removes enterprise imports from several Mako templates. Replacement values provided as pluggable overrides in lms/djangoapps/branding/api.py.
ENT-12353