Implemented ability to clear and properly validate alert interval (el…

…astic#60571) * Implemented ability to clear and properly validate alert interval * Fixed due to comments * Fixed additional request for the last field * Fixed failing test
YulNaumenko · Mar 20, 2020 · d2428b5 · d2428b5
1 parent 78c0c4d
commit d2428b5
Show file tree

Hide file tree

Showing 8 changed files with 75 additions and 25 deletions.
diff --git 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
@@ -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/...ers_actions_ui/public/application/components/builtin_alert_types/threshold/expression.tsx b/...ers_actions_ui/public/application/components/builtin_alert_types/threshold/expression.tsx
@@ -399,8 +399,8 @@ export const IndexThresholdAlertTypeExpression: React.FunctionComponent<IndexThr
         <EuiFlexItem grow={false}>
           <ForLastExpression
             popupPosition={'upLeft'}
-            timeWindowSize={timeWindowSize || 1}
-            timeWindowUnit={timeWindowUnit || ''}
+            timeWindowSize={timeWindowSize}
+            timeWindowUnit={timeWindowUnit}
             errors={errors}
             onChangeWindowSize={(selectedWindowSize: any) =>
               setAlertParams('timeWindowSize', selectedWindowSize)

diff --git a/...ck/plugins/triggers_actions_ui/public/application/sections/alert_form/alert_form.test.tsx b/...ck/plugins/triggers_actions_ui/public/application/sections/alert_form/alert_form.test.tsx
@@ -86,7 +86,7 @@ describe('alert_form', () => {
             uiSettings: deps!.uiSettings,
           }}
         >
-          <AlertForm alert={initialAlert} dispatch={() => {}} errors={{ name: [] }} />
+          <AlertForm alert={initialAlert} dispatch={() => {}} errors={{ name: [], interval: [] }} />
         </AlertsContextProvider>
       );
 
@@ -165,7 +165,7 @@ describe('alert_form', () => {
             uiSettings: deps!.uiSettings,
           }}
         >
-          <AlertForm alert={initialAlert} dispatch={() => {}} errors={{ name: [] }} />
+          <AlertForm alert={initialAlert} dispatch={() => {}} errors={{ name: [], interval: [] }} />
         </AlertsContextProvider>
       );
 

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
@@ -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';
@@ -48,7 +52,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,17 +85,17 @@ export const AlertForm = ({ alert, canChangeTrigger = true, dispatch, errors }:
   );
 
   const [alertTypesIndex, setAlertTypesIndex] = useState<AlertTypeIndex | undefined>(undefined);
-  const [alertInterval, setAlertInterval] = useState<number>(
-    alert.schedule.interval ? parseInt(alert.schedule.interval.replace(/^[A-Za-z]+$/, ''), 0) : 1
+  const [alertInterval, setAlertInterval] = useState<number | undefined>(
+    alert.schedule.interval ? getDurationNumberInItsUnit(alert.schedule.interval) : undefined
   );
   const [alertIntervalUnit, setAlertIntervalUnit] = useState<string>(
-    alert.schedule.interval ? alert.schedule.interval.replace(alertInterval.toString(), '') : 'm'
+    alert.schedule.interval ? getDurationUnitValue(alert.schedule.interval) : 'm'
   );
   const [alertThrottle, setAlertThrottle] = useState<number | null>(
-    alert.throttle ? parseInt(alert.throttle.replace(/^[A-Za-z]+$/, ''), 0) : null
+    alert.throttle ? getDurationNumberInItsUnit(alert.throttle) : null
   );
   const [alertThrottleUnit, setAlertThrottleUnit] = useState<string>(
-    alert.throttle ? alert.throttle.replace((alertThrottle ?? '').toString(), '') : 'm'
+    alert.throttle ? getDurationUnitValue(alert.throttle) : 'm'
   );
   const [defaultActionGroupId, setDefaultActionGroupId] = useState<string | undefined>(undefined);
 
@@ -344,19 +348,27 @@ export const AlertForm = ({ alert, canChangeTrigger = true, dispatch, errors }:
       <EuiSpacer size="m" />
       <EuiFlexGrid columns={2}>
         <EuiFlexItem>
-          <EuiFormRow fullWidth compressed label={labelForAlertChecked}>
+          <EuiFormRow
+            fullWidth
+            compressed
+            label={labelForAlertChecked}
+            isInvalid={errors.interval.length > 0}
+            error={errors.interval}
+          >
             <EuiFlexGroup gutterSize="s">
               <EuiFlexItem>
                 <EuiFieldNumber
                   fullWidth
                   min={1}
+                  isInvalid={errors.interval.length > 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/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(

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
@@ -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 = ({
               <EuiFieldNumber
                 data-test-subj="timeWindowSizeNumber"
                 isInvalid={errors.timeWindowSize.length > 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)}
             />
           </EuiFlexItem>
         </EuiFlexGroup>

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
@@ -63,7 +63,6 @@ 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');
 
       await testSubjects.click('.slack-ActionTypeSelectOption');
       await testSubjects.click('createActionConnectorButton');