-
Notifications
You must be signed in to change notification settings - Fork 8k
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.
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
[Security Solution] expandable flyout - fix topN opening with Raw Events instead of Detection Alerts + add missing interaction in cell actions #164923
Conversation
1eade65
to
26fd26c
Compare
26fd26c
to
b8d58ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM. Thanks for updating the entities sections as well! Left a comment regarding the toggle column option.
I did some research on the host info and user info top n, they are still using the old hover actions. The new hover action uses sourcererScopeId
, the older version still leverages scopeId
. It seems the scopeId got lost in between components. I will check with explore to see whether they will use new hover actions everywhere soon, if not, will get a PR in 8.11
data={{ | ||
field: 'user.name', | ||
value: user, | ||
}} | ||
mode={CellActionsMode.HOVER_RIGHT} | ||
triggerId={SecurityCellActionsTrigger.DETAILS_FLYOUT} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI - toggle column is not working due to this bug. Is it safer to revert back to default until it is fixed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm divided, on one side I agree we should not show an option if it doesn't work, but on the other side the old flyout shows the option... Also the day we actually fix the source of the issue we would have to make sure to put it back everywhere...
I have a feeling that @paulewing will choose to not show the option, so I'm going to prepare a commit right now to remove it, and will merge once I have confirmation that's the behavior we want
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@christineweng see newest commit
…nts instead of Detection Alerts + add missing interaction in cell actions
…ot working on alerts table
b8d58ff
to
0e97079
Compare
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
…nts instead of Detection Alerts + add missing interaction in cell actions (elastic#164923) (cherry picked from commit 94b947e)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…Raw Events instead of Detection Alerts + add missing interaction in cell actions (#164923) (#165016) # Backport This will backport the following commits from `main` to `8.10`: - [[Security Solution] expandable flyout - fix topN opening with Raw Events instead of Detection Alerts + add missing interaction in cell actions (#164923)](#164923) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Philippe Oberti","email":"philippe.oberti@elastic.co"},"sourceCommit":{"committedDate":"2023-08-28T16:32:49Z","message":"[Security Solution] expandable flyout - fix topN opening with Raw Events instead of Detection Alerts + add missing interaction in cell actions (#164923)","sha":"94b947e625b4fdbb2affb8e6e9d8507f639d3ac6","branchLabelMapping":{"^v8.11.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:Threat Hunting:Investigations","v8.10.0","v8.11.0"],"number":164923,"url":"#164923 Solution] expandable flyout - fix topN opening with Raw Events instead of Detection Alerts + add missing interaction in cell actions (#164923)","sha":"94b947e625b4fdbb2affb8e6e9d8507f639d3ac6"}},"sourceBranch":"main","suggestedTargetBranches":["8.10"],"targetPullRequestStates":[{"branch":"8.10","label":"v8.10.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.11.0","labelRegex":"^v8.11.0$","isSourceBranch":true,"state":"MERGED","url":"#164923 Solution] expandable flyout - fix topN opening with Raw Events instead of Detection Alerts + add missing interaction in cell actions (#164923)","sha":"94b947e625b4fdbb2affb8e6e9d8507f639d3ac6"}}]}] BACKPORT--> Co-authored-by: Philippe Oberti <philippe.oberti@elastic.co>
…nts instead of Detection Alerts + add missing interaction in cell actions (elastic#164923)
Summary
This PR aims at fixing a couple of behaviors related to cell actions in the new expandable flyout:
showTopN
action now showsDetection Alerts
by default instead ofRaw Events
for:toggleColumInTable
action) for:I couldn't have the
toggleColumInTable
action in the alert reason preview because the component we're using doesn't expose the option (the other instances of that component don't provide the option either...)Screen.Recording.2023-08-26.at.6.46.47.PM.mov
Screen.Recording.2023-08-26.at.6.56.02.PM.mov
The following places could not be changed:
Because we're leveraging existing components. Changing those is feasible (I had a working POC) but seems very risky a couple of days before BC3, as it impacts other teams.
Screen.Recording.2023-08-26.at.6.47.14.PM.mov
Fixes #164801
Partially fixes #164553
Checklist