Filter manager improvements \ bug fixes (#41999) (#42105)

* Filter manager improvements

 - Avoid firing update \ fetch events twice from filter manager, by making filter state manager use getFilters rathre than storing its own previous filters.
- Don't fire fetch event when only disabled filters were changed
- Replaced onlyDisabledChanged implementation with a more readable one (karma tests pass!)
- Exported onlyDisabledChanged from data
- Deleted unused only_state_changed

* Delete empty afterEach

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', () => {

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();
+      }
     }
   }
 

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);
+    });
+  });
 });

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);
   }
 

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';

src/legacy/core_plugins/data/public/filter/filter_manager/lib/__tests__/only_disabled.js → 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();
     });
-
   });
 });

src/legacy/core_plugins/data/public/filter/filter_manager/lib/only_disabled.js → 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);
 }