Skip to content

chore: migrate experiments -> library_assistant and cleanup experime… - #3571

Open
akiva10b wants to merge 1 commit into
masterfrom
migrate-experiment-to-la
Open

chore: migrate experiments -> library_assistant and cleanup experime…#3571
akiva10b wants to merge 1 commit into
masterfrom
migrate-experiment-to-la

Conversation

@akiva10b

@akiva10b akiva10b commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Graduates the Library Assistant out of the experiments program and into a plain user setting.

Closes sc-46171. Follow-up work tracked in sc-46240.

Problem

profiles.experiments was doing two jobs at once:

  • Eligibility for the experiments program — encoded as the existence of a UserExperimentSettings row in Postgres
  • Preference for the Library Assistant — the boolean itself

Because the assistant was the program's only member, nobody had to separate them. The cost was already visible in templates/account_settings.html, which carried the real "Experiments" toggle commented out, with a note explaining that both toggles wrote the same field so only one could ever ship.

The assistant is now a permanent feature, so the two concepts come apart.

Approach

profiles.settings.library_assistant becomes the single source of truth for the assistant — a plain user setting alongside reading_history and interface_language, on by default, toggleable by every logged-in user regardless of experiments enrollment.

UserExperimentSettings and profile.experiments are left in place and now mean only "participates in the experiments program". They gate nothing about the assistant.

New: sefaria/helper/library_assistant.py

The one module that names the setting key. Everything else imports from it, so the next graduation is a copy of one file rather than another 15-file sweep.

Migration

scripts/migrations/migrate_experiments_to_library_assistant.py — idempotent, re-runnable, supports --dry-run:

  • Profiles carrying an experiments value inherit it, so anyone who deliberately turned the assistant off stays off
  • Every other profile is backfilled to True

Decoupling

  • _is_user_in_experiment is gone. chatbot_user_token (context_processors.py) now gates on the setting alone — that's what drives the <script> tag in base.html. Same for chatbot_enabled / chatbot_user_token in base_props.
  • The CRM opt-in webhook moved off _set_user_experiments. It is a chatbot opt-in webhook, so it belongs to the assistant; it now fires from the profile API and /enable-library-assistant, and is suppressible via notify_crm=False for automated enrollment.
  • in_chatbot_experiment is removed. Both the join-banner suppression (ReaderApp.jsx) and the Strapi banner suppression (Misc.jsx) read Sefaria.library_assistant.
  • The promo banner's join button posts the setting directly instead of experimentsOptInAPI + editProfileAPI.

Settings page

The Library Assistant toggle renders unconditionally and writes into profile.settings. The Experiments toggle is fully commented out with restore instructions for the next experiment.

/api/profile coerces the posted value — it's a public endpoint and bool("false") is True.

⚠️ Deploy ordering

The migration must run before or with the app deploy.

UserProfile defaults the setting to True, and any profile save materializes defaults. An un-migrated opt-out whose profile is saved for any reason would silently flip to on. This is the one step in the change that can lose a user preference; everything else is recoverable.

python scripts/migrations/migrate_experiments_to_library_assistant.py --dry-run
python scripts/migrations/migrate_experiments_to_library_assistant.py

sc-46171 acceptance criteria

All three are satisfied at the next page load rather than at next login, which is strictly broader than the card asked for:

Card criterion How it's met
Opted in, then disabled → stays disabled Migration inherits experiments: False
Opted in and enabled → last open/minimized state Inherits True; the widget keeps its own persisted state
Never opted in → assistant opens automatically Backfilled True; with no persisted widget state the unconditional default-open={true} opens it

This also closes a gap the card's login-based framing left open: users with long-lived sessions who never log in again would otherwise have sat at assistant-off indefinitely.

Supersedes #3560

That PR's login hook is redundant once the backfill runs, and it had two defects: it fired a CRM webhook per auto-enrolled user (a Salesforce burst across the logged-in user base as they returned), and it granted the experiments whitelist as a side effect — the exact conflation this PR removes.

Testing

Added reader/tests/library_assistant_setting_test.py: default-on without enrollment, toggle round-trip, "false" coercion, CRM fires exactly once per real change, and script-tag gating.

Rewrote enable_library_assistant_test.py against the setting. Repointed the webhook integration tests in chatbot_webhook_test.py. The experiments admin tests now assert the absence of assistant side effects.

These have not been executed — the local env can't boot Django (ModuleNotFoundError: adminsortable, no pytest-django), and the suite runs in a k8s pod in CI. Verified statically: every changed Python and JS file parses, no references to removed symbols survive anywhere in the tree, and the template's comment block is balanced. Please confirm the CI run is green before merging.

Reviewer notes

  • One thing I could not verify from this repo: criterion 2's "last open/minimized state" lives inside the external lc-chatbot bundle. We pass default-open={true} unconditionally, so correct behavior for returning users depends on the bundle persisting its own state. Worth a line of confirmation from the LA team.
  • Naming: library_assistant, matching every existing string in the codebase and the public /enable-library-assistant URL.
  • Renaming, dead-code cleanup (the now-callerless /api/profile/experiments/opt-in), and the question of whether existing whitelist rows stay enrolled are deliberately out of scope here and tracked in sc-46240.

@gitvelocity-reviewer

Copy link
Copy Markdown

📊 Code Quality Score: 51/100

Base Score 64 × ESF 0.8 = 51.2 ≈ 51

Category Score Factors
🔭 Scope 14/20 10+ files across backend, frontend, templates, tests, and migration script; new public helper module API; cross-cutting impact on auth flow, profile API, CRM integration, template rendering, and JS state; no new external service integrations
🏗️ Architecture 14/20 New canonical helper module establishes pattern for future experiment graduation; removes conflation between experiments whitelist and assistant preference; decouples CRM webhook from experiments enrollment; deletes _is_user_in_experiment; no new external dependencies
⚙️ Implementation 12/20 normalize() handles string coercion edge cases on public API; change detection logic in profile_api; three-case idempotent migration script; cross-site request guard preserved; solid defensive programming throughout; no complex algorithms
⚠️ Risk 10/20 Data migration required with ordering constraint; behavioral change (assistant now on-by-default for all users, not just whitelist); CRM webhook behavior changes; migration ordering documented; no feature flag for rollout; rollback requires reverting migration and code
✅ Quality 12/15 New test file with ~85% coverage of new module; existing tests correctly updated; good docstrings explaining historical context; migration script has --dry-run; solid Django test client integration tests; missing test for notify_crm_of_change with empty email edge case
🔒 Perf / Security 2/5 Cross-site request guard preserved; string coercion on public API via normalize(); no performance benchmarks needed for this change type

Was this score accurate? 👍 Yes · 👎 No

How this was scored →

Scored by GitVelocity · How are scores calculated?

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.

1 participant