From dfe6897eb9e7494eb353a15f9562cea7140cd245 Mon Sep 17 00:00:00 2001 From: Jan Monschke Date: Fri, 28 Apr 2023 09:50:21 +0200 Subject: [PATCH] [SecuritySolution] Fix building block alert highlighting (#155497) ## Summary As described in https://github.com/elastic/kibana/issues/152318, we noticed that building block alerts were not highlighted anymore after the migration to the new alerts table. A preferred implementation of building block alert highlighting would follow the [`EUIDataGrid` approach of row highlighting](https://eui.elastic.co/#/tabular-content/data-grid-style-display#grid-row-classes). The `DataGrid` allows you to pass custom CSS class names for each row (`gridStyle.rowClasses`). That would allow us to highlight table rows with building block alerts. However, without access to the underlying data, we would not be able generate the correct `rowClasses` for rows with building block alerts. So simply passing `gridStyle.rowClasses` to the `AlertsStateTable` was not an option. Therefore in this PR we're introducing a new prop on the `AlertsTable`, the `highlightedRowMapper`. It's a callback function that receives the alert data and when it returns true, that row will be highlighted. This allows for highlighting of rows from the outside without exposing too many details about the underlying data structures. **Screenshot of the alerts table with a highlightedRowMapper that highlights building block alerts** Screenshot 2023-04-21 at 13 03 54 ### Additional notes - Since the alerts table has default grid styles, it allows to pass `gridStyle` and it computes its own `rowClasses` for "active row" highlighting, the logic for merging all those styles looks intimidating. I tried my best to comment that part of code to make it clear why the merges are necessary and how they work. - While working on the issue, I noticed that active rows are not highlighted anymore (related bug: https://github.com/elastic/kibana/issues/155487). The changes in this PR fix that behaviour as well as you can see in the screenshot below: Screenshot 2023-04-21 at 13 04 15 ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit ce71264d88c46c45ed8ac00dc6c74bcbd05809c7) --- .../building_block_alerts.cy.ts | 4 + .../cypress/screens/rule_details.ts | 3 + .../components/alerts_table/index.tsx | 6 ++ .../sections/alerts_table/alerts_table.scss | 4 + .../sections/alerts_table/alerts_table.tsx | 74 +++++++++++++++++-- .../alerts_table/alerts_table_state.tsx | 7 ++ .../triggers_actions_ui/public/types.ts | 4 + 7 files changed, 95 insertions(+), 7 deletions(-) diff --git a/x-pack/plugins/security_solution/cypress/e2e/detection_alerts/building_block_alerts.cy.ts b/x-pack/plugins/security_solution/cypress/e2e/detection_alerts/building_block_alerts.cy.ts index 8d831f29bd653..fcd98e0f1f127 100644 --- a/x-pack/plugins/security_solution/cypress/e2e/detection_alerts/building_block_alerts.cy.ts +++ b/x-pack/plugins/security_solution/cypress/e2e/detection_alerts/building_block_alerts.cy.ts @@ -7,6 +7,7 @@ import { getBuildingBlockRule } from '../../objects/rule'; import { OVERVIEW_ALERTS_HISTOGRAM_EMPTY } from '../../screens/overview'; +import { HIGHLIGHTED_ROWS_IN_TABLE } from '../../screens/rule_details'; import { OVERVIEW } from '../../screens/security_header'; import { goToRuleDetails } from '../../tasks/alerts_detection_rules'; import { createRule } from '../../tasks/api_calls/rules'; @@ -40,6 +41,9 @@ describe('Alerts generated by building block rules', () => { // Check that generated events are visible on the Details page waitForAlertsToPopulate(EXPECTED_NUMBER_OF_ALERTS); + // Make sure rows are highlighted + cy.get(HIGHLIGHTED_ROWS_IN_TABLE).should('exist'); + navigateFromHeaderTo(OVERVIEW); // Check that generated events are hidden on the Overview page diff --git a/x-pack/plugins/security_solution/cypress/screens/rule_details.ts b/x-pack/plugins/security_solution/cypress/screens/rule_details.ts index 4abb1cad40d05..70c9ac4bc207d 100644 --- a/x-pack/plugins/security_solution/cypress/screens/rule_details.ts +++ b/x-pack/plugins/security_solution/cypress/screens/rule_details.ts @@ -141,3 +141,6 @@ export const THREAT_TECHNIQUE = '[data-test-subj="threatTechniqueLink"]'; export const THREAT_SUBTECHNIQUE = '[data-test-subj="threatSubtechniqueLink"]'; export const BACK_TO_RULES_TABLE = '[data-test-subj="breadcrumb"][title="Rules"]'; + +export const HIGHLIGHTED_ROWS_IN_TABLE = + '[data-test-subj="euiDataGridBody"] .alertsTableHighlightedRow'; diff --git a/x-pack/plugins/security_solution/public/detections/components/alerts_table/index.tsx b/x-pack/plugins/security_solution/public/detections/components/alerts_table/index.tsx index 44958844bae18..eb8435d7578be 100644 --- a/x-pack/plugins/security_solution/public/detections/components/alerts_table/index.tsx +++ b/x-pack/plugins/security_solution/public/detections/components/alerts_table/index.tsx @@ -12,6 +12,8 @@ import type { FC } from 'react'; import React, { useRef, useEffect, useState, useCallback, useMemo } from 'react'; import { Storage } from '@kbn/kibana-utils-plugin/public'; import type { AlertsTableStateProps } from '@kbn/triggers-actions-ui-plugin/public/application/sections/alerts_table/alerts_table_state'; +import type { Alert } from '@kbn/triggers-actions-ui-plugin/public/types'; +import { ALERT_BUILDING_BLOCK_TYPE } from '@kbn/rule-data-utils'; import styled from 'styled-components'; import { useDispatch, useSelector } from 'react-redux'; import { getEsQueryConfig } from '@kbn/data-plugin/public'; @@ -49,6 +51,9 @@ import * as i18n from './translations'; const { updateIsLoading, updateTotalCount } = dataTableActions; +// Highlight rows with building block alerts +const shouldHighlightRow = (alert: Alert) => !!alert[ALERT_BUILDING_BLOCK_TYPE]; + const storage = new Storage(localStorage); interface GridContainerProps { @@ -255,6 +260,7 @@ export const AlertsTableComponent: FC = ({ query: finalBoolQuery, showExpandToDetails: false, gridStyle, + shouldHighlightRow, rowHeightsOptions, columns: finalColumns, browserFields: finalBrowserFields, diff --git a/x-pack/plugins/triggers_actions_ui/public/application/sections/alerts_table/alerts_table.scss b/x-pack/plugins/triggers_actions_ui/public/application/sections/alerts_table/alerts_table.scss index fa62ba0307bf0..672ffa2b9d36f 100644 --- a/x-pack/plugins/triggers_actions_ui/public/application/sections/alerts_table/alerts_table.scss +++ b/x-pack/plugins/triggers_actions_ui/public/application/sections/alerts_table/alerts_table.scss @@ -1,3 +1,7 @@ +.alertsTableHighlightedRow { + background-color: $euiColorHighlight; +} + .alertsTableActiveRow { background-color: tintOrShade($euiColorLightShade, 50%, 0); } diff --git a/x-pack/plugins/triggers_actions_ui/public/application/sections/alerts_table/alerts_table.tsx b/x-pack/plugins/triggers_actions_ui/public/application/sections/alerts_table/alerts_table.tsx index e01000507c882..3612beb93102c 100644 --- a/x-pack/plugins/triggers_actions_ui/public/application/sections/alerts_table/alerts_table.tsx +++ b/x-pack/plugins/triggers_actions_ui/public/application/sections/alerts_table/alerts_table.tsx @@ -33,10 +33,8 @@ import { SystemCellId } from './types'; import { SystemCellFactory, systemCells } from './cells'; import { triggersActionsUiQueriesKeys } from '../../hooks/constants'; -export const ACTIVE_ROW_CLASS = 'alertsTableActiveRow'; - const AlertsFlyout = lazy(() => import('./alerts_flyout')); -const GridStyles: EuiDataGridStyle = { +const DefaultGridStyle: EuiDataGridStyle = { border: 'none', header: 'underline', fontSize: 's', @@ -63,7 +61,9 @@ const isSystemCell = (columnId: string): columnId is SystemCellId => { const AlertsTable: React.FunctionComponent = (props: AlertsTableProps) => { const dataGridRef = useRef(null); - const [_, setRowClasses] = useState({}); + const [activeRowClasses, setActiveRowClasses] = useState< + NonNullable + >({}); const alertsData = props.useFetchAlertsData(); const { activePage, @@ -286,11 +286,30 @@ const AlertsTable: React.FunctionComponent = (props: AlertsTab useEffect(() => { // Row classes do not deal with visible row indices, so we need to handle page offset const rowIndex = flyoutAlertIndex + pagination.pageIndex * pagination.pageSize; - setRowClasses({ - [rowIndex]: ACTIVE_ROW_CLASS, + setActiveRowClasses({ + [rowIndex]: 'alertsTableActiveRow', }); }, [flyoutAlertIndex, pagination.pageIndex, pagination.pageSize]); + // Update highlighted rows when alerts or pagination changes + const highlightedRowClasses = useMemo(() => { + let mappedRowClasses: EuiDataGridStyle['rowClasses'] = {}; + const shouldHighlightRowCheck = props.shouldHighlightRow; + if (shouldHighlightRowCheck) { + mappedRowClasses = alerts.reduce>( + (rowClasses, alert, index) => { + if (shouldHighlightRowCheck(alert)) { + rowClasses[index + pagination.pageIndex * pagination.pageSize] = + 'alertsTableHighlightedRow'; + } + return rowClasses; + }, + {} + ); + } + return mappedRowClasses; + }, [props.shouldHighlightRow, alerts, pagination.pageIndex, pagination.pageSize]); + const handleFlyoutClose = useCallback(() => setFlyoutAlertIndex(-1), [setFlyoutAlertIndex]); const renderCellValue = useCallback( @@ -377,6 +396,47 @@ const AlertsTable: React.FunctionComponent = (props: AlertsTab return props.columns; }, [getCellActions, disabledCellActions, props.columns, visibleCellActions]); + // Merges the default grid style with the grid style that comes in through props. + const actualGridStyle = useMemo(() => { + const propGridStyle: NonNullable = props.gridStyle ?? {}; + // Merges default row classes, custom ones and adds the active row class style + const mergedGridStyle: EuiDataGridStyle = { + ...DefaultGridStyle, + ...propGridStyle, + rowClasses: { + // We're spreadind the highlighted row classes first, so that the active + // row classed can override the highlighted row classes. + ...highlightedRowClasses, + ...activeRowClasses, + }, + }; + + // If ANY additional rowClasses have been provided, we need to merge them with our internal ones + if (propGridStyle.rowClasses) { + // Get all row indices with a rowClass. + const mergedKeys = [ + ...Object.keys(mergedGridStyle.rowClasses || {}), + ...Object.keys(propGridStyle.rowClasses || {}), + ]; + // Deduplicate keys to avoid extra iterations + const dedupedKeys = Array.from(new Set(mergedKeys)); + + // For each index, merge row classes + const mergedRowClasses = dedupedKeys.reduce>( + (rowClasses, key) => { + const intKey = parseInt(key, 10); + // Use internal row classes over custom row classes. + rowClasses[intKey] = + mergedGridStyle.rowClasses?.[intKey] || propGridStyle.rowClasses?.[intKey] || ''; + return rowClasses; + }, + {} + ); + mergedGridStyle.rowClasses = mergedRowClasses; + } + return mergedGridStyle; + }, [activeRowClasses, highlightedRowClasses, props.gridStyle]); + return (
@@ -404,7 +464,7 @@ const AlertsTable: React.FunctionComponent = (props: AlertsTab leadingControlColumns={leadingControlColumns} rowCount={alertsCount} renderCellValue={handleRenderCellValue} - gridStyle={{ ...GridStyles, ...(props.gridStyle ?? {}) }} + gridStyle={actualGridStyle} sorting={{ columns: sortingColumns, onSort }} toolbarVisibility={toolbarVisibility} pagination={{ diff --git a/x-pack/plugins/triggers_actions_ui/public/application/sections/alerts_table/alerts_table_state.tsx b/x-pack/plugins/triggers_actions_ui/public/application/sections/alerts_table/alerts_table_state.tsx index d9353516c6472..e9123776a2144 100644 --- a/x-pack/plugins/triggers_actions_ui/public/application/sections/alerts_table/alerts_table_state.tsx +++ b/x-pack/plugins/triggers_actions_ui/public/application/sections/alerts_table/alerts_table_state.tsx @@ -70,6 +70,10 @@ export type AlertsTableStateProps = { onUpdate?: (args: TableUpdateHandlerArgs) => void; showAlertStatusWithFlapping?: boolean; toolbarVisibility?: EuiDataGridToolBarVisibilityOptions; + /** + * Allows to consumers of the table to decide to highlight a row based on the current alert. + */ + shouldHighlightRow?: (alert: Alert) => boolean; } & Partial; export interface AlertsTableStorage { @@ -138,6 +142,7 @@ const AlertsTableStateWithQueryProvider = ({ onUpdate, showAlertStatusWithFlapping, toolbarVisibility, + shouldHighlightRow, }: AlertsTableStateProps) => { const { cases: casesService } = useKibana<{ cases?: CasesService }>().services; @@ -353,6 +358,7 @@ const AlertsTableStateWithQueryProvider = ({ controls: persistentControls, showInspectButton, toolbarVisibility, + shouldHighlightRow, }), [ alertsTableConfiguration, @@ -380,6 +386,7 @@ const AlertsTableStateWithQueryProvider = ({ persistentControls, showInspectButton, toolbarVisibility, + shouldHighlightRow, ] ); diff --git a/x-pack/plugins/triggers_actions_ui/public/types.ts b/x-pack/plugins/triggers_actions_ui/public/types.ts index 756ea14488fad..4c7d743cb3085 100644 --- a/x-pack/plugins/triggers_actions_ui/public/types.ts +++ b/x-pack/plugins/triggers_actions_ui/public/types.ts @@ -532,6 +532,10 @@ export type AlertsTableProps = { controls?: EuiDataGridToolBarAdditionalControlsOptions; showInspectButton?: boolean; toolbarVisibility?: EuiDataGridToolBarVisibilityOptions; + /** + * Allows to consumers of the table to decide to highlight a row based on the current alert. + */ + shouldHighlightRow?: (alert: Alert) => boolean; } & Partial>; // TODO We need to create generic type between our plugin, right now we have different one because of the old alerts table