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

* 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>
elastic · Jul 3, 2020 · 4d1d79a · 4d1d79a
1 parent 4cc8ca8
commit 4d1d79a
Show file tree

Hide file tree

Showing 29 changed files with 399 additions and 172 deletions.
diff --git a/x-pack/plugins/security_solution/public/alerts/components/alerts_histogram_panel/index.tsx b/x-pack/plugins/security_solution/public/alerts/components/alerts_histogram_panel/index.tsx
@@ -69,6 +69,7 @@ interface AlertsHistogramPanelProps {
   showLinkToAlerts?: boolean;
   showTotalAlertsCount?: boolean;
   stackByOptions?: AlertsHistogramOption[];
+  timelineId?: string;
   title?: string;
   to: number;
   updateDateRange: UpdateDateRange;
@@ -98,8 +99,9 @@ export const AlertsHistogramPanel = memo<AlertsHistogramPanelProps>(
     showLinkToAlerts = false,
     showTotalAlertsCount = false,
     stackByOptions,
-    to,
+    timelineId,
     title = i18n.HISTOGRAM_HEADER,
+    to,
     updateDateRange,
   }) => {
     // create a unique, but stable (across re-renders) query id
@@ -163,11 +165,12 @@ export const AlertsHistogramPanel = memo<AlertsHistogramPanelProps>(
                 `draggable-legend-item-${uuid.v4()}-${selectedStackByOption.value}-${bucket.key}`
               ),
               field: selectedStackByOption.value,
+              timelineId,
               value: bucket.key,
             }))
           : NO_LEGEND_DATA,
       // eslint-disable-next-line react-hooks/exhaustive-deps
-      [alertsData, selectedStackByOption.value]
+      [alertsData, selectedStackByOption.value, timelineId]
     );
 
     useEffect(() => {

diff --git a/x-pack/plugins/security_solution/public/common/components/charts/barchart.tsx b/x-pack/plugins/security_solution/public/common/components/charts/barchart.tsx
@@ -117,6 +117,7 @@ interface BarChartComponentProps {
   barChart: ChartSeriesData[] | null | undefined;
   configs?: ChartSeriesConfigs | undefined;
   stackByField?: string;
+  timelineId?: string;
 }
 
 const NO_LEGEND_DATA: LegendItem[] = [];
@@ -125,6 +126,7 @@ export const BarChartComponent: React.FC<BarChartComponentProps> = ({
   barChart,
   configs,
   stackByField,
+  timelineId,
 }) => {
   const { ref: measureRef, width, height } = useThrottledResizeObserver();
   const legendItems: LegendItem[] = useMemo(
@@ -135,11 +137,12 @@ export const BarChartComponent: React.FC<BarChartComponentProps> = ({
             dataProviderId: escapeDataProviderId(
               `draggable-legend-item-${uuid.v4()}-${stackByField}-${d.key}`
             ),
+            timelineId,
             field: stackByField,
             value: d.key,
           }))
         : NO_LEGEND_DATA,
-    [barChart, stackByField]
+    [barChart, stackByField, timelineId]
   );
 
   const customHeight = get('customHeight', configs);

diff --git a/x-pack/plugins/security_solution/public/common/components/charts/draggable_legend_item.tsx b/x-pack/plugins/security_solution/public/common/components/charts/draggable_legend_item.tsx
@@ -21,13 +21,14 @@ export interface LegendItem {
   color?: string;
   dataProviderId: string;
   field: string;
+  timelineId?: string;
   value: string;
 }
 
 const DraggableLegendItemComponent: React.FC<{
   legendItem: LegendItem;
 }> = ({ legendItem }) => {
-  const { color, dataProviderId, field, value } = legendItem;
+  const { color, dataProviderId, field, timelineId, value } = legendItem;
 
   return (
     <EuiText size="xs">
@@ -44,6 +45,7 @@ const DraggableLegendItemComponent: React.FC<{
               data-test-subj={`legend-item-${dataProviderId}`}
               field={field}
               id={dataProviderId}
+              timelineId={timelineId}
               value={value}
             />
           ) : (

diff --git a/...ns/security_solution/public/common/components/drag_and_drop/drag_drop_context_wrapper.tsx b/...ns/security_solution/public/common/components/drag_and_drop/drag_drop_context_wrapper.tsx
@@ -18,11 +18,10 @@ import { IdToDataProvider } from '../../store/drag_and_drop/model';
 import { State } from '../../store/types';
 import { DataProvider } from '../../../timelines/components/timeline/data_providers/data_provider';
 import { reArrangeProviders } from '../../../timelines/components/timeline/data_providers/helpers';
-import { ACTIVE_TIMELINE_REDUX_ID } from '../top_n';
 import { ADDED_TO_TIMELINE_MESSAGE } from '../../hooks/translations';
 import { useAddToTimelineSensor } from '../../hooks/use_add_to_timeline';
 import { displaySuccessToast, useStateToaster } from '../toasters';
-
+import { TimelineId } from '../../../../common/types/timeline';
 import {
   addFieldToTimelineColumns,
   addProviderToTimeline,
@@ -35,7 +34,7 @@ import {
   userIsReArrangingProviders,
 } from './helpers';
 
-// @ts-ignore
+// @ts-expect-error
 window['__react-beautiful-dnd-disable-dev-warnings'] = true;
 
 interface Props {
@@ -67,7 +66,7 @@ const onDragEndHandler = ({
       destination: result.destination,
       dispatch,
       source: result.source,
-      timelineId: ACTIVE_TIMELINE_REDUX_ID,
+      timelineId: TimelineId.active,
     });
   } else if (providerWasDroppedOnTimeline(result)) {
     addProviderToTimeline({
@@ -76,7 +75,7 @@ const onDragEndHandler = ({
       dispatch,
       onAddedToTimeline,
       result,
-      timelineId: ACTIVE_TIMELINE_REDUX_ID,
+      timelineId: TimelineId.active,
     });
   } else if (fieldWasDroppedOnTimelineColumns(result)) {
     addFieldToTimelineColumns({
@@ -130,7 +129,6 @@ export const DragDropContextWrapperComponent = React.memo<Props & PropsFromRedux
       [dataProviders, activeTimelineDataProviders, browserFields]
     );
     return (
-      // @ts-ignore
       <DragDropContext onDragEnd={onDragEnd} onBeforeCapture={onBeforeCapture} sensors={sensors}>
         {children}
       </DragDropContext>
@@ -152,7 +150,7 @@ const emptyActiveTimelineDataProviders: DataProvider[] = []; // stable reference
 
 const mapStateToProps = (state: State) => {
   const activeTimelineDataProviders =
-    timelineSelectors.getTimelineByIdSelector()(state, ACTIVE_TIMELINE_REDUX_ID)?.dataProviders ??
+    timelineSelectors.getTimelineByIdSelector()(state, TimelineId.active)?.dataProviders ??
     emptyActiveTimelineDataProviders;
   const dataProviders = dragAndDropSelectors.dataProvidersSelector(state) ?? emptyDataProviders;
 

diff --git a/...ck/plugins/security_solution/public/common/components/drag_and_drop/draggable_wrapper.tsx b/...ck/plugins/security_solution/public/common/components/drag_and_drop/draggable_wrapper.tsx
@@ -4,7 +4,7 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import React, { useCallback, useEffect, useMemo, useState } from 'react';
+import React, { useCallback, useEffect, useMemo, useState, useRef } from 'react';
 import {
   Draggable,
   DraggableProvided,
@@ -22,7 +22,7 @@ import { DataProvider } from '../../../timelines/components/timeline/data_provid
 import { TruncatableText } from '../truncatable_text';
 import { WithHoverActions } from '../with_hover_actions';
 
-import { DraggableWrapperHoverContent } from './draggable_wrapper_hover_content';
+import { DraggableWrapperHoverContent, useGetTimelineId } from './draggable_wrapper_hover_content';
 import { getDraggableId, getDroppableId } from './helpers';
 import { ProviderContainer } from './provider_container';
 
@@ -76,6 +76,7 @@ interface Props {
   dataProvider: DataProvider;
   inline?: boolean;
   render: RenderFunctionProp;
+  timelineId?: string;
   truncate?: boolean;
   onFilterAdded?: () => void;
 }
@@ -100,16 +101,31 @@ export const getStyle = (
 };
 
 export const DraggableWrapper = React.memo<Props>(
-  ({ dataProvider, onFilterAdded, render, truncate }) => {
+  ({ dataProvider, onFilterAdded, render, timelineId, truncate }) => {
+    const draggableRef = useRef<HTMLDivElement | null>(null);
+    const [closePopOverTrigger, setClosePopOverTrigger] = useState(false);
     const [showTopN, setShowTopN] = useState<boolean>(false);
-    const toggleTopN = useCallback(() => {
-      setShowTopN(!showTopN);
-    }, [setShowTopN, showTopN]);
-
+    const [goGetTimelineId, setGoGetTimelineId] = useState(false);
+    const timelineIdFind = useGetTimelineId(draggableRef, goGetTimelineId);
     const [providerRegistered, setProviderRegistered] = useState(false);
 
     const dispatch = useDispatch();
 
+    const handleClosePopOverTrigger = useCallback(
+      () => setClosePopOverTrigger((prevClosePopOverTrigger) => !prevClosePopOverTrigger),
+      []
+    );
+
+    const toggleTopN = useCallback(() => {
+      setShowTopN((prevShowTopN) => {
+        const newShowTopN = !prevShowTopN;
+        if (newShowTopN === false) {
+          handleClosePopOverTrigger();
+        }
+        return newShowTopN;
+      });
+    }, [handleClosePopOverTrigger]);
+
     const registerProvider = useCallback(() => {
       if (!providerRegistered) {
         dispatch(dragAndDropActions.registerProvider({ provider: dataProvider }));
@@ -126,17 +142,19 @@ export const DraggableWrapper = React.memo<Props>(
       () => () => {
         unRegisterProvider();
       },
-      // eslint-disable-next-line react-hooks/exhaustive-deps
-      []
+      [unRegisterProvider]
     );
 
     const hoverContent = useMemo(
       () => (
         <DraggableWrapperHoverContent
+          closePopOver={handleClosePopOverTrigger}
           draggableId={getDraggableId(dataProvider.id)}
           field={dataProvider.queryMatch.field}
+          goGetTimelineId={setGoGetTimelineId}
           onFilterAdded={onFilterAdded}
           showTopN={showTopN}
+          timelineId={timelineId ?? timelineIdFind}
           toggleTopN={toggleTopN}
           value={
             typeof dataProvider.queryMatch.value !== 'number'
@@ -145,7 +163,15 @@ export const DraggableWrapper = React.memo<Props>(
           }
         />
       ),
-      [dataProvider, onFilterAdded, showTopN, toggleTopN]
+      [
+        dataProvider,
+        handleClosePopOverTrigger,
+        onFilterAdded,
+        showTopN,
+        timelineId,
+        timelineIdFind,
+        toggleTopN,
+      ]
     );
 
     const renderContent = useCallback(
@@ -184,7 +210,10 @@ export const DraggableWrapper = React.memo<Props>(
                       <ProviderContainer
                         {...provided.draggableProps}
                         {...provided.dragHandleProps}
-                        ref={provided.innerRef}
+                        ref={(e: HTMLDivElement) => {
+                          provided.innerRef(e);
+                          draggableRef.current = e;
+                        }}
                         data-test-subj="providerContainer"
                         isDragging={snapshot.isDragging}
                         registerProvider={registerProvider}
@@ -214,7 +243,12 @@ export const DraggableWrapper = React.memo<Props>(
     );
 
     return (
-      <WithHoverActions alwaysShow={showTopN} hoverContent={hoverContent} render={renderContent} />
+      <WithHoverActions
+        alwaysShow={showTopN}
+        closePopOverTrigger={closePopOverTrigger}
+        hoverContent={hoverContent}
+        render={renderContent}
+      />
     );
   },
   (prevProps, nextProps) =>

diff --git a/..._solution/public/common/components/drag_and_drop/draggable_wrapper_hover_content.test.tsx b/..._solution/public/common/components/drag_and_drop/draggable_wrapper_hover_content.test.tsx
@@ -52,6 +52,7 @@ jest.mock('../../../timelines/components/manage_timeline', () => {
   return {
     ...original,
     useManageTimeline: () => ({
+      getManageTimelineById: jest.fn().mockReturnValue({ indexToAdd: [] }),
       getTimelineFilterManager: mockGetTimelineFilterManager,
       isManagedTimeline: jest.fn().mockReturnValue(false),
     }),
@@ -63,8 +64,10 @@ const timelineId = TimelineId.active;
 const field = 'process.name';
 const value = 'nice';
 const toggleTopN = jest.fn();
+const goGetTimelineId = jest.fn();
 const defaultProps = {
   field,
+  goGetTimelineId,
   showTopN: false,
   timelineId,
   toggleTopN,
@@ -130,6 +133,18 @@ describe('DraggableWrapperHoverContent', () => {
           wrapper.find(`[data-test-subj="filter-${hoverAction}-value"]`).first().exists()
         ).toBe(false);
       });
+
+      test(`it should call goGetTimelineId when user is over the 'Filter ${hoverAction} value' button`, () => {
+        const wrapper = mount(
+          <TestProviders>
+            <DraggableWrapperHoverContent {...{ ...defaultProps, timelineId: undefined }} />
+          </TestProviders>
+        );
+        const button = wrapper.find(`[data-test-subj="filter-${hoverAction}-value"]`).first();
+        button.simulate('mouseenter');
+        expect(goGetTimelineId).toHaveBeenCalledWith(true);
+      });
+
       describe('when run in the context of a timeline', () => {
         let wrapper: ReactWrapper;
         let onFilterAdded: () => void;
@@ -151,6 +166,7 @@ describe('DraggableWrapperHoverContent', () => {
             </TestProviders>
           );
         });
+
         test('when clicked, it adds a filter to the timeline when running in the context of a timeline', () => {
           wrapper.find(`[data-test-subj="filter-${hoverAction}-value"]`).first().simulate('click');
           wrapper.update();
@@ -459,6 +475,24 @@ describe('DraggableWrapperHoverContent', () => {
       expect(wrapper.find('[data-test-subj="show-top-field"]').first().exists()).toBe(false);
     });
 
+    test(`it should invokes goGetTimelineId when user is over the 'Show top field' button`, () => {
+      const whitelistedField = 'signal.rule.name';
+      const wrapper = mount(
+        <TestProviders>
+          <DraggableWrapperHoverContent
+            {...{
+              ...defaultProps,
+              field: whitelistedField,
+              timelineId: undefined,
+            }}
+          />
+        </TestProviders>
+      );
+      const button = wrapper.find(`[data-test-subj="show-top-field"]`).first();
+      button.simulate('mouseenter');
+      expect(goGetTimelineId).toHaveBeenCalledWith(true);
+    });
+
     test(`invokes the toggleTopN function when the 'Show top field' button is clicked`, async () => {
       const whitelistedField = 'signal.rule.name';
       const wrapper = mount(