-
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][Exceptions] - Fix bug of alerts not updating after closure from exceptions modal #76145
Conversation
Pinging @elastic/siem (Team:SIEM) |
@@ -259,6 +261,7 @@ const EventsViewerComponent: React.FC<Props> = ({ | |||
</HeaderFilterGroupWrapper> | |||
)} | |||
</HeaderSection> | |||
{exceptionsModal && exceptionsModal(refetch)} |
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 is the callback that the modal needed access to. I followed the pattern used for the utility bar here.
@@ -220,6 +223,7 @@ type PropsFromRedux = ConnectedProps<typeof connector>; | |||
export const StatefulEventsViewer = connector( | |||
React.memo( | |||
StatefulEventsViewerComponent, | |||
// eslint-disable-next-line complexity |
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.
😢
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 complexity warning is a good warning that we can use to track tech debt. Looking below, this indeed looks close to needing a refactor from someone soon-ish.
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.
LGTM! I think a future update could include a count of success / failures for when all the alerts are closed. But this looks great!
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.
Left a thought about tests we might wanna consider later on but other than that looks good
}); | ||
}); | ||
|
||
test('it does not render exception modal if "exceptionModal" callback does not exist', async () => { |
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 think for the future we might wanna take a look at the structure of these files so we can lessen the number of mounts we're using, especially in cases where this test block is identical to others in this file save for the expect function. Could be a way to speed up the tests
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.
That's a great point - @patrykkopycinski is working on refactoring this code, may be interesting to look at patterns there #73228
@elasticmachine merge upstream |
/> | ||
); | ||
} else { | ||
return <></>; |
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.
Small nit. We could return null
here instead. Otherwise I think we'll end up with an unnecessary empty div
.
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.
👍 that's a good point. Since this file is being largely refactored for 7.10, I'm leaving here for now if that's ok. Happy to revisit too.
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 clarity a react fragment <></>
won't insert an empty div:
https://reactjs.org/docs/fragments.html
However, returning null should be faster than a react fragment and when possible to return null I think we should.
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.
Thanks for the info there @FrankHassanabad ! The code was reworked here so I think it's been updated - https://github.com/elastic/kibana/pull/73228/files#diff-2d4b35336b307ef04e3129bd4aecf10aL470
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.
Pulled down the branch and tested functionality in both the Detection Alerts page and the Rule Details page with both the "close" and "bulk close" options. It's working as expected now. Thanks for the fix!
💚 Build SucceededBuild metricsasync chunks size
History
To update your PR or re-run it, just comment with: |
…r closure from exceptions modal (elastic#76145) ## Summary Fixes bug found in 7.9. **Current behavior:** - Go to alerts page - Select action to add exception item - Select checkbox to close all alerts matching exception - Click `Add exception` - Note that the alerts table does not update until after you refresh or toggle between `Closed` and `Open` **New behavior:** - Go to alerts page - Select action to add exception item - Select checkbox to close all alerts matching exception - Click `Add exception` - Note that the alerts table updates
…r closure from exceptions modal (elastic#76145) ## Summary Fixes bug found in 7.9. **Current behavior:** - Go to alerts page - Select action to add exception item - Select checkbox to close all alerts matching exception - Click `Add exception` - Note that the alerts table does not update until after you refresh or toggle between `Closed` and `Open` **New behavior:** - Go to alerts page - Select action to add exception item - Select checkbox to close all alerts matching exception - Click `Add exception` - Note that the alerts table updates
…r closure from exceptions modal (#76145) (#76217) ## Summary Fixes bug found in 7.9. **Current behavior:** - Go to alerts page - Select action to add exception item - Select checkbox to close all alerts matching exception - Click `Add exception` - Note that the alerts table does not update until after you refresh or toggle between `Closed` and `Open` **New behavior:** - Go to alerts page - Select action to add exception item - Select checkbox to close all alerts matching exception - Click `Add exception` - Note that the alerts table updates
…r closure from exceptions modal (#76145) (#76216) ## Summary Fixes bug found in 7.9. **Current behavior:** - Go to alerts page - Select action to add exception item - Select checkbox to close all alerts matching exception - Click `Add exception` - Note that the alerts table does not update until after you refresh or toggle between `Closed` and `Open` **New behavior:** - Go to alerts page - Select action to add exception item - Select checkbox to close all alerts matching exception - Click `Add exception` - Note that the alerts table updates Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
…s-for-710 * 'master' of github.com:elastic/kibana: (43 commits) [APM] Chart units don't update when toggling the chart legends (elastic#74931) [ILM] Add support for frozen phase in UI (elastic#75968) Hides advanced json for count metric (elastic#74636) add client-side feature usage API (elastic#75486) [Maps] add drilldown support map embeddable (elastic#75598) [Enterprise Search] Request handler refactors/enhancements + update existing routes to use shared handler (elastic#76106) [Resolver] model `location.search` in redux (elastic#76140) [APM] Prevent imports of public in server code (elastic#75979) fix eslint issue skip flaky suite (elastic#76223) [APM] Transaction duration anomaly alerting integration (elastic#75719) [Transforms] Avoid using "Are you sure" (elastic#75932) [Security Solution][Exceptions] - Fix bug of alerts not updating after closure from exceptions modal (elastic#76145) [plugin-helpers] improve 3rd party KP plugin support (elastic#75019) [docs/getting-started] link to yarn v1 specifically (elastic#76169) [Security_Solution][Resolver] Resolver loading and error state (elastic#75600) Fixes App Search documentation links (elastic#76133) Fix alerts unable to create / update when the name has trailing whitepace(s) (elastic#76079) [Resolver] Fix useSelector usage (elastic#76129) [Enterprise Search] Migrate util and components from ent-search (elastic#76051) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/data_tier_allocation/node_allocation.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/edit_policy.tsx # x-pack/plugins/index_lifecycle_management/public/application/services/policies/types.ts # x-pack/plugins/index_lifecycle_management/public/application/services/policies/warm_phase.ts
@@ -228,7 +228,7 @@ export const AlertsTableComponent: React.FC<AlertsTableComponentProps> = ({ | |||
exceptionListType, | |||
alertData, | |||
}: AddExceptionModalBaseProps) => { | |||
if (alertData !== null && alertData !== undefined) { | |||
if (alertData != null) { |
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.
Nice idiomatic JS fix using the !=
here 👍
Pinging @elastic/security-solution (Team: SecuritySolution) |
Summary
Fixes bug found in 7.9.
Current behavior:
Add exception
Closed
andOpen
New behavior:
Add exception
Fix
Note the updated alerts total at the end.
Checklist