diff --git a/plugins/ui/apps/wizards/src/api/__tests__/wizardCohortApi.test.ts b/plugins/ui/apps/wizards/src/api/__tests__/wizardCohortApi.test.ts index 8a4426cd89..f0bffcc850 100644 --- a/plugins/ui/apps/wizards/src/api/__tests__/wizardCohortApi.test.ts +++ b/plugins/ui/apps/wizards/src/api/__tests__/wizardCohortApi.test.ts @@ -96,17 +96,21 @@ describe("Wizard cohort API", () => { }); it("materializes a Wizard bookmark with the existing cohort payload", async () => { - mockedPost.mockResolvedValue({ data: "Cohort successfully materialized" }); + mockedPost.mockResolvedValue({ + data: { message: "Cohort successfully materialized", cohortDefinitionId: 42 }, + }); const bookmark = buildMriBookmark([], {}, { configId: "pa-config", configVersion: "7" }, "dataset-1").bookmark; const mriQuery = buildMriMaterializationQuery(bookmark, "dataset-1"); - await materializeWizardBookmark({ - datasetId: "dataset-1", - bookmarkId: "bookmark-1", - bookmarkName: "wizards-1783670400000", - description: "Generated by Wizards", - mriQuery, - }); + await expect( + materializeWizardBookmark({ + datasetId: "dataset-1", + bookmarkId: "bookmark-1", + bookmarkName: "wizards-1783670400000", + description: "Generated by Wizards", + mriQuery, + }), + ).resolves.toEqual({ cohortDefinitionId: 42 }); expect(mockedPost).toHaveBeenCalledWith( "/d2e/analytics-svc/api/services/cohort", @@ -123,6 +127,20 @@ describe("Wizard cohort API", () => { expect(decompress(submittedPayload.mriquery)).toEqual(mriQuery); }); + it("rejects materialization when the backend omits the cohort definition id", async () => { + mockedPost.mockResolvedValue({ data: { message: "Cohort successfully materialized" } }); + const bookmark = buildMriBookmark([], {}, { configId: "pa-config", configVersion: "7" }, "dataset-1").bookmark; + + await expect( + materializeWizardBookmark({ + datasetId: "dataset-1", + bookmarkId: "bookmark-1", + bookmarkName: "wizards-1783670400000", + mriQuery: buildMriMaterializationQuery(bookmark, "dataset-1"), + }), + ).rejects.toMatchObject({ operation: "materialize-cohort", code: "invalid-response" }); + }); + it("rejects bookmark creation across datasets before making a request", async () => { const bookmark = buildMriBookmark([], {}, { configId: "pa-config", configVersion: "7" }, "dataset-2").bookmark; diff --git a/plugins/ui/apps/wizards/src/api/wizardCohortApi.ts b/plugins/ui/apps/wizards/src/api/wizardCohortApi.ts index 6f2e50d7fe..6f5b9d32d8 100644 --- a/plugins/ui/apps/wizards/src/api/wizardCohortApi.ts +++ b/plugins/ui/apps/wizards/src/api/wizardCohortApi.ts @@ -45,6 +45,10 @@ export interface MaterializeWizardBookmarkInput { mriQuery: MriMaterializationQuery; } +export interface MaterializeWizardBookmarkResult { + cohortDefinitionId: number; +} + const operationMessages: Record = { "list-cohorts": "Unable to check previous Wizard analyses", "create-bookmark": "Unable to save the Wizard analysis", @@ -126,13 +130,15 @@ export async function createWizardBookmark(input: CreateWizardBookmarkInput): Pr } } -export async function materializeWizardBookmark(input: MaterializeWizardBookmarkInput): Promise { +export async function materializeWizardBookmark( + input: MaterializeWizardBookmarkInput, +): Promise { const operation: WizardCohortApiOperation = "materialize-cohort"; requireValue(input.datasetId, "datasetId", operation); requireValue(input.bookmarkId, "bookmarkId", operation); requireValue(input.bookmarkName, "bookmarkName", operation); try { - await client.post( + const response = await client.post( "/d2e/analytics-svc/api/services/cohort", { datasetId: input.datasetId, @@ -143,6 +149,12 @@ export async function materializeWizardBookmark(input: MaterializeWizardBookmark }, { headers: { datasetid: input.datasetId } }, ); + const result = response.data as Partial | null; + const cohortDefinitionId = Number(result?.cohortDefinitionId); + if (!Number.isInteger(cohortDefinitionId) || cohortDefinitionId <= 0) { + throw new WizardCohortApiError(operationMessages[operation], operation, "invalid-response"); + } + return { cohortDefinitionId }; } catch (error) { throw wrapApiError(error, operation); } diff --git a/plugins/ui/apps/wizards/src/components/WizardDashboardModal.tsx b/plugins/ui/apps/wizards/src/components/WizardDashboardModal.tsx index 109ac46ed0..1b434fc3f8 100644 --- a/plugins/ui/apps/wizards/src/components/WizardDashboardModal.tsx +++ b/plugins/ui/apps/wizards/src/components/WizardDashboardModal.tsx @@ -4,10 +4,8 @@ import { ShinyDashboardIframe } from "./ShinyDashboardIframe"; import styles from "./WizardDashboardModal.module.css"; const stageMessages = { - "awaiting-cache": "Checking your previous Wizard analyses…", - "saving-bookmark": "Saving this Wizard analysis…", + "applying-filters": "Applying filters...", materializing: "Creating the cohort…", - "resolving-cohort": "Waiting for the cohort to become available…", "opening-dashboard": "Opening the dashboard…", } as const; diff --git a/plugins/ui/apps/wizards/src/hooks/useWizardDashboardFlow.ts b/plugins/ui/apps/wizards/src/hooks/useWizardDashboardFlow.ts index 07cbcd4aa3..eeedb79ebe 100644 --- a/plugins/ui/apps/wizards/src/hooks/useWizardDashboardFlow.ts +++ b/plugins/ui/apps/wizards/src/hooks/useWizardDashboardFlow.ts @@ -28,10 +28,8 @@ export interface OpenWizardDashboardInput { type ActiveStage = Exclude; const stageErrorMessages: Record = { - "awaiting-cache": "We couldn't check your previous Wizard analyses. Please try again.", - "saving-bookmark": "We couldn't save this Wizard analysis. Please try again.", + "applying-filters": "We couldn't apply the filters. Please try again.", materializing: "We couldn't generate the cohort. Please try again.", - "resolving-cohort": "The cohort is taking longer than expected. Please try again.", "opening-dashboard": "We couldn't open the dashboard. Please try again.", }; @@ -56,8 +54,7 @@ export function useWizardDashboardFlow({ const abortRef = useRef(null); const lastInputRef = useRef(null); const pendingBookmarkRef = useRef(null); - const materializationSubmittedRef = useRef(null); - const activeStageRef = useRef("awaiting-cache"); + const activeStageRef = useRef("applying-filters"); const loadInputRef = useRef<(() => Promise) | null>(null); const execute = useCallback( @@ -68,7 +65,7 @@ export function useWizardDashboardFlow({ abortRef.current = controller; const flowInput = { ...input, signal: controller.signal }; lastInputRef.current = flowInput; - activeStageRef.current = "awaiting-cache"; + activeStageRef.current = "applying-filters"; dispatch({ type: "start", operationId, @@ -88,10 +85,6 @@ export function useWizardDashboardFlow({ if (lastInputRef.current) lastInputRef.current.pendingBookmark = pendingBookmark; dispatch({ type: "bookmark-name", operationId, bookmarkName: pendingBookmark.bookmarkName }); }, - onMaterializationSubmitted: (bookmarkId) => { - materializationSubmittedRef.current = bookmarkId; - if (lastInputRef.current) lastInputRef.current.materializationSubmittedForBookmarkId = bookmarkId; - }, }) .then((result) => { dispatch({ type: "ready", operationId, result }); @@ -114,7 +107,6 @@ export function useWizardDashboardFlow({ (loadInput: () => Promise) => { loadInputRef.current = loadInput; pendingBookmarkRef.current = null; - materializationSubmittedRef.current = null; const operationId = ++operationIdRef.current; abortRef.current?.abort(); const controller = new AbortController(); @@ -129,7 +121,7 @@ export function useWizardDashboardFlow({ type: "fail", operationId, message: "The active dataset configuration is incomplete. The Cohort Builder option is still available.", - stage: "awaiting-cache", + stage: "applying-filters", }); return; } @@ -166,8 +158,6 @@ export function useWizardDashboardFlow({ execute({ ...input, pendingBookmark: pendingBookmarkRef.current ?? input.pendingBookmark, - materializationSubmittedForBookmarkId: - materializationSubmittedRef.current ?? input.materializationSubmittedForBookmarkId, }); }, [execute, openDashboard]); @@ -180,7 +170,6 @@ export function useWizardDashboardFlow({ abortRef.current?.abort(); lastInputRef.current = null; pendingBookmarkRef.current = null; - materializationSubmittedRef.current = null; loadInputRef.current = null; dispatch({ type: "dataset-changed", datasetId }); }, [datasetId]); diff --git a/plugins/ui/apps/wizards/src/services/__tests__/wizardDashboardFlow.test.ts b/plugins/ui/apps/wizards/src/services/__tests__/wizardDashboardFlow.test.ts index 48affa5125..c2c332fba7 100644 --- a/plugins/ui/apps/wizards/src/services/__tests__/wizardDashboardFlow.test.ts +++ b/plugins/ui/apps/wizards/src/services/__tests__/wizardDashboardFlow.test.ts @@ -34,10 +34,10 @@ describe("Wizard dashboard flow", () => { expect(materializeBookmark).not.toHaveBeenCalled(); }); - it("materializes an existing match and refreshes the bookmark list once", async () => { + it("uses the materialization response without waiting for the bookmark refresh", async () => { const createBookmark = vi.fn(); - const materializeBookmark = vi.fn().mockResolvedValue(undefined); - const refreshCache = vi.fn().mockResolvedValue([bookmarkItem({ cohortDefinitionId: 42 })]); + const materializeBookmark = vi.fn().mockResolvedValue({ cohortDefinitionId: 42 }); + const refreshCache = vi.fn().mockImplementation(() => new Promise(() => undefined)); const result = await runWizardDashboardFlow(baseInput, { ensureCache: vi.fn().mockResolvedValue([bookmarkItem()]), @@ -52,13 +52,10 @@ describe("Wizard dashboard flow", () => { expect(result).toMatchObject({ cohortId: 42, cacheOutcome: "hit-unmaterialized" }); }); - it("uses the returned bookmark id, materializes, then refreshes once for the cohort id", async () => { + it("uses the returned bookmark and cohort ids for a new analysis", async () => { const createBookmark = vi.fn().mockResolvedValue({ status: "success", bmkId: "created-bookmark" }); - const materializeBookmark = vi.fn().mockResolvedValue(undefined); - const refreshCache = vi - .fn() - .mockResolvedValueOnce([]) - .mockResolvedValueOnce([bookmarkItem({ bmkId: "created-bookmark", cohortDefinitionId: 42 })]); + const materializeBookmark = vi.fn().mockResolvedValue({ cohortDefinitionId: 42 }); + const refreshCache = vi.fn().mockResolvedValue([]); const onBookmarkCreated = vi.fn(); const stages: string[] = []; @@ -80,25 +77,19 @@ describe("Wizard dashboard flow", () => { bmkId: "created-bookmark", bookmarkName: "wizards-1783670400000", }); - expect(stages).toEqual([ - "awaiting-cache", - "saving-bookmark", - "materializing", - "resolving-cohort", - "opening-dashboard", - ]); + expect(stages).toEqual(["applying-filters", "materializing", "opening-dashboard"]); }); it("reuses a saved bookmark id on retry instead of saving again", async () => { const createBookmark = vi.fn(); - const materializeBookmark = vi.fn().mockResolvedValue(undefined); + const materializeBookmark = vi.fn().mockResolvedValue({ cohortDefinitionId: 9 }); const pendingBookmark = { bmkId: "created-bookmark", bookmarkName: "wizards-1783670400000" }; const result = await runWizardDashboardFlow( { ...baseInput, pendingBookmark }, { ensureCache: vi.fn().mockResolvedValue([]), - refreshCache: vi.fn().mockResolvedValue([bookmarkItem({ bmkId: "created-bookmark", cohortDefinitionId: 9 })]), + refreshCache: vi.fn().mockResolvedValue([]), createBookmark, materializeBookmark, }, @@ -124,33 +115,6 @@ describe("Wizard dashboard flow", () => { expect(materializeBookmark).not.toHaveBeenCalled(); }); - it("refreshes without materializing again after submission already completed", async () => { - const materializeBookmark = vi.fn(); - const refreshCache = vi.fn().mockResolvedValue([bookmarkItem({ cohortDefinitionId: 42 })]); - - await runWizardDashboardFlow( - { ...baseInput, materializationSubmittedForBookmarkId: "bookmark-1" }, - { - ensureCache: vi.fn().mockResolvedValue([bookmarkItem()]), - refreshCache, - materializeBookmark, - }, - ); - - expect(materializeBookmark).not.toHaveBeenCalled(); - expect(refreshCache).toHaveBeenCalledTimes(1); - }); - - it("fails after the single refresh when no cohort id is returned", async () => { - await expect( - runWizardDashboardFlow(baseInput, { - ensureCache: vi.fn().mockResolvedValue([bookmarkItem()]), - refreshCache: vi.fn().mockResolvedValue([bookmarkItem()]), - materializeBookmark: vi.fn().mockResolvedValue(undefined), - }), - ).rejects.toThrow("materialized Wizard cohort was not returned"); - }); - it("generates only the strict timestamp bookmark format", () => { expect(createWizardBookmarkName(1783670400000)).toBe("wizards-1783670400000"); expect(() => createWizardBookmarkName(123)).toThrow(); diff --git a/plugins/ui/apps/wizards/src/services/__tests__/wizardDashboardState.test.ts b/plugins/ui/apps/wizards/src/services/__tests__/wizardDashboardState.test.ts index d696d25976..8ff067670f 100644 --- a/plugins/ui/apps/wizards/src/services/__tests__/wizardDashboardState.test.ts +++ b/plugins/ui/apps/wizards/src/services/__tests__/wizardDashboardState.test.ts @@ -2,14 +2,14 @@ import { describe, expect, it } from "vitest"; import { initialWizardDashboardState, wizardDashboardReducer } from "../wizardDashboardState"; describe("wizard dashboard state", () => { - it("opens immediately in the cache-waiting stage", () => { + it("opens immediately in the filter-application stage", () => { const state = wizardDashboardReducer(initialWizardDashboardState, { type: "start", operationId: 1, datasetId: "dataset-1", }); - expect(state).toMatchObject({ isOpen: true, status: "awaiting-cache", operationId: 1 }); + expect(state).toMatchObject({ isOpen: true, status: "applying-filters", operationId: 1 }); }); it("ignores late events from a superseded operation", () => { @@ -19,10 +19,10 @@ describe("wizard dashboard state", () => { operationId: 2, datasetId: "dataset-1", }), - { type: "fail", operationId: 1, message: "late failure", stage: "saving-bookmark" } + { type: "fail", operationId: 1, message: "late failure", stage: "applying-filters" }, ); - expect(state.status).toBe("awaiting-cache"); + expect(state.status).toBe("applying-filters"); expect(state.error).toBeNull(); }); @@ -41,14 +41,14 @@ describe("wizard dashboard state", () => { type: "fail", operationId: 1, message: "request failed", - stage: "saving-bookmark", + stage: "applying-filters", }); expect(state).toMatchObject({ status: "error", pendingBookmarkName: "wizards-1783670400000", error: "request failed", - errorStage: "saving-bookmark", + errorStage: "applying-filters", }); }); diff --git a/plugins/ui/apps/wizards/src/services/wizardDashboardFlow.ts b/plugins/ui/apps/wizards/src/services/wizardDashboardFlow.ts index d6da573cd6..d09d1bc2df 100644 --- a/plugins/ui/apps/wizards/src/services/wizardDashboardFlow.ts +++ b/plugins/ui/apps/wizards/src/services/wizardDashboardFlow.ts @@ -4,6 +4,7 @@ import { type CreateWizardBookmarkResult, type CreateWizardBookmarkInput, type MaterializeWizardBookmarkInput, + type MaterializeWizardBookmarkResult, } from "../api/wizardCohortApi"; import type { MriBookmark } from "../utils/mriQuery"; import { buildMriMaterializationQuery } from "../utils/mriMaterializationQuery"; @@ -31,7 +32,6 @@ export interface RunWizardDashboardFlowInput { bookmark: MriBookmark; wizardConfig: Record; pendingBookmark?: PendingWizardBookmark | null; - materializationSubmittedForBookmarkId?: string | null; signal?: AbortSignal; } @@ -39,11 +39,10 @@ export interface WizardDashboardFlowDependencies { ensureCache: () => Promise; refreshCache: () => Promise; createBookmark?: (input: CreateWizardBookmarkInput) => Promise; - materializeBookmark?: (input: MaterializeWizardBookmarkInput) => Promise; + materializeBookmark?: (input: MaterializeWizardBookmarkInput) => Promise; now?: () => number; onStage?: (stage: FlowStage) => void; onBookmarkCreated?: (bookmark: PendingWizardBookmark) => void; - onMaterializationSubmitted?: (bookmarkId: string) => void; } export function createWizardBookmarkName(now = Date.now()): string { @@ -71,7 +70,7 @@ export async function runWizardDashboardFlow( const stage = dependencies.onStage ?? (() => undefined); throwIfAborted(input.signal); - stage("awaiting-cache"); + stage("applying-filters"); let items = await dependencies.ensureCache(); throwIfAborted(input.signal); @@ -100,7 +99,6 @@ export async function runWizardDashboardFlow( if (!candidate) { if (!bookmarkId || !bookmarkName) { bookmarkName = createWizardBookmarkName((dependencies.now ?? Date.now)()); - stage("saving-bookmark"); const created = await createBookmark({ datasetId: input.datasetId, bookmarkname: bookmarkName, @@ -120,25 +118,16 @@ export async function runWizardDashboardFlow( const mriQuery = buildMriMaterializationQuery(input.bookmark, input.datasetId); if (cohortDefinitionId === undefined) { - if (input.materializationSubmittedForBookmarkId !== bookmarkId) { - stage("materializing"); - await materializeBookmark({ - datasetId: input.datasetId, - bookmarkId, - bookmarkName, - mriQuery, - }); - dependencies.onMaterializationSubmitted?.(bookmarkId); - throwIfAborted(input.signal); - } - stage("resolving-cohort"); - const refreshedItems = await dependencies.refreshCache(); + stage("materializing"); + const materialized = await materializeBookmark({ + datasetId: input.datasetId, + bookmarkId, + bookmarkName, + mriQuery, + }); throwIfAborted(input.signal); - const refreshedBookmark = findWizardBookmarkById(refreshedItems, scope, bookmarkId); - if (refreshedBookmark?.cohortDefinitionId === undefined) { - throw new Error("The materialized Wizard cohort was not returned by the bookmark list"); - } - cohortDefinitionId = refreshedBookmark.cohortDefinitionId; + cohortDefinitionId = materialized.cohortDefinitionId; + void dependencies.refreshCache().catch(() => undefined); } stage("opening-dashboard"); diff --git a/plugins/ui/apps/wizards/src/services/wizardDashboardState.ts b/plugins/ui/apps/wizards/src/services/wizardDashboardState.ts index 632b15c9fd..85b98a533a 100644 --- a/plugins/ui/apps/wizards/src/services/wizardDashboardState.ts +++ b/plugins/ui/apps/wizards/src/services/wizardDashboardState.ts @@ -1,9 +1,7 @@ export type WizardDashboardStatus = | "idle" - | "awaiting-cache" - | "saving-bookmark" + | "applying-filters" | "materializing" - | "resolving-cohort" | "opening-dashboard" | "ready" | "error"; @@ -62,7 +60,7 @@ export function wizardDashboardReducer(state: WizardDashboardState, event: Wizar case "start": return { isOpen: true, - status: "awaiting-cache", + status: "applying-filters", operationId: event.operationId, datasetId: event.datasetId, pendingBookmarkName: event.pendingBookmarkName ?? null,