Hide timeline footer when Resolver is open by oatkiller · Pull Request #71516 · elastic/kibana

oatkiller · 2020-07-13T19:17:25Z

Summary

When Resolver opens up, the Timeline footer is still visible. A user might think that the timeline footer controls Resolver. This PR hides the footer when Resolver is open.

The footer (pagination) is now hidden when Resolver is visible

Edit: new gif for ~~a4a496b~~ 96e642e

BEFORE

Checklist

Unit or functional tests were updated or added to match the most common scenarios
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

oatkiller · 2020-07-13T19:49:17Z

x-pack/plugins/security_solution/public/common/components/events_viewer/events_viewer.tsx

 } from '../../../../../../../src/plugins/data/public';
 import { inputsModel } from '../../store';
 import { useManageTimeline } from '../../../timelines/components/manage_timeline';
+import { showGraphView } from '../../../timelines/components/timeline/body/helpers';


this was defined here, but perhaps i should move it if im using it this way? looking for guidance from SIEM team.

oatkiller · 2020-07-13T19:49:40Z

x-pack/plugins/security_solution/public/common/components/events_viewer/events_viewer.tsx

-                      tieBreaker={getOr(null, 'endCursor.tiebreaker', pageInfo)}
-                    />
+                    {
+                      /** Hide the footer if Resolver is showing. */


Add a guard statement and remove data-test-subj

oatkiller · 2020-07-13T19:49:50Z

x-pack/plugins/security_solution/public/common/components/events_viewer/events_viewer.tsx

    prevProps.sort === nextProps.sort &&
-    prevProps.utilityBar === nextProps.utilityBar
+    prevProps.utilityBar === nextProps.utilityBar &&
+    prevProps.graphEventId === nextProps.graphEventId


This seems to be required for it to work.

oatkiller · 2020-07-13T19:50:10Z

x-pack/plugins/security_solution/public/common/components/events_viewer/index.tsx

        sort={sort}
        toggleColumn={toggleColumn}
        utilityBar={utilityBar}
+        graphEventId={graphEventId}


Pass the new prop to the unconnected event viewer

oatkiller · 2020-07-13T19:50:47Z

x-pack/plugins/security_solution/public/common/components/events_viewer/index.tsx

      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(


i was confused looking at the types in my editor. Basically, getTimeline returns TimelineModel | undefined but is typed as TimelineModel.

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

oatkiller · 2020-07-13T19:52:10Z

x-pack/plugins/security_solution/public/common/components/events_viewer/index.tsx

      prevProps.start === nextProps.start &&
-      prevProps.utilityBar === nextProps.utilityBar
+      prevProps.utilityBar === nextProps.utilityBar &&
+      prevProps.graphEventId === nextProps.graphEventId


required for rendering to work correctly

oatkiller · 2020-07-13T19:52:26Z

x-pack/plugins/security_solution/public/timelines/components/timeline/timeline.tsx

-                </StyledEuiFlyoutFooter>
+                {
+                  /** Hide the footer if Resolver is showing. */
+                  !showGraphView(graphEventId) && (


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

bkimmel · 2020-07-13T19:58:33Z

x-pack/plugins/security_solution/public/common/components/events_viewer/events_viewer.tsx

  sort: Sort;
  toggleColumn: (column: ColumnHeaderOptions) => void;
  utilityBar?: (refetch: inputsModel.Refetch, totalCount: number) => React.ReactNode;
+  // If truthy, the graph viewer (Resolver) is showing


❔ Should this come from a selector like shouldShowResolver?

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.

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

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.

oatkiller · 2020-07-13T21:22:42Z

x-pack/plugins/security_solution/public/timelines/components/timeline/timeline.tsx

+                      className="timeline-flyout-footer"
+                    >
+                      <Footer
+                        data-test-subj="timeline-footer"


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.

oatkiller · 2020-07-13T21:23:49Z

...urity_solution/public/timelines/components/timeline/footer/__snapshots__/index.test.tsx.snap


 exports[`Footer Timeline Component rendering it renders the default timeline footer 1`] = `
 <FooterContainer
-  data-test-subj="timeline-footer"


this wasn't being used so i took it. Enzyme didn't seem to render this far. I expected it to (the call was to mount.) But I've never used enzyme so I'm probably goofing it up. This works, but if anyone has improvement suggestions let me know.

oatkiller · 2020-07-13T21:26:59Z

x-pack/plugins/security_solution/public/timelines/components/timeline/body/helpers.ts

  return 'raw';
 };

-export const showGraphView = (graphEventId?: string) =>


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

oatkiller · 2020-07-13T21:34:28Z

@andrew-goldstein I tried to put a test in this file: https://github.com/elastic/kibana/blob/master/x-pack/plugins/security_solution/public/common/components/events_viewer/index.test.tsx

I couldn't find an easy way to change the value of graphEventId for the stateful event viewer. My thoughts:

Create a store w/ a different default state, pass that into the mock provider
Test the EventViewer directly (not the stateful one). This would require mocking all of the props which are normally defined in the connect function.

Do either of these seem like a good idea? Is the test necessary? ty

…prop on TimelineBody

oatkiller · 2020-07-13T22:09:43Z

x-pack/plugins/security_solution/public/timelines/components/timeline/body/index.test.tsx

  };
 });
 jest.mock('../../../../common/components/link_to');
+jest.mock('../../graph_overlay');


prevents resolver from rendering.

oatkiller · 2020-07-13T22:10:05Z

x-pack/plugins/security_solution/public/timelines/components/timeline/body/index.test.tsx

+            <Body {...props} />
+          </TestProviders>
+        );
+        expect(wrapper.find(TimelineBody).props().visible).toBe(false);


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

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

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.

thanks for the explanation. i took your advice.

andrew-goldstein

Thanks for this addition @oatkiller!
Tested locally with agent.type: endpoint data
LGTM 🚀

oatkiller · 2020-07-13T22:53:46Z

x-pack/plugins/security_solution/public/timelines/components/timeline/body/index.test.tsx

+        // The first TimelineBody component
+        const timelineBody: TimelineBodyEnzymeWrapper = wrapper
+          .find('[data-test-subj="timeline-body"]')
+          .first() as TimelineBodyEnzymeWrapper;


These tests render 2 things that match .find('[data-test-subj="timeline-body"]'), get the first.

kibanamachine · 2020-07-14T00:35:52Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 96e642e

Build metrics

✅ unchanged

History

💚 Build #61137 succeeded b0b048b
💔 Build #61110 failed a4a496b
💚 Build #61074 succeeded 8a75f79
💚 Build #61060 succeeded 0e3ad6f

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…ic#71516) * Hide the Timeline footer, in the event viewer, if Resolver is showing

* master: (21 commits) [Maps] 7.9 design improvements (elastic#71563) [ML] Changing all calls to ML endpoints to use internal user (elastic#70487) [eventLog] prevent log writing when initialization fails (elastic#71339) [Observability] landing page always being displayed (elastic#71494) [IM] Address data stream copy feedback (elastic#71615) [Logs UI] Anomalies page dataset filtering (elastic#71110) [data.search.aggs] Remove `use_field_mapping` from top hits agg (elastic#71168) [ML] Anomaly swim lane embeddable navigation and filter actions (elastic#71082) Fixes typo in siem_cloudtrail job description (elastic#71569) Require granted API Keys to have a name (elastic#71623) Update getUsageForCollection (elastic#71609) Only fetch saved elements once (elastic#71310) [SecuritySolution][Resolver] Adding siem index and guarding process ancestry (elastic#71570) [APM] Additional data telemetry changes (elastic#71112) [Visualize] Fix export table for table export links (elastic#71249) [Search] Server side search API (elastic#70446) use inclusive language (elastic#71607) [Security Solution] Hide timeline footer when Resolver is open (elastic#71516) [Index template wizard] Remove shadow and use border for components panels (elastic#71606) [ML] Kibana API endpoint for histogram chart data (elastic#70976) ...

… (#71640) * Hide the Timeline footer, in the event viewer, if Resolver is showing

oatkiller added 2 commits July 13, 2020 14:33

Hide the Timeline footer, in the event viewer, if Resolver is showing

32dc074

Hide the Timeline footer, in the timeline flyout, if Resolver is showing

1ad5679

oatkiller requested review from XavierM, andrew-goldstein, james-elastic, kqualters-elastic, lindseypoli and monina-n July 13, 2020 19:17

oatkiller requested review from a team as code owners July 13, 2020 19:17

oatkiller added release_note:skip Skip the PR/issue when compiling release notes v7.9.0 labels Jul 13, 2020

fixup: getTimeline actually returns TimelineModel | undefined

0e3ad6f

james-elastic approved these changes Jul 13, 2020

View reviewed changes

oatkiller requested a review from marrasherrier July 13, 2020 19:45

code style

8a75f79

oatkiller commented Jul 13, 2020

View reviewed changes

bkimmel reviewed Jul 13, 2020

View reviewed changes

bkimmel approved these changes Jul 13, 2020

View reviewed changes

Add tests. Remove showGraphView helper

a4a496b

oatkiller commented Jul 13, 2020

View reviewed changes

michaelolo24 approved these changes Jul 13, 2020

View reviewed changes

oatkiller added 2 commits July 13, 2020 18:06

mock GraphOverlay so that Resolver doesnt mount. Check the visiblity …

7e5ed4f

…prop on TimelineBody

comments

68c0e91

oatkiller commented Jul 13, 2020

View reviewed changes

comments

b0b048b

andrew-goldstein approved these changes Jul 13, 2020

View reviewed changes

test uses data-test-subj instead of component reference

96e642e

oatkiller commented Jul 13, 2020

View reviewed changes

oatkiller merged commit 97afee5 into elastic:master Jul 14, 2020

oatkiller mentioned this pull request Jul 14, 2020

[7.x] [Security Solution] Hide timeline footer when Resolver is open (#71516) #71640

Merged

oatkiller pushed a commit to oatkiller/kibana that referenced this pull request Jul 14, 2020

[Security Solution] Hide timeline footer when Resolver is open (elast…

63c3609

…ic#71516) * Hide the Timeline footer, in the event viewer, if Resolver is showing

oatkiller pushed a commit that referenced this pull request Jul 14, 2020

[Security Solution] Hide timeline footer when Resolver is open (#71516)…

6640232

… (#71640) * Hide the Timeline footer, in the event viewer, if Resolver is showing

oatkiller deleted the hide-timeline-footer branch March 31, 2022 11:02

Conversation

oatkiller commented Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

The footer (pagination) is now hidden when Resolver is visible

BEFORE

Checklist

For maintainers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oatkiller Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oatkiller Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oatkiller commented Jul 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrew-goldstein left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jul 14, 2020

💚 Build Succeeded

Build metrics

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

oatkiller commented Jul 13, 2020 •

edited

Loading

oatkiller Jul 13, 2020 •

edited

Loading

oatkiller Jul 13, 2020 •

edited

Loading