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

Hide timeline footer when Resolver is open #71516

Merged
merged 9 commits into from
Jul 14, 2020
Merged

Hide timeline footer when Resolver is open #71516

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
32dc074
Hide the Timeline footer, in the event viewer, if Resolver is showing
Jul 13, 2020
1ad5679
Hide the Timeline footer, in the timeline flyout, if Resolver is showing
Jul 13, 2020
0e3ad6f
fixup: `getTimeline` actually returns `TimelineModel | undefined`
Jul 13, 2020
8a75f79
code style
Jul 13, 2020
a4a496b
Add tests. Remove showGraphView helper
Jul 13, 2020
7e5ed4f
mock GraphOverlay so that Resolver doesnt mount. Check the visiblity …
Jul 13, 2020
68c0e91
comments
Jul 13, 2020
b0b048b
comments
Jul 13, 2020
96e642e
test uses data-test-subj instead of component reference
Jul 13, 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
44 changes: 27 additions & 17 deletions 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 @@ -67,6 +67,8 @@ interface Props {
sort: Sort;
toggleColumn: (column: ColumnHeaderOptions) => void;
utilityBar?: (refetch: inputsModel.Refetch, totalCount: number) => React.ReactNode;
// If truthy, the graph viewer (Resolver) is showing
Copy link
Contributor

Choose a reason for hiding this comment

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

❔ Should this come from a selector like shouldShowResolver?

Copy link
Contributor

Choose a reason for hiding this comment

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

I only ask because I've done sparse business logic like this before and have been asked to move it to a selector to make it more readable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see later maybe there is a selector? It's not clear from reading this the way the comment is written.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm changing some existing code here, but my thoughts:

  • There is a selector (that this uses) that gets the TimelineModel.
  • TimelineModel has graphEventId as a field
  • If graphEventId is falsy, there is no 'graphEvent' (aka resolver)
  • If graphEventId ts truthy, the value is the _id of the document that Resolver should use.

The existing code doesn't have a specific selector for graphEventId. Instead it exposes the TimelineModel interface throughout the view. If there was a desire to encapsulate the logic of "given a TimelineModel, how do I know if the graph view should show" then I would add a method to the TimelineModel. I don't think that would fit the code style of the SIEM team, but I'm open to changing it if they want that.

graphEventId: string | undefined;
}

const EventsViewerComponent: React.FC<Props> = ({
Expand All @@ -90,6 +92,7 @@ const EventsViewerComponent: React.FC<Props> = ({
sort,
toggleColumn,
utilityBar,
graphEventId,
}) => {
const columnsHeader = isEmpty(columns) ? defaultHeaders : columns;
const kibana = useKibana();
Expand Down Expand Up @@ -191,22 +194,28 @@ const EventsViewerComponent: React.FC<Props> = ({
toggleColumn={toggleColumn}
/>

<Footer
getUpdatedAt={getUpdatedAt}
hasNextPage={getOr(false, 'hasNextPage', pageInfo)!}
height={footerHeight}
id={id}
isLive={isLive}
isLoading={loading}
itemsCount={events.length}
itemsPerPage={itemsPerPage}
itemsPerPageOptions={itemsPerPageOptions}
onChangeItemsPerPage={onChangeItemsPerPage}
onLoadMore={loadMore}
nextCursor={getOr(null, 'endCursor.value', pageInfo)!}
serverSideEventCount={totalCountMinusDeleted}
tieBreaker={getOr(null, 'endCursor.tiebreaker', pageInfo)}
/>
{
/** Hide the footer if Resolver is showing. */
Copy link
Contributor Author

@oatkiller oatkiller Jul 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Add a guard statement and remove data-test-subj

!graphEventId && (
<Footer
data-test-subj="events-viewer-footer"
getUpdatedAt={getUpdatedAt}
hasNextPage={getOr(false, 'hasNextPage', pageInfo)!}
height={footerHeight}
id={id}
isLive={isLive}
isLoading={loading}
itemsCount={events.length}
itemsPerPage={itemsPerPage}
itemsPerPageOptions={itemsPerPageOptions}
onChangeItemsPerPage={onChangeItemsPerPage}
onLoadMore={loadMore}
nextCursor={getOr(null, 'endCursor.value', pageInfo)!}
serverSideEventCount={totalCountMinusDeleted}
tieBreaker={getOr(null, 'endCursor.tiebreaker', pageInfo)}
/>
)
}
</EventsContainerLoading>
</>
);
Expand Down Expand Up @@ -237,5 +246,6 @@ export const EventsViewer = React.memo(
deepEqual(prevProps.query, nextProps.query) &&
prevProps.start === nextProps.start &&
prevProps.sort === nextProps.sort &&
prevProps.utilityBar === nextProps.utilityBar
prevProps.utilityBar === nextProps.utilityBar &&
prevProps.graphEventId === nextProps.graphEventId
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 seems to be required for it to work.

);
10 changes: 9 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 @@ -62,6 +62,8 @@ const StatefulEventsViewerComponent: React.FC<Props> = ({
updateItemsPerPage,
upsertColumn,
utilityBar,
// If truthy, the graph viewer (Resolver) is showing
graphEventId,
}) => {
const [{ browserFields, indexPatterns }] = useFetchIndexPatterns(
defaultIndices ?? useUiSetting<string[]>(DEFAULT_INDEX_KEY)
Expand Down Expand Up @@ -135,6 +137,7 @@ const StatefulEventsViewerComponent: React.FC<Props> = ({
sort={sort}
toggleColumn={toggleColumn}
utilityBar={utilityBar}
graphEventId={graphEventId}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pass the new prop to the unconnected event viewer

/>
</InspectButtonContainer>
);
Expand All @@ -145,6 +148,7 @@ const makeMapStateToProps = () => {
const getGlobalQuerySelector = inputsSelectors.globalQuerySelector();
const getGlobalFiltersQuerySelector = inputsSelectors.globalFiltersQuerySelector();
const getEvents = timelineSelectors.getEventsByIdSelector();
const getTimeline = timelineSelectors.getTimelineByIdSelector();
const mapStateToProps = (state: State, { id, defaultModel }: OwnProps) => {
const input: inputsModel.InputsRange = getInputsTimeline(state);
const events: TimelineModel = getEvents(state, id) ?? defaultModel;
Expand Down Expand Up @@ -174,6 +178,9 @@ const makeMapStateToProps = () => {
query: getGlobalQuerySelector(state),
sort,
showCheckboxes,
// Used to determine whether the footer should show (since it is hidden if the graph is showing.)
// `getTimeline` actually returns `TimelineModel | undefined`
graphEventId: (getTimeline(state, id) as TimelineModel | undefined)?.graphEventId,
};
};
return mapStateToProps;
Expand Down Expand Up @@ -213,6 +220,7 @@ export const StatefulEventsViewer = connector(
deepEqual(prevProps.pageFilters, nextProps.pageFilters) &&
prevProps.showCheckboxes === nextProps.showCheckboxes &&
prevProps.start === nextProps.start &&
prevProps.utilityBar === nextProps.utilityBar
prevProps.utilityBar === nextProps.utilityBar &&
prevProps.graphEventId === nextProps.graphEventId
Copy link
Contributor Author

Choose a reason for hiding this comment

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

required for rendering to work correctly

)
);
3 changes: 0 additions & 3 deletions x-pack/plugins/security_solution/public/timelines/components/timeline/body/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,6 @@ export const getEventType = (event: Ecs): Omit<EventType, 'all'> => {
return 'raw';
};

export const showGraphView = (graphEventId?: string) =>
Copy link
Contributor Author

@oatkiller oatkiller Jul 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I removed this function. Instead i'm just using the graphEventIds truthy/falsiness. The extra abstraction didn't seem helpful.

graphEventId != null && graphEventId.length > 0;

export const isInvestigateInResolverActionEnabled = (ecsData?: Ecs) => {
return (
get(['agent', 'type', 0], ecsData) === 'endpoint' &&
Expand Down
20 changes: 19 additions & 1 deletion x-pack/plugins/security_solution/public/timelines/components/timeline/body/index.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import { columnRenderers, rowRenderers } from './renderers';
import { Sort } from './sort';
import { wait } from '../../../../common/lib/helpers';
import { useMountAppended } from '../../../../common/utils/use_mount_appended';
import { SELECTOR_TIMELINE_BODY_CLASS_NAME } from '../styles';
import { SELECTOR_TIMELINE_BODY_CLASS_NAME, TimelineBody } from '../styles';

const testBodyHeight = 700;
const mockGetNotesByIds = (eventId: string[]) => [];
Expand All @@ -33,8 +33,12 @@ jest.mock('react-redux', () => {
useSelector: jest.fn(),
};
});

jest.mock('../../../../common/components/link_to');

// Prevent Resolver from rendering
jest.mock('../../graph_overlay');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

prevents resolver from rendering.


jest.mock(
'react-visibility-sensor',
() => ({ children }: { children: (args: { isVisible: boolean }) => React.ReactNode }) =>
Expand Down Expand Up @@ -148,6 +152,20 @@ describe('Body', () => {
.exists()
).toEqual(true);
});
describe('when there is a graphEventId', () => {
beforeEach(() => {
props.graphEventId = 'graphEventId'; // any string w/ length > 0 works
});
it('should not render the timeline body', () => {
const wrapper = mount(
<TestProviders>
<Body {...props} />
</TestProviders>
);
// the timeline body still renders, but it gets a `display: none` style via `styled-components`.
expect(wrapper.find(TimelineBody).props().visible).toBe(false);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the timeline body still renders, but it gets a display: none style via styled-components.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Passing the component (function) reference here. Seems neat. Is this a good thing to do in enzyme? ty

Copy link
Contributor

Choose a reason for hiding this comment

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

Passing the component reference is a totally valid thing to do in Enzyme, however we generally prefer to instrument with data-test-subjs, and them for selection in Enzyme tests.

The rational for standardizing on data-test-subj means we can instrument the code under test "once", and then use the same test id in both enyzme and functional / Cypress tests.

Consider replacing the use of TimelineBody for selection with a data-test-subj.

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 explanation. i took your advice.

});
});
});

describe('action on event', () => {
Expand Down
5 changes: 2 additions & 3 deletions x-pack/plugins/security_solution/public/timelines/components/timeline/body/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import { EventsTable, TimelineBody, TimelineBodyGlobalStyle } from '../styles';
import { ColumnHeaders } from './column_headers';
import { getActionsColumnWidth } from './column_headers/helpers';
import { Events } from './events';
import { showGraphView } from './helpers';
import { ColumnRenderer } from './renderers/column_renderer';
import { RowRenderer } from './renderers/row_renderer';
import { Sort } from './sort';
Expand Down Expand Up @@ -146,15 +145,15 @@ export const Body = React.memo<BodyProps>(

return (
<>
{showGraphView(graphEventId) && (
{graphEventId && (
<GraphOverlay bodyHeight={height} graphEventId={graphEventId} timelineId={id} />
)}
<TimelineBody
data-test-subj="timeline-body"
data-timeline-id={id}
bodyHeight={height}
ref={containerElementRef}
visible={show && !showGraphView(graphEventId)}
visible={show && !graphEventId}
>
<EventsTable data-test-subj="events-table" columnWidths={columnWidths}>
<ColumnHeaders
Expand Down
3 changes: 1 addition & 2 deletions x-pack/plugins/security_solution/public/timelines/components/timeline/header/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import React from 'react';
import { FilterManager, IIndexPattern } from 'src/plugins/data/public';
import deepEqual from 'fast-deep-equal';

import { showGraphView } from '../body/helpers';
import { DataProviders } from '../data_providers';
import { DataProvider } from '../data_providers/data_provider';
import {
Expand Down Expand Up @@ -80,7 +79,7 @@ const TimelineHeaderComponent: React.FC<Props> = ({
size="s"
/>
)}
{show && !showGraphView(graphEventId) && (
{show && !graphEventId && (
<>
<DataProviders
browserFields={browserFields}
Expand Down
28 changes: 28 additions & 0 deletions x-pack/plugins/security_solution/public/timelines/components/timeline/timeline.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,34 @@ describe('Timeline', () => {
'All events'
);
});

it('it shows the timeline footer', () => {
const wrapper = mount(
<TestProviders>
<MockedProvider mocks={mocks}>
<TimelineComponent {...props} />
</MockedProvider>
</TestProviders>
);

expect(wrapper.find('[data-test-subj="timeline-footer"]').exists()).toEqual(true);
});
describe('when there is a graphEventId', () => {
beforeEach(() => {
props.graphEventId = 'graphEventId'; // any string w/ length > 0 works
});
it('should not show the timeline footer', () => {
const wrapper = mount(
<TestProviders>
<MockedProvider mocks={mocks}>
<TimelineComponent {...props} />
</MockedProvider>
</TestProviders>
);

expect(wrapper.find('[data-test-subj="timeline-footer"]').exists()).toEqual(false);
});
});
});

describe('event wire up', () => {
Expand Down
48 changes: 27 additions & 21 deletions x-pack/plugins/security_solution/public/timelines/components/timeline/timeline.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,27 +282,33 @@ export const TimelineComponent: React.FC<Props> = ({
toggleColumn={toggleColumn}
/>
</StyledEuiFlyoutBody>
<StyledEuiFlyoutFooter
data-test-subj="eui-flyout-footer"
className="timeline-flyout-footer"
>
<Footer
getUpdatedAt={getUpdatedAt}
hasNextPage={getOr(false, 'hasNextPage', pageInfo)!}
height={footerHeight}
id={id}
isLive={isLive}
isLoading={loading || loadingIndexName}
itemsCount={events.length}
itemsPerPage={itemsPerPage}
itemsPerPageOptions={itemsPerPageOptions}
nextCursor={getOr(null, 'endCursor.value', pageInfo)!}
onChangeItemsPerPage={onChangeItemsPerPage}
onLoadMore={loadMore}
serverSideEventCount={totalCount}
tieBreaker={getOr(null, 'endCursor.tiebreaker', pageInfo)}
/>
</StyledEuiFlyoutFooter>
{
/** Hide the footer if Resolver is showing. */
!graphEventId && (
<StyledEuiFlyoutFooter
data-test-subj="eui-flyout-footer"
className="timeline-flyout-footer"
>
<Footer
data-test-subj="timeline-footer"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this data-test-subj. The same data-test-subj was already defined (on the source of Footer) but not used. I moved it here.

getUpdatedAt={getUpdatedAt}
hasNextPage={getOr(false, 'hasNextPage', pageInfo)!}
height={footerHeight}
id={id}
isLive={isLive}
isLoading={loading || loadingIndexName}
itemsCount={events.length}
itemsPerPage={itemsPerPage}
itemsPerPageOptions={itemsPerPageOptions}
nextCursor={getOr(null, 'endCursor.value', pageInfo)!}
onChangeItemsPerPage={onChangeItemsPerPage}
onLoadMore={loadMore}
serverSideEventCount={totalCount}
tieBreaker={getOr(null, 'endCursor.tiebreaker', pageInfo)}
/>
</StyledEuiFlyoutFooter>
)
}
</>
);
}}
Expand Down