-
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
[Visualization] Get rid of saved object loader and use savedObjectClient resolve #113121
[Visualization] Get rid of saved object loader and use savedObjectClient resolve #113121
Conversation
src/plugins/visualizations/public/utils/saved_visualize_utils.ts
Outdated
Show resolved
Hide resolved
src/plugins/visualizations/public/utils/saved_visualize_utils.ts
Outdated
Show resolved
Hide resolved
@elasticmachine merge upstream |
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.
Not finished yet, but noticed one thing I want to change in general
@elasticmachine merge upstream |
src/plugins/visualizations/public/utils/saved_visualize_utils.ts
Outdated
Show resolved
Hide resolved
Pinging @elastic/kibana-vis-editors (Team:VisEditors) |
@elasticmachine merge upstream |
This looks mostly good to me. However just yesterday evening a unified error component got merged to guide the user into the right direction to delete the alias: #112606 This is how it looks right now: See for example the usage in Lens:
We can do this on a separate PR to get this one merged, but we should definitely align here. |
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 didn't notice any weird behavior anymore, LGTM
Added some nits about any
s in there (we should not introduce them anymore)
src/plugins/visualize/public/application/utils/get_visualization_instance.test.ts
Outdated
Show resolved
Hide resolved
@elasticmachine merge upstream |
if (savedObject.sharingSavedObjectProps?.outcome === 'conflict') { | ||
return new ErrorEmbeddable( | ||
i18n.translate('visualizations.embeddable.legacyURLConflict.errorMessage', { | ||
defaultMessage: `This visualization has the same URL as a legacy alias. Disable the alias to resolve this error : {json}`, | ||
values: { json: savedObject.sharingSavedObjectProps?.errorJSON }, | ||
}), | ||
input, | ||
parent | ||
); | ||
} |
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.
As @flash1293 mentioned in #113121 (comment), the Spaces plugin now provides a component for this. Whether you want to change it in this PR or leave it for a follow-on PR, I just wanted to leave a note and let you know that I made some additional changes to it in #114172.
You can load this component with spaces.ui.components.getEmbeddableLegacyUrlConflict
; you can see how Lens consumes it here:
kibana/x-pack/plugins/lens/public/embeddable/embeddable.tsx
Lines 283 to 296 in 961fe75
if (sharingSavedObjectProps?.outcome === 'conflict' && this.deps.spaces) { | |
const conflictError = { | |
shortMessage: i18n.translate('xpack.lens.embeddable.legacyURLConflict.shortMessage', { | |
defaultMessage: `You've encountered a URL conflict`, | |
}), | |
longMessage: ( | |
<this.deps.spaces.ui.components.getEmbeddableLegacyUrlConflict | |
targetType={DOC_TYPE} | |
sourceId={sharingSavedObjectProps.sourceId!} | |
/> | |
), | |
}; | |
this.errors = this.errors ? [...this.errors, conflictError] : [conflictError]; | |
} |
The sourceId
is the ID of the saved object.
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
References to deprecated APIs
History
To update your PR or re-run it, just comment with: |
…ent resolve (elastic#113121) * First step: create saved_visualize_utils, starting use new get/save methods * Use new util methods in embeddable * move findListItem in utils * some clean up * clean up * Some fixes * Fix saved object tags * Some types fixes * Fix unit tests * Clean up code * Add unit tests for new utils * Fix lint * Fix tagging * Add unit tests * Some fixes * Clean up code * Fix lint * Fix types * put new methods in start contract * Fix imports * Fix lint * Fix comments * Fix lint * Fix CI * use local url instead of full path * Fix unit test * Some clean up * Fix nits * fix types Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
…ent resolve (#113121) (#114442) * First step: create saved_visualize_utils, starting use new get/save methods * Use new util methods in embeddable * move findListItem in utils * some clean up * clean up * Some fixes * Fix saved object tags * Some types fixes * Fix unit tests * Clean up code * Add unit tests for new utils * Fix lint * Fix tagging * Add unit tests * Some fixes * Clean up code * Fix lint * Fix types * put new methods in start contract * Fix imports * Fix lint * Fix comments * Fix lint * Fix CI * use local url instead of full path * Fix unit test * Some clean up * Fix nits * fix types Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Closes: #105809
Summary
Get rid of
SavedObjectLoader
inVisualization
. This work is done because theSavedObject
class has been deprecated and will be removed inKibana 8
What was done:
visualization
and use it from all external plugins instead ofSavedObjectLoader
savedObject.get
was replaced tosavedObject.resolve
conflict
andaliasMatch