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

[Lens] Keep global filters, time range and refresh interval on refresh #68075

Merged
merged 4 commits into from
Jun 10, 2020

Conversation

flash1293
Copy link
Contributor

@flash1293 flash1293 commented Jun 3, 2020

Fixes #61758

This PR uses the same approach as Visualize/Discover/Dashboard to keep the global query context (time range, refresh interval and pinned filters) around even if the page is refreshed or opened in another tab / browser.

As this behavior is difficult to validate via unit test, it adds a functional test suite which also makes sure the context is kept when navigating between apps.

  • x-pack/plugins/lens/public/app_plugin/app.tsx contains the main changes.
  • x-pack/plugins/lens/public/app_plugin/app.test.tsx was only changed to keep existing unit tests working, the behavior is not tested (because the only test I can think of is basically a reverse implementation of the actual code)
  • x-pack/test/functional/apps/lens/persistent_context.ts contains the functional tests

Note: The original issue also states

Also a page reload in Lens should not lose the query and non-pinned filters

This is handled by #67689 - if query or non-pinned filters are changed, the user will be stopped from navigating away by modal. With this PR all changes the user did are either persisted in the URL or in the saved object and protected via modal.

Considerations

On this PR changing the time range will write a new history entry, so going back will bring you back to the last time range instead of navigating away from Lens. I'm not sure whether that's expected behavior, but it seems like an improvement over the current state, so I left it in. A thing we should add (if possible) is to prevent the user from losing configuration by using the back button, but it seems like this is out of scope for this PR. For reference: Maps is trapping the user and the back button doesn't work at all (not sure whether that's the right approach).

@flash1293 flash1293 marked this pull request as ready for review June 4, 2020 11:45
@flash1293 flash1293 requested review from a team, mbondyra and wylieconlon June 4, 2020 11:45
Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

Tested in Chrome and Safari and LGTM. Found an edge case which we may need to combine with index pattern persistence:

  1. Create a pinned filter in a Dashboard
  2. Go to Lens, and you might see that the pinned filter is showing a Warning text label
  3. Change the Lens index pattern and the warning goes away

I don't expect you to fix this issue in this PR.

await PageObjects.timePicker.setAbsoluteRange(
'Sep 06, 2015 @ 06:31:44.000',
'Sep 18, 2025 @ 06:31:44.000'
);
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume it's intentional that this range doesn't overlap with the default time range, but I'm not sure why? We do have a PageObjects.lens.goToTimeRange helper which wraps this.

const { stop: stopSyncingQueryServiceStateWithUrl } = syncQueryStateWithUrl(
data.query,
kbnUrlStateStorage
);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to add some of these dependencies to the useEffect list explicitly- the linting rule is not currently being required, but should be.

@mbondyra
Copy link
Contributor

mbondyra commented Jun 9, 2020

@elasticmachine merge upstream

@mbondyra
Copy link
Contributor

mbondyra commented Jun 9, 2020

@flash1293 could you modify the title and description of PR to include the fact that the refreshInterval is also included? Just for the clarity.

On this PR changing the time range will write a new history entry, so going back will bring you back to the last time range instead of navigating away from Lens.

The behaviour is consistent with the rest of visualize so it's a good start in my opinion.

Tested on FF, code LGTM.

@wylieconlon I checked the edge case you've written down here, but I don't get how it's buggy, it seems like a proper behaviour for me. Could you clarify what's the expected behaviour?

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@wylieconlon
Copy link
Contributor

@mbondyra The text "Warning" is caused by a mismatch in the default index pattern vs the index pattern used in the filters. If we want to fix this, the best way is to look at the meta.indexpattern property of the filters and use that as our default.

@flash1293 flash1293 changed the title [Lens] Keep global filters and time range on refresh [Lens] Keep global filters, time range and refresh interval on refresh Jun 10, 2020
@flash1293 flash1293 merged commit a462e2c into elastic:master Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Persist timerange/pinned filters across reload/apps
5 participants