[Time to Visualize] Enable by Default

[ThomThomson]

Summary

A major piece of #51318
Closes #52682

Release Notes: Enable "by value embeddables" (dashboard.allowByValueEmbeddables) by default

All progress on the Time to Visualize project has been hidden behind the flag dashboard.allowByValueEmbeddables. This PR changes the default value of that config option to true.

Documentation will be added in a followup.

Checklist

Delete any items that are not applicable to this PR.

• Documentation was added for features that require explanation or tutorials
• Unit or functional tests were updated or added to match the most common scenarios

For maintainers

• This was checked for breaking API changes and was labeled appropriately

[majagrubic]

I'm a bit confused with the flow here: is the goal to ensure just that the all dashboard selectors exist, without actually adding the Lens visualization to the dashboard?

[ThomThomson]

as part of #83140, a new selector was added to the visualize save modal to allow users to add by value directly to a dashboard from visualize. That also means that in order to save it to the library, the user needs to select the add to library option on the dashboard selector.

What I've added here is a default path that says 'if the dashboard selection section exists, and we haven't been asked to add it directly to a dashboard, select the add to library option. This prevents a number of test failures - and you can see a similar implementation in the Visualize page.

[lukeelmers]

What I've added here is a default path that says 'if the dashboard selection section exists, and we haven't been asked to add it directly to a dashboard, select the add to library option. This prevents a number of test failures - and you can see a similar implementation in the Visualize page.

nit: It might be helpful to extract this logic into another helper function that could be pulled into the lens & SO tagging functional tests. It feels like the type of implementation detail that other apps shouldn't necessarily need to concern themselves with when writing tests.

e.g. something like this could potentially be used everywhere:

await saveToNewDashboard();
await saveToExistingDashboard('my-dashboard-id');
await saveToLibrary();

[elasticmachine]

Pinging @elastic/kibana-presentation (Team:Presentation)

[majagrubic]

🎉

[majagrubic]

Just curious, is the plan to enable this in 7.12 and then delete references to it in 7.13? Or are you also planning for a cleanup for 7.12?

[ThomThomson]

That's still up in the air - because removing the ability to turn the feature off could constitute a breaking change. Right now the consensus seems to be removing references to it in 8.0.0

[clintandrewhall]

We'll craft a plan to at least deprecate with a warning in 8.0 for removal in a follow-on minor, if not remove outright. It kind of depends on feedback from the field.

[clintandrewhall]

Fantastic... LGTM! 🚢

Congrats @elastic/kibana-presentation and @ThomThomson and @majagrubic, especially.

[lukeelmers]

Updates to core tests LGTM, just had one suggestion of a possible way to clean things up.

[lukeelmers]

What I've added here is a default path that says 'if the dashboard selection section exists, and we haven't been asked to add it directly to a dashboard, select the add to library option. This prevents a number of test failures - and you can see a similar implementation in the Visualize page.

nit: It might be helpful to extract this logic into another helper function that could be pulled into the lens & SO tagging functional tests. It feels like the type of implementation detail that other apps shouldn't necessarily need to concern themselves with when writing tests.

e.g. something like this could potentially be used everywhere:

await saveToNewDashboard();
await saveToExistingDashboard('my-dashboard-id');
await saveToLibrary();

[ThomThomson]

nit: It might be helpful to extract this logic into another helper function

Good call! I've cleaned this up by extracting all of the setting of save modal values into a new function in visualize page. Updated everywhere else it's used to use the new function.

[ThomThomson]

@elasticmachine merge upstream

[kibanamachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request
• Commit: bc94480
• Pipeline Steps (look for red circles / failed steps)
• Interpreting CI Failures

Failed CI Steps

• Setup Build Environment and Dependencies
• Setup Build Environment and Dependencies

Metrics [docs]

‼️ ERROR: no builds found for mergeBase sha [53f4b21]

History

• 💚 Build #99424 succeeded bc94480
• 💚 Build #99289 succeeded 068aab1
• 💔 Build #99282 failed 7a9176951629be316f8083ceae654ab4626c58a2
• 💚 Build #99136 succeeded b33de6e

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