Use app id instead of pluginId to generate navlink from legacy apps

[pgayvallet]

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]

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

[pgayvallet]

retest

[sergibondarenko]

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

[pgayvallet]

@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]

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

[pgayvallet]

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.

[pgayvallet]

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]

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

[pgayvallet]

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?

[pgayvallet]

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

[mshustov]

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

[pgayvallet]

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,
	});

[mshustov]

So I guess I should not add them, wdyt?

agree. probably we should clean up types as well

[pgayvallet]

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.

[pgayvallet]

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]

retest

[kibanamachine]

💚 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