[Time to Visualize] Lens By Value With AttributeService

[ThomThomson]

This PR is a simpler version of #70272. To standardize the code, the lens editor has been changed to use the attributeService as an additional layer of abstraction above savedObjects for loading and saving.

Summary

This PR is a part of the Time to Visualize initiative. It allows for Lens embeddables to be created and edited 'by value'. This results in less clicks to create, and edit visualizations, and avoids the slow 'think up a unique name, then search for it later to add the panel to a dashboard` process. In concert with #72256, and other visualize by value PRs a faster work flow for dashboards is created, and the number of savedObjects can be greatly reduced.

How should I review 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

Config

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 }),

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.
• Any time you have decided to remain in the editor via 'save as' the originating app connection should be broken - i.e. no more 'save and return' button.
• Editing a lens embeddable by reference should work exactly as before.

Fancy gifs

Creating a panel by value

Editing a panel by value

Using save as to transform a panel into 'by reference'

Convert a panel to 'by value' with the Unlink action

Convert a panel to 'by reference' with the Add to Library action

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

[majagrubic]

I tested this in Chrome on Mac OS, looks great! I tested the following scenarios:

1. Adding Lens visualization by value
2. Editing Lens by value -> saving changes
3. Editing Lens by value -> discarding changes
4. Adding Lens by value to library
5. Unlinking saved Lens from library
6. Editing panel title
7. Filtering on Dashboard

All above scenarios work as expected. However, I found this breaks saving "regular visualizations" to library.
Steps to reproduce:

1. Add Lens visualization by value to dashboard
2. Add regular visualization by value to dasbhoard
3. Add regular visualization to library -> this fails with an error

Uncaught (in promise) Error: Save with duplicate title confirmation was rejected
    at checkForDuplicateTitle (:5601/klt/9007199254740991/bundles/plugin/savedObjects/savedObjects.plugin.js:20295)
    at async onSave (:5601/klt/9007199254740991/bundles/plugin/dashboard/dashboard.plugin.js:44792)
    at async Object.onSaveConfirmed [as onSave] (:5601/klt/9007199254740991/bundles/plugin/savedObjects/savedObjects.plugin.js:19955)
    at async SavedObjectSaveModal.saveSavedObject (:5601/klt/9007199254740991/bundles/plugin/savedObjects/savedObjects.plugin.js:19650)

(I don't have a duplicate title for this visualization)

[majagrubic]

savedVis won't exist in by-value mode. Why this change?

[ThomThomson]

I don't know the visualize embeddable too well, but all the changes in this file were required to make visualize work with the new stateTransfer types. I added a unified EmbeddablePackageState with an input key which means that dashboard doesn't have to worry at all about 'by reference' vs 'by value' when adding an embeddable.

So it seems like this change here is done for the by reference case.

[ThomThomson]

@majagrubic, good find with the issue relating to getInputAsRefType! Turns out I didn't delete the attributes from the original input when I should have in that case. It should be fixed now.

[Dosant]

I tested different scenarios. Looks good 🔥

Noticed couple edge case:

1. Seems like saving lens by value with filters doesn't always work. I think it is related to this issue: cleanup filters meta types and usage #77588
   This is the error:

I see you have a "list of issues" to solve before releasing this? I guess this could just be another one.

2. Story around browser navigation and history: I guess it isn't necessary to improve this in scope of this pr, but worth creating an issue? Basically I'd except the behavior to be as consistent with visualize / lens by saved object as possible. Some quirks:
   2.1. Back on a dashboard just after navigating from editor first removes just added panel. 2nd Back gets back to Lens. (this actually happen for all visualisations., not only by value Lens, I guess this isn't a regression..🤔). Wonder if it is a quick fix on a dashboard side.
   2.2. Back on a dashboard to editor: works fine for saved visualisations, but in case of "by value", we don't get navigated to the latest state. I wonder if this could be fixed by "replaceState" somewhere inside transferStateService
   2.3 Dashboard -> Edit Lens -> Back -> Forward ---> we don't have "return and save to dashboard" button anymore. (this seems minor and an edge case)
   2.4. When your try to reload the Lens page it normally notifies if there are unsaved changes. Seems like it shows an alert if there are any edits. This is fine in case of saved visualisation. But in scenario when you've just navigated to /edit_by_value and do the page reload without any changes, the page reloads and Lens gets into "empty" state. I think in case of edit_by_value unsaved_changed dialog should always be triggered.

3. I was really confused by the following scenario: Dashboard -> edit by value lens -> Save As: Now there is an Add to Dashboard after saving switch.
   3.1 If it is ON It is not clear if it would add a new panel on a dashboard or if it would change an existing one. Technically this vis is already on a dashboard, so it probably should be labeled as Update panel on a dashboard
   3.2 If it is OFF it is not clear if changes would be saved to the panel on a dashboard.
   Maybe it would be simpler to remove this flow completely: when you EDIT by value vis which is already on a dashboard, then the only option you have is to "SAVE & RETURN" ? Otherwise some work has to be done to make UX about this clearer.

[Dosant]

nit: technically this promises array could contain null. To avoid this, I'd filter null in the end of this chain instead of filtering after Promise.all

[ThomThomson]

I only moved this piece of code from embeddable_factory into embeddable, but it does make more sense to filter out the nulls here instead of after the Promise.all, so I've made that change

[ppisljar]

have some questions but mostly the code looks good to me

[ppisljar]

code LGTM

[ThomThomson]

Responding to @Dosant's comments

• (1) Seems like saving lens by value with filters doesn't always work.
  I have recreated this, and am looking into it. Depending on the size of the fix, it could be a followup I have added a workaround that deletes the value function if it exists, and we are in by value mode - so far I haven't
  noticed any issues with this approach.

• (2.1) Back button on dashboard removes panel
  This is a 'feature' that I'm not super happy with, and is a side effect of the panels being stored in the url. The fix to [Discuss][Meta] Dashboard By Value URL length considerations #71499 should also handle this problem.

• (2.2 & 2.3) Back to editor from dashboard state not preserved
  These two issues have to do with our usage of the state param in navigateToApp, and how it gets implicitly reset to undefined any time that the hash history is updated. Check out this code sandbox for an example. I am wondering if it might be time to switch the state transfer service to use something more robust like a key in the sessionStorage so that we can reset it explicitly, and navigating back and forward wouldn't end up in limbo so often.

• (2.4) Unsaved changes notification on app leave in by value mode
  I am not totally sure about this one, because the onAppLeave handler currently doesn't warn you in by value mode unless you have active changes. If I was to warn any time the editor was in by value mode, then you would get a warning from the cancel button and the 'dashboard' breadcrumb even when you hadn't changed anything. To me, returning you to the 'create' state after reloading the page is the lesser of two evils.

• (3.1 & 3.2) Confusing behaviour with 'save as' on a by value visualization
  I am against removing the 'save as' flow completely when editing a by value lens visualization, because I think it's important for consistency, and to provide an 'escape hatch' so we aren't forcing users into the by value paradigm. I do agree that the UX needs work though. As it is I see two options for this case.

  1. The copy is changed to something like 'Update on dashboard', and the current behaviour is kept
  2. The behaviour is changed to append a new panel to the dashboard after the 'by value' panel.

  I personally favor the first option because I think of the action of 'save as' in terms of "save the panel you were editing, but with a name and by reference". It might be good to get input from @ryankeairns on this!

[ryankeairns]

(3.1 & 3.2) Confusing behaviour with 'save as' on a by value visualization
I am against removing the 'save as' flow completely when editing a by value lens visualization, because I think it's important for consistency, and to provide an 'escape hatch' so we aren't forcing users into the by value paradigm. I do agree that the UX needs work though. As it is I see two options for this case.

The copy is changed to something like 'Update on dashboard', and the current behaviour is kept
The behaviour is changed to append a new panel to the dashboard after the 'by value' panel.

I personally favor the first option because I think of the action of 'save as' in terms of "save the panel you were editing, but with a name and by reference". It might be good to get input from @ryankeairns on this!

Checked this out and came to following recommendations:

1. My expectation with any 'Save as' feature is that it will create a new thing which makes the replacement of the original by value panel, in this case, surprising. Thinking further, it struck me that the current 'Save as' UX is essentially the equivalent to 'Add to library' in that it makes a by reference object from a by value object. Given those circumstances, I propose changing the menu link from 'Save as' to 'Add to library'.
2. The label change that @ThomThomson proposes - 'Update on dashboard' - makes sense to me. When coupled with the previous bullet, I think it will provide even greater clarity.

I think of this 'Add to library' UX in terms of:
(When keeping toggle on) "I'm adding this to the library, and I want to replace/update the existing (by value) panel"
or, conversely,
(When switching toggle off) "I'm adding this to the library, and I want to keep it separate from the original dashboard".

[ThomThomson]

Thanks for the help @ryankeairns, I've changed the copy to match the suggestions!

This is definitely a UX improvement, and helps to fill in a use case I wasn't quite sure about before. Thanks for pointing this out @Dosant

Top nav in edit by value mode & create by value mode:

Save modal in edit by value mode

[Dosant]

I retested saving with filters scenario. Worked well.
Also label update in "Save as" scenario makes sense to me and makes it a lot clearer 👍
Thanks for looking into it.

[ThomThomson]

@elasticmachine merge upstream

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 27ce740

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
dashboard	172	+1	171
lens	465	+2	463
visualize	209	-5	214
total		-2

async chunks size

id	value	diff	baseline
lens	32.9KB	+6.8KB	26.2KB
maps	3.3MB	+85.0B	3.3MB
visualize	411.6KB	-3.2KB	414.8KB
total		+3.7KB

page load bundle size

id	value	diff	baseline
dashboard	709.3KB	+936.0B	708.3KB
embeddable	431.0KB	+736.0B	430.2KB
lens	1.1MB	+8.2KB	1.1MB
savedObjects	235.8KB	+143.0B	235.7KB
total		+10.0KB

History

• 💚 Build #76259 succeeded 82e89a8
• 💚 Build #76203 succeeded 2f9c843
• 💔 Build #76015 failed d08ff4e
• 💚 Build #75951 succeeded fdd75b7
• 💚 Build #75718 succeeded 3a25f1f

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