[Security Solution][Notes] - fix pinning behavior in document notes #194473

christineweng · 2024-09-30T18:26:44Z

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: [Bug][Security Solution] - Unable to pin event with existing note #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

Screen.Recording.2024-10-01.at.4.26.25.PM.mov

Checklist

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

elasticmachine · 2024-10-01T21:29:25Z

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

elasticmachine · 2024-10-01T21:29:26Z

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

PhilippeOberti

I'm desk testing at the moment but left a couple of implementation notes, let me know what you think!

PhilippeOberti · 2024-10-01T21:35:49Z

x-pack/plugins/security_solution/public/notes/components/add_note.tsx

@@ -88,11 +94,22 @@ export const AddNote = memo(
          },
        })
      );
+
+      // Automatically pin an associated event if it's attached to a timeline and it's not pinned yet


would you be ok with passing a callback as props instead than having the code here? Something like onNoteAdded. The reason is when I created those components, I made sure (as much as possible) that they would not be anything timeline-specific code (as these are notes and shouldn't really know about timeline, alerts...)

I understand that there are a couple of places in this file called timelineId but that is just because currently the api is using that word. Ideally we would want to rename all those variables in the frontend by savedObjectId as a note can be associated with pretty much any savedObjectId, not just timeline ones...

Does that makes sense?

I think it is a great feedback but i have a question on a separate scenario.

Consider below scenario:

If I add a note on an alert in AlertTable.

Then I add that event to a timeline.

Now since that event is attached to the timeline, should it already be pinned to that timeline?

nope because in your scenario, you created a note attached to a document only. Having an event in a timeline doesn't mean that all its note will be associated with that timeline. You can still create note NOT associated with the timeline by unchecking the Attach to timeline checkbox.
The pin icon only reflects note that have been associated with a timelineId.

Does that make sense?

It does make sense. In that case, do you think we should be more clear with the modal message which says Event cannot be unpinned because there are notes attached. It should rather say something along the lines of Event cannot be unpinned because this event and timeline have notes attached.

Not a big issue but something to keep a note of.

yes that makes sense, we can get with @nastasha-solomon for the proper wording. Something like This event cannot be unpinned because it has notes associated with this current timeline.

I don't really have a good answer at this time. I'll think about it some more!

Ah, ok I gotcha now. From the flyout (at least from now), there's no easy way to tell when a note is associated with a specific Timeline. Adding more text into the tooltip wouldn't necessarily help with that. Users would still need to go to the Notes management page to determine which notes are associated with certain Timelines.

One possibility is to move forward with the original language (This event cannot be unpinned because it has notes associated with a Timeline) and link to the docs. For example:

This event cannot be unpinned because it has notes associated with a Timeline. Learn more

It's not the best UX, but it might be a better experience than forcing users to read a multi-sentence tooltip message. The docs could also provide more guidance for figuring out when notes are associated with Timelines so users don't need to figure that out on their own.

I think this is the best we can do with the time we have right now yes. Thank you @nastasha-solomon

Connected with @nastasha-solomon offline, we are going with This event cannot be unpinned because it has notes in Timeline. at this time.

When the docs issue is available, consider adding a link at the end: This event cannot be unpinned because it has notes in Timeline. Learn more (ticket here)

Love the collaboration here 🎆 🎆 . Thanks all.

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

logeekal · 2024-10-02T08:41:13Z

Hey @christineweng , Consider adding an integration test here. This would ensure that this functionality works with both securitySolutionNotesEnabled true and false.

kibana/x-pack/plugins/security_solution/public/timelines/components/timeline/tabs/query/query_tab_unified_components.test.tsx

Lines 1016 to 1020 in d1f24b0

    
           describe('Leading actions - pin', () => { 
        
             describe('securitySolutionNotesEnabled = true', () => { 
        
               beforeEach(() => { 
        
                 (useIsExperimentalFeatureEnabled as jest.Mock).mockImplementation( 
        
                   jest.fn((feature: keyof ExperimentalFeatures) => {

logeekal

Nothing blocking but some thoughts and questions. Let me know what you think.

I desk tested and all scenarios seem to be working fine except the one I mentioned below on add_note.tsx.

logeekal · 2024-10-02T09:19:03Z

x-pack/plugins/security_solution/public/common/components/header_actions/actions.tsx

-      : timelineNoteIds;
-  }, [documentBasedNotes, timelineNoteIds, securitySolutionNotesEnabled]);
+  /* note ids specific to the current timeline, it is used to enable/disable pinning */
+  const noteIdsInTimeline = useMemo(() => {


I think this change is a little misleading for couple of reasons:

These actions can run in both timeline and non-timeline environment. so noteIdsInTimeline will be an empty array outside timeline table ( for example, alert table ) which i think is misleading. Not your fault because there exists timelineNoteIds which also does the same. Although I see that noteIdsInTimeline is using timelineNoteIds, do you think there is value in combining both of them and comment the difference between them?

@PhilippeOberti , i think we should plan to clear/remove eventIdToNoteIds since that is not being used in latest when securitySolutionNotesEnabled is true. What do you think?

I don't think we should touch anything related to the old note system, it means in this case timelineNoteIds. The reason there is some confusion here (and in a couple of other places) is because I wanted to have the new system to be completely separate from the old note one, for 2 reasons:

less risk to break an existing functionality

easier to remove the old code when the time comes (hopefully soon)

the plan is to replace the securitySolutionNotesEnabled feature flag with a new securitySolutionNotesDisabled feature flag to enable the new notes system by default in 8.16. Then probably wait a release or two (or wait for 9.0.0 to remove the old system entirely and clean all this up (ticket)

Thoughts?

@logeekal i think the confusion also stems from the fact that pinning and old notes are only available in timeline, but were not obvious in the actions. The complexity increases now that notes can be outside of timeline, so the "if the event has notes it must be pinned" logic no longer applies.

I have merged the 2 variables as you suggested with added comments. I also think once the old notes system is removed, we should refactor/rewrite the pin action to have the timeline logic inside.

Thanks @christineweng and @PhilippeOberti , I agree with both of you and leave the decision to you. Current change by @christineweng also looks pretty good to me and much more clear. Thanks 🙏 .

logeekal · 2024-10-02T09:23:30Z

x-pack/plugins/security_solution/public/notes/components/add_note.tsx

@@ -88,11 +94,22 @@ export const AddNote = memo(
          },
        })
      );
+
+      // Automatically pin an associated event if it's attached to a timeline and it's not pinned yet


I think it is a great feedback but i have a question on a separate scenario.

Consider below scenario:

If I add a note on an alert in AlertTable.

Then I add that event to a timeline.

Now since that event is attached to the timeline, should it already be pinned to that timeline?

PhilippeOberti

thanks for making the changes, this is perfect!

kibana-ci · 2024-10-03T23:03:33Z

💚 Build Succeeded

Buildkite Build
Commit: 4b65093

Metrics [docs]

Async chunks

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

id	before	after	diff
`securitySolution`	20.5MB	20.5MB	+1.7KB

History

💔 Build #239251 failed ab82637
💔 Build #239175 failed 25796b6
💔 Build #238973 failed 1717d9646a454ab6993dbbe9db6bfa78959ed862
💔 Build #238964 failed db8edc8169d03e9bb6a95f029ddaf7d06af4329e
💔 Build #238590 failed 2d8e711

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

cc @christineweng

kibanamachine · 2024-10-04T00:42:47Z

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11171977869

…lastic#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: elastic#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 (cherry picked from commit 883dfa8)

kibanamachine · 2024-10-04T00:47:08Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…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 <18648970+christineweng@users.noreply.github.com>

…lastic#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: elastic#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

christineweng self-assigned this Sep 30, 2024

christineweng changed the title ~~[Security Solution][Notes] - fetch notes by saved object ids~~ [Security Solution][Notes] - fix pinning behavior in document notes Oct 1, 2024

fix note pinning

2d8e711

christineweng force-pushed the notes-pinning branch from 6091db5 to 2d8e711 Compare October 1, 2024 21:28

christineweng marked this pull request as ready for review October 1, 2024 21:29

christineweng requested review from a team as code owners October 1, 2024 21:29

PhilippeOberti reviewed Oct 1, 2024

View reviewed changes

logeekal reviewed Oct 2, 2024

View reviewed changes

christineweng force-pushed the notes-pinning branch 2 times, most recently from db8edc8 to 1717d96 Compare October 3, 2024 01:33

nastasha-solomon mentioned this pull request Oct 3, 2024

[Security Solution][Notes] - copy changes #193495

Closed

PR comments

25796b6

christineweng force-pushed the notes-pinning branch from 1717d96 to 25796b6 Compare October 3, 2024 15:11

PhilippeOberti approved these changes Oct 3, 2024

View reviewed changes

christineweng added 2 commits October 3, 2024 13:57

Merge branch 'main' into notes-pinning

ab82637

Merge branch 'main' into notes-pinning

4b65093

christineweng merged commit 883dfa8 into elastic:main Oct 4, 2024
40 checks passed

kibanamachine mentioned this pull request Oct 4, 2024

[8.x] [Security Solution][Notes] - fix pinning behavior in document notes (#194473) #194890

Merged

nastasha-solomon mentioned this pull request Oct 7, 2024

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

Closed

23 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Notes] - fix pinning behavior in document notes #194473

[Security Solution][Notes] - fix pinning behavior in document notes #194473

christineweng commented Sep 30, 2024 •

edited by kibanamachine

Loading

elasticmachine commented Oct 1, 2024

elasticmachine commented Oct 1, 2024

PhilippeOberti left a comment

PhilippeOberti Oct 1, 2024

logeekal Oct 2, 2024

PhilippeOberti Oct 2, 2024

logeekal Oct 2, 2024

PhilippeOberti Oct 2, 2024

PhilippeOberti Oct 3, 2024

nastasha-solomon Oct 3, 2024 •

edited

Loading

PhilippeOberti Oct 3, 2024

christineweng Oct 3, 2024

logeekal Oct 4, 2024

logeekal commented Oct 2, 2024 •

edited

Loading

logeekal left a comment

logeekal Oct 2, 2024

PhilippeOberti Oct 2, 2024 •

edited

Loading

christineweng Oct 2, 2024

logeekal Oct 3, 2024

logeekal Oct 2, 2024

PhilippeOberti left a comment

kibana-ci commented Oct 3, 2024

kibanamachine commented Oct 4, 2024

kibanamachine commented Oct 4, 2024

[Security Solution][Notes] - fix pinning behavior in document notes #194473

[Security Solution][Notes] - fix pinning behavior in document notes #194473

Conversation

christineweng commented Sep 30, 2024 • edited by kibanamachine Loading

Summary

Checklist

elasticmachine commented Oct 1, 2024

elasticmachine commented Oct 1, 2024

PhilippeOberti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nastasha-solomon Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logeekal commented Oct 2, 2024 • edited Loading

logeekal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilippeOberti Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilippeOberti left a comment

Choose a reason for hiding this comment

kibana-ci commented Oct 3, 2024

💚 Build Succeeded

Metrics [docs]

Async chunks

History

kibanamachine commented Oct 4, 2024

kibanamachine commented Oct 4, 2024

💚 All backports created successfully

Questions ?

christineweng commented Sep 30, 2024 •

edited by kibanamachine

Loading

nastasha-solomon Oct 3, 2024 •

edited

Loading

logeekal commented Oct 2, 2024 •

edited

Loading

PhilippeOberti Oct 2, 2024 •

edited

Loading