Allowing validation of multiple actions with same connector id (#98065)…

… (#99048) Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: ymao1 <[email protected]>
elastic · May 3, 2021 · d5295e7 · d5295e7
1 parent 5bbe5e3
commit d5295e7
Show file tree

Hide file tree

Showing 3 changed files with 139 additions and 40 deletions.
diff --git a/x-pack/plugins/triggers_actions_ui/public/application/lib/value_validators.test.ts b/x-pack/plugins/triggers_actions_ui/public/application/lib/value_validators.test.ts
@@ -13,7 +13,7 @@ import {
   getAlertWithInvalidatedFields,
 } from './value_validators';
 import uuid from 'uuid';
-import { Alert, UserConfiguredActionConnector } from '../../types';
+import { Alert, IErrorObject, UserConfiguredActionConnector } from '../../types';
 
 describe('throwIfAbsent', () => {
   test('throws if value is absent', () => {
@@ -156,7 +156,7 @@ describe('getConnectorWithInvalidatedFields', () => {
 });
 
 describe('getAlertWithInvalidatedFields', () => {
-  test('set nulls to all required undefined fields in alert', () => {
+  test('sets to null all fields that are required but undefined in alert', () => {
     const alert: Alert = {
       params: {},
       consumer: 'test',
@@ -169,14 +169,40 @@ describe('getAlertWithInvalidatedFields', () => {
       enabled: false,
       mutedInstanceIds: [],
     } as any;
-    const baseAlertErrors = { name: ['Name is required.'] };
-    const actionsErrors = {};
+    const baseAlertErrors = {
+      name: ['Name is required.'],
+      alertTypeId: ['Rule type is required.'],
+    };
+    const actionsErrors: IErrorObject[] = [];
     const paramsErrors = {};
     getAlertWithInvalidatedFields(alert, paramsErrors, baseAlertErrors, actionsErrors);
     expect(alert.name).toBeNull();
+    expect(alert.alertTypeId).toBeNull();
+  });
+
+  test('does not set to null any fields that are required and defined but invalid in alert', () => {
+    const alert: Alert = {
+      name: 'test',
+      id: '123',
+      params: {},
+      consumer: '@@@@',
+      schedule: {
+        interval: '1m',
+      },
+      actions: [],
+      tags: [],
+      muteAll: false,
+      enabled: false,
+      mutedInstanceIds: [],
+    } as any;
+    const baseAlertErrors = { consumer: ['Consumer is invalid.'] };
+    const actionsErrors: IErrorObject[] = [];
+    const paramsErrors = {};
+    getAlertWithInvalidatedFields(alert, paramsErrors, baseAlertErrors, actionsErrors);
+    expect(alert.consumer).toEqual('@@@@');
   });
 
-  test('set nulls to all required undefined fields in alert params', () => {
+  test('set to null all fields that are required but undefined in alert params', () => {
     const alert: Alert = {
       name: 'test',
       alertTypeId: '.threshold',
@@ -204,18 +230,23 @@ describe('getAlertWithInvalidatedFields', () => {
       updatedBy: '',
     };
     const baseAlertErrors = {};
-    const actionsErrors = {};
-    const paramsErrors = { index: ['Index is required.'] };
+    const actionsErrors: IErrorObject[] = [];
+    const paramsErrors = { index: ['Index is required.'], timeField: ['Time field is required.'] };
     getAlertWithInvalidatedFields(alert, paramsErrors, baseAlertErrors, actionsErrors);
     expect(alert.params.index).toBeNull();
+    expect(alert.params.timeField).toBeNull();
   });
 
-  test('do not set nulls to the invalid fields with values in the connector properties, config and secrets', () => {
+  test('does not set to null any fields that are required and defined but invalid in alert params', () => {
     const alert: Alert = {
       name: 'test',
+      alertTypeId: '.threshold',
       id: '123',
-      params: {},
-      consumer: '@@@@',
+      params: {
+        aggField: 'foo',
+        termSize: 'big',
+      },
+      consumer: 'test',
       schedule: {
         interval: '1m',
       },
@@ -224,15 +255,75 @@ describe('getAlertWithInvalidatedFields', () => {
       muteAll: false,
       enabled: false,
       mutedInstanceIds: [],
-    } as any;
-    const baseAlertErrors = { consumer: ['Consumer is invalid.'] };
-    const actionsErrors = {};
+      createdBy: '',
+      apiKeyOwner: '',
+      createdAt: new Date(),
+      executionStatus: {
+        status: 'ok',
+        lastExecutionDate: new Date(),
+      },
+      notifyWhen: 'onActionGroupChange',
+      throttle: '',
+      updatedAt: new Date(),
+      updatedBy: '',
+    };
+    const baseAlertErrors = {};
+    const actionsErrors: IErrorObject[] = [];
+    const paramsErrors = {
+      aggField: ['Aggregation field is invalid.'],
+      termSize: ['Term size is invalid.'],
+    };
+    getAlertWithInvalidatedFields(alert, paramsErrors, baseAlertErrors, actionsErrors);
+    expect(alert.params.aggField).toEqual('foo');
+    expect(alert.params.termSize).toEqual('big');
+  });
+
+  test('set to null all fields that are required but undefined in alert actions', () => {
+    const alert: Alert = {
+      name: 'test',
+      alertTypeId: '.threshold',
+      id: '123',
+      params: {},
+      consumer: 'test',
+      schedule: {
+        interval: '1m',
+      },
+      actions: [
+        {
+          actionTypeId: 'test',
+          group: 'qwer',
+          id: '123',
+          params: {
+            incident: {
+              field: {},
+            },
+          },
+        },
+      ],
+      tags: [],
+      muteAll: false,
+      enabled: false,
+      mutedInstanceIds: [],
+      createdBy: '',
+      apiKeyOwner: '',
+      createdAt: new Date(),
+      executionStatus: {
+        status: 'ok',
+        lastExecutionDate: new Date(),
+      },
+      notifyWhen: 'onActionGroupChange',
+      throttle: '',
+      updatedAt: new Date(),
+      updatedBy: '',
+    };
+    const baseAlertErrors = {};
+    const actionsErrors = [{ 'incident.field.name': ['Name is required.'] }];
     const paramsErrors = {};
     getAlertWithInvalidatedFields(alert, paramsErrors, baseAlertErrors, actionsErrors);
-    expect(alert.consumer).toEqual('@@@@');
+    expect((alert.actions[0].params as any).incident.field.name).toBeNull();
   });
 
-  test('if complex alert action fields which is required is set to nulls if it is undefined', () => {
+  test('validates multiple alert actions with the same connector id', () => {
     const alert: Alert = {
       name: 'test',
       alertTypeId: '.threshold',
@@ -253,6 +344,18 @@ describe('getAlertWithInvalidatedFields', () => {
             },
           },
         },
+        {
+          actionTypeId: 'test',
+          group: 'qwer',
+          id: '123',
+          params: {
+            incident: {
+              field: {
+                name: 'myIncident',
+              },
+            },
+          },
+        },
       ],
       tags: [],
       muteAll: false,
@@ -271,9 +374,13 @@ describe('getAlertWithInvalidatedFields', () => {
       updatedBy: '',
     };
     const baseAlertErrors = {};
-    const actionsErrors = { '123': { 'incident.field.name': ['Name is required.'] } };
+    const actionsErrors = [
+      { 'incident.field.name': ['Name is required.'] },
+      { 'incident.field.name': ['Name is invalid.'] },
+    ];
     const paramsErrors = {};
     getAlertWithInvalidatedFields(alert, paramsErrors, baseAlertErrors, actionsErrors);
     expect((alert.actions[0].params as any).incident.field.name).toBeNull();
+    expect((alert.actions[1].params as any).incident.field.name).toEqual('myIncident');
   });
 });
diff --git a/x-pack/plugins/triggers_actions_ui/public/application/lib/value_validators.ts b/x-pack/plugins/triggers_actions_ui/public/application/lib/value_validators.ts
@@ -74,7 +74,7 @@ export function getAlertWithInvalidatedFields(
   alert: Alert,
   paramsErrors: IErrorObject,
   baseAlertErrors: IErrorObject,
-  actionsErrors: Record<string, IErrorObject>
+  actionsErrors: IErrorObject[]
 ) {
   Object.keys(paramsErrors).forEach((errorKey) => {
     if (paramsErrors[errorKey].length >= 1 && get(alert.params, errorKey) === undefined) {
@@ -86,17 +86,15 @@ export function getAlertWithInvalidatedFields(
       set(alert, errorKey, null);
     }
   });
-  Object.keys(actionsErrors).forEach((actionId) => {
-    const actionToValidate = alert.actions.find((action) => action.id === actionId);
-    Object.keys(actionsErrors[actionId]).forEach((errorKey) => {
-      if (
-        actionToValidate &&
-        actionsErrors[actionId][errorKey].length >= 1 &&
-        get(actionToValidate!.params, errorKey) === undefined
-      ) {
-        set(actionToValidate!.params, errorKey, null);
-      }
-    });
+  actionsErrors.forEach((error: IErrorObject, index: number) => {
+    const actionToValidate = alert.actions.length > index ? alert.actions[index] : null;
+    if (actionToValidate) {
+      Object.keys(error).forEach((errorKey) => {
+        if (error[errorKey].length >= 1 && get(actionToValidate!.params, errorKey) === undefined) {
+          set(actionToValidate!.params, errorKey, null);
+        }
+      });
+    }
   });
   return alert;
 }
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
@@ -135,14 +135,10 @@ export function getAlertErrors(
     ...alertBaseErrors,
   } as IErrorObject;
 
-  const alertActionsErrors = alert.actions.reduce((prev, alertAction: AlertAction) => {
-    return {
-      ...prev,
-      [alertAction.id]: actionTypeRegistry
-        .get(alertAction.actionTypeId)
-        ?.validateParams(alertAction.params).errors,
-    };
-  }, {}) as Record<string, IErrorObject>;
+  const alertActionsErrors = alert.actions.map((alertAction: AlertAction) => {
+    return actionTypeRegistry.get(alertAction.actionTypeId)?.validateParams(alertAction.params)
+      .errors;
+  });
   return {
     alertParamsErrors,
     alertBaseErrors,
@@ -160,13 +156,11 @@ export const hasObjectErrors: (errors: IErrorObject) => boolean = (errors) =>
 export function isValidAlert(
   alertObject: InitialAlert | Alert,
   validationResult: IErrorObject,
-  actionsErrors: Record<string, IErrorObject>
+  actionsErrors: IErrorObject[]
 ): alertObject is Alert {
   return (
     !hasObjectErrors(validationResult) &&
-    Object.keys(actionsErrors).find((actionErrorsKey) =>
-      hasObjectErrors(actionsErrors[actionErrorsKey])
-    ) === undefined
+    actionsErrors.every((error: IErrorObject) => !hasObjectErrors(error))
   );
 }