Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix validation for entry fields in exception form #151654

Merged
merged 5 commits into from Feb 21, 2023
Merged

Fix validation for entry fields in exception form #151654

merged 5 commits into from Feb 21, 2023

Conversation

nkhristinin
Copy link
Contributor

@nkhristinin nkhristinin commented Feb 20, 2023 •
edited by kibanamachine

Change validation logic for entry exception field.

Close: #143051

Previously we didn't keep a validation state per field which caused a reset of validation if we still had invalid fields. Or we can have an invalid state for the form, but we removed the invalid field. You can see the videos on the ticket above.

Solution:

Keep validation state per field, like:

{
   [entry.id]: true,
}

This state can keep old fields, which already were removed, this is why we use the selector to get the actual amount of errors.

Screen.Recording.2023-02-21.at.12.49.13.mov

@nkhristinin nkhristinin added release_note:fix backport:prev-minor Backport to the previous minor version (i.e. one version back from main) Team:Security Solution Platform Security Solution Platform Team labels Feb 21, 2023
@nkhristinin nkhristinin changed the title Refactor validation logic Fix validation for entry fields in exception form Feb 21, 2023
@nkhristinin nkhristinin marked this pull request as ready for review February 21, 2023 11:52
@nkhristinin nkhristinin requested a review from a team as a code owner February 21, 2023 11:52
@nkhristinin
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lists 253 254 +1

Async chunks

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

id before after diff
lists 152.5KB 152.7KB +162.0B
securitySolution 13.7MB 13.7MB +168.0B
total +330.0B

History

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

dhurley14
dhurley14 approved these changes Feb 21, 2023
View reviewed changes
Copy link
Contributor

@dhurley14 dhurley14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I think opening a kibana issue with the comments you added here to the fix would be great too. Thanks for the fix.

packages/kbn-securitysolution-autocomplete/src/field_value_match/index.tsx
Comment on lines +217 to +220
// Looks like selectedField return new object every time when we for example add "and" entry
// that's why we need to check for name and type here
// Probably we should use some kind of memoization on parent components for entries
}, [selectedField?.name, selectedField?.type, selectedValue, handleSpacesWarning, onError]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These comments are really insightful thanks for finding the root cause of this. Would it be possible for you to add these comments to an issue? This way we can try to add it to the next release to-do list?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, will create an issue. This comment fixes one of the reasons of the validation problem, but main cause it's how we stored our errors before

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the issue: #151735

@nkhristinin nkhristinin merged commit d93eaa0 into elastic:main Feb 21, 2023
@kibanamachine kibanamachine mentioned this pull request Feb 21, 2023
Merged
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 21, 2023
@nkhristinin
Fix validation for entry fields in exception form (elastic#151654)
70cabfe 
## Change validation logic for entry exception field.

Close:
[elastic#143051

Previously we didn't keep a validation state per field which caused a
reset of validation if we still had invalid fields. Or we can have an
invalid state for the form, but we removed the invalid field. You can
see the videos on the ticket above.

## Solution:
Keep validation state per field, like:
```js
{
   [entry.id]: true,
}
```
This state can keep old fields, which already were removed, this is why
we use the selector to get the actual amount of errors.

https://user-images.githubusercontent.com/7609147/220337447-95c1558c-aa85-43d1-87e8-76370aeaf141.mov

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit d93eaa0)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.7

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

@nkhristinin nkhristinin mentioned this pull request Feb 21, 2023
Open
kibanamachine added a commit that referenced this pull request Feb 21, 2023
@kibanamachine @nkhristinin
[8.7] Fix validation for entry fields in exception form (#151654) (#1…
dbecc74 
…51732)

# Backport

This will backport the following commits from `main` to `8.7`:
- [Fix validation for entry fields in exception form
(#151654)](#151654)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Khristinin
Nikita","email":"nikita.khristinin@elastic.co"},"sourceCommit":{"committedDate":"2023-02-21T15:59:54Z","message":"Fix
validation for entry fields in exception form (#151654)\n\n## Change
validation logic for entry exception
field.\r\n\r\nClose:\r\n[#143051
we didn't keep a validation state per field which caused a\r\nreset of
validation if we still had invalid fields. Or we can have an\r\ninvalid
state for the form, but we removed the invalid field. You can\r\nsee the
videos on the ticket above.\r\n\r\n## Solution:\r\nKeep validation state
per field, like:\r\n```js \r\n{\r\n [entry.id]:
true,\r\n}\r\n```\r\nThis state can keep old fields, which already were
removed, this is why\r\nwe use the selector to get the actual amount of
errors.\r\n\r\n\r\n\r\nhttps://user-images.githubusercontent.com/7609147/220337447-95c1558c-aa85-43d1-87e8-76370aeaf141.mov\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<42973632+kibanamachine@users.noreply.github.com>","sha":"d93eaa010935518c45fbab1f8d57542f1ac1a83a","branchLabelMapping":{"^v8.8.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:Security
Solution
Platform","backport:prev-minor","v8.8.0"],"number":151654,"url":"#151654
validation for entry fields in exception form (#151654)\n\n## Change
validation logic for entry exception
field.\r\n\r\nClose:\r\n[#143051
we didn't keep a validation state per field which caused a\r\nreset of
validation if we still had invalid fields. Or we can have an\r\ninvalid
state for the form, but we removed the invalid field. You can\r\nsee the
videos on the ticket above.\r\n\r\n## Solution:\r\nKeep validation state
per field, like:\r\n```js \r\n{\r\n [entry.id]:
true,\r\n}\r\n```\r\nThis state can keep old fields, which already were
removed, this is why\r\nwe use the selector to get the actual amount of
errors.\r\n\r\n\r\n\r\nhttps://user-images.githubusercontent.com/7609147/220337447-95c1558c-aa85-43d1-87e8-76370aeaf141.mov\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<42973632+kibanamachine@users.noreply.github.com>","sha":"d93eaa010935518c45fbab1f8d57542f1ac1a83a"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.8.0","labelRegex":"^v8.8.0$","isSourceBranch":true,"state":"MERGED","url":"#151654
validation for entry fields in exception form (#151654)\n\n## Change
validation logic for entry exception
field.\r\n\r\nClose:\r\n[#143051
we didn't keep a validation state per field which caused a\r\nreset of
validation if we still had invalid fields. Or we can have an\r\ninvalid
state for the form, but we removed the invalid field. You can\r\nsee the
videos on the ticket above.\r\n\r\n## Solution:\r\nKeep validation state
per field, like:\r\n```js \r\n{\r\n [entry.id]:
true,\r\n}\r\n```\r\nThis state can keep old fields, which already were
removed, this is why\r\nwe use the selector to get the actual amount of
errors.\r\n\r\n\r\n\r\nhttps://user-images.githubusercontent.com/7609147/220337447-95c1558c-aa85-43d1-87e8-76370aeaf141.mov\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<42973632+kibanamachine@users.noreply.github.com>","sha":"d93eaa010935518c45fbab1f8d57542f1ac1a83a"}}]}]
BACKPORT-->

Co-authored-by: Khristinin Nikita <nikita.khristinin@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhurley14 dhurley14 dhurley14 approved these changes

Assignees
No one assigned
Labels
backport:prev-minor Backport to the previous minor version (i.e. one version back from main) release_note:fix Team:Security Solution Platform Security Solution Platform Team v8.7.0 v8.8.0
Projects
None yet
Milestone
No milestone
4 participants
@nkhristinin @kibana-ci @kibanamachine @dhurley14