Release 0.77.9 - #3799
Closed
odlbot wants to merge 10 commits into
Closed
Release 0.77.9#3799odlbot wants to merge 10 commits into
odlbot wants to merge 10 commits into
Conversation
* feat(settings): change email and password via Keycloak
Adds "Change Email" and "Change Password" to the dashboard Settings page,
behind the PostHog flag `account-management`. Both hand the user off to Keycloak
using its "application initiated actions" (kc_action), then return them to
Settings with a success alert. Follows the flow MITx Online already uses.
Backend
- AccountActionStartView redirects to Keycloak's authorization endpoint with
kc_action=UPDATE_EMAIL / UPDATE_PASSWORD.
- AccountActionCompleteView reads the kc_action_status Keycloak appends and
passes the outcome back to the frontend as query params, which the alert
consumes once and strips.
- Both legs build the callback URL through one helper, because the token
exchange below requires a byte-identical redirect_uri.
- `email` is requested explicitly in the authorization scope. It is a Keycloak
default client scope, but relying on that being true of every environment's
client would fail silently and confusingly.
Keeping the new address
The API gateway caches userinfo in its session and only writes it at initial
authentication — refreshing the access token does not refresh it — so for the
life of that session (14 days in deployed environments) the X-UserInfo header
keeps serving the pre-change email. Two changes make the new address stick:
- On a successful email change, the callback exchanges the authorization code
Keycloak sends (previously discarded) for the fresh `email` claim. The user is
identified from the exchange's own `sub` rather than request.user, because
ApisixUserMiddleware logs the Django user out on any request without an
X-UserInfo header — which is the case on this callback when the gateway is not
in front of Django.
- ApisixUserMiddleware no longer re-applies the header's email once a session is
established. mitxonline avoids this by skipping all field syncing for an
already authenticated user, relying on Keycloak's SCIM push as a backchannel;
Learn has no SCIM provisioning, so only email is held back and every other
field still syncs.
SSO users
Users who authenticate through an external identity provider cannot change these
credentials here, so the section is hidden from them and the start view
independently re-checks. Detection reads federated identities from the Keycloak
Admin API, cached per user. It needs MITOL_KEYCLOAK_ADMIN_CLIENT_* which no
deployed environment sets yet (see the companion ol-infrastructure PR); until
then it fails open and Keycloak remains the only gate.
Admin API timeouts
is_sso_user runs while serializing the current user, so it is on every page
load, and pushing the email opt-in runs on a profile PATCH and on one-click
unsubscribe. python-keycloak defaults to a 60 second timeout and
mitol.keycloak.api.get_admin_client does not expose one, so main/keycloak.py
builds the client with a 5 second timeout and both call sites use it. This also
means profiles.api.sync_email_optin_to_keycloak — which has never run in any
deployed environment because the admin client was never configured — becomes
safe to enable.
Local development
- Keycloak needs the `update-email` feature flag; UPDATE_EMAIL is a preview
action, and without it Keycloak fails with a generic error page whose only
clue is a NullPointerException in its logs.
- The realm export registers UPDATE_EMAIL and sets the ol-learn theme so local
login pages match production. `--import-realm` skips realms that already
exist, so pre-existing setups need it applied once by hand.
- scripts/fetch_keycloak_theme.sh lifts the ol-learn theme jar out of the
mitodl/keycloak image rather than running that image locally, which tracks a
newer Keycloak than docker-compose pins and would irreversibly migrate the
local realm database.
- Fixes the themes volume mount, which pointed at the WildFly path and was
silently ignored by Keycloak 26.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix: regenerate OpenAPI spec after get_email docstring change
The CurrentUserSerializer.get_email docstring was reworded to satisfy ruff's
imperative-mood rule (D401) after the spec had been generated, so the committed
spec and TS client still carried the old wording. drf-spectacular derives the
field description from that docstring, so openapi_spec_check.sh failed.
Regenerated with ./scripts/generate_openapi.sh; the only change is that one
description line in both the spec and the generated client.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix: don't 500 when KEYCLOAK_CLIENT_ID is unset, and pin it in tests
CI surfaced two problems that local runs hid, because env/backend.env supplies
KEYCLOAK_CLIENT_ID and MITOL_API_BASE_URL locally while CI leaves both unset.
The first is a real bug. KEYCLOAK_CLIENT_ID has no default, and urlencode raises
TypeError on a None value, so in any environment that hasn't configured the
client every click on Change Email or Change Password would have been a server
error. AccountActionStartView now checks for it and returns the user to Settings
with an error alert, matching how it already handles an unrecognized action.
The second was the tests' fault: they asserted against
settings.KEYCLOAK_CLIENT_ID and settings.MITOL_API_BASE_URL, so they described
whatever the environment happened to provide rather than fixed expectations. An
autouse fixture now pins both, plus KEYCLOAK_BASE_URL and the realm name.
Verified by running the suites with those variables explicitly emptied to
reproduce CI: 319 passed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* refactor: parse the account action explicitly instead of enum membership
Review feedback flagged `action not in AccountAction` as unable to match a
plain query-string value, which would make the callback skip both status
reporting and the email sync. That isn't the case here — Python 3.12 extended
enum membership to accept values, and test_account_action_complete plus
test_account_action_complete_syncs_email cover the reporting and sync branches.
The concern is fair regardless: the behaviour is version-dependent (before 3.12
the same expression raises TypeError rather than returning False) and it isn't
obvious to a reader, as this review shows. Replaced with an explicit
parse_account_action() lookup that returns the member or None, and kept the raw
value for the log message so an unrecognised action is still visible.
parse_account_action is covered directly, including the empty string and None,
so the branch this feedback was about can't regress silently.
AccountActionStartView is left alone: it tests membership against the
KEYCLOAK_ACTIONS dict, and dict lookup by string key against StrEnum keys works
on every version because StrEnum members hash equal to their values.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix: don't claim the email changed when it is only pending confirmation
All deployed realms have verify_email enabled, which makes Keycloak's
kc_action_status=success mean "confirmation email sent", not "address changed".
The address only changes when the user clicks the link, and that leg reaches our
callback with just the two params we put in the redirect URI ourselves — no
authorization code, no status — so the exchange cannot run there.
The result was an alert saying "Your email address has been updated" at a point
where nothing had changed, followed by Settings continuing to show the old
address. Reported as the feature being broken, and fairly so.
The callback now derives the message from the authoritative address instead of
from Keycloak's status: it only reports success if reading the email back proves
it changed, and otherwise downgrades to a new `pending` status whose copy tells
the user to check their inbox. This is correct whether or not verify_email is on,
so it also covers the verification-off and already-verified-bypass paths where
the change does apply immediately.
Also logs which params a callback carried when they are unusable. That is what
identified the confirmation leg as carrying no code; names only, since these
params carry authorization codes and addresses.
This does not close the data gap: when the user confirms via the link, nothing
notifies us, so the stored email stays stale until the gateway session turns
over. Closing that needs Keycloak to push the change over SCIM, which is
configured outside this repo. Tracked separately.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix: read the new email after the confirmation link is clicked
Closes the data gap left by the previous commit. With verify_email on — all
deployed environments — the address changes when the user clicks the link in
Keycloak's confirmation email, and that leg returns them to our callback through
a plain hyperlink carrying only the params we put in the redirect URI ourselves.
No authorization code, so there was nothing to exchange, and the change never
reached us. Confirmed by logging which params that leg carries: account_action
and next, nothing from Keycloak.
The visible symptom was Settings showing the previous address indefinitely. In
deployed environments it would have persisted until the gateway session turned
over, which is 14 days.
The callback now recognises that leg and makes one silent authorization round
trip — no kc_action, prompt=none — purely to obtain claims it can read. Keycloak
answers immediately from the existing SSO session, and the resulting code is
exchanged for the current address. A marker param on the redirect URI stops it
repeating if no code comes back, which is what happens when prompt=none finds no
session.
The marker has to be part of the redirect URI passed to the token exchange too,
since Keycloak requires that to match the authorization request byte for byte.
Verified against local Keycloak with verify_email enabled and Mailpit capturing
the mail: the confirmation leg redirects to the authorization endpoint with
prompt=none and the marker, and a marked callback without a code returns to
Settings rather than bouncing again.
SCIM would remove the need for this by having Keycloak push the change, and is
still worth doing — it would also cover changes made outside this flow. It is
configured outside this repo, so it is tracked separately rather than blocking.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Revert "fix: read the new email after the confirmation link is clicked"
The silent re-authorization does not work. It assumed the user still has a
Keycloak session when they return from the confirmation link, so that prompt=none
would answer immediately with a code. They do not: clicking an action-token link
authenticates only for that action and leaves no browser SSO session, and the
update-email form's "Sign out from other devices" is checked by default.
Observed on the refreshed callback:
params present: account_action,account_action_refreshed,error,next
Keycloak returns an error rather than a code, the guard stops the retry, and the
address is still not read. So the approach added a redirect that reliably fails
on the one leg it existed for.
Reverting rather than iterating: with no code, no session and no gateway header,
that leg carries no way to identify the user at all, so nothing can be read from
Keycloak there without new inputs.
The messaging fix in the preceding commit stands — it is what stops the flow
claiming the address changed when it has not.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* feat(local-dev): capture Keycloak email locally with Mailpit
Deployed realms have verify_email enabled, which changes the change-email flow
materially: submitting the form does not change the address, Keycloak emails a
confirmation link and only applies it once that link is clicked. The local realm
had verification off and no SMTP, so local testing exercised a flow that does not
exist in any deployed environment — which is how the flow shipped for review with
a success message that fires before anything has changed.
Adds Mailpit to the keycloak profile and points the realm's SMTP at it, with
verifyEmail on to match deployed realms. Nothing leaves the machine, so any
address works and the confirmation link is read at http://localhost:8025.
Also fixes the themes volume mount, which pointed at /opt/jboss — the WildFly
distribution's path, silently ignored by Keycloak 26.
Note for existing setups: --import-realm skips realms that already exist, so the
SMTP settings and verifyEmail need applying by hand once. Documented, along with
the confusing 500 from Keycloak's "Test connection" button when the admin user
has no email address of its own.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs: record the Keycloak SCIM request for Learn
Changing an email in a realm with verify_email on — all deployed environments —
applies when the user clicks the confirmation link, and that leg returns to Learn
with no authorization code and no session, so Learn cannot observe it. The stored
address then lags until the gateway session turns over, which is 14 days.
Keycloak already pushes user changes to MITx Online over SCIM. Learn has the
receiving side but no push configured, and the plugin's targets live in its own
admin backend rather than in ol-infrastructure, so this is a configuration request
rather than a pull request. Writing it down here so it doesn't live in a chat log.
Includes what was verified locally rather than assumed: pushing a replace
operation to /scim/v2/Users/<id> returns 200, stores the address, and survives a
subsequent request carrying a stale gateway header. Also records that
mitol.scim only accepts scim-for-keycloak's non-compliant payload shape — path
absent, value as a JSON-encoded string — because the spec-compliant form returns
a 500, and that is easy to lose an hour to.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* feat: provision_scim_client command for inbound SCIM credentials
Keycloak needs credentials before it can push user changes into Learn, and Learn
had no way to create them — no OAuth2 application provisioning anywhere in the
repo, and nothing that mints a service token. This adds a command so it is
repeatable across environments instead of clicks in Django admin.
It creates a staff service user, an OAuth2 application, and a bearer token bound
to that user, then prints the values whoever configures the Keycloak SCIM provider
needs. Idempotent, with --rotate-token for rotation or a lost token, since the
token is only displayed when issued.
The token has to be bound to a user. Learn's SCIM endpoints are guarded by Learn's
own OAuth2 provider: OAuth2TokenMiddleware resolves the token to a user and
mitol.scim.utils.is_authenticated_predicate then requires that user to be active
and staff. A client_credentials token belongs to the application rather than a
person, so AccessToken.user is null and the push is rejected with a 401 — verified
by trying it, which is why the earlier suggestion of CLIENT_CREDENTIALS_GRANT in
the request doc was wrong and is corrected here. The plugin's auth type must be
BEARER.
Verified locally end to end: the provisioned token authenticates a replace
operation against /scim/v2/Users/<id>, returns 200, and the pushed address is
stored.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs: rollout runbook for change email / change password
The work spans three pull requests across two repositories, a management command
run per environment, and one piece of Keycloak configuration owned by another
team, with a hard ordering constraint in the middle. That is too much to carry in
a chat log or a PR comment, so it lives here.
Records the ordering rules and why each exists, most importantly that the
mit_learn app stack must not be applied before the Keycloak substructure stack:
the secret it reads would not exist yet, and secrets are attached with
envFrom.secretRef without optional: true, so new pods fail with
CreateContainerConfigError and the rollout stalls.
Also corrects the provisioning command's own output. It claimed the bearer token
is shown only once, which is untrue: AccessToken.token is stored unhashed, unlike
Application.client_secret, so a mislaid token can be read back rather than
rotated. Rotating breaks whatever Keycloak config still holds the old value, so
the distinction matters.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* refactor: use the library's Keycloak admin client directly
The Admin API timeout moves upstream to mitol-django-keycloak
(mitodl/ol-django#535), which adds a timeout argument and a
MITOL_KEYCLOAK_ADMIN_TIMEOUT setting to get_admin_client().
That removes the reason main/keycloak.py existed. Its update_user_attributes
was a verbatim copy of the library's update_user, differing only in using the
locally-built client, so both call sites now use mitol.keycloak.api directly.
Until that release lands, is_sso_user falls back to python-keycloak's 60 second
default. It short-circuits before making a call whenever the admin client is
unconfigured, which is every environment today.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* refactor: narrow to the settings UI and the backend it needs
Reduces this to the change-email/change-password UI plus the endpoints and
serializer fields behind it, per review.
Keeping a changed address in step with Keycloak is dropped here in favour of
SCIM. That removes:
- the ApisixUserMiddleware change, superseded by #3747, which makes userinfo
updates togglable so deployed environments stop overwriting SCIM
- fetch_keycloak_userinfo/sync_email_from_keycloak, which read the new address
back from the authorization code because the gateway header was stale
- KEYCLOAK_CLIENT_SECRET, used only by that token exchange
Without the exchange the callback can't tell an applied change from one still
awaiting its confirmation link, so the AccountActionStatus.PENDING state goes
and the update-email success copy now points at the inbox rather than claiming
the address already changed.
Also moved out: the SCIM client provisioning command, which belongs with
enabling SCIM rather than with this UI, and the rollout runbook, which described
a sequence this no longer follows. The local Keycloak setup keeps only what the
flow cannot run without — the update-email preview feature and the UPDATE_EMAIL
required action.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* address feedback
* feat: store is_sso_user on the User model instead of in the cache
Adds a nullable User.is_sso_user, filled in from Keycloak the first time it is
needed — in practice on the first GET of users/me — and read straight from the
row afterwards.
A field rather than a cache entry because the value effectively never changes on
its own, because how we decide it may change, and because it can then be
overridden: clearing the flag grants someone local credentials, so they can keep
an account after leaving the organization that provided their identity. Exposed
in the user admin for that reason.
Null means undetermined rather than false, so a Keycloak failure or an
unconfigured admin client leaves it null and a later read retries instead of
recording a guess.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* address the feedback
* refactor: expose the current user's email as a plain read-only field
Replaces the SerializerMethodField with a field declaration, per review.
The default is load-bearing: users/me serializes AnonymousUser as well as a real
user, AnonymousUser has no email attribute, and a read-only field whose
attribute is missing is dropped from the output rather than rendered empty —
which is why first_name and last_name are already absent for anonymous users.
Without the default, `email` disappeared from that response.
Regenerates the spec and client: `email` stays required, so its TypeScript type
is unchanged; only the method docstring that served as the field description
goes away.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Ahtesham Quraish <ahtesham.quraish@192.168.1.11>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Ahtesham Quraish <ahtesham.quraish@A006-01455.local>
Co-authored-by: Ahtesham Quraish <ahtesham.quraish@arbisoft.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Ahtesham Quraish <ahtesham.quraish@192.168.1.62>
Co-authored-by: Ahtesham Quraish <ahtesham.quraish@192.168.1.96>
* Update Terms of Service (MicroMasters bundle, AI Tutor, date)
* fix(otel): continue the edge trace by extracting W3C traceparent
Every learn-nextjs trace roots at learn-nextjs, never at traefik, even though
Traefik has been emitting edge spans since ol-infrastructure#5480 shipped.
`{traefik} && {learn-nextjs}` matches zero traces.
Sentry's propagator is write-only for W3C. SentryPropagator.extract() reads
sentry-trace and baggage and never traceparent, while inject() does write one
and fields() advertises it. Traefik sends only traceparent, so the incoming
context is discarded and the SSR render starts a new trace. Keycloak, behind
the same Traefik, joins fine because Quarkus uses the standard W3C propagator
-- which is how we know the gateway is not at fault.
Verified against @sentry/opentelemetry 10.70.0, the current release, so this is
not something a version bump fixes.
Register a CompositePropagator before Sentry.init(). It has to be before:
setGlobalPropagator refuses a second registration and returns false, so doing
it afterwards silently does nothing.
Sentry goes last in the composite so it still wins when sentry-trace is
present, leaving browser-originated traces exactly as they are. Its extract()
returns the context untouched when that header is absent, so W3C's extraction
survives for edge traffic. Both directions are pinned by tests, along with a
test that fails the day Sentry learns to read traceparent and this composite
can be removed.
@sentry/opentelemetry and @opentelemetry/core become direct dependencies
because SentryPropagator is not exported from @sentry/nextjs or @sentry/node.
The Sentry one needs to stay in lockstep with @sentry/nextjs.
* fix(otel): dedupe the Sentry packages, and let the test pin the real ordering
The caret on @sentry/opentelemetry resolved to 10.70.0 while @sentry/nextjs
stayed at 10.50.0, so the lockfile carried two copies with two @sentry/core
versions. The composite imported SentryPropagator from a different Sentry
runtime than the SDK initialised -- precisely the split this change warned
about. Pin both to 10.50.0 exactly; the lockfile now has one entry.
Move the composition into otel-setup.ts and use it from both startup and the
test. The test previously rebuilt the composite itself, so reordering or
dropping a propagator in production would have left every assertion passing.
The "if this ever starts passing" comment was backwards: the assertion passes
today, and would start *failing* if Sentry learned to read traceparent.
* Make apisix userinfo updates togglable * Set default update flag to false * Address feedback
* Put the program price amounts on their theme typography tokens ProgramPriceAmount and ProgramListPriceAmount were the only elements in the Certificate Track card not using the theme font: both hardcoded 'Helvetica Neue', Helvetica, Arial alongside hand-typed sizes, while the title, both captions, and the savings line all rendered in neue-haas-grotesk-text. On macOS that showed as two typefaces in one card; everywhere else the amounts fell back to Arial. The hardcoded metrics turn out to be the tokens spelled out by hand -- 34/40 bold is exactly h2, and 28/36 is exactly h3 -- which matches what the designs specify. So this swaps in theme.typography.h2 and h3 and drops the font-family overrides. Rendered size and weight are unchanged: h3 is bold, so the list price overrides fontWeight back to regular, which is deliberate rather than incidental -- the struck comparison price is meant to read lighter than the current price beside it. Neue Haas Grotesk is wider than Helvetica at the same size (a $499 - $1,499 range measures 235px against 215px), so rows containing a wide price wrap slightly sooner than before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Draw the program price separator as a gap decoration The vertical rule between the current price and the struck list price was a 1px x 48px div sitting between them as a third flex item. That row wraps in narrow cells, and when it did the rule stranded itself at the end of the first line, floating beside the current price with nothing left to separate it from. A separator between two items is only meaningful when they share a line, and that is what gap decorations express: the rule is painted into gaps that exist, so wrapping removes it with no wrap detection. CSS has no way to select "the item that ended up first on a wrapped line", so a sibling element could never have known to hide itself. Because the rule now lives in the gap rather than beside it, the column gap absorbs the spacing the divider used to get from a gap on either side: 24 + 1 + 24 becomes a 48px gap with the rule down its middle, which keeps the blocks the same distance apart to within the width of the rule itself. Where gap decorations are unsupported nothing is drawn, which is an acceptable resting state: the two prices are already distinguished by size, weight, colour, strikethrough, and their captions. Dropping the element also gives the rule an intrinsic height instead of an arbitrary 48px. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Show advertised price ranges on product pages (hq#12786) mitxonline exposes min_price/max_price on courses and programs. Where the two differ the resource is advertised as a range, but the product pages rendered only the single certificate product price, so an about page could say $1,000 while the resource drawer next to it said $250 - $1,000. That discrepancy already existed because mit-learn's own ETL reads min/max with no additional gating (learning_resources/etl/mitxonline.py, parse_prices). common/mitxonline gains toPriceRange, formatPriceRange, and formatResourcePrice. The last is MitxOnlineResourceCard's local helper lifted out, with two corrections: the range predicate is min < max rather than min !== max, and the separator is an en dash to match getDisplayPrice in ol-utilities, so a resource reads identically on the about page and in the drawer. Both certificate-price hooks route the displayed price through it, behind their unchanged `if (!product?.price)` gate, so a resource with no purchasable product still shows no price. ProgramSavingsBlock now takes a PriceRange: savings derive from the top of the range and read "Save $150+", and a list price falling inside the range drops the savings framing entirely, since it does not beat every price in the range. TrackCard's header becomes a title-plus-price row with the subtitle at full card width beneath it. The subtitle, not the price, was what forced the wrap: a flex item's line-breaking uses its max-content width, and the subtitle's 195px beat the price every time. With the subtitle out of that row the title's flexGrow lets its own text decide when the price wraps. Ranges additionally render one step down the heading scale (compactPrice, h5) so they sit beside the title rather than below it, which is how spec item 8 is satisfied. The EnrollAreas set that flag from the same toPriceRange predicate the hooks format on, so sizing tracks display. Two things not to retry here. Do not claw back the header's extra height with a negative margin: when the price wraps it is the last flex line, so there is nothing beneath it to reclaim from and the glyphs collide with the subtitle. Do not render a smaller price by nesting a smaller element inside the price container: the container's line box is struck for its own font size, so smaller text inside sits on that larger strut's baseline and hangs below the title however the boxes are aligned. One element, one type token. A range never fits beside a list price in ProgramSavingsBlock at the desktop sidebar width -- 312px of content against a 235px range, a 121px caption and the gap between them -- so that row is always wrapped for ranges. No type scale reaches it; verified by measuring the real card with the gap zeroed, the rule removed and the caption already wrapped, which still needed 327px. The mitxonline test factories previously drew min_price and max_price as independent faker values, so course fixtures always advertised a range and program fixtures did about half the time. They now default equal, making a range opt-in per test; without that this change makes unrelated suites flaky. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Move the financial aid link into the certificate card header Per the hq#12786 wireframes, the financial aid link leaves the feature list and becomes its own row directly beneath the Certificate Track title -- the slot the design pairs with a right-hand price caption. It loses its check icon along with its place among the bullets. TrackCard gains a headerAside slot for it. The slot sits 4px under the title row so it reads as part of the title rather than as another header row, and widens the header's gap to the subtitle to 16px when filled. Cards with no aside keep the 8px gap and are unchanged. The link is now driven by linkStyles rather than a hand-rolled rule. Its small "red" variant is body3 at #A31F34, matching the wireframe exactly, so the unapplied state needs no override. The approved state keeps that scale and swaps in darkGreen, which the Link component has no variant for: the green used by the feature check icons is only 2.7:1 against the card and fails AA as text. Green marks a resolved state rather than a call to action, so it also drops the resting underline and takes one on hover -- it still links to the application record, but users have no reason to follow it. Copy becomes "Apply for financial aid" / "Financial aid applied (visible at checkout)". The enrollment dialog's own financial aid link is a separate surface and keeps its wording. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Address review feedback on the certificate card price and aid link Pin PriceContainer's line height to the title's. The price is the taller of the two in the flattened title row, so its token leading was setting the row height and pushing the subtitle and everything under it down 10px relative to the pre-flattening layout -- visible on every card with a top-right price. This also restores the aid link's 4px gap to the title, which was measuring from the row's bottom rather than the title's. Show an advertised price even when a resource has no purchasable product. Both hooks previously returned a null price before reaching the formatter, so a resource with an advertised range showed it on the carousel card and no price at all in the InfoBox. Program savings stay behind the product guard: there is nothing to have saved without a price you would pay. Withhold the aid link while the approval lookup is in flight, holding its row so resolving it does not shift the card. The lookup is client-only, so an already-approved user was being shown the red "apply" call to action on first paint. `pending` reads isLoading rather than isPending, because a disabled query stays pending forever and this one is disabled for anonymous visitors, who have nothing to wait for. The approved state's green now matches the `Save $X+` text that can sit a few rows below it in the same card, rather than introducing a second green. It is a raw hex in both places; no token is this shade, and the `green` token fails AA as text at 2.7:1 against the card. Also: approved copy reads "approved" rather than "applied", which otherwise collided with having submitted an application; the financial aid shape is a shared FinancialAid type instead of three inline duplicates; and the range-beats-product-price tests use a product price distinct from both ends of the range, so an implementation composing the range's minimum with the product price no longer passes them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Narrow the factory's advertised price before formatting it The `no product` test formats `program.min_price` for its expectation, but the field is `number | null` on the API type even though the factory always sets it, so `yarn typecheck` failed on the branch. An invariant narrows it and documents the factory assumption the expectation rests on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Draw the program price rule with a clip instead of a gap decoration `column-rule` on a flex container is CSS Gap Decorations (css-gaps-1), which Chrome and Edge only shipped in 149 and Firefox and Safari have not shipped at all, so the separator was missing for most visitors -- including in the common single-price-plus-savings case, where the two blocks do share a line. `@supports` cannot detect this, since the declaration parses everywhere for multi-column layout. Each block now paints a rule in the gap preceding it, and the row clips whatever lands at its left content edge, which is exactly the rule of a block that starts a line. That keeps the property the gap decoration was chosen for -- the rule disappears when the row wraps, with no width breakpoint predicting where the prices wrap -- using only overflow clipping and an absolutely positioned pseudo-element. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Render an advertised range at the title's size, per the design The wireframe puts the range price on Subtitle/S1 -- 16px, one weight lighter than the title beside it -- rather than a step down the heading scale, so the compact price is subtitle1 instead of h5. A single price keeps h4. The line-height pin stays: it is what keeps h4 from adding 10px to the header, and is simply redundant for subtitle1, which already has that line height. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…y's (#3788) * refactor(otel): own the OpenTelemetry setup instead of patching Sentry's Sentry's SDK builds the whole pipeline itself -- provider, sampler, resource, propagator, context manager -- and three of those choices were wrong for us, each previously worked around in place. getSentryResource('node') hardcodes service.name to "node" (getsentry/sentry-javascript#20502). The workaround was a span processor that rewrote every span's resource at export time. Building the provider with the real Resource deletes ResourceAttributeOverrideSpanProcessor along with applyResourceOverrides and detectResourceOverrides. SentryPropagator.extract() never reads traceparent, so Traefik's edge context was dropped. The workaround was registering a composite before Sentry.init to win a registration race. Now it is simply the propagator we install. SentleSampler applies Sentry's tracesSampleRate to span creation, making one knob govern both destinations. Ours is a plain ParentBased sampler, so Sentry's rate is Sentry's alone, applied by beforeSendTransaction after the span exists. httpIntegration({ spans: true }) is not optional: Sentry stops instrumenting node:http for spans under skipOpenTelemetrySetup (_shouldUseOtelHttpInstrumentation returns false), and without it there is no server span at all -- the same silent-nothing failure mode as the missing [asgi] extra in learn-ai. Verified the combination end to end in node before writing the module, since this is the part that cannot be reasoned about safely: validateOpenTelemetrySetup passed: true propagator fields: traceparent, tracestate, baggage, sentry-trace env-detected service.name: learn-nextjs | deployment.environment: production extracted traceId from a W3C traceparent: 0af7651916cd43dd8448eb211c80319c span recording: true | sampled: true * test(otel): actually assert the spans option the comment claimed was asserted The comment next to httpIntegration said this load-bearing option was covered by a test. It was not -- otel-setup.test.ts only exercised the resource, sampler and propagator helpers, so deleting `spans: true` would have removed every server span and passed review and CI alike. Lift it to a named constant, SENTRY_HTTP_INTEGRATION_OPTIONS, so the regression has to delete something a test asserts on, and assert it. * fix(otel): keep tracesSampleRate so Sentry's own spans stay recording Dropping tracesSampleRate from Sentry.init was wrong. It makes hasSpansEnabled() return false, and @sentry/opentelemetry 10.50.0 then wraps both startSpan and startInactiveSpan in suppressTracing() -- so every span Sentry or the Next.js integration creates would have been non-recording. It does not reintroduce the shared ceiling it was removed to avoid. With skipOpenTelemetrySetup, SentrySampler is never installed, so span creation is governed by the ParentBased sampler in installOpenTelemetry(); this only signals "spans are enabled" to Sentry's own code paths. Sentry's actual sampling stays in beforeSendTransaction. Lifted to a named constant with the reason attached, and asserted, so the next person reading "our provider owns the sampler" does not conclude this line is dead and delete it again. * fix(otel): stop duplicating what Sentry already does, and flush what we own Review of the Sentry 10.50.0 sources turned up five ways this setup either duplicated Sentry's work or quietly dropped it. @sentry/nextjs already builds its defaults as httpIntegration({ disableIncomingRequestSpans: true }) because Next.js emits its own server span. Passing a userland httpIntegration wins over that default (filterDuplicates only protects the reverse), so enableServerSpans went true and every SSR request got a Sentry SERVER span on top of Next's root span. Taking the default back costs nothing under skipOpenTelemetrySetup: the OTel HttpInstrumentation hardcodes disableIncomingRequestInstrumentation, and on Node 24 getConfigWithDefaults also disables the outgoing side, so it was inert either way -- outgoing spans come from SentryHttpInstrumentation. NodeClient.flush() awaits `this.traceProvider?.forceFlush()`, and that field is set only by Sentry's initOpenTelemetry(), which skipOpenTelemetrySetup skips. Unset, the Sentry.flush(2000) calls in captureRequestError and the route/component wrappers were no-ops for spans, and with no SIGTERM or beforeExit hook a pod rollout dropped whatever the BatchSpanProcessor held. Assign it, and match Sentry's forceFlushTimeoutMillis: 500 rather than the SDK's 30s default, which outlasts the 2s budget the flush calls give it. Keep SentrySampler. It is the only thing that emits `beforeSampling` -- which is how @sentry/nextjs drops spans for Sentry-ingest requests forwarded on behalf of the Edge runtime -- and the only thing that routes decisions through wrapSamplingDecision, which writes sample_rate/sample_rand onto the trace state that becomes the outgoing baggage DSC. Separating the two destinations never needed a second sampler: tracesSampleRate stays at 1 so every span is created, Alloy's tail sampler decides what Tempo keeps, and beforeSendTransaction sets Sentry's volume after the span exists. OTEL_TRACES_SAMPLER_ARG goes with it. Drop Sentry.validateOpenTelemetrySetup(). It returns void, early-returns on !DEBUG_BUILD, and otherwise only calls debug.error/debug.warn, which need debug.enable() -- reached only when options.debug is true, and it is false. It could not have reported anything, and the comment claimed it threw. Set NEXT_OTEL_FETCH_DISABLED, which Sentry sets only when it owns the OTel setup. nativeNodeFetchIntegration stays in the defaults, so without it every server-side fetch got both a Next span and a Sentry undici span. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013SrNkjjjDiSoQkJRvDJMTC * fix(otel): keep Next's fetch spans, which nothing was duplicating Setting NEXT_OTEL_FETCH_DISABLED was wrong. The premise was that Sentry's nativeNodeFetchIntegration stays in the defaults under skipOpenTelemetrySetup and would double every server-side fetch span. It does stay in the defaults, but it emits nothing: function _shouldInstrumentSpans(options, clientOptions = {}) { return typeof options.spans === 'boolean' ? options.spans : !clientOptions.skipOpenTelemetrySetup && core.hasSpansEnabled(clientOptions); } With skipOpenTelemetrySetup and no explicit spans option that is false, so instrumentOtelNodeFetch is never called. Next's patch-fetch was the only source of fetch spans, and disabling it left none. No coverage was actually lost in the deployed app -- server-side HTTP goes through axios on node:http, not fetch -- but it would have been a trap the first time anyone reached for fetch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013SrNkjjjDiSoQkJRvDJMTC --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* always select currently running course run as the context for dashboard cards and always show cert if one is available * handle runs with no start / end dates * a course should be complete if a past enrollment was completed * some accessibility fixes
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 10259317 | Triggered | Generic Password | 608a1cf | docker-compose.services.yml | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
OpenAPI Changes2 changes: 0 error, 0 warning, 2 info Unexpected changes? Ensure your branch is up-to-date with |
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
8 participants
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.
Carey P Gumaer
Tobias Macey
Chris Chudzicki
Nathan Levesque
Anastasia Beglova
Zaman Afzal
Ahtesham Quraish