-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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][Endpoint][Response Actions] Fix table navigation when trays are expanded #157777
[Security Solution][Endpoint][Response Actions] Fix table navigation when trays are expanded #157777
Changes from 15 commits
fa0c798
890eb1d
9e743e2
cf8dbfc
3bc094b
08ffdc7
2801a75
cbf0aa1
b6f3bbe
c09b974
bdac04c
a173f3e
df6d583
fd389bf
e3b26f4
555a9bf
ca1919b
50e8995
82b4a74
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,7 @@ import { | |
EuiToolTip, | ||
type HorizontalAlignment, | ||
type CriteriaWithPagination, | ||
EuiSkeletonText, | ||
} from '@elastic/eui'; | ||
import { euiStyled } from '@kbn/kibana-react-plugin/common'; | ||
import { FormattedMessage } from '@kbn/i18n-react'; | ||
|
@@ -55,12 +56,12 @@ interface ExpandedRowMapType { | |
|
||
const getResponseActionListTableColumns = ({ | ||
getTestId, | ||
itemIdToExpandedRowMap, | ||
expandedRowMap, | ||
showHostNames, | ||
onClickCallback, | ||
}: { | ||
getTestId: (suffix?: string | undefined) => string | undefined; | ||
itemIdToExpandedRowMap: ExpandedRowMapType; | ||
expandedRowMap: ExpandedRowMapType; | ||
showHostNames: boolean; | ||
onClickCallback: (actionListDataItem: ActionListApiResponse['data'][number]) => () => void; | ||
}) => { | ||
|
@@ -245,10 +246,8 @@ const getResponseActionListTableColumns = ({ | |
<EuiButtonIcon | ||
data-test-subj={getTestId('expand-button')} | ||
onClick={onClickCallback(actionListDataItem)} | ||
aria-label={ | ||
itemIdToExpandedRowMap[actionId] ? ARIA_LABELS.collapse : ARIA_LABELS.expand | ||
} | ||
iconType={itemIdToExpandedRowMap[actionId] ? 'arrowUp' : 'arrowDown'} | ||
aria-label={expandedRowMap[actionId] ? ARIA_LABELS.collapse : ARIA_LABELS.expand} | ||
iconType={expandedRowMap[actionId] ? 'arrowUp' : 'arrowDown'} | ||
/> | ||
); | ||
}, | ||
|
@@ -290,13 +289,13 @@ export const ActionsLogTable = memo<ActionsLogTableProps>( | |
showHostNames, | ||
totalItemCount, | ||
}) => { | ||
const [itemIdToExpandedRowMap, setItemIdToExpandedRowMap] = useState<ExpandedRowMapType>({}); | ||
|
||
const getTestId = useTestIdGenerator(dataTestSubj); | ||
const { pagination: paginationFromUrlParams } = useUrlPagination(); | ||
const { withOutputs: withOutputsFromUrl } = useActionHistoryUrlParams(); | ||
|
||
const getActionIdsWithDetails = useCallback((): string[] => { | ||
const [expandedRowMap, setExpandedRowMap] = useState<ExpandedRowMapType>({}); | ||
|
||
const actionIdsWithOpenTrays = useMemo((): string[] => { | ||
// get the list of action ids from URL params on the history page | ||
if (!isFlyout) { | ||
return withOutputsFromUrl ?? []; | ||
|
@@ -309,40 +308,48 @@ export const ActionsLogTable = memo<ActionsLogTableProps>( | |
: []; | ||
}, [isFlyout, queryParams.withOutputs, withOutputsFromUrl]); | ||
|
||
const redoOpenTrays = useCallback(() => { | ||
if (actionIdsWithOpenTrays.length && items.length) { | ||
const openDetails = actionIdsWithOpenTrays.reduce<ExpandedRowMapType>( | ||
(idToRowMap, actionId) => { | ||
const actionItem = items.find((item) => item.id === actionId); | ||
if (!actionItem) { | ||
idToRowMap[actionId] = <EuiSkeletonText size="relative" lines={8} />; | ||
} else { | ||
Comment on lines
+316
to
+318
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This case was not handled earlier and adding this change fixes the issue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good fix 👍 |
||
idToRowMap[actionId] = ( | ||
<ActionsLogExpandedTray action={actionItem} data-test-subj={dataTestSubj} /> | ||
); | ||
ashokaditya marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
return idToRowMap; | ||
}, | ||
{} | ||
); | ||
setExpandedRowMap(openDetails); | ||
} | ||
}, [actionIdsWithOpenTrays, dataTestSubj, items]); | ||
|
||
// open trays that were open using URL params/ query params | ||
useEffect(() => { | ||
const actionIdsWithDetails = getActionIdsWithDetails(); | ||
const openDetails = actionIdsWithDetails.reduce<ExpandedRowMapType>( | ||
(idToRowMap, actionId) => { | ||
idToRowMap[actionId] = ( | ||
<ActionsLogExpandedTray | ||
action={items.filter((item) => item.id === actionId)[0]} | ||
data-test-subj={dataTestSubj} | ||
/> | ||
); | ||
return idToRowMap; | ||
}, | ||
{} | ||
); | ||
setItemIdToExpandedRowMap(openDetails); | ||
}, [dataTestSubj, getActionIdsWithDetails, items, queryParams.withOutputs, withOutputsFromUrl]); | ||
redoOpenTrays(); | ||
}, [redoOpenTrays]); | ||
|
||
const toggleDetails = useCallback( | ||
(action: ActionListApiResponse['data'][number]) => { | ||
const itemIdToExpandedRowMapValues = { ...itemIdToExpandedRowMap }; | ||
if (itemIdToExpandedRowMapValues[action.id]) { | ||
const expandedRowMapCopy = { ...expandedRowMap }; | ||
if (expandedRowMapCopy[action.id]) { | ||
// close tray | ||
delete itemIdToExpandedRowMapValues[action.id]; | ||
delete expandedRowMapCopy[action.id]; | ||
} else { | ||
// assign the expanded tray content to the map | ||
// with action details | ||
itemIdToExpandedRowMapValues[action.id] = ( | ||
expandedRowMapCopy[action.id] = ( | ||
<ActionsLogExpandedTray action={action} data-test-subj={dataTestSubj} /> | ||
); | ||
} | ||
onShowActionDetails(Object.keys(itemIdToExpandedRowMapValues)); | ||
setItemIdToExpandedRowMap(itemIdToExpandedRowMapValues); | ||
onShowActionDetails(Object.keys(expandedRowMapCopy)); | ||
setExpandedRowMap(expandedRowMapCopy); | ||
}, | ||
[itemIdToExpandedRowMap, onShowActionDetails, dataTestSubj] | ||
[expandedRowMap, onShowActionDetails, dataTestSubj] | ||
); | ||
|
||
// memoized callback for toggleDetails | ||
|
@@ -409,11 +416,11 @@ export const ActionsLogTable = memo<ActionsLogTableProps>( | |
() => | ||
getResponseActionListTableColumns({ | ||
getTestId, | ||
itemIdToExpandedRowMap, | ||
expandedRowMap, | ||
onClickCallback, | ||
showHostNames, | ||
}), | ||
[itemIdToExpandedRowMap, getTestId, onClickCallback, showHostNames] | ||
[expandedRowMap, getTestId, onClickCallback, showHostNames] | ||
); | ||
|
||
return ( | ||
|
@@ -425,7 +432,7 @@ export const ActionsLogTable = memo<ActionsLogTableProps>( | |
items={items} | ||
columns={columns} | ||
itemId="id" | ||
itemIdToExpandedRowMap={itemIdToExpandedRowMap} | ||
itemIdToExpandedRowMap={expandedRowMap} | ||
isExpandable | ||
pagination={tablePagination} | ||
onChange={onChange} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -226,7 +226,7 @@ export const ResponseActionsLog = memo< | |
setUrlWithOutputs(actionIds.join()); | ||
} | ||
}, | ||
[isFlyout, setUrlWithOutputs] | ||
[isFlyout, setUrlWithOutputs, setQueryParams] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was not related to the bug, only a missing dependency |
||
); | ||
|
||
if (error?.body?.statusCode === 404 && error?.body?.message === 'index_not_found_exception') { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ | ||
|
||
import type { ReturnTypeFromChainable } from '../../types'; | ||
import { indexEndpointHosts } from '../../tasks/index_endpoint_hosts'; | ||
import { login } from '../../tasks/login'; | ||
|
||
describe('Response actions history page', () => { | ||
let endpointData: ReturnTypeFromChainable<typeof indexEndpointHosts>; | ||
// let actionData: ReturnTypeFromChainable<typeof indexActionResponses>; | ||
|
||
before(() => { | ||
indexEndpointHosts().then((indexEndpoints) => { | ||
endpointData = indexEndpoints; | ||
}); | ||
}); | ||
|
||
beforeEach(() => { | ||
login(); | ||
}); | ||
|
||
after(() => { | ||
if (endpointData) { | ||
endpointData.cleanup(); | ||
// @ts-expect-error ignore setting to undefined | ||
endpointData = undefined; | ||
} | ||
}); | ||
|
||
it('retains expanded action details on page navigation', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm ok if you leave this as is, but curious: aren't these tests already covered with Jest Unit? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, you're right! I also covered this in jest. I'll remove this one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
cy.visit(`/app/security/administration/response_actions_history`); | ||
cy.getByTestSubj('response-actions-list-expand-button').first().click(); // 1st row | ||
cy.getByTestSubj('response-actions-list-expand-button').eq(2).click(); // 3rd row | ||
cy.getByTestSubj('response-actions-list-details-tray').should('exist'); | ||
|
||
// on page 2 | ||
cy.getByTestSubj('pagination-button-1').click(); | ||
cy.getByTestSubj('response-actions-list-details-tray').not('exist'); | ||
// page back to 1 | ||
cy.getByTestSubj('pagination-button-0').click(); | ||
cy.getByTestSubj('response-actions-list-details-tray').should('exist'); | ||
cy.get('button[aria-label="Collapse"]').should('have.length', 2); | ||
}); | ||
|
||
it('retains expanded action details on page reload', () => { | ||
cy.visit(`/app/security/administration/response_actions_history`); | ||
cy.getByTestSubj('response-actions-list-expand-button').eq(3).click(); // 4th row on 1st page | ||
cy.getByTestSubj('response-actions-list-details-tray').should('exist'); | ||
cy.url().should('include', 'withOutputs'); | ||
|
||
// navigate to page 2 | ||
cy.getByTestSubj('pagination-button-1').click(); | ||
cy.getByTestSubj('response-actions-list-details-tray').should('not.exist'); | ||
|
||
// reload with URL params on page 2 with existing URL | ||
cy.reload(); | ||
cy.getByTestSubj('response-actions-list-details-tray').should('not.exist'); | ||
|
||
// navigate to page 1 | ||
cy.getByTestSubj('pagination-button-0').click(); | ||
cy.getByTestSubj('response-actions-list-details-tray').should('exist'); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -343,6 +343,43 @@ describe('Response actions history page', () => { | |
); | ||
expect(history.location.search).toEqual(`?startDate=${startDate}&endDate=${endDate}`); | ||
}); | ||
|
||
it('should read and expand actions using `withOutputs`', () => { | ||
const allActionIds = mockUseGetEndpointActionList.data?.data.map((action) => action.id) ?? []; | ||
// select 5 actions to show details | ||
const actionIdsWithDetails = allActionIds.filter((_, i) => [0, 2, 3, 4, 5].includes(i)); | ||
reactTestingLibrary.act(() => { | ||
// load page 1 but with expanded actions. | ||
history.push( | ||
`/administration/response_actions_history?withOutputs=${actionIdsWithDetails.join( | ||
',' | ||
)}&page=1&pageSize=10` | ||
); | ||
}); | ||
|
||
const { getByTestId, getAllByTestId } = render(); | ||
|
||
// verify on page 1 | ||
expect(getByTestId(`${testPrefix}-endpointListTableTotal`)).toHaveTextContent( | ||
'Showing 1-10 of 43 response actions' | ||
); | ||
|
||
const traysOnPage1 = getAllByTestId(`${testPrefix}-details-tray`); | ||
const expandButtonsOnPage1 = getAllByTestId(`${testPrefix}-expand-button`); | ||
const expandedButtons = expandButtonsOnPage1.reduce<number[]>((acc, button, i) => { | ||
// find expanded rows | ||
if (button.getAttribute('aria-label') === 'Collapse') { | ||
acc.push(i); | ||
} | ||
return acc; | ||
}, []); | ||
|
||
// verify 5 rows are expanded | ||
expect(traysOnPage1).toBeTruthy(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is always There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh indeed. I'll update it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
expect(traysOnPage1.length).toEqual(5); | ||
// verify 5 rows that are expanded are the ones from before | ||
expect(expandedButtons).toEqual([0, 2, 3, 4, 5]); | ||
}); | ||
}); | ||
|
||
describe('Set selected/set values to URL params', () => { | ||
|
@@ -442,5 +479,33 @@ describe('Response actions history page', () => { | |
|
||
expect(history.location.search).toEqual('?endDate=now&startDate=now-15m'); | ||
}); | ||
|
||
it('should set actionIds using `withOutputs` to URL params ', async () => { | ||
const allActionIds = mockUseGetEndpointActionList.data?.data.map((action) => action.id) ?? []; | ||
const actionIdsWithDetails = allActionIds | ||
.reduce<string[]>((acc, e, i) => { | ||
if ([0, 1].includes(i)) { | ||
acc.push(e); | ||
} | ||
return acc; | ||
}, []) | ||
.join() | ||
.split(',') | ||
.join('%2C'); | ||
|
||
render(); | ||
const { getAllByTestId } = renderResult; | ||
|
||
const expandButtons = getAllByTestId(`${testPrefix}-expand-button`); | ||
// expand some rows | ||
expandButtons.forEach((button, i) => { | ||
if ([0, 1].includes(i)) { | ||
userEvent.click(button); | ||
} | ||
}); | ||
|
||
// verify 2 rows are expanded and are the ones from before | ||
expect(history.location.search).toEqual(`?withOutputs=${actionIdsWithDetails}`); | ||
}); | ||
}); | ||
}); |
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.
Do we really want 11 actions to be index every time the generator runs? Remember that this is used in other use cases outside of your test.
Suggestion: consider adding an
options
argument to this function and introduce acount
parameter that drives how many to create and then use that from your 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.
Yeah, adding options is a good idea.
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.
ca1919b