[Alerting] fixes buggy default message behaviour #84202

gmmorris · 2020-11-24T12:24:48Z

Summary

This PR addresses some weird UX we've identified with default values in Action Params components and their inferred defaults when placed inside of an Alerts flyout.
Key changes:

Typing of these components has been corrected to reflect that we expect these parameters to only be partial, as the form is used to set these values (for example, the message field of the Server Log action, might or might not be set, so it should be nullable, but in the typing we treated it as the final valid state, which is message not being nullable).
When a default message is set by the params components, the are tracked against the value of the default, which means that if the default changes, then so will the value in the field. Custom values provided by the user will not be overridden when the default changes. This has to be handled by the component itself at the moment (hopefully in the future we can make this a concern of the flyout and not each component).
The concept of the "Recovered" action group has been removed from these components - that's an Alerting concern, not actions, and shouldn't appear in the action components' code.

Checklist

Delete any items that are not applicable to this PR.

~~[] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~[ ] Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
~~[ ] Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)~~
~~[ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)~~
~~[ ] This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)~~
~~[ ] This was checked for cross-browser compatibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately

* master: (70 commits) [Uptime] Fix headers io-ts type (elastic#84089) [fleet] Add config options to accepted docker env vars (elastic#84338) [Fleet] Support URL query state in agent logs UI (elastic#84298) [basePathProxy] include query in redirect (elastic#84356) [Security Solution] Add Endpoint policy feature checks (elastic#83972) Fix issues with show_license_expiration (elastic#84361) [Security Solution][Resolver] Add support for predefined schemas for endpoint and winlogbeat (elastic#84103) [cli/dev] log a warning when --no-base-path is used with --dev (elastic#84354) [Fleet] Support input-level vars & templates (elastic#83878) [APM] Elastic chart issues (elastic#84238) [Time to Visualize] Fix Unlink Action via Rollback of ReplacePanel (elastic#83873) redirect to visualize listing page when by value visualization editor doesn't have a value input (elastic#84287) add live region for field search (elastic#84310) [ML] Persisted URL state for Anomalies table (elastic#84314) [dev/cli] detect worker type using env, not cluster module (elastic#83977) [Workplace Search] Migrate DisplaySettings tree (elastic#84283) Deprecate `xpack.task_manager.index` setting (elastic#84155) [Search] Search batching using bfetch (again) (elastic#84043) Use .kibana instead of .kibana_current to mark migration completion (elastic#83373) [Monitoring] Only look at ES for the missing data alert for now (elastic#83839) ...

elasticmachine · 2020-11-26T11:43:58Z

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

cnasikas

Jira changes LGTM!

gmmorris · 2020-11-30T09:50:27Z

@elasticmachine merge upstream

kibanamachine · 2020-11-30T11:30:18Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 031c902

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`triggersActionsUi`	1.5MB	1.5MB	+3.3KB

History

💚 Build #90420 succeeded c967f84
💚 Build #90387 succeeded 2c5348f
💔 Build #89990 failed f9c7560
💔 Build #89925 failed 00f7ef3

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

ymao1

LGTM. Verified that the buggy behavior is fixed.

YulNaumenko

LGTM

This PR addresses some weird UX we've identified with default values in Action Params components and their inferred defaults when placed inside of an Alerts flyout. Key changes: 1. Typing of these components has been corrected to reflect that we expect these parameters to only be _partial_, as the form is used to set these values (for example, the `message` field of the Server Log action, might or might not be set, so it should be nullable, but in the typing we treated it as the _final_ valid state, which is message not being nullable). 2. When a default message is set by the params components, the are tracked against the value of the default, which means that if the default changes, then so will the value in the field. Custom values provided by the user will not be overridden when the default changes. This has to be handled by the component itself at the moment (hopefully in the future we can make this a concern of the flyout and not each component). 3. The concept of the "Recovered" action group has been removed from these components - that's an Alerting concern, not actions, and shouldn't appear in the action components' code. # Conflicts: # x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/server_log/server_log_params.test.tsx

* master: (25 commits) [Alerting] fixes buggy default message behaviour (elastic#84202) [Graph] Use new ES client and change license API (elastic#84398) [DOCS] Adds redirect to known plugins page (elastic#84001) Update IndexPatternSelect to get fields from indexPatternService instead of savedObject attributes (elastic#84376) Adding timestamps to created events so the sorting is stable (elastic#84515) [DOCS] Redirects for drilldown links (elastic#83846) [Fleet] Support for showing an Integration Detail Custom (UI Extension) tab (elastic#83805) [Enterprise Search] Migrate shared Schema components (elastic#84381) [Discover] Unskip date_nanos and shard links functional tests (elastic#82878) [APM] ML anomaly detection integration: Displaying anomaly job results in the Transaction duration chart is not as intended (elastic#84415) Support for painless language autocomplete within monaco (elastic#80577) [Lens] Time scale ui (elastic#83904) removing beta callouts (elastic#84510) [Lens] (Accessibility) add aria-label to chart type icon (elastic#84493) Trusted Apps signer API. (elastic#83661) increase stdout max listeners for legacy logging (elastic#84497) [APM] Service overview: Add throughput chart (elastic#84439) [Discover] Unskip main functional tests (elastic#84300) Uptime overview overhaul (elastic#83406) [APM] Adjust time formats based on the difference between start and end (elastic#84470) ...

* [Alerting] fixes buggy default message behaviour (#84202) This PR addresses some weird UX we've identified with default values in Action Params components and their inferred defaults when placed inside of an Alerts flyout. Key changes: 1. Typing of these components has been corrected to reflect that we expect these parameters to only be _partial_, as the form is used to set these values (for example, the `message` field of the Server Log action, might or might not be set, so it should be nullable, but in the typing we treated it as the _final_ valid state, which is message not being nullable). 2. When a default message is set by the params components, the are tracked against the value of the default, which means that if the default changes, then so will the value in the field. Custom values provided by the user will not be overridden when the default changes. This has to be handled by the component itself at the moment (hopefully in the future we can make this a concern of the flyout and not each component). 3. The concept of the "Recovered" action group has been removed from these components - that's an Alerting concern, not actions, and shouldn't appear in the action components' code. # Conflicts: # x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/server_log/server_log_params.test.tsx * Update server_log_params.test.tsx Removed unused var (caused by bad merge) Co-authored-by: Yuliia Naumenko <yuliia.naumenko@elastic.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

gmmorris added 4 commits November 24, 2020 12:23

fixed buggy default message behaviour

00f7ef3

added missing test

f9c7560

fixed typing

b1eb47b

gmmorris marked this pull request as ready for review November 26, 2020 11:41

gmmorris requested a review from a team as a code owner November 26, 2020 11:41

gmmorris added Feature:Alerting release_note:fix Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.11.0 v8.0.0 labels Nov 26, 2020

gmmorris requested a review from cnasikas November 26, 2020 12:03

support unspecified parent field on jira

c967f84

cnasikas approved these changes Nov 26, 2020

View reviewed changes

Merge branch 'master' into alerts/fix-buggy-default-message

031c902

ymao1 approved these changes Nov 30, 2020

View reviewed changes

YulNaumenko approved these changes Nov 30, 2020

View reviewed changes

gmmorris merged commit bc1fd6c into elastic:master Nov 30, 2020

gmmorris mentioned this pull request Nov 30, 2020

[7.x] [Alerting] fixes buggy default message behaviour (#84202) #84553

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Alerting] fixes buggy default message behaviour #84202

[Alerting] fixes buggy default message behaviour #84202

gmmorris commented Nov 24, 2020 •

edited

Loading

elasticmachine commented Nov 26, 2020

cnasikas left a comment

gmmorris commented Nov 30, 2020

kibanamachine commented Nov 30, 2020

ymao1 left a comment

YulNaumenko left a comment

[Alerting] fixes buggy default message behaviour #84202

[Alerting] fixes buggy default message behaviour #84202

Conversation

gmmorris commented Nov 24, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Nov 26, 2020

cnasikas left a comment

Choose a reason for hiding this comment

gmmorris commented Nov 30, 2020

kibanamachine commented Nov 30, 2020

💚 Build Succeeded

Metrics [docs]

Async chunks

History

ymao1 left a comment

Choose a reason for hiding this comment

YulNaumenko left a comment

Choose a reason for hiding this comment

gmmorris commented Nov 24, 2020 •

edited

Loading