Identify PostHog persons by Keycloak global_id, not Django pk - #3798
Merged
Conversation
OpenAPI ChangesShow/hide changesUnexpected changes? Ensure your branch is up-to-date with |
ChristopherChudzicki
force-pushed
the
hq-12601-posthog-guid-identify
branch
from
July 29, 2026 14:56
eb172e9 to
50ebea7
Compare
ChristopherChudzicki
marked this pull request as draft
July 29, 2026 15:49
ChristopherChudzicki
force-pushed
the
hq-12601-posthog-guid-identify
branch
from
July 29, 2026 17:49
940d3ca to
5eaf9e6
Compare
| show_auto_daily_featured_items = is_enabled( | ||
| features.ENABLE_AUTO_DAILY_FEATURED_ITEMS, | ||
| False, # noqa: FBT003 | ||
| user, |
Contributor
Author
There was a problem hiding this comment.
This is the only use of the user var above. It's for checking a feature flag.
ChristopherChudzicki
marked this pull request as ready for review
July 30, 2026 19:05
For mitodl/hq#12601 Header.js was identifying by the local pk on every page, while DashboardPage.js separately (and correctly) identified by global_id on the dashboard only, creating two disagreeing identities. Header now guards on global_id (skipping identify if absent) and the now-redundant identify call in DashboardPage was removed.
Header.js is only mounted on some pages (it's skipped on cart/checkout/ orders/certificate pages), so identify() never fired there. Moving it to App.js wouldn't help either: Wagtail-rendered pages (course/program/home, financial aid) never mount App.js at all. Instead, hook into the redux-query REQUEST_SUCCESS action for /api/v0/users/current_user/, which every page that loads currentUser triggers regardless of which component asked for it. This fires once per successful fetch instead of once per Header render, and posthog.init() (a synchronous module-level side effect in lib/util.js) is always evaluated before any REQUEST_SUCCESS can be dispatched, since that requires an async network round trip.
The middleware fires on every successful current_user response and has no dedup guard. Point at posthog's docs for why one isn't needed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The identify call was switched to global_id, but the pk was still being sent as the user_id person property and as the distinct_id for server-side flag evaluation, so client identify, client flag overrides, and ol-django's person_properties disagreed on who the person was. ol-django's _get_person_properties derives user_id from opt_unique_id, so switching the identifier fixes the distinct_id and the property together. HomePage now falls through to the existing anonymous_session_id path for a user without a global_id, rather than to the HOSTNAME fallback inside is_enabled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
"handles checkFeatureFlag returning true" and "falls back to default URL when MIT_LEARN_DASHBOARD_URL is not set" were both the same test as "redirects when feature flag is enabled" -- the beforeEach/afterEach already leave mit_learn_dashboard_url unset, so the fallback case was never distinct. Dropped both and let each remaining case pin one branch. makeUser() already generates a global_id, so the "test-guid-*" overrides were fighting the factory and forcing the literal to be repeated in each withArgs and assertion. Use the factory's own value instead. Also removes the performance.mark polyfill from beforeEach. It never did anything for the tests it was added for: sandbox.useFakeTimers() in the inner beforeEach runs afterwards and swaps global.performance for sinon's, which only carries the methods it can copy off globalThis.Performance.prototype. That constructor is absent before node 18, so the mark stub goes missing on older local node -- but CI runs 20.18, where it exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Flow infers mockSettings.posthog_api_host as string from its initializer, so assigning null to it is an error. global.SETTINGS is the same object but loosely typed, which is what this test used before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The middleware mirrored mit-learn's ConfiguredPostHogProvider but dropped
its posthog.reset() branch. identify() latches $user_state to
"identified" and only reset() clears it, so a logged-out browser went on
attributing events to whoever logged in last, and nothing else in
frontend/public/src calls reset().
That was contained while MITxOnline had its own project and pk
identities. Once step 5 puts it in Learn's project it means
shared-computer cross-attribution, plus churn as Learn resets what
MITxOnline left identified.
Signin and signout both happen on the SSO server, so there is no
client-side signout to hook. Instead reset whenever the current_user
response turns out to be anonymous while posthog still thinks it is
identified -- the same approach, and the same reasoning, as mit-learn.
Both branches key off what the response affirmatively says rather than
one being the other's else, because two cases must not reset:
- a 2xx carrying no user at all. transform is objOf("currentUser"), so
an empty body yields {currentUser: undefined}.
- an authenticated user with no global_id. They are unidentifiable, not
anonymous. is_anonymous separates those; !global_id does not.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CheckoutInterstitialView passed request.user.global_id straight through,
but is_enabled does `unique_id = opt_unique_id or default_unique_id()`
and default_unique_id() returns settings.HOSTNAME. So every user without
a global_id collapsed into one shared bucket -- a percentage rollout
becomes all-or-nothing for them -- and the result cached under a cache
key derived from that shared id.
global_id is nullable and blankable by design ("we may want a Django
user that's not connected to an SSO user"), and request.user.id was
always truthy, so this only became reachable when the identifier
changed.
Guard it the way the other two global_id flag calls already do:
_should_redirect_to_learn in this same module and main/views.py. Passing
default/opt_unique_id by keyword matches those callers and drops the
FBT003 noqa.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ChristopherChudzicki
force-pushed
the
hq-12601-posthog-guid-identify
branch
from
July 31, 2026 00:56
ae0c757 to
bb1d26f
Compare
The stub returned "identified" for every key, and posthog reads its own feature flags through get_property too. A stray render elsewhere in the suite checking "new-cart-design" therefore hit `'new-cart-design' in "identified"` and failed whichever posthogIdentify test happened to be running, as an uncaught error. Returning undefined for other keys is what posthog already handles, so scoping the stub leaves the flag path alone while the middleware's own $user_state read is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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?
Part of https://github.com/mitodl/hq/issues/12601 (step 3)
Description (What does it do?)
Identifies PostHog persons by the Keycloak
global_idinstead of the Django user pk, so identities are globally unique across the MIT applications sharing a PostHog project.How can this be tested?
I recommend adding a little client-side logging, here: (Posthog requests are encoded so hard to see)
Add some logging
Load pretty much any page... If you're logged in, you should see a "Posthog Identifying GUID" call using your users keycloak GUID
If you log out in the same browser, you should see it "reset".
Additional Context
Identify moved out of
Header.jsinto a redux-query store middleware (store/posthogIdentify.js) keyed on a successful/api/v0/users/current_user/response, so it fires wherever that data loads regardless of which webpack bundle triggered it.Header.jswas the wrong layer — it's suppressed on cart, orders, records, and anything with?ecom-service, so those pages never identified even thoughApp.jsalready fetched the user there.base.htmlwraps only{% block headercontent %}in the suppression conditional andindex.htmlrenders therootbundle inside{% block content %}, so/cart?ecom-servicekeeps identifying.global_idis now also used for the feature-flag person properties (TopBar,CourseProductDetailEnroll) and for server-side flag evaluation (CheckoutInterstitialView,HomePage) — ol-django's_get_person_propertiesderivesuser_idfromopt_unique_id, so client identify, client flag overrides, and server-sideperson_propertiesnow agree on one value.The
email/nameperson properties the oldDashboardPageidentify sent are dropped deliberately; we don't set them anywhere else.Still uncovered: pages where the template suppresses the header and only the
djangobundle loads (/certificate/*,/checkout/*, Wagtail pages with?ecom-service). That bundle never reachesposthog.init()— a module side effect inlib/util.js— and each webpack entry gets its own posthog instance, soidentify()there is a no-op regardless of the caller. Not worth closing: certificate pages moved to the Next.js site,/checkout/is a transient redirect, and the financial-aid form is rare.Companion xPro PR for step 4: mitodl/mitxpro#4025