[8.x] [Security Solution][Notes] - fix pinning behavior in document n…

…otes (#194473) (#194890) # Backport This will backport the following commits from `main` to `8.x`: - [[Security Solution][Notes] - fix pinning behavior in document notes (#194473)](#194473)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: christineweng <[email protected]>
elastic · Oct 4, 2024 · 8bad3f1 · 8bad3f1
1 parent 34940a2
commit 8bad3f1
Show file tree

Hide file tree

Showing 10 changed files with 155 additions and 35 deletions.
diff --git a/x-pack/plugins/security_solution/public/common/components/header_actions/actions.tsx b/x-pack/plugins/security_solution/public/common/components/header_actions/actions.tsx
@@ -11,7 +11,10 @@ import { EuiButtonIcon, EuiToolTip } from '@elastic/eui';
 import styled from 'styled-components';
 
 import { TimelineTabs, TableId } from '@kbn/securitysolution-data-table';
-import { selectNotesByDocumentId } from '../../../notes/store/notes.slice';
+import {
+  selectNotesByDocumentId,
+  selectDocumentNotesBySavedObjectId,
+} from '../../../notes/store/notes.slice';
 import type { State } from '../../store';
 import { selectTimelineById } from '../../../timelines/store/selectors';
 import {
@@ -70,7 +73,7 @@ const ActionsComponent: React.FC<ActionProps> = ({
 }) => {
   const dispatch = useDispatch();
 
-  const { timelineType } = useShallowEqualSelector((state) =>
+  const { timelineType, savedObjectId } = useShallowEqualSelector((state) =>
     isTimelineScope(timelineId) ? selectTimelineById(state, timelineId) : timelineDefaults
   );
 
@@ -222,24 +225,34 @@ const ActionsComponent: React.FC<ActionProps> = ({
 
   /* only applicable for new event based notes */
   const documentBasedNotes = useSelector((state: State) => selectNotesByDocumentId(state, eventId));
-
-  /* only applicable notes before event based notes */
-  const timelineNoteIds = useMemo(
-    () => eventIdToNoteIds?.[eventId] ?? emptyNotes,
-    [eventIdToNoteIds, eventId]
+  const documentBasedNotesInTimeline = useSelector((state: State) =>
+    selectDocumentNotesBySavedObjectId(state, {
+      documentId: eventId,
+      savedObjectId: savedObjectId ?? '',
+    })
   );
 
+  /* note ids associated with the document AND attached to the current timeline, used for pinning */
+  const timelineNoteIds = useMemo(() => {
+    if (securitySolutionNotesEnabled) {
+      // if timeline is unsaved, there is no notes associated to timeline yet
+      return savedObjectId ? documentBasedNotesInTimeline.map((note) => note.noteId) : [];
+    }
+    return eventIdToNoteIds?.[eventId] ?? emptyNotes;
+  }, [
+    eventIdToNoteIds,
+    eventId,
+    documentBasedNotesInTimeline,
+    savedObjectId,
+    securitySolutionNotesEnabled,
+  ]);
+
+  /* note count of the document */
   const notesCount = useMemo(
     () => (securitySolutionNotesEnabled ? documentBasedNotes.length : timelineNoteIds.length),
     [documentBasedNotes, timelineNoteIds, securitySolutionNotesEnabled]
   );
 
-  const noteIds = useMemo(() => {
-    return securitySolutionNotesEnabled
-      ? documentBasedNotes.map((note) => note.noteId)
-      : timelineNoteIds;
-  }, [documentBasedNotes, timelineNoteIds, securitySolutionNotesEnabled]);
-
   return (
     <ActionsContainer data-test-subj="actions-container">
       <>
@@ -291,7 +304,7 @@ const ActionsComponent: React.FC<ActionProps> = ({
             isAlert={isAlert(eventType)}
             key="pin-event"
             onPinClicked={handlePinClicked}
-            noteIds={noteIds}
+            noteIds={timelineNoteIds}
             eventIsPinned={isEventPinned}
             timelineType={timelineType}
           />

diff --git a/...s/security_solution/public/flyout/document_details/left/components/notes_details.test.tsx b/...s/security_solution/public/flyout/document_details/left/components/notes_details.test.tsx
@@ -57,14 +57,16 @@ const mockGlobalStateWithSavedTimeline = {
       [TimelineId.active]: {
         ...mockGlobalState.timeline.timelineById[TimelineId.test],
         savedObjectId: 'savedObjectId',
+        pinnedEventIds: {},
       },
     },
   },
 };
 
+const mockStore = createMockStore(mockGlobalStateWithSavedTimeline);
 const renderNotesDetails = () =>
   render(
-    <TestProviders>
+    <TestProviders store={mockStore}>
       <DocumentDetailsContext.Provider value={panelContextValue}>
         <NotesDetails />
       </DocumentDetailsContext.Provider>
@@ -81,16 +83,7 @@ describe('NotesDetails', () => {
   });
 
   it('should fetch notes for the document id', () => {
-    const mockStore = createMockStore(mockGlobalStateWithSavedTimeline);
-
-    render(
-      <TestProviders store={mockStore}>
-        <DocumentDetailsContext.Provider value={panelContextValue}>
-          <NotesDetails />
-        </DocumentDetailsContext.Provider>
-      </TestProviders>
-    );
-
+    renderNotesDetails();
     expect(mockDispatch).toHaveBeenCalled();
   });
 

diff --git a/...lugins/security_solution/public/flyout/document_details/left/components/notes_details.tsx b/...lugins/security_solution/public/flyout/document_details/left/components/notes_details.tsx
@@ -17,6 +17,7 @@ import { AddNote } from '../../../../notes/components/add_note';
 import { useAppToasts } from '../../../../common/hooks/use_app_toasts';
 import { NOTES_LOADING_TEST_ID } from '../../../../notes/components/test_ids';
 import { NotesList } from '../../../../notes/components/notes_list';
+import { pinEvent } from '../../../../timelines/store/actions';
 import type { State } from '../../../../common/store';
 import type { Note } from '../../../../../common/api/timeline';
 import {
@@ -64,6 +65,19 @@ export const NotesDetails = memo(() => {
   );
   const timelineSavedObjectId = useMemo(() => timeline?.savedObjectId ?? '', [timeline]);
 
+  // Automatically pin an associated event if it's attached to a timeline and it's not pinned yet
+  const onNoteAddInTimeline = useCallback(() => {
+    const isEventPinned = eventId ? timeline?.pinnedEventIds[eventId] === true : false;
+    if (!isEventPinned && eventId && timelineSavedObjectId) {
+      dispatch(
+        pinEvent({
+          id: TimelineId.active,
+          eventId,
+        })
+      );
+    }
+  }, [dispatch, eventId, timelineSavedObjectId, timeline.pinnedEventIds]);
+
   const notes: Note[] = useSelector((state: State) =>
     selectSortedNotesByDocumentId(state, {
       documentId: eventId,
@@ -111,6 +125,7 @@ export const NotesDetails = memo(() => {
           <AddNote
             eventId={eventId}
             timelineId={isTimelineFlyout && attachToTimeline ? timelineSavedObjectId : ''}
+            onNoteAdd={isTimelineFlyout && attachToTimeline ? onNoteAddInTimeline : undefined}
           >
             {isTimelineFlyout && (
               <AttachToActiveTimeline

diff --git a/x-pack/plugins/security_solution/public/notes/components/add_note.test.tsx b/x-pack/plugins/security_solution/public/notes/components/add_note.test.tsx
@@ -127,4 +127,17 @@ describe('AddNote', () => {
       title: CREATE_NOTE_ERROR,
     });
   });
+
+  it('should call onNodeAdd callback when it is available', async () => {
+    const onNodeAdd = jest.fn();
+
+    const { getByTestId } = render(
+      <TestProviders>
+        <AddNote eventId={'event-id'} onNoteAdd={onNodeAdd} />
+      </TestProviders>
+    );
+    await userEvent.type(getByTestId('euiMarkdownEditorTextArea'), 'new note');
+    getByTestId(ADD_NOTE_BUTTON_TEST_ID).click();
+    expect(onNodeAdd).toHaveBeenCalled();
+  });
 });
diff --git a/x-pack/plugins/security_solution/public/notes/components/add_note.tsx b/x-pack/plugins/security_solution/public/notes/components/add_note.tsx
@@ -61,14 +61,18 @@ export interface AddNewNoteProps {
    * Children to render between the markdown and the add note button
    */
   children?: React.ReactNode;
+  /*
+   * Callback to execute when a new note is added
+   */
+  onNoteAdd?: () => void;
 }
 
 /**
  * Renders a markdown editor and an add button to create new notes.
  * The checkbox is automatically checked if the flyout is opened from a timeline and that timeline is saved. It is disabled if the flyout is NOT opened from a timeline.
  */
 export const AddNote = memo(
-  ({ eventId, timelineId, disableButton = false, children }: AddNewNoteProps) => {
+  ({ eventId, timelineId, disableButton = false, children, onNoteAdd }: AddNewNoteProps) => {
     const { telemetry } = useKibana().services;
     const dispatch = useDispatch();
     const { addError: addErrorToast } = useAppToasts();
@@ -88,11 +92,14 @@ export const AddNote = memo(
           },
         })
       );
+      if (onNoteAdd) {
+        onNoteAdd();
+      }
       telemetry.reportAddNoteFromExpandableFlyoutClicked({
         isRelatedToATimeline: timelineId != null,
       });
       setEditorValue('');
-    }, [dispatch, editorValue, eventId, telemetry, timelineId]);
+    }, [dispatch, editorValue, eventId, telemetry, timelineId, onNoteAdd]);
 
     // show a toast if the create note call fails
     useEffect(() => {

diff --git a/x-pack/plugins/security_solution/public/notes/store/notes.slice.test.ts b/x-pack/plugins/security_solution/public/notes/store/notes.slice.test.ts
@@ -27,6 +27,7 @@ import {
   selectNoteById,
   selectNoteIds,
   selectNotesByDocumentId,
+  selectDocumentNotesBySavedObjectId,
   selectNotesPagination,
   selectNotesTablePendingDeleteIds,
   selectNotesTableSearch,
@@ -608,6 +609,30 @@ describe('notesSlice', () => {
       expect(selectNotesByDocumentId(mockGlobalState, 'wrong-document-id')).toHaveLength(0);
     });
 
+    it('should return no notes if no notes is found with specified document id and saved object id', () => {
+      expect(
+        selectDocumentNotesBySavedObjectId(mockGlobalState, {
+          documentId: '1',
+          savedObjectId: 'wrong-savedObjectId',
+        })
+      ).toHaveLength(0);
+      expect(
+        selectDocumentNotesBySavedObjectId(mockGlobalState, {
+          documentId: 'wrong-document-id',
+          savedObjectId: 'some-timeline-id',
+        })
+      ).toHaveLength(0);
+    });
+
+    it('should return all notes for an existing document id and existing saved object id', () => {
+      expect(
+        selectDocumentNotesBySavedObjectId(mockGlobalState, {
+          documentId: '1',
+          savedObjectId: 'timeline-1',
+        })
+      ).toHaveLength(1);
+    });
+
     it('should return all notes sorted for an existing document id', () => {
       const oldestNote = {
         eventId: '1', // should be a valid id based on mockTimelineData

diff --git a/x-pack/plugins/security_solution/public/notes/store/notes.slice.ts b/x-pack/plugins/security_solution/public/notes/store/notes.slice.ts
@@ -318,6 +318,18 @@ export const selectNotesBySavedObjectId = createSelector(
     savedObjectId.length > 0 ? notes.filter((note) => note.timelineId === savedObjectId) : []
 );
 
+export const selectDocumentNotesBySavedObjectId = createSelector(
+  [
+    selectAllNotes,
+    (
+      state: State,
+      { documentId, savedObjectId }: { documentId: string; savedObjectId: string }
+    ) => ({ documentId, savedObjectId }),
+  ],
+  (notes, { documentId, savedObjectId }) =>
+    notes.filter((note) => note.eventId === documentId && note.timelineId === savedObjectId)
+);
+
 export const selectSortedNotesByDocumentId = createSelector(
   [
     selectAllNotes,

diff --git a/x-pack/plugins/security_solution/public/timelines/components/timeline/body/helpers.test.ts b/x-pack/plugins/security_solution/public/timelines/components/timeline/body/helpers.test.ts
@@ -28,7 +28,7 @@ describe('helpers', () => {
           eventHasNotes: true,
           timelineType: TimelineTypeEnum.default,
         })
-      ).toEqual('This event cannot be unpinned because it has notes');
+      ).toEqual('This event cannot be unpinned because it has notes in Timeline');
     });
 
     test('it indicates the alert may NOT be unpinned when `isPinned` is `true` and the alert has notes', () => {
@@ -39,7 +39,7 @@ describe('helpers', () => {
           eventHasNotes: true,
           timelineType: TimelineTypeEnum.default,
         })
-      ).toEqual('This alert cannot be unpinned because it has notes');
+      ).toEqual('This alert cannot be unpinned because it has notes in Timeline');
     });
 
     test('it indicates the event is pinned when `isPinned` is `true` and the event does NOT have notes', () => {
@@ -72,7 +72,7 @@ describe('helpers', () => {
           eventHasNotes: true,
           timelineType: TimelineTypeEnum.default,
         })
-      ).toEqual('This event cannot be unpinned because it has notes');
+      ).toEqual('This event cannot be unpinned because it has notes in Timeline');
     });
 
     test('it indicates the alert is pinned when `isPinned` is `false` and the alert has notes', () => {
@@ -83,7 +83,7 @@ describe('helpers', () => {
           eventHasNotes: true,
           timelineType: TimelineTypeEnum.default,
         })
-      ).toEqual('This alert cannot be unpinned because it has notes');
+      ).toEqual('This alert cannot be unpinned because it has notes in Timeline');
     });
 
     test('it indicates the event is NOT pinned when `isPinned` is `false` and the event does NOT have notes', () => {

diff --git a/x-pack/plugins/security_solution/public/timelines/components/timeline/body/translations.ts b/x-pack/plugins/security_solution/public/timelines/components/timeline/body/translations.ts
@@ -23,7 +23,7 @@ export const PINNED_WITH_NOTES = (isAlert: boolean) =>
   i18n.translate('xpack.securitySolution.timeline.body.pinning.pinnnedWithNotesTooltip', {
     values: { isAlert },
     defaultMessage:
-      'This {isAlert, select, true{alert} other{event}} cannot be unpinned because it has notes',
+      'This {isAlert, select, true{alert} other{event}} cannot be unpinned because it has notes in Timeline',
   });
 
 export const SORTED_ASCENDING = i18n.translate(

diff --git a/...ion/public/timelines/components/timeline/tabs/query/query_tab_unified_components.test.tsx b/...ion/public/timelines/components/timeline/tabs/query/query_tab_unified_components.test.tsx
@@ -1026,9 +1026,31 @@ describe('query tab with unified timeline', () => {
         );
       });
       it(
-        'should have the pin button with correct tooltip',
+        'should disable pinning when event has notes attached in timeline',
         async () => {
-          renderTestComponents();
+          const mockStateWithNoteInTimeline = {
+            ...mockGlobalState,
+            timeline: {
+              ...mockGlobalState.timeline,
+              timelineById: {
+                [TimelineId.test]: {
+                  ...mockGlobalState.timeline.timelineById[TimelineId.test],
+                  savedObjectId: 'timeline-1', // match timelineId in mocked notes data
+                  pinnedEventIds: { '1': true },
+                },
+              },
+            },
+          };
+
+          render(
+            <TestProviders
+              store={createMockStore({
+                ...structuredClone(mockStateWithNoteInTimeline),
+              })}
+            >
+              <TestComponent />
+            </TestProviders>
+          );
 
           expect(await screen.findByTestId('discoverDocTable')).toBeVisible();
 
@@ -1041,7 +1063,7 @@ describe('query tab with unified timeline', () => {
           await waitFor(() => {
             expect(screen.getByTestId('timeline-action-pin-tool-tip')).toBeVisible();
             expect(screen.getByTestId('timeline-action-pin-tool-tip')).toHaveTextContent(
-              'This event cannot be unpinned because it has notes'
+              'This event cannot be unpinned because it has notes in Timeline'
             );
             /*
              * Above event is alert and not an event but `getEventType` in
@@ -1054,6 +1076,26 @@ describe('query tab with unified timeline', () => {
         },
         SPECIAL_TEST_TIMEOUT
       );
+
+      it(
+        'should allow pinning when event has notes but notes are not attached in current timeline',
+        async () => {
+          renderTestComponents();
+          expect(await screen.findByTestId('discoverDocTable')).toBeVisible();
+
+          expect(screen.getAllByTestId('pin')).toHaveLength(1);
+          expect(screen.getByTestId('pin')).not.toBeDisabled();
+
+          fireEvent.mouseOver(screen.getByTestId('pin'));
+          await waitFor(() => {
+            expect(screen.getByTestId('timeline-action-pin-tool-tip')).toBeVisible();
+            expect(screen.getByTestId('timeline-action-pin-tool-tip')).toHaveTextContent(
+              'Pin event'
+            );
+          });
+        },
+        SPECIAL_TEST_TIMEOUT
+      );
     });
 
     describe('securitySolutionNotesEnabled = false', () => {