Query Filter \ Filter Manager: de-angularize and move to data plugin #37311

lizozom · 2019-05-29T07:36:35Z

Important: Much of the code was merged accidentally by another PR

Summary

This PR a full rewrite of query_filter into the data/filter/filter_manager new platform service.
This was done in such a way that the API is fully backwards compatible.

Previously, initializing queryFilter was done this way:

// Replace this import 
import { FilterBarQueryFilterProvider } from 'ui/filter_manager/query_filter';
const queryFilter = Private(FilterBarQueryFilterProvider);
queryFilter.addFilters(...);

While this is still supported, you can also use the FilterManager instance directly:

import { FilterStateManager } from 'plugins/data';
import { data } from 'plugins/data/setup';
const filterManager = data.filter.filterManager;

// You have to hook the filter manager instance into your app's state.
// FilterStateManager is a helper that hooks it into the existing global \ app scope)
const filterStateManager = new FilterStateManager(globalState, getAppState, filterManager);

filterManager.addFilters(...);

This PR also includes a very simple de-angularization of FilterManagerProvider.
It is now used in the following manner:

const filterGen = getFilterGenerator(queryFilter);

But it should be merged with FilterManager.

Notes and comments

The code still relies on Private caching to instantiate a single instance of FilterManager per app.
The es-query Filter type definition changed, and $state is now optional. This is due to the fact that some places create filters without state. FilterManager however, makes sure that all filters have state.
globalState, getAppState and indexPatterns are still angular injected dependencies.
FilterStateManager is just a convenience wrapper around app+global states, and might be removed when a new state manager is introduced. It uses setTimeout instead of rootScope to watch state variables. It can be changed, but everything works this way as well.
In FilterManager, there was a very old optimization that didn't re-fetch the data, when specific filter updates occurred:

!onlyDisabled(nextVal, prevVal) && !onlyStateChanged(newFilters, oldFilters)

I went over this with @spalger and this optimization seems irrelevant. I removed it and for now we just fire fetch and update events at the same time (to avoid changing subscribers).

In query_filter there several methods that were not being used by anything, but were there for completeness (like toggleAll and disableAll for example). I chose not to implement them for the time being, but they may be implemented again if needed.
I had to add a try-catch clause to ui/errors due to a new error that started to happen once I started loading the filter mappers from .ts files. If any one has suggestions about this error, I'd love to hear!

TypeError: Cannot redefine property: prototype

Full jest test coverage for FilterManager and FilterStateManager was added, and older query_filter tests were updated.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
Documentation was added for features that require explanation or tutorials
Unit or functional tests were update`d or added to match the most common scenarios

…ckage

…plugin/filter-manager-1

elasticmachine · 2019-05-29T08:33:36Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…ependency on rootScope

…plugin/filter-manager-1

elasticmachine · 2019-05-30T14:44:37Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…plugin/filter-manager-1

elasticmachine · 2019-06-03T08:03:35Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…plugin/filter-manager-1

…is now just a wrapper.

elasticmachine · 2019-06-05T09:27:06Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-05T16:25:33Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-05T17:41:11Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…r now!

… well

elasticmachine · 2019-06-06T11:18:44Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…nager

lizozom · 2019-06-27T14:13:00Z

retest

…plugin/filter-manager-1

lizozom · 2019-06-27T14:32:00Z

retest

elasticmachine · 2019-06-27T14:53:14Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

lizozom · 2019-06-27T14:53:23Z

retest

stacey-gammon

Looks good! I pulled down and tested quite a bit, using input controls with pinned filters by default and not pinned filters, a combination of unpinned and pinned filters created by visualizations and also by filtering on maps and saved searches (found an existing bug - #39802).

All seems to be working as expected! All my comments are minor.

...acy/core_plugins/data/public/filter/filter_bar/filter_editor/lib/filter_editor_utils.test.ts

src/legacy/core_plugins/data/public/filter/filter_manager/filter_manager.test.ts

stacey-gammon · 2019-06-26T17:40:20Z

src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.ts

+    this.globalState = globalState;
+    this.filterManager = filterManager;
+
+    this.watchFilterState();


What do you think about requiring users to directly call watchFilterState? Just thinking that it might be clearer to keep the side affects out of the constructor? I had a similar comment on a recent PR of mine, in DashboardStateManager, that the constructor has side affects. It did read a little funny in the test file that you need to instantiate the class, but then do nothing with it.

src/legacy/core_plugins/data/public/filter/filter_manager/filter_manager.ts

stacey-gammon · 2019-06-27T14:50:05Z

src/legacy/core_plugins/data/public/filter/filter_manager/filter_manager.ts

+      }
+    );
+
+    this.filters = newFilters;


Maybe move inside the if statement since if they aren't updated, no reason to set them?

src/legacy/core_plugins/data/public/filter/filter_manager/filter_manager.ts

src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.ts

…er_state_manager.ts Co-Authored-By: Stacey Gammon <gammon@elastic.co>

…er_manager.ts Co-Authored-By: Stacey Gammon <gammon@elastic.co>

elasticmachine · 2019-06-27T16:06:19Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-27T17:00:51Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

lizozom · 2019-06-30T08:30:12Z

retest

elasticmachine · 2019-06-30T09:31:20Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…plugin/filter-manager-1

…:lizozom/kibana into newplatform/data-plugin/filter-manager-1

elasticmachine · 2019-06-30T11:03:34Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-30T13:45:12Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

stacey-gammon

Here's my official LGTM! ... though I did not pull down and test the latest minor changes.

…lastic#37311) * Embeddable and Visualize to use type definitions from kbn-es-query package * moved state watching to new filter manager and removed query filter dependency on rootScope * Deleted unused pinFilter * Extracted merge fitlers function * Changed mappedFilters name * Move state management to new filter management as well. query filter is now just a wrapper. * Code works correctly with new setup of query filter * improved typing * Moved mapping into new filter manager * removed promise dependency from query filter * Extracted filter state manager from query filter * Moved addFiltersAndChangeTimeFilter logic into new filter manager * Fixed addFiltersAndChangeTimeFilter implementation * Moved all logic to new filter manager - query filter is just a wrapper now! * Connected query_filter observable management to new filter manager as well * Usage example in discover * filter state management tests + fire less events from filter state manager * Tests for filter manager and filter state manager * Moved new filter manager to data plugin * Got rid of FilterManagerProvider and turned it into a getFilterGenerator function that works with the new FilterManager. Need to merge this code with FilterManager. * Added tests that make sure that filter manager listens to filter state manager * fix typo * Fixed action add filter test * Fixed increasePredecessorCount test * Fixed increaseSuccessorCount test * Fixed setPredecessorCount test * Fixed setQueryParameters test * Fixed setSuccessorCount test * Fixed doc table filter actions test * Fixed filter generator tests * rename argument * Moved push filters into vis filters (only place where its used) * Filter type fix, test fix * Removed irrelevant filter tests (for deleted methods) Added state to filters * Fixed most of get filters errors by sleeping (TODO!) * Added destroy methods Improved merge logic Improved remove filter tests to use add filter (to avoid having for filter state manager to catch up) * Fixed discover functional tests Added default empty filter state in filter generator Some more browser test adjustments * Allow filter $state to be empty * Fixed types * Code review with @splager - subscribeWithScope - query filter to return angular promises - remove unneeded Promise.resolve * Separate class\type defenitions from plugin instance setup in shim plugin definition This helps avoiding circular dependency issues that were obsereved in filter-manager branch (due to code starting to use the data plugin). * typescript fun * Fixed merge * updated some get filter tests to work with add filter, and therefore moved them to add_filters.js * Remove shouldFetch and add comment explaning the observable. * Minor code style fixes * Code review fixes and changes * Moved Karma tests to jest * Fixed merge * Fixed some type errors * Improved watchFilterState logic * Removed addFiltersAndChangeTimeFilter test for now, as I'm having TS difficulties * filters can also be null * Further watch state fine tuning * Get data instance inside provider to avoid circular deps * mock chrome * Removed change to range filter * Deleted unnecessary injects * dedup setFilters as well * deleted unused subscription * Added tests for setFilters de-dupe * Update src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.ts Co-Authored-By: Stacey Gammon <gammon@elastic.co> * Update src/legacy/core_plugins/data/public/filter/filter_manager/filter_manager.ts Co-Authored-By: Stacey Gammon <gammon@elastic.co> * Code review minor fixes

…37311) (#40012) * Embeddable and Visualize to use type definitions from kbn-es-query package * moved state watching to new filter manager and removed query filter dependency on rootScope * Deleted unused pinFilter * Extracted merge fitlers function * Changed mappedFilters name * Move state management to new filter management as well. query filter is now just a wrapper. * Code works correctly with new setup of query filter * improved typing * Moved mapping into new filter manager * removed promise dependency from query filter * Extracted filter state manager from query filter * Moved addFiltersAndChangeTimeFilter logic into new filter manager * Fixed addFiltersAndChangeTimeFilter implementation * Moved all logic to new filter manager - query filter is just a wrapper now! * Connected query_filter observable management to new filter manager as well * Usage example in discover * filter state management tests + fire less events from filter state manager * Tests for filter manager and filter state manager * Moved new filter manager to data plugin * Got rid of FilterManagerProvider and turned it into a getFilterGenerator function that works with the new FilterManager. Need to merge this code with FilterManager. * Added tests that make sure that filter manager listens to filter state manager * fix typo * Fixed action add filter test * Fixed increasePredecessorCount test * Fixed increaseSuccessorCount test * Fixed setPredecessorCount test * Fixed setQueryParameters test * Fixed setSuccessorCount test * Fixed doc table filter actions test * Fixed filter generator tests * rename argument * Moved push filters into vis filters (only place where its used) * Filter type fix, test fix * Removed irrelevant filter tests (for deleted methods) Added state to filters * Fixed most of get filters errors by sleeping (TODO!) * Added destroy methods Improved merge logic Improved remove filter tests to use add filter (to avoid having for filter state manager to catch up) * Fixed discover functional tests Added default empty filter state in filter generator Some more browser test adjustments * Allow filter $state to be empty * Fixed types * Code review with @splager - subscribeWithScope - query filter to return angular promises - remove unneeded Promise.resolve * Separate class\type defenitions from plugin instance setup in shim plugin definition This helps avoiding circular dependency issues that were obsereved in filter-manager branch (due to code starting to use the data plugin). * typescript fun * Fixed merge * updated some get filter tests to work with add filter, and therefore moved them to add_filters.js * Remove shouldFetch and add comment explaning the observable. * Minor code style fixes * Code review fixes and changes * Moved Karma tests to jest * Fixed merge * Fixed some type errors * Improved watchFilterState logic * Removed addFiltersAndChangeTimeFilter test for now, as I'm having TS difficulties * filters can also be null * Further watch state fine tuning * Get data instance inside provider to avoid circular deps * mock chrome * Removed change to range filter * Deleted unnecessary injects * dedup setFilters as well * deleted unused subscription * Added tests for setFilters de-dupe * Update src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.ts Co-Authored-By: Stacey Gammon <gammon@elastic.co> * Update src/legacy/core_plugins/data/public/filter/filter_manager/filter_manager.ts Co-Authored-By: Stacey Gammon <gammon@elastic.co> * Code review minor fixes

… plugin (#37311)" This reverts commit 531e2b0.

Liza Katz added 2 commits May 29, 2019 10:34

Embeddable and Visualize to use type definitions from kbn-es-query pa…

fb72077

…ckage

Merge remote-tracking branch 'upstream/master' into newplatform/data-…

33e3684

…plugin/filter-manager-1

Liza Katz added 5 commits May 29, 2019 18:52

moved state watching to new filter manager and removed query filter d…

f243294

…ependency on rootScope

Merge remote-tracking branch 'upstream/master' into newplatform/data-…

4b70360

…plugin/filter-manager-1

Deleted unused pinFilter

fe60536

Extracted merge fitlers function

6231d85

Changed mappedFilters name

cff30fc

Merge remote-tracking branch 'upstream/master' into newplatform/data-…

ec4ae2f

…plugin/filter-manager-1

Liza Katz added 2 commits June 4, 2019 13:58

Merge remote-tracking branch 'upstream/master' into newplatform/data-…

ebf7dbd

…plugin/filter-manager-1

Move state management to new filter management as well. query filter …

e87524e

…is now just a wrapper.

Liza Katz added 2 commits June 5, 2019 16:34

Code works correctly with new setup of query filter

6cde2a7

improved typing

08ef0cd

lukeelmers mentioned this pull request Jun 5, 2019

App Architecture roadmap for New Platform migration #31968

Closed

Liza Katz added 2 commits June 5, 2019 17:58

Moved mapping into new filter manager

a26489f

removed promise dependency from query filter

1178338

Liza Katz added 2 commits June 5, 2019 19:49

Extracted filter state manager from query filter

9f0c490

Moved addFiltersAndChangeTimeFilter logic into new filter manager

313edcc

stacey-gammon mentioned this pull request Jun 5, 2019

Typescript dashboard app code. #37527

Merged

Liza Katz added 4 commits June 6, 2019 10:14

Fixed addFiltersAndChangeTimeFilter implementation

348461a

Moved all logic to new filter manager - query filter is just a wrappe…

efc85d9

…r now!

Connected query_filter observable management to new filter manager as…

ced4d16

… well

Usage example in discover

f1e142e

filter state management tests + fire less events from filter state ma…

4bb875b

…nager

Merge remote-tracking branch 'upstream/master' into newplatform/data-…

dabaa1b

…plugin/filter-manager-1

This comment has been minimized.

Sign in to view

stacey-gammon reviewed Jun 27, 2019

View reviewed changes

lizozom and others added 2 commits June 27, 2019 18:53

Update src/legacy/core_plugins/data/public/filter/filter_manager/filt…

496d976

…er_state_manager.ts Co-Authored-By: Stacey Gammon <gammon@elastic.co>

Update src/legacy/core_plugins/data/public/filter/filter_manager/filt…

b43256f

…er_manager.ts Co-Authored-By: Stacey Gammon <gammon@elastic.co>

stacey-gammon mentioned this pull request Jun 27, 2019

More dashboard migrations #39387

Merged

Liza Katz added 2 commits June 30, 2019 13:04

Merge remote-tracking branch 'upstream/master' into newplatform/data-…

399bfc5

…plugin/filter-manager-1

Merge branch 'newplatform/data-plugin/filter-manager-1' of github.com…

273e64d

…:lizozom/kibana into newplatform/data-plugin/filter-manager-1

Code review minor fixes

4376e3e

lizozom added release_note:enhancement and removed release_note:skip Skip the PR/issue when compiling release notes labels Jul 1, 2019

stacey-gammon approved these changes Jul 1, 2019

View reviewed changes

lizozom merged commit 531e2b0 into elastic:master Jul 1, 2019

lizozom mentioned this pull request Jul 1, 2019

[7.x] Query Filter \ Filter Manager: de-angularize and move to data plugin (#37311) #40012

Merged

spalger pushed a commit that referenced this pull request Jul 2, 2019

Revert "Query Filter \ Filter Manager: de-angularize and move to data…

8452918

… plugin (#37311)" This reverts commit 531e2b0.

This was referenced Jul 2, 2019

Revert "Query Filter \ Filter Manager: de-angularize and move to data plugin" #40201

Closed

Failing test: Browser Unit Tests."before each" hook."before each" hook #40220

Closed

lukeelmers mentioned this pull request Jul 17, 2019

Dev Docs entry: ui/public cleanup 7.3 #37007

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Query Filter \ Filter Manager: de-angularize and move to data plugin #37311

Query Filter \ Filter Manager: de-angularize and move to data plugin #37311

lizozom commented May 29, 2019 •

edited

elasticmachine commented May 29, 2019

elasticmachine commented May 30, 2019

elasticmachine commented Jun 3, 2019

elasticmachine commented Jun 5, 2019

elasticmachine commented Jun 5, 2019

elasticmachine commented Jun 5, 2019

elasticmachine commented Jun 6, 2019

lizozom commented Jun 27, 2019

lizozom commented Jun 27, 2019

This comment has been minimized.

elasticmachine commented Jun 27, 2019

lizozom commented Jun 27, 2019

stacey-gammon left a comment

stacey-gammon Jun 26, 2019

stacey-gammon Jun 27, 2019

elasticmachine commented Jun 27, 2019

elasticmachine commented Jun 27, 2019

lizozom commented Jun 30, 2019

elasticmachine commented Jun 30, 2019

elasticmachine commented Jun 30, 2019

elasticmachine commented Jun 30, 2019

stacey-gammon left a comment

Query Filter \ Filter Manager: de-angularize and move to data plugin #37311

Query Filter \ Filter Manager: de-angularize and move to data plugin #37311

Conversation

lizozom commented May 29, 2019 • edited

Summary

Notes and comments

Checklist

elasticmachine commented May 29, 2019

💚 Build Succeeded

elasticmachine commented May 30, 2019

💔 Build Failed

elasticmachine commented Jun 3, 2019

💔 Build Failed

elasticmachine commented Jun 5, 2019

💔 Build Failed

elasticmachine commented Jun 5, 2019

💔 Build Failed

elasticmachine commented Jun 5, 2019

💔 Build Failed

elasticmachine commented Jun 6, 2019

💔 Build Failed

lizozom commented Jun 27, 2019

lizozom commented Jun 27, 2019

This comment has been minimized.

elasticmachine commented Jun 27, 2019

💔 Build Failed

lizozom commented Jun 27, 2019

stacey-gammon left a comment

Choose a reason for hiding this comment

stacey-gammon Jun 26, 2019

Choose a reason for hiding this comment

stacey-gammon Jun 27, 2019

Choose a reason for hiding this comment

elasticmachine commented Jun 27, 2019

💔 Build Failed

elasticmachine commented Jun 27, 2019

💔 Build Failed

lizozom commented Jun 30, 2019

elasticmachine commented Jun 30, 2019

💚 Build Succeeded

elasticmachine commented Jun 30, 2019

💚 Build Succeeded

elasticmachine commented Jun 30, 2019

💚 Build Succeeded

stacey-gammon left a comment

Choose a reason for hiding this comment

lizozom commented May 29, 2019 •

edited