From 8ce9b474d671dab83591254f973e3cb506aca511 Mon Sep 17 00:00:00 2001 From: Devon Thomson Date: Fri, 18 Dec 2020 15:07:36 -0500 Subject: [PATCH] [Time to Visualize] Fix Dashboard OnAppLeave (#86193) Added isTransferInProgress to embeddable_state_transfer for apps to determine whether or not to show onAppLeave confirm --- ...c.embeddablestatetransfer._constructor_.md | 3 +- ...dablestatetransfer.istransferinprogress.md | 11 +++++++ ...beddable-public.embeddablestatetransfer.md | 3 +- .../public/application/dashboard_app.tsx | 17 +++++++--- .../embeddable_state_transfer.test.ts | 31 +++++++++++++++++-- .../embeddable_state_transfer.ts | 8 +++++ src/plugins/embeddable/public/plugin.tsx | 8 ++++- src/plugins/embeddable/public/public.api.md | 4 ++- test/common/services/security/test_user.ts | 6 ++-- .../dashboard/create_and_add_embeddables.ts | 2 +- .../apps/dashboard/dashboard_time_picker.ts | 6 ++++ .../apps/dashboard/panel_context_menu.ts | 2 +- .../apps/dashboard/panel_replacing.ts | 2 ++ test/functional/page_objects/header_page.ts | 17 ++++++++-- .../services/dashboard/visualizations.ts | 3 +- 15 files changed, 102 insertions(+), 21 deletions(-) create mode 100644 docs/development/plugins/embeddable/public/kibana-plugin-plugins-embeddable-public.embeddablestatetransfer.istransferinprogress.md diff --git a/docs/development/plugins/embeddable/public/kibana-plugin-plugins-embeddable-public.embeddablestatetransfer._constructor_.md b/docs/development/plugins/embeddable/public/kibana-plugin-plugins-embeddable-public.embeddablestatetransfer._constructor_.md index 276499b435e1..77e9c2d00b2d 100644 --- a/docs/development/plugins/embeddable/public/kibana-plugin-plugins-embeddable-public.embeddablestatetransfer._constructor_.md +++ b/docs/development/plugins/embeddable/public/kibana-plugin-plugins-embeddable-public.embeddablestatetransfer._constructor_.md @@ -9,7 +9,7 @@ Constructs a new instance of the `EmbeddableStateTransfer` class Signature: ```typescript -constructor(navigateToApp: ApplicationStart['navigateToApp'], appList?: ReadonlyMap | undefined, customStorage?: Storage); +constructor(navigateToApp: ApplicationStart['navigateToApp'], currentAppId$: ApplicationStart['currentAppId$'], appList?: ReadonlyMap | undefined, customStorage?: Storage); ``` ## Parameters @@ -17,6 +17,7 @@ constructor(navigateToApp: ApplicationStart['navigateToApp'], appList?: Readonly | Parameter | Type | Description | | --- | --- | --- | | navigateToApp | ApplicationStart['navigateToApp'] | | +| currentAppId$ | ApplicationStart['currentAppId$'] | | | appList | ReadonlyMap<string, PublicAppInfo> | undefined | | | customStorage | Storage | | diff --git a/docs/development/plugins/embeddable/public/kibana-plugin-plugins-embeddable-public.embeddablestatetransfer.istransferinprogress.md b/docs/development/plugins/embeddable/public/kibana-plugin-plugins-embeddable-public.embeddablestatetransfer.istransferinprogress.md new file mode 100644 index 000000000000..f00d015f316d --- /dev/null +++ b/docs/development/plugins/embeddable/public/kibana-plugin-plugins-embeddable-public.embeddablestatetransfer.istransferinprogress.md @@ -0,0 +1,11 @@ + + +[Home](./index.md) > [kibana-plugin-plugins-embeddable-public](./kibana-plugin-plugins-embeddable-public.md) > [EmbeddableStateTransfer](./kibana-plugin-plugins-embeddable-public.embeddablestatetransfer.md) > [isTransferInProgress](./kibana-plugin-plugins-embeddable-public.embeddablestatetransfer.istransferinprogress.md) + +## EmbeddableStateTransfer.isTransferInProgress property + +Signature: + +```typescript +isTransferInProgress: boolean; +``` diff --git a/docs/development/plugins/embeddable/public/kibana-plugin-plugins-embeddable-public.embeddablestatetransfer.md b/docs/development/plugins/embeddable/public/kibana-plugin-plugins-embeddable-public.embeddablestatetransfer.md index 3676b744b8cc..76b6708b93bd 100644 --- a/docs/development/plugins/embeddable/public/kibana-plugin-plugins-embeddable-public.embeddablestatetransfer.md +++ b/docs/development/plugins/embeddable/public/kibana-plugin-plugins-embeddable-public.embeddablestatetransfer.md @@ -16,13 +16,14 @@ export declare class EmbeddableStateTransfer | Constructor | Modifiers | Description | | --- | --- | --- | -| [(constructor)(navigateToApp, appList, customStorage)](./kibana-plugin-plugins-embeddable-public.embeddablestatetransfer._constructor_.md) | | Constructs a new instance of the EmbeddableStateTransfer class | +| [(constructor)(navigateToApp, currentAppId$, appList, customStorage)](./kibana-plugin-plugins-embeddable-public.embeddablestatetransfer._constructor_.md) | | Constructs a new instance of the EmbeddableStateTransfer class | ## Properties | Property | Modifiers | Type | Description | | --- | --- | --- | --- | | [getAppNameFromId](./kibana-plugin-plugins-embeddable-public.embeddablestatetransfer.getappnamefromid.md) | | (appId: string) => string | undefined | Fetches an internationalized app title when given an appId. | +| [isTransferInProgress](./kibana-plugin-plugins-embeddable-public.embeddablestatetransfer.istransferinprogress.md) | | boolean | | ## Methods diff --git a/src/plugins/dashboard/public/application/dashboard_app.tsx b/src/plugins/dashboard/public/application/dashboard_app.tsx index 8eff48251b37..845d64c16500 100644 --- a/src/plugins/dashboard/public/application/dashboard_app.tsx +++ b/src/plugins/dashboard/public/application/dashboard_app.tsx @@ -45,6 +45,7 @@ import { removeQueryParam } from '../services/kibana_utils'; import { IndexPattern } from '../services/data'; import { EmbeddableRenderer } from '../services/embeddable'; import { DashboardContainerInput } from '.'; +import { leaveConfirmStrings } from '../dashboard_strings'; export interface DashboardAppProps { history: History; @@ -64,8 +65,9 @@ export function DashboardApp({ core, onAppLeave, uiSettings, - indexPatterns: indexPatternService, + embeddable, dashboardCapabilities, + indexPatterns: indexPatternService, } = useKibana().services; const [lastReloadTime, setLastReloadTime] = useState(0); @@ -196,9 +198,14 @@ export function DashboardApp({ return; } onAppLeave((actions) => { - if (dashboardStateManager?.getIsDirty()) { - // TODO: Finish App leave handler with overrides when redirecting to an editor. - // return actions.confirm(leaveConfirmStrings.leaveSubtitle, leaveConfirmStrings.leaveTitle); + if ( + dashboardStateManager?.getIsDirty() && + !embeddable.getStateTransfer().isTransferInProgress + ) { + return actions.confirm( + leaveConfirmStrings.getLeaveSubtitle(), + leaveConfirmStrings.getLeaveTitle() + ); } return actions.default(); }); @@ -206,7 +213,7 @@ export function DashboardApp({ // reset on app leave handler so leaving from the listing page doesn't trigger a confirmation onAppLeave((actions) => actions.default()); }; - }, [dashboardStateManager, dashboardContainer, onAppLeave]); + }, [dashboardStateManager, dashboardContainer, onAppLeave, embeddable]); // Refresh the dashboard container when lastReloadTime changes useEffect(() => { diff --git a/src/plugins/embeddable/public/lib/state_transfer/embeddable_state_transfer.test.ts b/src/plugins/embeddable/public/lib/state_transfer/embeddable_state_transfer.test.ts index cbaeddf472d5..be034d125dce 100644 --- a/src/plugins/embeddable/public/lib/state_transfer/embeddable_state_transfer.test.ts +++ b/src/plugins/embeddable/public/lib/state_transfer/embeddable_state_transfer.test.ts @@ -23,6 +23,7 @@ import { EmbeddableStateTransfer } from '.'; import { ApplicationStart, PublicAppInfo } from '../../../../../core/public'; import { EMBEDDABLE_EDITOR_STATE_KEY, EMBEDDABLE_PACKAGE_STATE_KEY } from './types'; import { EMBEDDABLE_STATE_TRANSFER_STORAGE_KEY } from './embeddable_state_transfer'; +import { Subject } from 'rxjs'; const createStorage = (): Storage => { const createMockStore = () => { @@ -46,16 +47,24 @@ const createStorage = (): Storage => { describe('embeddable state transfer', () => { let application: jest.Mocked; let stateTransfer: EmbeddableStateTransfer; + let currentAppId$: Subject; let store: Storage; const destinationApp = 'superUltraVisualize'; const originatingApp = 'superUltraTestDashboard'; beforeEach(() => { + currentAppId$ = new Subject(); + currentAppId$.next(originatingApp); const core = coreMock.createStart(); application = core.application; store = createStorage(); - stateTransfer = new EmbeddableStateTransfer(application.navigateToApp, undefined, store); + stateTransfer = new EmbeddableStateTransfer( + application.navigateToApp, + currentAppId$, + undefined, + store + ); }); it('cannot fetch app name when given no app list', async () => { @@ -67,7 +76,7 @@ describe('embeddable state transfer', () => { ['testId', { title: 'State Transfer Test App Hello' } as PublicAppInfo], ['testId2', { title: 'State Transfer Test App Goodbye' } as PublicAppInfo], ]); - stateTransfer = new EmbeddableStateTransfer(application.navigateToApp, appsList); + stateTransfer = new EmbeddableStateTransfer(application.navigateToApp, currentAppId$, appsList); expect(stateTransfer.getAppNameFromId('kibanana')).toBeUndefined(); }); @@ -76,7 +85,7 @@ describe('embeddable state transfer', () => { ['testId', { title: 'State Transfer Test App Hello' } as PublicAppInfo], ['testId2', { title: 'State Transfer Test App Goodbye' } as PublicAppInfo], ]); - stateTransfer = new EmbeddableStateTransfer(application.navigateToApp, appsList); + stateTransfer = new EmbeddableStateTransfer(application.navigateToApp, currentAppId$, appsList); expect(stateTransfer.getAppNameFromId('testId')).toBe('State Transfer Test App Hello'); expect(stateTransfer.getAppNameFromId('testId2')).toBe('State Transfer Test App Goodbye'); }); @@ -107,6 +116,13 @@ describe('embeddable state transfer', () => { }); }); + it('sets isTransferInProgress to true when sending an outgoing editor state', async () => { + await stateTransfer.navigateToEditor(destinationApp, { state: { originatingApp } }); + expect(stateTransfer.isTransferInProgress).toEqual(true); + currentAppId$.next(destinationApp); + expect(stateTransfer.isTransferInProgress).toEqual(false); + }); + it('can send an outgoing embeddable package state', async () => { await stateTransfer.navigateToWithEmbeddablePackage(destinationApp, { state: { type: 'coolestType', input: { savedObjectId: '150' } }, @@ -135,6 +151,15 @@ describe('embeddable state transfer', () => { }); }); + it('sets isTransferInProgress to true when sending an outgoing embeddable package state', async () => { + await stateTransfer.navigateToWithEmbeddablePackage(destinationApp, { + state: { type: 'coolestType', input: { savedObjectId: '150' } }, + }); + expect(stateTransfer.isTransferInProgress).toEqual(true); + currentAppId$.next(destinationApp); + expect(stateTransfer.isTransferInProgress).toEqual(false); + }); + it('can fetch an incoming editor state', async () => { store.set(EMBEDDABLE_STATE_TRANSFER_STORAGE_KEY, { [EMBEDDABLE_EDITOR_STATE_KEY]: { originatingApp: 'superUltraTestDashboard' }, diff --git a/src/plugins/embeddable/public/lib/state_transfer/embeddable_state_transfer.ts b/src/plugins/embeddable/public/lib/state_transfer/embeddable_state_transfer.ts index 0b34bea81052..92900059668d 100644 --- a/src/plugins/embeddable/public/lib/state_transfer/embeddable_state_transfer.ts +++ b/src/plugins/embeddable/public/lib/state_transfer/embeddable_state_transfer.ts @@ -38,14 +38,20 @@ export const EMBEDDABLE_STATE_TRANSFER_STORAGE_KEY = 'EMBEDDABLE_STATE_TRANSFER' * @public */ export class EmbeddableStateTransfer { + public isTransferInProgress: boolean; private storage: Storage; constructor( private navigateToApp: ApplicationStart['navigateToApp'], + currentAppId$: ApplicationStart['currentAppId$'], private appList?: ReadonlyMap | undefined, customStorage?: Storage ) { this.storage = customStorage ? customStorage : new Storage(sessionStorage); + this.isTransferInProgress = false; + currentAppId$.subscribe(() => { + this.isTransferInProgress = false; + }); } /** @@ -105,6 +111,7 @@ export class EmbeddableStateTransfer { state: EmbeddableEditorState; } ): Promise { + this.isTransferInProgress = true; await this.navigateToWithState(appId, EMBEDDABLE_EDITOR_STATE_KEY, { ...options, appendToExistingState: true, @@ -119,6 +126,7 @@ export class EmbeddableStateTransfer { appId: string, options?: { path?: string; state: EmbeddablePackageState } ): Promise { + this.isTransferInProgress = true; await this.navigateToWithState(appId, EMBEDDABLE_PACKAGE_STATE_KEY, { ...options, appendToExistingState: true, diff --git a/src/plugins/embeddable/public/plugin.tsx b/src/plugins/embeddable/public/plugin.tsx index 5118a1a8818c..6f43d87bdcd5 100644 --- a/src/plugins/embeddable/public/plugin.tsx +++ b/src/plugins/embeddable/public/plugin.tsx @@ -161,6 +161,7 @@ export class EmbeddablePublicPlugin implements Plugin storage - ? new EmbeddableStateTransfer(core.application.navigateToApp, this.appList, storage) + ? new EmbeddableStateTransfer( + core.application.navigateToApp, + core.application.currentAppId$, + this.appList, + storage + ) : this.stateTransferService, EmbeddablePanel: getEmbeddablePanelHoc(), telemetry: getTelemetryFunction(commonContract), diff --git a/src/plugins/embeddable/public/public.api.md b/src/plugins/embeddable/public/public.api.md index 03818fccda0b..7563b66e58ae 100644 --- a/src/plugins/embeddable/public/public.api.md +++ b/src/plugins/embeddable/public/public.api.md @@ -628,12 +628,14 @@ export interface EmbeddableStartDependencies { export class EmbeddableStateTransfer { // Warning: (ae-forgotten-export) The symbol "ApplicationStart" needs to be exported by the entry point index.d.ts // Warning: (ae-forgotten-export) The symbol "PublicAppInfo" needs to be exported by the entry point index.d.ts - constructor(navigateToApp: ApplicationStart['navigateToApp'], appList?: ReadonlyMap | undefined, customStorage?: Storage); + constructor(navigateToApp: ApplicationStart['navigateToApp'], currentAppId$: ApplicationStart['currentAppId$'], appList?: ReadonlyMap | undefined, customStorage?: Storage); // (undocumented) clearEditorState(): void; getAppNameFromId: (appId: string) => string | undefined; getIncomingEditorState(removeAfterFetch?: boolean): EmbeddableEditorState | undefined; getIncomingEmbeddablePackage(removeAfterFetch?: boolean): EmbeddablePackageState | undefined; + // (undocumented) + isTransferInProgress: boolean; // Warning: (ae-unresolved-link) The @link reference could not be resolved: The package "kibana" does not have an export "ApplicationStart" navigateToEditor(appId: string, options?: { path?: string; diff --git a/test/common/services/security/test_user.ts b/test/common/services/security/test_user.ts index 7183943591c8..25a36fed9c9c 100644 --- a/test/common/services/security/test_user.ts +++ b/test/common/services/security/test_user.ts @@ -87,11 +87,11 @@ export async function createTestUserService( }); if (browser && testSubjects && shouldRefreshBrowser) { - // accept alert if it pops up - const alert = await browser.getAlert(); - await alert?.accept(); if (await testSubjects.exists('kibanaChrome', { allowHidden: true })) { await browser.refresh(); + // accept alert if it pops up + const alert = await browser.getAlert(); + await alert?.accept(); await testSubjects.find('kibanaChrome', config.get('timeouts.find') * 10); } } diff --git a/test/functional/apps/dashboard/create_and_add_embeddables.ts b/test/functional/apps/dashboard/create_and_add_embeddables.ts index d8b8a6f91fe3..605ea26b76c6 100644 --- a/test/functional/apps/dashboard/create_and_add_embeddables.ts +++ b/test/functional/apps/dashboard/create_and_add_embeddables.ts @@ -81,7 +81,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { }); it('saves the saved visualization url to the app link', async () => { - await PageObjects.header.clickVisualize(); + await PageObjects.header.clickVisualize(true); const currentUrl = await browser.getCurrentUrl(); expect(currentUrl).to.contain(VisualizeConstants.EDIT_PATH); }); diff --git a/test/functional/apps/dashboard/dashboard_time_picker.ts b/test/functional/apps/dashboard/dashboard_time_picker.ts index 274a4355e26e..c759edd63826 100644 --- a/test/functional/apps/dashboard/dashboard_time_picker.ts +++ b/test/functional/apps/dashboard/dashboard_time_picker.ts @@ -40,6 +40,8 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { after(async () => { await kibanaServer.uiSettings.replace({}); await browser.refresh(); + const alert = await browser.getAlert(); + await alert?.accept(); }); it('Visualization updated when time picker changes', async () => { @@ -88,6 +90,8 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { `)`; log.debug('go to url' + `${kibanaBaseUrl}#${urlQuery}`); await browser.get(`${kibanaBaseUrl}#${urlQuery}`, true); + const alert = await browser.getAlert(); + await alert?.accept(); await PageObjects.header.waitUntilLoadingHasFinished(); const time = await PageObjects.timePicker.getTimeConfig(); const refresh = await PageObjects.timePicker.getRefreshConfig(); @@ -99,6 +103,8 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { it('Timepicker respects dateFormat from UI settings', async () => { await kibanaServer.uiSettings.replace({ dateFormat: 'YYYY-MM-DD HH:mm:ss.SSS' }); await browser.refresh(); + const alert = await browser.getAlert(); + await alert?.accept(); await PageObjects.dashboard.gotoDashboardLandingPage(); await PageObjects.dashboard.clickNewDashboard(); await PageObjects.dashboard.addVisualizations([PIE_CHART_VIS_NAME]); diff --git a/test/functional/apps/dashboard/panel_context_menu.ts b/test/functional/apps/dashboard/panel_context_menu.ts index 0b9e873f4615..bd6756835af3 100644 --- a/test/functional/apps/dashboard/panel_context_menu.ts +++ b/test/functional/apps/dashboard/panel_context_menu.ts @@ -110,7 +110,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { const searchName = 'my search'; before(async () => { - await PageObjects.header.clickDiscover(); + await PageObjects.header.clickDiscover(true); await PageObjects.discover.clickNewSearchButton(); await dashboardVisualizations.createSavedSearch({ name: searchName, fields: ['bytes'] }); await PageObjects.header.waitUntilLoadingHasFinished(); diff --git a/test/functional/apps/dashboard/panel_replacing.ts b/test/functional/apps/dashboard/panel_replacing.ts index 6bf3dbbe47b1..c9ecf42dbc8e 100644 --- a/test/functional/apps/dashboard/panel_replacing.ts +++ b/test/functional/apps/dashboard/panel_replacing.ts @@ -70,6 +70,8 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { it('replaced panel persisted correctly when dashboard is hard refreshed', async () => { const currentUrl = await browser.getCurrentUrl(); await browser.get(currentUrl, true); + const alert = await browser.getAlert(); + await alert?.accept(); await PageObjects.header.waitUntilLoadingHasFinished(); await PageObjects.dashboard.waitForRenderComplete(); const panelTitles = await PageObjects.dashboard.getPanelTitles(); diff --git a/test/functional/page_objects/header_page.ts b/test/functional/page_objects/header_page.ts index d69a01ab6bb2..5a892bb4f6ca 100644 --- a/test/functional/page_objects/header_page.ts +++ b/test/functional/page_objects/header_page.ts @@ -31,14 +31,16 @@ export function HeaderPageProvider({ getService, getPageObjects }: FtrProviderCo const defaultFindTimeout = config.get('timeouts.find'); class HeaderPage { - public async clickDiscover() { + public async clickDiscover(ignoreAppLeaveWarning = false) { await appsMenu.clickLink('Discover', { category: 'kibana' }); + await this.onAppLeaveWarning(ignoreAppLeaveWarning); await PageObjects.common.waitForTopNavToBeVisible(); await this.awaitGlobalLoadingIndicatorHidden(); } - public async clickVisualize() { + public async clickVisualize(ignoreAppLeaveWarning = false) { await appsMenu.clickLink('Visualize', { category: 'kibana' }); + await this.onAppLeaveWarning(ignoreAppLeaveWarning); await this.awaitGlobalLoadingIndicatorHidden(); await retry.waitFor('first breadcrumb to be "Visualize"', async () => { const firstBreadcrumb = await globalNav.getFirstBreadcrumb(); @@ -95,6 +97,17 @@ export function HeaderPageProvider({ getService, getPageObjects }: FtrProviderCo log.debug('awaitKibanaChrome'); await testSubjects.find('kibanaChrome', defaultFindTimeout * 10); } + + public async onAppLeaveWarning(ignoreWarning = false) { + await retry.try(async () => { + const warning = await testSubjects.exists('confirmModalTitleText'); + if (warning) { + await testSubjects.click( + ignoreWarning ? 'confirmModalConfirmButton' : 'confirmModalCancelButton' + ); + } + }); + } } return new HeaderPage(); diff --git a/test/functional/services/dashboard/visualizations.ts b/test/functional/services/dashboard/visualizations.ts index b35ef1e8f2f9..22e1769145f8 100644 --- a/test/functional/services/dashboard/visualizations.ts +++ b/test/functional/services/dashboard/visualizations.ts @@ -58,8 +58,7 @@ export function DashboardVisualizationProvider({ getService, getPageObjects }: F fields?: string[]; }) { log.debug(`createSavedSearch(${name})`); - await PageObjects.header.clickDiscover(); - + await PageObjects.header.clickDiscover(true); await PageObjects.timePicker.setHistoricalDataRange(); if (query) {