-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Security Solution] Fixes elastic/kibana#84757 Use basePath in getAppOverviewUrl #87245
[Security Solution] Fixes elastic/kibana#84757 Use basePath in getAppOverviewUrl #87245
Conversation
@elasticmachine merge upstream |
): ChromeBreadcrumb[] | null => { | ||
const spyState: RouteSpyState = omit('navTabs', object); | ||
const siemRootBreadcrumb = fullSiemRootBreadcrumb(basePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it's worth it but do we need to add tests for this specific functionality? Like a test using a base path and checking that the SIEM URL includes it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will add
…h' into siem-breadcrumbs-respect-basepath
@@ -128,7 +127,7 @@ describe('Navigation Breadcrumbs', () => { | |||
); | |||
expect(breadcrumbs).toEqual([ | |||
{ | |||
href: '/app/security/overview', | |||
href: 'securitySolutionoverview', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: took me a second to figure out why this changed. I believe it's because of the mock on 112 that's doing:
const getUrlForAppMock = (appId: string, options?: { path?: string; absolute?: boolean }) =>
`${appId}${options?.path ?? ''}`;
That's used instead of this right?
getUrlForApp(APP_ID, { path: SecurityPageName.overview }),
Maybe a comment explaining why it's no longer /app/security/overview
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment explaining that the mock function returns not an actual url.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, took me a sec to realize it was just replacing the APP_OVERVIEW_PATH
const. Thanks
💚 Build SucceededMetrics [docs]Async chunks
Distributable file count
History
To update your PR or re-run it, just comment with: |
…87245) (#87559) * Use basePath in getAppOverviewUrl Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
…87245) (#88754) * Use basePath in getAppOverviewUrl Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
Summary
Fixes #84757 by making use of basePath in the getAppOverviewUrl function used in SIEM navigation components.
Checklist
Delete any items that are not applicable to this PR.
For maintainers