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

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 3 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 @@ -34,6 +34,7 @@ import {
} from '../../../../../../../src/plugins/data/public';
import { inputsModel } from '../../store';
import { useManageTimeline } from '../../../timelines/components/manage_timeline';
import { showGraphView } from '../../../timelines/components/timeline/body/helpers';
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 was defined here, but perhaps i should move it if im using it this way? looking for guidance from SIEM team.


const DEFAULT_EVENTS_VIEWER_HEIGHT = 500;

Expand Down Expand Up @@ -67,6 +68,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 +93,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 +195,27 @@ 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

!showGraphView(graphEventId) && (
<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.

);
12 changes: 11 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,11 @@ const makeMapStateToProps = () => {
query: getGlobalQuerySelector(state),
sort,
showCheckboxes,
// Used to determine whether the footer should show (since it is hidden if the graph is showing.)
graphEventId: /** `getTimeline` actually returns `TimelineModel | undefined` */ (getTimeline(
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 was confused looking at the types in my editor. Basically, getTimeline returns TimelineModel | undefined but is typed as TimelineModel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this is working correctly or not. Perhaps it's worth investigating?

state,
id
) as TimelineModel | undefined)?.graphEventId,
};
};
return mapStateToProps;
Expand Down Expand Up @@ -213,6 +222,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

)
);
49 changes: 27 additions & 22 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 @@ -17,7 +17,7 @@ import { Direction } from '../../../graphql/types';
import { useKibana } from '../../../common/lib/kibana';
import { ColumnHeaderOptions, KqlMode, EventType } from '../../../timelines/store/timeline/model';
import { defaultHeaders } from './body/column_headers/default_headers';
import { getInvestigateInResolverAction } from './body/helpers';
import { getInvestigateInResolverAction, showGraphView } from './body/helpers';
import { Sort } from './body/sort';
import { StatefulBody } from './body/stateful_body';
import { DataProvider } from './data_providers/data_provider';
Expand Down Expand Up @@ -282,27 +282,32 @@ 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. */
!showGraphView(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.

only change here is to add the guard. props are the same

<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>
)
}
</>
);
}}
Expand Down