[Visualize] Make linked saved search work when user navigates back using browser back button #59690
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sulemanof
commented
Mar 10, 2020
| searchSourceParent.getField('filter') | ||
| ); | ||
| searchSource.setField('index', searchSourceParent.getField('index')); | ||
| searchSource.setField('index', currentIndex); |
There was a problem hiding this comment.
I'm still not sure, whether we should inherit the index of the parent source while unlinking,
since both the searchSource index and searchSourceParent index are equal always. At least, I didn't find a case, when they could be different.
Maybe it's a question to @ppisljar
There was a problem hiding this comment.
if you create visualization based on saved search, then searchsource won't have index field, but the parent will.
calling searchSource.getField('index') will of course show the correct index (as that walks up the tree)
sulemanof
added
Feature:NP Migration
release_note:enhancement
Team:Visualizations
|
Pinging @elastic/kibana-app (Team:KibanaApp) |
kertal
approved these changes
Mar 10, 2020
ppisljar
reviewed
Mar 11, 2020
| @@ -379,6 +380,18 @@ function VisualizeAppController( | |||
| }, | |||
| }; | |||
|
|
|||
| const handleLinkedSearch = linked => { | |||
| if (linked && !savedVis.savedSearchId) { | |||
There was a problem hiding this comment.
this confuses me ... you desctructure savedSearchId from savedVis ... so this logic is actually:
if (linked && !savedVis.savdSearchId) {
savedVis.savedSearchId = savedVis.savedSearchId;
vis.savedSearchId = savedVis.savedSearchId;
...
}
also i am not sure why we need to duplicate information on vis.savedSearchId, which is also violating the types (vis has no savedSearchId property)
There was a problem hiding this comment.
i think once you unlink, all we need is to delete savedVis.savedSearchId (for the sake of saved object) and remove the parent search source (for the sake of current view)
and when linking back, you will need to select the saved search, so you get the new savedSearchId and you can set it on savedVis
There was a problem hiding this comment.
ps: i am working on a small refactor to vis saved object and vis state, which will allow to further clean this up: you will no longer need to care about savedVis here, all you will need to do is remove parent search source and we will provide utilities to serialize searchSource + vis into the saved vis object.
There was a problem hiding this comment.
Hey @ppisljar , thanks for taking a look and reasonable comments here!
PR updated!
About storing the vis.savedSearchId , I'm not sure why, but it's presented in the vis object on creation of a visualization based on saved search:
so would be great if you could get rid of it in your refactoring pr.
|
@ppisljar FYI |
ppisljar
approved these changes
Mar 11, 2020
|
@elasticmachine merge upstream |
sulemanof
added a commit
to sulemanof/kibana
that referenced
this pull request
Mar 12, 2020
* Fix linked search behavior * Revert relying on savedSearchId in url * Fix comments * Return back savedSearchId into vis object Co-authored-by: Elastic Machine <[email protected]>
sulemanof
added a commit
that referenced
this pull request
Mar 12, 2020
* Fix linked search behavior * Revert relying on savedSearchId in url * Fix comments * Return back savedSearchId into vis object Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
gmmorris
added a commit
to gmmorris/kibana
that referenced
this pull request
Mar 12, 2020
* master: (45 commits) skip flaky suite (elastic#59717) UI Metrics use findAll to retrieve all Saved Objects (elastic#59891) [Discover] Migrate Context mocha tests to use Jest (elastic#59658) [Maps] Move redux reducers and store logic to NP (elastic#58294) rebalance x-pack groups (elastic#58930) [Discover] Reimplement $route.reload when index pattern changes (elastic#59877) [Upgrade Assistant Meta] Breaking changes issue template (elastic#59745) Skip CI based on changes in PR (elastic#59939) [ML] Transforms: Replace KqlFilterBar with QueryStringInput. (elastic#59723) [ML] Functional tests - stabilize date_nanos test (elastic#59986) [ML] Typescripting client side endpoint functions (elastic#59928) a11y tests on adding columns to discover table (elastic#59375) fix graph plugin config path (elastic#59540) fix vega config issues (elastic#59737) [Upgrade Assistant] Open And Close Slight Refactor (elastic#59890) [ML] Adding shared services to ml setup contract (elastic#59730) [Visualize] Fix linked search behavior (elastic#59690) [ML] Register NP ML plugin for Kibana management section. (elastic#59762) [Lens] Adds using queries/filters for field existence endpoint (elastic#59033) Delete FilterStateManager and QueryFilter :-D (elastic#59872) ...
rayafratkina
changed the title
💔 Build FailedTest FailuresKibana Pipeline / kibana-oss-agent / Chrome UI Functional Tests.test/functional/apps/discover/_errors·js.discover app errors invalid scripted field error is renderedStandard OutStack TraceKibana Pipeline / kibana-intake-agent / Jest Integration Tests.packages/kbn-plugin-generator/integration_tests.running the plugin-generator via 'node scripts/generate_plugin.js plugin-name' with default config then running with es instance 'yarn start' should result in the spec plugin being initialized on kibana's stdoutStandard OutStack TraceKibana Pipeline / kibana-oss-agent / Chrome UI Functional Tests.test/functional/apps/discover/_errors·js.discover app errors invalid scripted field error is renderedStandard OutStack TraceHistory
To update your PR or re-run it, just comment with: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #57977
This PR fixes the behavior of
linked/unlinkedsaved search while playing with browser history in visualize.The first intention was to rely on
savedSearchIdquery parameter in the url:And it was working fine, before I found out, that the saved visualization doesn't have the same url and looks like:
So the previous approach (rely on
linkedparam in the appState -_apiece of url) was saved,but improved handling on browser
back,forward,reloadactions. That means you could unlink from the saved search, then go back in history - the state will becomelinkedagain, then you could unlink it again with button as usualChecklist
Delete any items that are not applicable to this PR.
For maintainers