Address comments

x-pack/plugins/alerting/common/routes/rule/validation/validate_flapping/v1.test.ts

@@ -0,0 +1,57 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0; you may not use this file except in compliance with the Elastic License
+ * 2.0.
+ */
+
+import { validateFlapping } from './v1';
+
+describe('validateFlapping', () => {
+  test('should error if look back window exceeds the lower bound', () => {
+    const result = validateFlapping({
+      look_back_window: 0,
+      status_change_threshold: 10,
+    });
+
+    expect(result).toEqual('look back window must be between 2 and 20');
+  });
+
+  test('should error if look back window exceeds the upper bound', () => {
+    const result = validateFlapping({
+      look_back_window: 50,
+      status_change_threshold: 10,
+    });
+
+    expect(result).toEqual('look back window must be between 2 and 20');
+  });
+
+  test('should error if status change threshold exceeds the lower bound', () => {
+    const result = validateFlapping({
+      look_back_window: 10,
+      status_change_threshold: 1,
+    });
+
+    expect(result).toEqual('status change threshold must be between 2 and 20');
+  });
+
+  test('should error if status change threshold exceeds the upper bound', () => {
+    const result = validateFlapping({
+      look_back_window: 10,
+      status_change_threshold: 50,
+    });
+
+    expect(result).toEqual('status change threshold must be between 2 and 20');
+  });
+
+  test('should error if status change threshold is greater than the look back window', () => {
+    const result = validateFlapping({
+      look_back_window: 10,
+      status_change_threshold: 15,
+    });
+
+    expect(result).toEqual(
+      'lookBackWindow (10) must be equal to or greater than statusChangeThreshold (15)'
+    );
+  });
+});

x-pack/plugins/alerting/server/application/rule/methods/update/update_rule.ts

@@ -51,6 +51,11 @@ const validateCanUpdateFlapping = (
     return;
   }
 
+  // If updated flapping is undefined then don't do anything, it's not being updated
+  if (updateFlapping === undefined) {
+    return;
+  }
+
   // If both versions are falsy, allow it even if its changing between undefined and null
   if (!originalFlapping && !updateFlapping) {
     return;
@@ -154,12 +159,6 @@ async function updateWithOCC<Params extends RuleParams = never>(
 
   validateCanUpdateFlapping(isFlappingEnabled, originalFlapping, initialData.flapping);
 
-  if (!isFlappingEnabled && !isEqual(originalFlapping, initialData.flapping)) {
-    throw Boom.badRequest(
-      `Error updating rule: can not update rule flapping if global flapping is disabled`
-    );
-  }
-
   let validationPayload: ValidateScheduleLimitResult = null;
   if (enabled && schedule.interval !== data.schedule.interval) {
     validationPayload = await validateScheduleLimit({

x-pack/plugins/alerting/server/application/rule/transforms/transform_rule_attributes_to_rule_domain.test.ts

@@ -92,6 +92,10 @@ describe('transformRuleAttributesToRuleDomain', () => {
     updatedBy: 'user',
     apiKey: MOCK_API_KEY,
     apiKeyOwner: 'user',
+    flapping: {
+      lookBackWindow: 20,
+      statusChangeThreshold: 20,
+    },
   };
 
   it('transforms the actions correctly', () => {
@@ -106,6 +110,13 @@ describe('transformRuleAttributesToRuleDomain', () => {
       isSystemAction
     );
 
+    expect(res.flapping).toMatchInlineSnapshot(`
+      Object {
+        "lookBackWindow": 20,
+        "statusChangeThreshold": 20,
+      }
+    `);
+
     expect(res.actions).toMatchInlineSnapshot(`
       Array [
         Object {

x-pack/plugins/alerting/server/application/rule/transforms/transform_rule_domain_to_rule.test.ts

@@ -67,6 +67,10 @@ describe('transformRuleDomainToRule', () => {
     updatedBy: 'user',
     apiKey: MOCK_API_KEY,
     apiKeyOwner: 'user',
+    flapping: {
+      lookBackWindow: 20,
+      statusChangeThreshold: 20,
+    },
   };
 
   it('should transform rule domain to rule', () => {
@@ -100,6 +104,10 @@ describe('transformRuleDomainToRule', () => {
       revision: 0,
       updatedBy: 'user',
       apiKeyOwner: 'user',
+      flapping: {
+        lookBackWindow: 20,
+        statusChangeThreshold: 20,
+      },
     });
   });
 
@@ -135,6 +143,10 @@ describe('transformRuleDomainToRule', () => {
       revision: 0,
       updatedBy: 'user',
       apiKeyOwner: 'user',
+      flapping: {
+        lookBackWindow: 20,
+        statusChangeThreshold: 20,
+      },
     });
   });
 
@@ -172,6 +184,10 @@ describe('transformRuleDomainToRule', () => {
       revision: 0,
       updatedBy: 'user',
       apiKeyOwner: 'user',
+      flapping: {
+        lookBackWindow: 20,
+        statusChangeThreshold: 20,
+      },
     });
   });
 });

x-pack/plugins/alerting/server/application/rule/transforms/transform_rule_domain_to_rule_attributes.test.ts

@@ -0,0 +1,122 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0; you may not use this file except in compliance with the Elastic License
+ * 2.0.
+ */
+
+import { RuleDomain } from '../types';
+import { transformRuleDomainToRuleAttributes } from './transform_rule_domain_to_rule_attributes';
+
+describe('transformRuleDomainToRuleAttributes', () => {
+  const MOCK_API_KEY = Buffer.from('123:abc').toString('base64');
+
+  const defaultAction = {
+    id: '1',
+    uuid: 'test-uuid',
+    group: 'default',
+    actionTypeId: 'test',
+    params: {},
+  };
+
+  const rule: RuleDomain<{}> = {
+    id: 'test',
+    enabled: false,
+    name: 'my rule name',
+    tags: ['foo'],
+    alertTypeId: 'myType',
+    consumer: 'myApp',
+    schedule: { interval: '1m' },
+    actions: [defaultAction],
+    params: {},
+    mapped_params: {},
+    createdBy: 'user',
+    createdAt: new Date('2019-02-12T21:01:22.479Z'),
+    updatedAt: new Date('2019-02-12T21:01:22.479Z'),
+    legacyId: 'legacyId',
+    muteAll: false,
+    mutedInstanceIds: [],
+    snoozeSchedule: [],
+    scheduledTaskId: 'task-123',
+    executionStatus: {
+      lastExecutionDate: new Date('2019-02-12T21:01:22.479Z'),
+      status: 'pending' as const,
+    },
+    throttle: null,
+    notifyWhen: null,
+    revision: 0,
+    updatedBy: 'user',
+    apiKey: MOCK_API_KEY,
+    apiKeyOwner: 'user',
+    flapping: {
+      lookBackWindow: 20,
+      statusChangeThreshold: 20,
+    },
+  };
+
+  test('should transform rule domain to rule attribute', () => {
+    const result = transformRuleDomainToRuleAttributes({
+      rule,
+      actionsWithRefs: [
+        {
+          group: 'default',
+          actionRef: 'action_0',
+          actionTypeId: 'test',
+          uuid: 'test-uuid',
+          params: {},
+        },
+      ],
+      params: {
+        legacyId: 'test',
+        paramsWithRefs: {},
+      },
+    });
+
+    expect(result).toMatchInlineSnapshot(`
+      Object {
+        "actions": Array [
+          Object {
+            "actionRef": "action_0",
+            "actionTypeId": "test",
+            "group": "default",
+            "params": Object {},
+            "uuid": "test-uuid",
+          },
+        ],
+        "alertTypeId": "myType",
+        "apiKey": "MTIzOmFiYw==",
+        "apiKeyOwner": "user",
+        "consumer": "myApp",
+        "createdAt": "2019-02-12T21:01:22.479Z",
+        "createdBy": "user",
+        "enabled": false,
+        "executionStatus": Object {
+          "lastExecutionDate": "2019-02-12T21:01:22.479Z",
+          "status": "pending",
+        },
+        "flapping": Object {
+          "lookBackWindow": 20,
+          "statusChangeThreshold": 20,
+        },
+        "legacyId": "test",
+        "muteAll": false,
+        "mutedInstanceIds": Array [],
+        "name": "my rule name",
+        "notifyWhen": null,
+        "params": Object {},
+        "revision": 0,
+        "schedule": Object {
+          "interval": "1m",
+        },
+        "scheduledTaskId": "task-123",
+        "snoozeSchedule": Array [],
+        "tags": Array [
+          "foo",
+        ],
+        "throttle": null,
+        "updatedAt": "2019-02-12T21:01:22.479Z",
+        "updatedBy": "user",
+      }
+    `);
+  });
+});

x-pack/test/alerting_api_integration/spaces_only/tests/alerting/group1/create.ts

@@ -636,7 +636,7 @@ export default function createAlertTests({ getService }: FtrProviderContext) {
           await resetRulesSettings(supertest, 'space1');
         });
 
-        it('should allow flapping to be updated', async () => {
+        it('should allow flapping to be created', async () => {
           const response = await supertest
             .post(`${getUrlPrefix(Spaces.space1.id)}/api/alerting/rule`)
             .set('kbn-xsrf', 'foo')

x-pack/test/alerting_api_integration/spaces_only/tests/alerting/group2/update.ts

@@ -294,13 +294,53 @@ export default function createUpdateTests({ getService }: FtrProviderContext) {
             },
           })
           .expect(400);
+      });
+
+      it('should allow rule to be updated when global flapping is off if not updating flapping', async () => {
+        const response = await supertest
+          .post(`${getUrlPrefix(Spaces.space1.id)}/api/alerting/rule`)
+          .set('kbn-xsrf', 'foo')
+          .send(
+            getTestRuleData({
+              flapping: {
+                look_back_window: 5,
+                status_change_threshold: 5,
+              },
+            })
+          );
+
+        objectRemover.add(Spaces.space1.id, response.body.id, 'rule', 'alerting');
+
+        await supertest
+          .post(`${getUrlPrefix(Spaces.space1.id)}/internal/alerting/rules/settings/_flapping`)
+          .set('kbn-xsrf', 'foo')
+          .send({
+            enabled: false,
+            look_back_window: 5,
+            status_change_threshold: 5,
+          });
 
-        // Can still update when global flapping is off if not updating flapping
         await supertest
           .put(`${getUrlPrefix(Spaces.space1.id)}/api/alerting/rule/${response.body.id}`)
           .set('kbn-xsrf', 'foo')
           .send({
-            name: 'bcd',
+            name: 'updated name 1',
+            tags: ['foo'],
+            params: {
+              foo: true,
+            },
+            schedule: { interval: '12s' },
+            actions: [],
+            throttle: '1m',
+            notify_when: 'onThrottleInterval',
+          })
+          .expect(200);
+
+        await supertest
+          .put(`${getUrlPrefix(Spaces.space1.id)}/api/alerting/rule/${response.body.id}`)
+          .set('kbn-xsrf', 'foo')
+          .send({
+            name: 'updated name 2',
             tags: ['foo'],
             params: {
               foo: true,
@@ -309,6 +349,10 @@ export default function createUpdateTests({ getService }: FtrProviderContext) {
             actions: [],
             throttle: '1m',
             notify_when: 'onThrottleInterval',
+            flapping: {
+              look_back_window: 5,
+              status_change_threshold: 5,
+            },
           })
           .expect(200);
       });