From b061d85f9a85d467beae8da9a0d46758d6e194eb Mon Sep 17 00:00:00 2001 From: Wylie Conlon Date: Mon, 1 Jun 2020 15:05:27 -0400 Subject: [PATCH] [Lens] Warn if leaving with unsaved visualization (#67689) * [Lens] Warn if leaving with unsaved visualization * Made confirmation logic more robust and add title Co-authored-by: Elastic Machine --- .../lens/public/app_plugin/app.test.tsx | 216 +++++++++++++----- x-pack/plugins/lens/public/app_plugin/app.tsx | 57 ++++- .../lens/public/app_plugin/mounter.tsx | 1 + 3 files changed, 208 insertions(+), 66 deletions(-) diff --git a/x-pack/plugins/lens/public/app_plugin/app.test.tsx b/x-pack/plugins/lens/public/app_plugin/app.test.tsx index 1762965478292..f1a2edd2d554f 100644 --- a/x-pack/plugins/lens/public/app_plugin/app.test.tsx +++ b/x-pack/plugins/lens/public/app_plugin/app.test.tsx @@ -9,6 +9,7 @@ import { ReactWrapper } from 'enzyme'; import { act } from 'react-dom/test-utils'; import { App } from './app'; import { EditorFrameInstance } from '../types'; +import { AppMountParameters } from 'kibana/public'; import { Storage } from '../../../../../src/plugins/kibana_utils/public'; import { Document, SavedObjectStore } from '../persistence'; import { mount } from 'enzyme'; @@ -111,6 +112,7 @@ describe('Lens App', () => { newlyCreated?: boolean ) => void; originatingApp: string | undefined; + onAppLeave: AppMountParameters['onAppLeave']; }> { return ({ navigation: navigationStartMock, @@ -153,6 +155,7 @@ describe('Lens App', () => { newlyCreated?: boolean ) => {} ), + onAppLeave: jest.fn(), } as unknown) as jest.Mocked<{ navigation: typeof navigationStartMock; editorFrame: EditorFrameInstance; @@ -168,6 +171,7 @@ describe('Lens App', () => { newlyCreated?: boolean ) => void; originatingApp: string | undefined; + onAppLeave: AppMountParameters['onAppLeave']; }>; } @@ -357,22 +361,7 @@ describe('Lens App', () => { newTitle: string; } - let defaultArgs: jest.Mocked<{ - editorFrame: EditorFrameInstance; - navigation: typeof navigationStartMock; - data: typeof dataStartMock; - core: typeof core; - storage: Storage; - docId?: string; - docStorage: SavedObjectStore; - redirectTo: ( - id?: string, - returnToOrigin?: boolean, - originatingApp?: string | undefined, - newlyCreated?: boolean - ) => void; - originatingApp: string | undefined; - }>; + let defaultArgs: ReturnType; beforeEach(() => { defaultArgs = makeDefaultArgs(); @@ -486,30 +475,6 @@ describe('Lens App', () => { expect(getButton(instance).disableButton).toEqual(true); }); - it('shows a disabled save button when there are no changes to the document', async () => { - const args = defaultArgs; - (args.docStorage.load as jest.Mock).mockResolvedValue({ - id: '1234', - title: 'My cool doc', - expression: '', - } as jest.ResolvedValue); - args.editorFrame = frame; - - instance = mount(); - expect(getButton(instance).disableButton).toEqual(true); - - const onChange = frame.mount.mock.calls[0][1].onChange; - - act(() => { - onChange({ - filterableIndexPatterns: [], - doc: ({ id: '1234', expression: 'valid expression' } as unknown) as Document, - }); - }); - instance.update(); - expect(getButton(instance).disableButton).toEqual(false); - }); - it('shows a save button that is enabled when the frame has provided its state', async () => { const args = defaultArgs; args.editorFrame = frame; @@ -691,21 +656,7 @@ describe('Lens App', () => { }); describe('query bar state management', () => { - let defaultArgs: jest.Mocked<{ - editorFrame: EditorFrameInstance; - data: typeof dataStartMock; - navigation: typeof navigationStartMock; - core: typeof core; - storage: Storage; - docId?: string; - docStorage: SavedObjectStore; - redirectTo: ( - id?: string, - returnToOrigin?: boolean, - originatingApp?: string | undefined, - newlyCreated?: boolean - ) => void; - }>; + let defaultArgs: ReturnType; beforeEach(() => { defaultArgs = makeDefaultArgs(); @@ -1001,4 +952,159 @@ describe('Lens App', () => { expect(args.core.notifications.toasts.addDanger).toHaveBeenCalled(); }); + + describe('showing a confirm message when leaving', () => { + let defaultArgs: ReturnType; + let defaultLeave: jest.Mock; + let confirmLeave: jest.Mock; + + beforeEach(() => { + defaultArgs = makeDefaultArgs(); + defaultLeave = jest.fn(); + confirmLeave = jest.fn(); + (defaultArgs.docStorage.load as jest.Mock).mockResolvedValue({ + id: '1234', + title: 'My cool doc', + expression: 'valid expression', + state: { + query: 'kuery', + datasourceMetaData: { filterableIndexPatterns: [{ id: '1', title: 'saved' }] }, + }, + } as jest.ResolvedValue); + }); + + it('should not show a confirm message if there is no expression to save', () => { + instance = mount(); + + const lastCall = + defaultArgs.onAppLeave.mock.calls[defaultArgs.onAppLeave.mock.calls.length - 1][0]; + lastCall({ default: defaultLeave, confirm: confirmLeave }); + + expect(defaultLeave).toHaveBeenCalled(); + expect(confirmLeave).not.toHaveBeenCalled(); + }); + + it('does not confirm if the user is missing save permissions', () => { + const args = defaultArgs; + args.core.application = { + ...args.core.application, + capabilities: { + ...args.core.application.capabilities, + visualize: { save: false, saveQuery: false, show: true }, + }, + }; + args.editorFrame = frame; + + instance = mount(); + + const onChange = frame.mount.mock.calls[0][1].onChange; + act(() => + onChange({ + filterableIndexPatterns: [], + doc: ({ id: undefined, expression: 'valid expression' } as unknown) as Document, + }) + ); + instance.update(); + + const lastCall = + defaultArgs.onAppLeave.mock.calls[defaultArgs.onAppLeave.mock.calls.length - 1][0]; + lastCall({ default: defaultLeave, confirm: confirmLeave }); + + expect(defaultLeave).toHaveBeenCalled(); + expect(confirmLeave).not.toHaveBeenCalled(); + }); + + it('should confirm when leaving with an unsaved doc', () => { + defaultArgs.editorFrame = frame; + instance = mount(); + + const onChange = frame.mount.mock.calls[0][1].onChange; + act(() => + onChange({ + filterableIndexPatterns: [], + doc: ({ id: undefined, expression: 'valid expression' } as unknown) as Document, + }) + ); + instance.update(); + + const lastCall = + defaultArgs.onAppLeave.mock.calls[defaultArgs.onAppLeave.mock.calls.length - 1][0]; + lastCall({ default: defaultLeave, confirm: confirmLeave }); + + expect(confirmLeave).toHaveBeenCalled(); + expect(defaultLeave).not.toHaveBeenCalled(); + }); + + it('should confirm when leaving with unsaved changes to an existing doc', async () => { + defaultArgs.editorFrame = frame; + instance = mount(); + await act(async () => { + instance.setProps({ docId: '1234' }); + }); + + const onChange = frame.mount.mock.calls[0][1].onChange; + act(() => + onChange({ + filterableIndexPatterns: [], + doc: ({ id: '1234', expression: 'different expression' } as unknown) as Document, + }) + ); + instance.update(); + + const lastCall = + defaultArgs.onAppLeave.mock.calls[defaultArgs.onAppLeave.mock.calls.length - 1][0]; + lastCall({ default: defaultLeave, confirm: confirmLeave }); + + expect(confirmLeave).toHaveBeenCalled(); + expect(defaultLeave).not.toHaveBeenCalled(); + }); + + it('should not confirm when changes are saved', async () => { + defaultArgs.editorFrame = frame; + instance = mount(); + await act(async () => { + instance.setProps({ docId: '1234' }); + }); + + const onChange = frame.mount.mock.calls[0][1].onChange; + act(() => + onChange({ + filterableIndexPatterns: [], + doc: ({ id: '1234', expression: 'valid expression' } as unknown) as Document, + }) + ); + instance.update(); + + const lastCall = + defaultArgs.onAppLeave.mock.calls[defaultArgs.onAppLeave.mock.calls.length - 1][0]; + lastCall({ default: defaultLeave, confirm: confirmLeave }); + + expect(defaultLeave).toHaveBeenCalled(); + expect(confirmLeave).not.toHaveBeenCalled(); + }); + + it('should confirm when the latest doc is invalid', async () => { + defaultArgs.editorFrame = frame; + instance = mount(); + await act(async () => { + instance.setProps({ docId: '1234' }); + }); + + const onChange = frame.mount.mock.calls[0][1].onChange; + act(() => + onChange({ + filterableIndexPatterns: [], + doc: ({ id: '1234', expression: null } as unknown) as Document, + }) + ); + instance.update(); + + const lastCall = + defaultArgs.onAppLeave.mock.calls[defaultArgs.onAppLeave.mock.calls.length - 1][0]; + lastCall({ default: defaultLeave, confirm: confirmLeave }); + + expect(confirmLeave).toHaveBeenCalled(); + expect(defaultLeave).not.toHaveBeenCalled(); + }); + }); }); diff --git a/x-pack/plugins/lens/public/app_plugin/app.tsx b/x-pack/plugins/lens/public/app_plugin/app.tsx index a77fbbb597564..ffa59a6fb6bc9 100644 --- a/x-pack/plugins/lens/public/app_plugin/app.tsx +++ b/x-pack/plugins/lens/public/app_plugin/app.tsx @@ -10,7 +10,7 @@ import { I18nProvider } from '@kbn/i18n/react'; import { i18n } from '@kbn/i18n'; import { Query, DataPublicPluginStart } from 'src/plugins/data/public'; import { NavigationPublicPluginStart } from 'src/plugins/navigation/public'; -import { AppMountContext, NotificationsStart } from 'kibana/public'; +import { AppMountContext, AppMountParameters, NotificationsStart } from 'kibana/public'; import { IStorageWrapper } from 'src/plugins/kibana_utils/public'; import { KibanaContextProvider } from '../../../../../src/plugins/kibana_react/public'; import { @@ -57,6 +57,7 @@ export function App({ redirectTo, originatingAppFromUrl, navigation, + onAppLeave, }: { editorFrame: EditorFrameInstance; data: DataPublicPluginStart; @@ -72,6 +73,7 @@ export function App({ newlyCreated?: boolean ) => void; originatingAppFromUrl?: string | undefined; + onAppLeave: AppMountParameters['onAppLeave']; }) { const language = storage.get('kibana.userQueryLanguage') || core.uiSettings.get('search:queryLanguage'); @@ -94,6 +96,12 @@ export function App({ const { lastKnownDoc } = state; + const isSaveable = + lastKnownDoc && + lastKnownDoc.expression && + lastKnownDoc.expression.length > 0 && + core.application.capabilities.visualize.save; + useEffect(() => { // Clear app-specific filters when navigating to Lens. Necessary because Lens // can be loaded without a full page refresh @@ -123,7 +131,31 @@ export function App({ filterSubscription.unsubscribe(); timeSubscription.unsubscribe(); }; - }, []); + }, [data.query.filterManager, data.query.timefilter.timefilter]); + + useEffect(() => { + onAppLeave((actions) => { + // Confirm when the user has made any changes to an existing doc + // or when the user has configured something without saving + if ( + core.application.capabilities.visualize.save && + (state.persistedDoc?.expression + ? !_.isEqual(lastKnownDoc?.expression, state.persistedDoc.expression) + : lastKnownDoc?.expression) + ) { + return actions.confirm( + i18n.translate('xpack.lens.app.unsavedWorkMessage', { + defaultMessage: 'Leave Lens with unsaved work?', + }), + i18n.translate('xpack.lens.app.unsavedWorkTitle', { + defaultMessage: 'Unsaved changes', + }) + ); + } else { + return actions.default(); + } + }); + }, [lastKnownDoc, onAppLeave, state.persistedDoc, core.application.capabilities.visualize.save]); // Sync Kibana breadcrumbs any time the saved document's title changes useEffect(() => { @@ -144,7 +176,7 @@ export function App({ : i18n.translate('xpack.lens.breadcrumbsCreate', { defaultMessage: 'Create' }), }, ]); - }, [state.persistedDoc && state.persistedDoc.title]); + }, [core.application, core.chrome, core.http.basePath, state.persistedDoc]); useEffect(() => { if (docId && (!state.persistedDoc || state.persistedDoc.id !== docId)) { @@ -187,13 +219,16 @@ export function App({ redirectTo(); }); } - }, [docId]); - - const isSaveable = - lastKnownDoc && - lastKnownDoc.expression && - lastKnownDoc.expression.length > 0 && - core.application.capabilities.visualize.save; + }, [ + core.notifications, + data.indexPatterns, + data.query.filterManager, + docId, + // TODO: These dependencies are changing too often + // docStorage, + // redirectTo, + // state.persistedDoc, + ]); const runSave = ( saveProps: Omit & { @@ -257,7 +292,7 @@ export function App({ core.notifications.toasts.addDanger({ title: e.message, }), - [] + [core.notifications.toasts] ); const { TopNavMenu } = navigation.ui; diff --git a/x-pack/plugins/lens/public/app_plugin/mounter.tsx b/x-pack/plugins/lens/public/app_plugin/mounter.tsx index 7c875935f6320..032ce8325dca1 100644 --- a/x-pack/plugins/lens/public/app_plugin/mounter.tsx +++ b/x-pack/plugins/lens/public/app_plugin/mounter.tsx @@ -92,6 +92,7 @@ export async function mountApp( redirectTo(routeProps, id, returnToOrigin, originatingApp, newlyCreated) } originatingAppFromUrl={originatingAppFromUrl} + onAppLeave={params.onAppLeave} /> ); };