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 generator to preserve saved filters from destination dashboard #64767

Merged

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented Apr 29, 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 fetches destination dashboard in url generator and adds it's saved filters into resulting url.
Url generator doesn't do any error handling and just swallows errors if retrieving those filters wasn't successful.

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 2 ⬆️
Initially I built Option 3: #64633

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@Dosant Dosant added Feature:Dashboard Dashboard related features Feature:Filters Feature:Embedding Embedding content via iFrame v8.0.0 Team:AppArch release_note:skip Skip the PR/issue when compiling release notes v7.8.0 labels Apr 29, 2020
@Dosant Dosant requested review from majagrubic, ThomThomson, stacey-gammon and a team April 29, 2020 13:16
@elasticmachine
Copy link
Contributor

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

@@ -99,7 +132,7 @@ export const createDirectAccessDashboardLinkGenerator = (
GLOBAL_STATE_STORAGE_KEY,
cleanEmptyKeys({
time: state.timeRange,
filters: state.filters?.filter(f => esFilters.isFilterPinned(f)),
filters: filters?.filter(f => esFilters.isFilterPinned(f)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is not a new change, but for my own understanding - why do we need to filter pinned filters here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

url_generator accepts array of filters to apply and to make pinned state work in the url properly we have to split those into _a and _g :(

Copy link
Contributor

@majagrubic majagrubic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this in Chrome 80 on Mac OS for the following scenarios (ie. existing functionality):

  • loading saved query preserves filters
  • add a new visualization to dashboard preserves filters
  • sharing a dashboard via link preserves filters
  • sharing a dashboard via embedding preserves filters
  • sharing a dashboard via link > change original filters > filter is changed on the shared dashboard only
  • sharing a dashboard via embedding > change original filters > filter is changed

I then followed @Dosant 's suggestion and added the following snippet at the end of start function in the dashboard plugin:

plugins.share?.urlGenerators
      .getUrlGenerator(DASHBOARD_APP_URL_GENERATOR)
      .createUrl({ dashboardId: 'some-dashbord', filters: [] })
      .then(url => {
        alert(url);
      });

This threw an error as expected.

Codewise, a lot cleaner than adding this in the URL. 🚢

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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

@Dosant Dosant merged commit dcc4081 into elastic:master Apr 30, 2020
@Dosant Dosant deleted the dev/dashboard-url-generator-merge-filters branch April 30, 2020 08:34
Dosant added a commit to Dosant/kibana that referenced this pull request Apr 30, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 30, 2020
* master: (42 commits)
  [Ingest] Allow aggent to send metadata compliant with ECS (elastic#64452)
  [Endpoint] Remove todos, urls to issues (elastic#64833)
  [Uptime] Remove hard coded value for monitor states histograms (elastic#64396)
  Feature/send feedback link (elastic#64845)
  [ML] Moving get filters capability to admin (elastic#64879)
  Remove edit alert button from alerts list (elastic#64643)
  [EPM] Handle constant_keyword type in KB index patterns and ES index templates (elastic#64876)
  [ML] Disable data frame anaylics clone button based on permission (elastic#64830)
  Dashboard url generator to preserve saved filters from destination dashboard (elastic#64767)
  add generic typings for SavedObjectMigrationFn (elastic#63943)
  Allow to define and update a defaultPath for applications (elastic#64498)
  [Event Log] add rel=primary to saved objects for query targets (elastic#64615)
  [Lens] Use a size of 5 for first string field in visualization (elastic#64726)
  [SIEM][Lists] Removes plugin dependencies, adds more unit tests, fixes more TypeScript types
  [Ingest] Edit datasource UI (elastic#64727)
  [Lens] Bind all time fields to the time picker (elastic#63874)
  [Lens] Use suggestion system in chart switcher for subtypes (elastic#64613)
  Improve alpha messaging (elastic#64692)
  [Ingest] Allow to enable monitoring of elastic agent (elastic#63598)
  [Metrics UI] Fix alerting when a filter query is present (elastic#64575)
  ...
Dosant added a commit that referenced this pull request Apr 30, 2020
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