[Alerting UI] Alert and Connector flyouts Save and Save&Test buttons should be active by default. by YulNaumenko · Pull Request #86708 · elastic/kibana

YulNaumenko · 2020-12-21T22:27:32Z

Current PR is making Save and Save&Test buttons always available:

Alert Create Flyout. With proper validation triggered on the click Save:

Alert Edit Flyout

Connector Create

Connector Edit

Connector Modal

Resolve #84091

…ve by default.

elasticmachine · 2020-12-22T20:11:29Z

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

ymao1

Initial review looks good! Very clear what the errors are now :) Left a question about whether we should be clearing invalid input on Save vs leaving it in the form for user to fix.

ymao1 · 2020-12-23T13:49:54Z

x-pack/plugins/triggers_actions_ui/public/application/lib/value_validators.ts

  }
 };
+
+export function getConnectorWithInvalidatedFields(


Can we add unit tests for these new functions?

ymao1 · 2020-12-23T14:19:55Z

x-pack/plugins/triggers_actions_ui/public/application/lib/value_validators.ts

+  configErrors: IErrorObject,
+  secretsErrors: IErrorObject,
+  baseConnectorErrors: IErrorObject
+) {


If I create an email connector and enter an invalid Sender field, I get immediate feedback on the form that "Sender is not a valid email address." but then if I press Save, my (invalid) entry is cleared and the error just says "Sender is required.". Would it be better to keep the user input so the user can fix it?

Great catch! I will fix it.

ymao1

LGTM!

igoristic

Monitoring looks good 👍

gmmorris · 2020-12-31T10:25:28Z

Alert trigger is required.

I'm wondering about the wording here... I don't think its right as it isn't quite clear what trigger means...
(I know this wasn't added in this PR, but this PR does make it more likely users will see it. :) )

The user has a list of Alert types... and we want them to choose one...
But I'm not sure Alert type is any clearer than Alert trigger... 🤷

@gchaps any thoughts?

gmmorris · 2020-12-31T10:28:37Z

We don't seem to show any error when the user chooses an action that has no connector (see below).

We don't allow the user to save, but we don't show a validation error either.

gmmorris · 2020-12-31T10:41:02Z

...gers_actions_ui/public/application/components/builtin_action_types/email/email_connector.tsx

            )}
            disabled={readOnly}
-            checked={hasAuth}
+            checked={hasAuth || false}


hasAuth is boolean, so I can't figure out why || false is needed.... 🤔
If it can be undefined then something seems to be wrong in the typing 😬

In the current implementation, the initial alert object has all user input fields as undefined to be able to not trigger validation on the form opening (each input field has a check on undefined to not set a validation error in this case). When onblur event on the field or click save we are triggering invalidating the values - basically setting it to null instead of undefined.
Do you think we should consider another approach for implementing UI inputs validation triggering?

gmmorris

Left a few typing problems that need sorting but nothing major.
Behaviour wise all looks good except for the missing error when an action is selected but no connector is configured, so happy to approve assuming this is sorted :)

👍

gmmorris · 2020-12-31T11:25:09Z

x-pack/plugins/triggers_actions_ui/public/application/sections/alert_form/alert_form.tsx

+  const alertParamsErrors = (alertTypeModel
+    ? alertTypeModel.validate(alert.params).errors
+    : []) as IErrorObject;


This looks wrong 🤔
the type signature for IErrorObject can't be []... this is probably passing because of the explicit as casting.
It's best to avoid this sort of casting as it can hide errors... better to do this:

const alertParamsErrors: IErrorObject = ....

stephmilovic · 2021-01-05T16:51:50Z

...ggers_actions_ui/public/application/components/builtin_action_types/jira/jira_connectors.tsx

  const { apiUrl, projectKey } = action.config;

-  const isApiUrlInvalid: boolean = errors.apiUrl.length > 0 && apiUrl != null;
+  const isApiUrlInvalid: boolean = errors.apiUrl.length > 0 && apiUrl !== undefined;


Same thing as != null except now you are allowing null as a valid apiUrl. Why?

I've added undefined as a value to compare to skip the input validation on the form initialization (before the user did some input or clicked Save). If it is null currently, this is expected an invalid user input trigger and invalid fields will be highlighted. Do you have any issues with this?

that sound good to me, just wondering the reasoning. thank you!

stephmilovic · 2021-01-05T16:53:21Z

...ons_ui/public/application/components/builtin_action_types/resilient/resilient_connectors.tsx

 }) => {
  const { apiUrl, orgId } = action.config;
-  const isApiUrlInvalid: boolean = errors.apiUrl.length > 0 && apiUrl != null;
+  const isApiUrlInvalid: boolean = errors.apiUrl.length > 0 && apiUrl !== undefined;


same question here

stephmilovic · 2021-01-05T16:55:21Z

..._ui/public/application/components/builtin_action_types/servicenow/servicenow_params.test.tsx

  actionParams,
-  errors: { short_description: [] },
+  // eslint-disable-next-line @typescript-eslint/naming-convention
+  errors: { 'subActionParams.incident.short_description': [] },


if you write it like this, there is no typescript error:

errors: { ['subActionParams.incident.short_description']: [] },

stephmilovic · 2021-01-05T17:32:28Z

...actions_ui/public/application/components/builtin_action_types/servicenow/servicenow.test.tsx

      errors: {
-        short_description: ['Short description is required.'],
+        // eslint-disable-next-line @typescript-eslint/naming-convention
+        'subActionParams.incident.short_description': ['Short description is required.'],


whoops, get it here too ;)

stephmilovic

Thanks for making those quick TS changes. Looks good from the SIEM perspective. Thank you! LGTM 🚀

kibanamachine · 2021-01-05T19:48:42Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 546f61d

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
`stackAlerts`	114.3KB	114.3KB	+4.0B
`triggersActionsUi`	1.6MB	1.5MB	-21.5KB
total			-21.5KB

Distributable file count

id	before	after	diff
`default`	47252	48015	+763

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`securitySolution`	246.7KB	246.7KB	+29.0B
`triggersActionsUi`	162.3KB	165.1KB	+2.8KB
total			+2.8KB

Unknown metric groups

async chunk count

id	before	after	diff
`triggersActionsUi`	31	32	+1

History

💚 Build #97036 succeeded 2eb2b3e
💚 Build #96857 succeeded 1bec31d
💔 Build #96838 failed 64526de
💔 Build #96774 failed fddf0fe
💔 Build #96181 failed e01d1e6

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

…should be active by default. (elastic#86708) * Alert and Connector flyouts Save and Save&Test buttons should be active by default. * fixed typechecks * fixed typechecks * refactored repeted code * fixed typechecks * fixed typechecks * fixed typechecks * fixed due to comments * fixed failing tests * fixed due to comments * fixed due to comments * fixed due to comments * fixed typescript checks

…should be active by default. (#86708) (#87380) * Alert and Connector flyouts Save and Save&Test buttons should be active by default. * fixed typechecks * fixed typechecks * refactored repeted code * fixed typechecks * fixed typechecks * fixed typechecks * fixed due to comments * fixed failing tests * fixed due to comments * fixed due to comments * fixed due to comments * fixed typescript checks

Alert and Connector flyouts Save and Save&Test buttons should be acti…

7a02b30

…ve by default.

YulNaumenko added Feature:Alerting v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Actions Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// v7.12.0 labels Dec 21, 2020

YulNaumenko self-assigned this Dec 21, 2020

YulNaumenko changed the title ~~Alert and Connector flyouts Save and Save&Test buttons should be active by default.~~ [Alerting UI] Alert and Connector flyouts Save and Save&Test buttons should be active by default. Dec 21, 2020

YulNaumenko added 3 commits December 21, 2020 17:32

fixed typechecks

0e1dcdf

Merge remote-tracking branch upstream/master

0dceb43

fixed typechecks

b3e89ff

YulNaumenko marked this pull request as ready for review December 22, 2020 20:11

YulNaumenko requested review from a team as code owners December 22, 2020 20:11

YulNaumenko added 4 commits December 22, 2020 12:52

refactored repeted code

7a280f4

fixed typechecks

aa4b8dd

fixed typechecks

46cc2c8

fixed typechecks

956393b

YulNaumenko requested a review from a team December 23, 2020 00:53

ymao1 reviewed Dec 23, 2020

View reviewed changes

fixed due to comments

e01d1e6

YulNaumenko requested a review from ymao1 December 23, 2020 16:07

ymao1 approved these changes Dec 23, 2020

View reviewed changes

igoristic approved these changes Dec 23, 2020

View reviewed changes

gmmorris reviewed Dec 31, 2020

View reviewed changes

gmmorris approved these changes Dec 31, 2020

View reviewed changes

YulNaumenko added 4 commits January 4, 2021 12:16

Merge remote-tracking branch upstream/master

7846ebf

fixed failing tests

fddf0fe

fixed due to comments

64526de

Merge remote-tracking branch upstream/master

1bec31d

stephmilovic reviewed Jan 5, 2021

View reviewed changes

YulNaumenko added 2 commits January 5, 2021 09:18

fixed due to comments

5dacd3f

fixed due to comments

2eb2b3e

YulNaumenko requested a review from stephmilovic January 5, 2021 17:21

stephmilovic reviewed Jan 5, 2021

View reviewed changes

stephmilovic approved these changes Jan 5, 2021

View reviewed changes

fixed typescript checks

546f61d

YulNaumenko merged commit dddd8e4 into elastic:master Jan 5, 2021

YulNaumenko mentioned this pull request Jan 5, 2021

[7.x] [Alerting UI] Alert and Connector flyouts Save and Save&Test buttons should be active by default. (#86708) #87380

Merged

mikecote added release_note:enhancement and removed release_note:skip Skip the PR/issue when compiling release notes labels Feb 2, 2021

Conversation

YulNaumenko commented Dec 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Dec 22, 2020

Uh oh!

ymao1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ymao1 left a comment

Choose a reason for hiding this comment

Uh oh!

igoristic left a comment

Choose a reason for hiding this comment

Uh oh!

gmmorris commented Dec 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmmorris commented Dec 31, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmmorris left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YulNaumenko Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephmilovic left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jan 5, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

Distributable file count

Page load bundle

async chunk count

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

YulNaumenko commented Dec 21, 2020 •

edited

Loading

gmmorris commented Dec 31, 2020 •

edited

Loading

YulNaumenko Jan 5, 2021 •

edited

Loading