From 9cf402a783a3d8f64d4688fb30510af4554567e8 Mon Sep 17 00:00:00 2001 From: Stacey Gammon Date: Fri, 3 Mar 2017 13:19:15 -0500 Subject: [PATCH] Remove unnecessary logic, unapplied query bug fix, code clean up --- .../kibana/public/dashboard/dashboard.js | 31 ++++----- .../public/dashboard/dashboard_constants.js | 2 +- .../public/dashboard/dashboard_state.js | 67 ++++++------------- test/functional/apps/dashboard/_view_edit.js | 33 +++++++++ test/functional/index.js | 3 +- 5 files changed, 69 insertions(+), 67 deletions(-) diff --git a/src/core_plugins/kibana/public/dashboard/dashboard.js b/src/core_plugins/kibana/public/dashboard/dashboard.js index 98c56824814de..a6077383831f3 100644 --- a/src/core_plugins/kibana/public/dashboard/dashboard.js +++ b/src/core_plugins/kibana/public/dashboard/dashboard.js @@ -9,6 +9,8 @@ import 'plugins/kibana/dashboard/panel/panel'; import { DashboardStrings } from './dashboard_strings'; import { DashboardViewMode } from './dashboard_view_mode'; +import { TopNavIds } from './top_nav/top_nav_ids'; +import { ConfirmationButtonTypes } from 'ui/modals/confirm_modal'; import dashboardTemplate from 'plugins/kibana/dashboard/dashboard.html'; import FilterBarQueryFilterProvider from 'ui/filter_bar/query_filter'; import DocTitleProvider from 'ui/doc_title'; @@ -18,8 +20,6 @@ import { VisualizeConstants } from 'plugins/kibana/visualize/visualize_constants import UtilsBrushEventProvider from 'ui/utils/brush_event'; import FilterBarFilterBarClickHandlerProvider from 'ui/filter_bar/filter_bar_click_handler'; import { DashboardState } from './dashboard_state'; -import { TopNavIds } from './top_nav/top_nav_ids'; -import { ConfirmationButtonTypes } from 'ui/modals/confirm_modal'; const app = uiModules.get('app/dashboard', [ 'elasticsearch', @@ -62,7 +62,7 @@ app.directive('dashboardApp', function (Notifier, courier, AppState, timefilter, restrict: 'E', controllerAs: 'dashboardApp', controller: function ($scope, $rootScope, $route, $routeParams, $location, Private, getAppState) { - const queryFilter = Private(FilterBarQueryFilterProvider); + const filterBar = Private(FilterBarQueryFilterProvider); const docTitle = Private(DocTitleProvider); const notify = new Notifier({ location: 'Dashboard' }); @@ -73,6 +73,8 @@ app.directive('dashboardApp', function (Notifier, courier, AppState, timefilter, const dashboardState = new DashboardState(dash, AppState); + // The 'previouslyStored' check is so we only update the time filter on dashboard open, not during + // normal cross app navigation. if (dashboardState.getIsTimeSavedWithDashboard() && !getAppState.previouslyStored()) { dashboardState.syncTimefilterWithDashboard(timefilter, quickRanges); } @@ -85,7 +87,7 @@ app.directive('dashboardApp', function (Notifier, courier, AppState, timefilter, this.appStatus.dirty = status.dirty || !dash.id; }); - dashboardState.updateFilters(queryFilter); + dashboardState.applyFilters(dashboardState.getQuery(), filterBar.getFilters()); let pendingVisCount = _.size(dashboardState.getPanels()); timefilter.enabled = true; @@ -130,8 +132,7 @@ app.directive('dashboardApp', function (Notifier, courier, AppState, timefilter, }; $scope.filterResults = function () { - dashboardState.setQuery($scope.model.query); - dashboardState.updateFilters(queryFilter); + dashboardState.applyFilters($scope.model.query, filterBar.getFilters()); $scope.refresh(); }; @@ -161,7 +162,6 @@ app.directive('dashboardApp', function (Notifier, courier, AppState, timefilter, $scope.onPanelRemoved = (panelIndex) => dashboardState.removePanel(panelIndex); - $scope.$watch('model.query', () => dashboardState.setQuery($scope.model.query)); $scope.$watch('model.darkTheme', () => { dashboardState.setDarkTheme($scope.model.darkTheme); updateTheme(); @@ -171,12 +171,8 @@ app.directive('dashboardApp', function (Notifier, courier, AppState, timefilter, $scope.$listen(timefilter, 'fetch', $scope.refresh); - // Defined up here, but filled in below, to avoid 'Defined before use' warning due to circular reference: - // changeViewMode calls updateViewMode, and navActions uses changeViewMode. - const navActions = {}; - function updateViewMode(newMode) { - $scope.topNavMenu = getTopNavConfig(newMode, navActions); + $scope.topNavMenu = getTopNavConfig(newMode, navActions); // eslint-disable-line no-use-before-define dashboardState.switchViewMode(newMode); $scope.dashboardViewMode = newMode; } @@ -217,9 +213,12 @@ app.directive('dashboardApp', function (Notifier, courier, AppState, timefilter, ); }; + const navActions = {}; navActions[TopNavIds.EXIT_EDIT_MODE] = () => onChangeViewMode(DashboardViewMode.VIEW); navActions[TopNavIds.ENTER_EDIT_MODE] = () => onChangeViewMode(DashboardViewMode.EDIT); + updateViewMode(dashboardState.getViewMode()); + $scope.save = function () { return dashboardState.saveDashboard(angular.toJson, timefilter).then(function (id) { $scope.kbnTopNav.close('save'); @@ -238,12 +237,12 @@ app.directive('dashboardApp', function (Notifier, courier, AppState, timefilter, }; // update root source when filters update - $scope.$listen(queryFilter, 'update', function () { - dashboardState.updateFilters(queryFilter); + $scope.$listen(filterBar, 'update', function () { + dashboardState.applyFilters($scope.model.query, filterBar.getFilters()); }); // update data when filters fire fetch event - $scope.$listen(queryFilter, 'fetch', $scope.refresh); + $scope.$listen(filterBar, 'fetch', $scope.refresh); $scope.$on('$destroy', () => { dashboardState.destroy(); @@ -265,7 +264,6 @@ app.directive('dashboardApp', function (Notifier, courier, AppState, timefilter, chrome.removeApplicationClass(['theme-dark']); chrome.addApplicationClass('theme-light'); } - updateViewMode(dashboardState.getViewMode()); $scope.$on('ready:vis', function () { if (pendingVisCount > 0) pendingVisCount--; @@ -286,7 +284,6 @@ app.directive('dashboardApp', function (Notifier, courier, AppState, timefilter, `${VisualizeConstants.WIZARD_STEP_1_PAGE_PATH}?${DashboardConstants.ADD_VISUALIZATION_TO_DASHBOARD_MODE_PARAM}`); }; - $scope.opts = { displayName: dash.getDisplayName(), dashboard: dash, diff --git a/src/core_plugins/kibana/public/dashboard/dashboard_constants.js b/src/core_plugins/kibana/public/dashboard/dashboard_constants.js index f90ff7bf92346..f74c7050440e6 100644 --- a/src/core_plugins/kibana/public/dashboard/dashboard_constants.js +++ b/src/core_plugins/kibana/public/dashboard/dashboard_constants.js @@ -4,5 +4,5 @@ export const DashboardConstants = { NEW_VISUALIZATION_ID_PARAM: 'addVisualization', LANDING_PAGE_PATH: '/dashboard', CREATE_NEW_DASHBOARD_URL: '/dashboard/create', - EXISTING_DASHBOARD_URL: '/dashboard/{{id}}', + EXISTING_DASHBOARD_URL: '/dashboard/{{id}}' }; diff --git a/src/core_plugins/kibana/public/dashboard/dashboard_state.js b/src/core_plugins/kibana/public/dashboard/dashboard_state.js index 9ab76fa93942f..720a8c39355cb 100644 --- a/src/core_plugins/kibana/public/dashboard/dashboard_state.js +++ b/src/core_plugins/kibana/public/dashboard/dashboard_state.js @@ -23,6 +23,12 @@ function getStateDefaults(dashboard) { }; } +/** + * Depending on how a dashboard is loaded, the filter object may contain a $$hashKey and $state that will throw + * off a filter comparison. This removes those variables. + * @param filters {Array.} + * @returns {Array.} + */ function cleanFiltersForComparison(filters) { return _.map(filters, (filter) => _.omit(filter, ['$$hashKey', '$state'])); } @@ -58,16 +64,12 @@ export class DashboardState { this.uiState = this.appState.makeStateful('uiState'); this.isDirty = false; + // We can't compare the filters stored on this.appState to this.dashboard because in order to apply + // the filters to the visualizations, we need to save it on the dashboard. We keep track of the original + // filter state in order to let the user know if their filters changed and provide this specific information + //in the 'lose changes' warning message. this.lastSavedDashboardFilters = this.getFilterState(); - // Unfortunately there is a hashkey stored with the filters on the appState, but not always - // in the dashboard searchSource. On a page refresh with a filter, this will cause the state - // monitor to count them as different even if they aren't. Hence this is a bit of a hack to get around - // that. TODO: Improve state monitor factory to take custom comparison functions for certain paths. - if (!this.getFilterBarChanged()) { - this.stateDefaults.filters = this.appState.filters; - } - PanelUtils.initPanelIndexes(this.getPanels()); this.createStateMonitor(); } @@ -91,7 +93,6 @@ export class DashboardState { setTitle(title) { this.appState.title = title; - this.dashboard.title = title; this.saveState(); } @@ -186,19 +187,15 @@ export class DashboardState { return this.getViewMode() === DashboardViewMode.EDIT; } - /** * * @returns {boolean} True if the dashboard has changed since the last save (or, is new). */ getIsDirty(timeFilter) { - const existingTitleChanged = - this.dashboard.lastSavedTitle && - this.dashboard.lastSavedTitle !== this.dashboard.title; - return this.isDirty || - this.getFiltersChanged(timeFilter) || // Not all filter changes are tracked by the state monitor. - existingTitleChanged; + // Filter bar comparison is done manually (see cleanFiltersForComparison for the reason) and time picker + // changes are not tracked by the state monitor. + this.getFiltersChanged(timeFilter); } getPanels() { @@ -272,10 +269,6 @@ export class DashboardState { } } - setQuery(newQuery) { - this.appState.query = newQuery; - } - saveState() { this.appState.save(); } @@ -286,6 +279,7 @@ export class DashboardState { * @param toJson {function} A custom toJson function. Used because the previous code used * the angularized toJson version, and it was unclear whether there was a reason not to use * JSON.stringify + * @param timefilter * @returns {Promise} A promise that if resolved, will contain the id of the newly saved * dashboard. */ @@ -315,11 +309,11 @@ export class DashboardState { } /** - * Stores the given filter with the dashboard and to the state. - * @param filter + * Applies the current filter state to the dashboard. + * @param filter {Array.} An array of filter bar filters. */ - updateFilters(filter) { - const filters = filter.getFilters(); + applyFilters(query, filters) { + this.appState.query = query; if (this.appState.query) { this.dashboard.searchSource.set('filter', _.union(filters, [{ query: this.appState.query @@ -335,7 +329,8 @@ export class DashboardState { this.stateMonitor = stateMonitorFactory.create(this.appState, this.stateDefaults); this.stateMonitor.ignoreProps('viewMode'); - this.stateMonitor.ignoreProps('filters'); // Filters need some tweaking to compare correctly. + // Filters need to be compared manually because they sometimes have a $$hashkey stored on the object. + this.stateMonitor.ignoreProps('filters'); this.stateMonitor.onChange(status => { this.isDirty = status.dirty; @@ -360,26 +355,4 @@ export class DashboardState { const options = this.dashboard.id ? { id: this.dashboard.id } : {}; return { url, options }; } - - refreshStateMonitor() { - if (this.stateMonitor) { - this.stateMonitor.destroy(); - } - this.createStateMonitor(); - } - - reloadLastSavedFilters() { - // Need to make a copy to ensure nothing overwrites the originals. - this.stateDefaults.filters = this.appState.filters = Object.assign(new Array(), this.getLastSavedFilterBars()); - this.stateDefaults.query = this.appState.query = Object.assign({}, this.getLastSavedQuery()); - - if (this.dashboard.timeRestore) { - this.timefilter.time.from = this.lastSavedDashboardFilters.timeFrom; - this.timefilter.time.to = this.lastSavedDashboardFilters.timeTo; - } - - this.refreshStateMonitor(); - this.isDirty = false; - this.saveState(); - } } diff --git a/test/functional/apps/dashboard/_view_edit.js b/test/functional/apps/dashboard/_view_edit.js index 3c0327e40c68f..9a3339d9f69eb 100644 --- a/test/functional/apps/dashboard/_view_edit.js +++ b/test/functional/apps/dashboard/_view_edit.js @@ -93,6 +93,22 @@ bdd.describe('dashboard view edit mode', function viewEditModeTests() { expect(newFromTime).to.equal(originalFromTime); expect(newToTime).to.equal(originalToTime); }); + + bdd.it('when the query is edited and applied', async function () { + await PageObjects.dashboard.clickEdit(); + + const originalQuery = await PageObjects.dashboard.getQuery(); + await PageObjects.dashboard.appendQuery('extra stuff'); + await PageObjects.dashboard.clickFilterButton(); + + await PageObjects.dashboard.clickCancelOutOfEditMode(); + + // confirm lose changes + await PageObjects.common.clickConfirmOnModal(); + + const query = await PageObjects.dashboard.getQuery(); + expect(query).to.equal(originalQuery); + }); }); bdd.describe('and preserves edits on cancel', function () { @@ -143,5 +159,22 @@ bdd.describe('dashboard view edit mode', function viewEditModeTests() { const isOpen = await PageObjects.common.isConfirmModalOpen(); expect(isOpen).to.be(false); }); + + // See https://github.com/elastic/kibana/issues/10110 - this is intentional. + bdd.it('when the query is edited but not applied', async function () { + await PageObjects.dashboard.clickEdit(); + + const originalQuery = await PageObjects.dashboard.getQuery(); + await PageObjects.dashboard.appendQuery('extra stuff'); + + await PageObjects.dashboard.clickCancelOutOfEditMode(); + + const isOpen = await PageObjects.common.isConfirmModalOpen(); + expect(isOpen).to.be(false); + + await PageObjects.dashboard.loadSavedDashboard(dashboardName); + const query = await PageObjects.dashboard.getQuery(); + expect(query).to.equal(originalQuery); + }); }); }); diff --git a/test/functional/index.js b/test/functional/index.js index 42331cde79a75..4b067580a087f 100644 --- a/test/functional/index.js +++ b/test/functional/index.js @@ -36,8 +36,7 @@ define(function (require) { 'intern/dojo/node!./apps/console', 'intern/dojo/node!./apps/dashboard', 'intern/dojo/node!./status_page', - 'intern/dojo/node!./apps/context', - 'intern/dojo/node!./status_page' + 'intern/dojo/node!./apps/context' ].filter((suite) => { if (!requestedApps) return true; return requestedApps.reduce((previous, app) => {