[Dashboard First] Lens By Value #70272

ThomThomson · 2020-06-29T22:45:25Z

Summary

This PR allows for Lens embeddables to be created and edited from a dashboard 'by value'. In concert with #72256, a faster work flow for dashboards is created, and the number of savedObjects can be greatly reduced.

This PR also allows for editing lens embeddables by value.

NEW: This PR also works with the Add to Library action, and the Unlink from Library action.

How to test this?

This is just one part of a much larger change, so all changes in this PR are hidden behind a configuration value. In order for the 'by value' workflow to be made default, some architectural changes need to be completed including #67931, #71499, and #61663

In order to test the new behaviour you'll need to set the allowByValueEmbeddables config value to true:

kibana/src/plugins/dashboard/config.ts

Line 23 in 5b64a4c

allowByValueEmbeddables: schema.boolean({ defaultValue: false }),

This PR also makes use of the attribute service code found in #68719.

New Functionality & Tests to Cover

Creating a lens visualization via dashboard 'create new' button will now show the 'save and return' button. Save and return will add the lens visualization to the dashboard by value. 'save as' will prompt for a name, then add a 'by reference' panel to the dashboard.
Creating/editing a lens visualization through the visualize app will only show the original 'save' button.
Editing a lens embeddable by value should redirect to the edit_by_value endpoint. Save and return should properly apply edits back to the dashboard.
When editing a lens visualization by value running save as without add to dashboards on save should redirect the editor to /edit/{savedObjectId}
When editing a lens visualization by value running save as with add to dashboards on save should return to dashboard and replace the by value embeddable with a by reference embeddable. The embeddableId should be persisted.
Editing a lens embeddable by reference should work exactly as before.

Technical Diagram

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, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

…yValue

…s button

…yValue

src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx

…yValue

…nd re-rendering

…yValue

…o avoid confusion. Added cancel button

…yValue

majagrubic · 2020-08-21T07:27:43Z

src/plugins/dashboard/public/application/dashboard_app_controller.tsx

-                );
-              }
+            // If the incomingEmbeddable does not yet exist in the panels listing, create a new panel using the container's addEmbeddable method.
+            if (


I already started making this change in implementing attribute service for visualize, so again - the question is who goes first with this 😬

I was hoping to sneak this one in before you started work on using attribute service. Seems like I was too slow on the draw!

majagrubic · 2020-08-21T12:40:08Z

src/plugins/embeddable/public/lib/embeddables/ref_or_val_embeddable/types.ts

+
+import { EmbeddableInput, SavedObjectEmbeddableInput } from '..';
+
+export interface ReferenceOrValueEmbeddable<


why is this showing as a diff? how is this not in master already?

Oh, good catch. Merge issue meant that this was in the wrong place. I've deleted this file.

majagrubic

Tested this in Chrome on Mac OS X. The embeddable - related code looks good to me. I'm not very familiar with the Lens code so would wait for an approval of someone from the Lens team.

flash1293

I focused on the Lens part here and most of the changes make sense to me. I left some small comments.

In terms of behavior, I noticed a custom panel title or time range is overwritten when editing a "by value" lens - seems like there is too much overwritten at some point.

The failing functional test seems like a real problem as well.

In general, it would be cool to have functional tests for the by value flows as well, but those can be added later as well once this moves out of the feature flag.

x-pack/plugins/lens/public/app_plugin/app.test.tsx

x-pack/plugins/lens/public/app_plugin/app.tsx

x-pack/plugins/lens/public/app_plugin/app.test.tsx

x-pack/plugins/lens/public/app_plugin/app.tsx

…for lens menu items

…into feature/lensByValue

…yValue

…ming input to preserve custom panel titles

…yValue

wylieconlon

I haven't reviewed the code fully, but I found bugs in the behavior.

Without the flag enabled:

Steps to reproduce:

Create a new dashboard
Create a Lens visualization for the non-default index pattern, and save it to the new dashboard
The filters in the dashboard will show the wrong field names

With the flag enabled:

Steps to reproduce:

Edit a dashboard with a Lens saved object, and click "Unlink from library"
Edit the Lens visualization that you just unlinked
Make a change and then click "Save and return"
This error appears:

wylieconlon

My highest-level comment about the code is that it's too much complexity spread across a high number of places, and it looks like the extra complexity is not adequately tested because it can't be. A lot of this is due to decisions we made for a lower-complexity app, and are only breaking now.

Since the main change that you've made here is to introduce a new "mode" that affects the way that Lens saving, loading, and state management works, I think the solution lies in also creating a "mode" for the saved objects. Both modes would have the same interface and would be called in a predictable way.

For example, I'm imagining this kind of interface:

interface AppState {
  mode: 'document' | 'value';
}

interface LensEditingMode {
  load: () => Document;
  save: () => Document;
  ...
}

x-pack/plugins/lens/kibana.json

wylieconlon · 2020-08-25T21:25:21Z

x-pack/plugins/lens/common/constants.ts

-export function getEditPath(id: string) {
-  return `#/edit/${encodeURIComponent(id)}`;
+export function getEditPath(id: string | undefined) {
+  return id ? `#/edit/${encodeURIComponent(id)}` : `#/${LENS_EDIT_BY_VALUE}`;


Was there a test for this? Should there be now?

x-pack/plugins/lens/public/app_plugin/app.test.tsx

x-pack/plugins/lens/public/app_plugin/app.tsx

wylieconlon · 2020-08-25T22:37:48Z

x-pack/plugins/lens/public/editor_frame_service/embeddable/embeddable.tsx

+      type: this.type,
+      savedObjectId: (input as LensByReferenceInput)?.savedObjectId,
+    };
+    const expression = await this.deps.documentToExpression(this.savedVis);


Maybe you can simplify the whole thing by changing the signature of documentToExpression?

wylieconlon · 2020-08-25T22:38:27Z

x-pack/plugins/lens/public/editor_frame_service/embeddable/embeddable.tsx

@@ -144,9 +167,7 @@ export class Embeddable extends AbstractEmbeddable<LensEmbeddableInput, LensEmbe
        filters: cleanedFilters,
      };


This is important to set on the first render, it seems like it isn't?

x-pack/plugins/lens/public/editor_frame_service/embeddable/embeddable.tsx

x-pack/plugins/lens/public/editor_frame_service/embeddable/embeddable_factory.ts

examples/embeddable_examples/public/book/book_embeddable.tsx

src/plugins/dashboard/public/application/dashboard_app_controller.tsx

…yValue

kibanamachine · 2020-08-31T21:22:13Z

💚 Build Succeeded

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

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
visualize	310	-5	315

async chunks size

id	value	diff	baseline
lens	30.5KB	+4.6KB	25.9KB
maps	3.3MB	+85.0B	3.3MB
visualize	690.6KB	-3.2KB	693.8KB
total		+1.5KB

page load bundle size

id	value	diff	baseline
dashboard	704.1KB	+1.9KB	702.2KB
embeddable	430.5KB	-49.0B	430.5KB
lens	874.4KB	+7.0KB	867.3KB
total		+8.9KB

History

💔 Build #71376 failed 6f28a7f
💚 Build #70318 succeeded d0e3fd8
💔 Build #70279 failed 566500c
💔 Build #70275 failed 3470c63
💚 Build #70160 succeeded 719d67c

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

ThomThomson · 2020-09-23T17:03:59Z

Closed in favour of #77561

ThomThomson added 7 commits June 25, 2020 12:39

Made lens work with the attribute service copied from elastic#68719

297dba0

Merge branch 'master' of github.com:elastic/kibana into feature/lensB…

168825a

…yValue

Editing lens by value now works.

0039a97

Merge branch 'master' of github.com:elastic/kibana into feature/lensB…

8ec8020

…yValue

Allow by value lens embeddable to be made by reference via the save a…

681625f

…s button

Merge branch 'master' of github.com:elastic/kibana into feature/lensB…

473a052

…yValue

fixed up some jest tests

3ee8263

ThomThomson added Feature:Dashboard Dashboard related features Feature:Embedding Embedding content via iFrame release_note:roadmap v8.0.0 Feature:Lens v7.10.0 labels Jun 29, 2020

functional and jest test fixes

a34d585

ThomThomson force-pushed the feature/lensByValue branch from 55b68a0 to a34d585 Compare June 30, 2020 17:17

ThomThomson added 2 commits June 30, 2020 13:28

Merge branch 'master' of github.com:elastic/kibana into feature/lensB…

b7ff957

…yValue

fix for visualize by value

5d05ac9

ThomThomson added the release_note:enhancement label Jun 30, 2020

ThomThomson mentioned this pull request Jun 30, 2020

Embeddable Panel is not reactive to Input Updates #70381

Closed

Merge branch 'master' of github.com:elastic/kibana into feature/lensB…

adc1298

…yValue

majagrubic reviewed Jul 8, 2020

View reviewed changes

src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx Outdated Show resolved Hide resolved

ThomThomson added a commit to ThomThomson/kibana that referenced this pull request Jul 8, 2020

Moved addOrUpdateEmbeddable and its usages from elastic#70272

f5a3135

ThomThomson mentioned this pull request Jul 8, 2020

Dashboard add or update panel #71130

Merged

1 task

ThomThomson added 7 commits July 13, 2020 14:52

Merge branch 'master' of github.com:elastic/kibana into feature/lensB…

a3d0adc

…yValue

Restructured adding and editing to minimize deletion of embeddables a…

3ecb3d9

…nd re-rendering

eslint changes

72a068a

cleanup continued...

d1ab683

Merge branch 'master' of github.com:elastic/kibana into feature/lensB…

9cb9a7c

…yValue

Finished cleanup - separated embeddableId and savedObjectId in lens t…

a0fffd0

…o avoid confusion. Added cancel button

Merge branch 'master' of github.com:elastic/kibana into feature/lensB…

4d64d5e

…yValue

Test fix

f497244

ThomThomson force-pushed the feature/lensByValue branch from e94152d to f497244 Compare August 20, 2020 21:33

majagrubic reviewed Aug 21, 2020

View reviewed changes

majagrubic approved these changes Aug 21, 2020

View reviewed changes

flash1293 reviewed Aug 21, 2020

View reviewed changes

ThomThomson added 12 commits August 21, 2020 12:43

Removed accidental duplication of ref_or_val_embeddable. Added tests …

5b6d397

…for lens menu items

M o n s t e r M e r g e branch 'master' of github.com:elastic/kibana …

d345571

…into feature/lensByValue

Made attribute service unwrap references as well

0dee8a7

type and jest fix

12b9aa4

Merge branch 'master' of github.com:elastic/kibana into feature/lensB…

e3dc91a

…yValue

Simplified and separated redirectTo method, made dashboard merge inco…

29ae393

…ming input to preserve custom panel titles

Merge branch 'master' of github.com:elastic/kibana into feature/lensB…

719d67c

…yValue

Merge branch 'master' of github.com:elastic/kibana into feature/lensB…

3470c63

…yValue

changed feature flag config to match dashboard

566500c

cleaned up types after 75560

8796ab0

Merge branch 'master' of github.com:elastic/kibana into feature/lensB…

216fd7f

…yValue

Type cleanup continued

d0e3fd8

wylieconlon suggested changes Aug 25, 2020

View reviewed changes

ThomThomson mentioned this pull request Aug 27, 2020

[Dashboard First] Library Notification #76122

Merged

3 tasks

dej611 reviewed Aug 31, 2020

View reviewed changes

examples/embeddable_examples/public/book/book_embeddable.tsx Show resolved Hide resolved

src/plugins/dashboard/public/application/dashboard_app_controller.tsx Show resolved Hide resolved

ThomThomson added 2 commits August 31, 2020 13:57

Merge branch 'master' of github.com:elastic/kibana into feature/lensB…

6f28a7f

…yValue

update eslint

cf74955

ThomThomson mentioned this pull request Sep 15, 2020

[Time to Visualize] Lens By Value With AttributeService #77561

Merged

7 tasks

ThomThomson closed this Sep 23, 2020

ThomThomson reopened this Sep 23, 2020

ThomThomson closed this Sep 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dashboard First] Lens By Value #70272

[Dashboard First] Lens By Value #70272

ThomThomson commented Jun 29, 2020 •

edited

Loading

majagrubic Aug 21, 2020

ThomThomson Aug 21, 2020

majagrubic Aug 21, 2020

ThomThomson Aug 21, 2020

majagrubic left a comment

flash1293 left a comment •

edited

Loading

wylieconlon left a comment •

edited

Loading

wylieconlon left a comment

wylieconlon Aug 25, 2020

wylieconlon Aug 25, 2020

wylieconlon Aug 25, 2020

kibanamachine commented Aug 31, 2020

ThomThomson commented Sep 23, 2020 •

edited

Loading


		import { EmbeddableInput, SavedObjectEmbeddableInput } from '..';

		export interface ReferenceOrValueEmbeddable<

		@@ -144,9 +167,7 @@ export class Embeddable extends AbstractEmbeddable<LensEmbeddableInput, LensEmbe
		filters: cleanedFilters,
		};

[Dashboard First] Lens By Value #70272

[Dashboard First] Lens By Value #70272

Conversation

ThomThomson commented Jun 29, 2020 • edited Loading

Summary

How to test this?

New Functionality & Tests to Cover

Technical Diagram

Checklist

For maintainers

majagrubic Aug 21, 2020

Choose a reason for hiding this comment

ThomThomson Aug 21, 2020

Choose a reason for hiding this comment

majagrubic Aug 21, 2020

Choose a reason for hiding this comment

ThomThomson Aug 21, 2020

Choose a reason for hiding this comment

majagrubic left a comment

Choose a reason for hiding this comment

flash1293 left a comment • edited Loading

Choose a reason for hiding this comment

wylieconlon left a comment • edited Loading

Choose a reason for hiding this comment

wylieconlon left a comment

Choose a reason for hiding this comment

wylieconlon Aug 25, 2020

Choose a reason for hiding this comment

wylieconlon Aug 25, 2020

Choose a reason for hiding this comment

wylieconlon Aug 25, 2020

Choose a reason for hiding this comment

kibanamachine commented Aug 31, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

async chunks size

page load bundle size

History

ThomThomson commented Sep 23, 2020 • edited Loading

ThomThomson commented Jun 29, 2020 •

edited

Loading

flash1293 left a comment •

edited

Loading

wylieconlon left a comment •

edited

Loading

ThomThomson commented Sep 23, 2020 •

edited

Loading