Skip to content

Commit

Permalink
[Graph] Fix guidance panel appearing for a moment when saving Graph (e…
Browse files Browse the repository at this point in the history
…lastic#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
  • Loading branch information
davismcphee authored Sep 22, 2022
1 parent 27b8e98 commit b026d2c
Show file tree
Hide file tree
Showing 4 changed files with 115 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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 = () => ({
Expand Down Expand Up @@ -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(
Expand All @@ -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(
<WorkspaceLayoutComponent
{...defaultProps}
workspaceInitialized={false}
savedWorkspace={{ id: undefined, isSaving: false } as GraphWorkspaceSavedObject}
/>
);
expect(component.find(GraphVisualization).exists()).toBe(false);
});
it('should show GraphVisualization when workspaceInitialized is true', () => {
const component = shallow(
<WorkspaceLayoutComponent
{...defaultProps}
workspaceInitialized={true}
savedWorkspace={{ id: undefined, isSaving: false } as GraphWorkspaceSavedObject}
/>
);
expect(component.find(GraphVisualization).exists()).toBe(true);
});
it('should show GraphVisualization when savedWorkspace.id is defined', () => {
const component = shallow(
<WorkspaceLayoutComponent
{...defaultProps}
workspaceInitialized={false}
savedWorkspace={{ id: 'test', isSaving: false } as GraphWorkspaceSavedObject}
/>
);
expect(component.find(GraphVisualization).exists()).toBe(true);
});
it('should show GraphVisualization when savedWorkspace.isSaving is true', () => {
const component = shallow(
<WorkspaceLayoutComponent
{...defaultProps}
workspaceInitialized={false}
savedWorkspace={{ id: undefined, isSaving: true } as GraphWorkspaceSavedObject}
/>
);
expect(component.find(GraphVisualization).exists()).toBe(true);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
47 changes: 47 additions & 0 deletions x-pack/plugins/graph/public/helpers/saved_workspace_utils.test.ts
Original file line number Diff line number Diff line change
@@ -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);
});
});
});
28 changes: 15 additions & 13 deletions x-pack/plugins/graph/public/helpers/saved_workspace_utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand All @@ -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,
Expand Down

0 comments on commit b026d2c

Please sign in to comment.