Skip to content

Commit

Permalink
[Security Solution][Detections] Better toast errors (#72205) (#72223)
Browse files Browse the repository at this point in the history
## Summary

A few changes related to toast messages:

* Better presentation of error data sent from the API (both general platform errors and app-specific errors)
* Uses platform's new Toasts service to prevent modal/toast z-index collision issues
* Updates both the Value Lists Modal and the Exceptions List Modals to use this new hook wrapping the above

This also fixes an issue with `useListsConfig` where, when the server is nonresponsive, it will not continue to poll awaiting a non-error response.
  • Loading branch information
rylnd authored Jul 17, 2020
1 parent bfb3bb9 commit ddec1b5
Show file tree
Hide file tree
Showing 14 changed files with 211 additions and 101 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ import {
} from '../../../../../public/lists_plugin_deps';
import * as i18n from './translations';
import { TimelineNonEcsData, Ecs } from '../../../../graphql/types';
import { useAppToasts } from '../../../hooks/use_app_toasts';
import { useKibana } from '../../../lib/kibana';
import { errorToToaster, displaySuccessToast, useStateToaster } from '../../toasters';
import { ExceptionBuilder } from '../builder';
import { Loader } from '../../loader';
import { useAddOrUpdateException } from '../use_add_exception';
Expand Down Expand Up @@ -115,7 +115,7 @@ export const AddExceptionModal = memo(function AddExceptionModal({
Array<ExceptionListItemSchema | CreateExceptionListItemSchema>
>([]);
const [fetchOrCreateListError, setFetchOrCreateListError] = useState(false);
const [, dispatchToaster] = useStateToaster();
const { addError, addSuccess } = useAppToasts();
const { loading: isSignalIndexLoading, signalIndexName } = useSignalIndex();

const [{ isLoading: indexPatternLoading, indexPatterns }] = useFetchIndexPatterns(
Expand All @@ -124,15 +124,15 @@ export const AddExceptionModal = memo(function AddExceptionModal({

const onError = useCallback(
(error: Error) => {
errorToToaster({ title: i18n.ADD_EXCEPTION_ERROR, error, dispatchToaster });
addError(error, { title: i18n.ADD_EXCEPTION_ERROR });
onCancel();
},
[dispatchToaster, onCancel]
[addError, onCancel]
);
const onSuccess = useCallback(() => {
displaySuccessToast(i18n.ADD_EXCEPTION_SUCCESS, dispatchToaster);
addSuccess(i18n.ADD_EXCEPTION_SUCCESS);
onConfirm(shouldCloseAlert);
}, [dispatchToaster, onConfirm, shouldCloseAlert]);
}, [addSuccess, onConfirm, shouldCloseAlert]);

const [{ isLoading: addExceptionIsLoading }, addOrUpdateExceptionItems] = useAddOrUpdateException(
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import {
} from '../../../../../public/lists_plugin_deps';
import * as i18n from './translations';
import { useKibana } from '../../../lib/kibana';
import { errorToToaster, displaySuccessToast, useStateToaster } from '../../toasters';
import { useAppToasts } from '../../../hooks/use_app_toasts';
import { ExceptionBuilder } from '../builder';
import { useAddOrUpdateException } from '../use_add_exception';
import { AddExceptionComments } from '../add_exception_comments';
Expand Down Expand Up @@ -93,7 +93,7 @@ export const EditExceptionModal = memo(function EditExceptionModal({
const [exceptionItemsToAdd, setExceptionItemsToAdd] = useState<
Array<ExceptionListItemSchema | CreateExceptionListItemSchema>
>([]);
const [, dispatchToaster] = useStateToaster();
const { addError, addSuccess } = useAppToasts();
const { loading: isSignalIndexLoading, signalIndexName } = useSignalIndex();

const [{ isLoading: indexPatternLoading, indexPatterns }] = useFetchIndexPatterns(
Expand All @@ -102,15 +102,15 @@ export const EditExceptionModal = memo(function EditExceptionModal({

const onError = useCallback(
(error) => {
errorToToaster({ title: i18n.EDIT_EXCEPTION_ERROR, error, dispatchToaster });
addError(error, { title: i18n.EDIT_EXCEPTION_ERROR });
onCancel();
},
[dispatchToaster, onCancel]
[addError, onCancel]
);
const onSuccess = useCallback(() => {
displaySuccessToast(i18n.EDIT_EXCEPTION_SUCCESS, dispatchToaster);
addSuccess(i18n.EDIT_EXCEPTION_SUCCESS);
onConfirm();
}, [dispatchToaster, onConfirm]);
}, [addSuccess, onConfirm]);

const [{ isLoading: addExceptionIsLoading }, addOrUpdateExceptionItems] = useAddOrUpdateException(
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import { ExceptionItem } from './';
import { getExceptionListItemSchemaMock } from '../../../../../../../lists/common/schemas/response/exception_list_item_schema.mock';
import { getCommentsArrayMock } from '../../../../../../../lists/common/schemas/types/comments.mock';

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

describe('ExceptionItem', () => {
it('it renders ExceptionDetails and ExceptionEntries', () => {
const exceptionItem = getExceptionListItemSchemaMock();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { isError } from 'lodash/fp';

import { AppToast, ActionToaster } from './';
import { isToasterError } from './errors';
import { isApiError } from '../../utils/api';
import { isAppError } from '../../utils/api';

/**
* Displays an error toast for the provided title and message
Expand Down Expand Up @@ -114,7 +114,7 @@ export const errorToToaster = ({
iconType,
errors: error.messages,
};
} else if (isApiError(error)) {
} else if (isAppError(error)) {
toast = {
id,
title,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/*
* 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 { renderHook } from '@testing-library/react-hooks';

import { useToasts } from '../lib/kibana';
import { useAppToasts } from './use_app_toasts';

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

describe('useDeleteList', () => {
let addErrorMock: jest.Mock;
let addSuccessMock: jest.Mock;

beforeEach(() => {
addErrorMock = jest.fn();
addSuccessMock = jest.fn();
(useToasts as jest.Mock).mockImplementation(() => ({
addError: addErrorMock,
addSuccess: addSuccessMock,
}));
});

it('works normally with a regular error', async () => {
const error = new Error('regular error');
const { result } = renderHook(() => useAppToasts());

result.current.addError(error, { title: 'title' });

expect(addErrorMock).toHaveBeenCalledWith(error, { title: 'title' });
});

it("uses a AppError's body.message as the toastMessage", async () => {
const kibanaApiError = {
message: 'Not Found',
body: { status_code: 404, message: 'Detailed Message' },
};

const { result } = renderHook(() => useAppToasts());

result.current.addError(kibanaApiError, { title: 'title' });

expect(addErrorMock).toHaveBeenCalledWith(kibanaApiError, {
title: 'title',
toastMessage: 'Detailed Message',
});
});

it('converts an unknown error to an Error', () => {
const unknownError = undefined;

const { result } = renderHook(() => useAppToasts());

result.current.addError(unknownError, { title: 'title' });

expect(addErrorMock).toHaveBeenCalledWith(Error(`${undefined}`), {
title: 'title',
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/*
* 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 { useCallback, useRef } from 'react';

import { ErrorToastOptions, ToastsStart, Toast } from '../../../../../../src/core/public';
import { useToasts } from '../lib/kibana';
import { isAppError, AppError } from '../utils/api';

export type UseAppToasts = Pick<ToastsStart, 'addSuccess'> & {
api: ToastsStart;
addError: (error: unknown, options: ErrorToastOptions) => Toast;
};

export const useAppToasts = (): UseAppToasts => {
const toasts = useToasts();
const addError = useRef(toasts.addError.bind(toasts)).current;
const addSuccess = useRef(toasts.addSuccess.bind(toasts)).current;

const addAppError = useCallback(
(error: AppError, options: ErrorToastOptions) =>
addError(error, {
...options,
toastMessage: error.body.message,
}),
[addError]
);

const _addError = useCallback(
(error: unknown, options: ErrorToastOptions) => {
if (isAppError(error)) {
return addAppError(error, options);
} else {
if (error instanceof Error) {
return addError(error, options);
} else {
return addError(new Error(String(error)), options);
}
}
},
[addAppError, addError]
);

return { api: toasts, addError: _addError, addSuccess };
};
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,12 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { EuiGlobalToastListToast as Toast, EuiButtonIcon } from '@elastic/eui';
import { EuiButtonIcon } from '@elastic/eui';
import copy from 'copy-to-clipboard';
import React from 'react';
import uuid from 'uuid';

import * as i18n from './translations';
import { useStateToaster } from '../../components/toasters';
import { useAppToasts } from '../../hooks/use_app_toasts';

export type OnCopy = ({
content,
Expand All @@ -20,17 +19,6 @@ export type OnCopy = ({
isSuccess: boolean;
}) => void;

interface GetSuccessToastParams {
titleSummary?: string;
}

const getSuccessToast = ({ titleSummary }: GetSuccessToastParams): Toast => ({
id: `copy-success-${uuid.v4()}`,
color: 'success',
iconType: 'copyClipboard',
title: `${i18n.COPIED} ${titleSummary} ${i18n.TO_THE_CLIPBOARD}`,
});

interface Props {
children?: JSX.Element;
content: string | number;
Expand All @@ -40,7 +28,7 @@ interface Props {
}

export const Clipboard = ({ children, content, onCopy, titleSummary, toastLifeTimeMs }: Props) => {
const dispatchToaster = useStateToaster()[1];
const { addSuccess } = useAppToasts();
const onClick = (event: React.MouseEvent<HTMLButtonElement>) => {
event.preventDefault();
event.stopPropagation();
Expand All @@ -52,10 +40,7 @@ export const Clipboard = ({ children, content, onCopy, titleSummary, toastLifeTi
}

if (isSuccess) {
dispatchToaster({
type: 'addToaster',
toast: { toastLifeTimeMs, ...getSuccessToast({ titleSummary }) },
});
addSuccess(`${i18n.COPIED} ${titleSummary} ${i18n.TO_THE_CLIPBOARD}`, { toastLifeTimeMs });
}
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { notificationServiceMock } from '../../../../../../../../src/core/public/mocks';
import {
createKibanaContextProviderMock,
createUseUiSettingMock,
Expand All @@ -19,6 +20,7 @@ export const useUiSetting$ = jest.fn(createUseUiSetting$Mock());
export const useTimeZone = jest.fn();
export const useDateFormat = jest.fn();
export const useBasePath = jest.fn(() => '/test/base/path');
export const useToasts = jest.fn(() => notificationServiceMock.createStartContract().toasts);
export const useCurrentUser = jest.fn();
export const withKibana = jest.fn(createWithKibanaMock());
export const KibanaContextProvider = jest.fn(createKibanaContextProviderMock());
23 changes: 21 additions & 2 deletions x-pack/plugins/security_solution/public/common/utils/api/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,33 @@

import { has } from 'lodash/fp';

export interface KibanaApiError {
export interface AppError {
name: string;
message: string;
body: {
message: string;
};
}

export interface KibanaError extends AppError {
body: {
message: string;
statusCode: number;
};
}

export interface SecurityAppError extends AppError {
body: {
message: string;
status_code: number;
};
}

export const isApiError = (error: unknown): error is KibanaApiError =>
export const isKibanaError = (error: unknown): error is KibanaError =>
has('message', error) && has('body.message', error) && has('body.statusCode', error);

export const isSecurityAppError = (error: unknown): error is SecurityAppError =>
has('message', error) && has('body.message', error) && has('body.status_code', error);

export const isAppError = (error: unknown): error is AppError =>
isKibanaError(error) || isSecurityAppError(error);
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ import {
useDeleteList,
useCursor,
} from '../../../shared_imports';
import { useToasts, useKibana } from '../../../common/lib/kibana';
import { useKibana } from '../../../common/lib/kibana';
import { useAppToasts } from '../../../common/hooks/use_app_toasts';
import { GenericDownloader } from '../../../common/components/generic_downloader';
import * as i18n from './translations';
import { ValueListsTable } from './table';
Expand All @@ -45,7 +46,7 @@ export const ValueListsModalComponent: React.FC<ValueListsModalProps> = ({
const { start: findLists, ...lists } = useFindLists();
const { start: deleteList, result: deleteResult } = useDeleteList();
const [exportListId, setExportListId] = useState<string>();
const toasts = useToasts();
const { addError, addSuccess } = useAppToasts();

const fetchLists = useCallback(() => {
findLists({ cursor, http, pageIndex: pageIndex + 1, pageSize });
Expand Down Expand Up @@ -82,21 +83,21 @@ export const ValueListsModalComponent: React.FC<ValueListsModalProps> = ({
const handleUploadError = useCallback(
(error: Error) => {
if (error.name !== 'AbortError') {
toasts.addError(error, { title: i18n.UPLOAD_ERROR });
addError(error, { title: i18n.UPLOAD_ERROR });
}
},
[toasts]
[addError]
);
const handleUploadSuccess = useCallback(
(response: ListSchema) => {
toasts.addSuccess({
addSuccess({
text: i18n.uploadSuccessMessage(response.name),
title: i18n.UPLOAD_SUCCESS_TITLE,
});
fetchLists();
},
// eslint-disable-next-line react-hooks/exhaustive-deps
[toasts]
[addSuccess]
);

useEffect(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { useEffect, useState } from 'react';
import { errorToToaster, useStateToaster } from '../../../../common/components/toasters';
import { createSignalIndex, getSignalIndex } from './api';
import * as i18n from './translations';
import { isApiError } from '../../../../common/utils/api';
import { isSecurityAppError } from '../../../../common/utils/api';

type Func = () => void;

Expand Down Expand Up @@ -59,7 +59,7 @@ export const useSignalIndex = (): ReturnSignalIndex => {
signalIndexName: null,
createDeSignalIndex: createIndex,
});
if (isApiError(error) && error.body.status_code !== 404) {
if (isSecurityAppError(error) && error.body.status_code !== 404) {
errorToToaster({ title: i18n.SIGNAL_GET_NAME_FAILURE, error, dispatchToaster });
}
}
Expand All @@ -81,7 +81,7 @@ export const useSignalIndex = (): ReturnSignalIndex => {
}
} catch (error) {
if (isSubscribed) {
if (isApiError(error) && error.body.status_code === 409) {
if (isSecurityAppError(error) && error.body.status_code === 409) {
fetchData();
} else {
setSignalIndex({
Expand Down
Loading

0 comments on commit ddec1b5

Please sign in to comment.