Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Security Solution][Exceptions] - Fix bug of alerts not updating after closure from exceptions modal #76145

Merged
merged 3 commits into from
Aug 28, 2020
Merged

[Security Solution][Exceptions] - Fix bug of alerts not updating after closure from exceptions modal #76145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7cf701a
fix bug of alerts not updating after closure from exceptions modal
yctercero Aug 27, 2020
15b89cf
no functional changes, just cleanup
yctercero Aug 27, 2020
77b8b33
Merge branch 'master' into update_alerts
elasticmachine Aug 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions ...k/plugins/security_solution/public/common/components/events_viewer/events_viewer.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ const utilityBar = (refetch: inputsModel.Refetch, totalCount: number) => (
<div data-test-subj="mock-utility-bar" />
);

const exceptionsModal = (refetch: inputsModel.Refetch) => (
<div data-test-subj="mock-exceptions-modal" />
);

const eventsViewerDefaultProps = {
browserFields: {},
columns: [],
Expand Down Expand Up @@ -460,4 +464,42 @@ describe('EventsViewer', () => {
});
});
});

describe('exceptions modal', () => {
test('it renders exception modal if "exceptionsModal" callback exists', async () => {
const wrapper = mount(
<TestProviders>
<MockedProvider mocks={mockEventViewerResponse} addTypename={false}>
<EventsViewer
{...eventsViewerDefaultProps}
exceptionsModal={exceptionsModal}
graphEventId=""
/>
</MockedProvider>
</TestProviders>
);

await waitFor(() => {
wrapper.update();

expect(wrapper.find(`[data-test-subj="mock-exceptions-modal"]`).exists()).toBeTruthy();
});
});

test('it does not render exception modal if "exceptionModal" callback does not exist', async () => {
Copy link
Contributor

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

Copy link
Contributor Author

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

const wrapper = mount(
<TestProviders>
<MockedProvider mocks={mockEventViewerResponse} addTypename={false}>
<EventsViewer {...eventsViewerDefaultProps} graphEventId="" />
</MockedProvider>
</TestProviders>
);

await waitFor(() => {
wrapper.update();

expect(wrapper.find(`[data-test-subj="mock-exceptions-modal"]`).exists()).toBeFalsy();
});
});
});
});
6 changes: 5 additions & 1 deletion x-pack/plugins/security_solution/public/common/components/events_viewer/events_viewer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ interface Props {
utilityBar?: (refetch: inputsModel.Refetch, totalCount: number) => React.ReactNode;
// If truthy, the graph viewer (Resolver) is showing
graphEventId: string | undefined;
exceptionsModal?: (refetch: inputsModel.Refetch) => React.ReactNode;
}

const EventsViewerComponent: React.FC<Props> = ({
Expand All @@ -134,6 +135,7 @@ const EventsViewerComponent: React.FC<Props> = ({
toggleColumn,
utilityBar,
graphEventId,
exceptionsModal,
}) => {
const { globalFullScreen } = useFullScreen();
const columnsHeader = isEmpty(columns) ? defaultHeaders : columns;
Expand Down Expand Up @@ -259,6 +261,7 @@ const EventsViewerComponent: React.FC<Props> = ({
</HeaderFilterGroupWrapper>
)}
</HeaderSection>
{exceptionsModal && exceptionsModal(refetch)}
Copy link
Contributor Author

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.

{utilityBar && !resolverIsShowing(graphEventId) && (
<UtilityBar>{utilityBar?.(refetch, totalCountMinusDeleted)}</UtilityBar>
)}
Expand Down Expand Up @@ -335,5 +338,6 @@ export const EventsViewer = React.memo(
prevProps.start === nextProps.start &&
prevProps.sort === nextProps.sort &&
prevProps.utilityBar === nextProps.utilityBar &&
prevProps.graphEventId === nextProps.graphEventId
prevProps.graphEventId === nextProps.graphEventId &&
prevProps.exceptionsModal === nextProps.exceptionsModal
);
7 changes: 6 additions & 1 deletion x-pack/plugins/security_solution/public/common/components/events_viewer/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ export interface OwnProps {
headerFilterGroup?: React.ReactNode;
pageFilters?: Filter[];
utilityBar?: (refetch: inputsModel.Refetch, totalCount: number) => React.ReactNode;
exceptionsModal?: (refetch: inputsModel.Refetch) => React.ReactNode;
}

type Props = OwnProps & PropsFromRedux;
Expand Down Expand Up @@ -74,6 +75,7 @@ const StatefulEventsViewerComponent: React.FC<Props> = ({
utilityBar,
// If truthy, the graph viewer (Resolver) is showing
graphEventId,
exceptionsModal,
}) => {
const [
{ docValueFields, browserFields, indexPatterns, isLoading: isLoadingIndexPattern },
Expand Down Expand Up @@ -156,6 +158,7 @@ const StatefulEventsViewerComponent: React.FC<Props> = ({
toggleColumn={toggleColumn}
utilityBar={utilityBar}
graphEventId={graphEventId}
exceptionsModal={exceptionsModal}
/>
</InspectButtonContainer>
</FullScreenContainer>
Expand Down Expand Up @@ -220,6 +223,7 @@ type PropsFromRedux = ConnectedProps<typeof connector>;
export const StatefulEventsViewer = connector(
React.memo(
StatefulEventsViewerComponent,
// eslint-disable-next-line complexity
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😢

Copy link
Contributor

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.

(prevProps, nextProps) =>
prevProps.id === nextProps.id &&
deepEqual(prevProps.columns, nextProps.columns) &&
Expand All @@ -240,6 +244,7 @@ export const StatefulEventsViewer = connector(
prevProps.showCheckboxes === nextProps.showCheckboxes &&
prevProps.start === nextProps.start &&
prevProps.utilityBar === nextProps.utilityBar &&
prevProps.graphEventId === nextProps.graphEventId
prevProps.graphEventId === nextProps.graphEventId &&
prevProps.exceptionsModal === nextProps.exceptionsModal
)
);
55 changes: 39 additions & 16 deletions x-pack/plugins/security_solution/public/detections/components/alerts_table/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ export const AlertsTableComponent: React.FC<AlertsTableComponentProps> = ({
exceptionListType,
alertData,
}: AddExceptionModalBaseProps) => {
if (alertData !== null && alertData !== undefined) {
if (alertData != null) {
Copy link
Contributor

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 👍

setShouldShowAddExceptionModal(true);
setAddExceptionModalState({
ruleName,
Expand Down Expand Up @@ -441,9 +441,43 @@ export const AlertsTableComponent: React.FC<AlertsTableComponentProps> = ({
closeAddExceptionModal();
}, [closeAddExceptionModal]);

const onAddExceptionConfirm = useCallback(() => closeAddExceptionModal(), [
closeAddExceptionModal,
]);
const onAddExceptionConfirm = useCallback(
(refetch: inputsModel.Refetch) => (): void => {
refetch();
closeAddExceptionModal();
},
[closeAddExceptionModal]
);

// Callback for creating the AddExceptionModal and allowing it
// access to the refetchQuery to update the page
const exceptionModalCallback = useCallback(
(refetchQuery: inputsModel.Refetch) => {
if (shouldShowAddExceptionModal) {
return (
<AddExceptionModal
ruleName={addExceptionModalState.ruleName}
ruleId={addExceptionModalState.ruleId}
ruleIndices={addExceptionModalState.ruleIndices}
exceptionListType={addExceptionModalState.exceptionListType}
alertData={addExceptionModalState.alertData}
onCancel={onAddExceptionCancel}
onConfirm={onAddExceptionConfirm(refetchQuery)}
alertStatus={filterGroup}
/>
);
} else {
return <></>;
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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

}
},
[
addExceptionModalState,
filterGroup,
onAddExceptionCancel,
onAddExceptionConfirm,
shouldShowAddExceptionModal,
]
);

if (loading || indexPatternsLoading || isEmpty(signalsIndex)) {
return (
Expand All @@ -465,19 +499,8 @@ export const AlertsTableComponent: React.FC<AlertsTableComponentProps> = ({
id={timelineId}
start={from}
utilityBar={utilityBarCallback}
exceptionsModal={exceptionModalCallback}
/>
{shouldShowAddExceptionModal === true && addExceptionModalState.alertData !== null && (
<AddExceptionModal
ruleName={addExceptionModalState.ruleName}
ruleId={addExceptionModalState.ruleId}
ruleIndices={addExceptionModalState.ruleIndices}
exceptionListType={addExceptionModalState.exceptionListType}
alertData={addExceptionModalState.alertData}
onCancel={onAddExceptionCancel}
onConfirm={onAddExceptionConfirm}
alertStatus={filterGroup}
/>
)}
</>
);
};
Expand Down