Allow to define and update a defaultPath for applications #64498

pgayvallet · 2020-04-27T08:01:48Z

Summary

Add a defaultPath property to applications and to their updatable fields, and use it in application.navigateToApp and in the UI navlinks urls.

Checklist

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

Dev Docs

KP applications can now define and update the defaultPath that should be used when navigating to them from another application or from the navigation bar.

core.application.register({
    id: 'my-app',
    // ...
    defaultPath: '/some-path',
})

const appUpdater = new BehaviorSubject<AppUpdater>(() => ({}));
core.application.register({
    id: 'my-app',
    // ...
    updater$: appUpdater,
})

// later
appUpdater.next(() => ({ defaultPath: '/some-updated-path' }));

elasticmachine · 2020-04-27T08:44:41Z

Pinging @elastic/kibana-platform (Team:Platform)

pgayvallet · 2020-04-27T08:57:35Z

src/core/public/application/types.ts

+   * When defined, this value will be used as a default for the `path` option when calling {@link ApplicationStart.navigateToApp | navigateToApp}`,
+   * and will also be appended to the {@link ChromeNavLink | application navLink} in the navigation bar.
+   */
+  defaultPath?: string;


Note: this is on AppBase because AppUpdatableFields is a pick from this root class (and I didn't want to overcomplexify the AppUpdatableFields type with generics). However this property is only used for KP apps. This will be cleaned up 'naturally' once we drop legacy support

should we mark it as @deprecated?

It's not deprecated. It's just that even if on AppBase, it's effectively only usable by the KP App subclass, and not the LegacyApp one.

pgayvallet · 2020-04-27T09:00:49Z

test/plugin_functional/test_suites/core_plugins/application_status.ts

+const getKibanaUrl = (pathname?: string, search?: string) =>
+  url.format({
+    protocol: 'http:',
+    hostname: process.env.TEST_KIBANA_HOST || 'localhost',
+    port: process.env.TEST_KIBANA_PORT || '5620',
+    pathname,
+    search,
+  });


So, this is the third duplication of this helper in test/plugin_functional/test_suites/core_plugins/* files. I would gladly extract it to an util file, however I'm unsure of the conventions for such files in FTR code. Moving it to a service seems overkill as it's 'stateless'. Any idea?

FTR already uses utils decalred in https://github.com/elastic/kibana/blob/8e9a8a84dccfa7965ce8a22362885e6cdef8b51f/src/test_utils/get_url.js

you can extend it

pgayvallet · 2020-04-27T09:01:44Z

src/core/public/application/utils.ts

+ * Utility to remove trailing, leading or duplicate slashes.
+ * By default will only remove duplicates.
+ */
+export const removeSlashes = (


Should I move this to src/core/utils instead?

I'd consider to move it when needed in another place

flash1293

Pulled this into my cutover PR, renamed the property and everything continues to work. LGTM, great job! No detailed code review

flash1293 · 2020-04-28T13:11:11Z

@pgayvallet I found a minor issue - in src/core/public/chrome/ui/header/recent_links.tsx the navLink.baseUrl is compared to the href of the recently accessed item. With this PR this doesn't work anymore, because baseUrl in that place will contain the whole defaultPath as well. In this place it should just compare to the actual baseUrl (without the defaultPath part).

pgayvallet · 2020-04-28T14:17:53Z

@flash1293 good catch, thanks. I did not want to introduce a new field, which is why I choose to alter the baseUrl instead, but I guess I will need to go back on that.

flash1293 · 2020-04-28T15:18:18Z

@pgayvallet Noticed another problem - I thought it would be irrelevant first, but it seems like it's breaking the history object in some cases. In https://github.com/elastic/kibana/pull/64498/files#diff-1ab7c26e3c5a0d226f771d13620db1e3L88 it's stripping away trailing slashes. But those are required for these cases: /app/discover#/ (and should be kept)

Edit: nvm, seems to be an unrelated problem.

…ater-defaultPath

pgayvallet · 2020-04-29T07:32:57Z

@flash1293 the recent links issue should be fixed. I also fixed the trailing / being removed in hashbangs, even if it seems it's not the actual cause of your other issue.

flash1293 · 2020-04-29T08:11:02Z

@pgayvallet Tested the fix and everything works fine now, thanks 👍

mshustov · 2020-04-29T09:22:01Z

src/core/public/application/application_service.test.ts

+      expect(MockHistory.push).toHaveBeenCalledWith('/app/app1', undefined);
+      MockHistory.push.mockClear();
+
+      updater.next(app => ({ defaultPath: 'default-path' }));


from this example I'd expect it to be called as defaultSubpath / internalPath, etc.

I think the default part is important as the prop is used as a default in navigateToApp.
However in navigateToApp, the app path option is called path, not subPath, so I choose defaultPath.

But I'm fine with defaultSubPath if we think this is more explicit.

mshustov · 2020-04-29T09:23:41Z

src/core/public/application/application_service.test.ts

+    it('preserves trailing slash when path contains a hash', async () => {
+      const { register } = service.setup(setupDeps);
+
+      register(Symbol(), createApp({ id: 'app2', appRoute: '/custom/path' }));


is it used in the test?

Adapted this rather quickly. The navigateToApp('myTestApp', { path: '#/' }) below should be using this app2 instead. Will fix.

mshustov · 2020-04-29T09:24:15Z

src/core/public/application/application_service.test.ts

+
+      const { navigateToApp } = await service.start(startDeps);
+      await navigateToApp('myTestApp', { path: '#/' });
+      expect(MockHistory.push).toHaveBeenCalledWith('/app/myTestApp#/', undefined);


could you add an assertion when # not present

mshustov · 2020-04-29T09:25:44Z

src/core/public/application/types.ts

+   * When defined, this value will be used as a default for the `path` option when calling {@link ApplicationStart.navigateToApp | navigateToApp}`,
+   * and will also be appended to the {@link ChromeNavLink | application navLink} in the navigation bar.
+   */
+  defaultPath?: string;


should we mark it as @deprecated?

mshustov · 2020-04-29T09:28:11Z

src/core/public/application/utils.ts

+  return url;
+};
+
+export const appendAppPath = (appBasePath: string, path: string = '') => {


are there any tests for appendAppPath?

My bad, this was introduced in my last commit, and I did not add proper coverage for it. Will add.

mshustov · 2020-04-29T09:34:47Z

src/core/public/application/utils.ts

+ * Utility to remove trailing, leading or duplicate slashes.
+ * By default will only remove duplicates.
+ */
+export const removeSlashes = (


I'd consider to move it when needed in another place

src/core/public/chrome/nav_links/nav_link.ts

mshustov · 2020-04-29T09:44:25Z

test/plugin_functional/test_suites/core_plugins/application_status.ts

+const getKibanaUrl = (pathname?: string, search?: string) =>
+  url.format({
+    protocol: 'http:',
+    hostname: process.env.TEST_KIBANA_HOST || 'localhost',
+    port: process.env.TEST_KIBANA_PORT || '5620',
+    pathname,
+    search,
+  });


FTR already uses utils decalred in https://github.com/elastic/kibana/blob/8e9a8a84dccfa7965ce8a22362885e6cdef8b51f/src/test_utils/get_url.js

you can extend it

kibanamachine · 2020-04-29T21:26:51Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 8fb0107

History

💚 Build #44002 succeeded 484f9f9
💚 Build #43314 succeeded 5122bbb

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

) * add defaultPath to `AppBase` and use it in `navigateToApp` * add removeSlashes util * adapt `toNavLink` to handle defaultPath * update generated doc * codestyle * add FTR test * address comments * add tests

…64867) * add defaultPath to `AppBase` and use it in `navigateToApp` * add removeSlashes util * adapt `toNavLink` to handle defaultPath * update generated doc * codestyle * add FTR test * address comments * add tests

* master: (42 commits) [Ingest] Allow aggent to send metadata compliant with ECS (elastic#64452) [Endpoint] Remove todos, urls to issues (elastic#64833) [Uptime] Remove hard coded value for monitor states histograms (elastic#64396) Feature/send feedback link (elastic#64845) [ML] Moving get filters capability to admin (elastic#64879) Remove edit alert button from alerts list (elastic#64643) [EPM] Handle constant_keyword type in KB index patterns and ES index templates (elastic#64876) [ML] Disable data frame anaylics clone button based on permission (elastic#64830) Dashboard url generator to preserve saved filters from destination dashboard (elastic#64767) add generic typings for SavedObjectMigrationFn (elastic#63943) Allow to define and update a defaultPath for applications (elastic#64498) [Event Log] add rel=primary to saved objects for query targets (elastic#64615) [Lens] Use a size of 5 for first string field in visualization (elastic#64726) [SIEM][Lists] Removes plugin dependencies, adds more unit tests, fixes more TypeScript types [Ingest] Edit datasource UI (elastic#64727) [Lens] Bind all time fields to the time picker (elastic#63874) [Lens] Use suggestion system in chart switcher for subtypes (elastic#64613) Improve alpha messaging (elastic#64692) [Ingest] Allow to enable monitoring of elastic agent (elastic#63598) [Metrics UI] Fix alerting when a filter query is present (elastic#64575) ...

pgayvallet added 6 commits April 27, 2020 09:23

add defaultPath to AppBase and use it in navigateToApp

071ee2e

add removeSlashes util

3ae2f6e

adapt toNavLink to handle defaultPath

a5a3554

update generated doc

52538ae

codestyle

08d81af

add FTR test

5122bbb

pgayvallet added this to Pending Review in kibana-core [DEPRECATED] via automation Apr 27, 2020

pgayvallet added release_note:skip Skip the PR/issue when compiling release notes v7.8.0 v8.0.0 Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Apr 27, 2020

pgayvallet added release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. and removed release_note:skip Skip the PR/issue when compiling release notes labels Apr 27, 2020

pgayvallet commented Apr 27, 2020

View reviewed changes

pgayvallet marked this pull request as ready for review April 27, 2020 10:50

pgayvallet requested a review from a team as a code owner April 27, 2020 10:50

pgayvallet requested a review from flash1293 April 27, 2020 10:50

flash1293 approved these changes Apr 27, 2020

View reviewed changes

pgayvallet added 2 commits April 29, 2020 08:11

address comments

62115a1

Merge remote-tracking branch 'upstream/master' into kbn-56027-app-upd…

484f9f9

…ater-defaultPath

mshustov approved these changes Apr 29, 2020

View reviewed changes

add tests

8fb0107

pgayvallet merged commit aa8cb62 into elastic:master Apr 30, 2020

kibana-core [DEPRECATED] automation moved this from Pending Review to Done (7.8) Apr 30, 2020

pgayvallet mentioned this pull request Apr 30, 2020

[7.x] Allow to define and update a defaultPath for applications (#64498) #64867

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to define and update a defaultPath for applications #64498

Allow to define and update a defaultPath for applications #64498

pgayvallet commented Apr 27, 2020 •

edited

Loading

elasticmachine commented Apr 27, 2020

pgayvallet Apr 27, 2020 •

edited

Loading

mshustov Apr 29, 2020

pgayvallet Apr 29, 2020

pgayvallet Apr 27, 2020

mshustov Apr 29, 2020

pgayvallet Apr 27, 2020

mshustov Apr 29, 2020

flash1293 left a comment

flash1293 commented Apr 28, 2020 •

edited

Loading

pgayvallet commented Apr 28, 2020

flash1293 commented Apr 28, 2020 •

edited

Loading

pgayvallet commented Apr 29, 2020

flash1293 commented Apr 29, 2020

mshustov Apr 29, 2020

pgayvallet Apr 29, 2020

mshustov Apr 29, 2020

pgayvallet Apr 29, 2020

mshustov Apr 29, 2020

mshustov Apr 29, 2020

mshustov Apr 29, 2020

pgayvallet Apr 29, 2020

mshustov Apr 29, 2020

mshustov Apr 29, 2020

kibanamachine commented Apr 29, 2020

Allow to define and update a defaultPath for applications #64498

Allow to define and update a defaultPath for applications #64498

Conversation

pgayvallet commented Apr 27, 2020 • edited Loading

Summary

Checklist

Dev Docs

elasticmachine commented Apr 27, 2020

pgayvallet Apr 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

flash1293 commented Apr 28, 2020 • edited Loading

pgayvallet commented Apr 28, 2020

flash1293 commented Apr 28, 2020 • edited Loading

pgayvallet commented Apr 29, 2020

flash1293 commented Apr 29, 2020

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

kibanamachine commented Apr 29, 2020

💚 Build Succeeded

History

pgayvallet commented Apr 27, 2020 •

edited

Loading

pgayvallet Apr 27, 2020 •

edited

Loading

flash1293 commented Apr 28, 2020 •

edited

Loading

flash1293 commented Apr 28, 2020 •

edited

Loading