docs(experiments): spec for editing a page while an experiment is active (#37308) - #37516
Open
oidacra wants to merge 5 commits into
Open
docs(experiments): spec for editing a page while an experiment is active (#37308)#37516oidacra wants to merge 5 commits into
oidacra wants to merge 5 commits into
Conversation
Spec for #37308: remove the RUNNING/SCHEDULED experiment condition from the UVE edit-capability rules and replace it with a persistent, non-dismissible warning banner carrying different copy per status. Grounded against the code rather than against the issue's description. Three findings change what the work actually delivers: - computeCanEditPage() has no production caller anywhere in the repository, so removing its experiment condition changes no runtime behavior. The live guard is entirely in withEditor.ts. - A SCHEDULED experiment never reaches the editor on an ordinary page load: pageAsset.runningExperimentId is filled from a query filtered to Status.RUNNING, so the scheduled banner can only render on the portlet arrival path #37005 added. - The guard is untested. withEditor.spec.ts asserts permission and lock cases and never experiment status, so every test the issue asks for is new coverage. Verified the issue's claim that no backend enforcement exists: no path rejects a contentlet or page save because an experiment is running, and ExperimentsAPIImpl subscribes to exactly two events, neither of which is a save or publish. 42 FRs, 14 SCs, 6 user stories, 7 resolved decisions and 5 open decisions for the coordinator. The largest open item is the banner's link target, which collides with #37478's FR-025c: that work takes the legacy full-screen results route out of service while FEATURE_FLAG_EXPERIMENTS_PORTLET is on. Epic: #36763 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#37308) Adversarially verified the "no backend enforcement" claim and, in doing so, found that the issue's model of the change is incomplete. Backend, now verified exhaustively rather than by a narrow grep: nine layers audited independently and each challenged by two reviewers briefed to find a block -- contentlet checkin, the REST endpoints UVE calls, permissions, the workflow engine, the variant domain, everything the experiments domain writes outward on start, every interception point (filters, WebInterceptors, CDI, the event subscriber registry, and the ContentletAPI pre/post hooks), push publishing, and test feasibility. No block exists. The structural reason: the save path holds no reference to the experiment domain, so there is nowhere for such a check to live -- the experiment is read when the page is rendered, never when it is saved. The finding that changes scope: removing the experiment guard does NOT make a variant editable. Two obstacles sit behind it and neither reads experiment status, so both survive this change. - $showWorkflowsActions (withView.ts:145-152) is `isEditMode && isDefaultVariant`, and <dot-uve-workflow-actions> has no other call site -- on a non-DEFAULT variant the Save/Publish control is never instantiated. - A save from the new Edit Content editor is coerced to the DEFAULT variant. EditContentDialogData carries no variant field and libs/edit-content has no variant handling, so the save fires the workflow endpoint with no variantName; WorkflowResource.java:5080 hard-codes VariantAPI.DEFAULT_VARIANT.name() and resolveContentletByVariant (:5164-5188) rewrites the contentlet to variantId = DEFAULT. Six of seven fire entry points use that overload. The legacy dialog is variant-correct, so exposure is bounded to content types carrying FEATURE_FLAG_CONTENT_EDITOR2_ENABLED. The second is a silent data-correctness bug: the editor believes they changed a variant and the change lands on the original. It is unreachable today because the guard stops the editor first. This change is what exposes it. Also corrected: the guard does not merely hide the edit affordance, it ejects -- $modeGuardEffect issues a real pageLoad({mode: PREVIEW}), and that branch is the default path since FEATURE_FLAG_UVE_TOGGLE_LOCK ships false. Spec changes: Scope Note grows to five findings; two rows added to the Verified Code Baseline and the backend row rewritten with the real evidence; FR-007 qualified and FR-007a added (an edit on a variant must reach that variant, and a save landing on DEFAULT is a failure, not a partial success); FR-008 now requires the workflow actions to be rendered; SC-001 scoped to the original variant with SC-001a gated on O6; new O6 with three options and a recommendation; O3, O4 and O5 marked resolved; A7 records that the primary scenario is unaffected. Epic: #36763 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
US1 acceptance scenario 3 still asserted flatly that editing a created variant is accepted and persists. The previous commit qualified FR-007, added FR-007a and gated SC-001a on O6, but left the user story saying the opposite. Scenario 3 now states that the edit must land on that variant rather than DEFAULT, and carries the same O6 gate as SC-001a: if O6 resolves to option A the scenario is struck along with FR-007, never reported as passing. Epic: #36763 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Corrects the previous commit's framing, which was wrong. A variant of a DRAFT experiment is editable today: the portlet opens non-control variants in EDIT whenever $isLocked is false (status !== DRAFT), and the save persists. Nothing on that path reads experiment status. So whatever makes a variant editable under DRAFT makes it editable under RUNNING once the guard is gone -- there is no separate variant-save problem for this issue to solve. What the previous commit got wrong: - $showWorkflowsActions was described as blocking variant editing. It does not. It gates <dot-uve-workflow-actions>, which fires on pageAsset().page.inode -- the PAGE's own workflow actions. Editing a contentlet goes through the dialog, a different path. Hiding page-level Save/Publish on a variant is existing, deliberate behavior, identical under DRAFT, and this change neither breaks nor must fix it. - The variant coercion was described as something this issue exposes. It is not. The path is reachable today under a DRAFT experiment with no running experiment and nothing from #37308. If it is a defect it is already a defect, for any variant edit on a content type carrying FEATURE_FLAG_CONTENT_EDITOR2_ENABLED. O6 is rewritten accordingly: from a scope-changing blocker with three options to a pre-existing, experiment-independent question parked for its own issue, with an explicit note that it has NOT been verified at runtime and must not be reported as a confirmed bug. The static analysis is inconclusive on its own, since resolveContentletByVariant returns the contentlet unchanged when the requested variant already matches -- which variant the edited contentlet lives in decides it, and reading the code does not settle that. New D8 records the decision this issue actually needs: release the portlet's variant mode for RUNNING and SCHEDULED, keep ENDED and ARCHIVED read-only. The shipped test's own rationale ("an ended or archived experiment's results are just as corruptible by an accidental edit") is strongest where D8 keeps it and weakest where it releases it. It must be done at dot-experiments-configure-variants.component.ts:193 and NOT by relaxing $isLocked, which also freezes the configuration form's name, description, traffic allocation, goal, scheduling and Save. This amends #37005 FR-009 and splits its describe.each, and extends the change into libs/portlets/dot-experiments -- still frontend, so the epic's rule holds. FR-007, FR-007a, SC-001a and US1 scenario 3 are restored to unconditional and re-pointed from O6 to D8. FR-007a becomes a hold-the-line requirement: introduce no new way for a variant edit to land on DEFAULT. SC-001a measures equivalence with the DRAFT case, so a divergence proves this change reached the save path. A7 rewritten. The "exposure risk" paragraph in Regression Risk is withdrawn. Only O1 and O2 remain open; neither blocks the guard removal, the banner, or FR-007. Epic: #36763 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rification (#37308) Runs /speckit-clarify's findings through dissect-idea. Five ambiguities were found; exploring the code settled two of them without needing a decision, and three were product calls. Settled by reading the code, no question needed: - Banner stability across reloads. pageLoad clears editorSelected and editorContentArea but NOT pageExperiment (withPageApi.ts:191-202), so the banner does not flicker on a mode change or reload. The one real case -- briefly carrying the previous page's experiment across a page-to-page navigation -- is added as an edge case. - Accessibility. No p-message anywhere in the repository declares role or aria, the lock banner included. The new banner follows that precedent rather than becoming the only one of its kind (A8). Decisions taken: - D9: when a page is both locked by another user and carries an active experiment, BOTH banners show, lock first. Suppressing the experiment banner while locked was rejected because the editor deciding whether to take the lock is exactly who benefits from knowing an experiment is running. FR-024 had demanded this be stated rather than left to template ordering, and the spec had never stated it -- the requirement contradicted itself until now. - D10: the banner is not gated on edit permission (FR-023a). Both existing precedents in the shell -- $showLockBanner and the toolbar's running-experiment tag -- consult canEdit nowhere. - D11: the data-integrity guarantee is proved by an integration test, not by manual QA (FR-040a). ExperimentDataGen.nextPersistedAndStart() already puts an experiment into RUNNING in one call, so the test that had been described all along as "not yet run" is writable today with no new infrastructure. It asserts both that collected measurements survive an edit AND which variant received the write -- the second assertion settles O6 with evidence, which static analysis cannot do because resolveContentletByVariant is conditional on which variant the contentlet already lives in. FR-040b requires registering it in a MainSuite, since an unregistered integration test compiles, leaves CI green and never runs. This puts a Java test in a frontend-only epic, deliberately: the claim under test is a backend claim, and the alternative was shipping the issue's central premise unverified. No production Java. Adds FR-023a, FR-040a, FR-040b, SC-015 through SC-017, A8, D9 through D11, a Clarifications section, and one edge case. O6 moves from an open suspicion to a question this issue answers. Epic: #36763 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
38 tasks
Contributor
|
Claude finished @oidacra's task in 3m 49s —— View job Spec review — #37308 "Editing a page while an experiment is active"
This is a spec-only PR (one added file, no code), so I reviewed it as a spec: accuracy of its code claims, internal consistency, and testability of its requirements. I independently checked every file/line/symbol reference against the tree. Code claims — all verified accurateTen of the spec's load-bearing claims were checked against the actual code and all confirmed, including exact line numbers:
The correction table and Scope Note are unusually rigorous — the "written against the code, not the issue" discipline holds up under checking. Findings
Not blockingThe spec is internally consistent on FR/SC/decision cross-references (O1/O2 open, O3/O4/O5 resolved, O6 recorded-not-blocking), the compound-expression regression hazard (FR-004/FR-012, Regression Risk) is called out correctly, and the |
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
1 participant
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 this delivers
A page carrying a
RUNNINGorSCHEDULEDexperiment is currently frozen for editing, which makesexperiments impractical on exactly the pages worth experimenting on. This spec removes that block and
replaces it with a persistent, non-dismissible warning.
editorHasAccessToEditMode,hasPermissionToEditLayoutandcomputeCanEditPage(). Lock and permission rules are untouched.p-message severity="warn"at the top of the UVE shell,modelled on the page-lock banner but with no close button, and with different copy for
RUNNINGand
SCHEDULED— framed as data pollution going forward, never as invalidating results alreadycollected.
RUNNINGand
SCHEDULEDexperiments in edit mode;ENDEDandARCHIVEDstay read-only, and the controlvariant stays read-only.
edits a variant, and asserts both that the collected measurements survive and which variant
received the write.
Corrections to the issue, from reading the code
The issue is accurate on the guard itself and on the absence of backend enforcement. Six of its
statements do not match the code, and the spec is written against the code:
computeCanEditPage()has no production caller. Its only references in the repository are itsown definition and its own unit test. Removing the experiment condition from it changes no runtime
behavior; the live guard is entirely in
withEditor.ts.docs/backend/EXPERIMENTS_CONSTRAINTS.mddoes not exist — not in the working tree, not inHEAD, and on no remote branch. The issue cites it twice.Language.propertiesline numbers are off by 8.experiment.running.edit.confirmationis at6042, not 6034. There is also a second dead string at 6043 with the same inaccurate "may
invalidate any results already collected" phrasing, which the issue does not mention.
SCHEDULEDexperiment never reaches the editor on an ordinary page load.runningExperimentIdcomes from a query filtered toStatus.RUNNING, so the scheduled half ofthe block only ever bit on the portlet arrival path.
runningIdsandlookBackWindoware not frontend-checkable. Both exist only on the backendmodel, so the data-integrity criteria are verified at the API level.
$modeGuardEffectissues a realpageLoad({mode: PREVIEW}), and that branch is the default path sinceFEATURE_FLAG_UVE_TOGGLE_LOCKships asfalse.No changes have been made to the issue.
What D8 implies for #37005
Releasing the variant mode for
RUNNINGandSCHEDULEDamends #37005 FR-009 ("every variant ofan experiment that is not a draft MUST open read-only") and splits the
describe.eachthat pins it:RUNNINGandSCHEDULEDflip to expecting edit mode,ENDEDandARCHIVEDkeep preview. Thechange belongs at
dot-experiments-configure-variants.component.ts:193and must not be made byrelaxing
$isLocked, which also freezes the configuration form's name, description, trafficallocation, goal, scheduling and Save.
Open decisions
FEATURE_FLAG_EXPERIMENTS_PORTLETis on. It currentlypoints at the legacy full-screen reports route, which Experiments Portlet — the page's experiments as a UVE panel, behind FEATURE_FLAG_EXPERIMENTS_PORTLET #37478 FR-025c takes out of service while the
flag is on.
After this change no experiment state forbids editing, so that wording needs revisiting.
Neither blocks the guard removal, the banner, or variant editing.
Closes #37308. Visual review page (summarizes; the spec decides): https://claude.ai/code/artifact/5b9f9f98-587c-4ac1-9766-5900989df333