[Drilldowns] Dashboard state fixes for drilldowns (#61457)

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 <[email protected]>
elastic · Apr 3, 2020 · ea7a81d · ea7a81d
1 parent aab3dff
commit ea7a81d
Show file tree

Hide file tree

Showing 9 changed files with 195 additions and 39 deletions.
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
@@ -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<QueryState>('_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 = () =>
@@ -652,26 +661,21 @@ 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
         // so have to use implementation of dashboardStateManager.changeDashboardUrl, which workarounds those issues
         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
@@ -59,7 +59,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');
@@ -74,7 +74,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');
@@ -89,7 +89,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
@@ -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
@@ -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
@@ -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
@@ -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&param2=y&param3=z
+ * output: undefined
+ * input: http://localhost:5601/lib/app/kibana#/dashboard/39292992?param1=x&param2=y&param3=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
@@ -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
@@ -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
@@ -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);