From fdd03b5260a27ab099be816051a09714f4c7021e Mon Sep 17 00:00:00 2001 From: Benjamin Kane Date: Wed, 7 Aug 2024 14:36:32 -0400 Subject: [PATCH] Fix modal linking (#4618) * id fixes, testing updates * cleanup * update for searchParams page load * undo --- app/packages/app/src/useEventSource.ts | 2 +- .../app/src/useEvents/useStateUpdate.ts | 4 +- app/packages/app/src/useEvents/utils.ts | 120 +++++++++++++----- .../app/src/useSetters/onSetDataset.ts | 2 +- app/packages/utilities/src/index.ts | 4 +- e2e-pw/src/oss/fixtures/loader.ts | 16 ++- .../oss/specs/groups/dynamic-groups.spec.ts | 4 +- .../groups/sparse-dynamic-groups.spec.ts | 2 +- e2e-pw/src/oss/specs/modal/linking.spec.ts | 61 +++++++++ .../specs/regression-tests/grid-page.spec.ts | 2 +- .../quickstart-groups-dynamic.spec.ts | 2 +- e2e-pw/src/shared/abstract-loader.ts | 4 +- 12 files changed, 173 insertions(+), 50 deletions(-) create mode 100644 e2e-pw/src/oss/specs/modal/linking.spec.ts diff --git a/app/packages/app/src/useEventSource.ts b/app/packages/app/src/useEventSource.ts index 8893099f06..439a2536bf 100644 --- a/app/packages/app/src/useEventSource.ts +++ b/app/packages/app/src/useEventSource.ts @@ -58,7 +58,7 @@ const useEventSource = ( dataset: getDatasetName(), group_id: getParam("groupId"), group_slice: getParam("slice"), - sample_id: getParam("sampleId"), + sample_id: getParam("id"), view: getParam("view"), workspace: getParam("workspace"), }, diff --git a/app/packages/app/src/useEvents/useStateUpdate.ts b/app/packages/app/src/useEvents/useStateUpdate.ts index 5d8ce4bcd7..f87572ed5e 100644 --- a/app/packages/app/src/useEvents/useStateUpdate.ts +++ b/app/packages/app/src/useEvents/useStateUpdate.ts @@ -29,8 +29,8 @@ const useStateUpdate: EventHandlerHook = ({ : (payload.state.saved_view_slug as string), extra: { groupId: state.modalSelector?.groupId || null, - slice: state.groupSlice || null, - sampleId: state.modalSelector?.id || null, + id: state.modalSelector?.id || null, + slice: stateless ? getParam("slice") : state.groupSlice || null, workspace: state.workspace?._name || null, }, }); diff --git a/app/packages/app/src/useEvents/utils.ts b/app/packages/app/src/useEvents/utils.ts index 7d16721413..58976bb7ea 100644 --- a/app/packages/app/src/useEvents/utils.ts +++ b/app/packages/app/src/useEvents/utils.ts @@ -7,6 +7,7 @@ import { env, toCamelCase } from "@fiftyone/utilities"; import { atom } from "recoil"; import type { DatasetPageQuery } from "../pages/datasets/__generated__/DatasetPageQuery.graphql"; import type { LocationState } from "../routing"; +import { getParam } from "../utils"; import { AppReadyState } from "./registerEvent"; export const appReadyState = atom({ @@ -23,43 +24,102 @@ export const processState = ( state.color_scheme as ColorSchemeInput ); - if (env().VITE_NO_STATE) { - session.sessionGroupSlice = data.dataset?.defaultGroupSlice || undefined; - unsubscribe(); - return; - } - - session.sessionGroupSlice = state.group_slice as string; - session.selectedLabels = toCamelCase( - state.selected_labels as object - ) as State.SelectedLabel[]; - session.selectedSamples = new Set(state.selected as string[]); - session.sessionSpaces = (state.spaces || - session.sessionSpaces) as SpaceNodeJSON; - session.fieldVisibilityStage = - (state.field_visibility_stage as State.FieldVisibilityStage) || undefined; + session.sessionGroupSlice = + groupSlice || data.dataset?.defaultGroupSlice || undefined; + session.selectedLabels = resolveSelectedLabels(state); + session.selectedSamples = resolveSelected(state); + session.sessionSpaces = workspace; + session.fieldVisibilityStage = fieldVisibility; session.modalSelector = modalSelector; unsubscribe(); }); - if (env().VITE_NO_STATE) { - return { view: [], fieldVisibility: undefined }; - } - - const modalSelector = - state.group_id || state.sample_id - ? { - groupId: state.group_id as string, - id: state.sample_id as string, - } - : undefined; + const fieldVisibility = resolveFieldVisibility(state); + const groupSlice = resolveGroupSlice(state); + const modalSelector = resolveModalSelector(state); + const view = resolveView(state); + const workspace = resolveWorkspace(session, state); return { - fieldVisibility: state.field_visibility_stage as State.FieldVisibilityStage, - groupSlice: state.group_slice as string, + fieldVisibility, + groupSlice, modalSelector, - view: state.view || [], - workspace: state.spaces as SpaceNodeJSON, + view, + workspace, }; }; +const resolveSelected = (state: { selected?: string[] }) => { + if (env().VITE_NO_STATE) { + return new Set(); + } + + return new Set(state.selected || []); +}; + +const resolveSelectedLabels = (state: { selected_labels?: string[] }) => { + if (env().VITE_NO_STATE) { + return []; + } + + return ( + (toCamelCase(state.selected_labels as object) as State.SelectedLabel[]) || + [] + ); +}; + +const resolveFieldVisibility = (state: { + field_visibility?: State.FieldVisibilityStage; +}) => { + if (env().VITE_NO_STATE) { + return undefined; + } + + return state.field_visibility; +}; + +const resolveGroupSlice = (state: { group_slice?: string }) => { + if (env().VITE_NO_STATE) { + return getParam("slice") || undefined; + } + + return state.group_slice; +}; + +const resolveModalSelector = (state: { + group_id?: string; + sample_id?: string; +}) => { + if (env().VITE_NO_STATE) { + const groupId = getParam("groupId") || undefined; + const id = getParam("id") || undefined; + + return groupId || id ? { groupId, id } : undefined; + } + + return state.group_id || state.sample_id + ? { + groupId: state.group_id as string, + id: state.sample_id as string, + } + : undefined; +}; + +const resolveView = (state: { view?: object[] }) => { + if (env().VITE_NO_STATE) { + return []; + } + + return state.view || []; +}; + +const resolveWorkspace = ( + session: Session, + state: { spaces?: SpaceNodeJSON } +) => { + if (env().VITE_NO_STATE) { + return session.sessionSpaces; + } + + return state.spaces || session.sessionSpaces; +}; diff --git a/app/packages/app/src/useSetters/onSetDataset.ts b/app/packages/app/src/useSetters/onSetDataset.ts index 93c7a5740f..3e0308d02a 100644 --- a/app/packages/app/src/useSetters/onSetDataset.ts +++ b/app/packages/app/src/useSetters/onSetDataset.ts @@ -48,7 +48,7 @@ const onSetDataset: RegisteredSetter = nextDataset: datasetName || null, extra: { groupId: null, - sampleId: null, + id: null, slice: null, workspace: null, }, diff --git a/app/packages/utilities/src/index.ts b/app/packages/utilities/src/index.ts index b5fbe3c66f..b7918836c8 100644 --- a/app/packages/utilities/src/index.ts +++ b/app/packages/utilities/src/index.ts @@ -4,16 +4,16 @@ import mime from "mime"; import { isElectron } from "./electron"; import { Field } from "./schema"; -export * from "./Resource"; export * from "./color"; export * from "./electron"; export * from "./errors"; export * from "./fetch"; +export * from "./order"; export * from "./paths"; +export * from "./Resource"; export * from "./schema"; export * from "./styles"; export * from "./type-check"; -export * from "./order"; interface O { [key: string]: O | any; diff --git a/e2e-pw/src/oss/fixtures/loader.ts b/e2e-pw/src/oss/fixtures/loader.ts index 5bcbafc281..b794922ef8 100644 --- a/e2e-pw/src/oss/fixtures/loader.ts +++ b/e2e-pw/src/oss/fixtures/loader.ts @@ -121,15 +121,15 @@ export class OssLoader extends AbstractFiftyoneLoader { datasetName: string, options?: WaitUntilGridVisibleOptions ) { - const { isEmptyDataset, savedView, withGrid } = options ?? { + const { isEmptyDataset, searchParams, withGrid } = options ?? { isEmptyDataset: false, - savedView: undefined, + searchParams: undefined, withGrid: true, }; const forceDatasetFromSelector = async () => { await page.goto("/"); - await page.getByTestId(`selector-Select dataset`).click(); + await page.getByTestId("selector-Select dataset").click(); if (datasetName) { await page.getByTestId(`selector-result-${datasetName}`).click(); @@ -141,8 +141,9 @@ export class OssLoader extends AbstractFiftyoneLoader { } }; - if (savedView) { - await page.goto(`/datasets/${datasetName}?view=${savedView}`); + const search = searchParams ? searchParams.toString() : undefined; + if (search) { + await page.goto(`/datasets/${datasetName}?${search}`); } else { await page.goto(`/datasets/${datasetName}`); } @@ -152,11 +153,12 @@ export class OssLoader extends AbstractFiftyoneLoader { await forceDatasetFromSelector(); } - if (savedView) { + const view = searchParams?.get("view"); + if (view) { const search = await page.evaluate(() => window.location.search); const params = new URLSearchParams(search); - if (params.get("view") !== savedView) { + if (params.get("view") !== view) { throw new Error(`wrong view: '${params.get("view")}'`); } } diff --git a/e2e-pw/src/oss/specs/groups/dynamic-groups.spec.ts b/e2e-pw/src/oss/specs/groups/dynamic-groups.spec.ts index 5b266d80b0..bb0d767b4f 100644 --- a/e2e-pw/src/oss/specs/groups/dynamic-groups.spec.ts +++ b/e2e-pw/src/oss/specs/groups/dynamic-groups.spec.ts @@ -54,7 +54,7 @@ extensionDatasetNamePairs.forEach(([extension, datasetName]) => { modal, }) => { await fiftyoneLoader.waitUntilGridVisible(page, datasetName, { - savedView: "dynamic-group", + searchParams: new URLSearchParams({ view: "dynamic-group" }), }); await grid.assert.isEntryCountTextEqualTo("10 groups"); @@ -74,7 +74,7 @@ extensionDatasetNamePairs.forEach(([extension, datasetName]) => { modal, }) => { await fiftyoneLoader.waitUntilGridVisible(page, datasetName, { - savedView: "dynamic-group", + searchParams: new URLSearchParams({ view: "dynamic-group" }), }); await grid.openFirstSample(); diff --git a/e2e-pw/src/oss/specs/groups/sparse-dynamic-groups.spec.ts b/e2e-pw/src/oss/specs/groups/sparse-dynamic-groups.spec.ts index d17688c402..9e11346e24 100644 --- a/e2e-pw/src/oss/specs/groups/sparse-dynamic-groups.spec.ts +++ b/e2e-pw/src/oss/specs/groups/sparse-dynamic-groups.spec.ts @@ -87,7 +87,7 @@ test(`left slice (default)`, async ({ fiftyoneLoader, page, grid, modal }) => { test(`right slice`, async ({ fiftyoneLoader, page, grid, modal }) => { await fiftyoneLoader.waitUntilGridVisible(page, datasetName, { - savedView: "group", + searchParams: new URLSearchParams({ view: "group" }), }); await grid.selectSlice("right"); await grid.assert.isEntryCountTextEqualTo("1 group with slice"); diff --git a/e2e-pw/src/oss/specs/modal/linking.spec.ts b/e2e-pw/src/oss/specs/modal/linking.spec.ts new file mode 100644 index 0000000000..1181ccbe8b --- /dev/null +++ b/e2e-pw/src/oss/specs/modal/linking.spec.ts @@ -0,0 +1,61 @@ +import { test as base } from "src/oss/fixtures"; +import { ModalPom } from "src/oss/poms/modal"; +import { getUniqueDatasetNameWithPrefix } from "src/oss/utils"; + +const test = base.extend<{ modal: ModalPom }>({ + modal: async ({ page, eventUtils }, use) => { + await use(new ModalPom(page, eventUtils)); + }, +}); + +const datasetName = getUniqueDatasetNameWithPrefix("linking"); +const groupDatasetName = getUniqueDatasetNameWithPrefix("group-linking"); + +const id = "000000000000000000000000"; + +test.beforeAll(async ({ fiftyoneLoader }) => { + await fiftyoneLoader.executePythonCode(` + from bson import ObjectId + + import fiftyone as fo + + dataset = fo.Dataset("${datasetName}") + dataset.persistent = True + + sample = fo.Sample(_id=ObjectId("${id}"), filepath="sample.png") + dataset._sample_collection.insert_many( + [dataset._make_dict(sample, include_id=True)] + ) + + + group_dataset = fo.Dataset("${groupDatasetName}") + group_dataset.persistent = True + + group = fo.Group(id="${id}") + group_sample = fo.Sample( + filepath="group_sample.png", group=group.element("only") + ) + group_dataset.add_sample(group_sample)`); +}); + +test(`sample linking`, async ({ page, fiftyoneLoader, modal }) => { + await fiftyoneLoader.waitUntilGridVisible(page, datasetName, { + searchParams: new URLSearchParams({ id }), + }); + + await modal.waitForSampleLoadDomAttribute(true); + + await modal.assert.isOpen(); + await modal.sidebar.assert.verifySidebarEntryText("id", id); +}); + +test(`group linking`, async ({ page, fiftyoneLoader, modal }) => { + await fiftyoneLoader.waitUntilGridVisible(page, groupDatasetName, { + searchParams: new URLSearchParams({ groupId: id }), + }); + + await modal.waitForSampleLoadDomAttribute(true); + + await modal.assert.isOpen(); + await modal.sidebar.assert.verifySidebarEntryText("group.id", id); +}); diff --git a/e2e-pw/src/oss/specs/regression-tests/grid-page.spec.ts b/e2e-pw/src/oss/specs/regression-tests/grid-page.spec.ts index b339b17b06..9a9a75eff1 100644 --- a/e2e-pw/src/oss/specs/regression-tests/grid-page.spec.ts +++ b/e2e-pw/src/oss/specs/regression-tests/grid-page.spec.ts @@ -69,7 +69,7 @@ test("modal dynamic group carousel has correct second page (all 21 samples)", as page, }) => { await fiftyoneLoader.waitUntilGridVisible(page, datasetName, { - savedView: "group", + searchParams: new URLSearchParams({ view: "group" }), }); await grid.openFirstSample(); await modal.group.setDynamicGroupsNavigationMode("carousel"); diff --git a/e2e-pw/src/oss/specs/smoke-tests/quickstart-groups-dynamic.spec.ts b/e2e-pw/src/oss/specs/smoke-tests/quickstart-groups-dynamic.spec.ts index 66c6463f37..87ded69cbd 100644 --- a/e2e-pw/src/oss/specs/smoke-tests/quickstart-groups-dynamic.spec.ts +++ b/e2e-pw/src/oss/specs/smoke-tests/quickstart-groups-dynamic.spec.ts @@ -54,7 +54,7 @@ test.describe("quickstart-groups", () => { test.beforeEach(async ({ page, fiftyoneLoader }) => { await fiftyoneLoader.waitUntilGridVisible(page, datasetName, { - savedView: "dynamic", + searchParams: new URLSearchParams({ view: "dynamic" }), }); }); diff --git a/e2e-pw/src/shared/abstract-loader.ts b/e2e-pw/src/shared/abstract-loader.ts index f1c97fb036..41ed010e98 100644 --- a/e2e-pw/src/shared/abstract-loader.ts +++ b/e2e-pw/src/shared/abstract-loader.ts @@ -8,9 +8,9 @@ export type WaitUntilGridVisibleOptions = { isEmptyDataset?: boolean; /** - * Name of the saved view to be loaded. + * Search parameters to include */ - savedView?: string; + searchParams?: URLSearchParams; /** * Whether to wait for the grid to be visible.