Put share link back in edit mode (#11003) (#11060)

* Put share link back in edit mode

See https://github.com/elastic/kibana/issues/10951 for background and
reasoning

* Preserve backward compatibility with pre 5.3 dashboard links

* Fix paths so create and edit use landing page as a substring

Apparently it has to be this way for the chrome nav to handle app
navigation the right way.

* Replace more strings with constants

* Update tests to point to new default dashboard url landing page

* [utils] move encodeQueryComponent into shared utils

* [uiNavLink] allow specifying subUrl pattern

* [dashboards] use dashboard urls that are more like before 5.3

* update comments

* Remove one comment, add another

* Remove failing toJSON test since the function was removed

Submitting now, but pending feedback that toJSON was removed
intentionally.

* Fix tests that need subUrlBase set manually

* fix bad markup in breadcrumbs

* Preserve 5.3 create dashboard links!

* add a comment

src/core_plugins/kibana/index.js

@@ -84,7 +84,13 @@ module.exports = function (kibana) {
           id: 'kibana:dashboard',
           title: 'Dashboard',
           order: -1001,
-          url: `${kbnBaseUrl}#/dashboard`,
+          url: `${kbnBaseUrl}#/dashboards`,
+          // The subUrlBase is the common substring of all urls for this app. If not given, it defaults to the url
+          // above. This app has to use a different subUrlBase, in addition to the url above, because "#/dashboard"
+          // routes to a page that creates a new dashboard. When we introduced a landing page, we needed to change
+          // the url above in order to preserve the original url for BWC. The subUrlBase helps the Chrome api nav
+          // to determine what url to use for the app link.
+          subUrlBase: `${kbnBaseUrl}#/dashboard`,
           description: 'compose visualizations for much win',
           icon: 'plugins/kibana/assets/dashboard.svg',
         }, {

src/core_plugins/kibana/public/dashboard/dashboard.html

@@ -7,12 +7,14 @@
       <div
         data-transclude-slot="topLeftCorner"
       >
-        <!-- Breadcrumbs. -->
-        <bread-crumbs
-          page-title="getDashTitle()"
-          use-links="true"
-          omit-current-page="true"
-        ></bread-crumbs>
+        <div class="kuiLocalBreadcrumbs" data-test-subj="breadcrumbs">
+          <div class="kuiLocalBreadcrumb">
+            <a class="kuiLocalBreadcrumb__link" href="{{landingPageUrl()}}">Dashboard</a>
+          </div>
+          <div class="kuiLocalBreadcrumb">
+            {{ getDashTitle() }}
+          </div>
+        </div>
       </div>
 
       <!-- Search. -->

src/core_plugins/kibana/public/dashboard/dashboard.js

@@ -7,6 +7,7 @@ import chrome from 'ui/chrome';
 import 'plugins/kibana/dashboard/grid';
 import 'plugins/kibana/dashboard/panel/panel';
 
+import { SavedObjectNotFound } from 'ui/errors';
 import { getDashboardTitle, getUnsavedChangesWarningMessage } from './dashboard_strings';
 import { DashboardViewMode } from './dashboard_view_mode';
 import { TopNavIds } from './top_nav/top_nav_ids';
@@ -15,11 +16,12 @@ import dashboardTemplate from 'plugins/kibana/dashboard/dashboard.html';
 import FilterBarQueryFilterProvider from 'ui/filter_bar/query_filter';
 import DocTitleProvider from 'ui/doc_title';
 import { getTopNavConfig } from './top_nav/get_top_nav_config';
-import { DashboardConstants } from './dashboard_constants';
+import { DashboardConstants, createDashboardEditUrl } from './dashboard_constants';
 import { VisualizeConstants } from 'plugins/kibana/visualize/visualize_constants';
 import UtilsBrushEventProvider from 'ui/utils/brush_event';
 import FilterBarFilterBarClickHandlerProvider from 'ui/filter_bar/filter_bar_click_handler';
 import { DashboardState } from './dashboard_state';
+import notify from 'ui/notify';
 
 const app = uiModules.get('app/dashboard', [
   'elasticsearch',
@@ -31,24 +33,37 @@ const app = uiModules.get('app/dashboard', [
 ]);
 
 uiRoutes
-  .when('/dashboard/create', {
+  .when(DashboardConstants.CREATE_NEW_DASHBOARD_URL, {
     template: dashboardTemplate,
     resolve: {
       dash: function (savedDashboards, courier) {
         return savedDashboards.get()
           .catch(courier.redirectWhenMissing({
-            'dashboard': '/dashboard'
+            'dashboard': DashboardConstants.LANDING_PAGE_PATH
           }));
       }
     }
   })
-  .when('/dashboard/:id', {
+  .when(createDashboardEditUrl(':id'), {
     template: dashboardTemplate,
     resolve: {
-      dash: function (savedDashboards, Notifier, $route, $location, courier) {
-        return savedDashboards.get($route.current.params.id)
+      dash: function (savedDashboards, Notifier, $route, $location, courier, kbnUrl, AppState) {
+        const id = $route.current.params.id;
+        return savedDashboards.get(id)
+          .catch((error) => {
+            // Preserve BWC of v5.3.0 links for new, unsaved dashboards.
+            // See https://github.com/elastic/kibana/issues/10951 for more context.
+            if (error instanceof SavedObjectNotFound && id === 'create') {
+              // Note "new AppState" is neccessary so the state in the url is preserved through the redirect.
+              kbnUrl.redirect(DashboardConstants.CREATE_NEW_DASHBOARD_URL, {}, new AppState());
+              notify.error(
+                'The url "dashboard/create" is deprecated and will be removed in 6.0. Please update your bookmarks.');
+            } else {
+              throw error;
+            }
+          })
           .catch(courier.redirectWhenMissing({
-            'dashboard' : '/dashboard'
+            'dashboard' : DashboardConstants.LANDING_PAGE_PATH
           }));
       }
     }
@@ -113,6 +128,7 @@ app.directive('dashboardApp', function (Notifier, courier, AppState, timefilter,
       $scope.expandedPanel = null;
       $scope.dashboardViewMode = dashboardState.getViewMode();
 
+      $scope.landingPageUrl = () => `#${DashboardConstants.LANDING_PAGE_PATH}`;
       $scope.getBrushEvent = () => brushEvent(dashboardState.getAppState());
       $scope.getFilterBarClickHandler = () => filterBarClickHandler(dashboardState.getAppState());
       $scope.hasExpandedPanel = () => $scope.expandedPanel !== null;
@@ -192,10 +208,7 @@ app.directive('dashboardApp', function (Notifier, courier, AppState, timefilter,
 
         function revertChangesAndExitEditMode() {
           dashboardState.resetState();
-          const refreshUrl = dash.id ?
-            DashboardConstants.EXISTING_DASHBOARD_URL : DashboardConstants.CREATE_NEW_DASHBOARD_URL;
-          const refreshUrlOptions = dash.id ? { id: dash.id } : {};
-          kbnUrl.change(refreshUrl, refreshUrlOptions);
+          kbnUrl.change(dash.id ? createDashboardEditUrl(dash.id) : DashboardConstants.CREATE_NEW_DASHBOARD_URL);
           // This is only necessary for new dashboards, which will default to Edit mode.
           updateViewMode(DashboardViewMode.VIEW);
 
@@ -231,9 +244,7 @@ app.directive('dashboardApp', function (Notifier, courier, AppState, timefilter,
           if (id) {
             notify.info(`Saved Dashboard as "${dash.title}"`);
             if (dash.id !== $routeParams.id) {
-              kbnUrl.change(
-                `${DashboardConstants.EXISTING_DASHBOARD_URL}`,
-                { id: dash.id });
+              kbnUrl.change(createDashboardEditUrl(dash.id));
             } else {
               docTitle.change(dash.lastSavedTitle);
               updateViewMode(DashboardViewMode.VIEW);

src/core_plugins/kibana/public/dashboard/dashboard_constants.js

@@ -1,8 +1,12 @@
+import { encodeQueryComponent } from '../../../../utils';
 
 export const DashboardConstants = {
   ADD_VISUALIZATION_TO_DASHBOARD_MODE_PARAM: 'addToDashboard',
   NEW_VISUALIZATION_ID_PARAM: 'addVisualization',
-  LANDING_PAGE_PATH: '/dashboard',
-  CREATE_NEW_DASHBOARD_URL: '/dashboard/create',
-  EXISTING_DASHBOARD_URL: '/dashboard/{{id}}'
+  LANDING_PAGE_PATH: '/dashboards',
+  CREATE_NEW_DASHBOARD_URL: '/dashboard',
 };
+
+export function createDashboardEditUrl(id) {
+  return `/dashboard/${encodeQueryComponent(id)}`;
+}

src/core_plugins/kibana/public/dashboard/listing/dashboard_listing.html

@@ -47,7 +47,7 @@
         <!-- Create dashboard button -->
         <a
           class="kuiButton kuiButton--primary"
-          href="#/dashboard/create"
+          href="{{listingController.getCreateDashboardHref()}}"
           aria-label="Create new dashboard"
           data-test-subj="newDashboardLink"
           ng-if="listingController.getSelectedItemsCount() === 0"
@@ -97,7 +97,7 @@
         <div class="kuiPromptForItems__actions">
           <a
             class="kuiButton kuiButton--primary kuiButton--iconText"
-            href="#/dashboard/create"
+            href="{{listingController.getCreateDashboardHref()}}"
           >
             <span class="kuiButton__inner">
               <span class="kuiButton__icon kuiIcon fa-plus"></span>

src/core_plugins/kibana/public/dashboard/listing/dashboard_listing.js

@@ -2,6 +2,7 @@ import SavedObjectRegistryProvider from 'ui/saved_objects/saved_object_registry'
 import 'ui/pager_control';
 import 'ui/pager';
 import _ from 'lodash';
+import { DashboardConstants, createDashboardEditUrl } from '../dashboard_constants';
 
 export function DashboardListingController($injector, $scope) {
   const $filter = $injector.get('$filter');
@@ -145,6 +146,10 @@ export function DashboardListingController($injector, $scope) {
   };
 
   this.getUrlForItem = function getUrlForItem(item) {
-    return `#/dashboard/${item.id}`;
+    return `#${createDashboardEditUrl(item.id)}`;
+  };
+
+  this.getCreateDashboardHref = function getCreateDashboardHref() {
+    return `#${DashboardConstants.CREATE_NEW_DASHBOARD_URL}`;
   };
 }

src/core_plugins/kibana/public/dashboard/top_nav/get_top_nav_config.js

@@ -17,7 +17,8 @@ export function getTopNavConfig(dashboardMode, actions) {
         getSaveConfig(),
         getViewConfig(actions[TopNavIds.EXIT_EDIT_MODE]),
         getAddConfig(),
-        getOptionsConfig()];
+        getOptionsConfig(),
+        getShareConfig()];
     default:
       return [];
   }

src/ui/__tests__/ui_nav_link.js

@@ -135,25 +135,4 @@ describe('UiNavLink', () => {
       expect(link.tooltip).to.be('');
     });
   });
-
-  describe('#toJSON', () => {
-    it ('returns the expected properties', () => {
-      const uiExports = {
-        urlBasePath: 'http://localhost:5601/rnd'
-      };
-      const spec = {
-        id: 'kibana:discover',
-        title: 'Discover',
-        order: -1003,
-        url: '/app/kibana#/discover',
-        description: 'interactively explore your data',
-        icon: 'plugins/kibana/assets/discover.svg',
-      };
-      const link = new UiNavLink(uiExports, spec);
-      const json = link.toJSON();
-
-      ['id', 'title', 'url', 'order', 'description', 'icon', 'linkToLastSubUrl', 'hidden', 'disabled', 'tooltip']
-      .forEach(expectedProperty => expect(json).to.have.property(expectedProperty));
-    });
-  });
 });

src/ui/public/chrome/api/__tests__/nav.js

@@ -95,9 +95,18 @@ describe('chrome nav apis', function () {
     it('injects the globalState of the current url to all links for the same app', function () {
       const appUrlStore = new StubBrowserStorage();
       const nav = [
-        { url: 'https://localhost:9200/app/kibana#discover' },
-        { url: 'https://localhost:9200/app/kibana#visualize' },
-        { url: 'https://localhost:9200/app/kibana#dashboard' },
+        {
+          url: 'https://localhost:9200/app/kibana#discover',
+          subUrlBase: 'https://localhost:9200/app/kibana#discover'
+        },
+        {
+          url: 'https://localhost:9200/app/kibana#visualize',
+          subUrlBase: 'https://localhost:9200/app/kibana#visualize'
+        },
+        {
+          url: 'https://localhost:9200/app/kibana#dashboards',
+          subUrlBase: 'https://localhost:9200/app/kibana#dashboard'
+        },
       ].map(l => {
         l.lastSubUrl = l.url;
         return l;

src/ui/public/chrome/api/nav.js

@@ -1,5 +1,5 @@
 import { parse, format } from 'url';
-import { startsWith, isString } from 'lodash';
+import { isString } from 'lodash';
 
 export default function (chrome, internals) {
   chrome.getNavLinks = function () {
@@ -110,7 +110,7 @@ export default function (chrome, internals) {
     const { appId, globalState: newGlobalState } = decodeKibanaUrl(url);
 
     for (const link of internals.nav) {
-      link.active = startsWith(url, link.url);
+      link.active = url.startsWith(link.subUrlBase);
       if (link.active) {
         setLastUrl(link, url);
         continue;
@@ -124,11 +124,16 @@ export default function (chrome, internals) {
     }
   };
 
-  internals.nav.forEach(link => {
+  function relativeToAbsolute(url) {
     // convert all link urls to absolute urls
     const a = document.createElement('a');
-    a.setAttribute('href', link.url);
-    link.url = a.href;
+    a.setAttribute('href', url);
+    return a.href;
+  }
+
+  internals.nav.forEach(link => {
+    link.url = relativeToAbsolute(link.url);
+    link.subUrlBase = relativeToAbsolute(link.subUrlBase);
   });
 
   // simulate a possible change in url to initialize the

src/ui/public/kbn_top_nav/bread_crumbs/bread_crumbs.html

@@ -1,6 +1,6 @@
 <div class="kuiLocalBreadcrumbs" data-test-subj="breadcrumbs">
   <div class="kuiLocalBreadcrumb" ng-if="useLinks && (!omitPages || !omitPages.includes(breadcrumb.path))" ng-repeat="breadcrumb in breadcrumbs">
-    <a class=kuiLocalBreadcrumb__link" href="{{breadcrumb.url}}">{{breadcrumb.title}}</a>
+    <a class="kuiLocalBreadcrumb__link" href="{{breadcrumb.url}}">{{breadcrumb.title}}</a>
   </div>
   <div class="kuiLocalBreadcrumb" ng-if="!useLinks && (!omitPages || !omitPages.includes(breadcrumb.path))" ng-repeat="breadcrumb in breadcrumbs">
     {{ breadcrumb.title }}

src/ui/public/utils/query_string.js

@@ -1,3 +1,5 @@
+import { encodeQueryComponent } from '../../../utils';
+
 const qs = {};
 
 /*****
@@ -12,26 +14,6 @@ function tryDecodeURIComponent(value) {
   catch (e) {} // eslint-disable-line no-empty
 }
 
-/**
- * This method is intended for encoding *key* or *value* parts of query component. We need a custom
- * method because encodeURIComponent is too aggressive and encodes stuff that doesn't have to be
- * encoded per http://tools.ietf.org/html/rfc3986:
- *    query         = *( pchar / "/" / "?" )
- *    pchar         = unreserved / pct-encoded / sub-delims / ":" / "@"
- *    unreserved    = ALPHA / DIGIT / "-" / "." / "_" / "~"
- *    pct-encoded   = "%" HEXDIG HEXDIG
- *    sub-delims    = "!" / "$" / "&" / "'" / "(" / ")"
- *                     / "*" / "+" / "," / ";" / "="
- */
-function encodeUriQuery(val, pctEncodeSpaces) {
-  return encodeURIComponent(val)
-    .replace(/%40/gi, '@')
-    .replace(/%3A/gi, ':')
-    .replace(/%24/g, '$')
-    .replace(/%2C/gi, ',')
-    .replace(/%20/g, (pctEncodeSpaces ? '%20' : '+'));
-}
-
 /**
  * Parses an escaped url query string into key-value pairs.
  * @returns {Object.<string,boolean|Array>}
@@ -82,7 +64,7 @@ qs.encode = function (obj) {
 };
 
 qs.param = function (key, val) {
-  return encodeUriQuery(key, true) + (val === true ? '' : '=' + encodeUriQuery(val, true));
+  return encodeQueryComponent(key, true) + (val === true ? '' : '=' + encodeQueryComponent(val, true));
 };
 
 /**

src/ui/ui_nav_link.js

@@ -1,11 +1,10 @@
-import { pick } from 'lodash';
-
 export default class UiNavLink {
   constructor(uiExports, spec) {
     this.id = spec.id;
     this.title = spec.title;
     this.order = spec.order || 0;
     this.url = `${uiExports.urlBasePath || ''}${spec.url}`;
+    this.subUrlBase = `${uiExports.urlBasePath || ''}${spec.subUrlBase || spec.url}`;
     this.description = spec.description;
     this.icon = spec.icon;
     this.linkToLastSubUrl = spec.linkToLastSubUrl === false ? false : true;
@@ -13,8 +12,4 @@ export default class UiNavLink {
     this.disabled = spec.disabled || false;
     this.tooltip = spec.tooltip || '';
   }
-
-  toJSON() {
-    return pick(this, ['id', 'title', 'url', 'order', 'description', 'icon', 'linkToLastSubUrl', 'hidden', 'disabled', 'tooltip']);
-  }
 }

src/utils/encode_query_component.js

@@ -0,0 +1,19 @@
+/**
+ * This method is intended for encoding *key* or *value* parts of query component. We need a custom
+ * method because encodeURIComponent is too aggressive and encodes stuff that doesn't have to be
+ * encoded per http://tools.ietf.org/html/rfc3986:
+ *    query         = *( pchar / "/" / "?" )
+ *    pchar         = unreserved / pct-encoded / sub-delims / ":" / "@"
+ *    unreserved    = ALPHA / DIGIT / "-" / "." / "_" / "~"
+ *    pct-encoded   = "%" HEXDIG HEXDIG
+ *    sub-delims    = "!" / "$" / "&" / "'" / "(" / ")"
+ *                     / "*" / "+" / "," / ";" / "="
+ */
+export function encodeQueryComponent(val, pctEncodeSpaces = false) {
+  return encodeURIComponent(val)
+    .replace(/%40/gi, '@')
+    .replace(/%3A/gi, ':')
+    .replace(/%24/g, '$')
+    .replace(/%2C/gi, ',')
+    .replace(/%20/g, (pctEncodeSpaces ? '%20' : '+'));
+}

src/utils/index.js

@@ -5,6 +5,8 @@ export fromRoot from './from_root';
 export pkg from './package_json';
 export unset from './unset';
 
+export { encodeQueryComponent } from './encode_query_component';
+
 export {
   createConcatStream,
   createIntersperseStream,

src/utils/package_json.js

@@ -1,22 +1,5 @@
-import { existsSync } from 'fs';
-import { join } from 'path';
+import { dirname } from 'path';
 
-let packageDir;
-let packagePath;
-
-while (!packagePath || !existsSync(packagePath)) {
-  const prev = packageDir;
-  packageDir = prev ? join(prev, '..') : __dirname;
-  packagePath = join(packageDir, 'package.json');
-
-  if (prev === packageDir) {
-    // if going up a directory doesn't work, we
-    // are already at the root of the filesystem
-    throw new Error('unable to find package.json');
-  }
-}
-
-
-module.exports = require(packagePath);
-module.exports.__filename = packagePath;
-module.exports.__dirname = packageDir;
+module.exports = require('../../package.json');
+module.exports.__filename = require.resolve('../../package.json');
+module.exports.__dirname = dirname(module.exports.__filename);

test/server_config.js

@@ -1,5 +1,4 @@
 const shield = require('./shield');
-
 const kibanaURL = '/app/kibana';
 
 module.exports = {
@@ -44,7 +43,7 @@ module.exports = {
     },
     dashboard: {
       pathname: kibanaURL,
-      hash: '/dashboard',
+      hash: '/dashboards',
     },
     settings: {
       pathname: kibanaURL,

test/support/page_objects/dashboard_page.js

@@ -1,6 +1,9 @@
 import _ from 'lodash';
 import { defaultFindTimeout } from '../';
 
+import {
+  DashboardConstants
+} from '../../../src/core_plugins/kibana/public/dashboard/dashboard_constants';
 
 import {
   scenarioManager,
@@ -35,7 +38,7 @@ export default class DashboardPage {
    */
   async onDashboardLandingPage() {
     PageObjects.common.debug(`onDashboardLandingPage`);
-    const exists = await PageObjects.common.doesCssSelectorExist('a[href="#/dashboard"]');
+    const exists = await PageObjects.common.doesCssSelectorExist(`a[href="#${DashboardConstants.LANDING_PAGE_PATH}"]`);
     return !exists;
   }
 
@@ -44,7 +47,8 @@ export default class DashboardPage {
     const onPage = await this.onDashboardLandingPage();
     if (!onPage) {
       await PageObjects.common.try(async () => {
-        const goToDashboardLink = await PageObjects.common.findByCssSelector('a[href="#/dashboard"]');
+        const goToDashboardLink =
+          await PageObjects.common.findByCssSelector(`a[href="#${DashboardConstants.LANDING_PAGE_PATH}"]`);
         await goToDashboardLink.click();
         // Once the searchFilter can be found, we know the page finished loading.
         await PageObjects.common.findTestSubject('searchFilter');