chore: migrate experiments -> library_assistant and cleanup experime… - #3571
Open
akiva10b wants to merge 1 commit into
Open
chore: migrate experiments -> library_assistant and cleanup experime…#3571akiva10b wants to merge 1 commit into
akiva10b wants to merge 1 commit into
Conversation
📊 Code Quality Score: 51/100
Was this score accurate? 👍 Yes · 👎 No Scored by GitVelocity · How are scores calculated? |
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.
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.experimentswas doing two jobs at once:UserExperimentSettingsrow in PostgresBecause 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_assistantbecomes the single source of truth for the assistant — a plain user setting alongsidereading_historyandinterface_language, on by default, toggleable by every logged-in user regardless of experiments enrollment.UserExperimentSettingsandprofile.experimentsare left in place and now mean only "participates in the experiments program". They gate nothing about the assistant.New:
sefaria/helper/library_assistant.pyThe 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:experimentsvalue inherit it, so anyone who deliberately turned the assistant off stays offTrueDecoupling
_is_user_in_experimentis gone.chatbot_user_token(context_processors.py) now gates on the setting alone — that's what drives the<script>tag inbase.html. Same forchatbot_enabled/chatbot_user_tokeninbase_props._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 vianotify_crm=Falsefor automated enrollment.in_chatbot_experimentis removed. Both the join-banner suppression (ReaderApp.jsx) and the Strapi banner suppression (Misc.jsx) readSefaria.library_assistant.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/profilecoerces the posted value — it's a public endpoint andbool("false")isTrue.The migration must run before or with the app deploy.
UserProfiledefaults the setting toTrue, 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.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:
experiments: FalseTrue; the widget keeps its own persisted stateTrue; with no persisted widget state the unconditionaldefault-open={true}opens itThis 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.pyagainst the setting. Repointed the webhook integration tests inchatbot_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
lc-chatbotbundle. We passdefault-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.library_assistant, matching every existing string in the codebase and the public/enable-library-assistantURL./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.