-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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 & Response] 131827 Update Detections Response view with pagination and opening numbers in timeline #131828
Conversation
cd0582b
to
90414d6
Compare
@@ -76,7 +76,7 @@ describe('CasesTable', () => { | |||
mockUseCaseItemsReturn({ isLoading: false }); | |||
const { getByText } = renderComponent(); | |||
|
|||
expect(getByText('Updated now')).toBeInTheDocument(); | |||
expect(getByText(/Updated/)).toBeInTheDocument(); |
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.
These tests are async, and caused flaky tests with 'now' not being in the doc as time passed before the check. So instead we're just checking that this part of the text is there.
90414d6
to
170df13
Compare
expect(queryByTestId('hostTablePaginator')).toBeInTheDocument(); | ||
|
||
fireEvent.click(page3); | ||
expect(mockSetPage).toHaveBeenCalledWith(2); |
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.
paginator uses 0 based index, so 3rd page is at index 2
...ty_solution/public/overview/components/detection_response/hooks/use_navigate_to_timeline.tsx
Show resolved
Hide resolved
...ty_solution/public/overview/components/detection_response/hooks/use_navigate_to_timeline.tsx
Outdated
Show resolved
Hide resolved
...ty_solution/public/overview/components/detection_response/hooks/use_navigate_to_timeline.tsx
Outdated
Show resolved
Hide resolved
…styled components
@elasticmachine merge upstream |
<EuiTablePagination | ||
data-test-subj="hostTablePaginator" | ||
activePage={pagination.currentPage} | ||
itemsPerPage={4} |
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 don't want the user to be able to configure the per page count?
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.
nah, hard coded to 4. I'll make it a const variable instead.
@@ -117,41 +108,53 @@ const getTableColumns: GetTableColumns = ({ getAppUrl, navigateTo }) => [ | |||
field: 'totalAlerts', | |||
name: i18n.ALERTS_TEXT, | |||
'data-test-subj': 'hostSeverityAlertsTable-totalAlerts', | |||
render: (totalAlerts: number) => <FormattedCount count={totalAlerts} />, | |||
render: (totalAlerts: number, { hostName }) => ( | |||
<EuiLink onClick={() => handleClick({ hostName })}> |
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: can you store this anonymous function and the others in this file in a 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.
fixed :D. Thank you!
@@ -79,7 +81,11 @@ export const getTableColumns: GetTableColumns = ({ getAppUrl, navigateTo }) => [ | |||
field: 'alert_count', | |||
name: i18n.RULE_ALERTS_COLUMN_ALERT_COUNT, | |||
'data-test-subj': 'severityRuleAlertsTable-alertCount', | |||
render: (alertCount: number) => <FormattedCount count={alertCount} />, | |||
render: (alertCount: number, { name }) => ( | |||
<EuiLink onClick={() => openRuleInTimeline(name)}> |
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 thing here regarding the anonymous function in a 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.
fixed!
|
||
setPaginationData((p) => ({ | ||
...p, | ||
pageCount: Math.ceil( |
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.
Not necessary, but you might be able to surface this to a util getPageCount
which can be used here and in the hosts one
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.
fixed :D
import { useCreateTimeline } from '../../../../timelines/components/timeline/properties/use_create_timeline'; | ||
import { updateProviders } from '../../../../timelines/store/timeline/actions'; | ||
|
||
export const useNavigateToTimeline = () => { |
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.
Just quick reminder to sync with @kqualters-elastic on this
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.
Had a few nits, but pulled down and tested and it works well! Thanks!
@elasticmachine merge upstream |
…f github.com:jamster10/kibana into 131827-Detection-response-product-and-design-changes
💚 Build SucceededMetrics [docs]Public APIs missing comments
Async chunks
History
To update your PR or re-run it, just comment with: cc @jamster10 |
…esponse view with pagination and opening numbers in timeline (elastic#131828) * Fix alert colour pallete & alerts chart header size * Add pagination and navigation to timeline capability * fix translation name conflict * Rename hook file to snake case to match elastic formatting * Change name scheme oof navigateToTimeline to OpenInTimeline & remove styled components Co-authored-by: Kristof-Pierre Cummings <kristofpierre.cummings@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
#131827
To test this PR locally: add
xpack.securitySolution.enableExperimental: [‘detectionResponseEnabled’]
to yourkibana.dev.yml
@elastic/security-threat-hunting-investigations:
The change is exporting
getDataProvider
, to allow for navigating to the Timeline flyout.Figma: https://www.figma.com/file/yGGkXDuUQzoR7faSVu6F9p/OOTB-Dashboard-for-analysts?node-id=1080%3A113129
Bugs fixed in this ticket:
Enhancements:
Update headers of vulnerable tables to new name "hosts/users by alert severity" & add tooltip for max results
Add ability for clicking on numbers to link to a timeline
Change alert colour scheme to match current alert colours
Implement pagination for vulnerable host/user tables and remove buttons.
Add cypress tests for testing navigation to timeline
Remove feature flag?
Background Conversations:
QA:
Information about the view:
For 8.3 We are only building the sections noted in this document by the thumbs up (with green circle) further work is slated potentially for 8.4.
Please refer to that document for more information about this project, but find some things noted below:
Open/Acknowledged/Closed hyperlinks in alerts chart are not clickable as the app does not have the ability to route to those specific sections in Alerts page. (we can for open, but not closed or acknowledged, so we have opted to make none of them clickable
Same as above with hyperlinks in cases chart
Alert / Alert counts for each table have been requested to link to timelines. For eg:
clicking 7 would link to a timeline filtered by alerts by host.name: "Win 8"
Permissions
If a user does not have case permissions, no case chart or tables will be seen.
If a user does not have alerts permissions, no alert chart or tables will be seen.
If a user has not pertinent permissions, they will see a no permissioned view.
Checklist