[Security Solution][Detections] - Fix query preview bugs + tests #79947

yctercero · 2020-10-07T22:29:41Z

Summary

Fixes the following in the rule query preview feature:

toasters were previously just showing an error message, not the details. Updated to use addError so now user can click to view details
disables eql query preview button while request is loading
moved to use existing search strategy pattern, this resolved the react memory leak error encountered when moving away from page before preview resolved
removes super descriptive someAriaId
adds tests 🎺 🎉

Checklist

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

yctercero · 2020-10-07T23:15:50Z

x-pack/plugins/security_solution/public/common/containers/matrix_histogram/index.ts

@XavierM please let me know if this is what you were thinking.

Yes but the const bucketEmpty need to go on line 34 ;)

yctercero · 2020-10-07T23:16:56Z

x-pack/plugins/security_solution/public/common/containers/matrix_histogram/index.ts

@patrykkopycinski @angorayc @XavierM Is this change here ok? addDanger only shows an error message, while addError allows the user to click to view full error details.

we might need to change it everywhere for consistency, but we can take care of that. Thank you!

yctercero · 2020-10-07T23:18:17Z

x-pack/plugins/security_solution/public/common/hooks/eql/helpers.ts

There's an existing bug with warnings, working on the fix in a separate PR to keep these small-ish.

yctercero · 2020-10-07T23:21:04Z

x-pack/plugins/security_solution/public/common/hooks/eql/use_eql_preview.test.ts

@patrykkopycinski I was able to test this hook more thoroughly, and was going to do the same for useMatrixHistogram but required one small change of first declaring the variable let subscription$ and then assigning it. Otherwise, the tests failed saying the variable was unreachable. If this seems like an ok change, happy to add similar tests to useMatrixHistogram.

madirey · 2020-10-08T18:18:17Z

x-pack/plugins/security_solution/public/common/hooks/eql/helpers.test.ts

rylnd

Looking good so far! Just a few minor things. Big remaining one is the syncing of validity to the preview button; I hit that a bunch in testing this.

rylnd · 2020-10-08T21:54:57Z

...s/security_solution/public/detections/components/rules/query_preview/threshold_histogram.tsx

This seems a bit strange to me. Is one of the comparisons meant to be a type check? If they're both numbers I think the latter check should be sufficient

rylnd · 2020-10-08T22:04:15Z

x-pack/plugins/security_solution/public/common/hooks/eql/use_eql_preview.ts

I'm not sure if it's the implementation here or something else, but I see a behavioral difference between the EQL preview loading and the other query previews loading. With EQL it does not populate the histogram with the loader:

Yea, I wanted to try to keep the PRs smallish - so I focused on fixing the bugs/tests with the api here and have a sequel "preview_buggies_2" that addresses these issues (#80110). I promise it's a really good sequel 😄

It currently includes the changes from this PR, but if you check out the last 2 commits you can see the new changes.

…tegy

yctercero · 2020-10-12T18:10:55Z

Had been trying to have smaller PRs, but going to just address the comments from here in this new PR that tackles all known bugs at once - #80110

kibanamachine · 2020-10-12T18:19:12Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 552b41f

Metrics [docs]

async chunks size

id	before	after	diff
`securitySolution`	10.6MB	10.6MB	+3.8KB

page load bundle size

id	before	after	diff
`securitySolution`	593.2KB	600.5KB	+7.2KB

History

💚 Build #80634 succeeded 32d5421247ea2e509b4950219f1e024b0bfecc10

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

yctercero self-assigned this Oct 7, 2020

yctercero added Feature:Detection Rules Security Solution rules and Detection Engine release_note:fix Team:Detections and Resp Security Detection Response Team v7.10.0 v7.11.0 v8.0.0 labels Oct 7, 2020

yctercero mentioned this pull request Oct 7, 2020

[Security Solution][Detections] - Rule creation query preview #78985

Merged

9 tasks

yctercero marked this pull request as ready for review October 7, 2020 23:06

yctercero requested review from a team as code owners October 7, 2020 23:06

yctercero commented Oct 7, 2020

View reviewed changes

madirey reviewed Oct 8, 2020

View reviewed changes

x-pack/plugins/security_solution/public/common/hooks/eql/helpers.test.ts Outdated

Copy link

Contributor

madirey Oct 8, 2020

Choose a reason for hiding this comment

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

👍

rylnd reviewed Oct 8, 2020

View reviewed changes

yctercero removed the Feature:Detection Rules Security Solution rules and Detection Engine label Oct 10, 2020

yctercero added 7 commits October 12, 2020 12:40

typed buildquerfilter to remove some icky casting

f7fec5d

removed unnecessary types

50ec6b0

fixed subscription bug with way i had written call to eql search stra…

3a836b1

…tegy

moved to use adError toast to include error details

646dfb7

fixed subscribe

182e72f

remove infamous someAriaId

282c344

remove something

552b41f

yctercero force-pushed the preview_buggies branch from 32d5421 to 552b41f Compare October 12, 2020 16:40

yctercero closed this Oct 12, 2020

yctercero deleted the preview_buggies branch October 14, 2020 11:58

[Security Solution][Detections] - Fix query preview bugs + tests #79947

[Security Solution][Detections] - Fix query preview bugs + tests #79947

Uh oh!

Conversation

yctercero commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

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!

Choose a reason for hiding this comment

Uh oh!

rylnd left a comment

Choose a reason for hiding this comment

Uh oh!

rylnd Oct 8, 2020 • 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!

yctercero commented Oct 12, 2020

Uh oh!

kibanamachine commented Oct 12, 2020

💚 Build Succeeded

Metrics [docs]

async chunks size

page load bundle size

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yctercero commented Oct 7, 2020 •

edited

Loading

rylnd Oct 8, 2020 •

edited

Loading