Skip to content

Commit

Permalink
Remove unnecessary logic, unapplied query bug fix, code clean up
Browse files Browse the repository at this point in the history
  • Loading branch information
@stacey-gammon
stacey-gammon committed Mar 23, 2017
1 parent 774267c commit 9cf402a
Show file tree
Hide file tree
Showing 5 changed files with 69 additions and 67 deletions.
31 changes: 14 additions & 17 deletions src/core_plugins/kibana/public/dashboard/dashboard.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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',
Expand Down Expand Up @@ -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' });

Expand All @@ -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);
}
Expand All @@ -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;
Expand Down Expand Up @@ -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();
};

Expand Down Expand Up @@ -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();
Expand All @@ -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;
}
Expand Down Expand Up @@ -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');
Expand All @@ -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();
Expand All @@ -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--;
Expand All @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion src/core_plugins/kibana/public/dashboard/dashboard_constants.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}}'
};
67 changes: 20 additions & 47 deletions src/core_plugins/kibana/public/dashboard/dashboard_state.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.<Object>}
* @returns {Array.<Object>}
*/
function cleanFiltersForComparison(filters) {
return _.map(filters, (filter) => _.omit(filter, ['$$hashKey', '$state']));
}
Expand Down Expand Up @@ -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();
}
Expand All @@ -91,7 +93,6 @@ export class DashboardState {

setTitle(title) {
this.appState.title = title;
this.dashboard.title = title;
this.saveState();
}

Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -272,10 +269,6 @@ export class DashboardState {
}
}

setQuery(newQuery) {
this.appState.query = newQuery;
}

saveState() {
this.appState.save();
}
Expand All @@ -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<string>} A promise that if resolved, will contain the id of the newly saved
* dashboard.
*/
Expand Down Expand Up @@ -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.<Object>} 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
Expand All @@ -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;
Expand All @@ -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();
}
}
33 changes: 33 additions & 0 deletions test/functional/apps/dashboard/_view_edit.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand Down Expand Up @@ -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);
});
});
});
3 changes: 1 addition & 2 deletions test/functional/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down

0 comments on commit 9cf402a

Please sign in to comment.