[Lens][Dashboard] Adding Lens to Dashboard #53110

majagrubic · 2019-12-16T11:40:18Z

Summary

Adding Lens visualization to Dashboard, without navigating away from the dashboard. The idea is the same as for regular visualizations - use the url parameter to know we're in addToDashboard mode and then redirect properly. Redirect is a bit ugly, would love to get thoughts and feedback on this.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
~~- [x] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~-[x] 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

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

majagrubic · 2019-12-16T11:59:53Z

x-pack/legacy/plugins/lens/public/app_plugin/plugin.tsx

+            routeProps.history.push(`/lens/edit/${id}`);
+          } else if (addToDashMode && id) {
+            routeProps.history.push(`/lens/edit/${id}`);
+            const url = context.core.chrome.navLinks.get('kibana:dashboard');


This is adding more dependencies on context, which is being deprecated. Is there a better way to do this?

majagrubic · 2019-12-16T13:23:55Z

src/legacy/core_plugins/kibana/public/visualize/wizard/new_vis_modal.tsx

      if (this.props.editorParams && this.props.editorParams.includes('addToDashboard')) {
+        params = `${params}?addToDashboard`;


This means maps will be called with addToDashboard param. If that's an issue, this can be an explicit check for Lens.

timroes · 2019-12-16T13:34:49Z

src/legacy/core_plugins/kibana/public/dashboard/dashboard_app_controller.tsx

@@ -319,6 +319,14 @@ export class DashboardAppController {
            kbnUrl.removeParam(DashboardConstants.ADD_VISUALIZATION_TO_DASHBOARD_MODE_PARAM);
            kbnUrl.removeParam(DashboardConstants.NEW_VISUALIZATION_ID_PARAM);
          }
+          if ($routeParams[DashboardConstants.NEW_LENS_VISUALIZATION_ID_PARAM]) {


I wonder if it would make sense to generalize this URL directly. So that you can pass the embeddable type and the embeddable id via a URL instead of creating a specific case for each embeddable here.

Another advantage is, that we currently have references to x-pack specific code in OSS, that way. Even if it's not code links, but just referencing specific embeddable ids and Lens by name, we usually try not to have those references. With a generic URL we wouldn't need any reference to x-pack specific code here.

Also -- especially since we don't have a good "URL service" to navigate between apps right now -- I like an idea that the observability plugin is taking, that they create short URLs for those kind of links, and then just internally forward to the long URL, so that those short URLs can stay forever the same even if something internally needs to change. So I wonder if we should have a URL in the format /dashboard/add/<dashboard-id>/<embeddable-type>/id/<embeddable-id>, and make this forward to the appropriate methods internally. (I btw added the id here explicitly, since I think we later want to be able to add also embeddables by their full embeddable config instead of saved embeddables via that way.)

@majagrubic what are your thoughts on that?

I like this idea. But to be sure I understand what you are saying - writing the URL in such a way would allow for a more generic way of adding embeddables, and solves the problem of having Lens reference inside Dashboard. The problem of "not having a good URL service to navigate between apps" still remains, as well as the idea of updating window.location directly.

Additionally, we'd need to handle the case when there is no dashboard-id, as the dashboard hasn't yet been saved.

Yes I totally agree. This does not solve the generic problem of us needing to build proper URL services in the long run. So this is just an easier way to keep URLs stable before we have those kind of URL services. (And even they might want to use those short URLs in the long run.)

I made a code change, so that embeddableType and embeddableId are more generic, but still passed as query params. Let me know if this is more along the lines what you were thinking.

majagrubic · 2019-12-18T17:07:47Z

Jenkins, test this

x-pack/test/functional/apps/dashboard_mode/dashboard_empty_screen.js

elasticmachine · 2019-12-19T07:11:39Z

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

wylieconlon

Functionality LGTM, left a nitpicky comment about the use of regexes. I don't quite understand the URL context question as I haven't worked with it personally.

wylieconlon · 2020-01-07T17:54:18Z

x-pack/legacy/plugins/lens/public/app_plugin/url_helper.ts

+const EMPTY_DASHBOARD_PATTERN = /(.*#\/dashboard\?)(.*)/;
+const DASHBOARD_WITH_ID_PATTERN = /(.*#\/dashboard\/.*\?)(.*)/;
+const TIME_PATTERN_1 = /(.*)(,time:[^)]+\))(.*)/;
+const TIME_PATTERN_2 = /(.*)(time:[^)]+\),)(.*)/; // same as TIME_PATTERN_1, but comma follows, not preceeds


Can we use rison to parse these instead of regexes? I think the code will be more readable.

wylieconlon · 2020-01-07T17:55:10Z

x-pack/test/functional/apps/dashboard_mode/dashboard_empty_screen.js

+      await dashboardVisualizations.ensureNewVisualizationDialogIsShowing();
+      await createAndAddLens(title);
+      await PageObjects.dashboard.waitForRenderComplete();
+      await testSubjects.exists(`embeddablePanelHeading-${title}`);


wylieconlon · 2020-01-08T16:04:50Z

x-pack/legacy/plugins/lens/public/app_plugin/url_helper.ts

+    vars[key] = value;
+  });
+  return vars;
+}


When I was looking at use of rison in our code, I also found use of the url library and some other Kibana-specific URL parsing. Both this function and the previous function seem to be common functions in our code, can we simplify them?

There is the old kibanaUrl thing, but that doesn't work in new platform afaik. Standard URL api would be the best, but that doesn't work in IE11 :(
Anything specific you had in mind?

I was hoping to find a way to get rid of the regexes and manual construction of URLs in favor of a standardized way. I'm not very familiar with the setup here, but it is still my assumption that there is a cleaner way of doing. I don't want to block this work over this concern though.

majagrubic · 2020-01-09T17:11:56Z

@elasticmachine merge upstream

chrisdavies

Nice work, Maja. I think this probably still needs a bit of work, but I probably won't be around to approve that work in time, so I'm only commenting (not rejecting).

chrisdavies · 2020-01-10T16:08:14Z

x-pack/legacy/plugins/lens/public/app_plugin/url_helper.ts

+ * output: http://localhost:5601/lib/app/kibana#
+ * @param url
+ */
+export function getKibanaBasePathFromDashboardUrl(url: string | undefined): string | null {


Rather than parsing this from the URL, shouldn't we just use Kibana's base path utilities? (e.g. core.http.basePath or something like that)

chrisdavies · 2020-01-10T16:09:24Z

x-pack/legacy/plugins/lens/public/app_plugin/url_helper.ts

+): string {
+  let dashboardUrl = getUrlWithoutQueryParams(absoluteUrl);
+  if (!dashboardUrl) {
+    return absoluteUrl;


Should we still add the query params to the absolute URL?

No, the only thing where the previous function will return null is if the absoluteUrl itself is null.

chrisdavies · 2020-01-10T16:14:04Z

x-pack/legacy/plugins/lens/public/app_plugin/url_helper.ts

+  if (!dashboardUrl) {
+    return absoluteUrl;
+  }
+  dashboardUrl += '?';


I suspect, you'll want to use encodeURIComponent on your keys and values here. Something like this:

const queryString = Object.entries(urlParams) .map(([k, v]) => `${encodeURIComponent(k)}=${encodeURIComponent(v)}`) .join('&'); return `${dashboardUrl}?${queryString}`;

chrisdavies · 2020-01-10T16:21:39Z

x-pack/legacy/plugins/lens/public/app_plugin/url_helper.ts

+  }
+  const base = regex[1];
+  const dashboardState = regex[2];
+  return `${base}${DashboardConstants.ADD_EMBEDDABLE_TYPE}=${embeddableType}&${DashboardConstants.ADD_EMBEDDABLE_ID}=${embeddableId}&${dashboardState}`;


You might want to escape the embeddableId here. Also, you may want to de-duplicate it, as it may already be embedded, and this'll duplicate it in the URL, if I'm not mistaken.

chrisdavies · 2020-01-10T16:23:30Z

x-pack/legacy/plugins/lens/public/app_plugin/plugin.tsx

+                urlVars
+              );
+              const dashboardParsedUrl = addEmbeddableToDashboardUrl(dashboardUrl, id, 'lens');
+              if (dashboardParsedUrl) {


If this returns null, we silently fail. Is that the right thing? It seems like a null here is a legit error that should be surfaced in some way.

chrisdavies · 2020-01-10T16:27:17Z

x-pack/legacy/plugins/lens/public/app_plugin/plugin.tsx

+            if (lastDashboardAbsoluteUrl && lensUrl) {
+              const urlVars = getUrlVars(lastDashboardAbsoluteUrl);
+              updateUrlTime(urlVars);
+              window.history.pushState({}, '', lensUrl);


I think we generally avoid using pushState directly, and instead use a core service to do this, but I'm not sure. Might be worth looking into, anyway.

I don't think it exists in the new platform. I think the pushState is our only option for now.

chrisdavies · 2020-01-10T16:36:33Z

x-pack/legacy/plugins/lens/public/app_plugin/plugin.tsx

+            if (!url) {
+              return;
+            }
+            const lastDashboardAbsoluteUrl = url.url;


This also seems like a bunch of detail that belongs to the dashboard module, not to Lens. Maybe something like this:

const url = dashboardUrl.addEmbeddable(url.url, id, 'lens'); if (url) { window.history.pushState({}, '', url); }

This was the original intention, but then we figured the time range is not properly set. So for the hack with setting timerange to work, this won't be so clean.

chrisdavies · 2020-01-10T16:40:11Z

x-pack/legacy/plugins/lens/public/app_plugin/url_helper.ts

+
+const EMPTY_DASHBOARD_PATTERN = /(.*#\/dashboard\?)(.*)/;
+const DASHBOARD_WITH_ID_PATTERN = /(.*#\/dashboard\/.*\?)(.*)/;
+


This url_helper module seems like it should live in src/legacy/core_plugins/kibana/public/dashboard rather than in Lens. Lens really shouldn't know much about the internal details of dashboard URLs. The dashboard app should provide an API to adjust URLs. (This file could serve as that API.)

import { addEmbeddable } from 'dashboard_url'; // Usage example... const url = addEmbeddable(someDashboardUrl, 'lens', id); /// Etc.. const url = addEmbeddable(someDashboardUrl, 'visualization', id); // We may want to allow callers to modify other things? const url = setDateRange(someDashboardUrl, dateRange;

Good point. I wasn't sure where to put it, since Lens is the only thing that was using it.

majagrubic · 2020-01-10T22:11:23Z

Thanks @wylieconlon and @chrisdavies for your comments. I've applied some of the comments, especially around url_helper living inside the dashboard code and better error handling.
Regarding the URL handling - my idea was to create a simple and platform-agnostic way to handle URLs, without any dependencies, so parsing urls with regex seemed like a way to go. I've added a dependency to legacy platform to handle URLs without any regexes, so that it's more standard now. I'm not super happy with that, but interested to see what you think.

majagrubic · 2020-01-11T18:51:41Z

@elasticmachine merge upstream

kibanamachine · 2020-01-11T20:06:54Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 9e15430

History

💔 Build #19450 failed 25010ff
💚 Build #19057 succeeded fe9ab85
💚 Build #18686 succeeded 64e6ceb
💚 Build #18448 succeeded 8022cea
💚 Build #18059 succeeded 377fd2b17a4bb232c1834205d82804186c58d09a

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

flash1293 · 2020-01-13T13:49:33Z

Found a separate bug when looking into this (but maybe it makes sense to fix it together with this PR):

When the "Go to lens" link is used to open lens, it is 1. opening Lens without closing the modal (that also happens on master) and 2. Lens is not returning to the Dashboard once the visualization was saved.

IMHO the "Go to Lens" link should ideally behave like the "Lens" visualization tile. I can also create a separate issue for this if it doesn't fit in this PR

flash1293

This looks good to me, tested and works as expected. Left some nits and a question that should not block this PR.

flash1293 · 2020-01-13T13:40:07Z

x-pack/legacy/plugins/lens/public/app_plugin/plugin.tsx

@@ -93,9 +102,60 @@ export class AppPlugin {
            http: core.http,
          })
        );
+        const updateUrlTime = (urlVars: Record<string, string>): void => {
+          const decoded: RisonObject = rison.decode(urlVars._g) as RisonObject;


This piece of code can be simplified from a typing perspective:

const decoded = rison.decode(urlVars._g); if (!isRisonObject(decoded)) { return; } decoded.time = data.query.timefilter.timefilter.getTime(); urlVars._g = rison.encode(decoded);

flash1293 · 2020-01-13T13:53:48Z

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

-                .catch(() => {
+                .catch(e => {
+                  // eslint-disable-next-line no-console
+                  console.dir(e);


Debugging leftover?

Not really: right now every exception is caught and a generic message "Unable to save document" is displayed, even if the error originates from trying to figure the dashboard URL to return to. I don't want to change this behavior now without figuring out proper error messages to be displayed to the user (if any), but thought it would be useful to have the error at least logged in case it occurs.

flash1293 · 2020-01-13T14:14:28Z

src/legacy/core_plugins/kibana/public/dashboard/np_ready/url_helper.ts

+export function getUrlVars(url: string): Record<string, string> {
+  const vars: Record<string, string> = {};
+  // @ts-ignore
+  url.replace(/[?&]+([^=&]+)=([^&]*)/gi, function(_, key, value) {


That's an interesting usage of replace. However I think we can use matchAll in this scenario:

for ([_, key, value] of url.matchAll(/[?&]+([^=&]+)=([^&]*)/gi)) { vars[key] = value; }

Then you don;t need any ts-ignores.

This was taken from here:

kibana/x-pack/legacy/plugins/ml/public/application/jobs/jobs_list/components/utils.js

Line 374 in 8e9a8a8

function getUrlVars(url) {

So I assumed it was well-tested. But I like any suggestion that gets rid of ts-ignore directives 👍

flash1293 · 2020-01-13T14:18:05Z

src/legacy/core_plugins/kibana/public/dashboard/np_ready/url_helper.ts

+  return `${protocol}//${host}${basePath}/app/kibana#/lens/edit/${id}`;
+}
+
+/**


Thanks for the documentation, that's very helpful!

majagrubic · 2020-01-13T17:38:20Z

Thanks @flash1293, I'll address your comments in a follow-up PR, with a fix for the dialog and a bit of a cleanup. I also pinged Alona regarding error messages.

* First version of adding Lens to dashboard * Fix failing unit test * Replacing explicit Lens query param with a more generic one * Fixing failing unit test * Adding a unit test for redirect * Do not show Save New if adding from Dashboard * Adding functional test * Adding functional test * Fixing type issues * Renaming query params * Fixing failing unit test * Removing unused constants * Fixing erroneous imports * Fixing erroneous import * Fixing import * Fix failing typecheck * Removing timefilter from Dashboard URL * Fixing type error * Replacing time parsing with rison * Replacing URL regex parsing with legacy URLs * Fixing failing test Co-authored-by: Elastic Machine <[email protected]>

* First version of adding Lens to dashboard * Fix failing unit test * Replacing explicit Lens query param with a more generic one * Fixing failing unit test * Adding a unit test for redirect * Do not show Save New if adding from Dashboard * Adding functional test * Adding functional test * Fixing type issues * Renaming query params * Fixing failing unit test * Removing unused constants * Fixing erroneous imports * Fixing erroneous import * Fixing import * Fix failing typecheck * Removing timefilter from Dashboard URL * Fixing type error * Replacing time parsing with rison * Replacing URL regex parsing with legacy URLs * Fixing failing test Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

* First version of adding Lens to dashboard * Fix failing unit test * Replacing explicit Lens query param with a more generic one * Fixing failing unit test * Adding a unit test for redirect * Do not show Save New if adding from Dashboard * Adding functional test * Adding functional test * Fixing type issues * Renaming query params * Fixing failing unit test * Removing unused constants * Fixing erroneous imports * Fixing erroneous import * Fixing import * Fix failing typecheck * Removing timefilter from Dashboard URL * Fixing type error * Replacing time parsing with rison * Replacing URL regex parsing with legacy URLs * Fixing failing test Co-authored-by: Elastic Machine <[email protected]>

majagrubic force-pushed the add-lens-to-dash branch from 0b089a3 to dcaa202 Compare December 16, 2019 11:54

majagrubic commented Dec 16, 2019

View reviewed changes

majagrubic requested review from flash1293 and chrisdavies December 16, 2019 12:01

majagrubic changed the title ~~First version of adding Lens to dashboard~~ [Lens][Dashboard]First version of adding Lens to dashboard Dec 16, 2019

majagrubic changed the title ~~[Lens][Dashboard]First version of adding Lens to dashboard~~ [Lens][Dashboard] Adding Lens to Dashboard Dec 16, 2019

majagrubic commented Dec 16, 2019

View reviewed changes

timroes reviewed Dec 16, 2019

View reviewed changes

majagrubic force-pushed the add-lens-to-dash branch from 4898615 to 346d1fb Compare December 17, 2019 15:20

majagrubic requested a review from timroes December 18, 2019 10:15

majagrubic commented Dec 18, 2019

View reviewed changes

x-pack/test/functional/apps/dashboard_mode/dashboard_empty_screen.js Outdated Show resolved Hide resolved

majagrubic marked this pull request as ready for review December 19, 2019 07:10

majagrubic requested a review from a team December 19, 2019 07:10

majagrubic added release_note:roadmap Feature:Dashboard Dashboard related features Feature:Lens Team:Visualizations Visualization editors, elastic-charts and infrastructure and removed Feature:Lens labels Dec 19, 2019

majagrubic added v8.0.0 v7.6.0 labels Dec 19, 2019

majagrubic requested review from a team as code owners December 19, 2019 07:24

Fixing type error

8022cea

majagrubic force-pushed the add-lens-to-dash branch from 377fd2b to 8022cea Compare January 7, 2020 16:45

wylieconlon reviewed Jan 7, 2020

View reviewed changes

Replacing time parsing with rison

64e6ceb

majagrubic requested a review from wylieconlon January 8, 2020 14:32

wylieconlon reviewed Jan 8, 2020

View reviewed changes

Merge branch 'master' into add-lens-to-dash

fe9ab85

chrisdavies reviewed Jan 10, 2020

View reviewed changes

majagrubic force-pushed the add-lens-to-dash branch 2 times, most recently from e6995d7 to 25010ff Compare January 10, 2020 22:26

Maja Grubic added 2 commits January 10, 2020 22:26

Replacing URL regex parsing with legacy URLs

25010ff

Fixing failing test

f78bdc7

Merge branch 'master' into add-lens-to-dash

9e15430

flash1293 approved these changes Jan 13, 2020

View reviewed changes

majagrubic mentioned this pull request Jan 13, 2020

Navigate between dashboard and Lens #53499

Closed

majagrubic merged commit 7543b0c into elastic:master Jan 13, 2020

majagrubic mentioned this pull request Jan 13, 2020

[7.x] [Lens][Dashboard] Adding Lens to Dashboard (#53110) #54607

Merged

majagrubic deleted the add-lens-to-dash branch January 13, 2020 17:53

majagrubic mentioned this pull request Jan 13, 2020

[Dashboard][Lens] Small code fixes #54625

Merged

7 tasks

Dosant mentioned this pull request Jan 14, 2020

[Bug][Lens][Dashboard] Dashboard title encoding issue after navigating back from creating new Lens vis #54723

Closed

ThomThomson mentioned this pull request Jun 1, 2020

Remove App communication from URL #67064

Merged

3 tasks

		if (this.props.editorParams && this.props.editorParams.includes('addToDashboard')) {
		params = `${params}?addToDashboard`;


		const EMPTY_DASHBOARD_PATTERN = /(.#\/dashboard\?)(.)/;
		const DASHBOARD_WITH_ID_PATTERN = /(.#\/dashboard\/.\?)(.*)/;

[Lens][Dashboard] Adding Lens to Dashboard #53110

[Lens][Dashboard] Adding Lens to Dashboard #53110

Conversation

majagrubic commented Dec 16, 2019 • edited Loading

Summary

Checklist

For maintainers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timroes Dec 16, 2019 • edited Loading

Choose a reason for hiding this comment

majagrubic Dec 16, 2019 • edited Loading

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

majagrubic commented Dec 18, 2019

elasticmachine commented Dec 19, 2019

wylieconlon 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majagrubic commented Jan 9, 2020

chrisdavies 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

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

majagrubic commented Jan 10, 2020

majagrubic commented Jan 11, 2020

kibanamachine commented Jan 11, 2020

💚 Build Succeeded

History

flash1293 commented Jan 13, 2020

flash1293 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majagrubic Jan 13, 2020 • edited Loading

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

majagrubic commented Jan 13, 2020 • edited Loading

majagrubic commented Dec 16, 2019 •

edited

Loading

timroes Dec 16, 2019 •

edited

Loading

majagrubic Dec 16, 2019 •

edited

Loading

majagrubic Jan 13, 2020 •

edited

Loading

majagrubic commented Jan 13, 2020 •

edited

Loading