-
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] [Feat] Add Bulk Events to Timeline. #142737
[Security Solution] [Feat] Add Bulk Events to Timeline. #142737
Conversation
814f338
to
9468db9
Compare
d9da37d
to
2a59c5e
Compare
}; | ||
|
||
const addToCaseBulkActions = useBulkAddToCaseActions(); | ||
const bulkActions = useMemo( |
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.
Bulk Actions prop has been moved up the tree so that it is easier for implementing function to add their own bulk actions.
This comment was marked as outdated.
This comment was marked as outdated.
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.
Looks good for detections alerts area code
5830cbf
to
7c36642
Compare
@@ -108,6 +108,7 @@ const SessionsViewComponent: React.FC<SessionsComponentsProps> = ({ | |||
pageFilters={sessionsFilter} | |||
defaultModel={getSessionsDefaultModel(columns, defaultColumns)} | |||
end={endDate} | |||
bulkActions={false} |
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.
Hi @logeekal,
I'm just curious, is there any particular reason to not add that feature on the Sessions Tab as well? It seems the only place to be left out.
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.
Hi @opauloh , you were right. I had missed it. But I have now added it to session viewer as well. Thanks for pointing it out.
293fb93
to
89047d2
Compare
Files by Code Ownerelastic/awp-viz
elastic/security-detections-response
elastic/security-detections-response-alerts
elastic/security-detections-response-rules
elastic/security-engineering-productivity
elastic/security-solution
elastic/security-threat-hunting
elastic/security-threat-hunting-explore
elastic/security-threat-hunting-investigations
|
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.
Tested locally, all looks good, thank you @logeekal
|
||
it('Adding multiple alerts to the timeline should be successful', () => { | ||
// select all visible events | ||
investigateFirstPageEventsInTimeline(); |
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.
Assertions on tests should not be wrapped inside the tasks methods, should be explicitly written in the test.
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.
@MadameSheema .. Sure, I have now removed the assertions from tasks. Please check new updates.
}); | ||
|
||
it('When selected all alerts are selected should be successfull', () => { | ||
investigateAllEventsInTimeline(); |
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.
Assertions on tests should not be wrapped inside the tasks methods, should be explicitly written in the test.
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.
@MadameSheema .. Sure, I have now removed the assertions from tasks. Please check new updates.
|
||
it('Adding multiple events to the timeline should be successful', () => { | ||
// select all visible events | ||
investigateFirstPageEventsInTimeline(); |
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.
Assertions on tests should not be wrapped inside the tasks methods, should be explicitly written in the test.
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.
@MadameSheema .. Sure, I have now removed the assertions from tasks. Please check new updates.
}); | ||
|
||
it('When selected all events are selected, bulk action should be disabled', () => { | ||
investigateAllEventsInTimeline(); |
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.
Assertions on tests should not be wrapped inside the tasks methods, should be explicitly written in the test.
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.
@MadameSheema .. Sure, I have now removed the assertions from tasks. Please check new updates.
|
||
it('Adding multiple events to the timeline should be successful', () => { | ||
// select all visible events | ||
investigateFirstPageEventsInTimeline(); |
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.
Assertions on tests should not be wrapped inside the tasks methods, should be explicitly written in the test.
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.
Same as above.
}); | ||
|
||
it('When selected all events are selected, bulk action should be disabled', () => { | ||
investigateAllEventsInTimeline(); |
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.
Assertions on tests should not be wrapped inside the tasks methods, should be explicitly written in the test.
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.
Same as above.
@@ -62,9 +68,39 @@ describe('Alerts timeline', () => { | |||
}); | |||
|
|||
it('Add an empty property to default timeline', () => { | |||
// add condition to make sure the field is empty |
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 is this needed now?
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 are lot of records and we want to make sure that first row has empty value for file.name
. Safest method to ensure that is to add a filter.
Now this situation arises sometimes when this test runs after another test which hasn't unloaded their own data. It looks like cleankibana
does not clear the data uploaded by esarchiver
cy.get(ADD_FILTER_FORM_OPERATOR_FIELD).click(); | ||
cy.get(ADD_FILTER_FORM_OPERATOR_OPTION_IS).click(); | ||
cy.get(ADD_FILTER_FORM_FILTER_VALUE_INPUT).type(value); | ||
if (!operator) { |
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.
We are trying to avoid conditionals inside tasks methods.
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 understand avoiding conditional inside assertions as they may become in-deterministic but is there particular reason to avoid conditionals in tasks.
Nonetheless, this condition can still be removed by making some changes but the next condition if (value)
cannot be removed as filling the value in the filter form depends on the operator you have selected. Please see below screenshots for comparison.
Value Field visible | Value Field not visible |
---|---|
![]() |
![]() |
cleanKibana(); | ||
login(); | ||
createCustomRuleEnabled(getNewRule()); | ||
esArchiverLoad('bulk_process'); |
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.
As the file we are using is huge, executing the loading and unloading of it is around one minute. As we are doing this in several tests, we are increasing the test execution time a lot. If it is not possible to reduce it, we should consider putting all the tests that are using it in the same cypress file to load it and unload it just once reducing the time execution.
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.
@MadameSheema , it makes sense.. I have consolidated all tests in a single test suite. Please check new updates. Thanks.
Here is new test file: https://github.com/elastic/kibana/pull/142737/files#diff-126a474680b59e71fa3835f6f722e95dc39ff263e3b7b32fae883ea5ba3c0dd5
}); | ||
|
||
beforeEach(() => { | ||
visit(ALERTS_URL); |
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.
It would be better in terms of execution time, to close the timeline each time instead of reloading the page and waiting for it.
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.
@MadameSheema You are right. Updated.
Thanks @MadameSheema for detailed feedback. |
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!
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
ESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @logeekal |
* main: [Lens] Rearrange options (elastic#144891) [Actionable Observability] Integrate alert search bar on rule details page (elastic#144718) [Security Solution] [Exceptions] Adds options to create a shared exception list and to create a single item from the manage exceptions view (elastic#144575) [Actionable Observability] Add context.alertDetailsUrl to connector template for Uptime > Monitor status & Uptime TLS rules (elastic#144740) [Security Solution] [Feat] Add Bulk Events to Timeline. (elastic#142737) [TIP] Env specific cypress config (elastic#144894) skip flaky suite (elastic#144885) [Enterprise Search] Fixes Search Index page to go blank when connection lost (elastic#144022) [Cloud Posture] track findings pages (elastic#144822) [ContentManagement] Inspector flyout (elastic#144240) [Cloud Posture] Dashboard Redesign - data counter cards (elastic#144565) [TIP] Run e2e pipeline on CI (elastic#144776) [Guided onboarding] Config updates for the Security guide (elastic#144844) Cleanup unused code for claiming tasks by id (elastic#144408) Ping the response-ops team whenever a new connector type is registered (elastic#144736)
Pinging @elastic/security-solution (Team: SecuritySolution) |
Summary
This PR implements functionality to add multiple events to the timeline. It is implements :
Implementation.
kql Filters
. [Security Solution] Add is one of operator for usage with DataProvider's QueryMatch value. #142436 is in progress to implementis-one-of
operator in the data provider. Once that is moved tomain
, we can change value ofprefer
parameter to send the IDs indataProvider
rather than filter.If you would like to test it with #142436, please clone : https://github.com/logeekal/kibana/tree/bulk_actions_add_timeline_with_is_one_of
Screen.Recording.2022-10-31.at.15.51.56.mov
Checklist
Delete any items that are not applicable to this PR.
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
For maintainers