-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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][Detection Alerts] Changes in-progress status to acknowledged #107972
[Security Solution][Detection Alerts] Changes in-progress status to acknowledged #107972
Conversation
5d05168
to
370ac1d
Compare
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
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.
For the text changes proposed here, It would be great to see an integration test added to this file where we update an alert to have an 'acknowledged'
status:
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 checked this out and observed behavior with existing alerts. We're broadening the validations here so we should be safe in terms of legacy data, and most logic appears to treat in-progress
and acknowledged
identically. At a high level this seems like an opportunity to apply some "transform on read" functionality to alerts, but totally understand if that's not feasible right now.
Like other reviewers I'd love to see an integration test that asserts something about the new status value; it's also a little concerning that none of the existing integration tests do so 😬 .
da035ee
to
51a4d0a
Compare
@@ -193,7 +194,8 @@ export const AlertsTableComponent: React.FC<AlertsTableComponentProps> = ({ | |||
title = i18n.OPENED_ALERT_FAILED_TOAST; | |||
break; | |||
case 'in-progress': |
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.
Can you create a separate issue to remove the in-progress
references later on?
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.
issue here #109030
ctx._source.threat.enrichments.add(enrichment); | ||
} | ||
ctx._source.threat.remove("indicator"); | ||
} | ||
|
||
// migrate status | ||
if(ctx._source.signal?.status == "in-progress") { |
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.
👍🏾
|
||
/** | ||
* @deprecated | ||
* TODO: Remove after `acknowledged` migration |
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.
This can go with the issue above. Just don't want us to lose track of this and have a build up of dead code
@elasticmachine merge upstream |
@@ -337,7 +337,9 @@ describe('EventsViewer', () => { | |||
<EventsViewer | |||
{...eventsViewerDefaultProps} | |||
graphEventId={undefined} | |||
headerFilterGroup={<AlertsTableFilterGroup onFilterGroupChanged={jest.fn()} />} | |||
headerFilterGroup={ |
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.
why are we updating?
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.
the headerFilterGroup should not be used anymore
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.
the AlertsTableFilterGroup
component updated, but we should probably delete/modify most of these since we don't use it this way in the app anymore
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.
@XavierM we still have the property there though, no? I don't think we need this many tests anymore but it might be worthwhile to keep some around in case we use it with the component somewhere down the line.
</StatusFilterButton> | ||
</StatusFilterGroup> | ||
<EuiButtonGroup | ||
legend="filter status" |
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.
maybe we need translations here
options={options} | ||
idSelected={status} | ||
data-test-subj="alerts-table-filter-group" | ||
onChange={(id) => onFilterGroupChanged(id as Status)} |
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.
nit -> use useCallback
<EuiFlexItem> | ||
<Link | ||
aria-label="markSelectedAlertsInProgress" | ||
aria-label="markSelectedAlertsAcknowledged" | ||
onClick={() => { |
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.
use. useCallback
onClick={() => onClickUpdate(FILTER_IN_PROGRESS)} | ||
key="acknowledge" | ||
data-test-subj="acknowledged-alert-status" | ||
onClick={() => onClickUpdate(FILTER_ACKNOWLEDGED)} |
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.
nit - use useCallback
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.
Code looks good
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.
Really nice job with this and thanks for making all the changes. 🔥 🔥 🔥 Great job!
a9bdacb
to
ecd286c
Compare
💚 Build SucceededMetrics [docs]Async chunks
Page load bundle
Unknown metric groupsAPI count
API count missing comments
History
To update your PR or re-run it, just comment with: cc @dplumlee |
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
Summary
Related to the RAC updates (#107923)
Updates alert status options to include
acknowledged
instead ofin-progress
Screenshots
Checklist
Delete any items that are not applicable to this PR.
For maintainers