[ContentManagement] Fix Visualize List search and CRUD operations via CM #165485

dej611 · 2023-09-01T15:17:45Z

Summary

This PR fixes the CM problems within the Visualize List page leveraging the services already in place.
The approach here is lighter than #165292 as it passes each client via the TypesService already used to register extensions in the Visualization scope. Also the search method now transparently uses the mSearch if more content types are detected without leaking any implementation detail outside the VisualizationClient interface.

More fixes/features:

fixed Maps update operation definition which was missing the overwrite flag
Allow mSearch to accept an options object argument
Added new helper functions to interact with the metadata flyout in Listing page

Checklist

Unit or functional tests were updated or added to match the most common scenarios

…fix/163246-2

elasticmachine · 2023-09-04T10:52:26Z

Pinging @elastic/kibana-visualizations (Team:Visualizations)

Dosant · 2023-09-04T12:33:21Z

src/plugins/content_management/common/rpc/msearch.ts

+export interface MSearchIn<Options extends void | object = object> {
+  contentTypes: Array<{ contentTypeId: string; version?: Version }>;
+  query: MSearchQuery;
+  options?: Options;


I agree that options make sense, but are they actually used anywhere for msearch in this pr? It seems they are not used and, as I understand, they won't work because the implementation of the msearch service wasn't changed to work with options. Or am I missing something?

Looking at the implementation it seems it's dropped. I can drop it here at search wrapping function.

got it. I suggest we revert these changes to msearch interface, because this can't be used in msearch

Dosant · 2023-09-04T12:34:33Z

src/plugins/visualizations/public/content_management/visualization_client.ts

@@ -66,6 +66,14 @@ const deleteVisualization = async (id: string) => {
 };

 const search = async (query: SearchQuery = {}, options?: VisualizationSearchQuery) => {


Does the return type of this method account for that it could return maps and lens objects?

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.

Dosant

I smoked checked the usage on visualize page and it looks good to me. just that comment amount reverting options change #165485 (comment)

great work 👏 thank you!

src/plugins/content_management/common/rpc/msearch.ts

stratoula

Vis team changes LGTM and the bug from the listing page has been fixed 🎉
There is only one unit test failing but I am approving as I dont want to block this PR. Great work Marco ❤️

ppisljar

LGTM, tested in chrome linux

ppisljar · 2023-09-04T11:26:10Z

packages/kbn-content-management-utils/src/types.ts

@@ -161,6 +161,8 @@ export interface SavedObjectSearchOptions {

 /** Saved Object update options  - Pick and Omit to customize */
 export interface SavedObjectUpdateOptions<Attributes = unknown> {
+  /** Overwrite existing documents (defaults to false) */


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.

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 the overwrite 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.

here is some context where a related bug in Lens was fixed #160116 (comment)

I'd like a more complete explanation of this which might be best done over zoom since its modifying api types.

nreese

kibana-gis changes LGTM. Thanks for adding functional test verifying map saved object types can be edited in visualize listing table.
code review, tested in chrome

mattkime · 2023-09-05T19:54:38Z

packages/kbn-content-management-utils/src/types.ts

@@ -161,6 +161,8 @@ export interface SavedObjectSearchOptions {

 /** Saved Object update options  - Pick and Omit to customize */
 export interface SavedObjectUpdateOptions<Attributes = unknown> {
+  /** Overwrite existing documents (defaults to false) */


I'd like a more complete explanation of this which might be best done over zoom since its modifying api types.

mattkime

Overall, looks great, just a question about passing overwrite to to the update method. Its nice that this PR is a net removal of code.

dej611 · 2023-09-08T13:54:21Z

@mattkime as discussed offline I've removed the overwrite from the UpdateOptions and had to make it an appExtension prop which each plugin could set for its own client. Maps code remains unchanged while Lens will export it as it is required on its storage side.

mattkime

changes look good to me, thanks!

kibana-ci · 2023-09-13T16:18:04Z

💚 Build Succeeded

Buildkite Build
Commit: 255e766

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`lens`	1156	1157	+1

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`visualizations`	793	807	+14

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`lens`	1.4MB	1.4MB	-106.0B
`maps`	2.8MB	2.8MB	-1019.0B
`visualizations`	264.5KB	265.2KB	+716.0B
total			-409.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`lens`	36.3KB	36.9KB	+581.0B
`maps`	45.3KB	46.3KB	+1.1KB
`visualizations`	56.9KB	57.1KB	+189.0B
total			+1.8KB

Unknown metric groups

API count

id	before	after	diff
`visualizations`	823	837	+14

History

💚 Build #158264 succeeded d8bd1fc
💚 Build #157880 succeeded 2bdff27
💔 Build #157644 failed 6c1c096
💔 Build #157574 failed d9b0427
💔 Build #156254 failed 3bd5cc0
💔 Build #156240 failed 016edec

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

cc @matthewabbott

dej611 added 6 commits September 1, 2023 16:45

♻️ Refactor client to factory fn

1432cce

✨ Create lens client

8663aef

🔧 Add client prop to type service

5b43067

✅ Add unit and functional tests

93b422b

🐛 Fixes after manual testing

d4c38d2

✨ Add new methods to better interact with listing page

7dc072b

dej611 changed the title ~~[ContentManagement] Fix Visualize List search + CRUD operations via CM~~ [ContentManagement] Fix Visualize List search and CRUD operations via CM Sep 1, 2023

dej611 and others added 8 commits September 1, 2023 17:37

🏷️ Fix type

9507501

🏷️ More type fixes

9bb387b

✅ Fix tests

f337a34

Merge branch 'main' into fix/163246-2

dd26a3d

♻️ Refactor types and storage

4202269

✅ Fix test

b52dc99

Merge remote-tracking branch 'upstream/main' into fix/163246-2

4b808e0

Merge branch 'fix/163246-2' of https://github.com/dej611/kibana into …

5d08c70

…fix/163246-2

dej611 marked this pull request as ready for review September 4, 2023 10:51

dej611 requested review from a team as code owners September 4, 2023 10:51

dej611 added release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens Feature:Content Management User generated content (saved objects) management Feature:Maps v8.11.0 labels Sep 4, 2023

Dosant reviewed Sep 4, 2023

View reviewed changes

Dosant approved these changes Sep 4, 2023

View reviewed changes

♻️ Remove unused options

1fb3457

kertal assigned matthewabbott Sep 4, 2023

mattkime reviewed Sep 4, 2023

View reviewed changes

src/plugins/content_management/common/rpc/msearch.ts Outdated Show resolved Hide resolved

Merge branch 'main' into fix/163246-2

016edec

stratoula approved these changes Sep 5, 2023

View reviewed changes

dej611 and others added 2 commits September 5, 2023 09:24

Update src/plugins/content_management/common/rpc/msearch.ts

e2d9b92

✅ Fix tests

3bd5cc0

ppisljar approved these changes Sep 5, 2023

View reviewed changes

nreese approved these changes Sep 5, 2023

View reviewed changes

dej611 enabled auto-merge (squash) September 5, 2023 14:34

mattkime requested changes Sep 5, 2023

View reviewed changes

dej611 added 3 commits September 8, 2023 12:05

Merge remote-tracking branch 'upstream/main' into fix/163246-2

7870b82

👌 Refactor based on feedback

d9b0427

🐛 Make update overwrite an optional prop owned by each plugin

6c1c096

dej611 added 2 commits September 11, 2023 09:39

Merge branch 'main' into fix/163246-2

2bdff27

Merge branch 'main' into fix/163246-2

d8bd1fc

dej611 requested a review from mattkime September 12, 2023 10:11

mattkime approved these changes Sep 13, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into fix/163246-2

255e766

dej611 merged commit 01ad4c2 into elastic:main Sep 13, 2023

kibanamachine added the backport:skip This commit does not require backporting label Sep 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ContentManagement] Fix Visualize List search and CRUD operations via CM #165485

[ContentManagement] Fix Visualize List search and CRUD operations via CM #165485

dej611 commented Sep 1, 2023

elasticmachine commented Sep 4, 2023

Dosant Sep 4, 2023

dej611 Sep 4, 2023

Dosant Sep 4, 2023 •

edited

Loading

Dosant Sep 4, 2023

dej611 Sep 4, 2023

Dosant left a comment

stratoula left a comment

ppisljar left a comment

ppisljar Sep 4, 2023

dej611 Sep 5, 2023

Dosant Sep 5, 2023

mattkime Sep 5, 2023

nreese left a comment

mattkime Sep 5, 2023

mattkime left a comment

dej611 commented Sep 8, 2023

mattkime left a comment

kibana-ci commented Sep 13, 2023

API count

		@@ -66,6 +66,14 @@ const deleteVisualization = async (id: string) => {
		};

		const search = async (query: SearchQuery = {}, options?: VisualizationSearchQuery) => {

[ContentManagement] Fix Visualize List search and CRUD operations via CM #165485

[ContentManagement] Fix Visualize List search and CRUD operations via CM #165485

Conversation

dej611 commented Sep 1, 2023

Summary

Checklist

elasticmachine commented Sep 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dosant Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dosant left a comment

Choose a reason for hiding this comment

stratoula left a comment

Choose a reason for hiding this comment

ppisljar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nreese left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattkime left a comment

Choose a reason for hiding this comment

dej611 commented Sep 8, 2023

mattkime left a comment

Choose a reason for hiding this comment

kibana-ci commented Sep 13, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

History

Dosant Sep 4, 2023 •

edited

Loading