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

[NP] Graph: get rid of saved objects class wrapper #59917

Merged
merged 25 commits into from
Mar 24, 2020
Merged

[NP] Graph: get rid of saved objects class wrapper #59917

merged 25 commits into from
Mar 24, 2020

Conversation

maryia-lapata
Copy link
Contributor

@maryia-lapata maryia-lapata commented Mar 11, 2020 •
edited
Loading

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 createSavedWorkspacesLoader and createSavedWorkspaceClass were replaced by helper functions in new saved_workspace_utils.ts which use savedObjectsClient directly.
It turned out that much functionality of SavedObjectClass isn't needed for Graph. And now Graph is responsible for saving saved object including check for duplicate title. And for this case saveWithConfirmation function was created (it's decoupled from core SavedObject).

Checklist

Delete any items that are not applicable to this PR.

@maryia-lapata maryia-lapata added Feature:Graph Graph application feature Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 labels Mar 11, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@maryia-lapata maryia-lapata added the WIP Work in progress label Mar 11, 2020
@maryia-lapata
Copy link
Contributor Author

@elasticmachine merge upstream

flash1293
flash1293 reviewed Mar 12, 2020
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.

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
src/plugins/saved_objects/public/saved_object/helpers/create_source.ts Outdated
@@ -80,3 +80,44 @@ export async function createSource(
return await Promise.reject(err);
}
}

export async function createSourceUtil(
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
x-pack/legacy/plugins/graph/public/helpers/saved_workspace_utils.ts Outdated Show resolved Hide resolved
@maryia-lapata
Remove applyESRespUtil
29b9b35
@maryia-lapata
Refactoring
14f7d7e
@maryia-lapata
Refactoring
26a7ad7
@maryia-lapata
Remove savedWorkspaceLoader
21088df
@maryia-lapata
Update unit test
890d9c6
@maryia-lapata
Merge branch 'master' into graph_saved_objects
933f3d6 
# 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
@maryia-lapata
Fix merge conflicts
62f91a2
@maryia-lapata
Add unit tests for saveWithConfirmation
2c01845
@flash1293 flash1293 mentioned this pull request Mar 16, 2020
Closed
9 tasks
@maryia-lapata maryia-lapata requested a review from kertal March 16, 2020 17:14
@maryia-lapata maryia-lapata removed the WIP Work in progress label Mar 16, 2020
@maryia-lapata maryia-lapata marked this pull request as ready for review March 16, 2020 17:14
kertal
kertal reviewed Mar 18, 2020
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.

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:

Bildschirmfoto 2020-03-18 um 09 39 38

which is expected
However when I clicked on Save, it wasn't overwritten, but added to the List

Bildschirmfoto 2020-03-18 um 09 40 46

could you investigate? many thx!

src/plugins/saved_objects/public/saved_object/helpers/create_source.ts Outdated Show resolved Hide resolved
@maryia-lapata
Copy link
Contributor Author

@elasticmachine merge upstream

@maryia-lapata
Copy link
Contributor Author

@elasticmachine merge upstream

@maryia-lapata
Copy link
Contributor Author

The issue, when saved object is not overwritten, is reproduced in master. Here is the issue #60735.

@maryia-lapata
Copy link
Contributor Author

@elasticmachine merge upstream

@kertal
Copy link
Member

kertal commented Mar 23, 2020

@elasticmachine merge upstream

@maryia-lapata
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

kertal
kertal approved these changes Mar 24, 2020
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.

Code LGTM! nice cleanup! Tested locally in chrome. works

@maryia-lapata maryia-lapata merged commit 78e0bdf into elastic:master Mar 24, 2020
@maryia-lapata maryia-lapata deleted the graph_saved_objects branch March 24, 2020 12:15
@maryia-lapata maryia-lapata mentioned this pull request Mar 24, 2020
Merged
maryia-lapata added a commit that referenced this pull request Mar 24, 2020
@maryia-lapata @elasticmachine
[NP] Graph: get rid of saved objects class wrapper (#59917) (#61057)
8edfd14 
* 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 <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 24, 2020
@gmmorris
Merge branch 'master' into alerting/tls-warning
bec4553 
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flash1293 flash1293 flash1293 left review comments

@kertal kertal kertal approved these changes

Assignees
No one assigned
Labels
backported Feature:Graph Graph application feature release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.7.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@maryia-lapata @elasticmachine @kertal @kibanamachine @flash1293