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

[Drilldowns] dashboard url param to merge filters #64633

Closed

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented Apr 28, 2020

Summary

Needed for #61219.

In drilldowns branch there is a bug with filters right now: When url to drilldown is generated and navigated to, fillers from URL always take precedence which makes sense for normal url flow, but doesn't make sense for navigation with drilldown.

This pr adds optional query param to dashboard url which allows to change default initial filters handling:
it allows instead of overriding saved filters by filters from URL merge filters from 2 sources together.

Please refer to original discussion for more context: #63108 (comment)

So options:
Option 1: retrieve saved filters in Drilldowns code. Pass all filters to url generator
Option 2: retrieve saved filters inside url generator. merge them with incoming filters when generating url
Option 3: Url generator optionally adds additional flag in the url to also preserve saved filters: e.g. &savedFilters=merge. When dashboard is loaded it will use that flag to decided what to do with filters saved in dashboard.

This pr goes with Option 3 ⬆️

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@elasticmachine
Copy link
Contributor

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

@Dosant Dosant added the release_note:skip Skip the PR/issue when compiling release notes label Apr 28, 2020
@Dosant Dosant marked this pull request as ready for review April 28, 2020 11:30
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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

@majagrubic
Copy link
Contributor

majagrubic commented Apr 29, 2020

I read the discussion in #63108 (comment) and I would recommend Option 2 here. Most of the arguments have already been laid out in there, but I just want to reiterate on arguments against it:

  1. introducing a query param creates a public contract that we need to maintain
  2. it's cumbersome to remove query params in the dashboard controller

As for the argument of taking care of the missing dashboard id - I think returning to dashboard listing page and displaying an error message is a perfectly acceptable flow and something we can't avoid.

@Dosant
Copy link
Contributor Author

Dosant commented Apr 29, 2020

Will reiterate and go with Option 2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Dashboard Dashboard related features Feature:Embedding Embedding content via iFrame Feature:Filters release_note:skip Skip the PR/issue when compiling release notes v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants