Skip to content
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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Jan 6, 2021
Merged

[Security Solution] Fixes elastic/kibana#84757 Use basePath in getAppOverviewUrl #87245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
146eff4
Use basePath in getAppOverviewUrl
kqualters-elastic Jan 4, 2021
d3858c3
Remove unused import and fix broken test
kqualters-elastic Jan 5, 2021
53d7fc1
Merge branch 'master' into siem-breadcrumbs-respect-basepath
kibanamachine Jan 5, 2021
bca803b
Use getUrlForApp rather than basePath
kqualters-elastic Jan 5, 2021
cc7e51b
Merge remote-tracking branch 'origin/siem-breadcrumbs-respect-basepat…
kqualters-elastic Jan 5, 2021
1e085f8
Use getUrlForApp on logo link
kqualters-elastic Jan 5, 2021
5f1709a
Memoize overview url variables
kqualters-elastic Jan 5, 2021
4737cee
Add comment explaining getUrlForAppMock has different format
kqualters-elastic Jan 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions x-pack/plugins/security_solution/public/common/components/header_global/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

import { EuiButtonEmpty, EuiFlexGroup, EuiFlexItem, EuiIcon } from '@elastic/eui';
import { pickBy } from 'lodash/fp';
import React, { forwardRef, useCallback } from 'react';
import React, { forwardRef, useCallback, useMemo } from 'react';
import styled from 'styled-components';
import { OutPortal } from 'react-reverse-portal';

Expand Down Expand Up @@ -72,7 +72,16 @@ export const HeaderGlobal = React.memo(
const { timelineFullScreen } = useTimelineFullScreen();
const search = useGetUrlSearch(navTabs.overview);
const { application, http } = useKibana().services;
const { navigateToApp } = application;
const { navigateToApp, getUrlForApp } = application;
const overviewPath = useMemo(
() => getUrlForApp(APP_ID, { path: SecurityPageName.overview }),
[getUrlForApp]
);
const overviewHref = useMemo(() => getAppOverviewUrl(overviewPath, search), [
overviewPath,
search,
]);

const basePath = http.basePath.get();
const goToOverview = useCallback(
(ev) => {
Expand All @@ -93,7 +102,7 @@ export const HeaderGlobal = React.memo(
<FlexItem>
<EuiFlexGroup alignItems="center" responsive={false}>
<FlexItem grow={false}>
<LinkAnchor onClick={goToOverview} href={getAppOverviewUrl(search)}>
<LinkAnchor onClick={goToOverview} href={overviewHref}>
<EuiIcon aria-label={i18n.SECURITY_SOLUTION} type="logoSecurity" size="l" />
</LinkAnchor>
</FlexItem>
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/security_solution/public/common/components/link_to/redirect_to_overview.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { APP_OVERVIEW_PATH } from '../../../../common/constants';
import { appendSearch } from './helpers';

export const getAppOverviewUrl = (search?: string) => `${APP_OVERVIEW_PATH}${appendSearch(search)}`;
export const getAppOverviewUrl = (overviewPath: string, search?: string) =>
`${overviewPath}${appendSearch(search)}`;
24 changes: 12 additions & 12 deletions ...k/plugins/security_solution/public/common/components/navigation/breadcrumbs/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
*/
import '../../../mock/match_media';
import { encodeIpv6 } from '../../../lib/helpers';

import { getBreadcrumbsForRoute, setBreadcrumbs } from '.';
import { HostsTableType } from '../../../../hosts/store/model';
import { RouteSpyState, SiemRouteType } from '../../../utils/route/types';
Expand Down Expand Up @@ -110,6 +109,7 @@ const getMockObject = (
sourcerer: {},
});

// The string returned is different from what getUrlForApp returns, but does not matter for the purposes of this test.
const getUrlForAppMock = (appId: string, options?: { path?: string; absolute?: boolean }) =>
`${appId}${options?.path ?? ''}`;

Expand All @@ -128,7 +128,7 @@ describe('Navigation Breadcrumbs', () => {
);
expect(breadcrumbs).toEqual([
{
href: '/app/security/overview',
href: 'securitySolutionoverview',
Copy link
Contributor

@jonathan-buttner jonathan-buttner Jan 5, 2021 •
edited
Loading

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

from here: https://github.com/elastic/kibana/pull/87245/files#diff-8c78fbdebc67d5ff9388db4a66a58416997b3745367d7c49d88a7834d963eec2R77

Maybe a comment explaining why it's no longer /app/security/overview?

Copy link
Contributor Author

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.

Copy link
Contributor

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

text: 'Security',
},
{
Expand All @@ -149,7 +149,7 @@ describe('Navigation Breadcrumbs', () => {
getUrlForAppMock
);
expect(breadcrumbs).toEqual([
{ text: 'Security', href: '/app/security/overview' },
{ text: 'Security', href: 'securitySolutionoverview' },
{
text: 'Network',
href:
Expand All @@ -168,7 +168,7 @@ describe('Navigation Breadcrumbs', () => {
getUrlForAppMock
);
expect(breadcrumbs).toEqual([
{ text: 'Security', href: '/app/security/overview' },
{ text: 'Security', href: 'securitySolutionoverview' },
{
text: 'Timelines',
href:
Expand All @@ -183,7 +183,7 @@ describe('Navigation Breadcrumbs', () => {
getUrlForAppMock
);
expect(breadcrumbs).toEqual([
{ text: 'Security', href: '/app/security/overview' },
{ text: 'Security', href: 'securitySolutionoverview' },
{
text: 'Hosts',
href:
Expand All @@ -204,7 +204,7 @@ describe('Navigation Breadcrumbs', () => {
getUrlForAppMock
);
expect(breadcrumbs).toEqual([
{ text: 'Security', href: '/app/security/overview' },
{ text: 'Security', href: 'securitySolutionoverview' },
{
text: 'Network',
href:
Expand All @@ -224,7 +224,7 @@ describe('Navigation Breadcrumbs', () => {
getUrlForAppMock
);
expect(breadcrumbs).toEqual([
{ text: 'Security', href: '/app/security/overview' },
{ text: 'Security', href: 'securitySolutionoverview' },
{
text: 'Network',
href:
Expand All @@ -244,7 +244,7 @@ describe('Navigation Breadcrumbs', () => {
getUrlForAppMock
);
expect(breadcrumbs).toEqual([
{ text: 'Security', href: '/app/security/overview' },
{ text: 'Security', href: 'securitySolutionoverview' },
{
text: 'Detections',
href:
Expand All @@ -258,7 +258,7 @@ describe('Navigation Breadcrumbs', () => {
getUrlForAppMock
);
expect(breadcrumbs).toEqual([
{ text: 'Security', href: '/app/security/overview' },
{ text: 'Security', href: 'securitySolutionoverview' },
{
text: 'Cases',
href:
Expand All @@ -279,7 +279,7 @@ describe('Navigation Breadcrumbs', () => {
getUrlForAppMock
);
expect(breadcrumbs).toEqual([
{ text: 'Security', href: '/app/security/overview' },
{ text: 'Security', href: 'securitySolutionoverview' },
{
text: 'Cases',
href:
Expand All @@ -297,7 +297,7 @@ describe('Navigation Breadcrumbs', () => {
getUrlForAppMock
);
expect(breadcrumbs).toEqual([
{ text: 'Security', href: '/app/security/overview' },
{ text: 'Security', href: 'securitySolutionoverview' },
{
text: 'Administration',
href: 'securitySolution:administration',
Expand All @@ -310,7 +310,7 @@ describe('Navigation Breadcrumbs', () => {
test('should call chrome breadcrumb service with correct breadcrumbs', () => {
setBreadcrumbs(getMockObject('hosts', '/', hostName), chromeMock, getUrlForAppMock);
expect(setBreadcrumbsMock).toBeCalledWith([
{ text: 'Security', href: '/app/security/overview' },
{ text: 'Security', href: 'securitySolutionoverview' },
{
text: 'Hosts',
href:
Expand Down
28 changes: 13 additions & 15 deletions x-pack/plugins/security_solution/public/common/components/navigation/breadcrumbs/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import { getOr, omit } from 'lodash/fp';

import { ChromeBreadcrumb } from '../../../../../../../../src/core/public';
import { APP_NAME } from '../../../../../common/constants';
import { APP_NAME, APP_ID } from '../../../../../common/constants';
import { StartServices } from '../../../../types';
import { getBreadcrumbs as getHostDetailsBreadcrumbs } from '../../../../hosts/pages/details/utils';
import { getBreadcrumbs as getIPDetailsBreadcrumbs } from '../../../../network/pages/details';
Expand Down Expand Up @@ -40,13 +40,6 @@ export const setBreadcrumbs = (
}
};

export const siemRootBreadcrumb: ChromeBreadcrumb[] = [
{
text: APP_NAME,
href: getAppOverviewUrl(),
},
];

const isNetworkRoutes = (spyState: RouteSpyState): spyState is NetworkRouteSpyState =>
spyState != null && spyState.pageName === SecurityPageName.network;

Expand All @@ -71,14 +64,19 @@ export const getBreadcrumbsForRoute = (
getUrlForApp: GetUrlForApp
): ChromeBreadcrumb[] | null => {
const spyState: RouteSpyState = omit('navTabs', object);
const overviewPath = getUrlForApp(APP_ID, { path: SecurityPageName.overview });
const siemRootBreadcrumb: ChromeBreadcrumb = {
text: APP_NAME,
href: getAppOverviewUrl(overviewPath),
};
if (isHostsRoutes(spyState) && object.navTabs) {
const tempNav: SearchNavTab = { urlKey: 'host', isDetailPage: false };
let urlStateKeys = [getOr(tempNav, spyState.pageName, object.navTabs)];
if (spyState.tabName != null) {
urlStateKeys = [...urlStateKeys, getOr(tempNav, spyState.tabName, object.navTabs)];
}
return [
...siemRootBreadcrumb,
siemRootBreadcrumb,
...getHostDetailsBreadcrumbs(
spyState,
urlStateKeys.reduce(
Expand All @@ -96,7 +94,7 @@ export const getBreadcrumbsForRoute = (
urlStateKeys = [...urlStateKeys, getOr(tempNav, spyState.tabName, object.navTabs)];
}
return [
...siemRootBreadcrumb,
siemRootBreadcrumb,
...getIPDetailsBreadcrumbs(
spyState,
urlStateKeys.reduce(
Expand All @@ -115,7 +113,7 @@ export const getBreadcrumbsForRoute = (
}

return [
...siemRootBreadcrumb,
siemRootBreadcrumb,
...getDetectionRulesBreadcrumbs(
spyState,
urlStateKeys.reduce(
Expand All @@ -134,7 +132,7 @@ export const getBreadcrumbsForRoute = (
}

return [
...siemRootBreadcrumb,
siemRootBreadcrumb,
...getCaseDetailsBreadcrumbs(
spyState,
urlStateKeys.reduce(
Expand All @@ -153,7 +151,7 @@ export const getBreadcrumbsForRoute = (
}

return [
...siemRootBreadcrumb,
siemRootBreadcrumb,
...getTimelinesBreadcrumbs(
spyState,
urlStateKeys.reduce(
Expand All @@ -173,7 +171,7 @@ export const getBreadcrumbsForRoute = (
}

return [
...siemRootBreadcrumb,
siemRootBreadcrumb,
...getAdminBreadcrumbs(
spyState,
urlStateKeys.reduce(
Expand All @@ -192,7 +190,7 @@ export const getBreadcrumbsForRoute = (
object.navTabs[spyState.pageName]
) {
return [
...siemRootBreadcrumb,
siemRootBreadcrumb,
{
text: object.navTabs[spyState.pageName].name,
href: '',
Expand Down