[Security Solution][Detections] Fix rule creation/editing forms #130825

banderror · 2022-04-21T19:31:10Z

Addresses: #130767, #130770
Related to: #130771

Summary

A bug in the forms library presumably introduced in #130544 broke Rule Creation and Editing pages in Security Solution in the main branch. Cypress tests haven't been run in this PR, so it was merged which caused Cypress tests in main to start failing. @MadameSheema skipped those failing tests in #130771.

This PR:

Fixes the forms library by returning back the effect in the useForm hook which has been deleted in [Form lib] Add "updateFieldValues" to the form hook API #130544.
Fixes the way rule creation/editing forms use the library and handle the rule type selection. Please check the commits for details.
Recovers Cypress tests skipped in [Security Solution] Unblocks main #130771.

Changes in `src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts`

@elastic/platform-deployment-management @sebelga @yuliacech please review the commit Fix 1: get back the deleted effect in useForm.

In Security Solution, we have multiple forms on the same rule creation/editing page for every step of the creation/editing process. We show or hide the forms depending on which step is currently active, so only one of the forms is visible at the same time. Without the effect that was deleted previously in #130544, it seems the useForm hook doesn't work on the mentioned pages.

elasticmachine · 2022-04-21T19:31:13Z

Pinging @elastic/security-detections-response (Team:Detections and Resp)

sebelga

Sorry for breaking your form by making that change. 😞 I still think I'd like to keep the change in the form lib and not rely on controlling the form values by changing the defaultValue object

const { form } = useForm({
  defaultValue: { ... } // this object will always be new on re-render of the component
});

I am happy to zoom and have a look at your form to see why you rely on the defaultValue (which is normally only used to declare the initial values of the fields) to swap field values.

You can use either the reset() method (which will also reset the states like isSubmitted) or now the new updateFieldValues() to update the form when needed.

Ping me if you want to zoom and have a look 😊

MadameSheema

Approved from quality perspective:

Executed the Cypress tests on my local and worked fine
Did a bit of exploratory testing and everything is working as expected. The only issue I found is the following one: [Security Solution] Not able to create a rule after adding an action and then change the frequency to perform no actions during the rule creation #130869 that I don't consider a blocker for this PR since it is already happening on previous versions.

Lots of thanks for the quick fix!!

sebelga · 2022-05-02T08:14:19Z

x-pack/plugins/security_solution/public/detections/components/rules/query_bar/index.tsx

@@ -29,7 +29,7 @@ import * as i18n from './translations';
 export interface FieldValueQueryBar {
  filters: Filter[];
  query: Query;
-  saved_id?: string;


FYI there is an option you can pass to the form to not strip undefined value (https://docs.elastic.dev/form-lib/core/use-form#stripemptyfields)

sebelga · 2022-05-02T08:15:43Z

src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts

@@ -659,6 +659,15 @@ export function useForm<T extends FormData = FormData, I extends FormData = T>(
  // ----------------------------------
  // -- EFFECTS
  // ----------------------------------
+  useEffect(() => {


As mentioned in my previous comment, please don't put the useEffect back. We can discuss over Zoom how to solve your form without counting on this.

banderror · 2022-05-04T16:54:07Z

@sebelga thank you for your review. And sorry, I had to switch to another topic but will get back to this PR later, maybe next week or so. I think I'll need your help with this one so thank you for offering it 🙏 I'll ping you later, thanks again!

sebelga · 2022-05-23T10:00:11Z

You won't solve the jest failure

Jest Tests #1 / useForm() hook config.defaultValue should be updated with the UseField "defaultValue" prop

until you remove the useEffect you've put back in.

IMO you should first remove the change to the form lib before fixing the form. 😊

…ped recursively by the forms lib

…eference

kibana-ci · 2022-05-24T12:04:09Z

💔 Build Failed

Failed CI Steps

Test Failures

[job] [logs] Jest Tests #6 / EqlQueryBar sets the field value on input change
[job] [logs] Jest Tests #6 / useForm() hook config.defaultValue should be updated with the UseField "defaultValue" prop
[job] [logs] Jest Tests #2 / UserActionMarkdown useForm stale state bug creates a stale state if a key is not passed to the component

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	2974	2975	+1

Async chunks

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

id	before	after	diff
`securitySolution`	5.0MB	5.0MB	+448.0B

Page load bundle

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

id	before	after	diff
`esUiShared`	130.3KB	130.4KB	+58.0B

History

💔 Build #46811 failed 34ec0a1eaeed12e8213d1fd5d5e3114625c51be6
💔 Build #46676 failed f90719b60df5881cff371e57532a0ba5475334ea
💔 Build #46380 failed 5d5feaad865eb1781d547f85b97305a8fe42057a
💔 Build #46035 failed 227376c0ba0f3144cb4f21bfabc58c1f51038c85

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

cc @banderror

@banderror

**Addresses:** #130767, #130770 Related to [this PR](#130825). @banderror described the reason for the problems in his PR. There was [ask ](#130825 (review)) to not return the `useEffect` to the form lib. The fix of the form clears the values [in this commit](df0b7bb) Then I cherry-pick commits from @banderror PR to fix form stripping undefined values and also fixed some tests ### Checklist Delete any items that are not applicable to this PR. - [ ] [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

@banderror

**Addresses:** #130767, #130770 Related to [this PR](#130825). @banderror described the reason for the problems in his PR. There was [ask ](#130825 (review)) to not return the `useEffect` to the form lib. The fix of the form clears the values [in this commit](df0b7bb) Then I cherry-pick commits from @banderror PR to fix form stripping undefined values and also fixed some tests ### Checklist Delete any items that are not applicable to this PR. - [ ] [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 (cherry picked from commit 52bdf9a)

banderror · 2022-05-30T16:20:30Z

Closing this PR as @nkhristinin fixed the issues in #132834

@banderror

**Addresses:** elastic#130767, elastic#130770 Related to [this PR](elastic#130825). @banderror described the reason for the problems in his PR. There was [ask ](elastic#130825 (review)) to not return the `useEffect` to the form lib. The fix of the form clears the values [in this commit](elastic@df0b7bb) Then I cherry-pick commits from @banderror PR to fix form stripping undefined values and also fixed some tests ### Checklist Delete any items that are not applicable to this PR. - [ ] [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 (cherry picked from commit 52bdf9a)

@banderror

**Addresses:** #130767, #130770 Related to [this PR](#130825). @banderror described the reason for the problems in his PR. There was [ask ](#130825 (review)) to not return the `useEffect` to the form lib. The fix of the form clears the values [in this commit](df0b7bb) Then I cherry-pick commits from @banderror PR to fix form stripping undefined values and also fixed some tests ### Checklist Delete any items that are not applicable to this PR. - [ ] [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 (cherry picked from commit 52bdf9a) Co-authored-by: Khristinin Nikita <[email protected]>

banderror requested review from a team as code owners April 21, 2022 19:31

banderror self-assigned this Apr 21, 2022

banderror requested a review from MadameSheema April 21, 2022 19:32

patrykkopycinski requested a review from sebelga April 21, 2022 19:58

banderror force-pushed the fix-bug-on-rule-creation-and-editing-pages branch from 2cffc6e to 4dbb898 Compare April 21, 2022 20:38

banderror requested a review from a team as a code owner April 21, 2022 20:38

This was linked to issues Apr 21, 2022

[Security Solution] Define rule, About rule, Schedule rule and Rule actions steps wiped during rule creation #130767

Closed

[Security Solution] Some default values on IM rule creation are not present #130770

Closed

sebelga reviewed Apr 25, 2022

View reviewed changes

MadameSheema approved these changes Apr 25, 2022

View reviewed changes

banderror force-pushed the fix-bug-on-rule-creation-and-editing-pages branch from c3d73cc to 3dcef9a Compare April 28, 2022 16:04

sebelga reviewed May 2, 2022

View reviewed changes

banderror force-pushed the fix-bug-on-rule-creation-and-editing-pages branch 3 times, most recently from c7bf08c to 7bac4cf Compare May 12, 2022 10:29

banderror force-pushed the fix-bug-on-rule-creation-and-editing-pages branch from 7bac4cf to 5e738f3 Compare May 16, 2022 10:39

banderror force-pushed the fix-bug-on-rule-creation-and-editing-pages branch 3 times, most recently from ca9c0dd to 227376c Compare May 19, 2022 09:21

banderror mentioned this pull request May 19, 2022

[Security Solution] Some default values on IM rule creation are not present #130770

Closed

banderror force-pushed the fix-bug-on-rule-creation-and-editing-pages branch 3 times, most recently from f90719b to 34ec0a1 Compare May 23, 2022 09:53

banderror added 5 commits May 24, 2022 13:17

Fix 1: get back the deleted effect in useForm

55e62d6

Fix 2: dont use undefined in forms because undefined values get strip…

951c624

…ped recursively by the forms lib

Fix 3: dont compare actual value of a field with the default one by r…

c455ed8

…eference

Revive the skipped Cypress tests

e83c88d

Fix CI

28c998a

banderror force-pushed the fix-bug-on-rule-creation-and-editing-pages branch from 34ec0a1 to 28c998a Compare May 24, 2022 11:17

nkhristinin mentioned this pull request May 25, 2022

Security form fix #132834

Merged

1 task

This was unlinked from issues May 30, 2022

[Security Solution] Define rule, About rule, Schedule rule and Rule actions steps wiped during rule creation #130767

Closed

[Security Solution] Some default values on IM rule creation are not present #130770

Closed

banderror closed this May 30, 2022

banderror deleted the fix-bug-on-rule-creation-and-editing-pages branch May 30, 2022 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Detections] Fix rule creation/editing forms #130825

[Security Solution][Detections] Fix rule creation/editing forms #130825

banderror commented Apr 21, 2022

elasticmachine commented Apr 21, 2022

sebelga left a comment

MadameSheema left a comment

sebelga May 2, 2022

sebelga May 2, 2022

banderror commented May 4, 2022

sebelga commented May 23, 2022 •

edited

Loading

kibana-ci commented May 24, 2022 •

edited

Loading

banderror commented May 30, 2022

[Security Solution][Detections] Fix rule creation/editing forms #130825

[Security Solution][Detections] Fix rule creation/editing forms #130825

Conversation

banderror commented Apr 21, 2022

Summary

Changes in src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts

elasticmachine commented Apr 21, 2022

sebelga left a comment

Choose a reason for hiding this comment

MadameSheema left a comment

Choose a reason for hiding this comment

sebelga May 2, 2022

Choose a reason for hiding this comment

sebelga May 2, 2022

Choose a reason for hiding this comment

banderror commented May 4, 2022

sebelga commented May 23, 2022 • edited Loading

kibana-ci commented May 24, 2022 • edited Loading

💔 Build Failed

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

banderror commented May 30, 2022

Changes in `src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts`

sebelga commented May 23, 2022 •

edited

Loading

kibana-ci commented May 24, 2022 •

edited

Loading