-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Alert Summaries] [FE] Move “Notify When” and throttle from rule to action #145637
Changes from 93 commits
52d3433
d077ea1
378d4e8
11cd1d3
4889023
1a84f27
4349c73
1c869ab
ef43549
cb3f976
77711c2
3d23b1c
2abc7ae
66810ae
adf815f
20d4983
24fe88c
b35150b
bed44bc
3b12f0f
b2bebdf
e6b1843
11e16a5
dec2b1a
bed5753
26df5b5
cda3467
023cb65
7a517e0
36d0843
da42cd2
62b4334
54024f9
62005c1
1c8e2cf
366e023
d6a2f34
c59a399
22eed60
5830ba1
0205923
9d626f2
074307e
8ea7895
5c578ca
6c577d6
bd6f4f1
966d2ce
d3e9e60
4624d59
63cec0d
0cb91c2
1906ea8
969a963
275ee63
ca2b24c
302f6d8
6b32d1c
595653b
2e88eea
3441667
71ce77b
8c0a66f
b41ec91
eadef78
43f84e9
44d95eb
49e5ff2
1326885
6a36797
ad90176
d839d89
5a50078
d1c330f
9ffa1a1
265a6c1
593fd95
dfdf80f
40ce55a
6e2a74a
cf1ba82
d489b24
275a603
faa989c
0b7980a
a359425
38dd49f
5c35ec0
1b259c9
524a2c0
72ffb30
1f36692
e4a6f17
756edc6
b846f90
b1aaca6
a4e2ff0
66f7156
4f2e4b3
c775791
c64b8cc
eb2d63d
4f30e42
18e277a
1d81b60
f3acdda
9d37ce7
2614f4a
339c67e
d748d54
6006849
c5dfe8a
8c13b2e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,27 +25,44 @@ export const RuleActionTypeId = t.string; | |
export type RuleActionParams = t.TypeOf<typeof RuleActionParams>; | ||
export const RuleActionParams = saved_object_attributes; | ||
|
||
export type RuleActionFrequency = t.TypeOf<typeof RuleActionFrequency>; | ||
export const RuleActionFrequency = t.type({ | ||
summary: t.boolean, | ||
notifyWhen: t.union([ | ||
t.literal('onActionGroupChange'), | ||
t.literal('onActiveAlert'), | ||
t.literal('onThrottleInterval'), | ||
]), | ||
throttle: t.union([t.string, t.null]), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if the action-level throttle is passed together with the rule-level throttle via API? Should we disallow that on the API level,i.e., return 400? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This already happens at the RulesClient level |
||
}); | ||
|
||
export type RuleAction = t.TypeOf<typeof RuleAction>; | ||
export const RuleAction = t.exact( | ||
t.type({ | ||
group: RuleActionGroup, | ||
id: RuleActionId, | ||
action_type_id: RuleActionTypeId, | ||
params: RuleActionParams, | ||
}) | ||
t.intersection([ | ||
t.type({ | ||
group: RuleActionGroup, | ||
id: RuleActionId, | ||
action_type_id: RuleActionTypeId, | ||
params: RuleActionParams, | ||
}), | ||
t.partial({ frequency: RuleActionFrequency }), | ||
Zacqary marked this conversation as resolved.
Show resolved
Hide resolved
|
||
]) | ||
); | ||
|
||
export type RuleActionArray = t.TypeOf<typeof RuleActionArray>; | ||
export const RuleActionArray = t.array(RuleAction); | ||
|
||
export type RuleActionCamel = t.TypeOf<typeof RuleActionCamel>; | ||
export const RuleActionCamel = t.exact( | ||
t.type({ | ||
group: RuleActionGroup, | ||
id: RuleActionId, | ||
actionTypeId: RuleActionTypeId, | ||
params: RuleActionParams, | ||
}) | ||
t.intersection([ | ||
t.type({ | ||
group: RuleActionGroup, | ||
id: RuleActionId, | ||
actionTypeId: RuleActionTypeId, | ||
params: RuleActionParams, | ||
}), | ||
t.partial({ frequency: RuleActionFrequency }), | ||
]) | ||
); | ||
|
||
export type RuleActionArrayCamel = t.TypeOf<typeof RuleActionArrayCamel>; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,12 @@ export const RuleNotifyWhenTypeValues = [ | |
] as const; | ||
export type RuleNotifyWhenType = typeof RuleNotifyWhenTypeValues[number]; | ||
|
||
export const RuleNotifyWhen: Record<string, RuleNotifyWhenType> = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm curious why don't you use enum for this type? |
||
CHANGE: 'onActionGroupChange', | ||
ACTIVE: 'onActiveAlert', | ||
THROTTLE: 'onThrottleInterval', | ||
}; | ||
|
||
export function validateNotifyWhenType(notifyWhen: string) { | ||
if (RuleNotifyWhenTypeValues.includes(notifyWhen as RuleNotifyWhenType)) { | ||
return; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -63,10 +63,7 @@ export const rewriteRule = ({ | |
connector_type_id: actionTypeId, | ||
...(frequency | ||
? { | ||
frequency: { | ||
...frequency, | ||
notify_when: frequency.notifyWhen, | ||
}, | ||
frequency, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why we do not need to do remove There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This fixes a breaking test. Something is broken in the library that converts between camelCase and snake_case inside nested properties. I'll create an issue to investigate further, forgot to do that when I found this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes please do and let's fix it before this is going in the next release! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
: {}), | ||
})), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -78,6 +78,7 @@ export type BulkEditOperation = | |
operation: 'add' | 'set'; | ||
field: Extract<BulkEditFields, 'actions'>; | ||
value: NormalizedAlertAction[]; | ||
syncFrequency?: boolean; | ||
} | ||
| { | ||
operation: 'set'; | ||
|
@@ -540,7 +541,23 @@ async function getUpdatedAttributesFromOperations( | |
// the `isAttributesUpdateSkipped` flag to false. | ||
switch (operation.field) { | ||
case 'actions': { | ||
await validateActions(context, ruleType, { ...attributes, actions: operation.value }); | ||
// Prepare to handle the case of when the rule attributes contain legacy rule-level throttle or notifyWhen, | ||
// and we're bulk adding actions with action-level frequency params. | ||
let attempts = 0; | ||
while (attempts < 2) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd recommend to move it into a separate function since attempts implementation isn't straightforward and it's only clear after reading a comment at the catch block. |
||
attempts++; | ||
try { | ||
await validateActions(context, ruleType, { | ||
...attributes, | ||
actions: operation.value, | ||
}); | ||
} catch (e) { | ||
// If validateActions fails on the first attempt, try to remove the rule-level frequency params. | ||
// The loop will attempt to validate again. Only allow the error to throw uncaught if it happens twice. | ||
if (typeof attributes.notifyWhen !== 'undefined') attributes.notifyWhen = undefined; | ||
if (attributes.throttle) attributes.throttle = undefined; | ||
} | ||
} | ||
|
||
const { modifiedAttributes, isAttributeModified } = applyBulkEditOperation( | ||
operation, | ||
|
@@ -550,6 +567,15 @@ async function getUpdatedAttributesFromOperations( | |
ruleActions = modifiedAttributes; | ||
isAttributesUpdateSkipped = false; | ||
} | ||
if (operation.syncFrequency) { | ||
const frequency = operation.value[0]?.frequency; | ||
if (frequency) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: It would be nice to get rid of a nested |
||
ruleActions.actions = ruleActions.actions.map((action) => ({ | ||
...action, | ||
frequency, | ||
})); | ||
} | ||
} | ||
break; | ||
} | ||
case 'snoozeSchedule': { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
frequency.notifyWhen
is always camelCased, even in API requests. When building the initial backend for this feature we ran into some issues with the functions that convert snake_casing to camelCasing in API requests when there's a nested parameter like the frequency, so I've replicated this schema in the security solution as well for consistency.