[Alert Summaries] [FE] Move “Notify When” and throttle from rule to action #145637
Conversation
…-ref HEAD~1..HEAD --fix'
…kibana into 143368-notify-migration
…kibana into 143368-notify-migration
…tion # Conflicts: # x-pack/plugins/alerting/server/routes/lib/index.ts
…tion # Conflicts: # x-pack/plugins/alerting/server/task_runner/task_runner.ts
…tion # Conflicts: # x-pack/plugins/alerting/server/routes/create_rule.ts # x-pack/plugins/alerting/server/routes/lib/index.ts # x-pack/plugins/alerting/server/routes/update_rule.ts # x-pack/plugins/alerting/server/rules_client/rules_client.ts
|
We tried to see if we were able to do some minimal change with the APIs to move forward in the wanted direction. However, @xcrzx is right that it won't work, so in the mean time we are going to avoid any changes in security solutions APIs. In the task runner, we will check the consumer of the rule and if the consumer is security solution then we will use/pick the global I think we will need to re-think about the bulk actions in security solution. Maybe they might be able to re-use our actions settings to make it easier for their users. |
| if ('actions' in editPayload.value) { | ||
| editPayload.value.actions = editPayload.value.actions.map((a) => omit(a, 'frequency')); | ||
| } |
There was a problem hiding this comment.
Why is this transformation needed? According to the type of actions, there is no frequency field:
(property) actions: {
group: string;
id: string;
params: SavedObjectAttributes;
}[]
There was a problem hiding this comment.
You asked me to remove frequency from this type definition, but the UI components from Triggers Actions UI are still going to send a frequency anyway. From what I can understand, the type system Security Solution built before Alerting/TriggersActionsUI was feature-complete includes a NormalizedRuleAction which is supposed to pull double duty for what we expect to receive from the Triggers Actions UI, and what we expect to send through Security Solution's now-redundant alerting API.
I do not know how to untangle this. I've added a comment explaining it, but at this point I have to consider perfect type definitions out of scope for this PR and recommend that your team fix it in #148414
There was a problem hiding this comment.
I got it. Thank you for adding a comment and creating a ticket to address the discrepancy in types.
This reverts commit 40ce55a.
...triggers_actions_ui/public/application/sections/action_connector_form/action_notify_when.tsx
Outdated
Show resolved
Hide resolved
| (newValue: RuleNotifyWhenType) => { | ||
| onNotifyWhenChange(newValue); | ||
| setNotifyWhenValue(newValue); | ||
| setTimeout( |
There was a problem hiding this comment.
why do we need the setTimeout here?
There was a problem hiding this comment.
Needed to call both onNotifyWhenChange and onThrottleChange in the same function, otherwise React gets weird. Trying to avoid refactoring the entire form in order to make this PR work. I can add a comment
There was a problem hiding this comment.
make sense!, let's add a comment/issue to refactor it later on
...triggers_actions_ui/public/application/sections/action_connector_form/action_notify_when.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I tested it locally by creating a rule from main and then upgrade to this PR and everything is/was working as expected.
Thank you for your hard work on this BIG PR! Good job as always! and nicely done!
in this file x-pack/plugins/triggers_actions_ui/public/application/sections/action_connector_form/action_notify_when.tsx, I still think we can remove the notifyWhenValue. ;)
|
@elasticmachine merge upstream |
|
I think @elastic/security-solution-platform just needs another 24 hours to test out some edge cases around bulk-editing actions for rules but I don't see any issues coming up. I believe @banderror will comment if anything looks out of the ordinary otherwise I can approve then 👍 thanks!! |
|
@XavierM We still need the |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
|
@dhurley14 Sorry this was on auto-merge and I didn't realize the Platform review was non-blocking. Let me know if any issues come up with edge cases and I'll prioritize them |
|
no worries! cc: @banderror |
…ction (elastic#145637) ## Summary Closes elastic#143369 (~blocked by elastic#143376) This PR updates the Stack Management UI and Observability UI to show Notify When and Throttle parameters at the **action level** instead of the **rule level**. The rule-level Check Every dropdown is moved to the end of the rule, right above the actions form The Security Solution UX remains unchanged, as it has a unique way of displaying action notification frequencies at the rule level. Instead, the API request has changed so that the selected action frequency will now be stored in each action's `frequency` param instead of at the rule level. In all three UIs, existing rules that have legacy rule-level `notifyWhen` and `frequency` params will have these parameters seamlessly migrated to the action level when the user edits a rule. The Rule Details page is also updated to show Notify frequencies in the Actions column instead of in the first, rule-level column. ### Rule Details Page update <img width="781" alt="Screen Shot 2022-11-17 at 4 23 02 PM" src="https://user-images.githubusercontent.com/1445834/202573067-bc55630d-f767-4a93-8d7c-752748da25c2.png"> ### Rule Form update <img width="605" alt="Screen Shot 2022-11-17 at 4 23 10 PM" src="https://user-images.githubusercontent.com/1445834/202573057-5d50e573-1453-4b63-8e1e-6505fa0261c6.png"> <img width="605" alt="Screen Shot 2022-12-27 at 1 18 12 PM" src="https://user-images.githubusercontent.com/1445834/209712784-34c2384b-bcc8-4db9-a42d-052d81099a40.png"> ### Checklist - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Xavier Mouligneau <xavier.mouligneau@elastic.co>
Summary
Closes #143369 (
blocked by #143376)This PR updates the Stack Management UI and Observability UI to show Notify When and Throttle parameters at the action level instead of the rule level.
The rule-level Check Every dropdown is moved to the end of the rule, right above the actions form
The Security Solution UX remains unchanged, as it has a unique way of displaying action notification frequencies at the rule level. Instead, the API request has changed so that the selected action frequency will now be stored in each action's
frequencyparam instead of at the rule level.In all three UIs, existing rules that have legacy rule-level
notifyWhenandfrequencyparams will have these parameters seamlessly migrated to the action level when the user edits a rule.The Rule Details page is also updated to show Notify frequencies in the Actions column instead of in the first, rule-level column.
Rule Details Page update
Rule Form update
Checklist