[NP] Graph: get rid of saved objects class wrapper#59917
[NP] Graph: get rid of saved objects class wrapper#59917maryia-lapata merged 25 commits intoelastic:masterfrom
Conversation
|
Pinging @elastic/kibana-app (Team:KibanaApp) |
|
@elasticmachine merge upstream |
flash1293
left a comment
There was a problem hiding this comment.
Added a bunch of general hints because I noticed too late it's probably still too early to review at this level. Most of it is probably planned like this anyway.
src/plugins/saved_objects/public/saved_object/helpers/apply_es_resp.ts
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| export async function createSourceUtil( |
There was a problem hiding this comment.
This helper makes a lot of sense to me, but the name is a little confusing to me - isn't this just saveWithConfirmation (or something like that)?
Also we shouldn't make this helper dependent on the legacy type SavedObject - just pass in the stuff you need directly.
There was a problem hiding this comment.
Of course, it was just a draft function name.
Updated.
x-pack/legacy/plugins/graph/public/helpers/saved_workspace_utils.ts
Outdated
Show resolved
Hide resolved
# Conflicts: # x-pack/legacy/plugins/graph/public/app.js # x-pack/legacy/plugins/graph/public/application.ts # x-pack/legacy/plugins/graph/public/state_management/mocks.ts # x-pack/legacy/plugins/graph/public/state_management/store.ts # x-pack/legacy/plugins/graph/public/types/persistence.ts
kertal
left a comment
There was a problem hiding this comment.
While testing I've encountered the following, I've added a new graph, named it test, created another one, also named test, got the following modal:
which is expected
However when I clicked on Save, it wasn't overwritten, but added to the List
could you investigate? many thx!
src/plugins/saved_objects/public/saved_object/helpers/create_source.ts
Outdated
Show resolved
Hide resolved
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
The issue, when saved object is not overwritten, is reproduced in master. Here is the issue #60735. |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
kertal
left a comment
There was a problem hiding this comment.
Code LGTM! nice cleanup! Tested locally in chrome. works
* Implement find saved workspace * Implement get saved workspace * Create helper function as applyESResp * Fix eslint * Implement savedWorkspaceLoader.get() * Implement deleteWS * Implement saveWS * Remove applyESRespUtil * Refactoring * Refactoring * Remove savedWorkspaceLoader * Update unit test * Fix merge conflicts * Add unit tests for saveWithConfirmation * Fix TS * Move saveWithConfirmation to a separate file Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
* master: Updating our direct usage of https-proxy-agent to 5.0.0 (elastic#58296) allow users to unset the throttle of an alert (elastic#60964) [Lens] Fix bug in metric config panel (elastic#60982) [SearchProfiler] Minor fixes (elastic#60919) [ML] Renaming ML setup and start contracts (elastic#60980) introduce StartServicesAccessor type for `CoreSetup.getStartServices` (elastic#60748) [SIEM][Detection Engine] Add rule's notification alert type (elastic#60832) [APM] Re-revert "Collect telemetry about data/API performance" (elastic#61030) [NP] Graph: get rid of saved objects class wrapper (elastic#59917) [EPM] merge duplicate fields when creating index patterns (elastic#60957) [Uptime] Ml detection of duration anomalies (elastic#59785) [Alerting] removes unimplemented buttons from Alert Details page (elastic#60934) [skip-ci] Fix CODEOWNERS paths for the Pulse team (elastic#60944) [APM] Threshold alerts (elastic#59566) [ML] Add support for percentiles aggregation to Transform wizard (elastic#60763) Cahgen save object duplicate message (elastic#60901)
Part of #46435 and #58243.
The main purpose is to decouple Graph from a saved object loader and a class wrapping the core saved object client in favor of using core saved object client directly.
So
createSavedWorkspacesLoaderandcreateSavedWorkspaceClasswere replaced by helper functions in newsaved_workspace_utils.tswhich usesavedObjectsClientdirectly.It turned out that much functionality of
SavedObjectClassisn't needed for Graph. And now Graph is responsible for saving saved object including check for duplicate title. And for this casesaveWithConfirmationfunction was created (it's decoupled from core SavedObject).Checklist
Delete any items that are not applicable to this PR.
[ ] This was checked for keyboard-only and screenreader accessibility[ ] This was checked for cross-browser compatibility, including a check against IE11