[Security Solution][Timeline] Notes management table #187214

kqualters-elastic · 2024-07-01T06:37:37Z

Summary

This pr adds a new notes management table, where notes from any timeline + notes not associated with a timeline can be viewed, searched, and deleted (for now). Hidden behind the securitySolutionNotesEnabled feature flag, the new page is a tab of the timelines pages, and will only appear in global search when the flag is enabled. Uses the previously added api from a table, and notes can be deleted individually or a page at a time.

t

Checklis

Delete any items that are not applicable to this PR.

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

- remove ids from pendingDeleteIds on delete success - remove no notes empty prompts and related translations - remove unnecessary index file - change data-test-subj to be more specific - merged deleteNote and deleteNotes - added unit tests

PhilippeOberti

overall this is amazing! You will see that I took the liberty of pushing one commit to fix a few things. Feel free to revert the commit if you don't like the changes~!

fix an issue with the translation displayed in the delete note modal
remove the ids from the pendingDeleteIds when the deleteNotes action succeeds or the modal was staying open
made some small tweaks to the data-test-subj to make them more specific
merged the deleteNote and deleteNotes actions as we don't really need both (I updated all the code and tests where the old deleteNote action was used
added some unit tests for the notes.slice file

Here are some things that I found during review that still could/need to be fixed:

searching requires the full string to be written, you can't search with partial string. I find this annoying but I've seen this behavior in other pages (like the rules page)
should we display the document ids (eventId) and/or the saved object ids (timelineId) in the notes management table?
deleting a note doesn't update the total number of notes in the pagination section. It also doesn't refresh the table correctly.

PhilippeOberti · 2024-07-01T10:33:37Z

x-pack/plugins/security_solution/public/notes/store/notes.slice.ts

@@ -122,10 +212,37 @@ const notesSlice = createSlice({
      .addCase(deleteNote.fulfilled, (state, action) => {
        notesAdapter.removeOne(state, action.payload);
        state.status.deleteNote = ReqStatus.Succeeded;
+        state.pendingDeleteIds = state.pendingDeleteIds.filter((id) => id !== action.payload);


I added here the code to remove the id from the pendingDeleteIds property. I originally had cleared everything (state.pendingDeleteIds = []) but then decided that this action should do just what it's supposed to and nothing more...

x-pack/plugins/security_solution/public/notes/store/notes.slice.ts

PhilippeOberti · 2024-07-01T10:37:34Z

x-pack/plugins/security_solution/public/notes/store/notes.slice.ts

+
+export const selectNotesTableSort = (state: State) => state.notes.sort;
+
+export const selectNotesTableTotalItems = (state: State) => state.notes.pagination.total;


I wonder why this selector is necessary/useful? Why not using the selectNotesPagination right above? We already use the other one in a few places, and we even destructure the properties (like

const pagination = useSelector(selectNotesPagination); const { perPage, page } = pagination;

for example)

any reason @kqualters-elastic?

.../security_solution/public/timelines/components/open_timeline/edit_timeline_batch_actions.tsx

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

...lugins/security_solution/public/timelines/components/open_timeline/timelines_table/index.tsx

x-pack/plugins/security_solution/public/timelines/components/open_timeline/types.ts

...k/plugins/security_solution/public/timelines/components/open_timeline/use_timeline_types.tsx

x-pack/plugins/security_solution/public/timelines/pages/index.tsx

x-pack/plugins/security_solution/public/timelines/pages/timelines_page.tsx

…es-management-table

elasticmachine · 2024-07-01T14:22:27Z

Pinging @elastic/security-threat-hunting-investigations (Team:Threat Hunting:Investigations)

PhilippeOberti

the code looks good, I'll do a last round of testing first thing tomorrow morning my time as it's getting late here!

PhilippeOberti · 2024-07-01T21:36:28Z

x-pack/plugins/security_solution/public/notes/store/notes.slice.test.ts

so many tests ❤️

PhilippeOberti · 2024-07-01T21:38:15Z

x-pack/plugins/security_solution/public/notes/store/notes.slice.ts

+
+export const selectNotesTableSort = (state: State) => state.notes.sort;
+
+export const selectNotesTableTotalItems = (state: State) => state.notes.pagination.total;


any reason @kqualters-elastic?

kqualters-elastic · 2024-07-01T21:49:22Z

@PhilippeOberti no, will remove

kibana-ci · 2024-07-02T01:08:01Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: c15c13f

Failed CI Steps

x-pack/test/alerting_api_integration/security_and_spaces/group1/config.ts

Test Failures

[job] [logs] x-pack/test/alerting_api_integration/security_and_spaces/group1/config.ts / alerting api integration security and spaces enabled Alerts - Group 1 alerts backfill rule runs find backfill superuser at space1 should handle finding backfill requests with query string appropriately

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	5582	5587	+5

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`securitySolution`	15.5MB	15.6MB	+42.4KB

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`securitySolution`	551	549	-2

Total ESLint disabled count

id	before	after	diff
`securitySolution`	634	632	-2

History

💔 Build #218886 failed bab95c1
💔 Build #218874 failed d461fb4

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

parkiino

Management part looks fine

PhilippeOberti

the code looks good to me, a few minor UI issues remaining (like total not updating after delete one or multiple notes) but we can fix that in a follow up PR!

Nice job!

kqualters-elastic added 21 commits May 23, 2024 20:09

WIP api hack and slash attempt

340da96

WIP allow deleting more than 1 note at a time

78de8a8

Merge branch 'main' into event-notes

298cb2b

Limit notes to 1000 via fake reference

c187da9

Fix validation and GET for 1 id

e43e53f

Merge branch 'main' into event-notes

9feb1f8

Add tests, fix types, remove demo frontend changes

964a639

Merge remote-tracking branch 'upstream/main' into event-notes

4eef238

Remove dangling frontend changes

c2ddbba

PR feedback

4a19aa8

Fix failing test

628d7e1

Revert type change

564d02d

Merge remote-tracking branch 'upstream/main' into event-notes

84e5db3

Undo unneeded test change

7a5d545

Merge remote-tracking branch 'upstream/main' into event-notes

76a717c

Add/update openapi schema

373af99

Merge remote-tracking branch 'upstream/main' into event-notes

ea05f66

WIP using slice

469f97f

Merge branch 'main' into notes-management-table

00f5fff

WIP use rtk for management page

11490b4

Notes management table and tab

9a65498

kqualters-elastic added the Team:Threat Hunting:Investigations Security Solution Investigations Team label Jul 1, 2024

PhilippeOberti force-pushed the notes-management-table branch 2 times, most recently from 61c0524 to 0145a48 Compare July 1, 2024 12:35

- add page title

0b5e41e

- remove ids from pendingDeleteIds on delete success - remove no notes empty prompts and related translations - remove unnecessary index file - change data-test-subj to be more specific - merged deleteNote and deleteNotes - added unit tests

PhilippeOberti force-pushed the notes-management-table branch from 0145a48 to 0b5e41e Compare July 1, 2024 12:39

PhilippeOberti reviewed Jul 1, 2024

View reviewed changes

kqualters-elastic added 3 commits July 1, 2024 13:00

Add tests

3e29f13

Merge remote-tracking branch 'origin/notes-management-table' into not…

b1097b6

…es-management-table

Move notes page to timelines, register deep link conditionally

e60aa4d

kqualters-elastic changed the title ~~[Security Solution][Timeline] WIP/DNM Notes management table~~ [Security Solution][Timeline] Notes management table Jul 1, 2024

kqualters-elastic added v8.15.0 release_note:skip Skip the PR/issue when compiling release notes labels Jul 1, 2024

kqualters-elastic marked this pull request as ready for review July 1, 2024 14:22

kqualters-elastic requested review from a team as code owners July 1, 2024 14:22

kqualters-elastic requested review from gergoabraham and parkiino July 1, 2024 14:22

kqualters-elastic added 5 commits July 1, 2024 13:59

Merge remote-tracking branch 'upstream/main' into notes-management-table

d461fb4

Fix types

a6fbb4e

Merge remote-tracking branch 'upstream/main' into notes-management-table

bab95c1

Fix tests

8821356

Merge remote-tracking branch 'upstream/main' into notes-management-table

56a42c6

PhilippeOberti reviewed Jul 1, 2024

View reviewed changes

kqualters-elastic and others added 3 commits July 1, 2024 21:50

Fix test and remove unneeded selector

ec24830

Merge remote-tracking branch 'upstream/main' into notes-management-table

ae8c7d1

Merge branch 'main' into notes-management-table

c15c13f

parkiino approved these changes Jul 2, 2024

View reviewed changes

PhilippeOberti approved these changes Jul 2, 2024

View reviewed changes

PhilippeOberti merged commit 1fa094b into elastic:main Jul 2, 2024
40 checks passed

kibanamachine added the backport:skip This commit does not require backporting label Jul 2, 2024

nastasha-solomon mentioned this pull request Oct 27, 2024

[Request][Serverless][8.16] New notes experience elastic/security-docs#5441

Closed

23 tasks

PhilippeOberti mentioned this pull request Nov 5, 2024

8.16 Elastic Security Release Notes elastic/security-docs#5977

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Timeline] Notes management table #187214

[Security Solution][Timeline] Notes management table #187214

kqualters-elastic commented Jul 1, 2024 •

edited

Loading

PhilippeOberti left a comment

PhilippeOberti Jul 1, 2024

PhilippeOberti Jul 1, 2024 •

edited

Loading

PhilippeOberti Jul 1, 2024

elasticmachine commented Jul 1, 2024

PhilippeOberti left a comment

PhilippeOberti Jul 1, 2024

PhilippeOberti Jul 1, 2024

kqualters-elastic commented Jul 1, 2024

kibana-ci commented Jul 2, 2024

ESLint disabled line counts

Total ESLint disabled count

parkiino left a comment

PhilippeOberti left a comment


		export const selectNotesTableSort = (state: State) => state.notes.sort;

		export const selectNotesTableTotalItems = (state: State) => state.notes.pagination.total;

[Security Solution][Timeline] Notes management table #187214

[Security Solution][Timeline] Notes management table #187214

Conversation

kqualters-elastic commented Jul 1, 2024 • edited Loading

Summary

Checklis

PhilippeOberti left a comment

Choose a reason for hiding this comment

PhilippeOberti Jul 1, 2024

Choose a reason for hiding this comment

PhilippeOberti Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

PhilippeOberti Jul 1, 2024

Choose a reason for hiding this comment

elasticmachine commented Jul 1, 2024

PhilippeOberti left a comment

Choose a reason for hiding this comment

PhilippeOberti Jul 1, 2024

Choose a reason for hiding this comment

PhilippeOberti Jul 1, 2024

Choose a reason for hiding this comment

kqualters-elastic commented Jul 1, 2024

kibana-ci commented Jul 2, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Async chunks

ESLint disabled line counts

Total ESLint disabled count

History

parkiino left a comment

Choose a reason for hiding this comment

PhilippeOberti left a comment

Choose a reason for hiding this comment

kqualters-elastic commented Jul 1, 2024 •

edited

Loading

PhilippeOberti Jul 1, 2024 •

edited

Loading