feat(experiments): view results screen (/experiments/:id/results) - #37135
feat(experiments): view results screen (/experiments/:id/results)#37135oidacra wants to merge 7 commits into
Conversation
|
Claude finished @oidacra's task in 4m 41s —— View job Review — PR #37135 (Experiments Results screen)Reviewed the results store, events, shell, header, stat-strip, summary table, charts, the relocated utils, routing, models, and the new i18n keys. Verified all referenced message keys resolve (new ones added; the 7 pre-existing ones exist). One finding. New Issues
Everything else looks solid: the sequential DRAFT/SCHEDULED short-circuit, the keyed-vs-unkeyed confirm-dialog separation, the single-mounted-chart
|
e2f3e2a to
52d89e5
Compare
github-actions
Bot
added
Area : Frontend
523ebf6 to
03f1e9f
Compare
53b1086 to
3b79943
Compare
3b79943 to
2cbdc0f
Compare
2cbdc0f to
bb2df76
Compare
bb2df76 to
d3a772c
Compare
The Results screen needs the Chart.js options, the interactive legend plugin, the p-chart wrapper, the headless details table and the winner/promoted/chart-data helpers. All of them live under old/, which is frozen and slated for deletion, so they are relocated to shared/ and old/ is repointed at the new paths rather than duplicating them: one source of truth, and old/ stays deletable. Only import lines change under old/, so its behaviour is untouched and its specs still pass unmodified. Three importers the plan had not accounted for turned up in the Configure legacy screen and needed the same repoint. getConversionRate, getConversionRateRage, getBayesianVariantResult and getProbabilityToBeBest stay where they are: they are not needed outside old/, and the dependency direction has to remain old/ -> shared/, never the reverse.
State is {experiment, results, status, refreshing, lastRefreshFailed}, keyed by
experimentId alone, so the screen is page-independent the way the old reports
store already was.
The load runs sequentially rather than as the old screen's forkJoin, because the
second call depends on the first: DRAFT and SCHEDULED settle on getById alone and
never reach getResults, which is uncached and costs two analytics round-trips plus
a Monte Carlo run. The branch reads the experiment's status, not whether results
happen to be null.
A failed refresh leaves results and status untouched, so the last good report
stays on screen and only a flag says the refresh failed. A first load that loses
only its report keeps the experiment and reports inline; blanking there would
regress against the screen this one replaces.
Lift vs Original is computed here since no field carries it. The baseline is
resolved by key and isControl by name, never by row position, so the order rows
arrive in cannot change the arithmetic.
The refresh flag is raised only for the statuses the handler will actually serve;
raising it for the others would spin forever with no request in flight.
Wires :experimentId/results, reachable on every status including DRAFT, and hangs dotAnalyticsHealthCheckResolver off this route only so a broken analytics app takes out the report and leaves the list reachable. That resolver is a plain ResolveFn: it reports rather than redirects, so the screen reads the resolved status and renders the misconfiguration state in place of the report. The edit-page-coupled AnalyticsAppGuard is deliberately not reused. DotExperimentsService is provided on the route rather than the component. The resolver injects it and runs in the route injector, before the screen exists, so providing it on the screen alone would still throw NG0201 on activation. configureCommandsOf moves to the list util: the list's row action and the Results header's Configuration button are two ways to the same URL, and a URL spelled out twice is a URL that can drift.
Shell, header, stat strip, Daily/Bayesian tabs and summary table, per the approved prototype. Both charts reuse the relocated Chart.js options and legend plugin; the prototype's inline SVG is not reimplemented and the Bayesian posterior comes entirely from the backend. The two charts render in @if/@else so exactly one canvas is ever mounted: the legend plugin resolves its container by walking up from the canvas and calling querySelector, which from a shared ancestor returns the first match in document order, so two mounted charts would silently claim each other's legend. The stat strip is presentational and takes the winner legend from the backend's suggested winner, never from a locally computed highest rate, so "no winner yet" renders instead of a false Leading Variant claim. Goal and period come off the experiment, so they still read when the report does not. The session gate is experiment-wide: below ten total sessions the whole summary is one empty state, with no per-row filtering above it. Promote asks first from both entry points -- the strip's inline shortcut and the table's per-row button -- since promoting a RUNNING experiment ends it server-side in the same call. The table hosts its own dialog so the shell and the table never share a key, which would open both at once.
Leads the row's action cell, with Configure staying the first kebab entry, as the design lays it out. Ungated on purpose, unlike every kebab entry: AllowedActionsByExperimentStatus clears RUNNING and ENDED only, but the report renders a waiting state of its own for an experiment with nothing counted yet, so the row offers it on every status. Also drops the module-local configureCommandsOf for the shared helper.
…ns shed pill and icon Design review on the running screen. View Results leaves the row for the menu, where it leads: it is the only entry every status allows, since the Results screen renders its own waiting state for an experiment with nothing counted yet. This is a temporary home — the design gives it the row's own control — so the reason is recorded next to it rather than lost here. Configuration, Stop Experiment and the row Promote drop the pill and the leading material symbol, matching the split the Configure screen already settled on: labelled buttons render plain, icon-only controls keep the pill. Back and the refresh control are unchanged.
…he report It shipped disabled behind a "coming soon" tooltip because the screen it points at did not exist yet; it does now, so the TODO and the tooltip go with it. `resultsCommandsOf` moves next to its Configure twin in the shared util: the list's menu entry and this button are two ways to the same URL, and a URL spelled out twice is a URL that can drift. The button belongs in this branch rather than in the one that owns the file — enabling it where the results route does not exist would link to a 404. The two specs that pinned the disabled state now assert the navigation instead.
d3a772c to
1f9eb41
Compare
1 participant
What
Screen 3 of the Experiments portlet: the Results screen on
/experiments/:experimentId/results— header, stat strip, Daily/Bayesian tabs and summary table, with promote and stop.Closes #37004
Reuse over reimplementation
The charts, the legend plugin, the headless table and the winner/promoted helpers already existed under
old/, which is frozen and slated for deletion. Rather than duplicating them, they are relocated toshared/andold/is repointed at the new paths — one source of truth, andold/stays deletable. Only import lines change underold/; git records the moves as R100/R099/R097, andold/'s specs pass unmodified.The approved prototype drives visual and behavioural detail only.
Notable decisions
forkJoin. DRAFT and SCHEDULED settle ongetByIdalone and never reachgetResults, which is uncached and costs two analytics round-trips plus a Monte Carlo run. The branch readsexperiment.status, not whetherresultsis null.getByIdsucceeds andgetResultsfails, the header, goal and schedule still render and the failure is reported inline. The full error card is reserved for a missing experiment. Blanking here regressed against the screen this replaces — reproduced against a CubeJS schema without theEventscube, where/results400s whilegetByIdreturns 200.bayesianResult.suggestedWinneris what lets the "no winner yet" state exist at all.querySelector, which from a shared ancestor returns the first match in document order — two mounted charts would silently claim each other's legend, with no thrown error.@if/@elsemakes that impossible.ExperimentsAPIImpl.java:1409), so the strip's inline shortcut cannot be the one path that skips the dialog.isControlby name, so row order cannot change the arithmetic.Verification
nx test portlets-dot-experiments-portlettsc --noEmitnx affected -t lintnx format:checkFour projects (
dotcms-js,block-editor,dotcms-block-editor,dotcms-webcomponents) failnx affected -t testfor reasons unrelated to this branch: the onlydotcms-modelschange here is 8 purely additive lines,block-editorhas no dependency on experiments models, anddotcms-jsfails onCannot find module 'karma'.Exercised in the browser on the routed screen: header, stat strip, tabs and summary render; tabs switch with a single legend container; the negative winner state renders; goal and period read off the experiment when the report is missing.
Acceptance criteria
Lift vs Originalin points vs the control, green/red, em dash on control and on a zero-rate controlNot in this PR
htmlLegendPlugincould not be exercised end to end: without results data there is no canvas. Needs an environment with analytics data, or Experiments: A/B Testing v2 #36763's migration.Language.propertieskeys need a dotCMS restart to load; the raw key shows until then.