Repository navigation
Add a course waffle flag and endpoint for the Contact us launcher - #931
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Removing all DRF authenticators anonymizes valid session users, preventing superuser, staff, group, and user-specific feature-flag rules from working when everyone=None.
1 open issue
What changed in this PR
Adds runtime control for the MFE Contact us launcher through the existing ol_openedx_chat plugin, allowing operators to toggle it without rebuilding the frontend.
Changes:
- Adds a course waffle flag and an LMS endpoint for querying its effective value.
- Adds tests for routing, flag precedence, validation, anonymous access, and caching.
- Documents flag operations and bumps the plugin to version 0.7.0.
| File | Description |
|---|---|
uv.lock |
Updates the locked plugin version to 0.7.0. |
src/ol_openedx_chat/tests/test_views.py |
Tests flag evaluation and endpoint responses. |
src/ol_openedx_chat/tests/test_urls.py |
Tests keyed and course-less routes. |
src/ol_openedx_chat/tests/test_compat.py |
Checks the new flag’s name and independence. |
src/ol_openedx_chat/tests/fixtures.py |
Adds a shared request factory fixture. |
src/ol_openedx_chat/tests/conftest.py |
Exposes the shared fixture to tests. |
src/ol_openedx_chat/README.rst |
Documents flag precedence and deployment behavior. |
src/ol_openedx_chat/pyproject.toml |
Bumps the package version to 0.7.0. |
src/ol_openedx_chat/ol_openedx_chat/views.py |
Adds the cacheable flag-status endpoint. |
src/ol_openedx_chat/ol_openedx_chat/urls.py |
Defines routes with optional course keys. |
src/ol_openedx_chat/ol_openedx_chat/compat.py |
Adds the Contact us flag accessor. |
src/ol_openedx_chat/ol_openedx_chat/apps.py |
Registers the plugin’s LMS routes. |
src/ol_openedx_chat/CHANGELOG.rst |
Records the new flag and endpoint release. |
Review effort: Balanced
The note claimed an unset flag shows the launcher to every superuser. It does
not. The view sets no authentication classes, so DRF resolves the caller to
AnonymousUser and writes it back onto the Django request that crum hands to
CourseWaffleFlag; superusers, staff and authenticated can never match, verified
by sending a superuser session cookie with all three set true and still getting
{"enabled": false}. everyone and the course overrides are the only live levers.
The README and the Pulumi PR both claimed this and nothing asserted it. Every existing test calls the view directly, so crum sees no request and the production `flag_is_active(request, ...)` path never ran. Drives the view through crum's real middleware with a real Flag row carrying superusers/staff/authenticated; dropping `authentication_classes = []` flips it to enabled=true, so it catches the regression it is there for. The views.py and README notes said the opposite - that waffle still evaluates the per-user path. DRF writes AnonymousUser through to the HttpRequest, so it cannot. The caller-independent fields (testing, languages, percent) are still evaluated, which the README now says instead of "everyone is the only field".
Both patterns end in $ and are endpoints, so Django already fullmatches them and the leading ^ changes no routing - test_urls.py's prefix-injection case passes either way. Adding it anyway to match ol_openedx_course_export, which anchors both ends. The 0.7.0 "Added" heading used the same underline character as "Change Log", so it rendered as a sibling of the log rather than nested under the version.
This was referenced Oct 29, 2026
zamanafzal
marked this pull request as ready for review
October 29, 2026 08:26
asadali145
approved these changes
Oct 30, 2026
asadali145
left a comment
Contributor
There was a problem hiding this comment.
Looks good to me, thanks! Verified live on a local Tutor LMS: course-less and keyed routes, 400/404 handling, everyone/course-override precedence, and that a signed-in session survives the anonymized request.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

What are the relevant tickets?
Refs mitodl/hq#14033, under mitodl/hq#13712. #14033 shipped the launcher behind a
build-time gate, which means turning it off needs a frontend rebuild and a redeploy.
This replaces that half of #14033 with a runtime switch; the rest of its acceptance
criteria still stand.
Description (What does it do?)
ol_openedx_chat.contact_us_enabled, aCourseWaffleFlag— site-wide, or overriddenper course or per org from Django admin
GET /ol-openedx-chat/api/contact-us-enabled/[{course_key}]→{"enabled": <bool>},or 400 for a course key that routes but doesn't parse
No new plugin — this rides in
ol_openedx_chat, which already owns the chat aside andits own waffle flag. Operating notes for the flag (including two ways it silently
doesn't do what you expect) are in the plugin README.
Implementation details
authentication_classesis emptied rather than left at the default, so an expiredcredential cannot produce a 401 that a fail-closed client reads as "off" — the launcher
would vanish for exactly the people whose session had just lapsed. This is not
theoretical: edxapp's
DEFAULT_AUTHENTICATION_CLASSESputsDefaultJwtAuthenticationfirst (openedx/envs/common.py#L822-L825),
and it raises
AuthenticationFailedon a bad JWT cookie regardless ofAllowAny.The cost is that the view is anonymous to waffle.
CourseWaffleFlagpicks the request upthrough
crum.get_current_request(), but DRF has already writtenAnonymousUserthroughto that same
HttpRequest, sosuperusers,staffandauthenticatedcan never match —a signed-in superuser gets the same answer as a logged-out learner.
test_signed_in_superuser_does_not_flip_a_per_user_flagpins that through crum's realmiddleware. The caller-independent fields waffle falls through to (
testing,languages,percent) are still evaluated, soeveryoneis the field to set, not the only one thatcan turn the flag on.
cache_control(public=True, max_age=60)bounds staleness after a flip. LMS middlewarestamps
Vary: Cookieregardless of what the view declares, so a shared cache keys on thecookie anyway and
max-ageis what actually bounds it.Two URL patterns, each accepting an optional trailing slash, because the footer builds the
URL by concatenating a possibly-empty course key onto the base. The keyed form uses
settings.COURSE_KEY_PATTERNrather than a hand-rolled regex, and a test asserts that. Asegment failing the pattern 404s before reaching the view; one that passes but is not a
real course key reaches the view and gets the 400.
PluginURLs.CONFIGis registered forProjectType.LMSonly — Studio reads the endpointcross-origin from the LMS.
Screenshots (if appropriate):
How can this be tested?
Runs the suite once per settings module, so you get two summaries:
lms.envs.testcms.envs.testAll 24 new tests pass on both legs. Every skip is a pre-existing
test_aside.pycase; thecount differs because more of them skip under LMS settings.
New tests cover the flag accessor; both URL patterns with and without a trailing slash plus
a prefix-injection attempt; the full precedence chain (course beats org beats site-wide,
both directions); a disabled override falling back to site-wide; a valid key for a
nonexistent course returning 200;
a+b+creturning 400; anonymous access; and theCache-Controlheader.Against a local Tutor stack, with
BASE=<your-lms-host>/ol-openedx-chat/api/contact-us-enabledand
FLAG=ol_openedx_chat.contact_us_enabled:curl -s $BASE/{"enabled":false}manage.py lms waffle_flag $FLAG --create --everyonecurl -s $BASE/andcurl -s $BASE/course-v1:edX+DemoX+2024{"enabled":true}bothoff, enabledfalse, siblings stilltruecurl -s -o /dev/null -w '%{http_code}' $BASE/course-v2:a+b+c400manage.py lms waffle_flag $FLAG --deactivate{"enabled":false}waffle_flaghas no--activate;--everyoneis what turns a flag on. A bare--createleaves
everyonenull, which reads as off — set it explicitly so the row says "off onpurpose" rather than "nobody decided".
curl -ion any of the above showsCache-Control: public, max-age=60andVary: Accept, Accept-Language, origin, Cookie. A request carryingOrigin: http://apps.local.openedx.io:2000gets that origin echoed back.Additional Context
The endpoint is not throttled, deliberately. A 429 is indistinguishable from "off" to a
fail-closed client, so a rate limit would present as a feature outage. The counter-argument
is real — a keyed request costs two uncached queries (
WaffleFlagCourseOverrideModel.override_valueis
@request_cached(), which is per-request only) on an unauthenticated route, and thesibling
ol_openedx_auto_select_language.CourseLanguageViewdoes throttle its equivalent.Tracking separately rather than holding this PR; reviewers who want it done here, say so.
Flipping this during an incident has two traps — Pulumi reverts an admin change on the
next edxapp rollout, and editing the Pulumi value doesn't itself flip the flag. Both are
written up in the README.
Checklist:
Deploy order across repos — each step depends on the one before it.
ol-openedx-chat==0.7.0publishedbuild_manifest.yamlpins bumped and the openedx image deployed — this replaces the pre-deploy Job, which creates the flag rowFooter.jsxchange through the MFE pipelinePulumi goes before the image because a waffle-flags-only Pulumi run rewrites the ConfigMap
and nothing re-reads it; the image deploy is what re-runs the Job. The footer goes last
because with no flag row the endpoint answers
falseand the client is fail-closed, soshipping it first removes the launcher instead of gating it.
Only visible in mitxonline QA — the one environment with
contact_us_support_api_urlset. Elsewhere the launcher does not render at all, so getting the order wrong is invisible.