Bugfix dashboard unpins filters #62301

Dosant · 2020-04-02T11:58:24Z

Summary

Fixes #62126

Fixes following cases:

~~Saving dashboard with pinned filter unpins it~~ Do not save pinned filters with dashboard see Bugfix dashboard unpins filters #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

Checklist

Delete any items that are not applicable to this PR.

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 updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-04-03T10:31:39Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

flash1293

I tested and couldn't find anything behaving strange (everything like in the description).

I just want to raise the point that this is not the behavior in Visualize - there pinned filters are just ignored when saving the visualization. In Discover it's currently unpinning the filter when saving a saved search and saving it along with the search (seems similar to the behavior of dashboard before this change).

This means this PR will change 2 different behaviors in different apps to 3 different behaviors in different apps - that doesn't seem right to me.

As pinned/non-pinned filters are already confusing enough I think we should at least make them behave consistent in all of the apps - then it doesn't matter to me too much what exactly the behavior is.

I think the following is the most sane (at least to me, but that might be highly subjective):

Don't save pinned filters ever - when saving a saved object, only non-pinned filters are considered
Don't unpin filters automatically in any case (that's definitely buggy behavior)
When carrying a pinned filter over from another app/saved object and the current saved object has the same filter in a non-pinned status, display both of them separately (pinned and non-pinned side by side). Otherwise we might accidentally save a saved object without this filter because we magically transformed a non-pinned filter in a pinned filter

This follows just one simple rule: Never change the filter status automatically - if a filter changes from pinned to unpinned or vice versa, it was always because of an explicit user action.

Sorry for hijacking this PR, we can also move this discussion somewhere else. Also cc @timroes

test/functional/apps/dashboard/dashboard_filter_bar.js

Dosant · 2020-04-03T15:23:20Z

@flash1293,

yee.. have to admit I didn't look into discover and visualize and was focused on fixing a regression in dashboard (my bad) which seemed to be like a really unpleasant one.

I think the following is the most sane (at least to me, but that might be highly subjective):
Don't save pinned filters ever - when saving a saved object, only non-pinned filters are considered

I think behaviour in this pr is better, because we save state closer to what user sees on ui and likely what user expects to see when opening saved stuff again

Don't unpin filters automatically in any case (that's definitely buggy behavior)

Yes, that is terrible. Agree. This is fixed in this pr in dashboard. We can follow up on that in discover in separate pr for 7.7 fix?

When carrying a pinned filter over from another app/saved object and the current saved object has the same filter in a non-pinned status, display both of them separately (pinned and non-pinned side by side). Otherwise we might accidentally save a saved object without this filter because we magically transformed a non-pinned filter in a pinned filter

Afaik how FilterManager works - this is not trivial and risky change. Not something I'd do without careful considerations.

I think (subjectively) current behaviour in dashboard is pretty good one :) Happy with the result from user standpoint

flash1293

I'm also happy with the behavior in this PR, @Dosant - you make a good point about saving what the user is currently seeing in terms of data. If it would work like this in Discover and Visualize as well, I think it would be totally fine.

With my "most sane" behavior I tried to rethink it from the beginning, but it's definitely not worth going through major changes just for the sake of it.

If we are following up with a discover fix introducing the same change there, it's definitely a LGTM from me (AFAIK Visualize was always an outlier in this regard).

Still interested in @timroes opinion on this :)

timroes · 2020-04-03T16:34:51Z

Thanks for pinning me. Yeah due to some regressions in the past this behavior has driftet apart in different applications. The "correct" (in the sense of how we defined it do behave) behavior is, that pinned filters should always be ignored when saving. They are just your temporary state while navigating and should not be stored in any saved object. I know that this is currently buggy in a couple of applications.

@cchaos is also working on designs to separate pinned filters and unpinned filters properly, and in general have a better way around showing what's part of the saved object and what not. But until we have that, pinned filters should basically just not be saved.

Dosant · 2020-04-03T16:50:21Z

@timroes, I'll adjust then this pr to not save pinned filter together with saved object.

lizozom · 2020-04-05T12:22:21Z

@elasticmachine merge upstream

…-upins-filters # Conflicts: # src/plugins/dashboard/public/application/lib/filter_utils.ts # src/plugins/dashboard/public/application/lib/update_saved_dashboard.ts

Dosant · 2020-04-06T12:15:50Z

@timroes, @flash1293, @lizozom please re-review. Change the behaviour to NOT save pinned filters at all (#62301 (comment)). Other 2 bugs stayed fixed

Reviewing new behavior

flash1293

Tested and everything seems fine, LGTM.

The current behavior is not 100% what I had in mind before reviewing this PR (so thanks for making me aware of the nuances), but it's consistent and easy to learn.

I think the only problem to have this consistent now is the Discover behavior (unpinning filters when saving and when there is a pinned and unpinned version of the same filter) - I'm going to create a separate PR for that.

Dosant · 2020-04-07T13:21:03Z

@elasticmachine merge upstream

Dosant · 2020-04-08T08:46:32Z

@elasticmachine merge upstream

@lizozom

thanks @lizozom

Dosant · 2020-04-08T18:51:14Z

@elasticmachine merge upstream

…-upins-filters

Dosant · 2020-04-10T09:45:17Z

Waiting for @lizozom approve.

fyi @flash1293, we changed implementation by moving it from dashboard code to filter manager: 94e360e

lizozom

LGTM

lizozom · 2020-04-12T07:41:49Z

@elasticmachine merge upstream

…a into dev/dashboard-upins-filters

…-upins-filters

kibanamachine · 2020-04-12T12:34:18Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: fe606cb

History

💚 Build #39706 succeeded fe5c259
💔 Build #39648 failed 7fa313f
💔 Build #39570 failed 94e360e
💚 Build #39527 succeeded 7d9ec0d
💚 Build #39243 succeeded 8bdc15c

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Fixes following cases: Saving dashboard with pinned filter unpins it. Do not save pinned filters with dashboard see elastic#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 <[email protected]>

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 <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

fix dashboard unpins filters

15fe150

Dosant force-pushed the dev/dashboard-upins-filters branch from b8f666e to 15fe150 Compare April 3, 2020 10:30

Dosant added the Team:AppArch label Apr 3, 2020

Dosant added release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v7.8.0 v8.0.0 Feature:Dashboard Dashboard related features Feature:Filters Feature:StateManagement labels Apr 3, 2020

Dosant marked this pull request as ready for review April 3, 2020 10:32

Dosant requested a review from a team April 3, 2020 10:32

Dosant requested a review from a team as a code owner April 3, 2020 10:32

Dosant requested review from flash1293 and lizozom April 3, 2020 10:32

Dosant added bug Fixes for quality problems that affect the customer experience regression labels Apr 3, 2020

flash1293 reviewed Apr 3, 2020

View reviewed changes

test/functional/apps/dashboard/dashboard_filter_bar.js Outdated Show resolved Hide resolved

Dosant requested a review from timroes April 3, 2020 15:24

flash1293 previously approved these changes Apr 3, 2020

View reviewed changes

Dosant added 3 commits April 6, 2020 12:26

do not save pinned filters

7f49af8

Merge branch 'master' of github.com:elastic/kibana into dev/dashboard…

a17648c

…-upins-filters # Conflicts: # src/plugins/dashboard/public/application/lib/filter_utils.ts # src/plugins/dashboard/public/application/lib/update_saved_dashboard.ts

merge

6ca3d60

Dosant removed the release_note:skip Skip the PR/issue when compiling release notes label Apr 6, 2020

Dosant requested a review from flash1293 April 6, 2020 14:28

flash1293 approved these changes Apr 7, 2020

View reviewed changes

flash1293 mentioned this pull request Apr 7, 2020

Discover pinned filters behavior inconsistent with other apps #62773

Closed

Merge branch 'master' into dev/dashboard-upins-filters

8bdc15c

elasticmachine and others added 3 commits April 8, 2020 04:46

Merge branch 'master' into dev/dashboard-upins-filters

7d9ec0d

simplify implementation

94e360e

thanks @lizozom

fix types

7fa313f

elasticmachine and others added 2 commits April 8, 2020 14:51

Merge branch 'master' into dev/dashboard-upins-filters

fe5c259

Merge branch 'master' of github.com:elastic/kibana into dev/dashboard…

5a340e5

…-upins-filters

lizozom approved these changes Apr 12, 2020

View reviewed changes

Dosant added 2 commits April 12, 2020 12:44

Merge branch 'dev/dashboard-upins-filters' of github.com:Dosant/kiban…

43c74c9

…a into dev/dashboard-upins-filters

Merge branch 'master' of github.com:elastic/kibana into dev/dashboard…

fe606cb

…-upins-filters

Dosant merged commit 1199c8c into elastic:master Apr 12, 2020

Dosant mentioned this pull request Apr 12, 2020

[7.x] Bugfix dashboard unpins filters (#62301) #63321

Merged

Dosant mentioned this pull request Apr 12, 2020

[7.7] Bugfix dashboard unpins filters (#62301) #63322

Merged

Dosant mentioned this pull request Sep 24, 2020

Pinned filters no longer save with objects in 7.8.1 like they did in 7.5.1 #76524

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix dashboard unpins filters #62301

Bugfix dashboard unpins filters #62301

Dosant commented Apr 2, 2020 •

edited

Loading

elasticmachine commented Apr 3, 2020

flash1293 left a comment •

edited

Loading

Dosant commented Apr 3, 2020

flash1293 left a comment

timroes commented Apr 3, 2020

Dosant commented Apr 3, 2020

lizozom commented Apr 5, 2020

Dosant commented Apr 6, 2020

flash1293 left a comment

Dosant commented Apr 7, 2020

Dosant commented Apr 8, 2020

Dosant commented Apr 8, 2020

Dosant commented Apr 10, 2020

lizozom left a comment

lizozom commented Apr 12, 2020

kibanamachine commented Apr 12, 2020

Bugfix dashboard unpins filters #62301

Bugfix dashboard unpins filters #62301

Conversation

Dosant commented Apr 2, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Apr 3, 2020

flash1293 left a comment • edited Loading

Choose a reason for hiding this comment

Dosant commented Apr 3, 2020

flash1293 left a comment

Choose a reason for hiding this comment

timroes commented Apr 3, 2020

Dosant commented Apr 3, 2020

lizozom commented Apr 5, 2020

Dosant commented Apr 6, 2020

flash1293 left a comment

Choose a reason for hiding this comment

Dosant commented Apr 7, 2020

Dosant commented Apr 8, 2020

Dosant commented Apr 8, 2020

Dosant commented Apr 10, 2020

lizozom left a comment

Choose a reason for hiding this comment

lizozom commented Apr 12, 2020

kibanamachine commented Apr 12, 2020

💚 Build Succeeded

History

Dosant commented Apr 2, 2020 •

edited

Loading

flash1293 left a comment •

edited

Loading