Skip to content

Commit

Permalink
Bugfix dashboard unpins filters (#62301)
Browse files Browse the repository at this point in the history
Fixes following cases:

Saving dashboard with pinned filter unpins it. Do not save pinned filters with dashboard see #62301 (comment)
When navigating with global filter to dashboard with same saved filter, filter becomes unpinned
When navigating from listing to dashboard with saved filter, back button didn't work

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
  • Loading branch information
Dosant and elasticmachine authored Apr 12, 2020
1 parent 2be6b7f commit 1199c8c
Show file tree
Hide file tree
Showing 9 changed files with 85 additions and 29 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -143,14 +143,19 @@ export class DashboardAppController {
});

// sync initial app filters from state to filterManager
// if there is an existing similar global filter, then leave it as global
filterManager.setAppFilters(_.cloneDeep(dashboardStateManager.appState.filters));
// setup syncing of app filters between appState and filterManager
const stopSyncingAppFilters = connectToQueryState(
queryService,
{
set: ({ filters }) => dashboardStateManager.setFilters(filters || []),
get: () => ({ filters: dashboardStateManager.appState.filters }),
state$: dashboardStateManager.appState$.pipe(map(state => ({ filters: state.filters }))),
state$: dashboardStateManager.appState$.pipe(
map(state => ({
filters: state.filters,
}))
),
},
{
filters: esFilters.FilterStateStore.APP_STATE,
Expand All @@ -170,13 +175,16 @@ export class DashboardAppController {
}

// 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
// otherwise it will case redundant browser history records
const { stop: stopSyncingQueryServiceStateWithUrl } = syncQueryStateWithUrl(
queryService,
kbnUrlStateStorage
);

// starts syncing `_a` portion of url
dashboardStateManager.startStateSyncing();

$scope.showSaveQuery = dashboardCapabilities.saveQuery as boolean;

const getShouldShowEditHelp = () =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,9 +164,6 @@ export class DashboardStateManager {
this.changeListeners.forEach(listener => listener({ dirty: this.isDirty }));
});

// make sure url ('_a') matches initial state
this.kbnUrlStateStorage.set(this.STATE_STORAGE_KEY, initialState, { replace: true });

// setup state syncing utils. state container will be synced with url into `this.STATE_STORAGE_KEY` query param
this.stateSyncRef = syncState<DashboardAppState>({
storageKey: this.STATE_STORAGE_KEY,
Expand Down Expand Up @@ -201,8 +198,10 @@ export class DashboardStateManager {
},
stateStorage: this.kbnUrlStateStorage,
});
}

// actually start syncing state with container
public startStateSyncing() {
this.saveState({ replace: true });
this.stateSyncRef.start();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

import _ from 'lodash';
import moment, { Moment } from 'moment';
import { Filter } from 'src/plugins/data/public';
import { Filter } from '../../../../data/public';

/**
* @typedef {Object} QueryFilter
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import { RefreshInterval, TimefilterContract } from 'src/plugins/data/public';
import { FilterUtils } from './filter_utils';
import { SavedObjectDashboard } from '../../saved_dashboards';
import { DashboardAppState } from '../../types';
import { esFilters } from '../../../../data/public';

export function updateSavedDashboard(
savedDashboard: SavedObjectDashboard,
Expand All @@ -48,4 +49,10 @@ export function updateSavedDashboard(
'value',
]);
savedDashboard.refreshInterval = savedDashboard.timeRestore ? timeRestoreObj : undefined;

// save only unpinned filters
const unpinnedFilters = savedDashboard
.getFilters()
.filter(filter => !esFilters.isFilterPinned(filter));
savedDashboard.searchSource.setField('filter', unpinnedFilters);
}
Original file line number Diff line number Diff line change
Expand Up @@ -204,18 +204,19 @@ describe('filter_manager', () => {
).toBe(3);
});

test('should set app filters and remove any duplicated global filters', async function() {
filterManager.addFilters(readyFilters, true);
test('should set app filters and merge them with duplicate global filters', async function() {
const [filter, ...otherFilters] = readyFilters;
filterManager.addFilters(otherFilters, true);
const appFilter1 = _.cloneDeep(readyFilters[1]);
const appFilter2 = _.cloneDeep(readyFilters[2]);

filterManager.setAppFilters([appFilter1, appFilter2]);
filterManager.setAppFilters([filter, appFilter1, appFilter2]);

const newGlobalFilters = filterManager.getGlobalFilters();
const newAppFilters = filterManager.getAppFilters();

expect(newGlobalFilters).toHaveLength(1);
expect(newAppFilters).toHaveLength(2);
expect(newGlobalFilters).toHaveLength(2);
expect(newAppFilters).toHaveLength(1);
});

test('should set global filters and remove any duplicated app filters', async function() {
Expand Down
17 changes: 4 additions & 13 deletions src/plugins/data/public/query/filter_manager/filter_manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -177,13 +177,9 @@ export class FilterManager {
public setGlobalFilters(newGlobalFilters: Filter[]) {
newGlobalFilters = mapAndFlattenFilters(newGlobalFilters);
FilterManager.setFiltersStore(newGlobalFilters, FilterStateStore.GLOBAL_STATE, true);
const { appFilters: currentAppFilters } = this.getPartitionedFilters();
// remove duplicates from current app filters, to make sure global will take precedence
const filteredAppFilters = currentAppFilters.filter(
appFilter => !newGlobalFilters.find(globalFilter => compareFilters(globalFilter, appFilter))
);
const { appFilters } = this.getPartitionedFilters();
const newFilters = this.mergeIncomingFilters({
appFilters: filteredAppFilters,
appFilters,
globalFilters: newGlobalFilters,
});

Expand All @@ -198,14 +194,9 @@ export class FilterManager {
public setAppFilters(newAppFilters: Filter[]) {
newAppFilters = mapAndFlattenFilters(newAppFilters);
FilterManager.setFiltersStore(newAppFilters, FilterStateStore.APP_STATE, true);
const { globalFilters: currentGlobalFilters } = this.getPartitionedFilters();
// remove duplicates from current global filters, to make sure app will take precedence
const filteredGlobalFilters = currentGlobalFilters.filter(
globalFilter => !newAppFilters.find(appFilter => compareFilters(appFilter, globalFilter))
);

const { globalFilters } = this.getPartitionedFilters();
const newFilters = this.mergeIncomingFilters({
globalFilters: filteredGlobalFilters,
globalFilters,
appFilters: newAppFilters,
});
this.handleStateUpdate(newFilters);
Expand Down
3 changes: 2 additions & 1 deletion src/plugins/data/public/ui/filter_bar/filter_item.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ class FilterItemUI extends Component<Props, State> {
const dataTestSubjDisabled = `filter-${
this.props.filter.meta.disabled ? 'disabled' : 'enabled'
}`;
const dataTestSubjPinned = `filter-${isFilterPinned(filter) ? 'pinned' : 'unpinned'}`;

const classes = classNames(
'globalFilterItem',
Expand All @@ -107,7 +108,7 @@ class FilterItemUI extends Component<Props, State> {
className={classes}
iconOnClick={() => this.props.onRemove()}
onClick={this.handleBadgeClick}
data-test-subj={`filter ${dataTestSubjDisabled} ${dataTestSubjKey} ${dataTestSubjValue}`}
data-test-subj={`filter ${dataTestSubjDisabled} ${dataTestSubjKey} ${dataTestSubjValue} ${dataTestSubjPinned}`}
/>
);

Expand Down
38 changes: 38 additions & 0 deletions test/functional/apps/dashboard/dashboard_filter_bar.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export default function({ getService, getPageObjects }) {
const pieChart = getService('pieChart');
const esArchiver = getService('esArchiver');
const kibanaServer = getService('kibanaServer');
const browser = getService('browser');
const PageObjects = getPageObjects(['common', 'dashboard', 'header', 'visualize', 'timePicker']);

describe('dashboard filter bar', () => {
Expand Down Expand Up @@ -126,9 +127,46 @@ export default function({ getService, getPageObjects }) {

const filterCount = await filterBar.getFilterCount();
expect(filterCount).to.equal(1);
await pieChart.expectPieSliceCount(1);
});

it("restoring filters doesn't break back button", async () => {
await browser.goBack();
await PageObjects.dashboard.expectExistsDashboardLandingPage();
await browser.goForward();
await PageObjects.header.waitUntilLoadingHasFinished();
await PageObjects.dashboard.waitForRenderComplete();
await pieChart.expectPieSliceCount(1);
});

it("saving with pinned filter doesn't unpin them", async () => {
const filterKey = 'bytes';
await filterBar.toggleFilterPinned(filterKey);
await PageObjects.dashboard.switchToEditMode();
await PageObjects.dashboard.saveDashboard('saved with pinned filters', {
saveAsNew: true,
});
expect(await filterBar.isFilterPinned(filterKey)).to.be(true);
await pieChart.expectPieSliceCount(1);
});

it("navigating to a dashboard with global filter doesn't unpin it if same filter is saved with dashboard", async () => {
await PageObjects.dashboard.preserveCrossAppState();
await PageObjects.dashboard.gotoDashboardLandingPage();
await PageObjects.dashboard.loadSavedDashboard('with filters');
await PageObjects.header.waitUntilLoadingHasFinished();
expect(await filterBar.isFilterPinned('bytes')).to.be(true);
await pieChart.expectPieSliceCount(1);
});

it("pinned filters aren't saved", async () => {
await filterBar.removeFilter('bytes');
await PageObjects.dashboard.gotoDashboardLandingPage();
await PageObjects.dashboard.loadSavedDashboard('saved with pinned filters');
await PageObjects.header.waitUntilLoadingHasFinished();
expect(await filterBar.getFilterCount()).to.be(0);
await pieChart.expectPieSliceCount(5);
});
});

describe('saved search filtering', function() {
Expand Down
15 changes: 13 additions & 2 deletions test/functional/services/filter_bar.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,16 @@ export function FilterBarProvider({ getService, getPageObjects }: FtrProviderCon
* @param value filter value
* @param enabled filter status
*/
public async hasFilter(key: string, value: string, enabled: boolean = true): Promise<boolean> {
public async hasFilter(
key: string,
value: string,
enabled: boolean = true,
pinned: boolean = false
): Promise<boolean> {
const filterActivationState = enabled ? 'enabled' : 'disabled';
const filterPinnedState = pinned ? 'pinned' : 'unpinned';
return testSubjects.exists(
`filter filter-${filterActivationState} filter-key-${key} filter-value-${value}`,
`filter filter-${filterActivationState} filter-key-${key} filter-value-${value} filter-${filterPinnedState}`,
{
allowHidden: true,
}
Expand Down Expand Up @@ -80,6 +86,11 @@ export function FilterBarProvider({ getService, getPageObjects }: FtrProviderCon
await PageObjects.header.awaitGlobalLoadingIndicatorHidden();
}

public async isFilterPinned(key: string): Promise<boolean> {
const filter = await testSubjects.find(`~filter & ~filter-key-${key}`);
return (await filter.getAttribute('data-test-subj')).includes('filter-pinned');
}

public async getFilterCount(): Promise<number> {
const filters = await testSubjects.findAll('~filter');
return filters.length;
Expand Down

0 comments on commit 1199c8c

Please sign in to comment.