[Security Sollution][Alerts] fixes rule preview issue for new terms field #145707
Conversation
vitaliidm
added
bug
…italiidm/kibana into alerts/preview-new-terms-issue
|
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
vitaliidm
added
the
backport:prev-minor
| // more details here: https://github.com/elastic/kibana/issues/144322#issuecomment-1321838136 | ||
| // wrapping in setTimeout is a workaround until solution within forms-lib can be found | ||
| const isValid = await new Promise<boolean>((resolve) => { | ||
| setTimeout(async () => { |
There was a problem hiding this comment.
The fix looks dirty but fine if it's temporary.
I just wanted to share my thoughts. Usually a necessity of setTimeout() usage says about design flaws and over-complication. In this case it looks possible to get rid of the problem by rethinking the approach to the data handling. For example I can see formHooks is used as a global variable and the value is reused the level up.
The cause of the problem is double validation which actually shouldn't happen.
I'm curious if an approach to redesign the components to handle validation in another way was considered as well.
There was a problem hiding this comment.
The fix looks dirty but fine if it's temporary.
I would recommend to read this section before writing comments like this.
I just wanted to share my thoughts. Usually a necessity of setTimeout() usage says about design flaws and over-complication. In this case it looks possible to get rid of the problem by rethinking the approach to the data handling. For example I can see formHooks is used as a global variable and the value is reused the level up.
#144322 (comment) is double validation which actually shouldn't happen.
What do you mean by 'double validation'?
I would expect once data in form is changed and available in onChange callback, it can be checked whether it's valid or not. But if you look into standalone example, it's not a case: data is changed, but its validity can not be established.
I would like to see forms-lib assessment of library behaviour before committing to any refactoring.
There was a problem hiding this comment.
Thank you for the explanation and pointing to the guidelines.
I didn't mean to offend but rather say that setTimeout usually is a red flag which signals there is something wrong.
You are right the problem in the the forms-lib. Though it's strange the popular scenario of accessing the validity state in the onChange callback wasn't covered already.
There was a problem hiding this comment.
rather say that setTimeout usually is a red flag which signals there is something wrong.
yes, I also commented that it's a workaround and put together ticket and links to it
// wrapping in setTimeout is a workaround until solution within forms-lib can be found
You are right, the problem is in the forms-lib. Though it's strange the popular scenario of accessing the validity state in the onChange callback wasn't covered already.
In this standalone example , it can be seen that in onChange, accessed validity is not correct. Very simple case, but the result is rather unexpected. Let's see what forms-lib will say on that.
I would expect if I get any data from callback: to either access validation status straight away or through running validate method. We did the latter in form(though through calling a submit before this change) and relied on its results. Which, as it turns out, are not correct in some cases.
💚 Build Succeeded
Metrics [docs]Async chunks
Unknown metric groupsESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @vitaliidm |
…ield (elastic#145707) ## Summary - fixes elastic#144322 - details on underlying [issue](elastic#144322 (comment)) within form-lib ### Before https://user-images.githubusercontent.com/92328789/202687215-e9606bd0-5cfd-4a92-9abf-edaf90868505.mov ### After https://user-images.githubusercontent.com/92328789/202688418-7cb7d250-02f3-4020-bfa0-65191b8a529b.mov (cherry picked from commit c086220)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…erms field (#145707) (#146449) # Backport This will backport the following commits from `main` to `8.6`: - [[Security Sollution][Alerts] fixes rule preview issue for new terms field (#145707)](#145707) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Vitalii Dmyterko","email":"92328789+vitaliidm@users.noreply.github.com"},"sourceCommit":{"committedDate":"2022-11-28T17:36:40Z","message":"[Security Sollution][Alerts] fixes rule preview issue for new terms field (#145707)\n\n## Summary\r\n\r\n- fixes https://github.com/elastic/kibana/issues/144322\r\n- details on underlying\r\n[issue](https://github.com/elastic/kibana/issues/144322#issuecomment-1321838136)\r\nwithin form-lib\r\n\r\n### Before\r\n\r\n\r\nhttps://user-images.githubusercontent.com/92328789/202687215-e9606bd0-5cfd-4a92-9abf-edaf90868505.mov\r\n\r\n### After\r\n\r\n\r\nhttps://user-images.githubusercontent.com/92328789/202688418-7cb7d250-02f3-4020-bfa0-65191b8a529b.mov","sha":"c086220f1ba89c9db0fe2c7500d86e3375aeee86","branchLabelMapping":{"^v8.7.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:fix","Team:Detections and Resp","Team: SecuritySolution","Team:Detection Alerts","backport:prev-minor","v8.6.0","v8.7.0"],"number":145707,"url":"https://github.com/elastic/kibana/pull/145707","mergeCommit":{"message":"[Security Sollution][Alerts] fixes rule preview issue for new terms field (#145707)\n\n## Summary\r\n\r\n- fixes https://github.com/elastic/kibana/issues/144322\r\n- details on underlying\r\n[issue](https://github.com/elastic/kibana/issues/144322#issuecomment-1321838136)\r\nwithin form-lib\r\n\r\n### Before\r\n\r\n\r\nhttps://user-images.githubusercontent.com/92328789/202687215-e9606bd0-5cfd-4a92-9abf-edaf90868505.mov\r\n\r\n### After\r\n\r\n\r\nhttps://user-images.githubusercontent.com/92328789/202688418-7cb7d250-02f3-4020-bfa0-65191b8a529b.mov","sha":"c086220f1ba89c9db0fe2c7500d86e3375aeee86"}},"sourceBranch":"main","suggestedTargetBranches":["8.6"],"targetPullRequestStates":[{"branch":"8.6","label":"v8.6.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.7.0","labelRegex":"^v8.7.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/145707","number":145707,"mergeCommit":{"message":"[Security Sollution][Alerts] fixes rule preview issue for new terms field (#145707)\n\n## Summary\r\n\r\n- fixes https://github.com/elastic/kibana/issues/144322\r\n- details on underlying\r\n[issue](https://github.com/elastic/kibana/issues/144322#issuecomment-1321838136)\r\nwithin form-lib\r\n\r\n### Before\r\n\r\n\r\nhttps://user-images.githubusercontent.com/92328789/202687215-e9606bd0-5cfd-4a92-9abf-edaf90868505.mov\r\n\r\n### After\r\n\r\n\r\nhttps://user-images.githubusercontent.com/92328789/202688418-7cb7d250-02f3-4020-bfa0-65191b8a529b.mov","sha":"c086220f1ba89c9db0fe2c7500d86e3375aeee86"}}]}] BACKPORT--> Co-authored-by: Vitalii Dmyterko <92328789+vitaliidm@users.noreply.github.com>
Summary
Before
Screen.Recording.2022-11-01.at.13.00.46.mov
After
Screen.Recording.2022-11-18.at.10.50.31.mov