From 8a83041e54cf00b932d8a94a4f6edf1c79e55275 Mon Sep 17 00:00:00 2001 From: Marta Bondyra Date: Fri, 3 Sep 2021 17:31:11 +0200 Subject: [PATCH 1/7] use resolve instead of get --- .../public/helpers/saved_workspace_utils.ts | 40 ++++++++++++------- .../public/helpers/use_workspace_loader.ts | 10 ++--- 2 files changed, 31 insertions(+), 19 deletions(-) diff --git a/x-pack/plugins/graph/public/helpers/saved_workspace_utils.ts b/x-pack/plugins/graph/public/helpers/saved_workspace_utils.ts index 336708173d321..2308067ec2d47 100644 --- a/x-pack/plugins/graph/public/helpers/saved_workspace_utils.ts +++ b/x-pack/plugins/graph/public/helpers/saved_workspace_utils.ts @@ -82,28 +82,38 @@ export function findSavedWorkspace( }); } +export function getEmptyWorkspace() { + return { + savedObject: { + displayName: 'graph workspace', + getEsType: () => savedWorkspaceType, + ...defaultsProps, + } as GraphWorkspaceSavedObject, + }; +} + export async function getSavedWorkspace( savedObjectsClient: SavedObjectsClientContract, - id?: string + id: string ) { - const savedObject = { - id, - displayName: 'graph workspace', - getEsType: () => savedWorkspaceType, - } as { [key: string]: any }; + const resolveResult = await savedObjectsClient.resolve>( + savedWorkspaceType, + id + ); - if (!id) { - assign(savedObject, defaultsProps); - return Promise.resolve(savedObject); - } - - const resp = await savedObjectsClient.get>(savedWorkspaceType, id); - savedObject._source = cloneDeep(resp.attributes); + const resp = resolveResult.saved_object; if (!resp._version) { throw new SavedObjectNotFound(savedWorkspaceType, id || ''); } + const savedObject = { + id, + displayName: 'graph workspace', + getEsType: () => savedWorkspaceType, + _source: cloneDeep(resp.attributes), + } as GraphWorkspaceSavedObject; + // assign the defaults to the response defaults(savedObject._source, defaultsProps); @@ -120,7 +130,9 @@ export async function getSavedWorkspace( injectReferences(savedObject, resp.references); } - return savedObject as GraphWorkspaceSavedObject; + return { + savedObject, + }; } export function deleteSavedWorkspace( diff --git a/x-pack/plugins/graph/public/helpers/use_workspace_loader.ts b/x-pack/plugins/graph/public/helpers/use_workspace_loader.ts index 8b91546d52446..a14e5d41c98e6 100644 --- a/x-pack/plugins/graph/public/helpers/use_workspace_loader.ts +++ b/x-pack/plugins/graph/public/helpers/use_workspace_loader.ts @@ -11,7 +11,7 @@ import { useHistory, useLocation, useParams } from 'react-router-dom'; import { i18n } from '@kbn/i18n'; import { GraphStore } from '../state_management'; import { GraphWorkspaceSavedObject, IndexPatternSavedObject, Workspace } from '../types'; -import { getSavedWorkspace } from './saved_workspace_utils'; +import { getEmptyWorkspace, getSavedWorkspace } from './saved_workspace_utils'; interface UseWorkspaceLoaderProps { store: GraphStore; @@ -71,8 +71,8 @@ export const useWorkspaceLoader = ({ .then((response) => response.savedObjects); } - async function fetchSavedWorkspace() { - return (id + async function fetchSavedWorkspace(): Promise<{ savedObject: GraphWorkspaceSavedObject }> { + return id ? await getSavedWorkspace(savedObjectsClient, id).catch(function (e) { toastNotifications.addError(e, { title: i18n.translate('xpack.graph.missingWorkspaceErrorMessage', { @@ -83,12 +83,12 @@ export const useWorkspaceLoader = ({ // return promise that never returns to prevent the controller from loading return new Promise(() => {}); }) - : await getSavedWorkspace(savedObjectsClient)) as GraphWorkspaceSavedObject; + : getEmptyWorkspace(); } async function initializeWorkspace() { const fetchedIndexPatterns = await fetchIndexPatterns(); - const fetchedSavedWorkspace = await fetchSavedWorkspace(); + const { savedObject: fetchedSavedWorkspace } = await fetchSavedWorkspace(); /** * Deal with situation of request to open saved workspace. Otherwise clean up store, From fcdaff487cbea76a5e3f7fbbaeef5e6913bc0018 Mon Sep 17 00:00:00 2001 From: Marta Bondyra Date: Fri, 3 Sep 2021 23:04:11 +0200 Subject: [PATCH 2/7] graph step 3 (tests missing) --- x-pack/plugins/graph/kibana.json | 27 +++++++++--- x-pack/plugins/graph/public/application.ts | 2 + .../graph/public/apps/workspace_route.tsx | 5 ++- .../workspace_layout/workspace_layout.tsx | 41 ++++++++++++++++++- .../public/helpers/saved_workspace_utils.ts | 6 +++ .../public/helpers/use_workspace_loader.ts | 18 ++++++-- x-pack/plugins/graph/public/plugin.ts | 3 ++ x-pack/plugins/graph/public/services/url.ts | 4 +- x-pack/plugins/graph/tsconfig.json | 3 +- 9 files changed, 96 insertions(+), 13 deletions(-) diff --git a/x-pack/plugins/graph/kibana.json b/x-pack/plugins/graph/kibana.json index cc732e67995ba..a8b0beecb361f 100644 --- a/x-pack/plugins/graph/kibana.json +++ b/x-pack/plugins/graph/kibana.json @@ -4,12 +4,29 @@ "kibanaVersion": "kibana", "server": true, "ui": true, - "requiredPlugins": ["licensing", "data", "navigation", "savedObjects", "kibanaLegacy"], - "optionalPlugins": ["home", "features"], - "configPath": ["xpack", "graph"], - "requiredBundles": ["kibanaUtils", "kibanaReact", "home"], + "requiredPlugins": [ + "licensing", + "data", + "navigation", + "savedObjects", + "kibanaLegacy", + "spaces" + ], + "optionalPlugins": [ + "home", + "features" + ], + "configPath": [ + "xpack", + "graph" + ], + "requiredBundles": [ + "kibanaUtils", + "kibanaReact", + "home" + ], "owner": { "name": "Data Discovery", "githubTeam": "kibana-data-discovery" } -} +} \ No newline at end of file diff --git a/x-pack/plugins/graph/public/application.ts b/x-pack/plugins/graph/public/application.ts index 7461a7b5fc172..90705e3aeb09d 100644 --- a/x-pack/plugins/graph/public/application.ts +++ b/x-pack/plugins/graph/public/application.ts @@ -31,6 +31,7 @@ import './index.scss'; import { SavedObjectsStart } from '../../../../src/plugins/saved_objects/public'; import { GraphSavePolicy } from './types'; import { graphRouter } from './router'; +import { SpacesApi } from '../../spaces/public'; /** * These are dependencies of the Graph app besides the base dependencies @@ -63,6 +64,7 @@ export interface GraphDependencies { setHeaderActionMenu: AppMountParameters['setHeaderActionMenu']; uiSettings: IUiSettingsClient; history: ScopedHistory; + spaces: SpacesApi; } export type GraphServices = Omit; diff --git a/x-pack/plugins/graph/public/apps/workspace_route.tsx b/x-pack/plugins/graph/public/apps/workspace_route.tsx index f4158238c33c6..9087c2ce601cb 100644 --- a/x-pack/plugins/graph/public/apps/workspace_route.tsx +++ b/x-pack/plugins/graph/public/apps/workspace_route.tsx @@ -40,6 +40,7 @@ export const WorkspaceRoute = ({ getBasePath, addBasePath, setHeaderActionMenu, + spaces, indexPatterns: getIndexPatternProvider, }, }: WorkspaceRouteProps) => { @@ -114,7 +115,7 @@ export const WorkspaceRoute = ({ }) ); - const { savedWorkspace, indexPatterns } = useWorkspaceLoader({ + const { savedWorkspace, indexPatterns, sharingSavedObjectProps } = useWorkspaceLoader({ workspaceRef, store, savedObjectsClient, @@ -130,6 +131,8 @@ export const WorkspaceRoute = ({ { const [currentIndexPattern, setCurrentIndexPattern] = useState(); const [showInspect, setShowInspect] = useState(false); @@ -154,6 +161,38 @@ const WorkspaceLayoutComponent = ({ [] ); + const getLegacyUrlConflictCallout = useCallback(() => { + // This function returns a callout component *if* we have encountered a "legacy URL conflict" scenario + const currentObjectId = savedWorkspace.id; + if (spaces && sharingSavedObjectProps?.outcome === 'conflict' && currentObjectId) { + // We have resolved to one object, but another object has a legacy URL alias associated with this ID/page. We should display a + // callout with a warning for the user, and provide a way for them to navigate to the other object. + const otherObjectId = sharingSavedObjectProps?.aliasTargetId!; // This is always defined if outcome === 'conflict' + const otherObjectPath = getEditUrl(coreStart.http.basePath.prepend, { id: otherObjectId }); + return spaces.ui.components.getLegacyUrlConflict({ + objectNoun: i18n.translate('xpack.graph.legacyUrlConflict.objectNoun', { + defaultMessage: 'Graph', + }), + currentObjectId, + otherObjectId, + otherObjectPath, + }); + } + return null; + }, [savedWorkspace.id, sharingSavedObjectProps, spaces, coreStart.http]); + + if (spaces && sharingSavedObjectProps?.outcome === 'aliasMatch') { + // We found this object by a legacy URL alias from its old ID; redirect the user to the page with its new ID, preserving any URL hash + const newObjectId = sharingSavedObjectProps?.aliasTargetId!; // This is always defined if outcome === 'aliasMatch' + const newPath = getEditUrl(coreStart.http.basePath.prepend, { id: newObjectId }); + spaces.ui.redirectLegacyUrl( + newPath, + i18n.translate('xpack.graph.legacyUrlConflict.objectNoun', { + defaultMessage: 'Graph', + }) + ); + } + return ( - {isInitialized && }
+ {getLegacyUrlConflictCallout()} {!isInitialized && (
{ const [indexPatterns, setIndexPatterns] = useState(); const [savedWorkspace, setSavedWorkspace] = useState(); + const [sharingSavedObjectProps, setSharingSavedObjectProps] = useState(); const history = useHistory(); const location = useLocation(); const { id } = useParams(); @@ -71,7 +76,10 @@ export const useWorkspaceLoader = ({ .then((response) => response.savedObjects); } - async function fetchSavedWorkspace(): Promise<{ savedObject: GraphWorkspaceSavedObject }> { + async function fetchSavedWorkspace(): Promise<{ + savedObject: GraphWorkspaceSavedObject; + sharingSavedObjectProps?: SharingSavedObjectProps; + }> { return id ? await getSavedWorkspace(savedObjectsClient, id).catch(function (e) { toastNotifications.addError(e, { @@ -88,7 +96,10 @@ export const useWorkspaceLoader = ({ async function initializeWorkspace() { const fetchedIndexPatterns = await fetchIndexPatterns(); - const { savedObject: fetchedSavedWorkspace } = await fetchSavedWorkspace(); + const { + savedObject: fetchedSavedWorkspace, + sharingSavedObjectProps: fetchedSharingSavedObjectProps, + } = await fetchSavedWorkspace(); /** * Deal with situation of request to open saved workspace. Otherwise clean up store, @@ -102,6 +113,7 @@ export const useWorkspaceLoader = ({ setIndexPatterns(fetchedIndexPatterns); setSavedWorkspace(fetchedSavedWorkspace); + setSharingSavedObjectProps(fetchedSharingSavedObjectProps); } initializeWorkspace(); @@ -116,5 +128,5 @@ export const useWorkspaceLoader = ({ workspaceRef, ]); - return { savedWorkspace, indexPatterns }; + return { savedWorkspace, indexPatterns, sharingSavedObjectProps }; }; diff --git a/x-pack/plugins/graph/public/plugin.ts b/x-pack/plugins/graph/public/plugin.ts index 1ff9afe505a3b..230359e6e4780 100644 --- a/x-pack/plugins/graph/public/plugin.ts +++ b/x-pack/plugins/graph/public/plugin.ts @@ -7,6 +7,7 @@ import { i18n } from '@kbn/i18n'; import { BehaviorSubject } from 'rxjs'; +import { SpacesApi } from '../../spaces/public'; import { AppNavLinkStatus, AppUpdater, @@ -44,6 +45,7 @@ export interface GraphPluginStartDependencies { savedObjects: SavedObjectsStart; kibanaLegacy: KibanaLegacyStart; home?: HomePublicPluginStart; + spaces: SpacesApi; } export class GraphPlugin @@ -110,6 +112,7 @@ export class GraphPlugin overlays: coreStart.overlays, savedObjects: pluginsStart.savedObjects, uiSettings: core.uiSettings, + spaces: pluginsStart.spaces, }); }, }); diff --git a/x-pack/plugins/graph/public/services/url.ts b/x-pack/plugins/graph/public/services/url.ts index e45d1f0d662be..b33fdc82d8642 100644 --- a/x-pack/plugins/graph/public/services/url.ts +++ b/x-pack/plugins/graph/public/services/url.ts @@ -18,13 +18,13 @@ export function getNewPath() { return '/workspace'; } -export function getEditPath({ id }: GraphWorkspaceSavedObject) { +export function getEditPath({ id }: Pick) { return `/workspace/${id}`; } export function getEditUrl( addBasePath: (url: string) => string, - workspace: GraphWorkspaceSavedObject + workspace: Pick ) { return addBasePath(`#${getEditPath(workspace)}`); } diff --git a/x-pack/plugins/graph/tsconfig.json b/x-pack/plugins/graph/tsconfig.json index d655f28c4e46e..6a5623b311d5e 100644 --- a/x-pack/plugins/graph/tsconfig.json +++ b/x-pack/plugins/graph/tsconfig.json @@ -24,6 +24,7 @@ { "path": "../../../src/plugins/kibana_legacy/tsconfig.json"}, { "path": "../../../src/plugins/home/tsconfig.json"}, { "path": "../../../src/plugins/kibana_utils/tsconfig.json" }, - { "path": "../../../src/plugins/kibana_react/tsconfig.json" } + { "path": "../../../src/plugins/kibana_react/tsconfig.json" }, + { "path": "../spaces/tsconfig.json" } ] } From 2203f6bee8bcd57fe0fb4283b16ce8a2b5a97b34 Mon Sep 17 00:00:00 2001 From: Marta Bondyra Date: Sat, 4 Sep 2021 14:56:04 +0200 Subject: [PATCH 3/7] tests --- .../workspace_layout.test.tsx | 68 +++++++++++++++++++ .../workspace_layout/workspace_layout.tsx | 6 +- 2 files changed, 71 insertions(+), 3 deletions(-) create mode 100644 x-pack/plugins/graph/public/components/workspace_layout/workspace_layout.test.tsx diff --git a/x-pack/plugins/graph/public/components/workspace_layout/workspace_layout.test.tsx b/x-pack/plugins/graph/public/components/workspace_layout/workspace_layout.test.tsx new file mode 100644 index 0000000000000..20dea4322b601 --- /dev/null +++ b/x-pack/plugins/graph/public/components/workspace_layout/workspace_layout.test.tsx @@ -0,0 +1,68 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +import React from 'react'; +import { shallow } from 'enzyme'; +import { WorkspaceLayoutComponent } from '.'; +import { coreMock } from 'src/core/public/mocks'; +import { spacesPluginMock } from '../../../../spaces/public/mocks'; +import { NavigationPublicPluginStart as NavigationStart } from '../../../../../../src/plugins/navigation/public'; +import { GraphSavePolicy, GraphWorkspaceSavedObject, IndexPatternProvider } from '../../types'; +import { OverlayStart, Capabilities } from 'kibana/public'; +import { SharingSavedObjectProps } from '../../helpers/use_workspace_loader'; + +describe('workspace_layout', () => { + const defaultProps = { + renderCounter: 1, + loading: false, + savedWorkspace: { id: 'test' } as GraphWorkspaceSavedObject, + hasFields: true, + overlays: {} as OverlayStart, + workspaceInitialized: true, + indexPatterns: [], + indexPatternProvider: {} as IndexPatternProvider, + capabilities: {} as Capabilities, + coreStart: coreMock.createStart(), + graphSavePolicy: 'configAndDataWithConsent' as GraphSavePolicy, + navigation: {} as NavigationStart, + canEditDrillDownUrls: true, + urlQuery: 'search', + setHeaderActionMenu: jest.fn(), + sharingSavedObjectProps: { + outcome: 'exactMatch', + aliasTargetId: '', + } as SharingSavedObjectProps, + spaces: spacesPluginMock.createStartContract(), + }; + it('should display conflict notification if outcome is conflict', () => { + shallow( + + ); + expect(defaultProps.spaces.ui.components.getLegacyUrlConflict).toHaveBeenCalledWith({ + currentObjectId: 'test', + objectNoun: 'Graph', + otherObjectId: 'conflictId', + otherObjectPath: '#/workspace/conflictId', + }); + }); + + it('should redirect if outcome is aliasMatch', () => { + shallow( + + ); + expect(defaultProps.spaces.ui.redirectLegacyUrl).toHaveBeenCalledWith( + '#/workspace/aliasMatchId', + 'Graph' + ); + }); +}); diff --git a/x-pack/plugins/graph/public/components/workspace_layout/workspace_layout.tsx b/x-pack/plugins/graph/public/components/workspace_layout/workspace_layout.tsx index 44bb8ebbf9a39..9299b6c8ba1f9 100644 --- a/x-pack/plugins/graph/public/components/workspace_layout/workspace_layout.tsx +++ b/x-pack/plugins/graph/public/components/workspace_layout/workspace_layout.tsx @@ -9,7 +9,6 @@ import React, { Fragment, memo, useCallback, useRef, useState } from 'react'; import { i18n } from '@kbn/i18n'; import { EuiSpacer } from '@elastic/eui'; import { connect } from 'react-redux'; -import { SpacesApi } from '../../../../spaces/public'; import { SearchBar } from '../search_bar'; import { GraphState, @@ -54,6 +53,7 @@ type WorkspaceLayoutProps = Pick< | 'coreStart' | 'canEditDrillDownUrls' | 'overlays' + | 'spaces' > & { renderCounter: number; workspace?: Workspace; @@ -63,7 +63,6 @@ type WorkspaceLayoutProps = Pick< indexPatternProvider: IndexPatternProvider; urlQuery: string | null; sharingSavedObjectProps?: SharingSavedObjectProps; - spaces: SpacesApi; }; interface WorkspaceLayoutStateProps { @@ -71,7 +70,7 @@ interface WorkspaceLayoutStateProps { hasFields: boolean; } -const WorkspaceLayoutComponent = ({ +export const WorkspaceLayoutComponent = ({ renderCounter, workspace, loading, @@ -191,6 +190,7 @@ const WorkspaceLayoutComponent = ({ defaultMessage: 'Graph', }) ); + return null; } return ( From 8508c8abe5ff6df9ff2974acc0d7e050a4e17ee0 Mon Sep 17 00:00:00 2001 From: Marta Bondyra Date: Sat, 4 Sep 2021 18:27:45 +0200 Subject: [PATCH 4/7] moving the functionality to workspace_route --- .../graph/public/apps/workspace_route.tsx | 11 ++- .../workspace_layout.test.tsx | 13 ---- .../workspace_layout/workspace_layout.tsx | 13 ---- .../helpers/use_workspace_loader.test.tsx | 75 +++++++++++++++++++ .../public/helpers/use_workspace_loader.ts | 32 ++++++-- 5 files changed, 109 insertions(+), 35 deletions(-) create mode 100644 x-pack/plugins/graph/public/helpers/use_workspace_loader.test.tsx diff --git a/x-pack/plugins/graph/public/apps/workspace_route.tsx b/x-pack/plugins/graph/public/apps/workspace_route.tsx index 9087c2ce601cb..79f687d789fb7 100644 --- a/x-pack/plugins/graph/public/apps/workspace_route.tsx +++ b/x-pack/plugins/graph/public/apps/workspace_route.tsx @@ -115,13 +115,20 @@ export const WorkspaceRoute = ({ }) ); - const { savedWorkspace, indexPatterns, sharingSavedObjectProps } = useWorkspaceLoader({ + const loaded = useWorkspaceLoader({ workspaceRef, store, savedObjectsClient, - toastNotifications, + spaces, + coreStart, }); + if (!loaded) { + return null; + } + + const { savedWorkspace, indexPatterns, sharingSavedObjectProps } = loaded; + if (!savedWorkspace || !indexPatterns) { return null; } diff --git a/x-pack/plugins/graph/public/components/workspace_layout/workspace_layout.test.tsx b/x-pack/plugins/graph/public/components/workspace_layout/workspace_layout.test.tsx index 20dea4322b601..ae1a726464be5 100644 --- a/x-pack/plugins/graph/public/components/workspace_layout/workspace_layout.test.tsx +++ b/x-pack/plugins/graph/public/components/workspace_layout/workspace_layout.test.tsx @@ -52,17 +52,4 @@ describe('workspace_layout', () => { otherObjectPath: '#/workspace/conflictId', }); }); - - it('should redirect if outcome is aliasMatch', () => { - shallow( - - ); - expect(defaultProps.spaces.ui.redirectLegacyUrl).toHaveBeenCalledWith( - '#/workspace/aliasMatchId', - 'Graph' - ); - }); }); diff --git a/x-pack/plugins/graph/public/components/workspace_layout/workspace_layout.tsx b/x-pack/plugins/graph/public/components/workspace_layout/workspace_layout.tsx index 9299b6c8ba1f9..4bb04264bb520 100644 --- a/x-pack/plugins/graph/public/components/workspace_layout/workspace_layout.tsx +++ b/x-pack/plugins/graph/public/components/workspace_layout/workspace_layout.tsx @@ -180,19 +180,6 @@ export const WorkspaceLayoutComponent = ({ return null; }, [savedWorkspace.id, sharingSavedObjectProps, spaces, coreStart.http]); - if (spaces && sharingSavedObjectProps?.outcome === 'aliasMatch') { - // We found this object by a legacy URL alias from its old ID; redirect the user to the page with its new ID, preserving any URL hash - const newObjectId = sharingSavedObjectProps?.aliasTargetId!; // This is always defined if outcome === 'aliasMatch' - const newPath = getEditUrl(coreStart.http.basePath.prepend, { id: newObjectId }); - spaces.ui.redirectLegacyUrl( - newPath, - i18n.translate('xpack.graph.legacyUrlConflict.objectNoun', { - defaultMessage: 'Graph', - }) - ); - return null; - } - return ( ({ + useHistory: () => ({ + replace: jest.fn(), + }), + useLocation: () => ({ + location: jest.fn(), + }), + useParams: () => ({ + id: jest.fn(), + }), +})); + +jest.mock('React', () => ({ + ...jest.requireActual('React'), + useEffect: jest.fn(), +})); + +const mockSavedObjectsClient = ({ + resolve: jest.fn().mockResolvedValue({ + saved_object: { id: 10, _version: '7.15.0', attributes: { wsState: '{}' } }, + outcome: 'aliasMatch', + alias_target_id: 'aliasTargetId', + }), + find: jest.fn().mockResolvedValue({ title: 'test' }), +} as unknown) as SavedObjectsClientCommon; + +async function setup(props: UseWorkspaceLoaderProps) { + const returnVal = {}; + function TestComponent() { + Object.assign(returnVal, useWorkspaceLoader(props)); + return null; + } + await act(async () => { + const promise = Promise.resolve(); + mount(); + await act(() => promise); + }); + return returnVal; +} + +describe('use_workspace_loader', () => { + const defaultProps = { + workspaceRef: { current: {} as Workspace }, + store: createMockGraphStore({}).store, + savedObjectsClient: mockSavedObjectsClient, + coreStart: coreMock.createStart(), + spaces: spacesPluginMock.createStartContract(), + }; + + it('should redirect if outcome is aliasMatch', async () => { + await act(async () => { + await setup((defaultProps as unknown) as UseWorkspaceLoaderProps); + }); + expect(defaultProps.spaces.ui.redirectLegacyUrl).toHaveBeenCalledWith( + '#/workspace/aliasTargetId', + 'Graph' + ); + }); +}); diff --git a/x-pack/plugins/graph/public/helpers/use_workspace_loader.ts b/x-pack/plugins/graph/public/helpers/use_workspace_loader.ts index 05a53ff23bfd9..74ba2f3bfbbd3 100644 --- a/x-pack/plugins/graph/public/helpers/use_workspace_loader.ts +++ b/x-pack/plugins/graph/public/helpers/use_workspace_loader.ts @@ -5,19 +5,22 @@ * 2.0. */ -import { SavedObjectsClientContract, ToastsStart } from 'kibana/public'; +import { SavedObjectsClientContract } from 'kibana/public'; import { useEffect, useState } from 'react'; import { useHistory, useLocation, useParams } from 'react-router-dom'; import { i18n } from '@kbn/i18n'; +import { CoreStart } from 'kibana/public'; import { GraphStore } from '../state_management'; import { GraphWorkspaceSavedObject, IndexPatternSavedObject, Workspace } from '../types'; import { getEmptyWorkspace, getSavedWorkspace } from './saved_workspace_utils'; - -interface UseWorkspaceLoaderProps { +import { getEditUrl } from '../services/url'; +import { SpacesApi } from '../../../spaces/public'; +export interface UseWorkspaceLoaderProps { store: GraphStore; workspaceRef: React.MutableRefObject; savedObjectsClient: SavedObjectsClientContract; - toastNotifications: ToastsStart; + spaces: SpacesApi; + coreStart: CoreStart; } interface WorkspaceUrlParams { @@ -29,10 +32,11 @@ export interface SharingSavedObjectProps { } export const useWorkspaceLoader = ({ + coreStart, + spaces, workspaceRef, store, savedObjectsClient, - toastNotifications, }: UseWorkspaceLoaderProps) => { const [indexPatterns, setIndexPatterns] = useState(); const [savedWorkspace, setSavedWorkspace] = useState(); @@ -82,7 +86,7 @@ export const useWorkspaceLoader = ({ }> { return id ? await getSavedWorkspace(savedObjectsClient, id).catch(function (e) { - toastNotifications.addError(e, { + coreStart.notifications.toasts.addError(e, { title: i18n.translate('xpack.graph.missingWorkspaceErrorMessage', { defaultMessage: "Couldn't load graph with ID", }), @@ -101,6 +105,19 @@ export const useWorkspaceLoader = ({ sharingSavedObjectProps: fetchedSharingSavedObjectProps, } = await fetchSavedWorkspace(); + if (spaces && fetchedSharingSavedObjectProps?.outcome === 'aliasMatch') { + // We found this object by a legacy URL alias from its old ID; redirect the user to the page with its new ID, preserving any URL hash + const newObjectId = fetchedSharingSavedObjectProps?.aliasTargetId!; // This is always defined if outcome === 'aliasMatch' + const newPath = getEditUrl(coreStart.http.basePath.prepend, { id: newObjectId }); + spaces.ui.redirectLegacyUrl( + newPath, + i18n.translate('xpack.graph.legacyUrlConflict.objectNoun', { + defaultMessage: 'Graph', + }) + ); + return null; + } + /** * Deal with situation of request to open saved workspace. Otherwise clean up store, * when navigating to a new workspace from existing one. @@ -124,8 +141,9 @@ export const useWorkspaceLoader = ({ history, savedObjectsClient, setSavedWorkspace, - toastNotifications, + coreStart, workspaceRef, + spaces, ]); return { savedWorkspace, indexPatterns, sharingSavedObjectProps }; From 0d63e9c1de07b7fb195c224c848d2e24cd098f2f Mon Sep 17 00:00:00 2001 From: Marta Bondyra Date: Tue, 7 Sep 2021 16:25:41 +0200 Subject: [PATCH 5/7] removing unnecessary mock --- .../graph/public/helpers/use_workspace_loader.test.tsx | 5 ----- 1 file changed, 5 deletions(-) diff --git a/x-pack/plugins/graph/public/helpers/use_workspace_loader.test.tsx b/x-pack/plugins/graph/public/helpers/use_workspace_loader.test.tsx index 84197e67724d4..65b3bb6c0b0d2 100644 --- a/x-pack/plugins/graph/public/helpers/use_workspace_loader.test.tsx +++ b/x-pack/plugins/graph/public/helpers/use_workspace_loader.test.tsx @@ -26,11 +26,6 @@ jest.mock('react-router-dom', () => ({ }), })); -jest.mock('React', () => ({ - ...jest.requireActual('React'), - useEffect: jest.fn(), -})); - const mockSavedObjectsClient = ({ resolve: jest.fn().mockResolvedValue({ saved_object: { id: 10, _version: '7.15.0', attributes: { wsState: '{}' } }, From 60fba05a088dab8f73421553c9cea1375e2502b6 Mon Sep 17 00:00:00 2001 From: Marta Bondyra Date: Wed, 8 Sep 2021 11:17:12 +0200 Subject: [PATCH 6/7] make spaces optional & add negative test --- x-pack/plugins/graph/kibana.json | 6 +- x-pack/plugins/graph/public/application.ts | 2 +- .../helpers/use_workspace_loader.test.tsx | 55 ++++++++++++++----- .../public/helpers/use_workspace_loader.ts | 37 +++++++------ x-pack/plugins/graph/public/plugin.ts | 2 +- 5 files changed, 66 insertions(+), 36 deletions(-) diff --git a/x-pack/plugins/graph/kibana.json b/x-pack/plugins/graph/kibana.json index a8b0beecb361f..410a5e2c160d6 100644 --- a/x-pack/plugins/graph/kibana.json +++ b/x-pack/plugins/graph/kibana.json @@ -9,12 +9,12 @@ "data", "navigation", "savedObjects", - "kibanaLegacy", - "spaces" + "kibanaLegacy" ], "optionalPlugins": [ "home", - "features" + "features", + "spaces" ], "configPath": [ "xpack", diff --git a/x-pack/plugins/graph/public/application.ts b/x-pack/plugins/graph/public/application.ts index 90705e3aeb09d..fc6c6170509d9 100644 --- a/x-pack/plugins/graph/public/application.ts +++ b/x-pack/plugins/graph/public/application.ts @@ -64,7 +64,7 @@ export interface GraphDependencies { setHeaderActionMenu: AppMountParameters['setHeaderActionMenu']; uiSettings: IUiSettingsClient; history: ScopedHistory; - spaces: SpacesApi; + spaces?: SpacesApi; } export type GraphServices = Omit; diff --git a/x-pack/plugins/graph/public/helpers/use_workspace_loader.test.tsx b/x-pack/plugins/graph/public/helpers/use_workspace_loader.test.tsx index 65b3bb6c0b0d2..b81be35d3c021 100644 --- a/x-pack/plugins/graph/public/helpers/use_workspace_loader.test.tsx +++ b/x-pack/plugins/graph/public/helpers/use_workspace_loader.test.tsx @@ -14,23 +14,29 @@ import { Workspace } from '../types'; import { SavedObjectsClientCommon } from 'src/plugins/data/common'; import { act } from 'react-dom/test-utils'; -jest.mock('react-router-dom', () => ({ - useHistory: () => ({ - replace: jest.fn(), - }), - useLocation: () => ({ - location: jest.fn(), - }), - useParams: () => ({ - id: jest.fn(), - }), -})); +jest.mock('react-router-dom', () => { + const useLocation = () => ({ + search: '2', + }); + + const replaceFn = jest.fn(); + + const useHistory = () => ({ + replace: replaceFn, + }); + return { + useHistory, + useLocation, + useParams: () => ({ + id: '1', + }), + }; +}); const mockSavedObjectsClient = ({ resolve: jest.fn().mockResolvedValue({ saved_object: { id: 10, _version: '7.15.0', attributes: { wsState: '{}' } }, - outcome: 'aliasMatch', - alias_target_id: 'aliasTargetId', + outcome: 'exactMatch', }), find: jest.fn().mockResolvedValue({ title: 'test' }), } as unknown) as SavedObjectsClientCommon; @@ -58,11 +64,30 @@ describe('use_workspace_loader', () => { spaces: spacesPluginMock.createStartContract(), }; - it('should redirect if outcome is aliasMatch', async () => { + it('should not redirect if outcome is exactMatch', async () => { await act(async () => { await setup((defaultProps as unknown) as UseWorkspaceLoaderProps); }); - expect(defaultProps.spaces.ui.redirectLegacyUrl).toHaveBeenCalledWith( + expect(defaultProps.spaces.ui.redirectLegacyUrl).not.toHaveBeenCalled(); + expect(defaultProps.store.dispatch).toHaveBeenCalled(); + }); + it('should redirect if outcome is aliasMatch', async () => { + const props = { + ...defaultProps, + spaces: spacesPluginMock.createStartContract(), + savedObjectsClient: { + ...mockSavedObjectsClient, + resolve: jest.fn().mockResolvedValue({ + saved_object: { id: 10, _version: '7.15.0', attributes: { wsState: '{}' } }, + outcome: 'aliasMatch', + alias_target_id: 'aliasTargetId', + }), + }, + }; + await act(async () => { + await setup((props as unknown) as UseWorkspaceLoaderProps); + }); + expect(props.spaces.ui.redirectLegacyUrl).toHaveBeenCalledWith( '#/workspace/aliasTargetId', 'Graph' ); diff --git a/x-pack/plugins/graph/public/helpers/use_workspace_loader.ts b/x-pack/plugins/graph/public/helpers/use_workspace_loader.ts index 74ba2f3bfbbd3..ee2afeee22e61 100644 --- a/x-pack/plugins/graph/public/helpers/use_workspace_loader.ts +++ b/x-pack/plugins/graph/public/helpers/use_workspace_loader.ts @@ -19,8 +19,8 @@ export interface UseWorkspaceLoaderProps { store: GraphStore; workspaceRef: React.MutableRefObject; savedObjectsClient: SavedObjectsClientContract; - spaces: SpacesApi; coreStart: CoreStart; + spaces?: SpacesApi; } interface WorkspaceUrlParams { @@ -31,6 +31,12 @@ export interface SharingSavedObjectProps { aliasTargetId?: string; } +interface WorkspaceLoadedState { + savedWorkspace?: GraphWorkspaceSavedObject; + indexPatterns?: IndexPatternSavedObject[]; + sharingSavedObjectProps?: SharingSavedObjectProps; +} + export const useWorkspaceLoader = ({ coreStart, spaces, @@ -38,11 +44,9 @@ export const useWorkspaceLoader = ({ store, savedObjectsClient, }: UseWorkspaceLoaderProps) => { - const [indexPatterns, setIndexPatterns] = useState(); - const [savedWorkspace, setSavedWorkspace] = useState(); - const [sharingSavedObjectProps, setSharingSavedObjectProps] = useState(); - const history = useHistory(); - const location = useLocation(); + const [state, setState] = useState({}); + const { replace: historyReplace } = useHistory(); + const { search } = useLocation(); const { id } = useParams(); /** @@ -50,7 +54,7 @@ export const useWorkspaceLoader = ({ * on changes in id parameter and URL query only. */ useEffect(() => { - const urlQuery = new URLSearchParams(location.search).get('query'); + const urlQuery = new URLSearchParams(search).get('query'); function loadWorkspace( fetchedSavedWorkspace: GraphWorkspaceSavedObject, @@ -91,7 +95,7 @@ export const useWorkspaceLoader = ({ defaultMessage: "Couldn't load graph with ID", }), }); - history.replace('/home'); + historyReplace('/home'); // return promise that never returns to prevent the controller from loading return new Promise(() => {}); }) @@ -127,24 +131,25 @@ export const useWorkspaceLoader = ({ } else if (workspaceRef.current) { clearStore(); } - - setIndexPatterns(fetchedIndexPatterns); - setSavedWorkspace(fetchedSavedWorkspace); - setSharingSavedObjectProps(fetchedSharingSavedObjectProps); + setState({ + savedWorkspace: fetchedSavedWorkspace, + indexPatterns: fetchedIndexPatterns, + sharingSavedObjectProps: fetchedSharingSavedObjectProps, + }); } initializeWorkspace(); }, [ id, - location, + search, store, - history, + historyReplace, savedObjectsClient, - setSavedWorkspace, + setState, coreStart, workspaceRef, spaces, ]); - return { savedWorkspace, indexPatterns, sharingSavedObjectProps }; + return state; }; diff --git a/x-pack/plugins/graph/public/plugin.ts b/x-pack/plugins/graph/public/plugin.ts index 230359e6e4780..1c44714a2c498 100644 --- a/x-pack/plugins/graph/public/plugin.ts +++ b/x-pack/plugins/graph/public/plugin.ts @@ -45,7 +45,7 @@ export interface GraphPluginStartDependencies { savedObjects: SavedObjectsStart; kibanaLegacy: KibanaLegacyStart; home?: HomePublicPluginStart; - spaces: SpacesApi; + spaces?: SpacesApi; } export class GraphPlugin From 889b608b9f3cb0a4e65f1c205374b0243d607396 Mon Sep 17 00:00:00 2001 From: Marta Bondyra Date: Thu, 9 Sep 2021 17:45:56 +0200 Subject: [PATCH 7/7] feedback from cr --- x-pack/plugins/graph/public/apps/workspace_route.tsx | 8 +------- .../workspace_layout/workspace_layout.test.tsx | 12 ++++++++++-- .../components/workspace_layout/workspace_layout.tsx | 12 ++++++++---- .../public/helpers/use_workspace_loader.test.tsx | 4 ++-- .../graph/public/helpers/use_workspace_loader.ts | 8 ++++---- 5 files changed, 25 insertions(+), 19 deletions(-) diff --git a/x-pack/plugins/graph/public/apps/workspace_route.tsx b/x-pack/plugins/graph/public/apps/workspace_route.tsx index 79f687d789fb7..55f481bf504f1 100644 --- a/x-pack/plugins/graph/public/apps/workspace_route.tsx +++ b/x-pack/plugins/graph/public/apps/workspace_route.tsx @@ -8,7 +8,7 @@ import React, { useMemo, useRef, useState } from 'react'; import { I18nProvider } from '@kbn/i18n/react'; import { Provider } from 'react-redux'; -import { useHistory, useLocation } from 'react-router-dom'; +import { useHistory } from 'react-router-dom'; import { KibanaContextProvider } from '../../../../../src/plugins/kibana_react/public'; import { showSaveModal } from '../../../../../src/plugins/saved_objects/public'; import { Workspace } from '../types'; @@ -57,7 +57,6 @@ export const WorkspaceRoute = ({ */ const [renderCounter, setRenderCounter] = useState(0); const history = useHistory(); - const urlQuery = new URLSearchParams(useLocation().search).get('query'); const indexPatternProvider = useMemo( () => createCachedIndexPatternProvider(getIndexPatternProvider.get), @@ -129,10 +128,6 @@ export const WorkspaceRoute = ({ const { savedWorkspace, indexPatterns, sharingSavedObjectProps } = loaded; - if (!savedWorkspace || !indexPatterns) { - return null; - } - return ( @@ -153,7 +148,6 @@ export const WorkspaceRoute = ({ indexPatterns={indexPatterns} savedWorkspace={savedWorkspace} indexPatternProvider={indexPatternProvider} - urlQuery={urlQuery} /> diff --git a/x-pack/plugins/graph/public/components/workspace_layout/workspace_layout.test.tsx b/x-pack/plugins/graph/public/components/workspace_layout/workspace_layout.test.tsx index ae1a726464be5..c535c9e32d335 100644 --- a/x-pack/plugins/graph/public/components/workspace_layout/workspace_layout.test.tsx +++ b/x-pack/plugins/graph/public/components/workspace_layout/workspace_layout.test.tsx @@ -15,6 +15,15 @@ import { GraphSavePolicy, GraphWorkspaceSavedObject, IndexPatternProvider } from import { OverlayStart, Capabilities } from 'kibana/public'; import { SharingSavedObjectProps } from '../../helpers/use_workspace_loader'; +jest.mock('react-router-dom', () => { + const useLocation = () => ({ + search: '?query={}', + }); + return { + useLocation, + }; +}); + describe('workspace_layout', () => { const defaultProps = { renderCounter: 1, @@ -30,7 +39,6 @@ describe('workspace_layout', () => { graphSavePolicy: 'configAndDataWithConsent' as GraphSavePolicy, navigation: {} as NavigationStart, canEditDrillDownUrls: true, - urlQuery: 'search', setHeaderActionMenu: jest.fn(), sharingSavedObjectProps: { outcome: 'exactMatch', @@ -49,7 +57,7 @@ describe('workspace_layout', () => { currentObjectId: 'test', objectNoun: 'Graph', otherObjectId: 'conflictId', - otherObjectPath: '#/workspace/conflictId', + otherObjectPath: '#/workspace/conflictId?query={}', }); }); }); diff --git a/x-pack/plugins/graph/public/components/workspace_layout/workspace_layout.tsx b/x-pack/plugins/graph/public/components/workspace_layout/workspace_layout.tsx index 4bb04264bb520..5426ae9228518 100644 --- a/x-pack/plugins/graph/public/components/workspace_layout/workspace_layout.tsx +++ b/x-pack/plugins/graph/public/components/workspace_layout/workspace_layout.tsx @@ -9,6 +9,7 @@ import React, { Fragment, memo, useCallback, useRef, useState } from 'react'; import { i18n } from '@kbn/i18n'; import { EuiSpacer } from '@elastic/eui'; import { connect } from 'react-redux'; +import { useLocation } from 'react-router-dom'; import { SearchBar } from '../search_bar'; import { GraphState, @@ -61,7 +62,6 @@ type WorkspaceLayoutProps = Pick< indexPatterns: IndexPatternSavedObject[]; savedWorkspace: GraphWorkspaceSavedObject; indexPatternProvider: IndexPatternProvider; - urlQuery: string | null; sharingSavedObjectProps?: SharingSavedObjectProps; }; @@ -85,7 +85,6 @@ export const WorkspaceLayoutComponent = ({ graphSavePolicy, navigation, canEditDrillDownUrls, - urlQuery, setHeaderActionMenu, sharingSavedObjectProps, spaces, @@ -96,6 +95,10 @@ export const WorkspaceLayoutComponent = ({ const [mergeCandidates, setMergeCandidates] = useState([]); const [control, setControl] = useState('none'); const selectedNode = useRef(undefined); + + const search = useLocation().search; + const urlQuery = new URLSearchParams(search).get('query'); + const isInitialized = Boolean(workspaceInitialized || savedWorkspace.id); const selectSelected = useCallback((node: WorkspaceNode) => { @@ -167,7 +170,8 @@ export const WorkspaceLayoutComponent = ({ // We have resolved to one object, but another object has a legacy URL alias associated with this ID/page. We should display a // callout with a warning for the user, and provide a way for them to navigate to the other object. const otherObjectId = sharingSavedObjectProps?.aliasTargetId!; // This is always defined if outcome === 'conflict' - const otherObjectPath = getEditUrl(coreStart.http.basePath.prepend, { id: otherObjectId }); + const otherObjectPath = + getEditUrl(coreStart.http.basePath.prepend, { id: otherObjectId }) + search; return spaces.ui.components.getLegacyUrlConflict({ objectNoun: i18n.translate('xpack.graph.legacyUrlConflict.objectNoun', { defaultMessage: 'Graph', @@ -178,7 +182,7 @@ export const WorkspaceLayoutComponent = ({ }); } return null; - }, [savedWorkspace.id, sharingSavedObjectProps, spaces, coreStart.http]); + }, [savedWorkspace.id, sharingSavedObjectProps, spaces, coreStart.http, search]); return ( diff --git a/x-pack/plugins/graph/public/helpers/use_workspace_loader.test.tsx b/x-pack/plugins/graph/public/helpers/use_workspace_loader.test.tsx index b81be35d3c021..db80289d0d32d 100644 --- a/x-pack/plugins/graph/public/helpers/use_workspace_loader.test.tsx +++ b/x-pack/plugins/graph/public/helpers/use_workspace_loader.test.tsx @@ -16,7 +16,7 @@ import { act } from 'react-dom/test-utils'; jest.mock('react-router-dom', () => { const useLocation = () => ({ - search: '2', + search: '?query={}', }); const replaceFn = jest.fn(); @@ -88,7 +88,7 @@ describe('use_workspace_loader', () => { await setup((props as unknown) as UseWorkspaceLoaderProps); }); expect(props.spaces.ui.redirectLegacyUrl).toHaveBeenCalledWith( - '#/workspace/aliasTargetId', + '#/workspace/aliasTargetId?query={}', 'Graph' ); }); diff --git a/x-pack/plugins/graph/public/helpers/use_workspace_loader.ts b/x-pack/plugins/graph/public/helpers/use_workspace_loader.ts index ee2afeee22e61..b9abf31e084fe 100644 --- a/x-pack/plugins/graph/public/helpers/use_workspace_loader.ts +++ b/x-pack/plugins/graph/public/helpers/use_workspace_loader.ts @@ -32,8 +32,8 @@ export interface SharingSavedObjectProps { } interface WorkspaceLoadedState { - savedWorkspace?: GraphWorkspaceSavedObject; - indexPatterns?: IndexPatternSavedObject[]; + savedWorkspace: GraphWorkspaceSavedObject; + indexPatterns: IndexPatternSavedObject[]; sharingSavedObjectProps?: SharingSavedObjectProps; } @@ -44,7 +44,7 @@ export const useWorkspaceLoader = ({ store, savedObjectsClient, }: UseWorkspaceLoaderProps) => { - const [state, setState] = useState({}); + const [state, setState] = useState(); const { replace: historyReplace } = useHistory(); const { search } = useLocation(); const { id } = useParams(); @@ -112,7 +112,7 @@ export const useWorkspaceLoader = ({ if (spaces && fetchedSharingSavedObjectProps?.outcome === 'aliasMatch') { // We found this object by a legacy URL alias from its old ID; redirect the user to the page with its new ID, preserving any URL hash const newObjectId = fetchedSharingSavedObjectProps?.aliasTargetId!; // This is always defined if outcome === 'aliasMatch' - const newPath = getEditUrl(coreStart.http.basePath.prepend, { id: newObjectId }); + const newPath = getEditUrl(coreStart.http.basePath.prepend, { id: newObjectId }) + search; spaces.ui.redirectLegacyUrl( newPath, i18n.translate('xpack.graph.legacyUrlConflict.objectNoun', {