-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Visualize] Remove global state in visualize #58352
[Visualize] Remove global state in visualize #58352
Conversation
@elasticmachine merge upstream |
Pinging @elastic/kibana-app (Team:KibanaApp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…l_state_visualize # Conflicts: # src/legacy/core_plugins/kibana/public/visualize/legacy_imports.ts # src/legacy/core_plugins/kibana/public/visualize/np_ready/application.ts
…anof/kibana into shim/remove_global_state_visualize
Hey @kertal ! That's about filter duplicates in es search, seems it's an old issue which reproduces in earlier versions: If you don't mind, I'll try to take this issue in a separate PR. |
@@ -334,6 +332,24 @@ function VisualizeAppController( | |||
} | |||
); | |||
|
|||
const updateSavedQueryFromUrl = savedQueryId => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is only setting savedQuery
, but it's not triggering the updateFromSavedQuery
transition (together with syncing the data plugin state) even though it looks like it should.
In 7.6 I can change the saved query in the url and it will re-apply the filters, but here it's just selecting it in the saved object management popover.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could find a small bug in how global/local filters are handled - seems like they get mixed. Global filters are added to the saved object as local filters:
- Create new visualization
- Add filter
- Pin it
- Save visualization
- Go to listing page
- Open visualization again
- Pinned filter is added as local filter
…l_state_visualize # Conflicts: # src/legacy/core_plugins/kibana/public/visualize/legacy_imports.ts
…l_state_visualize
Hey @flash1293 @kertal @Dosant Besides that, I didn't found any other inconsistency. |
Great work @sulemanof ! I will have another look shortly, we can also work on the saved query topic in a separate PR to get this change in. |
src/legacy/core_plugins/kibana/public/visualize/np_ready/listing/visualize_listing.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good. Tomorrow I will take some time to test as well.
src/legacy/core_plugins/kibana/public/visualize/np_ready/editor/editor.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't tested, but aside from the comments given already, code changes mostly LGTM
const { | ||
timefilter: { timefilter }, | ||
} = query; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two lines do nothing
You can remove them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usage of the timefilter
is not visible in current diff,
but it's used in previous code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM! Great work 🌻 ! Issue I've mentioned is resolved. Tested locally in Chrome.
Did you create an issue / PR about the duplicate params in the REST request, If not can do that, np
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-xpack-agent / X-Pack Spaces API Integration Tests -- security_and_spaces.x-pack/test/spaces_api_integration/security_and_spaces/apis/copy_to_space·ts.spaces api with security copy to spaces dual-privileges readonly user from the space_1 space "before each" hook for "should return 200 when copying to non-existent space"Standard Out
Stack Trace
Kibana Pipeline / kibana-xpack-agent / X-Pack Spaces API Integration Tests -- security_and_spaces.x-pack/test/spaces_api_integration/security_and_spaces/apis/copy_to_space·ts.spaces api with security copy to spaces dual-privileges readonly user from the space_1 space "after each" hook for "should return 200 when copying to non-existent space"Standard Out
Stack Trace
Kibana Pipeline / kibana-xpack-agent / X-Pack Spaces API Integration Tests -- security_and_spaces.x-pack/test/spaces_api_integration/security_and_spaces/apis/copy_to_space·ts.spaces api with security copy to spaces dual-privileges readonly user from the space_1 space "before each" hook for "should return 200 when copying to non-existent space"Standard Out
Stack Trace
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and works as expected. Thanks for this very important step 🎊
* Remove global state in visualize * Fix saved query * Update saved query handling * Resolve merge conflicts * Use new state syncing helpers * Fix state behavior * Prevent loosing the global state * Update state syncing with url Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
* Remove global state in visualize * Fix saved query * Update saved query handling * Resolve merge conflicts * Use new state syncing helpers * Fix state behavior * Prevent loosing the global state * Update state syncing with url Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
* master: (22 commits) Generate docs from data plugin (elastic#56955) Fixes elastic#59513 by hiding one of the symmetric edges rather than omiting it (elastic#59514) Harden creation of child processes (elastic#55697) [Alerting] replace watcher http APIs used by index threshold Alerting (elastic#59475) [Maps][docs] add more details to Quantitative data driven styling docs (elastic#59553) chore: 🤖 hide Drilldowns in master (elastic#59698) [Discover] Migrate AppState/GlobalState to new app state helpers (elastic#57175) Use HTTP request schemas to create types, use those types in the client (elastic#59340) [Maps] Support categorical styling for numbers and dates (elastic#57908) [ML] Functional API tests - bucket span estimation with custom search.max_buckets (elastic#59665) Fix slm_ui setting by changing camel case back to snake case. (elastic#59663) removes beta tag (elastic#59618) [DOCS] Removed spatial references (elastic#59595) fix outdated docs (elastic#58729) [ML] Fixes bucket span estimators loading of max_buckets setting (elastic#59639) [ML] Refactoring anomaly detector job types (elastic#59556) [Upgrade Assistant] Better handling of closed indices (elastic#58890) additional visualizations plugin cleanup before moving to NP (elastic#59318) In scripted fields, unable to switch the `Type` - getting a console error which says - Class constructor DecoratedFieldFormat cannot be invoked without 'new' (elastic#59285) [Visualize] Remove global state in visualize (elastic#58352) ...
…s/kibana into alerting/fix-flaky-instance-test * 'alerting/fix-flaky-instance-test' of github.com:gmmorris/kibana: (176 commits) Generate docs from data plugin (elastic#56955) Fixes elastic#59513 by hiding one of the symmetric edges rather than omiting it (elastic#59514) Harden creation of child processes (elastic#55697) [Alerting] replace watcher http APIs used by index threshold Alerting (elastic#59475) [Maps][docs] add more details to Quantitative data driven styling docs (elastic#59553) chore: 🤖 hide Drilldowns in master (elastic#59698) [Discover] Migrate AppState/GlobalState to new app state helpers (elastic#57175) Use HTTP request schemas to create types, use those types in the client (elastic#59340) [Maps] Support categorical styling for numbers and dates (elastic#57908) [ML] Functional API tests - bucket span estimation with custom search.max_buckets (elastic#59665) Fix slm_ui setting by changing camel case back to snake case. (elastic#59663) removes beta tag (elastic#59618) [DOCS] Removed spatial references (elastic#59595) fix outdated docs (elastic#58729) [ML] Fixes bucket span estimators loading of max_buckets setting (elastic#59639) [ML] Refactoring anomaly detector job types (elastic#59556) [Upgrade Assistant] Better handling of closed indices (elastic#58890) additional visualizations plugin cleanup before moving to NP (elastic#59318) In scripted fields, unable to switch the `Type` - getting a console error which says - Class constructor DecoratedFieldFormat cannot be invoked without 'new' (elastic#59285) [Visualize] Remove global state in visualize (elastic#58352) ...
Summary
Remove usage of Global State in Visualize, use
syncAppFilters
&syncQuery
instead.Fixes #57811
Checklist
Delete any items that are not applicable to this PR.
For maintainers