Skip to content

Commit

Permalink
fixed typing in alert flyouts and race condition on alertType id update
Browse files Browse the repository at this point in the history
  • Loading branch information
gmmorris committed Nov 17, 2020
1 parent 1b1c532 commit 158e1e3
Show file tree
Hide file tree
Showing 11 changed files with 172 additions and 86 deletions.
3 changes: 2 additions & 1 deletion x-pack/plugins/alerts/common/alert.ts
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 { SavedObjectAttributes } from 'kibana/server';
import { SavedObjectAttribute, SavedObjectAttributes } from 'kibana/server';

// eslint-disable-next-line @typescript-eslint/no-explicit-any
export type AlertTypeState = Record<string, any>;
Expand Down Expand Up @@ -37,6 +37,7 @@ export interface AlertExecutionStatus {
}

export type AlertActionParams = SavedObjectAttributes;
export type AlertActionParam = SavedObjectAttribute;

export interface AlertAction {
group: string;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@ import React, { Fragment, FunctionComponent, useEffect, useRef } from 'react';
import { EuiFormRow } from '@elastic/eui';
import { FormattedMessage } from '@kbn/i18n/react';
import { i18n } from '@kbn/i18n';
import { IErrorObject, AlertsContextValue } from '../../../../../../triggers_actions_ui/public';
import {
IErrorObject,
AlertsContextValue,
AlertTypeParamsExpressionProps,
} from '../../../../../../triggers_actions_ui/public';
import { ES_GEO_FIELD_TYPES } from '../../types';
import { GeoIndexPatternSelect } from '../util_components/geo_index_pattern_select';
import { SingleFieldSelect } from '../util_components/single_field_select';
Expand All @@ -23,7 +27,7 @@ interface Props {
errors: IErrorObject;
setAlertParamsDate: (date: string) => void;
setAlertParamsGeoField: (geoField: string) => void;
setAlertProperty: (alertProp: string, alertParams: unknown) => void;
setAlertProperty: AlertTypeParamsExpressionProps['setAlertProperty'];
setIndexPattern: (indexPattern: IIndexPattern) => void;
indexPattern: IIndexPattern;
isInvalid: boolean;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { Alert, AlertType } from '../../types';
import { AlertType } from '../../types';
import { InitialAlert } from '../sections/alert_form/alert_reducer';

/**
* NOTE: Applications that want to show the alerting UIs will need to add
Expand All @@ -21,9 +22,9 @@ export const hasExecuteActionsCapability = (capabilities: Capabilities) =>
export const hasDeleteActionsCapability = (capabilities: Capabilities) =>
capabilities?.actions?.delete;

export function hasAllPrivilege(alert: Alert, alertType?: AlertType): boolean {
export function hasAllPrivilege(alert: InitialAlert, alertType?: AlertType): boolean {
return alertType?.authorizedConsumers[alert.consumer]?.all ?? false;
}
export function hasReadPrivilege(alert: Alert, alertType?: AlertType): boolean {
export function hasReadPrivilege(alert: InitialAlert, alertType?: AlertType): boolean {
return alertType?.authorizedConsumers[alert.consumer]?.read ?? false;
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ import { AddConnectorInline } from './connector_add_inline';
import { actionTypeCompare } from '../../lib/action_type_compare';
import { checkActionFormActionTypeEnabled } from '../../lib/check_action_type_enabled';
import { VIEW_LICENSE_OPTIONS_LINK, DEFAULT_HIDDEN_ACTION_TYPES } from '../../../common/constants';
import { ActionGroup } from '../../../../../alerts/common';
import { ActionGroup, AlertActionParam } from '../../../../../alerts/common';

export interface ActionAccordionFormProps {
actions: AlertAction[];
Expand All @@ -45,7 +45,7 @@ export interface ActionAccordionFormProps {
setActionIdByIndex: (id: string, index: number) => void;
setActionGroupIdByIndex?: (group: string, index: number) => void;
setAlertProperty: (actions: AlertAction[]) => void;
setActionParamsProperty: (key: string, value: any, index: number) => void;
setActionParamsProperty: (key: string, value: AlertActionParam, index: number) => void;
http: HttpSetup;
actionTypeRegistry: ActionTypeRegistryContract;
toastNotifications: ToastsSetup;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import {
EuiLoadingSpinner,
EuiBadge,
} from '@elastic/eui';
import { ResolvedActionGroup } from '../../../../../alerts/common';
import { AlertActionParam, ResolvedActionGroup } from '../../../../../alerts/common';
import {
IErrorObject,
AlertAction,
Expand All @@ -50,7 +50,7 @@ export type ActionTypeFormProps = {
onAddConnector: () => void;
onConnectorSelected: (id: string) => void;
onDeleteAction: () => void;
setActionParamsProperty: (key: string, value: any, index: number) => void;
setActionParamsProperty: (key: string, value: AlertActionParam, index: number) => void;
actionTypesIndex: ActionTypeIndex;
connectors: ActionConnector[];
} & Pick<
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,13 @@
* you may not use this file except in compliance with the Elastic License.
*/
import React, { useCallback, useReducer, useState, useEffect } from 'react';
import { isObject } from 'lodash';
import { FormattedMessage } from '@kbn/i18n/react';
import { EuiTitle, EuiFlyoutHeader, EuiFlyout, EuiFlyoutBody, EuiPortal } from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import { useAlertsContext } from '../../context/alerts_context';
import { Alert, AlertAction, IErrorObject } from '../../../types';
import { AlertForm, validateBaseProperties } from './alert_form';
import { alertReducer } from './alert_reducer';
import { AlertForm, isValidAlert, validateBaseProperties } from './alert_form';
import { alertReducer, InitialAlert, InitialAlertReducer } from './alert_reducer';
import { createAlert } from '../../lib/alert_api';
import { HealthCheck } from '../../components/health_check';
import { ConfirmAlertSave } from './confirm_alert_save';
Expand All @@ -36,7 +35,7 @@ export const AlertAdd = ({
alertTypeId,
initialValues,
}: AlertAddProps) => {
const initialAlert = ({
const initialAlert: InitialAlert = {
params: {},
consumer,
alertTypeId,
Expand All @@ -46,16 +45,19 @@ export const AlertAdd = ({
actions: [],
tags: [],
...(initialValues ? initialValues : {}),
} as unknown) as Alert;
};

const [{ alert }, dispatch] = useReducer(alertReducer, { alert: initialAlert });
const [{ alert }, dispatch] = useReducer(alertReducer as InitialAlertReducer, {
alert: initialAlert,
});
const [isSaving, setIsSaving] = useState<boolean>(false);
const [isConfirmAlertSaveModalOpen, setIsConfirmAlertSaveModalOpen] = useState<boolean>(false);

const setAlert = (value: any) => {
const setAlert = (value: InitialAlert) => {
dispatch({ command: { type: 'setAlert' }, payload: { key: 'alert', value } });
};
const setAlertProperty = (key: string, value: any) => {

const setAlertProperty = <Key extends keyof Alert>(key: Key, value: Alert[Key] | null) => {
dispatch({ command: { type: 'setProperty' }, payload: { key, value } });
};

Expand All @@ -72,7 +74,7 @@ export const AlertAdd = ({
const canShowActions = hasShowActionsCapability(capabilities);

useEffect(() => {
setAlertProperty('alertTypeId', alertTypeId);
setAlertProperty('alertTypeId', alertTypeId ?? null);
}, [alertTypeId]);

const closeFlyout = useCallback(() => {
Expand Down Expand Up @@ -100,7 +102,7 @@ export const AlertAdd = ({
...(alertType ? alertType.validate(alert.params).errors : []),
...validateBaseProperties(alert).errors,
} as IErrorObject;
const hasErrors = parseErrors(errors);
const hasErrors = !isValidAlert(alert, errors);

const actionsErrors: Array<{
errors: IErrorObject;
Expand All @@ -120,16 +122,18 @@ export const AlertAdd = ({

async function onSaveAlert(): Promise<Alert | undefined> {
try {
const newAlert = await createAlert({ http, alert });
toastNotifications.addSuccess(
i18n.translate('xpack.triggersActionsUI.sections.alertAdd.saveSuccessNotificationText', {
defaultMessage: 'Created alert "{alertName}"',
values: {
alertName: newAlert.name,
},
})
);
return newAlert;
if (isValidAlert(alert, errors)) {
const newAlert = await createAlert({ http, alert });
toastNotifications.addSuccess(
i18n.translate('xpack.triggersActionsUI.sections.alertAdd.saveSuccessNotificationText', {
defaultMessage: 'Created alert "{alertName}"',
values: {
alertName: newAlert.name,
},
})
);
return newAlert;
}
} catch (errorRes) {
toastNotifications.addDanger(
errorRes.body?.message ??
Expand Down Expand Up @@ -206,11 +210,5 @@ export const AlertAdd = ({
);
};

const parseErrors: (errors: IErrorObject) => boolean = (errors) =>
!!Object.values(errors).find((errorList) => {
if (isObject(errorList)) return parseErrors(errorList as IErrorObject);
return errorList.length >= 1;
});

// eslint-disable-next-line import/no-default-export
export { AlertAdd as default };
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { i18n } from '@kbn/i18n';
import { useAlertsContext } from '../../context/alerts_context';
import { Alert, AlertAction, IErrorObject } from '../../../types';
import { AlertForm, validateBaseProperties } from './alert_form';
import { alertReducer } from './alert_reducer';
import { alertReducer, ConcreteAlertReducer } from './alert_reducer';
import { updateAlert } from '../../lib/alert_api';
import { HealthCheck } from '../../components/health_check';
import { HealthContextProvider } from '../../context/health_context';
Expand All @@ -34,14 +34,16 @@ interface AlertEditProps {
}

export const AlertEdit = ({ initialAlert, onClose }: AlertEditProps) => {
const [{ alert }, dispatch] = useReducer(alertReducer, { alert: initialAlert });
const [{ alert }, dispatch] = useReducer(alertReducer as ConcreteAlertReducer, {
alert: initialAlert,
});
const [isSaving, setIsSaving] = useState<boolean>(false);
const [hasActionsDisabled, setHasActionsDisabled] = useState<boolean>(false);
const [hasActionsWithBrokenConnector, setHasActionsWithBrokenConnector] = useState<boolean>(
false
);
const setAlert = (key: string, value: any) => {
dispatch({ command: { type: 'setAlert' }, payload: { key, value } });
const setAlert = (value: Alert) => {
dispatch({ command: { type: 'setAlert' }, payload: { key: 'alert', value } });
};

const {
Expand All @@ -55,7 +57,7 @@ export const AlertEdit = ({ initialAlert, onClose }: AlertEditProps) => {

const closeFlyout = useCallback(() => {
onClose();
setAlert('alert', initialAlert);
setAlert(initialAlert);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [onClose]);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,33 +33,34 @@ import {
} from '@elastic/eui';
import { some, filter, map, fold } from 'fp-ts/lib/Option';
import { pipe } from 'fp-ts/lib/pipeable';
import { capitalize } from 'lodash';
import { capitalize, isObject } from 'lodash';
import { KibanaFeature } from '../../../../../features/public';
import {
getDurationNumberInItsUnit,
getDurationUnitValue,
} from '../../../../../alerts/common/parse_duration';
import { loadAlertTypes } from '../../lib/alert_api';
import { AlertReducerAction } from './alert_reducer';
import { AlertReducerAction, InitialAlert } from './alert_reducer';
import {
AlertTypeModel,
Alert,
IErrorObject,
AlertAction,
AlertTypeIndex,
AlertType,
ValidationResult,
} from '../../../types';
import { getTimeOptions } from '../../../common/lib/get_time_options';
import { useAlertsContext } from '../../context/alerts_context';
import { ActionForm } from '../action_connector_form';
import { ALERTS_FEATURE_ID } from '../../../../../alerts/common';
import { AlertActionParam, ALERTS_FEATURE_ID } from '../../../../../alerts/common';
import { hasAllPrivilege, hasShowActionsCapability } from '../../lib/capabilities';
import { SolutionFilter } from './solution_filter';
import './alert_form.scss';

const ENTER_KEY = 13;

export function validateBaseProperties(alertObject: Alert) {
export function validateBaseProperties(alertObject: InitialAlert): ValidationResult {
const validationResult = { errors: {} };
const errors = {
name: new Array<string>(),
Expand Down Expand Up @@ -92,12 +93,25 @@ export function validateBaseProperties(alertObject: Alert) {
return validationResult;
}

const hasErrors: (errors: IErrorObject) => boolean = (errors) =>
!!Object.values(errors).find((errorList) => {
if (isObject(errorList)) return hasErrors(errorList as IErrorObject);
return errorList.length >= 1;
});

export function isValidAlert(
alertObject: InitialAlert | Alert,
validationResult: IErrorObject
): alertObject is Alert {
return !hasErrors(validationResult);
}

function getProducerFeatureName(producer: string, kibanaFeatures: KibanaFeature[]) {
return kibanaFeatures.find((featureItem) => featureItem.id === producer)?.name;
}

interface AlertFormProps {
alert: Alert;
alert: InitialAlert;
dispatch: React.Dispatch<AlertReducerAction>;
errors: IErrorObject;
canChangeTrigger?: boolean; // to hide Change trigger button
Expand Down Expand Up @@ -203,10 +217,13 @@ export const AlertForm = ({

useEffect(() => {
setAlertTypeModel(alert.alertTypeId ? alertTypeRegistry.get(alert.alertTypeId) : null);
}, [alert, alertTypeRegistry]);
if (alert.alertTypeId && alertTypesIndex && alertTypesIndex.has(alert.alertTypeId)) {
setDefaultActionGroupId(alertTypesIndex.get(alert.alertTypeId)!.defaultActionGroupId);
}
}, [alert, alert.alertTypeId, alertTypesIndex, alertTypeRegistry]);

const setAlertProperty = useCallback(
(key: string, value: any) => {
<Key extends keyof Alert>(key: Key, value: Alert[Key] | null) => {
dispatch({ command: { type: 'setProperty' }, payload: { key, value } });
},
[dispatch]
Expand All @@ -225,12 +242,16 @@ export const AlertForm = ({
dispatch({ command: { type: 'setScheduleProperty' }, payload: { key, value } });
};

const setActionProperty = (key: string, value: any, index: number) => {
const setActionProperty = <Key extends keyof AlertAction>(
key: Key,
value: AlertAction[Key] | null,
index: number
) => {
dispatch({ command: { type: 'setAlertActionProperty' }, payload: { key, value, index } });
};

const setActionParamsProperty = useCallback(
(key: string, value: any, index: number) => {
(key: string, value: AlertActionParam, index: number) => {
dispatch({ command: { type: 'setAlertActionParams' }, payload: { key, value, index } });
},
[dispatch]
Expand Down Expand Up @@ -438,6 +459,7 @@ export const AlertForm = ({
<EuiHorizontalRule />
{AlertParamsExpressionComponent &&
defaultActionGroupId &&
alert.alertTypeId &&
alertTypesIndex?.has(alert.alertTypeId) ? (
<Suspense fallback={<CenterJustifiedSpinner />}>
<AlertParamsExpressionComponent
Expand All @@ -456,6 +478,7 @@ export const AlertForm = ({
{canShowActions &&
defaultActionGroupId &&
alertTypeModel &&
alert.alertTypeId &&
alertTypesIndex?.has(alert.alertTypeId) ? (
<ActionForm
actions={alert.actions}
Expand Down
Loading

0 comments on commit 158e1e3

Please sign in to comment.