From 233e309343fcb891ed18a5d0e8d73ccf8c93e4be Mon Sep 17 00:00:00 2001 From: Anton Dosov Date: Fri, 3 Apr 2020 14:52:16 +0200 Subject: [PATCH] [Drilldowns] Dashboard state fixes for drilldowns (#61457) (#62421) 1. Change logic around deciding wether to use time from url or from saved object. Previously code looked only into if _g is present in the url at all. And didn't consider edge case if time or refreshInterval is missing in _g 2. Fix initial syncing of time from savedobject causing redundant history record. _This changed caused order of _a and g params in url change. One test was affected by it because it relied on the order. I don't think it should be considered breaking as order app puts it's query params shouldn't matter. 3. Fix another race condition between state syncing with url and angular controller $destroy. Similar fix was done before in #57795, but this on covers case when we stay within dashboard app, but change dashboard 4. Fix initial panel state migration causing redundant browser history records Co-authored-by: Elastic Machine Co-authored-by: Elastic Machine --- .../np_ready/dashboard_app_controller.tsx | 48 ++++++++++--------- .../np_ready/dashboard_state.test.ts | 6 +-- .../np_ready/dashboard_state_manager.ts | 36 +++++++++++++- .../public/dashboard/np_ready/lib/index.ts | 1 + .../public/dashboard/np_ready/lib/url.test.ts | 46 ++++++++++++++++++ .../public/dashboard/np_ready/lib/url.ts | 35 ++++++++++++++ .../apps/dashboard/bwc_shared_urls.js | 21 ++++++++ .../apps/dashboard/dashboard_time.js | 14 ++++++ .../apps/management/_kibana_settings.js | 27 ++++++----- 9 files changed, 195 insertions(+), 39 deletions(-) create mode 100644 src/legacy/core_plugins/kibana/public/dashboard/np_ready/lib/url.test.ts create mode 100644 src/legacy/core_plugins/kibana/public/dashboard/np_ready/lib/url.ts diff --git a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_app_controller.tsx b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_app_controller.tsx index 0c6686c993371..2865f7d669418 100644 --- a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_app_controller.tsx +++ b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_app_controller.tsx @@ -36,6 +36,7 @@ import { IndexPattern, IndexPatternsContract, Query, + QueryState, SavedQuery, syncQueryStateWithUrl, } from '../../../../../../plugins/data/public'; @@ -132,13 +133,6 @@ export class DashboardAppController { const queryFilter = filterManager; const timefilter = queryService.timefilter.timefilter; - // starts syncing `_g` portion of url with query services - // note: dashboard_state_manager.ts syncs `_a` portion of url - const { - stop: stopSyncingQueryServiceStateWithUrl, - hasInheritedQueryFromUrl: hasInheritedGlobalStateFromUrl, - } = syncQueryStateWithUrl(queryService, kbnUrlStateStorage); - let lastReloadRequestTime = 0; const dash = ($scope.dash = $route.current.locals.dash); if (dash.id) { @@ -170,9 +164,24 @@ export class DashboardAppController { // The hash check is so we only update the time filter on dashboard open, not during // normal cross app navigation. - if (dashboardStateManager.getIsTimeSavedWithDashboard() && !hasInheritedGlobalStateFromUrl) { - dashboardStateManager.syncTimefilterWithDashboard(timefilter); + if (dashboardStateManager.getIsTimeSavedWithDashboard()) { + const initialGlobalStateInUrl = kbnUrlStateStorage.get('_g'); + if (!initialGlobalStateInUrl?.time) { + dashboardStateManager.syncTimefilterWithDashboardTime(timefilter); + } + if (!initialGlobalStateInUrl?.refreshInterval) { + dashboardStateManager.syncTimefilterWithDashboardRefreshInterval(timefilter); + } } + + // starts syncing `_g` portion of url with query services + // note: dashboard_state_manager.ts syncs `_a` portion of url + // it is important to start this syncing after `dashboardStateManager.syncTimefilterWithDashboard(timefilter);` above is run, + // otherwise it will case redundant browser history record + const { stop: stopSyncingQueryServiceStateWithUrl } = syncQueryStateWithUrl( + queryService, + kbnUrlStateStorage + ); $scope.showSaveQuery = dashboardCapabilities.saveQuery as boolean; const getShouldShowEditHelp = () => @@ -643,6 +652,14 @@ export class DashboardAppController { // This is only necessary for new dashboards, which will default to Edit mode. updateViewMode(ViewMode.VIEW); + // We need to do a hard reset of the timepicker. appState will not reload like + // it does on 'open' because it's been saved to the url and the getAppState.previouslyStored() check on + // reload will cause it not to sync. + if (dashboardStateManager.getIsTimeSavedWithDashboard()) { + dashboardStateManager.syncTimefilterWithDashboardTime(timefilter); + dashboardStateManager.syncTimefilterWithDashboardRefreshInterval(timefilter); + } + // Angular's $location skips this update because of history updates from syncState which happen simultaneously // when calling kbnUrl.change() angular schedules url update and when angular finally starts to process it, // the update is considered outdated and angular skips it @@ -650,19 +667,6 @@ export class DashboardAppController { dashboardStateManager.changeDashboardUrl( dash.id ? createDashboardEditUrl(dash.id) : DashboardConstants.CREATE_NEW_DASHBOARD_URL ); - - // We need to do a hard reset of the timepicker. appState will not reload like - // it does on 'open' because it's been saved to the url and the getAppState.previouslyStored() check on - // reload will cause it not to sync. - if (dashboardStateManager.getIsTimeSavedWithDashboard()) { - // have to use $evalAsync here until '_g' is migrated from $location to state sync utility ('history') - // When state sync utility changes url, angular's $location is missing it's own updates which happen during the same digest cycle - // temporary solution is to delay $location updates to next digest cycle - // unfortunately, these causes 2 browser history entries, but this is temporary and will be fixed after migrating '_g' to state_sync utilities - $scope.$evalAsync(() => { - dashboardStateManager.syncTimefilterWithDashboard(timefilter); - }); - } } overlays diff --git a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_state.test.ts b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_state.test.ts index 815e4975b9784..3ef5492086e79 100644 --- a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_state.test.ts +++ b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_state.test.ts @@ -63,7 +63,7 @@ describe('DashboardState', function() { mockTime.to = '2015-09-29 06:31:44.000'; initDashboardState(); - dashboardState.syncTimefilterWithDashboard(mockTimefilter); + dashboardState.syncTimefilterWithDashboardTime(mockTimefilter); expect(mockTime.to).toBe('now/w'); expect(mockTime.from).toBe('now/w'); @@ -78,7 +78,7 @@ describe('DashboardState', function() { mockTime.to = '2015-09-29 06:31:44.000'; initDashboardState(); - dashboardState.syncTimefilterWithDashboard(mockTimefilter); + dashboardState.syncTimefilterWithDashboardTime(mockTimefilter); expect(mockTime.to).toBe('now'); expect(mockTime.from).toBe('now-13d'); @@ -93,7 +93,7 @@ describe('DashboardState', function() { mockTime.to = 'now/w'; initDashboardState(); - dashboardState.syncTimefilterWithDashboard(mockTimefilter); + dashboardState.syncTimefilterWithDashboardTime(mockTimefilter); expect(mockTime.to).toBe(savedDashboard.timeTo); expect(mockTime.from).toBe(savedDashboard.timeFrom); diff --git a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_state_manager.ts b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_state_manager.ts index 171f08b45cf8d..9b8f75bdcf953 100644 --- a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_state_manager.ts +++ b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_state_manager.ts @@ -35,7 +35,7 @@ import { TimefilterContract as Timefilter, } from '../../../../../../plugins/data/public'; -import { getAppStateDefaults, migrateAppState } from './lib'; +import { getAppStateDefaults, migrateAppState, getDashboardIdFromUrl } from './lib'; import { convertPanelStateToSavedDashboardPanel } from './lib/embeddable_saved_object_converters'; import { FilterUtils } from './lib/filter_utils'; import { @@ -175,6 +175,14 @@ export class DashboardStateManager { // sync state required state container to be able to handle null // overriding set() so it could handle null coming from url if (state) { + // Skip this update if current dashboardId in the url is different from what we have in the current instance of state manager + // As dashboard is driven by angular at the moment, the destroy cycle happens async, + // If the dashboardId has changed it means this instance + // is going to be destroyed soon and we shouldn't sync state anymore, + // as it could potentially trigger further url updates + const currentDashboardIdInUrl = getDashboardIdFromUrl(history.location.pathname); + if (currentDashboardIdInUrl !== this.savedDashboard.id) return; + this.stateContainer.set({ ...this.stateDefaults, ...state, @@ -203,6 +211,7 @@ export class DashboardStateManager { public handleDashboardContainerChanges(dashboardContainer: DashboardContainer) { let dirty = false; + let dirtyBecauseOfInitialStateMigration = false; const savedDashboardPanelMap: { [key: string]: SavedDashboardPanel } = {}; @@ -236,11 +245,20 @@ export class DashboardStateManager { ) { // A panel was changed dirty = true; + + const oldVersion = savedDashboardPanelMap[panelState.explicitInput.id]?.version; + const newVersion = convertedPanelStateMap[panelState.explicitInput.id]?.version; + if (oldVersion && newVersion && oldVersion !== newVersion) { + dirtyBecauseOfInitialStateMigration = true; + } } }); if (dirty) { this.stateContainer.transitions.set('panels', Object.values(convertedPanelStateMap)); + if (dirtyBecauseOfInitialStateMigration) { + this.saveState({ replace: true }); + } } if (input.isFullScreenMode !== this.getFullScreenMode()) { @@ -498,7 +516,7 @@ export class DashboardStateManager { * @param timeFilter.setTime * @param timeFilter.setRefreshInterval */ - public syncTimefilterWithDashboard(timeFilter: Timefilter) { + public syncTimefilterWithDashboardTime(timeFilter: Timefilter) { if (!this.getIsTimeSavedWithDashboard()) { throw new Error( i18n.translate('kbn.dashboard.stateManager.timeNotSavedWithDashboardErrorMessage', { @@ -513,6 +531,20 @@ export class DashboardStateManager { to: this.savedDashboard.timeTo, }); } + } + + /** + * Updates timeFilter to match the refreshInterval saved with the dashboard. + * @param timeFilter + */ + public syncTimefilterWithDashboardRefreshInterval(timeFilter: Timefilter) { + if (!this.getIsTimeSavedWithDashboard()) { + throw new Error( + i18n.translate('kbn.dashboard.stateManager.timeNotSavedWithDashboardErrorMessage', { + defaultMessage: 'The time is not saved with this dashboard so should not be synced.', + }) + ); + } if (this.savedDashboard.refreshInterval) { timeFilter.setRefreshInterval(this.savedDashboard.refreshInterval); diff --git a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/lib/index.ts b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/lib/index.ts index b4c9e939d3083..e9ebe73c3b34d 100644 --- a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/lib/index.ts +++ b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/lib/index.ts @@ -20,3 +20,4 @@ export { saveDashboard } from './save_dashboard'; export { getAppStateDefaults } from './get_app_state_defaults'; export { migrateAppState } from './migrate_app_state'; +export { getDashboardIdFromUrl } from './url'; diff --git a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/lib/url.test.ts b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/lib/url.test.ts new file mode 100644 index 0000000000000..70a9d86206fd6 --- /dev/null +++ b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/lib/url.test.ts @@ -0,0 +1,46 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { getDashboardIdFromUrl } from './url'; + +test('getDashboardIdFromUrl', () => { + let url = + "http://localhost:5601/wev/app/kibana#/dashboard?_g=(refreshInterval:(pause:!t,value:0),time:(from:now-15m,to:now))&_a=(description:'',filters:!()"; + expect(getDashboardIdFromUrl(url)).toEqual(undefined); + + url = + "http://localhost:5601/wev/app/kibana#/dashboard/625357282?_a=(description:'',filters:!()&_g=(refreshInterval:(pause:!t,value:0),time:(from:now-15m,to:now))"; + expect(getDashboardIdFromUrl(url)).toEqual('625357282'); + + url = 'http://myserver.mydomain.com:5601/wev/app/kibana#/dashboard/777182'; + expect(getDashboardIdFromUrl(url)).toEqual('777182'); + + url = + "http://localhost:5601/app/kibana#/dashboard?_g=(refreshInterval:(pause:!t,value:0),time:(from:now-15m,to:now))&_a=(description:'',filters:!()"; + expect(getDashboardIdFromUrl(url)).toEqual(undefined); + + url = '/dashboard/test/?_g=(refreshInterval:'; + expect(getDashboardIdFromUrl(url)).toEqual('test'); + + url = 'dashboard/test/?_g=(refreshInterval:'; + expect(getDashboardIdFromUrl(url)).toEqual('test'); + + url = '/other-app/test/'; + expect(getDashboardIdFromUrl(url)).toEqual(undefined); +}); diff --git a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/lib/url.ts b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/lib/url.ts new file mode 100644 index 0000000000000..2489867fa6233 --- /dev/null +++ b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/lib/url.ts @@ -0,0 +1,35 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/** + * Returns dashboard id from URL + * literally looks from id after `dashboard/` string and before `/`, `?` and end of string + * @param url to extract dashboardId from + * input: http://localhost:5601/lib/app/kibana#/dashboard?param1=x¶m2=y¶m3=z + * output: undefined + * input: http://localhost:5601/lib/app/kibana#/dashboard/39292992?param1=x¶m2=y¶m3=z + * output: 39292992 + */ +export function getDashboardIdFromUrl(url: string): string | undefined { + const [, dashboardId] = url.match(/dashboard\/(.*?)(\/|\?|$)/) ?? [ + undefined, // full match + undefined, // group with dashboardId + ]; + return dashboardId ?? undefined; +} diff --git a/test/functional/apps/dashboard/bwc_shared_urls.js b/test/functional/apps/dashboard/bwc_shared_urls.js index fb1e580135e5a..b56cb658b80bb 100644 --- a/test/functional/apps/dashboard/bwc_shared_urls.js +++ b/test/functional/apps/dashboard/bwc_shared_urls.js @@ -135,6 +135,27 @@ export default function({ getService, getPageObjects }) { await dashboardExpect.selectedLegendColorCount('#000000', 5); }); + + it('back button works for old dashboards after state migrations', async () => { + await PageObjects.dashboard.preserveCrossAppState(); + const oldId = await PageObjects.dashboard.getDashboardIdFromCurrentUrl(); + await PageObjects.dashboard.waitForRenderComplete(); + await dashboardExpect.selectedLegendColorCount('#000000', 5); + + const url = `${kibanaBaseUrl}#/dashboard?${urlQuery}`; + log.debug(`Navigating to ${url}`); + await browser.get(url); + await PageObjects.header.waitUntilLoadingHasFinished(); + await PageObjects.dashboard.waitForRenderComplete(); + await dashboardExpect.selectedLegendColorCount('#F9D9F9', 5); + await browser.goBack(); + + await PageObjects.header.waitUntilLoadingHasFinished(); + const newId = await PageObjects.dashboard.getDashboardIdFromCurrentUrl(); + expect(newId).to.be.equal(oldId); + await PageObjects.dashboard.waitForRenderComplete(); + await dashboardExpect.selectedLegendColorCount('#000000', 5); + }); }); }); } diff --git a/test/functional/apps/dashboard/dashboard_time.js b/test/functional/apps/dashboard/dashboard_time.js index 2e7b7f9a6dbb1..5a2628f42ded5 100644 --- a/test/functional/apps/dashboard/dashboard_time.js +++ b/test/functional/apps/dashboard/dashboard_time.js @@ -91,6 +91,20 @@ export default function({ getPageObjects, getService }) { expect(time.start).to.equal('~ an hour ago'); expect(time.end).to.equal('now'); }); + + it('should use saved time, if time is missing in global state, but _g is present in the url', async function() { + const currentUrl = await browser.getCurrentUrl(); + const kibanaBaseUrl = currentUrl.substring(0, currentUrl.indexOf('#')); + const id = await PageObjects.dashboard.getDashboardIdFromCurrentUrl(); + + await PageObjects.dashboard.gotoDashboardLandingPage(); + + const urlWithGlobalTime = `${kibanaBaseUrl}#/dashboard/${id}?_g=(filters:!())`; + await browser.get(urlWithGlobalTime, false); + const time = await PageObjects.timePicker.getTimeConfig(); + expect(time.start).to.equal(PageObjects.timePicker.defaultStartTime); + expect(time.end).to.equal(PageObjects.timePicker.defaultEndTime); + }); }); // If the user has time stored with a dashboard, it's supposed to override the current time settings diff --git a/test/functional/apps/management/_kibana_settings.js b/test/functional/apps/management/_kibana_settings.js index c99368ba4e859..97337d4573e2a 100644 --- a/test/functional/apps/management/_kibana_settings.js +++ b/test/functional/apps/management/_kibana_settings.js @@ -46,6 +46,18 @@ export default function({ getService, getPageObjects }) { }); describe('state:storeInSessionStorage', () => { + async function getStateFromUrl() { + const currentUrl = await browser.getCurrentUrl(); + let match = currentUrl.match(/(.*)?_g=(.*)&_a=(.*)/); + if (match) return [match[2], match[3]]; + match = currentUrl.match(/(.*)?_a=(.*)&_g=(.*)/); + if (match) return [match[3], match[2]]; + + if (!match) { + throw new Error('State in url is missing or malformed'); + } + } + it('defaults to null', async () => { await PageObjects.settings.clickKibanaSettings(); const storeInSessionStorage = await PageObjects.settings.getAdvancedSettingCheckbox( @@ -58,10 +70,7 @@ export default function({ getService, getPageObjects }) { await PageObjects.common.navigateToApp('dashboard'); await PageObjects.dashboard.clickNewDashboard(); await PageObjects.timePicker.setDefaultAbsoluteRange(); - const currentUrl = await browser.getCurrentUrl(); - const urlPieces = currentUrl.match(/(.*)?_g=(.*)&_a=(.*)/); - const globalState = urlPieces[2]; - const appState = urlPieces[3]; + const [globalState, appState] = await getStateFromUrl(); // We don't have to be exact, just need to ensure it's greater than when the hashed variation is being used, // which is less than 20 characters. @@ -83,10 +92,7 @@ export default function({ getService, getPageObjects }) { await PageObjects.common.navigateToApp('dashboard'); await PageObjects.dashboard.clickNewDashboard(); await PageObjects.timePicker.setDefaultAbsoluteRange(); - const currentUrl = await browser.getCurrentUrl(); - const urlPieces = currentUrl.match(/(.*)?_g=(.*)&_a=(.*)/); - const globalState = urlPieces[2]; - const appState = urlPieces[3]; + const [globalState, appState] = await getStateFromUrl(); // We don't have to be exact, just need to ensure it's less than the unhashed version, which will be // greater than 20 characters with the default state plus a time. @@ -100,10 +106,7 @@ export default function({ getService, getPageObjects }) { await PageObjects.settings.clickKibanaSettings(); await PageObjects.settings.toggleAdvancedSettingCheckbox('state:storeInSessionStorage'); await PageObjects.header.clickDashboard(); - const currentUrl = await browser.getCurrentUrl(); - const urlPieces = currentUrl.match(/(.*)?_g=(.*)&_a=(.*)/); - const globalState = urlPieces[2]; - const appState = urlPieces[3]; + const [globalState, appState] = await getStateFromUrl(); // We don't have to be exact, just need to ensure it's greater than when the hashed variation is being used, // which is less than 20 characters. expect(globalState.length).to.be.greaterThan(20);