Skip to content

Commit

Permalink
[Enterprise Search] DRY out createHref helper for navigateToUrl calls (
Browse files Browse the repository at this point in the history
…#78717)

* DRY out createHref helper
- that navigateToUrl will shortly also use

+ fix mockHistory type complaint

* KibanaLogic: Update navigateToUrl to use createHref helper

+ add param.history value, since we're technically supposed to be using Kibana's passed history over anything from useHistory

* Update breadcrumbs & eui link helpers w/ new KibanaLogic changes

- navigateToUrl is now double-dipping createHref, so we update it to not do so
- remove useHistory in favor of grabbing history from KibanaLogic

* [Optional?] Update FlashMessages to grab history from KibanaLogic

- since we're now storing it there, we might as well grab it as well instead of passing in props?

* [Misc] Fix failing tests due to react router mock
  • Loading branch information
Constance committed Sep 29, 2020
1 parent 66ad3b4 commit e873848
Show file tree
Hide file tree
Showing 15 changed files with 122 additions and 39 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,11 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { mockHistory } from './';

export const mockKibanaValues = {
config: { host: 'http://localhost:3002' },
history: mockHistory,
navigateToUrl: jest.fn(),
setBreadcrumbs: jest.fn(),
setDocTitle: jest.fn(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export const mockHistory = {
pathname: '/current-path',
},
listen: jest.fn(() => jest.fn()),
};
} as any;
export const mockLocation = {
key: 'someKey',
pathname: '/current-path',
Expand All @@ -25,6 +25,7 @@ export const mockLocation = {
};

jest.mock('react-router-dom', () => ({
...(jest.requireActual('react-router-dom') as object),
useHistory: jest.fn(() => mockHistory),
useLocation: jest.fn(() => mockLocation),
}));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export const renderApp = (

const unmountKibanaLogic = mountKibanaLogic({
config,
history: params.history,
navigateToUrl: core.application.navigateToUrl,
setBreadcrumbs: core.chrome.setBreadcrumbs,
setDocTitle: core.chrome.docTitle.change,
Expand All @@ -53,9 +54,7 @@ export const renderApp = (
errorConnecting,
readOnlyMode: initialData.readOnlyMode,
});
const unmountFlashMessagesLogic = mountFlashMessagesLogic({
history: params.history,
});
const unmountFlashMessagesLogic = mountFlashMessagesLogic();

ReactDOM.render(
<I18nProvider>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,14 @@
import { resetContext } from 'kea';

import { mockHistory } from '../../__mocks__';
jest.mock('../kibana', () => ({
KibanaLogic: { values: { history: mockHistory } },
}));

import { FlashMessagesLogic, mountFlashMessagesLogic, IFlashMessage } from './';

describe('FlashMessagesLogic', () => {
const mount = () => mountFlashMessagesLogic({ history: mockHistory as any });
const mount = () => mountFlashMessagesLogic();

beforeEach(() => {
jest.clearAllMocks();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@

import { kea, MakeLogicType } from 'kea';
import { ReactNode } from 'react';
import { History } from 'history';

import { KibanaLogic } from '../kibana';

export interface IFlashMessage {
type: 'success' | 'info' | 'warning' | 'error';
Expand Down Expand Up @@ -61,10 +62,10 @@ export const FlashMessagesLogic = kea<MakeLogicType<IFlashMessagesValues, IFlash
},
],
},
events: ({ props, values, actions }) => ({
events: ({ values, actions }) => ({
afterMount: () => {
// On React Router navigation, clear previous flash messages and load any queued messages
const unlisten = props.history.listen(() => {
const unlisten = KibanaLogic.values.history.listen(() => {
actions.clearFlashMessages();
actions.setFlashMessages(values.queuedMessages);
actions.clearQueuedMessages();
Expand All @@ -81,11 +82,7 @@ export const FlashMessagesLogic = kea<MakeLogicType<IFlashMessagesValues, IFlash
/**
* Mount/props helper
*/
interface IFlashMessagesLogicProps {
history: History;
}
export const mountFlashMessagesLogic = (props: IFlashMessagesLogicProps) => {
FlashMessagesLogic(props);
export const mountFlashMessagesLogic = () => {
const unmount = FlashMessagesLogic.mount();
return unmount;
};
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
*/

import { mockHistory } from '../../__mocks__';
jest.mock('../kibana', () => ({
KibanaLogic: { values: { history: mockHistory } },
}));

import {
FlashMessagesLogic,
Expand All @@ -18,7 +21,7 @@ describe('Flash Message Helpers', () => {
const message = 'I am a message';

beforeEach(() => {
mountFlashMessagesLogic({ history: mockHistory as any });
mountFlashMessagesLogic();
});

it('setSuccessMessage()', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@ describe('KibanaLogic', () => {
it('sets values from props', () => {
mountKibanaLogic(mockKibanaValues);

expect(KibanaLogic.values).toEqual(mockKibanaValues);
expect(KibanaLogic.values).toEqual({
...mockKibanaValues,
navigateToUrl: expect.any(Function),
});
});

it('gracefully handles missing configs', () => {
Expand All @@ -29,4 +32,20 @@ describe('KibanaLogic', () => {
expect(KibanaLogic.values.config).toEqual({});
});
});

describe('navigateToUrl()', () => {
beforeEach(() => mountKibanaLogic(mockKibanaValues));

it('runs paths through createHref before calling navigateToUrl', () => {
KibanaLogic.values.navigateToUrl('/test');

expect(mockKibanaValues.navigateToUrl).toHaveBeenCalledWith('/app/enterprise_search/test');
});

it('does not run paths through createHref if the shouldNotCreateHref option is passed', () => {
KibanaLogic.values.navigateToUrl('/test', { shouldNotCreateHref: true });

expect(mockKibanaValues.navigateToUrl).toHaveBeenCalledWith('/test');
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -6,26 +6,40 @@

import { kea, MakeLogicType } from 'kea';

import { History } from 'history';
import { ApplicationStart, ChromeBreadcrumb } from 'src/core/public';

export interface IKibanaValues {
import { createHref, ICreateHrefOptions } from '../react_router_helpers';

interface IKibanaLogicProps {
config: { host?: string };
history: History;
navigateToUrl: ApplicationStart['navigateToUrl'];
setBreadcrumbs(crumbs: ChromeBreadcrumb[]): void;
setDocTitle(title: string): void;
}
export interface IKibanaValues extends IKibanaLogicProps {
navigateToUrl(path: string, options?: ICreateHrefOptions): Promise<void>;
}

export const KibanaLogic = kea<MakeLogicType<IKibanaValues>>({
path: ['enterprise_search', 'kibana_logic'],
reducers: ({ props }) => ({
config: [props.config || {}, {}],
navigateToUrl: [props.navigateToUrl, {}],
history: [props.history, {}],
navigateToUrl: [
(url: string, options?: ICreateHrefOptions) => {
const href = createHref(url, props.history, options);
return props.navigateToUrl(href);
},
{},
],
setBreadcrumbs: [props.setBreadcrumbs, {}],
setDocTitle: [props.setDocTitle, {}],
}),
});

export const mountKibanaLogic = (props: IKibanaValues) => {
export const mountKibanaLogic = (props: IKibanaLogicProps) => {
KibanaLogic(props);
const unmount = KibanaLogic.mount();
return unmount;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,12 @@
*/

import '../../__mocks__/kea.mock';
import '../../__mocks__/react_router_history.mock';
import { mockKibanaValues, mockHistory } from '../../__mocks__';

jest.mock('../react_router_helpers', () => ({ letBrowserHandleEvent: jest.fn(() => false) }));
jest.mock('../react_router_helpers', () => ({
letBrowserHandleEvent: jest.fn(() => false),
createHref: jest.requireActual('../react_router_helpers').createHref,
}));
import { letBrowserHandleEvent } from '../react_router_helpers';

import {
Expand Down Expand Up @@ -50,21 +52,23 @@ describe('useBreadcrumbs', () => {

it('prevents default navigation and uses React Router history on click', () => {
const breadcrumb = useBreadcrumbs([{ text: '', path: '/test' }])[0] as any;

expect(breadcrumb.href).toEqual('/app/enterprise_search/test');
expect(mockHistory.createHref).toHaveBeenCalled();

const event = { preventDefault: jest.fn() };
breadcrumb.onClick(event);

expect(mockKibanaValues.navigateToUrl).toHaveBeenCalledWith('/app/enterprise_search/test');
expect(mockHistory.createHref).toHaveBeenCalled();
expect(event.preventDefault).toHaveBeenCalled();
expect(mockKibanaValues.navigateToUrl).toHaveBeenCalled();
});

it('does not call createHref if shouldNotCreateHref is passed', () => {
const breadcrumb = useBreadcrumbs([
{ text: '', path: '/test', shouldNotCreateHref: true },
])[0] as any;
breadcrumb.onClick({ preventDefault: () => null });

expect(mockKibanaValues.navigateToUrl).toHaveBeenCalledWith('/test');
expect(breadcrumb.href).toEqual('/test');
expect(mockHistory.createHref).not.toHaveBeenCalled();
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
*/

import { useValues } from 'kea';
import { useHistory } from 'react-router-dom';
import { EuiBreadcrumb } from '@elastic/eui';

import { KibanaLogic } from '../../shared/kibana';
Expand All @@ -16,7 +15,7 @@ import {
WORKPLACE_SEARCH_PLUGIN,
} from '../../../../common/constants';

import { letBrowserHandleEvent } from '../react_router_helpers';
import { letBrowserHandleEvent, createHref } from '../react_router_helpers';

/**
* Generate React-Router-friendly EUI breadcrumb objects
Expand All @@ -33,20 +32,17 @@ interface IBreadcrumb {
export type TBreadcrumbs = IBreadcrumb[];

export const useBreadcrumbs = (breadcrumbs: TBreadcrumbs) => {
const history = useHistory();
const { navigateToUrl } = useValues(KibanaLogic);
const { navigateToUrl, history } = useValues(KibanaLogic);

return breadcrumbs.map(({ text, path, shouldNotCreateHref }) => {
const breadcrumb = { text } as EuiBreadcrumb;

if (path) {
const href = shouldNotCreateHref ? path : (history.createHref({ pathname: path }) as string);

breadcrumb.href = href;
breadcrumb.href = createHref(path, history, { shouldNotCreateHref });
breadcrumb.onClick = (event) => {
if (letBrowserHandleEvent(event)) return;
event.preventDefault();
navigateToUrl(href);
navigateToUrl(path, { shouldNotCreateHref });
};
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { mockHistory } from '../../__mocks__';

import { createHref } from './';

describe('createHref', () => {
it('generates a path with the React Router basename included', () => {
expect(createHref('/test', mockHistory)).toEqual('/app/enterprise_search/test');
});

it('does not include the basename if shouldNotCreateHref is passed', () => {
expect(createHref('/test', mockHistory, { shouldNotCreateHref: true })).toEqual('/test');
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { History } from 'history';

/**
* This helper uses React Router's createHref function to generate links with router basenames accounted for.
* For example, if we perform navigateToUrl('/engines') within App Search, we expect the app basename
* to be taken into account to be intelligently routed to '/app/enterprise_search/app_search/engines'.
*
* This helper accomplishes that, while still giving us an escape hatch for navigation *between* apps.
* For example, if we want to navigate the user from App Search to Enterprise Search we could
* navigateToUrl('/app/enterprise_search', { shouldNotCreateHref: true })
*/
export interface ICreateHrefOptions {
shouldNotCreateHref?: boolean;
}
export const createHref = (
path: string,
history: History,
options?: ICreateHrefOptions
): string => {
return options?.shouldNotCreateHref ? path : history.createHref({ pathname: path });
};
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
*/

import '../../__mocks__/kea.mock';
import '../../__mocks__/react_router_history.mock';

import React from 'react';
import { shallow, mount } from 'enzyme';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,10 @@

import React from 'react';
import { useValues } from 'kea';
import { useHistory } from 'react-router-dom';
import { EuiLink, EuiButton, EuiButtonProps, EuiLinkAnchorProps } from '@elastic/eui';

import { KibanaLogic } from '../../shared/kibana';
import { letBrowserHandleEvent } from './link_events';
import { letBrowserHandleEvent, createHref } from './';

/**
* Generates either an EuiLink or EuiButton with a React-Router-ified link
Expand All @@ -33,11 +32,10 @@ export const EuiReactRouterHelper: React.FC<IEuiReactRouterProps> = ({
shouldNotCreateHref,
children,
}) => {
const history = useHistory();
const { navigateToUrl } = useValues(KibanaLogic);
const { navigateToUrl, history } = useValues(KibanaLogic);

// Generate the correct link href (with basename etc. accounted for)
const href = shouldNotCreateHref ? to : history.createHref({ pathname: to });
const href = createHref(to, history, { shouldNotCreateHref });

const reactRouterLinkClick = (event: React.MouseEvent) => {
if (onClick) onClick(); // Run any passed click events (e.g. telemetry)
Expand All @@ -47,7 +45,7 @@ export const EuiReactRouterHelper: React.FC<IEuiReactRouterProps> = ({
event.preventDefault();

// Perform SPA navigation.
navigateToUrl(href);
navigateToUrl(to, { shouldNotCreateHref });
};

const reactRouterProps = { href, onClick: reactRouterLinkClick };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,6 @@
*/

export { letBrowserHandleEvent } from './link_events';
export { createHref, ICreateHrefOptions } from './create_href';
export { EuiReactRouterLink as EuiLink } from './eui_link';
export { EuiReactRouterButton as EuiButton } from './eui_link';

0 comments on commit e873848

Please sign in to comment.