Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Security Solution] [Fix] Row Renderer + Notes in Flyout #186948

Merged
merged 33 commits into from
Jul 2, 2024
Merged

[Security Solution] [Fix] Row Renderer + Notes in Flyout #186948

merged 33 commits into from
Jul 2, 2024
+1,888 −877

Conversation

logeekal
Copy link
Contributor

@logeekal logeekal commented Jun 26, 2024 •
edited
Loading

Summary

This PR introduces below mentioned 3 changes:

Row Renderer Switch

A quick switch to switch on/off all the row-renderers without going into settings.

Caution

This is only available with feature flag unifiedComponentsInTimelineEnabled

row_renderer_switch.mp4

Notes in a separate Flyout

  • Notes do not appear inline anymore. They are now part of separate Flyout.
  • This Change also introduces a notification dot to highlight that existing notes are available.
notes_flyout_notification_dot.mp4

Color Distinction between enabled/disabled Row Renderers.

Previously it was difficult to see what row renderers are available and what are not. This change introduces a small color distinction.

row_renderer_enable_disable_color.mp4

Desk Testing

Please desk test following functionalities with AND without below feature flag:

  1. Add Note
  2. Cancel when adding note.
  3. Create a new timeline
  4. Load saved timeline
  5. Change from one timeline to other
  6. Open Timeline page directly with saved timeline in the address bar.

In all above scenarios row renders and notes should be shown as expected.

xpack.securitySolution.enableExperimental:
  - unifiedComponentsInTimelineEnabled

@logeekal
Copy link
Contributor Author

/ci

@logeekal logeekal added Team:Threat Hunting:Investigations Security Solution Investigations Team release_note:feature Makes this part of the condensed release notes backport:skip This commit does not require backporting labels Jun 27, 2024
@logeekal logeekal marked this pull request as ready for review June 27, 2024 12:19
@logeekal logeekal requested review from a team as code owners June 27, 2024 12:19
@elasticmachine
Copy link
Contributor

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

PhilippeOberti
PhilippeOberti reviewed Jun 28, 2024
View reviewed changes
Copy link
Contributor

@PhilippeOberti PhilippeOberti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code looks good overall! I left a few minor comments.

As you know we'll have a few conflicts once this PR is merged (I'm waiting on the build to finish) then will come back test this PR again

x-pack/plugins/security_solution/public/common/components/header_actions/add_note_icon_item.tsx Outdated Show resolved Hide resolved
x-pack/plugins/security_solution/public/common/components/header_actions/translations.ts Outdated Show resolved Hide resolved
x-pack/plugins/security_solution/public/common/components/header_actions/translations.ts Outdated
Comment on lines 32 to 48
'xpack.securitySolution.timeline.body.notes.addNote.singleNoteAvailableTooltip',
{
defaultMessage: '1 note available. Click to Add more.',
}
);

export const NOTES_TOOLTIP_ADD_NOTE_MULTIPLE_NOTES_AVAILABLE = ({
notesCount,
}: {
notesCount: number;
}) =>
i18n.translate(
'xpack.securitySolution.timeline.body.notes.addNote.multipleNotesAvailableTooltip',
{
values: { notesCount },
defaultMessage: '{notesCount} note available. Click to Add more.',
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like I mentioned in a previous comment, you could have a single translation done like this one?

x-pack/plugins/security_solution/public/timelines/components/row_renderer_switch/index.tsx Outdated Show resolved Hide resolved
x-pack/plugins/security_solution/public/timelines/components/row_renderer_switch/index.tsx Outdated Show resolved Hide resolved
x-pack/plugins/security_solution/public/timelines/components/timeline/properties/helpers.tsx Outdated Show resolved Hide resolved
...k/plugins/security_solution/public/timelines/components/timeline/properties/notes_flyout.tsx Show resolved Hide resolved
@logeekal
Copy link
Contributor Author

logeekal commented Jul 2, 2024

09eeb10 : @kqualters-elastic and @PhilippeOberti . Please take a look at this commit. I have changed document-id-1 to 1 so that it aligns with mockTimelineData

@logeekal
Copy link
Contributor Author

logeekal commented Jul 2, 2024 •
edited
Loading

@PhilippeOberti , here is a fix for the Pin issue that you mentioned : 82531c1. One issue I still see is that the width of action bar in explore table is not adjusted based on the securitySolutionNotesEnabled flag. Please take a look.

FYI @michaelolo24, I added a new flag for toggling pin action.

PhilippeOberti
PhilippeOberti approved these changes Jul 2, 2024
View reviewed changes
Copy link
Contributor

@PhilippeOberti PhilippeOberti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for making all the changes asked, the code LGTM now but maybe we should wait for @michaelolo24 's re-review before merging as he's been also focusing on the row renderers which I have not?

I pulled down the latest code and tested all possible combinations of the 3 feature flags (securitySolutionNotesEnabled, expandableFlyoutDisabled and unifiedComponentsInTimelineEnabled) and it all looks good to me now for the alerts page, timeline, the explore pages and the alerts table in cases!

Awesome job!

logeekal
logeekal commented Jul 2, 2024
View reviewed changes
...lution/public/timelines/components/timeline/tabs/query/query_tab_unified_components.test.tsx
Comment on lines +1141 to +1146
/*
* Above event is alert and not an event but `getEventType` in
*x-pack/plugins/security_solution/public/timelines/components/timeline/body/helpers.tsx
* returns it has event and not an alert even though, it has event.kind as signal.
* Need to see if it is okay
*
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michaelolo24, need your opinion here. What do you think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, it's not ideal, but I'm not going to ask you to rebuild to update the mock to match the 8.0+ data since that might have unintended consequences elsewhere. Also, the getEventType helper could probably also check for the signal case, but once again...no idea what unintended consequences would happen elsewhere. Thanks for the comment though, will help if/when we get to updating all of this!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not going to ask you to rebuild to update the mock to match the 8.0+ data

I thought about doing that but that does not help us much in test scenarios.

Also, the getEventType helper could probably also check for the signal case

that is the most strange thing. I think we should correct this function and then one of the test will fail. For now it is okay.

@kibana-ci
Copy link
Collaborator

kibana-ci commented Jul 2, 2024 •
edited
Loading

💔 Build Failed

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #1 / Actions Pin action should show pin Action by when disablePinAction = false
  • [job] [logs] Jest Tests #1 / Actions Pin action should show pin Action by when disablePinAction = false
  • [job] [logs] Jest Tests #1 / AddEventNoteAction display notes button should render button correctly when multiple notes exist
  • [job] [logs] Jest Tests #1 / AddEventNoteAction display notes button should render button correctly when multiple notes exist
  • [job] [logs] Jest Tests #1 / AddEventNoteAction display notes button should render button correctly when single note exists
  • [job] [logs] Jest Tests #1 / AddEventNoteAction display notes button should render button correctly when single note exists
  • [job] [logs] Jest Tests #9 / helpers getPinTooltip it indicates the alert is NOT pinned when isPinned is false and the alert has notes
  • [job] [logs] Jest Tests #9 / helpers getPinTooltip it indicates the alert is NOT pinned when isPinned is false and the alert has notes
  • [job] [logs] Jest Tests #9 / helpers getPinTooltip it indicates the event is NOT pinned when isPinned is false and the event has notes
  • [job] [logs] Jest Tests #9 / helpers getPinTooltip it indicates the event is NOT pinned when isPinned is false and the event has notes
  • [job] [logs] Jest Tests #9 / query tab with unified timeline Leading actions - notes securitySolutionNotesEnabled = false expandableFlyoutDisabled = false should have the notification dot & correct tooltip
  • [job] [logs] Jest Tests #9 / query tab with unified timeline Leading actions - notes securitySolutionNotesEnabled = false expandableFlyoutDisabled = false should have the notification dot & correct tooltip
  • [job] [logs] Jest Tests #9 / query tab with unified timeline Leading actions - notes securitySolutionNotesEnabled = false expandableFlyoutDisabled = true should have the notification dot & correct tooltip
  • [job] [logs] Jest Tests #9 / query tab with unified timeline Leading actions - notes securitySolutionNotesEnabled = false expandableFlyoutDisabled = true should have the notification dot & correct tooltip
  • [job] [logs] Jest Tests #9 / query tab with unified timeline Leading actions - notes securitySolutionNotesEnabled = true expandableFlyoutDisabled = false should have the notification dot & correct tooltip
  • [job] [logs] Jest Tests #9 / query tab with unified timeline Leading actions - notes securitySolutionNotesEnabled = true expandableFlyoutDisabled = false should have the notification dot & correct tooltip
  • [job] [logs] Jest Tests #9 / query tab with unified timeline Leading actions - notes securitySolutionNotesEnabled = true expandableFlyoutDisabled = true should have the notification dot & correct tooltip
  • [job] [logs] Jest Tests #9 / query tab with unified timeline Leading actions - notes securitySolutionNotesEnabled = true expandableFlyoutDisabled = true should have the notification dot & correct tooltip
  • [job] [logs] Jest Tests #9 / Timeline note middleware should persist a note on an event of a timeline
  • [job] [logs] Jest Tests #9 / Timeline note middleware should persist a note on an event of a timeline

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 5588 5592 +4

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.6MB 15.6MB +8.0KB

History

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

@michaelolo24
Copy link
Contributor

michaelolo24 commented Jul 2, 2024 •
edited
Loading

One thing I noticed... you can open the expandable flyout and the investigation query modal for the alert from the note and this happened:

Expandable flyout:
image

Investigation query:
image

@PhilippeOberti thoughts on closing the notes flyout when the alert/event is opened from the note? We will need to show the investigation query modal though as that adds to the note

@PhilippeOberti
Copy link
Contributor

PhilippeOberti commented Jul 2, 2024 •
edited
Loading

One thing I noticed... you can open the expandable flyout and the investigation query modal for the alert from the note and this happened:

@PhilippeOberti thoughts on closing the notes flyout when the alert/event is opened from the note? We will need to show the investigation query modal though as that adds to the note

it can be a bit annoying, but I'm also worried about building some fancy logic to check if the flyout is open and close it, but some pretty unlikely scenario where users would do this...
I don't really have strong opinions on it yet, but I'm tempted to say let's not fix this... it's easy for the users to just the notes flyout...

@logeekal
Copy link
Contributor Author

logeekal commented Jul 2, 2024 •
edited
Loading

I don't really have strong opinions on it yet, but I'm tempted to say let's not fix this... it's easy for the users to just the notes flyout...

I agree. It might make sense for users to decide if they want to close notes flyout andgot working on a new workflow.

But great catch.

@michaelolo24
Copy link
Contributor

michaelolo24 commented Jul 2, 2024 •
edited
Loading

Yea, I agree, just note that for the alerts flyout that's fine, but for the investigation query, if you close the notes flyout, it also closes the investigation query modal. How many people use that workflow, I'm not sure, but it's not really usable while the notes flyout is open.

@logeekal
Copy link
Contributor Author

logeekal commented Jul 2, 2024

Ohh that is a bummer...

Yea, I agree, just note that for the alerts flyout that's fine, but for the investigation query, if you close the notes flyout, it also closes the investigation query modal. How many people use that workflow, I'm not sure, but it's not really usable while the notes flyout is open.

Do you mind creating an issue where we can have more discussion on what should we do?

michaelolo24
michaelolo24 approved these changes Jul 2, 2024
View reviewed changes
Copy link
Contributor

@michaelolo24 michaelolo24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice work with these changes! Excited for users to get their hands on it 👍🏾

@logeekal logeekal enabled auto-merge (squash) July 2, 2024 15:17
@logeekal logeekal merged commit 965062d into elastic:main Jul 2, 2024
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PhilippeOberti PhilippeOberti PhilippeOberti approved these changes

@kqualters-elastic kqualters-elastic kqualters-elastic left review comments

@michaelolo24 michaelolo24 michaelolo24 approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting release_note:feature Makes this part of the condensed release notes Team:Threat Hunting:Investigations Security Solution Investigations Team v8.15.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@logeekal @elasticmachine @kibana-ci @michaelolo24 @PhilippeOberti @kqualters-elastic @kibanamachine