From 04b873091eca2330ba60b81127d398137a2f4db0 Mon Sep 17 00:00:00 2001 From: Davis McPhee Date: Thu, 22 Sep 2022 10:22:18 -0300 Subject: [PATCH] [Graph] Fix guidance panel appearing for a moment when saving Graph (#141228) * [Discover] Fix issue where workspace switches to uninitialized briefly when saving as copy in Graph * [Graph] Add Jest tests for Graph workspace saving bug (cherry picked from commit b026d2c84d25be61293dd2212c29780efedafceb) --- .../workspace_layout.test.tsx | 49 ++++++++++++++++++- .../workspace_layout/workspace_layout.tsx | 6 ++- .../helpers/saved_workspace_utils.test.ts | 47 ++++++++++++++++++ .../public/helpers/saved_workspace_utils.ts | 28 ++++++----- 4 files changed, 115 insertions(+), 15 deletions(-) create mode 100644 x-pack/plugins/graph/public/helpers/saved_workspace_utils.test.ts 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 57b4ff613c88b..cb43b5ec5d688 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 @@ -11,9 +11,15 @@ import { WorkspaceLayoutComponent } from '.'; import { coreMock } from '@kbn/core/public/mocks'; import { spacesPluginMock } from '@kbn/spaces-plugin/public/mocks'; import { NavigationPublicPluginStart as NavigationStart } from '@kbn/navigation-plugin/public'; -import { GraphSavePolicy, GraphWorkspaceSavedObject, IndexPatternProvider } from '../../types'; +import { + GraphSavePolicy, + GraphWorkspaceSavedObject, + IndexPatternProvider, + Workspace, +} from '../../types'; import { OverlayStart, Capabilities } from '@kbn/core/public'; import { SharingSavedObjectProps } from '../../helpers/use_workspace_loader'; +import { GraphVisualization } from '../graph_visualization'; jest.mock('react-router-dom', () => { const useLocation = () => ({ @@ -45,6 +51,7 @@ describe('workspace_layout', () => { aliasTargetId: '', } as SharingSavedObjectProps, spaces: spacesPluginMock.createStartContract(), + workspace: {} as unknown as Workspace, }; it('should display conflict notification if outcome is conflict', () => { shallow( @@ -60,4 +67,44 @@ describe('workspace_layout', () => { otherObjectPath: '#/workspace/conflictId?query={}', }); }); + it('should not show GraphVisualization when workspaceInitialized is false, savedWorkspace.id is undefined, and savedWorkspace.isSaving is false', () => { + const component = shallow( + + ); + expect(component.find(GraphVisualization).exists()).toBe(false); + }); + it('should show GraphVisualization when workspaceInitialized is true', () => { + const component = shallow( + + ); + expect(component.find(GraphVisualization).exists()).toBe(true); + }); + it('should show GraphVisualization when savedWorkspace.id is defined', () => { + const component = shallow( + + ); + expect(component.find(GraphVisualization).exists()).toBe(true); + }); + it('should show GraphVisualization when savedWorkspace.isSaving is true', () => { + const component = shallow( + + ); + expect(component.find(GraphVisualization).exists()).toBe(true); + }); }); 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 6cfbbcf7d9105..3eede479bd801 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 @@ -91,7 +91,11 @@ export const WorkspaceLayoutComponent = ({ const search = useLocation().search; const urlQuery = new URLSearchParams(search).get('query'); - const isInitialized = Boolean(workspaceInitialized || savedWorkspace.id); + // savedWorkspace.id gets set to null while saving a copy of an existing + // workspace, so we need to check for savedWorkspace.isSaving as well + const isInitialized = Boolean( + workspaceInitialized || savedWorkspace.id || savedWorkspace.isSaving + ); const selectSelected = useCallback((node: WorkspaceNode) => { selectedNode.current = node; diff --git a/x-pack/plugins/graph/public/helpers/saved_workspace_utils.test.ts b/x-pack/plugins/graph/public/helpers/saved_workspace_utils.test.ts new file mode 100644 index 0000000000000..7d4027c91858f --- /dev/null +++ b/x-pack/plugins/graph/public/helpers/saved_workspace_utils.test.ts @@ -0,0 +1,47 @@ +/* + * 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 { coreMock } from '@kbn/core/public/mocks'; +import { GraphWorkspaceSavedObject } from '../types'; +import { saveSavedWorkspace } from './saved_workspace_utils'; + +const core = coreMock.createStart(); + +describe('saved_workspace_utils', () => { + describe('saveSavedWorkspace', () => { + it('should delete the savedWorkspace id and set isSaving to true immediately when copyOnSave is true', async () => { + const savedWorkspace = { + id: '123', + title: 'my workspace', + lastSavedTitle: 'my workspace', + migrationVersion: {}, + wsState: '{ "indexPattern": "my-index-pattern" }', + getEsType: () => 'graph-workspace', + copyOnSave: true, + isSaving: false, + } as GraphWorkspaceSavedObject; + const promise = saveSavedWorkspace( + savedWorkspace, + {}, + { + savedObjectsClient: { + ...core.savedObjects.client, + find: jest.fn().mockResolvedValue({ savedObjects: [] }), + create: jest.fn().mockResolvedValue({ id: '456' }), + }, + overlays: core.overlays, + } + ); + expect(savedWorkspace.id).toBe(undefined); + expect(savedWorkspace.isSaving).toBe(true); + const id = await promise; + expect(id).toBe('456'); + expect(savedWorkspace.id).toBe('456'); + expect(savedWorkspace.isSaving).toBe(false); + }); + }); +}); 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 202d13f9cd539..03e377792f1ac 100644 --- a/x-pack/plugins/graph/public/helpers/saved_workspace_utils.ts +++ b/x-pack/plugins/graph/public/helpers/saved_workspace_utils.ts @@ -162,18 +162,6 @@ export async function saveSavedWorkspace( overlays: OverlayStart; } ) { - // Save the original id in case the save fails. - const originalId = savedObject.id; - // Read https://github.com/elastic/kibana/issues/9056 and - // https://github.com/elastic/kibana/issues/9012 for some background into why this copyOnSave variable - // exists. - // The goal is to move towards a better rename flow, but since our users have been conditioned - // to expect a 'save as' flow during a rename, we are keeping the logic the same until a better - // UI/UX can be worked out. - if (savedObject.copyOnSave) { - delete savedObject.id; - } - let attributes: SavedObjectAttributes = {}; forOwn(mapping, (fieldType, fieldName) => { @@ -191,14 +179,28 @@ export async function saveSavedWorkspace( throw new Error('References not returned from extractReferences'); } + // Save the original id in case the save fails. + const originalId = savedObject.id; + try { + // Read https://github.com/elastic/kibana/issues/9056 and + // https://github.com/elastic/kibana/issues/9012 for some background into why this copyOnSave variable + // exists. + // The goal is to move towards a better rename flow, but since our users have been conditioned + // to expect a 'save as' flow during a rename, we are keeping the logic the same until a better + // UI/UX can be worked out. + if (savedObject.copyOnSave) { + delete savedObject.id; + } + + savedObject.isSaving = true; + await checkForDuplicateTitle( savedObject as any, isTitleDuplicateConfirmed, onTitleDuplicate, services ); - savedObject.isSaving = true; const createOpt = { id: savedObject.id,