Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[context view] Apply filters to the context query #11466

Merged
merged 14 commits into from May 18, 2017

Conversation

@weltenwort
Copy link
Contributor

commented Apr 27, 2017

Summary: This adds the ability to display a filter bar in the Context view and to apply those filters to the queries. It also modifies the link from the Discover view to the Context view to copy the currently defined filters when switching. New filters can be added from within the Context view using the icons in the expanded detail rows.

resolves #10954
relates to #275
builds upon #9198 and #11127

Progress

  • Add filter bar
  • Enable filter icons in detail view
  • Copy disabled filters from Discover
  • Add functional tests

Screenshots

enhancement-context-filter

image

@Bargs
Copy link
Contributor

left a comment

This looks really good already!

I only noticed one issue: pinning a filter in the context view throws an error and causes the page to reload.

I think the only other outstanding items are disabling the filters by default and re-adding the filter buttons on the field rows.

@tbragin tbragin referenced this pull request May 1, 2017

Merged

Show event context #9198

13 of 13 tasks complete

@weltenwort weltenwort force-pushed the weltenwort:enhancement/context-filter-bar branch 2 times, most recently from d81a08f to f0152d0 May 3, 2017

@weltenwort

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2017

@Bargs I think it is in a state that justifies another look

@Bargs
Copy link
Contributor

left a comment

Functionality works well, overall the code looks great! Left just a few nitpicky questions.

@@ -43,7 +46,13 @@ export function QueryParameterActionsProvider() {
)
);

const addTermsFilter = (state) => (field, values, operation) => {

This comment has been minimized.

Copy link
@Bargs

Bargs May 8, 2017

Contributor

"termsFilter" is a bit of an overloaded (ahem) term since there's an actual Elasticsearch query type called "terms". Is there something you were trying to convey with this name that "addFilter" doesn't?

This comment has been minimized.

Copy link
@weltenwort

weltenwort May 9, 2017

Author Contributor

I actually intended to convey that this adds a terms filter in the ES sense, but just noticed that it actually adds either an exists or a phrase filter. So the name is really misleading. I'll change it to addFilter.

@@ -43,7 +46,13 @@ export function QueryParameterActionsProvider() {
)
);

const addTermsFilter = (state) => (field, values, operation) => {
const { queryParameters: { indexPatternId } } = state;

This comment has been minimized.

Copy link
@Bargs

Bargs May 8, 2017

Contributor

Does this provide a benefit over const indexPatternId = state.queryParameters.indexPatternId? Does it not throw if queryParameters is undefined or something? Just trying to understand the preference for destructing here since a regular assignment seems much more readable to me.

This comment has been minimized.

Copy link
@weltenwort

weltenwort May 11, 2017

Author Contributor

This just stems from the attempt to be consistent with the structure of the other action creators that fetch more than one value from the state. You are correct in that it does not contribute to readability here.

This comment has been minimized.

Copy link
@Bargs

Bargs May 11, 2017

Contributor

Ah ok, I just wasn't sure if there was some benefit I wasn't aware of.

@@ -43,7 +46,13 @@ export function QueryParameterActionsProvider() {
)
);

const addTermsFilter = (state) => (field, values, operation) => {
const { queryParameters: { indexPatternId } } = state;
filterManager.add(field, values, operation, indexPatternId);

This comment has been minimized.

Copy link
@Bargs

Bargs May 8, 2017

Contributor

Should we also call $scope.indexPattern.popularizeField(field, 1); like in discover?

This comment has been minimized.

Copy link
@weltenwort

weltenwort May 11, 2017

Author Contributor

I'm not sure how useful it would be to influence the ordering of the fields in the Discover field chooser via some action in the Context view. It might actually cause more confusion. 🤔

@tbragin what is your opinion?

This comment has been minimized.

Copy link
@Bargs

Bargs May 11, 2017

Contributor

We do it in Dashboard too fwiw. While ordering in the Discover sidebar is the only external effect at the moment, I always thought of field popularity as a more general purpose concept, a way to track how often fields are used across apps. But, we use it pretty inconsistently ¯_(ツ)_/¯

This comment has been minimized.

Copy link
@tbragin

tbragin May 16, 2017

Contributor

I don't see a problem with actions in context view influencing field popularity.

This comment has been minimized.

Copy link
@weltenwort

weltenwort May 16, 2017

Author Contributor

So while trying to add this I noticed that bumping the field popularity when adding a filter never worked in 5.x, because indexPattern.popularizeField() was always passed an incorrect argument for filtering operations. It only worked for column adding/removing. Do we still want to add it to the context view and fix it in other places?

export function setFilterDisabled(filter, disabled) {
const { meta = {} } = filter;

return {

This comment has been minimized.

Copy link
@Bargs

Bargs May 8, 2017

Contributor

Should this return a deep clone?

This comment has been minimized.

Copy link
@weltenwort

weltenwort May 11, 2017

Author Contributor

I have found that the best way to use javascript objects immutably is to return new objects when they are modified and original objects when they are not. This has the effect, that cheap comparisons by identity work well to detect changes while still keeping memory overhead at bay. It almost resembles structural sharing as used by the popular persistent data structure implementations and is in line with the best practice used by the redux and react communities.

The obvious downside is that it does not protect you against "malicious" code that modifies deep subtrees in-place, but I hope we will rather change that code than resolve to overly defensive programming.

This comment has been minimized.

Copy link
@Bargs

Bargs May 11, 2017

Contributor

I'm not sure I buy it, but it sounds like a debate not worth getting into here :) let's leave it as is.

@@ -0,0 +1,25 @@
export function disableFilter(filter) {

This comment has been minimized.

Copy link
@Bargs

Bargs May 8, 2017

Contributor

Could you add a few unit tests for the functions in this module?

@weltenwort weltenwort force-pushed the weltenwort:enhancement/context-filter-bar branch 3 times, most recently from 7bf5a36 to 9ad7933 May 11, 2017

@weltenwort weltenwort changed the title [WIP] [context view] Apply filters to the context query [context view] Apply filters to the context query May 15, 2017

@weltenwort

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2017

the test failures seem to be unrelated

@Bargs I hope to have addressed your points

@weltenwort weltenwort requested a review from lukasolson May 15, 2017

@Bargs

Bargs approved these changes May 15, 2017

@Bargs

This comment has been minimized.

Copy link
Contributor

commented May 15, 2017

LGTM

@lukasolson
Copy link
Member

left a comment

Awesome improvement! I found what I think are a couple of issues:

When I create a filter, then pin it, then switch to the context view, the filter appears to be enabled, but it really isn't. If I disable it then re-enable it, it actually becomes enabled:

may-15-2017 14-16-30

Also, when I go to the context view, it appears that the filter in/out backgrounds persist, making white boxes in each of the columns of the anchor row:

image

When I create a filter in discover, then click the back button, the filter gets removed, but it doesn't navigate anywhere. When I go to the context view, then create a filter, then click the back button, I'm returned to the discover view. However, if I'm in the context view, then create a filter, then disable the filter, then click the back button, I stay in the context view but the filter is re-enabled:

may-15-2017 14-21-40

@weltenwort weltenwort force-pushed the weltenwort:enhancement/context-filter-bar branch from 9ad7933 to ef09aa6 May 16, 2017

@weltenwort

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2017

Thanks for the thorough testing, @lukasolson!

I hope to have addressed the inconsistencies in the filter behavior. The broken
rendering of the filter icons in the table is occurring in Discover on master too, so I
will fix that in a separate PR.

@weltenwort

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2017

@lukasolson here is the promised PR: #11819

@alexfrancoeur

This comment has been minimized.

Copy link
Contributor

commented May 17, 2017

@weltenwort this looks great! I was hoping you could provide some clarification on a few items.

It appears with this workflow, any pre-defined filters will automatically be disabled. I understand this workflow and think it makes sense for viewing surrounding documents. Changes made to these filters are local to the context viewer as well. Pinned filters seem to be the exception here. If a pinned filter is enabled is the expectation that they are enabled upon entering the context viewer as well? Or are they expected to follow suite and disable as non-pinned filters do? I believe Lukas ran into the same issue here. I'm just wondering what the expected behavior is. It's a little odd that pinned filters keep their state when entering the context viewer but other filters do not. At the same time, the alternative of automatically disabling a global pinned filter doesn't necessarily make sense either.

may-17-2017 06-48-21

This is minor and can be addressed with platform changes later on, but there is no way to add a breadcrumb at the moment is that correct? It can be a bit annoying to hit the back button for each filter change to return to discover and I don't believe it's always the users first inclination to click the discover button in the left hand nav.

@lukasolson I'm wondering, since we are targeting both this and #11375 for 5.5 - are there any concerns combining the two? The ability to add new filters directly from the menu will make this view even more powerful - I'm just wondering if there are any conflicts that we might want to get ahead of. For example - from a user experience perspective the filters are all automatically disabled upon entering the context view. In it's current state, there is an action to enable or disable all. I believe these bulk actions are lost with the new filtering capabilities but correct me if I'm wrong.

@weltenwort

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2017

If a pinned filter is enabled is the expectation that they are enabled upon entering the context viewer as well?

That is what I intended here. (I hopefully fixed the problem that they appeared to be enabled but were not applied.) It kind of makes sense given the semantics of pinned filters (in so far as the pinned filters themselves make sense 😉). This is certainly up for discussion if there are some clear arguments and use-cases for different behavior.

breadcrumbs

The breadcrumbs were removed as a result of the discussion in #9198, because we did not want to represent history with them. This becomes especially important with the new accessibility criteria that the design team is currently trying to make Kibana comply with. Maybe they have something to say about this.

filter editors from #11375

We are not expecting any particular difficulties here. Since the context app is just consuming the filter bar as-is, improvements made to it should "just work", assuming that the interface via the queryFilter service remains stable.

@epixa epixa added the v6.0.0-alpha2 label May 17, 2017

@lukasolson
Copy link
Member

left a comment

LGTM!

weltenwort added some commits Apr 27, 2017

Only store the index pattern id in the state
Since the index pattern object is not clonable and serializable, only
the index pattern id is stored in the state now.
Move filter handling to RouteController
In order to prevent special treatment in the context app component of
filters from the app state and the global state depending on their
origin, reading the filters is now moved up to the
`ContextAppRouteController`. While doing so
`ContextAppRouteController` does not handle `appState.filters` anymore,
but delegates all access to the `queryFilter` service. Writing filter
changes stays unchanged, as it was already fully delegated to
`queryFilter`.

@weltenwort weltenwort force-pushed the weltenwort:enhancement/context-filter-bar branch from ef09aa6 to cd5d987 May 18, 2017

@weltenwort weltenwort merged commit bc64b9a into elastic:master May 18, 2017

1 check passed

kibana-ci Build finished.
Details

weltenwort added a commit to weltenwort/kibana that referenced this pull request May 18, 2017

[5.x] [context view] Apply filters to the context query
Backports PR elastic#11466

This adds the ability to display a filter bar in the Context view and to
apply those filters to the queries. It also modifies the link from the
Discover view to the Context view to copy the currently defined filters
when switching. New filters can be added from within the Context view
using the icons in the expanded detail rows.

weltenwort added a commit to weltenwort/kibana that referenced this pull request May 18, 2017

[5.x] [context view] Apply filters to the context query
Backports PR elastic#11466

This adds the ability to display a filter bar in the Context view and to
apply those filters to the queries. It also modifies the link from the
Discover view to the Context view to copy the currently defined filters
when switching. New filters can be added from within the Context view
using the icons in the expanded detail rows.

weltenwort added a commit that referenced this pull request May 18, 2017

[5.x] [context view] Apply filters to the context query (#11890)
Backports PR #11466

This adds the ability to display a filter bar in the Context view and to
apply those filters to the queries. It also modifies the link from the
Discover view to the Context view to copy the currently defined filters
when switching. New filters can be added from within the Context view
using the icons in the expanded detail rows.

snide added a commit to snide/kibana that referenced this pull request May 30, 2017

[context view] Apply filters to the context query (elastic#11466)
This adds the ability to display a filter bar in the Context view and to apply those filters to the queries. It also modifies the link from the Discover view to the Context view to copy the currently defined filters when switching. New filters can be added from within the Context view using the icons in the expanded detail rows.

@epixa epixa removed the WIP label Jun 2, 2017

Dreadnoth added a commit to Dreadnoth/kibana that referenced this pull request Aug 8, 2017

[5.x] [context view] Apply filters to the context query (elastic#11890)
Backports PR elastic#11466

This adds the ability to display a filter bar in the Context view and to
apply those filters to the queries. It also modifies the link from the
Discover view to the Context view to copy the currently defined filters
when switching. New filters can be added from within the Context view
using the icons in the expanded detail rows.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.