[Saved Object Finder] Move to the component in saved_objects_finder plugin #151764

majagrubic · 2023-02-21T17:39:58Z

Summary

This PR replaces the SavedObjectsFinder component from saved_objects plugin with the one in saved_objects_finder plugin. Code changes are pretty straightforward; most of the changes are around modifying the functional tests to work with the selectors from the new component. This is a part of making the component BWCA compliant effort

Checklist

Delete any items that are not applicable to this PR.

~~- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~- [ ] Documentation was added for features that require explanation or tutorials~~

Unit or functional tests were updated or added to match the most common scenarios
~~- [ ] Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)~~
~~- [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)~~
~~- [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list~~
~~- [ ] This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)~~
~~- [ ] This was checked for cross-browser compatibility~~!

For maintainers

This was checked for breaking API changes and was labeled appropriately

…fix'

## Summary This PR is a part of standardizing the `SavedObjectsFinder` component across our codebase. This PR moves the server-side logic from `saved_objects` plugin to `saved_objects_finder` plugin and removes the deprecated `savedObjects.client.find` from the `saved_objects_finder` component. The component in `saved_objects_finder` plugin supports tagging, while the one in `saved_objects` does not. Ideally, we'd move to using that component everywhere. I started working on moving to this component in the follow-up [PR](#151764). ### Checklist Delete any items that are not applicable to this PR. - [X] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) ~- [] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials~ - [X] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ~- [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/))~ ~- [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))~ ~- [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)~ ~- [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))~ ~- [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers)~ ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: kibanamachine <[email protected]>

…fix'

src/plugins/dashboard/public/dashboard_container/embeddable/api/add_panel_from_library.ts

...ugins/dashboard/public/services/saved_objects_management/saved_objects_management_service.ts

…fix'

davismcphee · 2023-02-27T18:35:54Z

src/plugins/saved_objects_finder/public/finder/saved_object_finder.tsx

@@ -257,7 +259,7 @@ export class SavedObjectFinderUi extends React.Component<

              return currentSavedObjectMetaData?.name ?? '';
            },
-            'data-test-subj': 'savedObjectFinderType',
+            'data-test-subj': 'savedObjectFinderFilterButton',


Suggested change

'data-test-subj': 'savedObjectFinderFilterButton',

'data-test-subj': 'savedObjectFinderType',

As far as I can tell, this selector isn't referenced anywhere else in code anymore. Could we revert it back to the previous data-test-subj since it makes more sense in the context of this component?

It is referenced in functional tests: https://github.com/elastic/kibana/blob/main/test/examples/embeddables/adding_children.ts#L26

I can change it back, but thought for the sake of minimizing changes to the functional tests, it made more sense to change the dataTestSubj in the component itself.

It looks like that may have been the only functional that referenced it and it was removed as part of this PR: https://github.com/elastic/kibana/pull/151764/files#diff-efccbf6e6131341694eaf2e5b00ae0160216efdb522c63a5f098ac041312ed08R46-R47.

If I'm missing others though, feel free to disregard. I just couldn't find any other references searching in VS Code.

davismcphee · 2023-02-27T18:39:33Z

src/plugins/saved_objects_finder/public/finder/saved_object_finder.tsx

    };
-    const response = (await this.props.services.http.get('/internal/saved-objects-finder/find', {
+
+    const { queryText, visibleTypes, selectedTags } = savedObjectsManagement.parseQuery(


I don't understand the impact of these changes -- it seems to be the same logic as before but with some of the params props assigned on separate lines now. Am I missing something here?

Ah, fair point. This is a leftover from when I wanted to make savedObjectsManagement an optional dependency, so the parameters could have been assigned with or without it. But since now it's a required dependency, will change it 👍

davismcphee · 2023-02-27T18:44:57Z

src/plugins/saved_objects_finder/public/finder/saved_object_finder.tsx

+    params.search = queryText ? `${queryText}*` : undefined;
+    params.hasReference = hasReference ? JSON.stringify(hasReference) : undefined;
+
+    const response = (await http.get('/internal/saved-objects-finder/find', {
      query: params as Record<string, any>,
    })) as FindResponseHTTP<FinderAttributes>;


As a follow up to my comments on the last PR, could we remove the local FinderAttributes interface and import from common instead to avoid the duplicate definition?

davismcphee · 2023-02-27T18:49:10Z

src/plugins/saved_objects_finder/public/finder/saved_object_finder.tsx

@@ -369,6 +371,7 @@ export class SavedObjectFinderUi extends React.Component<
        itemId="id"
        items={this.state.items}
        columns={columns}
+        data-test-subj="savedObjectsFinder-table"


Suggested change

data-test-subj="savedObjectsFinder-table"

data-test-subj="savedObjectsFinderTable"

Nit: We don't have to change this if you feel strongly about it, but I just found it a little odd that this is the only test subject in the component that doesn't adhere to the style guide.

davismcphee · 2023-02-27T18:59:26Z

src/plugins/saved_objects_finder/public/finder/index.tsx

+  savedObjectsTagging?: SavedObjectsTaggingApi
+) => {
+  return (props: SavedObjectFinderProps) => (
+    <SavedObjectFinderUi


Suggested change

<SavedObjectFinderUi

<SavedObjectFinder

Any chance we could use the lazy loaded component here instead to avoid the hit to page load bundles?

davismcphee · 2023-02-27T19:02:46Z

src/plugins/saved_objects_finder/public/index.ts

@@ -8,6 +8,6 @@

 import { SavedObjectsFinderPublicPlugin } from './plugin';
 export type { SavedObjectMetaData, SavedObjectFinderProps } from './finder';
-export { SavedObjectFinder } from './finder';
+export { SavedObjectFinder, SavedObjectFinderUi, getSavedObjectFinder } from './finder';


Suggested change

export { SavedObjectFinder, SavedObjectFinderUi, getSavedObjectFinder } from './finder';

export { SavedObjectFinder, getSavedObjectFinder } from './finder';

Similar to my other comment about lazy loading, but are we able to just use the lazy loaded component instead of including this export which adds ~4kb to the page load bundle?

src/plugins/dashboard/public/dashboard_container/embeddable/api/add_panel_from_library.ts

test/functional/services/dashboard/add_panel.ts

nreese

kibana-presentation changes LGTM. Thanks for making SavedObjectsFinderService and consolidating all of the logic there.

davismcphee · 2023-03-01T00:41:13Z

src/plugins/saved_objects_finder/public/finder/saved_object_finder.tsx

@@ -257,7 +259,7 @@ export class SavedObjectFinderUi extends React.Component<

              return currentSavedObjectMetaData?.name ?? '';
            },
-            'data-test-subj': 'savedObjectFinderType',
+            'data-test-subj': 'savedObjectFinderFilterButton',


It looks like that may have been the only functional that referenced it and it was removed as part of this PR: https://github.com/elastic/kibana/pull/151764/files#diff-efccbf6e6131341694eaf2e5b00ae0160216efdb522c63a5f098ac041312ed08R46-R47.

If I'm missing others though, feel free to disregard. I just couldn't find any other references searching in VS Code.

davismcphee · 2023-03-01T00:46:23Z

src/plugins/saved_objects_finder/public/finder/index.tsx

 import type { SavedObjectFinderProps } from './saved_object_finder';
+import SavedObjectFinderUi from './saved_object_finder';


I'm making this difficult, sorry 😅. It looks like there's still a 4kb bundle increase and I think it's because of this import which is no longer used since we switched to the lazy one for getSavedObjectFinder. Could we remove this import and its export below and see if that fixes the bundle size increase? If it doesn't fix it, I'll give up and approve anyway -- just want to see if that does it first.

I think it should be fine now

kibana-ci · 2023-03-01T10:12:29Z

💚 Build Succeeded

Buildkite Build
Commit: 8565fad

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`dashboard`	376	377	+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
`embeddable`	430	431	+1
`savedObjectsFinder`	39	44	+5
total			+6

Async chunks

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

id	before	after	diff
`canvas`	1.0MB	1.0MB	+119.0B
`dashboard`	385.3KB	385.5KB	+188.0B
`savedObjectsFinder`	4.9KB	4.9KB	-35.0B
total			+272.0B

Page load bundle

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

id	before	after	diff
`canvas`	13.3KB	13.3KB	+6.0B
`dashboard`	26.3KB	26.4KB	+148.0B
`embeddable`	74.6KB	74.7KB	+190.0B
`savedObjectsFinder`	3.1KB	3.3KB	+227.0B
total			+571.0B

Unknown metric groups

API count

id	before	after	diff
`embeddable`	532	533	+1
`savedObjectsFinder`	39	44	+5
total			+6

ESLint disabled line counts

id	before	after	diff
`securitySolution`	428	430	+2

Total ESLint disabled count

id	before	after	diff
`securitySolution`	506	508	+2

History

💚 Build #111043 succeeded 318d2bc
💔 Build #111033 failed 079f376
💔 Build #111005 failed aaf7209
💔 Build #110989 failed a74490f
💚 Build #110773 succeeded 882e22a
💚 Build #110668 succeeded d37e628

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

davismcphee

No more bundle size increase 🙌 Thanks for making those changes! Pulled and tested locally and everything seems to be working as expected, LGTM.

…lugin (elastic#151764) ## Summary This PR replaces the `SavedObjectsFinder` component from `saved_objects` plugin with the one in `saved_objects_finder` plugin. Code changes are pretty straightforward; most of the changes are around modifying the functional tests to work with the selectors from the new component. ### Checklist Delete any items that are not applicable to this PR. ~- [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)~ ~- [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials~ - [X] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ~- [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/))~ ~- [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))~ ~- [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)~ ~- [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))~ ~- [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers)~! ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: kibanamachine <[email protected]>

Maja Grubic and others added 7 commits February 16, 2023 17:46

[SavedObjectsFinder] Replace savedObjects.client.find

1793fb4

Move server-side logic to saved_objects_finder plugin

14041e6

Fix type

9ddeec1

Fix translation

c286b73

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

5480698

…fix'

Merge branch 'main' into so-finder-2

afc785c

[Saved Object Finder] Standardize the calls

f2191ab

majagrubic mentioned this pull request Feb 21, 2023

[SavedObjectsFinder] Replace savedObjects.client.find #151549

Merged

3 tasks

Change data-test-subj

bc34f07

majagrubic force-pushed the so-finder-3 branch from e20d1ae to bc34f07 Compare February 22, 2023 08:19

Maja Grubic added 2 commits February 23, 2023 12:21

Fix rendering test

ae9fa26

Merge main

67a8706

majagrubic force-pushed the so-finder-3 branch from f47ad16 to 67a8706 Compare February 23, 2023 11:40

Fix merge conflicts

39ec7a2

majagrubic force-pushed the so-finder-3 branch from b9b4211 to 39ec7a2 Compare February 23, 2023 12:09

Fix typescript

2a64434

majagrubic force-pushed the so-finder-3 branch from ae1658e to 2a64434 Compare February 23, 2023 13:14

Add missing types

c11b0f7

majagrubic force-pushed the so-finder-3 branch from f6855f3 to c11b0f7 Compare February 23, 2023 14:18

Add missing stub

0db1f29

majagrubic force-pushed the so-finder-3 branch from 20649dd to 0db1f29 Compare February 23, 2023 15:23

Fix functional tests

f123ece

majagrubic force-pushed the so-finder-3 branch from 191d856 to f123ece Compare February 23, 2023 21:40

Merge branch 'main' into so-finder-3

1ce60a4

majagrubic force-pushed the so-finder-3 branch from 407a6a7 to 1ce60a4 Compare February 24, 2023 07:57

kibanamachine and others added 4 commits February 24, 2023 08:03

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

f74971e

…fix'

Fix rendering test

ea2c057

Remove sleep

c4159e8

Update test

08c2d9e

majagrubic marked this pull request as ready for review February 25, 2023 14:19

majagrubic requested review from a team as code owners February 25, 2023 14:19

Merge branch 'main' into so-finder-3

d37e628

botelastic bot added the Feature:Embedding Embedding content via iFrame label Feb 27, 2023

nreese reviewed Feb 27, 2023

View reviewed changes

src/plugins/dashboard/public/dashboard_container/embeddable/api/add_panel_from_library.ts Outdated Show resolved Hide resolved

...ugins/dashboard/public/services/saved_objects_management/saved_objects_management_service.ts Outdated Show resolved Hide resolved

Maja Grubic and others added 2 commits February 27, 2023 18:25

Change DashboardService > SavedObjectsManagementPluginStart

c7713ef

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

882e22a

…fix'

majagrubic requested a review from davismcphee February 27, 2023 19:06

davismcphee reviewed Feb 27, 2023

View reviewed changes

nreese reviewed Feb 27, 2023

View reviewed changes

src/plugins/dashboard/public/dashboard_container/embeddable/api/add_panel_from_library.ts Outdated Show resolved Hide resolved

test/functional/services/dashboard/add_panel.ts Outdated Show resolved Hide resolved

test/functional/services/dashboard/add_panel.ts Outdated Show resolved Hide resolved

Maja Grubic added 6 commits February 28, 2023 10:42

Lazy loading components

4314b87

Extract common logic to service

a74490f

Fix test

4d70c00

Extract common functionality to service

aaf7209

Fix missing import

079f376

Fix test

318d2bc

majagrubic requested review from nreese and davismcphee February 28, 2023 14:41

nreese approved these changes Feb 28, 2023

View reviewed changes

davismcphee reviewed Mar 1, 2023

View reviewed changes

Change selector

8565fad

davismcphee approved these changes Mar 1, 2023

View reviewed changes

majagrubic merged commit d4abfa2 into elastic:main Mar 1, 2023

kibanamachine added the backport:skip This commit does not require backporting label Mar 1, 2023

majagrubic deleted the so-finder-3 branch March 1, 2023 21:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Saved Object Finder] Move to the component in saved_objects_finder plugin #151764

[Saved Object Finder] Move to the component in saved_objects_finder plugin #151764

majagrubic commented Feb 21, 2023 •

edited

Loading

davismcphee Feb 27, 2023

majagrubic Feb 28, 2023

davismcphee Mar 1, 2023

davismcphee Feb 27, 2023

majagrubic Feb 28, 2023

davismcphee Feb 27, 2023

davismcphee Feb 27, 2023

davismcphee Feb 27, 2023

davismcphee Feb 27, 2023

nreese left a comment

davismcphee Mar 1, 2023

davismcphee Mar 1, 2023

majagrubic Mar 1, 2023

kibana-ci commented Mar 1, 2023

API count

ESLint disabled line counts

Total ESLint disabled count

davismcphee left a comment

	'data-test-subj': 'savedObjectFinderFilterButton',
	'data-test-subj': 'savedObjectFinderType',

	data-test-subj="savedObjectsFinder-table"
	data-test-subj="savedObjectsFinderTable"

	export { SavedObjectFinder, SavedObjectFinderUi, getSavedObjectFinder } from './finder';
	export { SavedObjectFinder, getSavedObjectFinder } from './finder';

		import type { SavedObjectFinderProps } from './saved_object_finder';
		import SavedObjectFinderUi from './saved_object_finder';

[Saved Object Finder] Move to the component in saved_objects_finder plugin #151764

[Saved Object Finder] Move to the component in saved_objects_finder plugin #151764

Conversation

majagrubic commented Feb 21, 2023 • edited Loading

Summary

Checklist

For maintainers

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Mar 1, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

ESLint disabled line counts

Total ESLint disabled count

History

davismcphee left a comment

Choose a reason for hiding this comment

majagrubic commented Feb 21, 2023 •

edited

Loading