[Dashboard First] Decouple Attribute Service and By Value Embeddables #74302

ThomThomson · 2020-08-04T20:49:34Z

Summary

This PR accomplishes 3 things:

It decouples the attribute service from the concept of 'by value' or 'by reference' embeddables. For this, it includes an interface, ReferenceOrValueEmbeddable, which can be used by actions such as the unlink from library action [Dashboard First] Unlink from Library Action #73899 and the future add to library action. Implementing the interface, will allow any embeddable to opt into the embeddables by value paradigm. Upon implementation, embeddable authors could either use the attribute_service which requires their input to be in a specific format, or implement their interface in any other way.
It moves the Attribute_service out of the embeddable plugin and in to the dashboard plugin in order to avoid coupling embeddables and saved objects.
It contains an example implementation of the ReferenceOrValueEmbeddable, in the BookEmbeddable, and two example actions for adding a book to the library and unlinking a book from its library item.

This PR closes #74297

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
This was checked for keyboard-only and screenreader accessibility
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

…can be treated as either by reference or by value

…tions to test the referenceOrValueEmbeddable interface.

…enceOrvalueInterface

elasticmachine · 2020-08-10T15:41:12Z

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

majagrubic

Looks great! Just a few small questions.

majagrubic · 2020-08-10T15:14:10Z

src/plugins/dashboard/public/attribute_service/attribute_service.tsx

+
+  public async wrapAttributes(
+    newAttributes: SavedObjectAttributes,
+    useRefType: boolean,


does this need to be an explicit parameter? can we somehow figure this from attributes?

The wrap attributes method on the attribute service is meant to transform given attributes in the exact same shape into either of the two input types. This means that somewhere there has to be an explicit switch to determine which one to use. This could potentially be split into two methods, but that would only shift the logic to the editor.

src/plugins/dashboard/public/attribute_service/attribute_service.tsx

majagrubic · 2020-08-10T15:46:19Z

src/plugins/dashboard/public/attribute_service/attribute_service.tsx

+      const onSave = async (props: OnSaveProps): Promise<SaveResult> => {
+        try {
+          const wrappedInput = (await this.wrapAttributes(input.attributes, true)) as RefType;
+          wrappedInput.title = props.newTitle;


why is this assignment needed?

The title is assigned here because adding an embeddable to the library via the attribute service shows a save modal with the opportunity to name (or re-name) the saved object.

It is not a saved object rename, but just embeddable panel title change? isn't it?
Is this expected?

This modal shows up when adding an item to the library. That item (when it is by value) usually doesn't yet have a saved object title.

When the item already has a saved object title, like in a case where it was previously in the library, you are given the option to change the name. An improvement here would be duplicate detection, which will be added in the follow up PR which adds the actual 'add to library' action.

For book embeddables, the titling is a little strange, which is what you're picking up on I believe. They have a saved Object title input.title and a book title input.attributes.title. In the add flyout, they are referred to by their book title.

Edit: The title actually should be taken from the savedObjectAttributes. I have pushed an update that makes this much more aligned.

majagrubic

Tested this on an example scenario, works as expected. I really like moving attributeService away from embeddable plugin. I am not sure dashboard plugin is the right place to put in though. But for now, since we don't plan to use it outside the dashboard, it should be fine.

ThomThomson · 2020-08-11T14:53:49Z

I agree about the dashboard plugin not being a permanent home for attributeService. In the future, it might be worth creating a new plugin for all of the 'by reference or by value' code. Other than that, I can't think of a better place for now.

…enceOrvalueInterface

Dosant

LGTM, tested example plugin.
One question I am curious about: #74302 (comment)

ppisljar

code LGTM

kibanamachine · 2020-08-12T19:27:04Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: ed0cb93

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
dashboard	174	+1	173
embeddable	92	+1	91
total		+2

page load bundle size

id	value	diff	baseline
dashboard	687.7KB	+8.3KB	679.4KB
embeddable	427.2KB	-1.5KB	428.7KB
total		+6.8KB

History

💚 Build #67537 succeeded 3586923
💚 Build #67341 succeeded 230341d
💚 Build #67245 succeeded 98f3e56
💔 Build #67035 failed e8abb12
💚 Build #66662 succeeded 6ba38d0

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

…elastic#74302) * Added an interface that determines if an embeddable can be treated as either by reference or by value

…#74302) (#74895) * Added an interface that determines if an embeddable can be treated as either by reference or by value

* master: (28 commits) [Task manager] Prevents edge case where already running tasks are reschedule every polling interval (elastic#74606) [Security Solution] Fix the status of timelines' bulk actions (elastic#74560) Data plugin: Suggested enhance pattern (elastic#74505) Use jest.useFakeTimers instead of hard coded timeout for tooltip tests. (elastic#74642) [Security Solution][lists] Adds tests for exception lists and items part 2 (elastic#74815) [Security Solution][Resolver] fix presentation role on edgeline (elastic#74869) [Security Solution][Detections] Refactor ML calls for newest ML permissions (elastic#74582) [bin/kibana-plugin] support KP plugins instead (elastic#74604) Reduce number of indexed fields in index pattern saved object (elastic#74817) [reporting] Pass along generic parameters in high-order route handler (elastic#74892) Migrated last pieces of legacy fixture code (elastic#74470) Empty index patterns page re-design (elastic#68819) [babel] coalese some versions to prevent breaking yarn install (elastic#74864) [Dashboard First] Decouple Attribute Service and By Value Embeddables (elastic#74302) Revert "[reporting] Pass along generic parameters in high-order route handler" (elastic#74891) [reporting] Pass along generic parameters in high-order route handler (elastic#74879) [src/dev/build] implement a getBuildNumber() mock (elastic#74881) [Enterprise Search] Add solution-level side navigation (elastic#74705) [DOCS] Canvas docs 7.9 refresh (elastic#74000) [Security Solution][Resolver]Enzyme test related events closing (elastic#74811) ...

…le-buffer-with-update-of-same-id * upstream/master: (37 commits) [Task manager] Prevents edge case where already running tasks are reschedule every polling interval (elastic#74606) [Security Solution] Fix the status of timelines' bulk actions (elastic#74560) Data plugin: Suggested enhance pattern (elastic#74505) Use jest.useFakeTimers instead of hard coded timeout for tooltip tests. (elastic#74642) [Security Solution][lists] Adds tests for exception lists and items part 2 (elastic#74815) [Security Solution][Resolver] fix presentation role on edgeline (elastic#74869) [Security Solution][Detections] Refactor ML calls for newest ML permissions (elastic#74582) [bin/kibana-plugin] support KP plugins instead (elastic#74604) Reduce number of indexed fields in index pattern saved object (elastic#74817) [reporting] Pass along generic parameters in high-order route handler (elastic#74892) Migrated last pieces of legacy fixture code (elastic#74470) Empty index patterns page re-design (elastic#68819) [babel] coalese some versions to prevent breaking yarn install (elastic#74864) [Dashboard First] Decouple Attribute Service and By Value Embeddables (elastic#74302) Revert "[reporting] Pass along generic parameters in high-order route handler" (elastic#74891) [reporting] Pass along generic parameters in high-order route handler (elastic#74879) [src/dev/build] implement a getBuildNumber() mock (elastic#74881) [Enterprise Search] Add solution-level side navigation (elastic#74705) [DOCS] Canvas docs 7.9 refresh (elastic#74000) [Security Solution][Resolver]Enzyme test related events closing (elastic#74811) ...

Added a first pass for an interface that determines if an embeddable …

14dcd6a

…can be treated as either by reference or by value

ThomThomson added Feature:Dashboard Dashboard related features Feature:Embedding Embedding content via iFrame Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Aug 4, 2020

ThomThomson added 5 commits August 5, 2020 11:03

type fix

6ba38d0

Moved attributeservice to dashboard plugin. Created add and unlink ac…

d21daef

…tions to test the referenceOrValueEmbeddable interface.

Merge branch 'master' of github.com:elastic/kibana into example/refer…

e8abb12

…enceOrvalueInterface

type fix

19b2bc0

Merge branch 'master' of github.com:elastic/kibana into example/refer…

98f3e56

…enceOrvalueInterface

ThomThomson marked this pull request as ready for review August 10, 2020 15:41

ThomThomson requested a review from a team August 10, 2020 15:41

ThomThomson requested a review from a team as a code owner August 10, 2020 15:41

majagrubic reviewed Aug 10, 2020

View reviewed changes

Added error handling to the attribute_service save methods

230341d

majagrubic approved these changes Aug 11, 2020

View reviewed changes

Dosant self-requested a review August 11, 2020 15:38

Merge branch 'master' of github.com:elastic/kibana into example/refer…

3586923

…enceOrvalueInterface

Dosant approved these changes Aug 12, 2020

View reviewed changes

ppisljar approved these changes Aug 12, 2020

View reviewed changes

Sorted out title issue

ed0cb93

ThomThomson merged commit 2e5140d into elastic:master Aug 12, 2020

This was referenced Aug 12, 2020

[7.x] [Dashboard First] Decouple Attribute Service and By Value Embeddables (#74302) #74895

Merged

[Dashboard First] Unlink from Library Action With ReferenceOrValueEmbeddable #74905

Merged

ThomThomson added a commit that referenced this pull request Aug 12, 2020

[Dashboard First] Decouple Attribute Service and By Value Embeddables (…

3d38bab

…#74302) (#74895) * Added an interface that determines if an embeddable can be treated as either by reference or by value

ThomThomson mentioned this pull request Aug 14, 2020

[Dashboard First] Add to Library Action #75098

Merged

2 tasks

This was referenced Sep 14, 2020

Implementing ReferenceOrValueEmbeddable for visualize embeddable #76088

Merged

Find a Permanent Home for the Attribute Service #77505

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dashboard First] Decouple Attribute Service and By Value Embeddables #74302

[Dashboard First] Decouple Attribute Service and By Value Embeddables #74302

ThomThomson commented Aug 4, 2020 •

edited

Loading

elasticmachine commented Aug 10, 2020

majagrubic left a comment

majagrubic Aug 10, 2020

ThomThomson Aug 10, 2020 •

edited

Loading

majagrubic Aug 10, 2020

ThomThomson Aug 10, 2020

Dosant Aug 12, 2020

ThomThomson Aug 12, 2020 •

edited

Loading

majagrubic left a comment

ThomThomson commented Aug 11, 2020

Dosant left a comment

ppisljar left a comment

kibanamachine commented Aug 12, 2020

[Dashboard First] Decouple Attribute Service and By Value Embeddables #74302

[Dashboard First] Decouple Attribute Service and By Value Embeddables #74302

Conversation

ThomThomson commented Aug 4, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Aug 10, 2020

majagrubic left a comment

Choose a reason for hiding this comment

majagrubic Aug 10, 2020

Choose a reason for hiding this comment

ThomThomson Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

majagrubic Aug 10, 2020

Choose a reason for hiding this comment

ThomThomson Aug 10, 2020

Choose a reason for hiding this comment

Dosant Aug 12, 2020

Choose a reason for hiding this comment

ThomThomson Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

majagrubic left a comment

Choose a reason for hiding this comment

ThomThomson commented Aug 11, 2020

Dosant left a comment

Choose a reason for hiding this comment

ppisljar left a comment

Choose a reason for hiding this comment

kibanamachine commented Aug 12, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

page load bundle size

History

ThomThomson commented Aug 4, 2020 •

edited

Loading

ThomThomson Aug 10, 2020 •

edited

Loading

ThomThomson Aug 12, 2020 •

edited

Loading