From b4dc79401bef1656b6f3ee7d32256b5b3386e09b Mon Sep 17 00:00:00 2001 From: Dominik Prokop Date: Wed, 3 Apr 2024 12:06:38 +0200 Subject: [PATCH] DashboardScene: Fix explore to dashboard flow (#85140) * DashboardScene: Fix explore to dashboard flow * Tests * Make sure dashboard is in edit mode when adding from explore * Allow discarding changes when coming from explore * Tests --- .../pages/DashboardScenePage.test.tsx | 17 +++++++ .../pages/DashboardScenePage.tsx | 23 ++++++++- .../DashboardScenePageStateManager.test.ts | 48 +++++++++++++++++++ .../pages/DashboardScenePageStateManager.ts | 42 ++++++++++++++-- .../scene/DashboardScene.test.tsx | 47 ++++++++++++++++++ .../dashboard-scene/scene/DashboardScene.tsx | 27 ++++++++++- .../containers/DashboardPageProxy.tsx | 1 + .../features/dashboard/state/initDashboard.ts | 2 +- 8 files changed, 199 insertions(+), 8 deletions(-) diff --git a/public/app/features/dashboard-scene/pages/DashboardScenePage.test.tsx b/public/app/features/dashboard-scene/pages/DashboardScenePage.test.tsx index 26b17dcfd41..e6953c5e135 100644 --- a/public/app/features/dashboard-scene/pages/DashboardScenePage.test.tsx +++ b/public/app/features/dashboard-scene/pages/DashboardScenePage.test.tsx @@ -11,7 +11,9 @@ import { config, getPluginLinkExtensions, locationService, setPluginImportUtils import { VizPanel } from '@grafana/scenes'; import { Dashboard } from '@grafana/schema'; import { getRouteComponentProps } from 'app/core/navigation/__mocks__/routeProps'; +import store from 'app/core/store'; import { DashboardLoaderSrv, setDashboardLoaderSrv } from 'app/features/dashboard/services/DashboardLoaderSrv'; +import { DASHBOARD_FROM_LS_KEY } from 'app/features/dashboard/state/initDashboard'; import { dashboardSceneGraph } from '../utils/dashboardSceneGraph'; @@ -128,6 +130,7 @@ describe('DashboardScenePage', () => { Object.defineProperty(HTMLElement.prototype, 'offsetWidth', { configurable: true, value: 1000 }); getPluginLinkExtensionsMock.mockRestore(); getPluginLinkExtensionsMock.mockReturnValue({ extensions: [] }); + store.delete(DASHBOARD_FROM_LS_KEY); }); it('Can render dashboard', async () => { @@ -236,6 +239,20 @@ describe('DashboardScenePage', () => { expect(await screen.queryByText('Start your new dashboard by adding a visualization')).not.toBeInTheDocument(); }); }); + + it('is in edit mode when coming from explore to an existing dashboard', async () => { + store.setObject(DASHBOARD_FROM_LS_KEY, { dashboard: simpleDashboard }); + + setup(); + + await waitForDashbordToRender(); + + const panelAMenu = await screen.findByLabelText('Menu for panel with title Panel A'); + expect(panelAMenu).toBeInTheDocument(); + await userEvent.click(panelAMenu); + const editMenuItem = await screen.findAllByText('Edit'); + expect(editMenuItem).toHaveLength(1); + }); }); interface VizOptions { diff --git a/public/app/features/dashboard-scene/pages/DashboardScenePage.tsx b/public/app/features/dashboard-scene/pages/DashboardScenePage.tsx index e1f90a24ea3..3220e9aa0b0 100644 --- a/public/app/features/dashboard-scene/pages/DashboardScenePage.tsx +++ b/public/app/features/dashboard-scene/pages/DashboardScenePage.tsx @@ -1,12 +1,14 @@ // Libraries -import React, { useEffect } from 'react'; +import React, { useEffect, useMemo } from 'react'; import { PageLayoutType } from '@grafana/data'; import { Page } from 'app/core/components/Page/Page'; import PageLoader from 'app/core/components/PageLoader/PageLoader'; import { GrafanaRouteComponentProps } from 'app/core/navigation/types'; +import store from 'app/core/store'; import { DashboardPageRouteParams, DashboardPageRouteSearchParams } from 'app/features/dashboard/containers/types'; -import { DashboardRoutes } from 'app/types'; +import { DASHBOARD_FROM_LS_KEY } from 'app/features/dashboard/state/initDashboard'; +import { DashboardDTO, DashboardRoutes } from 'app/types'; import { DashboardPrompt } from '../saving/DashboardPrompt'; @@ -20,6 +22,12 @@ export function DashboardScenePage({ match, route, queryParams, history }: Props // After scene migration is complete and we get rid of old dashboard we should refactor dashboardWatcher so this route reload is not need const routeReloadCounter = (history.location.state as any)?.routeReloadCounter; + // Check if the user is coming from Explore, it's indicated byt the dashboard existence in local storage + const comingFromExplore = useMemo(() => { + return Boolean(store.getObject(DASHBOARD_FROM_LS_KEY)); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [match.params.uid, match.params.slug, match.params.type]); + useEffect(() => { if (route.routeName === DashboardRoutes.Normal && match.params.type === 'snapshot') { stateManager.loadSnapshot(match.params.slug!); @@ -28,6 +36,7 @@ export function DashboardScenePage({ match, route, queryParams, history }: Props uid: match.params.uid ?? '', route: route.routeName as DashboardRoutes, urlFolderUid: queryParams.folderUid, + keepDashboardFromExploreInLocalStorage: false, }); } @@ -44,6 +53,16 @@ export function DashboardScenePage({ match, route, queryParams, history }: Props match.params.type, ]); + // Effect that handles explore->dashboards workflow + useEffect(() => { + // When coming from explore and adding to an existing dashboard, we should enter edit mode + if (dashboard && comingFromExplore) { + if (route.routeName !== DashboardRoutes.New) { + dashboard.onEnterEditMode(comingFromExplore); + } + } + }, [dashboard, comingFromExplore, route.routeName]); + if (!dashboard) { return ( diff --git a/public/app/features/dashboard-scene/pages/DashboardScenePageStateManager.test.ts b/public/app/features/dashboard-scene/pages/DashboardScenePageStateManager.test.ts index 9c64f4251cc..6326a7f8981 100644 --- a/public/app/features/dashboard-scene/pages/DashboardScenePageStateManager.test.ts +++ b/public/app/features/dashboard-scene/pages/DashboardScenePageStateManager.test.ts @@ -2,6 +2,8 @@ import { advanceBy } from 'jest-date-mock'; import { locationService } from '@grafana/runtime'; import { getUrlSyncManager } from '@grafana/scenes'; +import store from 'app/core/store'; +import { DASHBOARD_FROM_LS_KEY } from 'app/features/dashboard/state/initDashboard'; import { DashboardRoutes } from 'app/types'; import { DashboardScene } from '../scene/DashboardScene'; @@ -10,6 +12,10 @@ import { setupLoadDashboardMock } from '../utils/test-utils'; import { DashboardScenePageStateManager, DASHBOARD_CACHE_TTL } from './DashboardScenePageStateManager'; describe('DashboardScenePageStateManager', () => { + afterEach(() => { + store.delete(DASHBOARD_FROM_LS_KEY); + }); + describe('when fetching/loading a dashboard', () => { it('should call loader from server if the dashboard is not cached', async () => { const loadDashboardMock = setupLoadDashboardMock({ dashboard: { uid: 'fake-dash', editable: true }, meta: {} }); @@ -35,6 +41,17 @@ describe('DashboardScenePageStateManager', () => { expect(loader.state.loadError).toBe('Error: Dashboard not found'); }); + it('shoud fetch dashboard from local storage and remove it after if it exists', async () => { + const loader = new DashboardScenePageStateManager({}); + const localStorageDashboard = { uid: 'fake-dash' }; + store.setObject(DASHBOARD_FROM_LS_KEY, localStorageDashboard); + + const result = await loader.fetchDashboard({ uid: 'fake-dash', route: DashboardRoutes.Normal }); + + expect(result).toEqual(localStorageDashboard); + expect(store.getObject(DASHBOARD_FROM_LS_KEY)).toBeUndefined(); + }); + it('should initialize the dashboard scene with the loaded dashboard', async () => { setupLoadDashboardMock({ dashboard: { uid: 'fake-dash' }, meta: {} }); @@ -187,5 +204,36 @@ describe('DashboardScenePageStateManager', () => { expect(loadDashSpy).toHaveBeenCalledTimes(2); }); }); + + describe('When coming from explore', () => { + it('shoud fetch dashboard from local storage and keep it there after when asked', async () => { + const loader = new DashboardScenePageStateManager({}); + const localStorageDashboard = { uid: 'fake-dash' }; + store.setObject(DASHBOARD_FROM_LS_KEY, { dashboard: localStorageDashboard }); + + const result = await loader.fetchDashboard({ + uid: 'fake-dash', + route: DashboardRoutes.Normal, + keepDashboardFromExploreInLocalStorage: true, + }); + + expect(result).toEqual({ dashboard: localStorageDashboard }); + expect(store.getObject(DASHBOARD_FROM_LS_KEY)).toEqual({ dashboard: localStorageDashboard }); + }); + + it('shoud not store dashboard in cache when coming from Explore', async () => { + const loader = new DashboardScenePageStateManager({}); + const localStorageDashboard = { uid: 'fake-dash' }; + store.setObject(DASHBOARD_FROM_LS_KEY, { dashboard: localStorageDashboard }); + + await loader.loadDashboard({ + uid: 'fake-dash', + route: DashboardRoutes.Normal, + keepDashboardFromExploreInLocalStorage: false, + }); + + expect(loader.getFromCache('fake-dash')).toBeNull(); + }); + }); }); }); diff --git a/public/app/features/dashboard-scene/pages/DashboardScenePageStateManager.ts b/public/app/features/dashboard-scene/pages/DashboardScenePageStateManager.ts index 6abb169ee8e..73e4a4f6d48 100644 --- a/public/app/features/dashboard-scene/pages/DashboardScenePageStateManager.ts +++ b/public/app/features/dashboard-scene/pages/DashboardScenePageStateManager.ts @@ -3,8 +3,13 @@ import { config, getBackendSrv, isFetchError, locationService } from '@grafana/r import { updateNavIndex } from 'app/core/actions'; import { StateManagerBase } from 'app/core/services/StateManagerBase'; import { backendSrv } from 'app/core/services/backend_srv'; +import { default as localStorageStore } from 'app/core/store'; import { dashboardLoaderSrv } from 'app/features/dashboard/services/DashboardLoaderSrv'; import { getDashboardSrv } from 'app/features/dashboard/services/DashboardSrv'; +import { + DASHBOARD_FROM_LS_KEY, + removeDashboardToFetchFromLocalStorage, +} from 'app/features/dashboard/state/initDashboard'; import { buildNavModel } from 'app/features/folders/state/navModel'; import { store } from 'app/store/store'; import { DashboardDTO, DashboardRoutes } from 'app/types'; @@ -36,6 +41,12 @@ export interface LoadDashboardOptions { uid: string; route: DashboardRoutes; urlFolderUid?: string; + // A temporary approach not to clean the dashboard from local storage when navigating from Explore to Dashboard + // We currently need it as there are two flows of fetching dashboard. The legacy one (initDashboard), uses the new one(DashboardScenePageStateManager.fetch) where the + // removal of the dashboard from local storage is implemented. So in the old flow we wouldn't be able to early return dashboard from local storage, if we prematurely + // removed it when prefetching the dashboard in DashboardPageProxy. + // This property will be removed when the old flow (initDashboard) is removed. + keepDashboardFromExploreInLocalStorage?: boolean; } export class DashboardScenePageStateManager extends StateManagerBase { @@ -46,7 +57,21 @@ export class DashboardScenePageStateManager extends StateManagerBase { + public async fetchDashboard({ + uid, + route, + urlFolderUid, + keepDashboardFromExploreInLocalStorage, + }: LoadDashboardOptions): Promise { + const model = localStorageStore.getObject(DASHBOARD_FROM_LS_KEY); + + if (model) { + if (!keepDashboardFromExploreInLocalStorage) { + removeDashboardToFetchFromLocalStorage(); + } + return model; + } + const cacheKey = route === DashboardRoutes.Home ? HOME_DASHBOARD_CACHE_KEY : uid; const cachedDashboard = this.getFromCache(cacheKey); @@ -156,12 +181,20 @@ export class DashboardScenePageStateManager extends StateManagerBase { + const comingFromExplore = Boolean( + localStorageStore.getObject(DASHBOARD_FROM_LS_KEY) && + options.keepDashboardFromExploreInLocalStorage === false + ); + const rsp = await this.fetchDashboard(options); const fromCache = this.cache[options.uid]; - if (fromCache && fromCache.state.version === rsp?.dashboard.version) { - return fromCache; + // When coming from Explore, skip returnning scene from cache + if (!comingFromExplore) { + if (fromCache && fromCache.state.version === rsp?.dashboard.version) { + return fromCache; + } } this.setState({ isLoading: true }); @@ -169,7 +202,8 @@ export class DashboardScenePageStateManager extends StateManagerBase { } }); }); + + describe('When coming from explore', () => { + // When coming from Explore the first panel in a dashboard is a temporary panel + it('should remove first panel from the grid when discarding changes', () => { + const scene = new DashboardScene({ + title: 'hello', + uid: 'dash-1', + description: 'hello description', + editable: true, + $timeRange: new SceneTimeRange({ + timeZone: 'browser', + }), + controls: new DashboardControls({}), + $behaviors: [new behaviors.CursorSync({})], + body: new SceneGridLayout({ + children: [ + new DashboardGridItem({ + key: 'griditem-1', + x: 0, + body: new VizPanel({ + title: 'Panel A', + key: 'panel-1', + pluginId: 'table', + $data: new SceneQueryRunner({ key: 'data-query-runner', queries: [{ refId: 'A' }] }), + }), + }), + new DashboardGridItem({ + key: 'griditem-2', + body: new VizPanel({ + title: 'Panel B', + key: 'panel-2', + pluginId: 'table', + }), + }), + ], + }), + }); + + scene.onEnterEditMode(true); + expect(scene.state.isEditing).toBe(true); + expect((scene.state.body as SceneGridLayout).state.children.length).toBe(2); + + scene.exitEditMode({ skipConfirm: true }); + expect(scene.state.isEditing).toBe(false); + expect((scene.state.body as SceneGridLayout).state.children.length).toBe(1); + }); + }); }); function buildTestScene(overrides?: Partial) { diff --git a/public/app/features/dashboard-scene/scene/DashboardScene.tsx b/public/app/features/dashboard-scene/scene/DashboardScene.tsx index cbac30ea17c..3609a79b98d 100644 --- a/public/app/features/dashboard-scene/scene/DashboardScene.tsx +++ b/public/app/features/dashboard-scene/scene/DashboardScene.tsx @@ -145,6 +145,11 @@ export class DashboardScene extends SceneObjectBase { */ private _changeTracker: DashboardSceneChangeTracker; + /** + * Flag to indicate if the user came from Explore + */ + private _fromExplore = false; + public constructor(state: Partial) { super({ title: 'Dashboard', @@ -211,9 +216,11 @@ export class DashboardScene extends SceneObjectBase { getUrlSyncManager().cleanUp(this); } - public onEnterEditMode = () => { + public onEnterEditMode = (fromExplore = false) => { + this._fromExplore = fromExplore; // Save this state this._initialState = sceneUtils.cloneSceneObjectState(this.state); + this._initialUrlState = locationService.getLocation(); // Switch to edit mode @@ -299,6 +306,10 @@ export class DashboardScene extends SceneObjectBase { }) ); + if (this._fromExplore) { + this.cleanupStateFromExplore(); + } + if (restoreInitialState) { // Restore initial state and disable editing this.setState({ ...this._initialState, isEditing: false }); @@ -312,6 +323,20 @@ export class DashboardScene extends SceneObjectBase { this.propagateEditModeChange(); } + private cleanupStateFromExplore() { + this._fromExplore = false; + // When coming from explore but discarding changes, remove the panel that explore is potentially adding. + if (this._initialSaveModel?.panels) { + this._initialSaveModel.panels = this._initialSaveModel.panels.slice(1); + } + + if (this._initialState && this._initialState.body instanceof SceneGridLayout) { + this._initialState.body.setState({ + children: this._initialState.body.state.children.slice(1), + }); + } + } + public canDiscard() { return this._initialState !== undefined; } diff --git a/public/app/features/dashboard/containers/DashboardPageProxy.tsx b/public/app/features/dashboard/containers/DashboardPageProxy.tsx index aa3c7944fe7..b7b70b12f16 100644 --- a/public/app/features/dashboard/containers/DashboardPageProxy.tsx +++ b/public/app/features/dashboard/containers/DashboardPageProxy.tsx @@ -42,6 +42,7 @@ function DashboardPageProxy(props: DashboardPageProxyProps) { return stateManager.fetchDashboard({ route: props.route.routeName as DashboardRoutes, uid: props.match.params.uid ?? '', + keepDashboardFromExploreInLocalStorage: true, }); }, [props.match.params.uid, props.route.routeName]); diff --git a/public/app/features/dashboard/state/initDashboard.ts b/public/app/features/dashboard/state/initDashboard.ts index 712e7ec5967..a3fa2238856 100644 --- a/public/app/features/dashboard/state/initDashboard.ts +++ b/public/app/features/dashboard/state/initDashboard.ts @@ -295,7 +295,7 @@ export function initDashboard(args: InitDashboardArgs): ThunkResult { }; } -const DASHBOARD_FROM_LS_KEY = 'DASHBOARD_FROM_LS_KEY'; +export const DASHBOARD_FROM_LS_KEY = 'DASHBOARD_FROM_LS_KEY'; export function setDashboardToFetchFromLocalStorage(model: DashboardDTO) { store.setObject(DASHBOARD_FROM_LS_KEY, model);