From 621420d6c895fafab4792db7319c848bf41e0e24 Mon Sep 17 00:00:00 2001 From: Ying Mao Date: Wed, 9 Dec 2020 14:09:39 -0500 Subject: [PATCH 1/2] Remove add alerts flyout after onClose --- .../public/components/create_alert.tsx | 13 ++++++++----- .../alerting/AlertingFlyout/index.tsx | 14 +++++++++----- .../inventory/components/alert_flyout.tsx | 9 ++++----- .../log_threshold/components/alert_flyout.tsx | 13 ++++++------- .../components/alert_flyout.tsx | 12 ++++++------ .../sections/alert_form/alert_add.tsx | 14 ++++---------- .../alerts_list/components/alerts_list.tsx | 19 +++++++++++-------- .../alerts/uptime_alerts_flyout_wrapper.tsx | 13 +++++++------ 8 files changed, 55 insertions(+), 52 deletions(-) diff --git a/x-pack/examples/alerting_example/public/components/create_alert.tsx b/x-pack/examples/alerting_example/public/components/create_alert.tsx index 8b62dfbb0997b..ade2cdbbbd4a9 100644 --- a/x-pack/examples/alerting_example/public/components/create_alert.tsx +++ b/x-pack/examples/alerting_example/public/components/create_alert.tsx @@ -4,7 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ -import React, { useMemo, useState } from 'react'; +import React, { useMemo, useState, useCallback } from 'react'; import { EuiIcon, EuiFlexItem, EuiCard, EuiFlexGroup } from '@elastic/eui'; @@ -14,15 +14,18 @@ import { ALERTING_EXAMPLE_APP_ID } from '../../common/constants'; export const CreateAlert = ({ triggersActionsUi }: AlertingExampleComponentParams) => { const [alertFlyoutVisible, setAlertFlyoutVisibility] = useState(false); + const onCloseAlertFlyout = useCallback(() => setAlertFlyoutVisibility(false), [ + setAlertFlyoutVisibility, + ]); + const AddAlertFlyout = useMemo( () => triggersActionsUi.getAddAlertFlyout({ consumer: ALERTING_EXAMPLE_APP_ID, - addFlyoutVisible: alertFlyoutVisible, - setAddFlyoutVisibility: setAlertFlyoutVisibility, + onClose: onCloseAlertFlyout, }), // eslint-disable-next-line react-hooks/exhaustive-deps - [alertFlyoutVisible] + [onCloseAlertFlyout] ); return ( @@ -35,7 +38,7 @@ export const CreateAlert = ({ triggersActionsUi }: AlertingExampleComponentParam onClick={() => setAlertFlyoutVisibility(true)} /> - {AddAlertFlyout} + {alertFlyoutVisible && AddAlertFlyout} ); }; diff --git a/x-pack/plugins/apm/public/components/alerting/AlertingFlyout/index.tsx b/x-pack/plugins/apm/public/components/alerting/AlertingFlyout/index.tsx index aa1d21dd1d580..88a897d7baf50 100644 --- a/x-pack/plugins/apm/public/components/alerting/AlertingFlyout/index.tsx +++ b/x-pack/plugins/apm/public/components/alerting/AlertingFlyout/index.tsx @@ -3,7 +3,7 @@ * or more contributor license agreements. Licensed under the Elastic License; * you may not use this file except in compliance with the Elastic License. */ -import React, { useMemo } from 'react'; +import React, { useCallback, useMemo } from 'react'; import { useKibana } from '../../../../../../../src/plugins/kibana_react/public'; import { AlertType } from '../../../../common/alert_types'; import { TriggersAndActionsUIPublicPluginStart } from '../../../../../triggers_actions_ui/public'; @@ -23,17 +23,21 @@ export function AlertingFlyout(props: Props) { const { services: { triggersActionsUi }, } = useKibana(); + + const onCloseAddFlyout = useCallback(() => setAddFlyoutVisibility(false), [ + setAddFlyoutVisibility, + ]); + const addAlertFlyout = useMemo( () => alertType && triggersActionsUi.getAddAlertFlyout({ consumer: 'apm', - addFlyoutVisible, - setAddFlyoutVisibility, + onClose: onCloseAddFlyout, alertTypeId: alertType, canChangeTrigger: false, }), - [addFlyoutVisible, alertType, setAddFlyoutVisibility, triggersActionsUi] + [alertType, onCloseAddFlyout, triggersActionsUi] ); - return <>{addAlertFlyout}; + return <>{addFlyoutVisible && addAlertFlyout}; } diff --git a/x-pack/plugins/infra/public/alerting/inventory/components/alert_flyout.tsx b/x-pack/plugins/infra/public/alerting/inventory/components/alert_flyout.tsx index 432d2073d93b6..1c6852c5eb8d9 100644 --- a/x-pack/plugins/infra/public/alerting/inventory/components/alert_flyout.tsx +++ b/x-pack/plugins/infra/public/alerting/inventory/components/alert_flyout.tsx @@ -4,7 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ -import React, { useContext, useMemo } from 'react'; +import React, { useCallback, useContext, useMemo } from 'react'; import { TriggerActionsContext } from '../../../utils/triggers_actions_context'; // eslint-disable-next-line @kbn/eslint/no-restricted-paths @@ -26,14 +26,13 @@ export const AlertFlyout = ({ options, nodeType, filter, visible, setVisible }: const { inventoryPrefill } = useAlertPrefillContext(); const { customMetrics } = inventoryPrefill; - + const onCloseFlyout = useCallback(() => setVisible(false), [setVisible]); const AddAlertFlyout = useMemo( () => triggersActionsUI && triggersActionsUI.getAddAlertFlyout({ consumer: 'infrastructure', - addFlyoutVisible: visible!, - setAddFlyoutVisibility: setVisible, + onClose: onCloseFlyout, canChangeTrigger: false, alertTypeId: METRIC_INVENTORY_THRESHOLD_ALERT_TYPE_ID, metadata: { @@ -47,5 +46,5 @@ export const AlertFlyout = ({ options, nodeType, filter, visible, setVisible }: [triggersActionsUI, visible] ); - return <>{AddAlertFlyout}; + return <>{visible && AddAlertFlyout}; }; diff --git a/x-pack/plugins/infra/public/alerting/log_threshold/components/alert_flyout.tsx b/x-pack/plugins/infra/public/alerting/log_threshold/components/alert_flyout.tsx index 206621c4d4dc8..fe8493ccd0fbf 100644 --- a/x-pack/plugins/infra/public/alerting/log_threshold/components/alert_flyout.tsx +++ b/x-pack/plugins/infra/public/alerting/log_threshold/components/alert_flyout.tsx @@ -4,7 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ -import React, { useContext, useMemo } from 'react'; +import React, { useCallback, useContext, useMemo } from 'react'; import { TriggerActionsContext } from '../../../utils/triggers_actions_context'; import { LOG_DOCUMENT_COUNT_ALERT_TYPE_ID } from '../../../../common/alerting/logs/log_threshold/types'; @@ -14,24 +14,23 @@ interface Props { } export const AlertFlyout = (props: Props) => { + const { visible, setVisible } = props; const { triggersActionsUI } = useContext(TriggerActionsContext); - + const onCloseFlyout = useCallback(() => setVisible(false), [setVisible]); const AddAlertFlyout = useMemo( () => triggersActionsUI && triggersActionsUI.getAddAlertFlyout({ consumer: 'logs', - addFlyoutVisible: props.visible!, - setAddFlyoutVisibility: props.setVisible, + onClose: onCloseFlyout, canChangeTrigger: false, alertTypeId: LOG_DOCUMENT_COUNT_ALERT_TYPE_ID, metadata: { isInternal: true, }, }), - // eslint-disable-next-line react-hooks/exhaustive-deps - [triggersActionsUI, props.visible] + [triggersActionsUI, onCloseFlyout] ); - return <>{AddAlertFlyout}; + return <>{visible && AddAlertFlyout}; }; diff --git a/x-pack/plugins/infra/public/alerting/metric_threshold/components/alert_flyout.tsx b/x-pack/plugins/infra/public/alerting/metric_threshold/components/alert_flyout.tsx index 779478a313b71..72782f555d9ca 100644 --- a/x-pack/plugins/infra/public/alerting/metric_threshold/components/alert_flyout.tsx +++ b/x-pack/plugins/infra/public/alerting/metric_threshold/components/alert_flyout.tsx @@ -4,7 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ -import React, { useContext, useMemo } from 'react'; +import React, { useCallback, useContext, useMemo } from 'react'; import { TriggerActionsContext } from '../../../utils/triggers_actions_context'; // eslint-disable-next-line @kbn/eslint/no-restricted-paths import { METRIC_THRESHOLD_ALERT_TYPE_ID } from '../../../../server/lib/alerting/metric_threshold/types'; @@ -19,15 +19,15 @@ interface Props { } export const AlertFlyout = (props: Props) => { + const { visible, setVisible } = props; const { triggersActionsUI } = useContext(TriggerActionsContext); - + const onCloseFlyout = useCallback(() => setVisible(false), [setVisible]); const AddAlertFlyout = useMemo( () => triggersActionsUI && triggersActionsUI.getAddAlertFlyout({ consumer: 'infrastructure', - addFlyoutVisible: props.visible!, - setAddFlyoutVisibility: props.setVisible, + onClose: onCloseFlyout, canChangeTrigger: false, alertTypeId: METRIC_THRESHOLD_ALERT_TYPE_ID, metadata: { @@ -36,8 +36,8 @@ export const AlertFlyout = (props: Props) => { }, }), // eslint-disable-next-line react-hooks/exhaustive-deps - [triggersActionsUI, props.visible] + [triggersActionsUI, onCloseFlyout] ); - return <>{AddAlertFlyout}; + return <>{visible && AddAlertFlyout}; }; diff --git a/x-pack/plugins/triggers_actions_ui/public/application/sections/alert_form/alert_add.tsx b/x-pack/plugins/triggers_actions_ui/public/application/sections/alert_form/alert_add.tsx index 03c8b539227a2..408c3c10b2fce 100644 --- a/x-pack/plugins/triggers_actions_ui/public/application/sections/alert_form/alert_add.tsx +++ b/x-pack/plugins/triggers_actions_ui/public/application/sections/alert_form/alert_add.tsx @@ -26,10 +26,9 @@ import { useKibana } from '../../../common/lib/kibana'; export interface AlertAddProps> { consumer: string; - addFlyoutVisible: boolean; alertTypeRegistry: AlertTypeRegistryContract; actionTypeRegistry: ActionTypeRegistryContract; - setAddFlyoutVisibility: React.Dispatch>; + onClose: () => void; alertTypeId?: string; canChangeTrigger?: boolean; initialValues?: Partial; @@ -39,10 +38,9 @@ export interface AlertAddProps> { const AlertAdd = ({ consumer, - addFlyoutVisible, alertTypeRegistry, actionTypeRegistry, - setAddFlyoutVisibility, + onClose, canChangeTrigger, alertTypeId, initialValues, @@ -91,9 +89,9 @@ const AlertAdd = ({ }, [alertTypeId]); const closeFlyout = useCallback(() => { - setAddFlyoutVisibility(false); setAlert(initialAlert); - }, [initialAlert, setAddFlyoutVisibility]); + onClose(); + }, [initialAlert, onClose]); const saveAlertAndCloseFlyout = async () => { const savedAlert = await onSaveAlert(); @@ -106,10 +104,6 @@ const AlertAdd = ({ } }; - if (!addFlyoutVisible) { - return null; - } - const alertType = alert.alertTypeId ? alertTypeRegistry.get(alert.alertTypeId) : null; const errors = { ...(alertType ? alertType.validate(alert.params).errors : []), diff --git a/x-pack/plugins/triggers_actions_ui/public/application/sections/alerts_list/components/alerts_list.tsx b/x-pack/plugins/triggers_actions_ui/public/application/sections/alerts_list/components/alerts_list.tsx index aaaf843d43eab..e1fadeade102d 100644 --- a/x-pack/plugins/triggers_actions_ui/public/application/sections/alerts_list/components/alerts_list.tsx +++ b/x-pack/plugins/triggers_actions_ui/public/application/sections/alerts_list/components/alerts_list.tsx @@ -653,14 +653,17 @@ export const AlertsList: React.FunctionComponent = () => { ) : ( noPermissionPrompt )} - + {alertFlyoutVisible && ( + { + setAlertFlyoutVisibility(false); + }} + actionTypeRegistry={actionTypeRegistry} + alertTypeRegistry={alertTypeRegistry} + reloadAlerts={loadAlertsData} + /> + )} ); }; diff --git a/x-pack/plugins/uptime/public/components/overview/alerts/uptime_alerts_flyout_wrapper.tsx b/x-pack/plugins/uptime/public/components/overview/alerts/uptime_alerts_flyout_wrapper.tsx index 7995cf88df9ba..75cbd43cd0b38 100644 --- a/x-pack/plugins/uptime/public/components/overview/alerts/uptime_alerts_flyout_wrapper.tsx +++ b/x-pack/plugins/uptime/public/components/overview/alerts/uptime_alerts_flyout_wrapper.tsx @@ -4,7 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ -import React, { useMemo } from 'react'; +import React, { useCallback, useMemo } from 'react'; import { useKibana } from '../../../../../../../src/plugins/kibana_react/public'; import { TriggersAndActionsUIPublicPluginStart } from '../../../../../../plugins/triggers_actions_ui/public'; @@ -24,19 +24,20 @@ export const UptimeAlertsFlyoutWrapperComponent = ({ setAlertFlyoutVisibility, }: Props) => { const { triggersActionsUi } = useKibana().services; - + const onCloseAlertFlyout = useCallback(() => setAlertFlyoutVisibility(false), [ + setAlertFlyoutVisibility, + ]); const AddAlertFlyout = useMemo( () => triggersActionsUi.getAddAlertFlyout({ consumer: 'uptime', - addFlyoutVisible: alertFlyoutVisible, - setAddFlyoutVisibility: setAlertFlyoutVisibility, + onClose: onCloseAlertFlyout, alertTypeId, canChangeTrigger: !alertTypeId, }), // eslint-disable-next-line react-hooks/exhaustive-deps - [alertFlyoutVisible, alertTypeId] + [onCloseAlertFlyout, alertTypeId] ); - return <>{AddAlertFlyout}; + return <>{alertFlyoutVisible && AddAlertFlyout}; }; From 9e245cb10447fa75b7edd76dae06ef9bec4fc0b8 Mon Sep 17 00:00:00 2001 From: Ying Mao Date: Wed, 9 Dec 2020 15:12:41 -0500 Subject: [PATCH 2/2] Updating tests --- .../sections/alert_form/alert_add.test.tsx | 3 +-- .../alerts_list/components/alerts_list.test.tsx | 12 +++++++++++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/x-pack/plugins/triggers_actions_ui/public/application/sections/alert_form/alert_add.test.tsx b/x-pack/plugins/triggers_actions_ui/public/application/sections/alert_form/alert_add.test.tsx index 117abddb4d9d7..598260512d3bf 100644 --- a/x-pack/plugins/triggers_actions_ui/public/application/sections/alert_form/alert_add.test.tsx +++ b/x-pack/plugins/triggers_actions_ui/public/application/sections/alert_form/alert_add.test.tsx @@ -128,8 +128,7 @@ describe('alert_add', () => { wrapper = mountWithIntl( {}} + onClose={() => {}} initialValues={initialValues} reloadAlerts={() => { return new Promise(() => {}); diff --git a/x-pack/plugins/triggers_actions_ui/public/application/sections/alerts_list/components/alerts_list.test.tsx b/x-pack/plugins/triggers_actions_ui/public/application/sections/alerts_list/components/alerts_list.test.tsx index cb4d6d8097463..f35c10b228369 100644 --- a/x-pack/plugins/triggers_actions_ui/public/application/sections/alerts_list/components/alerts_list.test.tsx +++ b/x-pack/plugins/triggers_actions_ui/public/application/sections/alerts_list/components/alerts_list.test.tsx @@ -26,6 +26,7 @@ jest.mock('../../../lib/action_connector_api', () => ({ jest.mock('../../../lib/alert_api', () => ({ loadAlerts: jest.fn(), loadAlertTypes: jest.fn(), + health: jest.fn(() => ({ isSufficientlySecure: true, hasPermanentEncryptionKey: true })), })); jest.mock('react-router-dom', () => ({ useHistory: () => ({ @@ -114,7 +115,16 @@ describe('alerts_list component empty', () => { expect( wrapper.find('[data-test-subj="createFirstAlertButton"]').find('EuiButton') ).toHaveLength(1); - expect(wrapper.find('AlertAdd')).toHaveLength(1); + expect(wrapper.find('AlertAdd').exists()).toBeFalsy(); + + wrapper.find('button[data-test-subj="createFirstAlertButton"]').simulate('click'); + + // When the AlertAdd component is rendered, it waits for the healthcheck to resolve + await new Promise((resolve) => { + setTimeout(resolve, 1000); + }); + wrapper.update(); + expect(wrapper.find('AlertAdd').exists()).toEqual(true); }); });