Skip to content

fix(sso): redesign auth analytics for login/register parity and outcome tracking - #3632

Open
YishaiGlasner wants to merge 3 commits into
masterfrom
feature/sc-46271/analytics-for-sso
Open

fix(sso): redesign auth analytics for login/register parity and outcome tracking#3632
YishaiGlasner wants to merge 3 commits into
masterfrom
feature/sc-46271/analytics-for-sso

Conversation

@YishaiGlasner

Copy link
Copy Markdown
Contributor

Summary

  • Renames the sign-up-only analytics vocabulary (sign_up_* events/constants/files) to the spec'd auth_* events, and fixes login attempts firing tracking events with a null flow_id and no bookending flow_started/flow_ended (the tracking hook was gated to flow === 'register' only).
  • Adds flow_intent (registration/login/one_tap_login) and outcome (created_new_account/existing_user_login) fields across all five events, and fixes LoginView.jsx, which never called endProcess at all.
  • Adds backend support so outcome is accurate for Google/Apple SSO too: a new /api/auth/google/callback endpoint (mirroring the existing Apple one) replaces a direct call to allauth's stock headless endpoint, which had no hook to report it; redirect-mode (mobile web) SSO gets the same signal via a new sefaria_sso_outcome cookie set through get_login_redirect_url/get_signup_redirect_url and the existing ClearSsoNextCookieMiddleware.

Test plan

  • npx jest static/js/auth — 102 tests passing across 7 suites
  • python manage.py test sso.tests.adapters_test sso.tests.middleware_test sso.tests.views_test — 67 tests passing
  • Manual smoke test in a browser: exercise /login and /register for email, Google (popup), Google One Tap, and Apple (popup), confirming a consistent flow_id/attempt_id sequence with correct flow_intent/outcome on success

🤖 Generated with Claude Code

…me tracking

The sign-up funnel analytics shipped alongside SSO were built for register only
and reused as-is for login, causing login attempts to fire method_chosen/
process_started/process_ended events with a null flow_id and no bookending
flow_started/flow_ended. Rename the whole sign_up_* vocabulary to the spec's
auth_* events, track login and register as equally first-class flows (a direct
register<->login transition now correctly ends the old flow and starts a new
one), add flow_intent and outcome fields, and fix LoginView never calling
endProcess at all.

Getting `outcome` (created_new_account vs existing_user_login) for Google/Apple
SSO required backend support: a new Sefaria-owned /api/auth/google/callback
endpoint (mirroring the existing Apple one) replaces a direct call to allauth's
stock headless endpoint, which had no hook to report it. Redirect-mode SSO
(mobile web) gets the same signal via a new sefaria_sso_outcome cookie, set by
extending get_login_redirect_url/get_signup_redirect_url and the existing
ClearSsoNextCookieMiddleware.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@gitvelocity-reviewer

Copy link
Copy Markdown

📊 Code Quality Score: 52/100

Base Score 65 × ESF 0.8 (Large: 682 effective lines, 20 files, 0-tier gap) = 52

Category Score Factors
🔭 Scope 15/20 20 files touched across sefaria/system/middleware.py, sso/adapters.py, sso/views.py, sso/urls.py, and 8 frontend files (authAnalytics.js, useAuthTracking.js, useSsoSignIn.jsx, GoogleOneTap.jsx, LoginView.jsx, RegisterView.jsx, AuthPage.jsx, ReaderApp.jsx) plus 6 test files. One new URL route (/api/auth/google/callback) and one new view (google_web) are introduced.
🏗️ Architecture 12/20 _social_login_or_error changes from a 2-tuple to a 3-tuple return, requiring updates at all four call sites (google_mobile, google_web, apple_callback, apple_mobile). useAuthTracking replaces useSignUpTracking with isTrackedFlow expanding the predicate from register-only to login+register. ALLAUTH_PROVIDER_TOKEN_URL is removed from utils.js and all callers now POST to /api/auth/google/callback. No new module boundary is introduced.
⚙️ Implementation 13/20 readAndClearOutcomeCookie() in authAnalytics.js implements a read-once-and-clear cookie relay using a regex match on document.cookie, clearing via max-age=0. resumePendingAuthAttempt threads the outcome through both the pending-attempt and active-flow synthesis paths. useAuthTracking's prevFlowRef replaces prevIsRegisterRef to handle direct register<->login transitions by ending the old flow and starting a new one. endProcess gains an outcome parameter stored in attemptRef.current.outcome and forwarded through endFlow.
⚠️ Risk 10/20 The 3-tuple change to _social_login_or_error touches all five SSO auth paths in production. apple_callback's response shape changes from {} to {outcome: ...}, which is additive but observable by any caller inspecting the response body. The ACTIVE_FLOW_KEY rename from sefaria_active_signup_flow to sefaria_active_auth_flow orphans any sessionStorage entry written before the deploy. The sefaria_sso_outcome cookie uses secure=True, which silently no-ops on HTTP (local dev and any non-HTTPS path).
✅ Quality 12/15 middleware_test.py adds ClearSsoNextCookieMiddlewareOutcomeTest with 4 direct process_response tests covering both SSO callback paths, the absent-flag case, and the unrelated-path case. adapters_test.py adds test_login_redirect_sets_existing_user_login_outcome, test_signup_redirect_sets_created_new_account_outcome, and a _sefaria_new_social_user assertion. views_test.py adds GoogleWebCallbackTest (5 tests) and 2 outcome assertions to AppleCallbackTest. authAnalytics.test.js adds a mockCookieJar helper and tests for the outcome cookie relay and flow_intent. useAuthTracking.test.js adds direct-transition tests and login-flow popstate/pageshow coverage. No E2E test covers the full redirect-mode SSO round trip.
🔒 Perf / Security 3/5 The sefaria_sso_outcome cookie is bounded to max_age=300 seconds and cleared immediately on read by readAndClearOutcomeCookie, preventing replay across unrelated page loads. samesite='None' and secure=True match the existing sefaria_sso_next cookie convention. No rate limiting is added to the new google_web endpoint, consistent with the existing apple_callback and google_mobile endpoints.

Was this score accurate? 👍 Yes · 👎 No

How this was scored →

Scored by GitVelocity · How are scores calculated?

@yitzhakc
yitzhakc requested review from yitzhakc and yodem August 23, 2026 10:37
PROCESS_ENDED: 'sign_up_process_ended',
FLOW_ENDED: 'sign_up_flow_ended',
export const AUTH_EVENT = {
FLOW_STARTED: 'auth_flow_started',

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[claude] Context for reviewers: this rename isn't unilateral — it ships the web half of the cross-platform event contract the team already documented, which mobile shipped on 2026-08-11, and that doc carries its own dashboard-impact note.

grep -rn "sign_up_" on master shows no remaining consumers in the repo, so the rename is clean internally. The open item is external: confirm the GA4/BigQuery saved queries and audiences keyed to sign_up_* are repointed, since this PR is what triggers that for web.


export function fireFlowStarted(flowId, source) {
sendEvent(SIGNUP_EVENT.FLOW_STARTED, { flow_id: flowId, source });
export function fireFlowStarted(flowId, source, flowIntent) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[claude] flow_intent is emitted only here on flow_started, and outcome only on process_ended/flow_ended (lines 41-45) — which matches the spec in the file header. The PR description says both are added "across all five events"; the description is what's off, not the code.

YishaiGlasner and others added 2 commits September 2, 2026 17:30
…values

QA found two bugs in the new auth analytics: (1) startProcess()/endProcess()
each had a one-shot guard, so retrying an email login/register submission
after a failure fired no further process_started, and a later successful
retry's endProcess call was silently dropped, leaving auth_flow_ended stuck
reporting the earlier failure; (2) several login entry points never set
data-signup-source, so auth_flow_started fired with source: null for the
register->login crosslink, the LA/chatbot banner, and the shared SignUpModal's
"Sign In" link (Save/AddToSheet interstitials).

startProcess() now only no-ops while an attempt is genuinely still in flight,
and re-arms (same attempt_id, fresh process_started) once the prior attempt
has concluded; LoginView/RegisterView call it on every submit rather than only
once. sourceForModalKind() takes a prefix so the modal's Sign In link can use
login_modal_* alongside its existing signup_modal_* Sign Up link.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants