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.

Sign up for GitHub

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

Jump to bottom

[Discover] Enable Explore in Discover for adhoc data views in Lens #140726

Merged
merged 13 commits into from Sep 20, 2022
Merged

[Discover] Enable Explore in Discover for adhoc data views in Lens #140726

merged 13 commits into from Sep 20, 2022

Conversation

dimaanj
Copy link
Contributor

@dimaanj dimaanj commented Sep 14, 2022 •
edited

Summary

This PR is a one of the follow up tasks of #138283 which:

  • enables Explore in Discover button for adhoc data views in Lens.
  • fixes toast warn about missing data view when clicking browser back button

Steps to reproduce the second case

  • create adhoc data view from Discover
  • click visualise in Lens
  • modify data view in Lens (creating runtime field)
  • click browser back button
  • toast warn will about missing data view in Discover will apper

Checklist

@dimaanj dimaanj added release_note:enhancement Feature:Discover Discover Application Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Graph) v8.5.0 labels Sep 14, 2022
@dimaanj dimaanj self-assigned this Sep 14, 2022
@dimaanj dimaanj added this to PRs in Discover via automation Sep 14, 2022
@dimaanj
Copy link
Contributor Author

dimaanj commented Sep 15, 2022

@elasticmachine merge upstream

@dimaanj
Copy link
Contributor Author

dimaanj commented Sep 15, 2022

@elasticmachine merge upstream

@dimaanj
Copy link
Contributor Author

dimaanj commented Sep 15, 2022

@elasticmachine merge upstream

@dimaanj
Copy link
Contributor Author

dimaanj commented Sep 15, 2022

@elasticmachine merge upstream

@dimaanj dimaanj marked this pull request as ready for review September 15, 2022 16:04
@dimaanj dimaanj requested review from a team as code owners September 15, 2022 16:04
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

davismcphee
davismcphee approved these changes Sep 15, 2022
View reviewed changes
Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

Pulled and tested locally, both test cases worked for me, and code LGTM too 👍

@dimaanj dimaanj added the WIP Work in progress label Sep 16, 2022
ppisljar
ppisljar approved these changes Sep 20, 2022
View reviewed changes
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

LGTM

flash1293
flash1293 reviewed Sep 20, 2022
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Works fine AFAICT, just not sure about that one change

x-pack/plugins/lens/public/data_views_service/service.ts
// Do not clear initial data view instance from cache
// if adhoc data view id has been provided by the context.
if (contextDataViewSpec && contextDataViewSpec.id !== dataView.id) {
dataViews.clearInstanceCache(dataView.id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to keep the old data view in cache here? When navigating back to discover the spec is passed over so it should be able to re-create from that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

spec isn't really passed over, since Discover -> Lens navigation uses ui-actions. Going back will route to URL state with data view id.

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean going back via browser back button?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, alright, let's keep it in like this. It's not 100% reliable as the user could reload the page while in Lens, then navigate back which would break Discover. Seems like an edge case though. cc @kertal maybe we can find a more stable way to handle this longterm.

Copy link
Member

Choose a reason for hiding this comment

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

yes, if we find a better way here to prevent user running into ad-hoc data views that for various reasons no longer exist, it would be awesome, also with increased adaptations by our userbase I'm pretty sure this will be requested

@dimaanj
Merge branch 'main' of https://github.com/elastic/kibana into enable-…
b6f4cdd 
…explore-in-discover-for-adhoc-dw

# Conflicts:
#	x-pack/plugins/lens/public/indexpattern_datasource/utils.tsx
@kertal kertal self-requested a review September 20, 2022 10:10
kertal
kertal approved these changes Sep 20, 2022
View reviewed changes
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

LGTM, works as expected 👍

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
discover 79 80 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
discover 477.3KB 477.7KB +338.0B
lens 1.2MB 1.2MB -71.0B
total +267.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
discover 26.6KB 26.6KB +76.0B
Unknown metric groups

API count

id before after diff
discover 96 97 +1

References to deprecated APIs

id before after diff
lens 4 3 -1

History

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

cc @dimaanj

@dimaanj dimaanj merged commit 003d37e into elastic:main Sep 20, 2022
Discover automation moved this from PRs to Done Sep 20, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kertal kertal kertal approved these changes

@flash1293 flash1293 flash1293 approved these changes

@ppisljar ppisljar ppisljar approved these changes

@davismcphee davismcphee davismcphee approved these changes

Assignees

@dimaanj dimaanj

Labels
backport:skip This commit does not require backporting Feature:Discover Discover Application release_note:enhancement Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Graph) v8.5.0 WIP Work in progress
Projects
No open projects
Discover
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@dimaanj @elasticmachine @kibana-ci @kertal @flash1293 @ppisljar @davismcphee @kibanamachine