From dd07e209271f53daa8d33840d4d9e7c980d14a1e Mon Sep 17 00:00:00 2001 From: christineweng <18648970+christineweng@users.noreply.github.com> Date: Thu, 3 Oct 2024 19:42:26 -0500 Subject: [PATCH] [Security Solution][Notes] - fix pinning behavior in document notes (#194473) ## Summary Fixed some pinning behaviors in new notes system, namely: - Pinning is greyed out only when an event has a note attached to the **current** timeline, else pinning should work as usual (related: https://github.com/elastic/kibana/issues/193738) - Adding a note and attaching to current timeline automatically pins the event - Pinned tab and pinning capability are updated when a note attached to current timeline is deleted Feature flag: `securitySolutionNotesEnabled` https://github.com/user-attachments/assets/0163d4a4-354c-4928-a337-40a93f6c7b2a ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --- .../components/header_actions/actions.tsx | 41 ++++++++++------ .../left/components/notes_details.test.tsx | 15 ++---- .../left/components/notes_details.tsx | 15 ++++++ .../public/notes/components/add_note.test.tsx | 13 +++++ .../public/notes/components/add_note.tsx | 11 ++++- .../public/notes/store/notes.slice.test.ts | 25 ++++++++++ .../public/notes/store/notes.slice.ts | 12 +++++ .../components/timeline/body/helpers.test.ts | 8 ++-- .../components/timeline/body/translations.ts | 2 +- .../query_tab_unified_components.test.tsx | 48 +++++++++++++++++-- 10 files changed, 155 insertions(+), 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 index d6226ea0df00b..7a1bc09c6c45e 100644 --- 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 = ({ }) => { 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 = ({ /* 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 ( <> @@ -291,7 +304,7 @@ const ActionsComponent: React.FC = ({ isAlert={isAlert(eventType)} key="pin-event" onPinClicked={handlePinClicked} - noteIds={noteIds} + noteIds={timelineNoteIds} eventIsPinned={isEventPinned} timelineType={timelineType} /> diff --git a/x-pack/plugins/security_solution/public/flyout/document_details/left/components/notes_details.test.tsx b/x-pack/plugins/security_solution/public/flyout/document_details/left/components/notes_details.test.tsx index 9426d604bce57..fbca02404fda4 100644 --- a/x-pack/plugins/security_solution/public/flyout/document_details/left/components/notes_details.test.tsx +++ b/x-pack/plugins/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( - + @@ -81,16 +83,7 @@ describe('NotesDetails', () => { }); it('should fetch notes for the document id', () => { - const mockStore = createMockStore(mockGlobalStateWithSavedTimeline); - - render( - - - - - - ); - + renderNotesDetails(); expect(mockDispatch).toHaveBeenCalled(); }); diff --git a/x-pack/plugins/security_solution/public/flyout/document_details/left/components/notes_details.tsx b/x-pack/plugins/security_solution/public/flyout/document_details/left/components/notes_details.tsx index b3dfbd53416be..46c7573277967 100644 --- a/x-pack/plugins/security_solution/public/flyout/document_details/left/components/notes_details.tsx +++ b/x-pack/plugins/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(() => { {isTimelineFlyout && ( { title: CREATE_NOTE_ERROR, }); }); + + it('should call onNodeAdd callback when it is available', async () => { + const onNodeAdd = jest.fn(); + + const { getByTestId } = render( + + + + ); + 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 index d54e0e42c86eb..d31aaaa028b56 100644 --- 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,6 +61,10 @@ 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; } /** @@ -68,7 +72,7 @@ export interface AddNewNoteProps { * 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 index 396940c892a6e..5f55a7ef49c82 100644 --- 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 index 3f0439e7298e4..6732f9491676e 100644 --- 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 index 2ebbe54b6fac5..7a115c35c7e9b 100644 --- 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 index a279b3a654006..b201e766689b2 100644 --- 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/x-pack/plugins/security_solution/public/timelines/components/timeline/tabs/query/query_tab_unified_components.test.tsx b/x-pack/plugins/security_solution/public/timelines/components/timeline/tabs/query/query_tab_unified_components.test.tsx index 2fa1b53881b08..3d7f37205ca94 100644 --- a/x-pack/plugins/security_solution/public/timelines/components/timeline/tabs/query/query_tab_unified_components.test.tsx +++ b/x-pack/plugins/security_solution/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( + + + + ); 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', () => {