-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Added UI support for the default action group for Alert Type Model #57603
Added UI support for the default action group for Alert Type Model #57603
Conversation
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
…efault_action_group_as_props # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
@@ -95,6 +96,7 @@ export interface AlertTypeModel { | |||
validate: (alertParams: any) => ValidationResult; | |||
alertParamsExpression: React.FunctionComponent<any>; | |||
defaultActionMessage?: string; | |||
defaultActionGroup?: string; |
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.
I'm thinking this would belong nicely next to actionGroups
on the server side? here: https://github.com/elastic/kibana/blob/master/x-pack/plugins/alerting/server/types.ts#L66. Then we return the defaultActionGroup with the /types
API. Thoughts?
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.
Good point. I will update the PR the way you proposed.
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.
Make it optional? And then the same logic to return the first one, if not specified? Probably makes sense to do that, as long as we can do it in one place; we shouldn't repeat that default calculation on the client, if we can.
…efault_action_group_as_props # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
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.
LGTM
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.
Looking good! Just a few small comments from my side. I believe we should have documentation about this limitation of only 1 action group?
x-pack/plugins/triggers_actions_ui/public/application/sections/alert_add/alert_form.tsx
Outdated
Show resolved
Hide resolved
…stic.com> Feb 20, 2020 at 12:40 PM
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.
@YulNaumenko For clarity, do you think it'd be better to call it defaultActionGroupId
? Since ActionGroup
is a type, being explicit that this is the id
might reduce confusion. This might be a nit, but wanted to at least bring it up.
Yeah, from the usage prospective it might be more clear. I will rename it. |
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.
Changes LGTM, thanks for doing this! 🙏
…efault_action_group_as_props # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
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.
LGTM!
…efault_action_group_as_props # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
…_improve-advanced-settings-save * commit '98aa1d2d4f974f72a9a5397b1b91f11509f6fb7a': [SIEM] [Case] Enable case by default. Snake to camel on UI (elastic#57936) [File upload] Update remaining File Upload dependencies for NP migration (elastic#58128) Use EuiTokens for ES field types (elastic#57911) Added UI support for the default action group for Alert Type Model (elastic#57603) force savedObject API consumers to define SO type explicitly (elastic#58022) Update dependency @elastic/charts to ^17.1.1 (elastic#57634)
Friendly reminder: Looks like this PR hasn’t been backported yet. |
1 similar comment
Friendly reminder: Looks like this PR hasn’t been backported yet. |
…-out-of-legacy * 'master' of github.com:elastic/kibana: [SIEM] [Case] Enable case by default. Snake to camel on UI (elastic#57936) [File upload] Update remaining File Upload dependencies for NP migration (elastic#58128) Use EuiTokens for ES field types (elastic#57911) Added UI support for the default action group for Alert Type Model (elastic#57603) force savedObject API consumers to define SO type explicitly (elastic#58022) Update dependency @elastic/charts to ^17.1.1 (elastic#57634) [Endpoint] Add a flyout to alert list. (elastic#57926) Make sure index pattern has fields before parsing (elastic#58242) Sanitize workpad before sending to API (elastic#57704) [ML] Transform: Support multi-line JSON notation in advanced editor (elastic#58015) [Endpoint] Refactor Management List Tests (elastic#58148) [kbn/optimizer] include bootstrap cache key in optimizer cache key (elastic#58176) Do not refresh color scale on each lookup (elastic#57792) Updating to @elastic/[email protected] (elastic#54662) Trigger context (elastic#57870) [ML] Transforms: Adds clone feature to transforms list. (elastic#57837) [ML] New Platform server shim: update fields service routes (elastic#58060) [Endpoint] EMT-184: change endpoints to metadata up and down the code base. (elastic#58038)
Friendly reminder: Looks like this PR hasn’t been backported yet. |
1 similar comment
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Summary
Added UI support for the default action group for Alert Type Model
Checklist
Delete any items that are not applicable to this PR.