-
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
[ContentManagement] Fix Visualize List search and CRUD operations via CM #165485
Changes from 18 commits
1432cce
8663aef
5b43067
93b422b
d4c38d2
7dc072b
9507501
9bb387b
f337a34
dd26a3d
4202269
b52dc99
4b808e0
5d08c70
1fb3457
016edec
e2d9b92
3bd5cc0
7870b82
d9b0427
6c1c096
2bdff27
d8bd1fc
255e766
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,6 +66,13 @@ const deleteVisualization = async (id: string) => { | |
}; | ||
|
||
const search = async (query: SearchQuery = {}, options?: VisualizationSearchQuery) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the return type of this method account for that it could return There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not specifically the types of those, but all adhere to a generic interface defined https://github.com/elastic/kibana/pull/165485/files#diff-72eb8e94b67c0c56b5744ac732c6164a66b68e6e4a6fb1857da4d361acb6b149R52 Having the exact types returned by this method, from my understanding, requires either a type cast or a full refactor of the architecture, as types are registered dynamically to Visualizations and there's no static way to detect the correct type. |
||
if (options && options.types && options.types.length > 1) { | ||
const { types } = options; | ||
return getContentManagement().client.mSearch<VisualizationSearchOut['hits'][number]>({ | ||
contentTypes: types.map((type) => ({ contentTypeId: type })), | ||
query, | ||
}); | ||
} | ||
return getContentManagement().client.search<VisualizationSearchIn, VisualizationSearchOut>({ | ||
contentTypeId: 'visualization', | ||
query, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,5 +22,5 @@ export type { | |
LensSearchIn, | ||
LensSearchOut, | ||
LensSearchQuery, | ||
Reference, | ||
LensCrudTypes, | ||
} from './types'; |
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.
why is this needed ? in the old times we used this as we didnt have the update operation, but now we have full crud operation set.
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 is used by all visualization types but maps right now. I had to introduce it to
maps
in order to make it work within the metadata editing in visualize Listing page.Other visualizations were using the
CreateOptions
type on Update in order to validate/pass theoverwrite
flag.I can see how this can be removed, but I would like to move it in a follow up PR unless it's strictly required.
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.
here is some context where a related bug in Lens was fixed #160116 (comment)
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'd like a more complete explanation of this which might be best done over zoom since its modifying api types.