From 7fdfee628baeab8d63d9decbbfa25db9a17bd314 Mon Sep 17 00:00:00 2001 From: Yuliia Naumenko Date: Wed, 18 Mar 2020 14:53:39 -0700 Subject: [PATCH 1/4] Implemented ability to clear and properly validate alert interval --- .../sections/alert_form/alert_form.test.tsx | 4 +-- .../sections/alert_form/alert_form.tsx | 30 +++++++++++++------ .../apps/triggers_actions_ui/alerts.ts | 7 ++++- 3 files changed, 29 insertions(+), 12 deletions(-) diff --git a/x-pack/plugins/triggers_actions_ui/public/application/sections/alert_form/alert_form.test.tsx b/x-pack/plugins/triggers_actions_ui/public/application/sections/alert_form/alert_form.test.tsx index b31be7ecb9a79..b87aaacb3ec0e 100644 --- a/x-pack/plugins/triggers_actions_ui/public/application/sections/alert_form/alert_form.test.tsx +++ b/x-pack/plugins/triggers_actions_ui/public/application/sections/alert_form/alert_form.test.tsx @@ -86,7 +86,7 @@ describe('alert_form', () => { uiSettings: deps!.uiSettings, }} > - {}} errors={{ name: [] }} /> + {}} errors={{ name: [], interval: [] }} /> ); @@ -165,7 +165,7 @@ describe('alert_form', () => { uiSettings: deps!.uiSettings, }} > - {}} errors={{ name: [] }} /> + {}} errors={{ name: [], interval: [] }} /> ); diff --git a/x-pack/plugins/triggers_actions_ui/public/application/sections/alert_form/alert_form.tsx b/x-pack/plugins/triggers_actions_ui/public/application/sections/alert_form/alert_form.tsx index 1fa620c5394a1..598fa4b81bbf1 100644 --- a/x-pack/plugins/triggers_actions_ui/public/application/sections/alert_form/alert_form.tsx +++ b/x-pack/plugins/triggers_actions_ui/public/application/sections/alert_form/alert_form.tsx @@ -48,7 +48,7 @@ export function validateBaseProperties(alertObject: Alert) { }) ); } - if (!alertObject.schedule.interval) { + if (alertObject.schedule.interval.length < 2) { errors.interval.push( i18n.translate('xpack.triggersActionsUI.sections.alertForm.error.requiredIntervalText', { defaultMessage: 'Check interval is required.', @@ -81,11 +81,15 @@ export const AlertForm = ({ alert, canChangeTrigger = true, dispatch, errors }: ); const [alertTypesIndex, setAlertTypesIndex] = useState(undefined); - const [alertInterval, setAlertInterval] = useState( - alert.schedule.interval ? parseInt(alert.schedule.interval.replace(/^[A-Za-z]+$/, ''), 0) : 1 + const [alertInterval, setAlertInterval] = useState( + alert.schedule.interval + ? parseInt(alert.schedule.interval.replace(/^[A-Za-z]+$/, ''), 0) + : undefined ); const [alertIntervalUnit, setAlertIntervalUnit] = useState( - alert.schedule.interval ? alert.schedule.interval.replace(alertInterval.toString(), '') : 'm' + alert.schedule.interval + ? alert.schedule.interval.replace((alertInterval ?? '').toString(), '') + : 'm' ); const [alertThrottle, setAlertThrottle] = useState( alert.throttle ? parseInt(alert.throttle.replace(/^[A-Za-z]+$/, ''), 0) : null @@ -344,19 +348,27 @@ export const AlertForm = ({ alert, canChangeTrigger = true, dispatch, errors }: - + 0} + error={errors.interval} + > 0} compressed - value={alertInterval} + value={alertInterval || ''} name="interval" data-test-subj="intervalInput" onChange={e => { - const interval = e.target.value !== '' ? parseInt(e.target.value, 10) : null; - setAlertInterval(interval ?? 1); + const interval = + e.target.value !== '' ? parseInt(e.target.value, 10) : undefined; + setAlertInterval(interval); setScheduleProperty('interval', `${e.target.value}${alertIntervalUnit}`); }} /> @@ -366,7 +378,7 @@ export const AlertForm = ({ alert, canChangeTrigger = true, dispatch, errors }: fullWidth compressed value={alertIntervalUnit} - options={getTimeOptions(alertInterval)} + options={getTimeOptions(alertInterval ?? 1)} onChange={e => { setAlertIntervalUnit(e.target.value); setScheduleProperty('interval', `${alertInterval}${e.target.value}`); diff --git a/x-pack/test/functional_with_es_ssl/apps/triggers_actions_ui/alerts.ts b/x-pack/test/functional_with_es_ssl/apps/triggers_actions_ui/alerts.ts index 791712fa24489..d8f6a33b5d66c 100644 --- a/x-pack/test/functional_with_es_ssl/apps/triggers_actions_ui/alerts.ts +++ b/x-pack/test/functional_with_es_ssl/apps/triggers_actions_ui/alerts.ts @@ -62,7 +62,12 @@ export default ({ getPageObjects, getService }: FtrProviderContext) => { await fieldOptions[1].click(); // need this two out of popup clicks to close them await nameInput.click(); - await testSubjects.click('intervalInput'); + const intervalInput = await testSubjects.find('intervalInput'); + await intervalInput.click(); + await intervalInput.clearValue(); + const validationError = await find.byCssSelector(`.euiFormErrorText`); + expect(validationError.isDisplayed).to.be(true); + await intervalInput.type(1); await testSubjects.click('.slack-ActionTypeSelectOption'); await testSubjects.click('createActionConnectorButton'); From 198ca721f9f05321a3c0394bea7c2a844cc82ede Mon Sep 17 00:00:00 2001 From: Yuliia Naumenko Date: Thu, 19 Mar 2020 10:14:23 -0700 Subject: [PATCH 2/4] Fixed due to comments --- .../alerting/common/parse_duration.test.ts | 32 ++++++++++++++++++- .../plugins/alerting/common/parse_duration.ts | 9 ++++++ .../sections/alert_form/alert_form.tsx | 16 +++++----- .../apps/triggers_actions_ui/alerts.ts | 6 ---- 4 files changed, 48 insertions(+), 15 deletions(-) diff --git a/x-pack/plugins/alerting/common/parse_duration.test.ts b/x-pack/plugins/alerting/common/parse_duration.test.ts index ccdddd8ecf5f4..41d3ab5868c9e 100644 --- a/x-pack/plugins/alerting/common/parse_duration.test.ts +++ b/x-pack/plugins/alerting/common/parse_duration.test.ts @@ -4,7 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ -import { parseDuration } from './parse_duration'; +import { parseDuration, getDurationNumberInItsUnit, getDurationUnitValue } from './parse_duration'; test('parses seconds', () => { const result = parseDuration('10s'); @@ -52,3 +52,33 @@ test('throws error when 0 based', () => { `"Invalid duration \\"0d\\". Durations must be of the form {number}x. Example: 5s, 5m, 5h or 5d\\""` ); }); + +test('getDurationNumberInItsUnit days', () => { + const result = getDurationNumberInItsUnit('10d'); + expect(result).toEqual(10); +}); + +test('getDurationNumberInItsUnit minutes', () => { + const result = getDurationNumberInItsUnit('1m'); + expect(result).toEqual(1); +}); + +test('getDurationNumberInItsUnit seconds', () => { + const result = getDurationNumberInItsUnit('123s'); + expect(result).toEqual(123); +}); + +test('getDurationUnitValue minutes', () => { + const result = getDurationUnitValue('1m'); + expect(result).toEqual('m'); +}); + +test('getDurationUnitValue days', () => { + const result = getDurationUnitValue('23d'); + expect(result).toEqual('d'); +}); + +test('getDurationUnitValue hours', () => { + const result = getDurationUnitValue('100h'); + expect(result).toEqual('h'); +}); diff --git a/x-pack/plugins/alerting/common/parse_duration.ts b/x-pack/plugins/alerting/common/parse_duration.ts index 4e35a4c4cb0cf..c271035f012e5 100644 --- a/x-pack/plugins/alerting/common/parse_duration.ts +++ b/x-pack/plugins/alerting/common/parse_duration.ts @@ -25,6 +25,15 @@ export function parseDuration(duration: string): number { ); } +export function getDurationNumberInItsUnit(duration: string): number { + return parseInt(duration.replace(/[^0-9.]/g, ''), 0); +} + +export function getDurationUnitValue(duration: string): string { + const durationNumber = getDurationNumberInItsUnit(duration); + return duration.replace(durationNumber.toString(), ''); +} + export function validateDurationSchema(duration: string) { if (duration.match(SECONDS_REGEX)) { return; diff --git a/x-pack/plugins/triggers_actions_ui/public/application/sections/alert_form/alert_form.tsx b/x-pack/plugins/triggers_actions_ui/public/application/sections/alert_form/alert_form.tsx index 598fa4b81bbf1..8382cbe825da3 100644 --- a/x-pack/plugins/triggers_actions_ui/public/application/sections/alert_form/alert_form.tsx +++ b/x-pack/plugins/triggers_actions_ui/public/application/sections/alert_form/alert_form.tsx @@ -24,6 +24,10 @@ import { EuiButtonIcon, EuiHorizontalRule, } from '@elastic/eui'; +import { + getDurationNumberInItsUnit, + getDurationUnitValue, +} from '../../../../../alerting/common/parse_duration'; import { loadAlertTypes } from '../../lib/alert_api'; import { actionVariablesFromAlertType } from '../../lib/action_variables'; import { AlertReducerAction } from './alert_reducer'; @@ -82,20 +86,16 @@ export const AlertForm = ({ alert, canChangeTrigger = true, dispatch, errors }: const [alertTypesIndex, setAlertTypesIndex] = useState(undefined); const [alertInterval, setAlertInterval] = useState( - alert.schedule.interval - ? parseInt(alert.schedule.interval.replace(/^[A-Za-z]+$/, ''), 0) - : undefined + alert.schedule.interval ? getDurationNumberInItsUnit(alert.schedule.interval) : undefined ); const [alertIntervalUnit, setAlertIntervalUnit] = useState( - alert.schedule.interval - ? alert.schedule.interval.replace((alertInterval ?? '').toString(), '') - : 'm' + alert.schedule.interval ? getDurationUnitValue(alert.schedule.interval) : 'm' ); const [alertThrottle, setAlertThrottle] = useState( - alert.throttle ? parseInt(alert.throttle.replace(/^[A-Za-z]+$/, ''), 0) : null + alert.throttle ? getDurationNumberInItsUnit(alert.throttle) : null ); const [alertThrottleUnit, setAlertThrottleUnit] = useState( - alert.throttle ? alert.throttle.replace((alertThrottle ?? '').toString(), '') : 'm' + alert.throttle ? getDurationUnitValue(alert.throttle) : 'm' ); const [defaultActionGroupId, setDefaultActionGroupId] = useState(undefined); diff --git a/x-pack/test/functional_with_es_ssl/apps/triggers_actions_ui/alerts.ts b/x-pack/test/functional_with_es_ssl/apps/triggers_actions_ui/alerts.ts index d8f6a33b5d66c..05d4eafe72490 100644 --- a/x-pack/test/functional_with_es_ssl/apps/triggers_actions_ui/alerts.ts +++ b/x-pack/test/functional_with_es_ssl/apps/triggers_actions_ui/alerts.ts @@ -62,12 +62,6 @@ export default ({ getPageObjects, getService }: FtrProviderContext) => { await fieldOptions[1].click(); // need this two out of popup clicks to close them await nameInput.click(); - const intervalInput = await testSubjects.find('intervalInput'); - await intervalInput.click(); - await intervalInput.clearValue(); - const validationError = await find.byCssSelector(`.euiFormErrorText`); - expect(validationError.isDisplayed).to.be(true); - await intervalInput.type(1); await testSubjects.click('.slack-ActionTypeSelectOption'); await testSubjects.click('createActionConnectorButton'); From 4206b7df27316590d1ea3642197f4817fb9a32a8 Mon Sep 17 00:00:00 2001 From: Yuliia Naumenko Date: Thu, 19 Mar 2020 18:47:12 -0700 Subject: [PATCH 3/4] Fixed additional request for the last field --- .../builtin_alert_types/threshold/expression.tsx | 4 ++-- .../common/expression_items/for_the_last.tsx | 14 +++++++------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/x-pack/plugins/triggers_actions_ui/public/application/components/builtin_alert_types/threshold/expression.tsx b/x-pack/plugins/triggers_actions_ui/public/application/components/builtin_alert_types/threshold/expression.tsx index 5c7f48de81f75..f866685e8da74 100644 --- a/x-pack/plugins/triggers_actions_ui/public/application/components/builtin_alert_types/threshold/expression.tsx +++ b/x-pack/plugins/triggers_actions_ui/public/application/components/builtin_alert_types/threshold/expression.tsx @@ -399,8 +399,8 @@ export const IndexThresholdAlertTypeExpression: React.FunctionComponent setAlertParams('timeWindowSize', selectedWindowSize) diff --git a/x-pack/plugins/triggers_actions_ui/public/common/expression_items/for_the_last.tsx b/x-pack/plugins/triggers_actions_ui/public/common/expression_items/for_the_last.tsx index 844551de3171d..673391dd9cbad 100644 --- a/x-pack/plugins/triggers_actions_ui/public/common/expression_items/for_the_last.tsx +++ b/x-pack/plugins/triggers_actions_ui/public/common/expression_items/for_the_last.tsx @@ -25,7 +25,7 @@ interface ForLastExpressionProps { timeWindowSize?: number; timeWindowUnit?: string; errors: { [key: string]: string[] }; - onChangeWindowSize: (selectedWindowSize: number | '') => void; + onChangeWindowSize: (selectedWindowSize: number | undefined) => void; onChangeWindowUnit: (selectedWindowUnit: string) => void; popupPosition?: | 'upCenter' @@ -43,7 +43,7 @@ interface ForLastExpressionProps { } export const ForLastExpression = ({ - timeWindowSize = 1, + timeWindowSize, timeWindowUnit = 's', errors, onChangeWindowSize, @@ -64,7 +64,7 @@ export const ForLastExpression = ({ )} value={`${timeWindowSize} ${getTimeUnitLabel( timeWindowUnit as TIME_UNITS, - timeWindowSize.toString() + (timeWindowSize ?? '').toString() )}`} isActive={alertDurationPopoverOpen} onClick={() => { @@ -97,11 +97,11 @@ export const ForLastExpression = ({ 0 && timeWindowSize !== undefined} - min={1} - value={timeWindowSize} + min={0} + value={timeWindowSize || ''} onChange={e => { const { value } = e.target; - const timeWindowSizeVal = value !== '' ? parseInt(value, 10) : value; + const timeWindowSizeVal = value !== '' ? parseInt(value, 10) : undefined; onChangeWindowSize(timeWindowSizeVal); }} /> @@ -114,7 +114,7 @@ export const ForLastExpression = ({ onChange={e => { onChangeWindowUnit(e.target.value); }} - options={getTimeOptions(timeWindowSize)} + options={getTimeOptions(timeWindowSize ?? 1)} /> From 92609c31ce86e45542cee35b88c4dfba2d0afdba Mon Sep 17 00:00:00 2001 From: Yuliia Naumenko Date: Thu, 19 Mar 2020 20:19:17 -0700 Subject: [PATCH 4/4] Fixed failing test --- .../public/common/expression_items/for_the_last.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/triggers_actions_ui/public/common/expression_items/for_the_last.test.tsx b/x-pack/plugins/triggers_actions_ui/public/common/expression_items/for_the_last.test.tsx index 6ae3056001c8f..e66bb1e7b4b9a 100644 --- a/x-pack/plugins/triggers_actions_ui/public/common/expression_items/for_the_last.test.tsx +++ b/x-pack/plugins/triggers_actions_ui/public/common/expression_items/for_the_last.test.tsx @@ -36,7 +36,7 @@ describe('for the last expression', () => { /> ); wrapper.simulate('click'); - expect(wrapper.find('[value=1]').length > 0).toBeTruthy(); + expect(wrapper.find('[value=""]').length > 0).toBeTruthy(); expect(wrapper.find('[value="s"]').length > 0).toBeTruthy(); expect( wrapper.contains(