Use app id instead of pluginId to generate navlink from legacy apps by pgayvallet · Pull Request #57542 · elastic/kibana

pgayvallet · 2020-02-13T09:45:19Z

Summary

Fix #57470

Use the app id and not pluginId when generating navlinks from legacy app specs, as it was the case before #52161

Checklist

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

elasticmachine · 2020-02-13T09:45:22Z

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

pgayvallet · 2020-02-13T11:32:07Z

retest

…acy-multi-apps

sergibondarenko · 2020-02-13T11:46:20Z

@pgayvallet will the fix be backported to 7.6.0? I don't see v7.6.0 label

pgayvallet · 2020-02-13T12:50:52Z

@sergibondarenko The 7.6.0 release is already out. This will lands in 7.6.1, which is due for around end of feb.

mshustov · 2020-02-13T13:11:07Z

src/core/server/legacy/plugins/get_nav_links.ts

+      throw new Error('Every app must specify an id');
+    }
+
+    if (spec.pluginId && !pluginSpecs.some(plugin => plugin.getId() === spec.pluginId)) {


minor: Does it belong here? Such checks are more appropriate in find_legacy_plugin_specs.ts
the same for id

For the id, we could perform the check after in find_legacy_plugin_specs.ts, However for the pluginId check, as the pluginId value is actually not present in the generated navlinks structure, that would mean looking twice to perform this validation.

I moved it outside of the mapping function (well, at least for the pluginId check). That seems the best I can do without more heavy changes, unless you see something better?

mshustov · 2020-02-13T13:12:48Z

src/core/server/legacy/plugins/get_nav_links.ts

+      icon: spec.icon,
+      euiIconType: spec.euiIconType,
+      url: spec.url || `/app/${id}`,
+      linkToLastSubUrl: spec.linkToLastSubUrl,


seems to be optional and should repeat this logic https://github.com/elastic/kibana/pull/57542/files#diff-d1ea0ed9e7f4c8bacd7b2ecc4453fa05R71
I'd suggest normalizing spec content, even though this code will be removed soon-ish

I'd suggest normalizing spec content

These are two distinct types though, LegacyNavLinkSpec and LegacyAppSpec, with different fields and optionals on field (because, 'reasons' I guess). Not really confident on touching these things?

Can try to extract a mapping function that would accept either of those though

mshustov · 2020-02-13T13:14:00Z

src/core/server/legacy/plugins/get_nav_links.ts

+      category: spec.category,
+      title: spec.title,
+      order: typeof spec.order === 'number' ? spec.order : 0,
+      icon: spec.icon,


what about other fields from https://github.com/elastic/kibana/pull/57542/files#diff-d1ea0ed9e7f4c8bacd7b2ecc4453fa05R62-R74 ? subUrlBase, disableSubUrlTracking, tooltip, etc.

disableSubUrlTracking seems to be indeed missing when compared to the pre-#52161 version, even if not present in our TS type

export type LegacyAppSpec = Pick< ChromeNavLink, 'title' | 'order' | 'icon' | 'euiIconType' | 'url' | 'linkToLastSubUrl' | 'hidden' | 'category' > & { pluginId?: string; id?: string; listed?: boolean };

However the other properties were just not present when generating the navlink from the legacy app... So I guess I should not add them, wdyt?

kibana/src/legacy/ui/ui_apps/ui_app.js

Lines 67 to 77 in 8c29802

this._navLink = new UiNavLink({

id: this._id,

title: this._title,

order: this._order,

icon: this._icon,

euiIconType: this._euiIconType,

url: this._url,

linkToLastSubUrl: this._linkToLastSubUrl,

disableSubUrlTracking: this._disableSubUrlTracking,

category: this._category,

});

So I guess I should not add them, wdyt?

agree. probably we should clean up types as well

I ended up by doing the opposite of what I suggested to be able to factorize the mapping method...

export type LegacyNavLinkSpec = Partial<ChromeNavLink>; export type LegacyAppSpec = Partial<ChromeNavLink> & { pluginId?: string; listed?: boolean; };

PTAL, tell me if you think this is a risk.

kibana/src/legacy/ui/ui_nav_links/__tests__/ui_nav_link.js

Line 81 in 8c29802

it('initializes the linkToLastSubUrl property to true by default', () => {

The typo in linkToLastSub instead of linkToLastSubUrl was actually countering the wrong false default value in the mapping function...

pgayvallet · 2020-02-13T19:58:21Z

retest

…acy-multi-apps

kibanamachine · 2020-02-14T08:02:50Z

💚 Build Succeeded

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

History

💔 Build #26645 failed 9e7f382
💔 Build #26573 failed aadb6c5
💔 Build #26514 failed a817d84
💛 Build #26465 was flaky 19823a8
💔 Build #26441 failed b1a8f68

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

…lastic#57542) * properly use app id instead of pluginId to generate navlink * extract convertToNavLink, add more tests * use distinct mapping methods * fix linkToLastSubUrl default value * nits & doc

…apps (#57542) (#57672) * Use app id instead of pluginId to generate navlink from legacy apps (#57542) * properly use app id instead of pluginId to generate navlink * extract convertToNavLink, add more tests * use distinct mapping methods * fix linkToLastSubUrl default value * nits & doc * remove category, add disableSubUrlTracking * update doc

…apps (#57542) (#57670) * Use app id instead of pluginId to generate navlink from legacy apps (#57542) * properly use app id instead of pluginId to generate navlink * extract convertToNavLink, add more tests * use distinct mapping methods * fix linkToLastSubUrl default value * nits & doc * update generated doc

properly use app id instead of pluginId to generate navlink

b1a8f68

pgayvallet added Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// release_note:fix Feature:New Platform v8.0.0 v7.7.0 v7.6.1 labels Feb 13, 2020

pgayvallet requested a review from a team as a code owner February 13, 2020 09:45

Merge remote-tracking branch 'upstream/master' into kbn-57470-fix-leg…

19823a8

…acy-multi-apps

mshustov reviewed Feb 13, 2020

View reviewed changes

pgayvallet added 4 commits February 13, 2020 15:24

extract convertToNavLink, add more tests

a817d84

use distinct mapping methods

3fe38d7

fix linkToLastSubUrl default value

aadb6c5

nits & doc

9e7f382

Merge remote-tracking branch 'upstream/master' into kbn-57470-fix-leg…

9e75d6d

…acy-multi-apps

mshustov approved these changes Feb 14, 2020

View reviewed changes

pgayvallet merged commit f87053e into elastic:master Feb 14, 2020

pgayvallet mentioned this pull request Feb 14, 2020

[7.x] Use app id instead of pluginId to generate navlink from legacy apps (#57542) #57670

Merged

pgayvallet mentioned this pull request Feb 14, 2020

[7.6] Use app id instead of pluginId to generate navlink from legacy apps (#57542) #57672

Merged

joshdover mentioned this pull request Feb 18, 2020

Add ScopedHistory to AppMountParams #56705

Merged

8 tasks

	this._navLink = new UiNavLink({
	id: this._id,
	title: this._title,
	order: this._order,
	icon: this._icon,
	euiIconType: this._euiIconType,
	url: this._url,
	linkToLastSubUrl: this._linkToLastSubUrl,
	disableSubUrlTracking: this._disableSubUrlTracking,
	category: this._category,
	});

Conversation

pgayvallet commented Feb 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Feb 13, 2020

Uh oh!

pgayvallet commented Feb 13, 2020

Uh oh!

sergibondarenko commented Feb 13, 2020

Uh oh!

pgayvallet commented Feb 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgayvallet commented Feb 13, 2020

Uh oh!

kibanamachine commented Feb 14, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pgayvallet commented Feb 13, 2020 •

edited

Loading

pgayvallet commented Feb 13, 2020 •

edited

Loading