[SECURITY] Bug fix for topN on draggables by XavierM · Pull Request #70450 · elastic/kibana

XavierM · 2020-07-01T14:11:46Z

Summary

We lost some features on the topN due to the introduction of the global timeline context + we were doing some branching logic on i18n field.

Now the branch logic is done on the timelineID and we will only get the timelineId when users hover the TopN icon just before they click on it. And applied the same logic for the filter management.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2020-07-01T14:11:49Z

Pinging @elastic/siem (Team:SIEM)

andrew-goldstein · 2020-07-01T16:47:24Z

x-pack/plugins/security_solution/public/common/components/top_n/top_n.tsx

  const [view, setView] = useState<EventType>(defaultView);
  const onViewSelected = useCallback((value: string) => setView(value as EventType), [setView]);

+  useEffect(() => {


thanks for adding this to handle the scenario where a user changes the Timeline events filter while the graph is already open! 🙏

XavierM · 2020-07-01T17:10:08Z

@elasticmachine merge upstream

...ity_solution/public/common/components/drag_and_drop/draggable_wrapper_hover_content.test.tsx

x-pack/plugins/security_solution/public/timelines/components/timeline/body/index.tsx

...security_solution/public/common/components/drag_and_drop/draggable_wrapper_hover_content.tsx

stephmilovic

code review looks good, just had the one question on that test. pulled and manually tested a variety of timelines and all the filter managers are working as expected. hopefully this is the last bug! LGTM 👍

x-pack/plugins/security_solution/public/common/components/drag_and_drop/draggable_wrapper.tsx

x-pack/plugins/security_solution/public/timelines/components/manage_timeline/index.tsx

andrew-goldstein · 2020-07-02T15:32:13Z

x-pack/plugins/security_solution/public/common/components/with_hover_actions/index.tsx

-      return (
+    useEffect(() => {
+      setShowHoverContent(false);
+    }, [closePopOverTrigger]);


consider declaring setShowHoverContent as a dependency

there is no need because it is from useState

here the explanation -> https://reactjs.org/docs/hooks-faq.html#what-can-i-do-if-my-effect-dependencies-change-too-often

andrew-goldstein · 2020-07-02T22:52:35Z

x-pack/plugins/security_solution/public/common/lib/keury/index.ts


 // See the Keyword rule in kuery.peg
-const escapeAndOr = (val: string) => val.replace(/(\s+)(and|or)(\s+)/gi, '$1\\$2$3');
+// I do not think that we need that anymore since we are doing a full match_phrase all the time now => return `"${escapeKuery(val)}"`;


I did some desk testing around this and agree 🙏
consider removing this meta-comment, and the commented-out definitions below 🙏

kibanamachine · 2020-07-03T04:08:13Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: c5771b6

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
securitySolution	791	+1	790

History

💚 Build #58529 succeeded d25a6d3
💚 Build #58499 succeeded 2925543
💔 Build #58447 failed 3ca1eb7a4d7deef67334cb99f77e655d495f4c4c
💔 Build #58332 failed ff31a46
💚 Build #58127 succeeded e2df588

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

andrew-goldstein

Thanks for the fixes, refactoring, and ❤️ in this PR @XavierM! 🙏🙏🙏
I desk tested the Top N and Filter for / out context menu actions in Overview, Alerts, Events, Timeline, plus sub-contexts, including the Field and Value of expanded events, and in the Customize Columns view.
LGTM 🚀

* back to normal * add unit test * hover issue + indexToAdd issue * fix unit test * review II * fix bug + review * simplification * do not update state when component is unmounted * fix hover action on field name Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* back to normal * add unit test * hover issue + indexToAdd issue * fix unit test * review II * fix bug + review * simplification * do not update state when component is unmounted * fix hover action on field name Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* master: [APM-UI] e2e speed up build (elastic#70704) skip flaky suite (elastic#70764) skip flaky suite (elastic#70762) [Security Solution][Endpoint] Update to new manifest format (without compression) (elastic#70752) [functional tests] test url field formatter on dashboard and discover (elastic#70736) logout from transform_poweruser user in after method of transform tests (elastic#70644) [SECURITY] Bug fix for topN on draggables (elastic#70450) [Logs UI] Reorganise log rate anomaly table (elastic#69516) Update dependency @elastic/charts to v19.7.0 (elastic#69791) Add googlecloud metricbeat module to Kibana Home (elastic#70652) [Logs UI] Logs overview queries for the observability dashboard (elastic#70413) [Lens] Fitting functions (elastic#69820)

* master: [APM-UI] e2e speed up build (elastic#70704) skip flaky suite (elastic#70764) skip flaky suite (elastic#70762) [Security Solution][Endpoint] Update to new manifest format (without compression) (elastic#70752) [functional tests] test url field formatter on dashboard and discover (elastic#70736) logout from transform_poweruser user in after method of transform tests (elastic#70644) [SECURITY] Bug fix for topN on draggables (elastic#70450) [Logs UI] Reorganise log rate anomaly table (elastic#69516) Update dependency @elastic/charts to v19.7.0 (elastic#69791) Add googlecloud metricbeat module to Kibana Home (elastic#70652) [Logs UI] Logs overview queries for the observability dashboard (elastic#70413)

* actions/feature: fixed type errors [APM-UI] e2e speed up build (elastic#70704) skip flaky suite (elastic#70764) skip flaky suite (elastic#70762) [Security Solution][Endpoint] Update to new manifest format (without compression) (elastic#70752) [functional tests] test url field formatter on dashboard and discover (elastic#70736) logout from transform_poweruser user in after method of transform tests (elastic#70644) [SECURITY] Bug fix for topN on draggables (elastic#70450) [Logs UI] Reorganise log rate anomaly table (elastic#69516) Update dependency @elastic/charts to v19.7.0 (elastic#69791) Add googlecloud metricbeat module to Kibana Home (elastic#70652) [Logs UI] Logs overview queries for the observability dashboard (elastic#70413)

elasticmachine · 2021-09-23T14:31:46Z

Pinging @elastic/security-solution (Team: SecuritySolution)

XavierM added 2 commits June 30, 2020 23:52

back to normal

28db11a

add unit test

d522633

XavierM added bug Fixes for quality problems that affect the customer experience Team:SIEM v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.9.0 labels Jul 1, 2020

XavierM requested a review from andrew-goldstein July 1, 2020 14:11

XavierM requested review from a team as code owners July 1, 2020 14:11

XavierM self-assigned this Jul 1, 2020

andrew-goldstein reviewed Jul 1, 2020

View reviewed changes

Merge branch 'master' into bug-fix-top-n

2234845

stephmilovic reviewed Jul 1, 2020

View reviewed changes

...ity_solution/public/common/components/drag_and_drop/draggable_wrapper_hover_content.test.tsx Outdated Show resolved Hide resolved

stephmilovic reviewed Jul 1, 2020

View reviewed changes

x-pack/plugins/security_solution/public/timelines/components/timeline/body/index.tsx Show resolved Hide resolved

stephmilovic reviewed Jul 1, 2020

View reviewed changes

...security_solution/public/common/components/drag_and_drop/draggable_wrapper_hover_content.tsx Show resolved Hide resolved

stephmilovic approved these changes Jul 1, 2020

View reviewed changes

XavierM added 3 commits July 1, 2020 22:45

hover issue + indexToAdd issue

ff96a6f

Merge branch 'master' of github.com:elastic/kibana into bug-fix-top-n

1b86b49

fix unit test

e2df588

andrew-goldstein reviewed Jul 2, 2020

View reviewed changes

x-pack/plugins/security_solution/public/common/components/drag_and_drop/draggable_wrapper.tsx Outdated Show resolved Hide resolved

andrew-goldstein reviewed Jul 2, 2020

View reviewed changes

x-pack/plugins/security_solution/public/timelines/components/manage_timeline/index.tsx Outdated Show resolved Hide resolved

review II

81fbd51

andrew-goldstein reviewed Jul 2, 2020

View reviewed changes

XavierM added 3 commits July 2, 2020 11:56

Merge branch 'master' of github.com:elastic/kibana into bug-fix-top-n

ff31a46

fix bug + review

e81d6e1

Merge branch 'master' of github.com:elastic/kibana into bug-fix-top-n

2925543

XavierM force-pushed the bug-fix-top-n branch from 3ca1eb7 to 2925543 Compare July 2, 2020 20:54

simplification

d25a6d3

andrew-goldstein reviewed Jul 2, 2020

View reviewed changes

XavierM added 3 commits July 2, 2020 22:04

do not update state when component is unmounted

396af6a

fix hover action on field name

9a6b5d6

Merge branch 'master' of github.com:elastic/kibana into bug-fix-top-n

c5771b6

andrew-goldstein approved these changes Jul 3, 2020

View reviewed changes

XavierM merged commit 78fc9fb into elastic:master Jul 3, 2020

XavierM mentioned this pull request Jul 3, 2020

[7.x] [SECURITY] Bug fix for topN on draggables (#70450) #70737

Merged

MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021

Conversation

XavierM commented Jul 1, 2020

Summary

Checklist

Uh oh!

elasticmachine commented Jul 1, 2020

Uh oh!

andrew-goldstein Jul 1, 2020

Choose a reason for hiding this comment

Uh oh!

XavierM commented Jul 1, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stephmilovic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

andrew-goldstein Jul 2, 2020

Choose a reason for hiding this comment

Uh oh!

XavierM Jul 2, 2020

Choose a reason for hiding this comment

Uh oh!

XavierM Jul 2, 2020

Choose a reason for hiding this comment

Uh oh!

andrew-goldstein Jul 2, 2020

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jul 3, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

History

Uh oh!

andrew-goldstein left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Sep 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants