diff --git a/src/legacy/core_plugins/data/public/filter/filter_manager/filter_manager.test.ts b/src/legacy/core_plugins/data/public/filter/filter_manager/filter_manager.test.ts index d27b0eab0e34a..8a42f409bb47b 100644 --- a/src/legacy/core_plugins/data/public/filter/filter_manager/filter_manager.test.ts +++ b/src/legacy/core_plugins/data/public/filter/filter_manager/filter_manager.test.ts @@ -217,6 +217,30 @@ describe('filter_manager', () => { expect(updateListener.called).toBeTruthy(); expect(updateListener.callCount).toBe(2); }); + + test('changing a disabled filter should fire only update event', async function() { + const updateStub = jest.fn(); + const fetchStub = jest.fn(); + const f1 = getFilter(FilterStateStore.GLOBAL_STATE, true, false, 'age', 34); + + await filterManager.setFilters([f1]); + + filterManager.getUpdates$().subscribe({ + next: updateStub, + }); + + filterManager.getFetches$().subscribe({ + next: fetchStub, + }); + + const f2 = _.cloneDeep(f1); + f2.meta.negate = true; + await filterManager.setFilters([f2]); + + // this time, events should be emitted + expect(fetchStub).toBeCalledTimes(0); + expect(updateStub).toBeCalledTimes(1); + }); }); describe('add filters', () => { diff --git a/src/legacy/core_plugins/data/public/filter/filter_manager/filter_manager.ts b/src/legacy/core_plugins/data/public/filter/filter_manager/filter_manager.ts index 6c472ab76f9c0..ccb26c801c701 100644 --- a/src/legacy/core_plugins/data/public/filter/filter_manager/filter_manager.ts +++ b/src/legacy/core_plugins/data/public/filter/filter_manager/filter_manager.ts @@ -35,6 +35,8 @@ import { extractTimeFilter } from './lib/extract_time_filter'; // @ts-ignore import { changeTimeFilter } from './lib/change_time_filter'; +import { onlyDisabledFiltersChanged } from './lib/only_disabled'; + import { PartitionedFilters } from './partitioned_filters'; import { IndexPatterns } from '../../index_patterns'; @@ -92,13 +94,14 @@ export class FilterManager { }); const filtersUpdated = !_.isEqual(this.filters, newFilters); + const updatedOnlyDisabledFilters = onlyDisabledFiltersChanged(newFilters, this.filters); this.filters = newFilters; if (filtersUpdated) { this.updated$.next(); - // Fired together with updated$, because historically (~4 years ago) there was a fetch optimization, that didn't call fetch for very specific cases. - // This optimization seems irrelevant at the moment, but I didn't want to change the logic of all consumers. - this.fetch$.next(); + if (!updatedOnlyDisabledFilters) { + this.fetch$.next(); + } } } diff --git a/src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.test.ts b/src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.test.ts index b413efc0ba0f5..7ccdc6237e901 100644 --- a/src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.test.ts +++ b/src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.test.ts @@ -20,8 +20,6 @@ import sinon from 'sinon'; import { FilterStateStore } from '@kbn/es-query'; - -import { Subscription } from 'rxjs'; import { FilterStateManager } from './filter_state_manager'; import { StubState } from './test_helpers/stub_state'; @@ -50,7 +48,6 @@ describe('filter_state_manager', () => { let appStateStub: StubState; let globalStateStub: StubState; - let subscription: Subscription | undefined; let filterManager: FilterManager; beforeEach(() => { @@ -60,12 +57,6 @@ describe('filter_state_manager', () => { filterManager = new FilterManager(indexPatterns); }); - afterEach(() => { - if (subscription) { - subscription.unsubscribe(); - } - }); - describe('app_state_undefined', () => { beforeEach(() => { // FilterStateManager is tested indirectly. @@ -164,4 +155,25 @@ describe('filter_state_manager', () => { sinon.assert.calledOnce(globalStateStub.save); }); }); + + describe('bug fixes', () => { + /* + ** This test is here to reproduce a bug where a filter manager update + ** would cause filter state manager detects those changes + ** And triggers *another* filter manager update. + */ + test('should NOT re-trigger filter manager', async done => { + const f1 = getFilter(FilterStateStore.APP_STATE, false, false, 'age', 34); + filterManager.setFilters([f1]); + const setFiltersSpy = sinon.spy(filterManager, 'setFilters'); + + f1.meta.negate = true; + await filterManager.setFilters([f1]); + + setTimeout(() => { + expect(setFiltersSpy.callCount).toEqual(1); + done(); + }, 100); + }); + }); }); diff --git a/src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.ts b/src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.ts index 032233cbc0a8b..06f91e35db96e 100644 --- a/src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.ts +++ b/src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.ts @@ -17,7 +17,7 @@ * under the License. */ -import { Filter, FilterStateStore } from '@kbn/es-query'; +import { FilterStateStore } from '@kbn/es-query'; import _ from 'lodash'; import { State } from 'ui/state_management/state'; @@ -34,8 +34,6 @@ export class FilterStateManager { filterManager: FilterManager; globalState: State; getAppState: GetAppStateFunc; - prevGlobalFilters: Filter[] | undefined; - prevAppFilters: Filter[] | undefined; interval: NodeJS.Timeout | undefined; constructor(globalState: State, getAppState: GetAppStateFunc, filterManager: FilterManager) { @@ -67,10 +65,8 @@ export class FilterStateManager { const globalFilters = this.globalState.filters || []; const appFilters = (appState && appState.filters) || []; - const globalFilterChanged = !( - this.prevGlobalFilters && _.isEqual(this.prevGlobalFilters, globalFilters) - ); - const appFilterChanged = !(this.prevAppFilters && _.isEqual(this.prevAppFilters, appFilters)); + const globalFilterChanged = !_.isEqual(this.filterManager.getGlobalFilters(), globalFilters); + const appFilterChanged = !_.isEqual(this.filterManager.getAppFilters(), appFilters); const filterStateChanged = globalFilterChanged || appFilterChanged; if (!filterStateChanged) return; @@ -81,10 +77,6 @@ export class FilterStateManager { FilterManager.setFiltersStore(newGlobalFilters, FilterStateStore.GLOBAL_STATE); this.filterManager.setFilters(newGlobalFilters.concat(newAppFilters)); - - // store new filter changes - this.prevGlobalFilters = newGlobalFilters; - this.prevAppFilters = newAppFilters; }, 10); } diff --git a/src/legacy/core_plugins/data/public/filter/filter_manager/index.ts b/src/legacy/core_plugins/data/public/filter/filter_manager/index.ts index 0e37e1ed4dec8..dc70ae910835d 100644 --- a/src/legacy/core_plugins/data/public/filter/filter_manager/index.ts +++ b/src/legacy/core_plugins/data/public/filter/filter_manager/index.ts @@ -22,3 +22,4 @@ export { FilterStateManager } from './filter_state_manager'; // @ts-ignore export { uniqFilters } from './lib/uniq_filters'; +export { onlyDisabledFiltersChanged } from './lib/only_disabled'; diff --git a/src/legacy/core_plugins/data/public/filter/filter_manager/lib/__tests__/only_disabled.js b/src/legacy/core_plugins/data/public/filter/filter_manager/lib/only_disabled.test.ts similarity index 51% rename from src/legacy/core_plugins/data/public/filter/filter_manager/lib/__tests__/only_disabled.js rename to src/legacy/core_plugins/data/public/filter/filter_manager/lib/only_disabled.test.ts index a6f4c74a70bab..52dd27e9fc7ad 100644 --- a/src/legacy/core_plugins/data/public/filter/filter_manager/lib/__tests__/only_disabled.js +++ b/src/legacy/core_plugins/data/public/filter/filter_manager/lib/only_disabled.test.ts @@ -17,114 +17,111 @@ * under the License. */ -import { onlyDisabled } from '../only_disabled'; -import expect from '@kbn/expect'; +import { Filter } from '@kbn/es-query'; -describe('Filter Bar Directive', function () { - describe('onlyDisabled()', function () { +import { onlyDisabledFiltersChanged } from './only_disabled'; +import expect from '@kbn/expect'; - it('should return true if all filters are disabled', function () { +describe('Filter Bar Directive', function() { + describe('onlyDisabledFiltersChanged()', function() { + it('should return true if all filters are disabled', function() { const filters = [ { meta: { disabled: true } }, { meta: { disabled: true } }, - { meta: { disabled: true } } - ]; - const newFilters = [{ meta: { disabled: true } }]; - expect(onlyDisabled(newFilters, filters)).to.be(true); + { meta: { disabled: true } }, + ] as Filter[]; + const newFilters = [{ meta: { disabled: true } }] as Filter[]; + expect(onlyDisabledFiltersChanged(newFilters, filters)).to.be(true); }); - it('should return false if all filters are not disabled', function () { + it('should return false if all filters are not disabled', function() { const filters = [ { meta: { disabled: false } }, { meta: { disabled: false } }, - { meta: { disabled: false } } - ]; - const newFilters = [{ meta: { disabled: false } }]; - expect(onlyDisabled(newFilters, filters)).to.be(false); + { meta: { disabled: false } }, + ] as Filter[]; + const newFilters = [{ meta: { disabled: false } }] as Filter[]; + expect(onlyDisabledFiltersChanged(newFilters, filters)).to.be(false); }); - it('should return false if only old filters are disabled', function () { + it('should return false if only old filters are disabled', function() { const filters = [ { meta: { disabled: true } }, { meta: { disabled: true } }, - { meta: { disabled: true } } - ]; - const newFilters = [{ meta: { disabled: false } }]; - expect(onlyDisabled(newFilters, filters)).to.be(false); + { meta: { disabled: true } }, + ] as Filter[]; + const newFilters = [{ meta: { disabled: false } }] as Filter[]; + expect(onlyDisabledFiltersChanged(newFilters, filters)).to.be(false); }); - it('should return false if new filters are not disabled', function () { + it('should return false if new filters are not disabled', function() { const filters = [ { meta: { disabled: false } }, { meta: { disabled: false } }, - { meta: { disabled: false } } - ]; - const newFilters = [{ meta: { disabled: true } }]; - expect(onlyDisabled(newFilters, filters)).to.be(false); + { meta: { disabled: false } }, + ] as Filter[]; + const newFilters = [{ meta: { disabled: true } }] as Filter[]; + expect(onlyDisabledFiltersChanged(newFilters, filters)).to.be(false); }); - it('should return true when all removed filters were disabled', function () { + it('should return true when all removed filters were disabled', function() { const filters = [ { meta: { disabled: true } }, { meta: { disabled: true } }, - { meta: { disabled: true } } - ]; - const newFilters = []; - expect(onlyDisabled(newFilters, filters)).to.be(true); + { meta: { disabled: true } }, + ] as Filter[]; + const newFilters = [] as Filter[]; + expect(onlyDisabledFiltersChanged(newFilters, filters)).to.be(true); }); - it('should return false when all removed filters were not disabled', function () { + it('should return false when all removed filters were not disabled', function() { const filters = [ { meta: { disabled: false } }, { meta: { disabled: false } }, - { meta: { disabled: false } } - ]; - const newFilters = []; - expect(onlyDisabled(newFilters, filters)).to.be(false); + { meta: { disabled: false } }, + ] as Filter[]; + const newFilters = [] as Filter[]; + expect(onlyDisabledFiltersChanged(newFilters, filters)).to.be(false); }); - it('should return true if all changed filters are disabled', function () { + it('should return true if all changed filters are disabled', function() { const filters = [ { meta: { disabled: true, negate: false } }, - { meta: { disabled: true, negate: false } } - ]; + { meta: { disabled: true, negate: false } }, + ] as Filter[]; const newFilters = [ { meta: { disabled: true, negate: true } }, - { meta: { disabled: true, negate: true } } - ]; - expect(onlyDisabled(newFilters, filters)).to.be(true); + { meta: { disabled: true, negate: true } }, + ] as Filter[]; + expect(onlyDisabledFiltersChanged(newFilters, filters)).to.be(true); }); - it('should return false if all filters remove were not disabled', function () { + it('should return false if all filters remove were not disabled', function() { const filters = [ { meta: { disabled: false } }, { meta: { disabled: false } }, - { meta: { disabled: true } } - ]; - const newFilters = [{ meta: { disabled: false } }]; - expect(onlyDisabled(newFilters, filters)).to.be(false); + { meta: { disabled: true } }, + ] as Filter[]; + const newFilters = [{ meta: { disabled: false } }] as Filter[]; + expect(onlyDisabledFiltersChanged(newFilters, filters)).to.be(false); }); - it('should return false when all removed filters are not disabled', function () { + it('should return false when all removed filters are not disabled', function() { const filters = [ { meta: { disabled: true } }, { meta: { disabled: false } }, - { meta: { disabled: true } } - ]; - const newFilters = []; - expect(onlyDisabled(newFilters, filters)).to.be(false); + { meta: { disabled: true } }, + ] as Filter[]; + const newFilters = [] as Filter[]; + expect(onlyDisabledFiltersChanged(newFilters, filters)).to.be(false); }); - it('should not throw with null filters', function () { - const filters = [ - null, - { meta: { disabled: true } } - ]; - const newFilters = []; - expect(function () { - onlyDisabled(newFilters, filters); + it('should not throw with null filters', function() { + const filters = [null, { meta: { disabled: true } }] as Filter[]; + const newFilters = [] as Filter[]; + expect(function() { + onlyDisabledFiltersChanged(newFilters, filters); }).to.not.throwError(); }); - }); }); diff --git a/src/legacy/core_plugins/data/public/filter/filter_manager/lib/only_disabled.js b/src/legacy/core_plugins/data/public/filter/filter_manager/lib/only_disabled.ts similarity index 66% rename from src/legacy/core_plugins/data/public/filter/filter_manager/lib/only_disabled.js rename to src/legacy/core_plugins/data/public/filter/filter_manager/lib/only_disabled.ts index b78fde4e28209..6df761c3db09f 100644 --- a/src/legacy/core_plugins/data/public/filter/filter_manager/lib/only_disabled.js +++ b/src/legacy/core_plugins/data/public/filter/filter_manager/lib/only_disabled.ts @@ -18,17 +18,19 @@ */ import _ from 'lodash'; +import { Filter } from '@kbn/es-query'; -const pluckDisabled = function (filter) { - return _.get(filter, 'meta.disabled'); +const isEnabled = function(filter: Filter) { + return filter && filter.meta && !filter.meta.disabled; }; - /** * Checks to see if only disabled filters have been changed * @returns {bool} Only disabled filters */ -export function onlyDisabled(newFilters, oldFilters) { - return _.every(newFilters.concat(oldFilters), function (newFilter) { - return pluckDisabled(newFilter); - }); +export function onlyDisabledFiltersChanged(newFilters: Filter[], oldFilters: Filter[]) { + // If it's the same - compare only enabled filters + const newEnabledFilters = _.filter(newFilters, isEnabled); + const oldEnabledFilters = _.filter(oldFilters, isEnabled); + + return _.isEqual(oldEnabledFilters, newEnabledFilters); } diff --git a/src/legacy/core_plugins/data/public/filter/filter_manager/lib/only_state_changed.js b/src/legacy/core_plugins/data/public/filter/filter_manager/lib/only_state_changed.js deleted file mode 100644 index 73e35bfec07ea..0000000000000 --- a/src/legacy/core_plugins/data/public/filter/filter_manager/lib/only_state_changed.js +++ /dev/null @@ -1,35 +0,0 @@ -/* - * 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 _ from 'lodash'; -import { compareFilters } from './compare_filters'; -const compareOptions = { disabled: true, negate: true }; - -/** - * Checks to see if only disabled filters have been changed - * @returns {bool} Only disabled filters - */ -export function onlyStateChanged(newFilters, oldFilters) { - return _.every(newFilters, function (newFilter) { - const match = _.find(oldFilters, function (oldFilter) { - return compareFilters(newFilter, oldFilter, compareOptions); - }); - return !!match; - }); -} diff --git a/src/legacy/core_plugins/data/public/index.ts b/src/legacy/core_plugins/data/public/index.ts index 9cce64b0c5741..b592bc7a33f73 100644 --- a/src/legacy/core_plugins/data/public/index.ts +++ b/src/legacy/core_plugins/data/public/index.ts @@ -85,7 +85,12 @@ export { ExpressionRenderer, ExpressionRendererProps, ExpressionRunner } from '. export { IndexPattern, StaticIndexPattern, StaticIndexPatternField, Field } from './index_patterns'; export { Query, QueryBar } from './query'; export { FilterBar } from './filter'; -export { FilterManager, FilterStateManager, uniqFilters } from './filter/filter_manager'; +export { + FilterManager, + FilterStateManager, + uniqFilters, + onlyDisabledFiltersChanged, +} from './filter/filter_manager'; /** @public static code */ export { dateHistogramInterval } from '../common/date_histogram_interval';