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.

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

[SIEM] Migrating frontend services to NP #52783

Merged
merged 40 commits into from
Dec 20, 2019
Merged
Show file tree
Hide file tree
Changes from 38 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
021c4e1
Remove legacy index_patterns import
rylnd Dec 11, 2019
bc62682
Replace ui/documentation_links with core NP service
rylnd Dec 11, 2019
f23a295
Update snapshots following new docLink mocks
rylnd Dec 11, 2019
3d8ac01
Remove unused manual mocks
rylnd Dec 11, 2019
1cabdd9
WIP: Use kibana core mock everywhere we're doing it manually
rylnd Dec 11, 2019
62906ad
Use new mocks on timeline test that doesn't hang
rylnd Dec 11, 2019
566d373
Remove remaining uses of mockUiSettings in useKibanaCore mocks
rylnd Dec 11, 2019
e61b37d
Invoke platform's mock factory at mock time
rylnd Dec 12, 2019
d08b492
WIP: migrating to use kibana_react's provider and helpers
rylnd Dec 16, 2019
86d047b
WIP: Migrating to use kibana_react
rylnd Dec 16, 2019
c412394
Bind a copy of kibana at mock creation
rylnd Dec 16, 2019
2b3e663
Remove internal context providers and last usage
rylnd Dec 16, 2019
21a20b5
Fix tests failing due to wrong mocks
rylnd Dec 16, 2019
51f5619
Fix test failures related to date formatting
rylnd Dec 16, 2019
5a127e1
Remove unnecessary and/or redundant mocks
rylnd Dec 16, 2019
b7ff02f
Refactor kibana_react mocks
rylnd Dec 16, 2019
ea1a780
Replace usage of chrome.getUiSettingsClient with useUiSetting
rylnd Dec 16, 2019
633bc16
Remove ui_settings mocks
rylnd Dec 16, 2019
2d4c1a2
Remove siem's UI settings hook
rylnd Dec 16, 2019
9be9f4f
Use withKibana HOC on our component classes
rylnd Dec 16, 2019
c0d2a4a
Set defaults for some unknown UI settings
rylnd Dec 16, 2019
7fcc147
Remove old hooks
rylnd Dec 16, 2019
b78bf54
Fix type error on usage of useKibana hook
rylnd Dec 16, 2019
f982691
Fix type error on ML call
rylnd Dec 16, 2019
5843449
Export a 'bound' version of the useKibana function
rylnd Dec 16, 2019
d62d583
Instantiate our mock function
rylnd Dec 16, 2019
9f4f0dd
Fix test that relies on unmocked service
rylnd Dec 16, 2019
8c1282e
Remove use of ui/chrome in our charts
rylnd Dec 17, 2019
0fab29b
Remove last use of chrome.getUiSettingsClient
rylnd Dec 17, 2019
62a40d6
Remove unnecessary non-null assertions
rylnd Dec 17, 2019
8ff4f77
Fix chart tests
rylnd Dec 17, 2019
deb1ea1
Replace missing mock
rylnd Dec 17, 2019
ad83050
Merge branch 'master' into siem_ui_new_platform_services
elasticmachine Dec 17, 2019
c5e9fbb
Merge branch 'master' into siem_ui_new_platform_services
rylnd Dec 17, 2019
aa8fe1b
Add back tests for our theming hook
rylnd Dec 17, 2019
c517c89
Style: cleanup
rylnd Dec 17, 2019
83f4eb4
Merge branch 'master' into siem_ui_new_platform_services
rylnd Dec 17, 2019
26a6d0e
Merge branch 'master' into siem_ui_new_platform_services
rylnd Dec 18, 2019
cd3bd29
Remove unneeded default UI Settings values
rylnd Dec 19, 2019
512a949
Simplify kibana_react mocks
rylnd Dec 19, 2019
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
13 changes: 12 additions & 1 deletion x-pack/legacy/plugins/siem/public/apps/plugin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,16 @@ import { Chrome } from 'ui/chrome';

import { SiemApp } from './start_app';
import template from './template.html';
import { DEFAULT_KBN_VERSION, DEFAULT_TIMEZONE_BROWSER } from '../../common/constants';

export const ROOT_ELEMENT_ID = 'react-siem-root';

export type StartCore = LegacyCoreStart;
export type StartPlugins = Required<
Pick<PluginsStart, 'data' | 'embeddable' | 'inspector' | 'uiActions'>
>;
export type StartServices = StartCore & StartPlugins;

export class Plugin {
constructor(
// @ts-ignore this is added to satisfy the New Platform typing constraint,
Expand All @@ -25,7 +32,11 @@ export class Plugin {
this.chrome = chrome;
}

public start(core: LegacyCoreStart, plugins: PluginsStart) {
public start(core: StartCore, plugins: StartPlugins) {
// TODO(rylnd): once we're on NP, we can populate version from env.packageInfo
core.uiSettings.set(DEFAULT_KBN_VERSION, '8.0.0');
core.uiSettings.set(DEFAULT_TIMEZONE_BROWSER, 'UTC');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get an error from the service when I try to retrieve this value. I know I can get the version from the plugin's init context, but I'm unsure if/where there's an equivalent for this setting, or if the warning is just a bug.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What error are you getting? Why do you set these settings here? Did you mean to use core.uiSettings.get?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was receiving warnings when trying to get these two keys. The UI Settings service did not seem to know about them and advised I set defaults. I don't see that now, though, so I'll remove these 🤷‍♂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshdover Ok, the rest of my team hit this when it hit master. For a fresh app, the service doesn't know about these keys, and throws errors. On my instance, these defaults were persisted to my kibana index, which is why I didn't see the error after removing these lines.

The unknown keys here are kbnVersion and timezoneBrowser. I know that the version is available on the initializer context, so I could understand it being removed from UI settings, but I'm not sure about timezoneBrowser.


// @ts-ignore improper type description
this.chrome.setRootTemplate(template);
const checkForRoot = () => {
Comment on lines 36 to 37
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you can replace all this code with the NP mounting mechanism in setup: core.application.register

Expand Down
40 changes: 15 additions & 25 deletions x-pack/legacy/plugins/siem/public/apps/start_app.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ import React, { memo, FC } from 'react';
import { ApolloProvider } from 'react-apollo';
import { Provider as ReduxStoreProvider } from 'react-redux';
import { ThemeProvider } from 'styled-components';
import { LegacyCoreStart } from 'kibana/public';
import { PluginsStart } from 'ui/new_platform/new_platform';

import { EuiErrorBoundary } from '@elastic/eui';
import euiDarkVars from '@elastic/eui/dist/eui_theme_dark.json';
Expand All @@ -19,16 +17,14 @@ import { BehaviorSubject } from 'rxjs';
import { pluck } from 'rxjs/operators';
import { I18nContext } from 'ui/i18n';

import { KibanaContextProvider } from '../../../../../../src/plugins/kibana_react/public';
import { KibanaContextProvider, useUiSetting$ } from '../lib/kibana';
import { Storage } from '../../../../../../src/plugins/kibana_utils/public';

import { DEFAULT_DARK_MODE } from '../../common/constants';
import { ErrorToastDispatcher } from '../components/error_toast_dispatcher';
import { compose } from '../lib/compose/kibana_compose';
import { AppFrontendLibs } from '../lib/lib';
import { KibanaCoreContextProvider } from '../lib/compose/kibana_core';
import { KibanaPluginsContextProvider } from '../lib/compose/kibana_plugins';
import { useKibanaUiSetting } from '../lib/settings/use_kibana_ui_setting';
import { StartCore, StartPlugins } from './plugin';
import { PageRouter } from '../routes';
import { createStore } from '../store';
import { GlobalToaster, ManageGlobalToaster } from '../components/toasters';
Expand All @@ -44,7 +40,7 @@ const StartApp: FC<AppFrontendLibs> = memo(libs => {
const store = createStore(undefined, libs$.pipe(pluck('apolloClient')));
Copy link
Contributor Author

@rylnd rylnd Dec 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rudolf we have a few uses of chrome.getUiSettingsClient remaining in functions that we use to populate part of our initial redux state (time filter preferences, etc.). Here's how we're thinking of tackling that, just wanted a sanity check from your end:

  1. Define the setup() method of our plugin
  2. Fetch those UI settings in setup, and do some merging with the rest of our static initial state to create our store object
  3. Pass that store to the store Provider (via a private plugin property?) during the start() phase

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Step 1 & 2 sound fine, however step 3 is a bit off. Registering applications is done during setup via the core.application.register API. You'll be able to use your initial store right there without using a private property on your plugin class.


const AppPluginRoot = memo(() => {
const [darkMode] = useKibanaUiSetting(DEFAULT_DARK_MODE);
const [darkMode] = useUiSetting$<boolean>(DEFAULT_DARK_MODE);
return (
<EuiErrorBoundary>
<I18nContext>
Expand Down Expand Up @@ -77,21 +73,15 @@ const StartApp: FC<AppFrontendLibs> = memo(libs => {

export const ROOT_ELEMENT_ID = 'react-siem-root';

export const SiemApp = memo<{ core: LegacyCoreStart; plugins: PluginsStart }>(
({ core, plugins }) => (
<KibanaContextProvider
services={{
appName: 'siem',
data: plugins.data,
storage: new Storage(localStorage),
...core,
}}
>
<KibanaCoreContextProvider core={core}>
<KibanaPluginsContextProvider plugins={plugins}>
<StartApp {...compose()} />
</KibanaPluginsContextProvider>
</KibanaCoreContextProvider>
</KibanaContextProvider>
)
);
export const SiemApp = memo<{ core: StartCore; plugins: StartPlugins }>(({ core, plugins }) => (
<KibanaContextProvider
services={{
appName: 'siem',
data: plugins.data,
storage: new Storage(localStorage),
...core,
}}
>
<StartApp {...compose()} />
</KibanaContextProvider>
));
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@ import { useMountAppended } from '../../utils/use_mount_appended';

import { Bytes } from '.';

jest.mock('../../lib/settings/use_kibana_ui_setting');

describe('Bytes', () => {
const mount = useMountAppended();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import { AreaChartBaseComponent, AreaChartComponent } from './areachart';
import { ChartSeriesData } from './common';
import { ScaleType, AreaSeries, Axis } from '@elastic/charts';

jest.mock('../../lib/kibana');

const customHeight = '100px';
const customWidth = '120px';
const chartDataSets = [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,15 @@ import { getOr, get, isNull, isNumber } from 'lodash/fp';
import { AutoSizer } from '../auto_sizer';
import { ChartPlaceHolder } from './chart_place_holder';
import {
browserTimezone,
chartDefaultSettings,
ChartSeriesConfigs,
ChartSeriesData,
getChartHeight,
getChartWidth,
getSeriesStyle,
WrappedByAutoSizer,
useTheme,
useBrowserTimeZone,
} from './common';

// custom series styles: https://ela.st/areachart-styling
Expand Down Expand Up @@ -72,12 +73,15 @@ export const AreaChartBaseComponent = ({
height: string | null | undefined;
configs?: ChartSeriesConfigs | undefined;
}) => {
const theme = useTheme();
const timeZone = useBrowserTimeZone();
const xTickFormatter = get('configs.axis.xTickFormatter', chartConfigs);
const yTickFormatter = get('configs.axis.yTickFormatter', chartConfigs);
const xAxisId = getAxisId(`group-${data[0].key}-x`);
const yAxisId = getAxisId(`group-${data[0].key}-y`);
const settings = {
...chartDefaultSettings,
theme,
...get('configs.settings', chartConfigs),
};
return chartConfigs.width && chartConfigs.height ? (
Expand All @@ -95,7 +99,7 @@ export const AreaChartBaseComponent = ({
data={series.value || undefined}
xScaleType={getOr(ScaleType.Linear, 'configs.series.xScaleType', chartConfigs)}
yScaleType={getOr(ScaleType.Linear, 'configs.series.yScaleType', chartConfigs)}
timeZone={browserTimezone}
timeZone={timeZone}
xAccessor="x"
yAccessors={['y']}
areaSeriesStyle={getSeriesLineStyle()}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import { BarChartBaseComponent, BarChartComponent } from './barchart';
import { ChartSeriesData } from './common';
import { BarSeries, ScaleType, Axis } from '@elastic/charts';

jest.mock('../../lib/kibana');

const customHeight = '100px';
const customWidth = '120px';
const chartDataSets = [
Expand Down
10 changes: 7 additions & 3 deletions x-pack/legacy/plugins/siem/public/components/charts/barchart.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,17 @@ import { getOr, get, isNumber } from 'lodash/fp';
import { AutoSizer } from '../auto_sizer';
import { ChartPlaceHolder } from './chart_place_holder';
import {
browserTimezone,
chartDefaultSettings,
ChartSeriesConfigs,
ChartSeriesData,
checkIfAllValuesAreZero,
getSeriesStyle,
getChartHeight,
getChartWidth,
getSeriesStyle,
SeriesType,
WrappedByAutoSizer,
useBrowserTimeZone,
useTheme,
} from './common';

const checkIfAllTheDataInTheSeriesAreValid = (series: ChartSeriesData): series is ChartSeriesData =>
Expand All @@ -53,13 +54,16 @@ export const BarChartBaseComponent = ({
height: string | null | undefined;
configs?: ChartSeriesConfigs | undefined;
}) => {
const theme = useTheme();
const timeZone = useBrowserTimeZone();
const xTickFormatter = get('configs.axis.xTickFormatter', chartConfigs);
const yTickFormatter = get('configs.axis.yTickFormatter', chartConfigs);
const tickSize = getOr(0, 'configs.axis.tickSize', chartConfigs);
const xAxisId = getAxisId(`stat-items-barchart-${data[0].key}-x`);
const yAxisId = getAxisId(`stat-items-barchart-${data[0].key}-y`);
const settings = {
...chartDefaultSettings,
theme,
...get('configs.settings', chartConfigs),
};

Expand All @@ -79,7 +83,7 @@ export const BarChartBaseComponent = ({
yScaleType={getOr(ScaleType.Linear, 'configs.series.yScaleType', chartConfigs)}
xAccessor="x"
yAccessors={['y']}
timeZone={browserTimezone}
timeZone={timeZone}
splitSeriesAccessors={['g']}
data={series.value!}
stackAccessors={get('configs.series.stackAccessors', chartConfigs)}
Expand Down
44 changes: 26 additions & 18 deletions x-pack/legacy/plugins/siem/public/components/charts/common.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,27 @@
*/
import { shallow } from 'enzyme';
import React from 'react';
import { renderHook } from '@testing-library/react-hooks';

import { useUiSetting } from '../../lib/kibana';
import {
checkIfAllValuesAreZero,
defaultChartHeight,
getChartHeight,
getChartWidth,
getSeriesStyle,
getTheme,
SeriesType,
WrappedByAutoSizer,
ChartSeriesData,
useTheme,
} from './common';
import { mergeWithDefaultTheme, LIGHT_THEME } from '@elastic/charts';

jest.mock('../../lib/kibana');

jest.mock('@elastic/charts', () => {
return {
...jest.requireActual('@elastic/charts'),
getSpecId: jest.fn(() => {}),
mergeWithDefaultTheme: jest.fn(),
};
});

Expand Down Expand Up @@ -57,21 +61,6 @@ describe('getSeriesStyle', () => {
});
});

describe('getTheme', () => {
it('should merge custom theme with default theme', () => {
const defaultTheme = {
chartMargins: { bottom: 0, left: 0, right: 0, top: 4 },
chartPaddings: { bottom: 0, left: 0, right: 0, top: 0 },
scales: {
barsPadding: 0.05,
},
};
getTheme();
expect((mergeWithDefaultTheme as jest.Mock).mock.calls[0][0]).toMatchObject(defaultTheme);
expect((mergeWithDefaultTheme as jest.Mock).mock.calls[0][1]).toEqual(LIGHT_THEME);
});
});

describe('getChartHeight', () => {
it('should render customHeight', () => {
const height = getChartHeight(10, 100);
Expand Down Expand Up @@ -197,4 +186,23 @@ describe('checkIfAllValuesAreZero', () => {
expect(result).toBeTruthy();
});
});

describe('useTheme', () => {
it('merges our spacing with the default theme', () => {
const { result } = renderHook(() => useTheme());

expect(result.current).toEqual(
expect.objectContaining({ chartMargins: expect.objectContaining({ top: 4, bottom: 0 }) })
);
});

it('returns a different theme depending on user settings', () => {
const { result: defaultResult } = renderHook(() => useTheme());
(useUiSetting as jest.Mock).mockImplementation(() => true);

const { result: darkResult } = renderHook(() => useTheme());

expect(defaultResult.current).not.toMatchObject(darkResult.current);
});
});
});
50 changes: 26 additions & 24 deletions x-pack/legacy/plugins/siem/public/components/charts/common.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
* you may not use this file except in compliance with the Elastic License.
*/

import chrome from 'ui/chrome';
import {
CustomSeriesColorsMap,
DARK_THEME,
Expand All @@ -21,6 +20,7 @@ import {
} from '@elastic/charts';
import moment from 'moment-timezone';
import styled from 'styled-components';
import { useUiSetting } from '../../lib/kibana';
import { DEFAULT_DATE_FORMAT_TZ, DEFAULT_DARK_MODE } from '../../../common/constants';

export const defaultChartHeight = '100%';
Expand Down Expand Up @@ -95,27 +95,28 @@ export const getSeriesStyle = (
};

// Apply margins and paddings: https://ela.st/charts-spacing
export const getTheme = () => {
const theme: PartialTheme = {
chartMargins: {
left: 0,
right: 0,
// Apply some paddings to the top to avoid chopping the y tick https://ela.st/chopping-edge
top: 4,
bottom: 0,
},
chartPaddings: {
left: 0,
right: 0,
top: 0,
bottom: 0,
},
scales: {
barsPadding: 0.05,
},
};
const isDarkMode: boolean = chrome.getUiSettingsClient().get(DEFAULT_DARK_MODE);
const theme: PartialTheme = {
chartMargins: {
left: 0,
right: 0,
// Apply some paddings to the top to avoid chopping the y tick https://ela.st/chopping-edge
top: 4,
bottom: 0,
},
chartPaddings: {
left: 0,
right: 0,
top: 0,
bottom: 0,
},
scales: {
barsPadding: 0.05,
},
};
export const useTheme = () => {
const isDarkMode = useUiSetting<boolean>(DEFAULT_DARK_MODE);
const defaultTheme = isDarkMode ? DARK_THEME : LIGHT_THEME;

return mergeWithDefaultTheme(theme, defaultTheme);
};

Expand All @@ -126,11 +127,12 @@ export const chartDefaultSettings = {
showLegend: false,
showLegendDisplayValue: false,
debug: false,
theme: getTheme(),
};

const kibanaTimezone: string = chrome.getUiSettingsClient().get(DEFAULT_DATE_FORMAT_TZ);
export const browserTimezone = kibanaTimezone === 'Browser' ? moment.tz.guess() : kibanaTimezone;
export const useBrowserTimeZone = () => {
const kibanaTimezone = useUiSetting<string>(DEFAULT_DATE_FORMAT_TZ);
return kibanaTimezone === 'Browser' ? moment.tz.guess() : kibanaTimezone;
};

export const getChartHeight = (customHeight?: number, autoSizerHeight?: number): string => {
const height = customHeight || autoSizerHeight;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import { mount, shallow } from 'enzyme';
import toJson from 'enzyme-to-json';
import React from 'react';

import '../../../mock/ui_settings';
import { TestProviders } from '../../../mock';
import {
UtilityBar,
Expand All @@ -19,8 +18,6 @@ import {
UtilityBarText,
} from './index';

jest.mock('../../../lib/settings/use_kibana_ui_setting');

describe('UtilityBar', () => {
test('it renders', () => {
const wrapper = shallow(
Expand Down
Loading