[Endpoint] Alert Details Overview by dplumlee · Pull Request #58412 · elastic/kibana

dplumlee · 2020-02-24T21:55:23Z

Summary

Adds to the alert details flyout with expanded alert details in an accordion style layout

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-02-24T21:55:26Z

Pinging @elastic/endpoint-response (Team:Endpoint Response)

elasticmachine · 2020-02-27T08:25:29Z

Pinging @elastic/endpoint-app-team (Feature:Endpoint)

peluja1012 · 2020-02-27T08:36:40Z

x-pack/plugins/endpoint/public/applications/endpoint/types.ts

You should rebase with master. AlertListState has changed a bit.

peluja1012 · 2020-02-27T10:27:48Z

x-pack/plugins/endpoint/public/applications/endpoint/view/alerts/alert_details_overview.tsx

We need to i18n this whole file

peluja1012 · 2020-02-27T10:31:15Z

x-pack/plugins/endpoint/public/applications/endpoint/view/alerts/alert_details_overview.tsx

peluja1012 · 2020-02-27T10:38:43Z

x-pack/plugins/endpoint/public/applications/endpoint/view/alerts/alert_details_overview.tsx

We should only expand the first one by default

peluja1012 · 2020-02-27T10:42:15Z

x-pack/plugins/endpoint/public/applications/endpoint/view/alerts/alert_details_overview.tsx

Could we use https://styled-components.com/ instead of inline styles?

this is actually just copied over from the eui example, ill probably end up removing it, but for future styles, yes

peluja1012 · 2020-02-27T10:44:52Z

x-pack/plugins/endpoint/public/applications/endpoint/view/alerts/alert_details_overview.tsx

I think we should pull these out into their own components because we'll have to reuse them in the near future when we add more alert types. The new components could take isOpen as a prop and pass that down to initialIsOpen.

paul-tavares

I did not run it locally, but code is looking good 👍

I saw a few TODO in description: "" - I assume you are still working on that since this is marked as wip.

Also - this is more of an unknown for me: I noticed several of your components share the same id for i18n.translate(). Wondering if that is a problem for localization since the keys are being duplicated across multiple components

paul-tavares · 2020-02-28T14:50:53Z

x-pack/plugins/endpoint/common/types.ts

ping @nnamdifrankie - Just FYI. Does not change the type, just breaks out into smaller interfaces for reuse purposes 😃

paul-tavares · 2020-02-28T14:51:56Z

x-pack/plugins/endpoint/public/applications/endpoint/store/alerts/middleware.ts

at some point - need to add error handling

Yes, i have a task for it in the github issue

paul-tavares · 2020-02-28T14:52:41Z

x-pack/plugins/endpoint/public/applications/endpoint/store/alerts/mock_alert_result_list.ts

can you reference an Issue that is tracking this TODO (if its not going to be included in this PR)?

It will be included in this PR

paul-tavares · 2020-02-28T14:58:50Z

x-pack/plugins/endpoint/public/applications/endpoint/view/alerts/details/overview/index.tsx

🤣
I'm assuming you will remove this before commit to master

Good call haha

paul-tavares · 2020-02-28T14:59:18Z

x-pack/plugins/endpoint/public/applications/endpoint/view/alerts/details/overview/index.tsx

you still working on this one? i18n needed below

good call. done.

paul-tavares · 2020-02-28T15:04:05Z

x-pack/plugins/endpoint/public/applications/endpoint/view/alerts/index.tsx

Should be using a connected EuiLink. I think @parkiino 's PR will include a common one in our components/ dir, so you can use that when its merged in.

paul-tavares · 2020-02-28T15:05:33Z

x-pack/plugins/endpoint/public/applications/endpoint/view/alerts/index.tsx

@parkiino - we probably should use this same loading component in Management Host details ⬆️ to stay consistent.

paul-tavares · 2020-02-28T15:07:15Z

x-pack/plugins/endpoint/public/applications/endpoint/view/alerts/index.tsx

for consistency - consider moving this useMemo to the top of the of this component

dplumlee · 2020-03-01T23:33:50Z

@elasticmachine merge upstream

paul-tavares

Just some minor comments that can be addressed (if needed) in subsequent PRs

paul-tavares · 2020-03-02T14:02:13Z

x-pack/plugins/endpoint/public/applications/endpoint/store/alerts/alert_details.test.ts

FYI: I was going to create something similar as well - some of my Policy middleware tests started to fail in Release branch and kibana-operation turn them off. The only think I was thinking about doing that differs from this approach was that I was going to reject the Promise after some x times.

i do agree the test is kind of brittle, i think the goal is to remove it when qualters creates a functional test for the flyout/resolver in his next pr

@paul-tavares we originally had it reject after 10 times, but since no additional actions are ever dispatched (no user exists after all) that limit was never hit. At the moment jest just times out after 4.5 seconds

paul-tavares · 2020-03-02T14:09:11Z

x-pack/plugins/endpoint/public/applications/endpoint/view/alerts/index.tsx

missing href??

does it require an href? we're getting all the functionality we need from just the onclick since its just changing the search query in the url

I think the href is needed if you would like to enable users to open the link in a separate tab/window or be able to copy the link

ok, we've opened a ticket for it to address

kqualters-elastic · 2020-03-02T14:34:47Z

x-pack/plugins/endpoint/public/applications/endpoint/view/alerts/details/overview/index.tsx

probably remove "Endgame?"

…u want to use redux saga

oatkiller

do good

oatkiller · 2020-03-02T18:41:40Z

...lugins/endpoint/public/applications/endpoint/view/alerts/details/metadata/file_accordion.tsx

+        title: i18n.translate('xpack.endpoint.application.endpoint.alertDetails.fileSize', {
+          defaultMessage: 'File Size',
+        }),
+        description: alertData.file.size,


TODO formatting

oatkiller · 2020-03-02T18:42:04Z

...ins/endpoint/public/applications/endpoint/view/alerts/details/metadata/general_accordion.tsx

+        title: i18n.translate('xpack.endpoint.application.endpoint.alertDetails.alertType', {
+          defaultMessage: 'Alert Type',
+        }),
+        description: alertData.event.category,


make a ticket? i18n

Added task to do this here: https://github.com/elastic/endpoint-app-team/issues/92

oatkiller · 2020-03-02T18:43:16Z

...ins/endpoint/public/applications/endpoint/view/alerts/details/metadata/general_accordion.tsx

+        title: i18n.translate('xpack.endpoint.application.endpoint.alertDetails.eventType', {
+          defaultMessage: 'Event Type',
+        }),
+        description: alertData.event.kind,


probably also

oatkiller · 2020-03-02T18:44:10Z

...lugins/endpoint/public/applications/endpoint/view/alerts/details/metadata/host_accordion.tsx

+        title: i18n.translate('xpack.endpoint.application.endpoint.alertDetails.hostIP', {
+          defaultMessage: 'Host IP',
+        }),
+        description: alertData.host.ip.join(','),


oatkiller · 2020-03-02T18:46:19Z

x-pack/plugins/endpoint/public/applications/endpoint/view/alerts/details/metadata/index.ts

+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+export { GeneralAccordion } from './general_accordion';


oatkiller · 2020-03-02T18:49:12Z

x-pack/plugins/endpoint/public/applications/endpoint/view/alerts/details/overview/index.tsx

+        <EuiText>Alert Status: Open</EuiText>
+        <EuiSpacer />
+      </section>
+      <EuiTabbedContent tabs={tabs} initialSelectedTab={tabs[0]} />


consider syncing the tab selection with the URL (maybe make it a future improvement?)
https://elastic.github.io/eui/#/navigation/tabs

Added task for it here: https://github.com/elastic/endpoint-app-team/issues/92

oatkiller · 2020-03-02T18:49:29Z

...lugins/endpoint/public/applications/endpoint/view/alerts/details/overview/metadata_panel.tsx

+      <FileAccordion alertData={alertDetailsData} />
+      <EuiSpacer />
+      <SourceProcessAccordion alertData={alertDetailsData} />
+      <EuiSpacer />


oatkiller · 2020-03-02T18:51:23Z

x-pack/plugins/endpoint/public/applications/endpoint/view/alerts/index.tsx

oatkiller · 2020-03-02T18:52:33Z

x-pack/plugins/endpoint/public/applications/endpoint/view/alerts/resolver.tsx

+    return (
+      <div className={className} data-test-subj="alertResolver" data-testid="alertResolver">
+        <Provider store={store}>
+          <Resolver selectedEvent={(alertDetailsData as unknown) as LegacyEndpointEvent} />


why not use the predicate that you removed https://github.com/elastic/kibana/pull/58412/files#diff-e42c64f81c4fa55ac260536744187224L99

oatkiller · 2020-03-02T18:55:43Z

x-pack/plugins/endpoint/public/applications/endpoint/view/alerts/resolver.tsx

-          </Provider>
-        </div>
-      );
+  React.memo(({ className }: { className?: string }) => {


consider taking in a LegacyEndpointEvent. The component that renders this could get selectedAlertDetailsData and then check if its a LegacyEndpointEvent via a predicate function and then pass it down. that way there is no unsafe casting

marshallmain · 2020-03-02T22:27:28Z

x-pack/plugins/endpoint/common/types.ts

+        data: {
+          buffer: string;
+          decompressed_size: number;
+          encoding: string;


These fields are not going to be used by the UI, right? Anything the UI won't use should be optional or even removed from the types.

but we can do that in subsequent PRs

adding this to this ticket elastic/endpoint-app-team#189

kibanamachine · 2020-03-02T23:46:53Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 93f7cf0

History

💔 Build #30200 failed 6d946f8886a4d7b55c01af14cd971e2ef04fe177
💔 Build #30150 failed 5500167
💚 Build #29957 succeeded 900c43bf3570b62bbf3dee9291f285d5974d2be8
💔 Build #29930 failed d0880d523546663678f0dc134411c9de951ef6ab
💔 Build #29915 failed 1bbbaa8cee8622325dd0f2911193a7ef8241a3db

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

EricDavisX · 2020-03-03T01:46:27Z

@peluja1012 I'm clearly behind in pushing 'test considerations' on the PRs we have, now that *that is the desired process. However, I think the test idea are in another castle. I am curious whats our process is tho? ...This was closed out without fulfilling all of the tasks (which is ok by me) or they just need to be updated, lol. If not, can we plan to always put a prominent link to the new 'remaining work' ticket or PR that's been insta-opened?

And there aren't any browser tests for a UI focused pr, @dplumlee do you want to work with me to get up to speed on the Functional Test Runner jazz so we can bang out some tests for this new component?

* master: (26 commits) [Endpoint] Alert Details Overview (elastic#58412) Service map language icons (elastic#58633) [SIEM] [Case] Comments to case view (elastic#58315) Remove appBasePath from docs + add mock for AppMountParameters (elastic#58775) [kbn/optimizer] fix ui/* url rewrites in dist (elastic#58627) Dashboard a11y tests (elastic#58122) Downgrade "setting up plugin" log to debug (elastic#58776) [CI] Pipeline refactoring (elastic#56447) [Advanced Settings] Fix a11y of unsaved indicator (elastic#58511) put params into short url instead of behind it (elastic#58846) show timepicker in timelion and tsvb (elastic#58857) improve graph missing workspace error message (elastic#58876) [Maps] direct Discover "visualize" to open Maps application (elastic#58549) Disallow duplicate percentiles (elastic#57444) (elastic#58299) removing references to visTypes uiExports (elastic#58337) [SIEM] Default the Timeline events filter to show All events (elastic#58953) [Remote clusters] Add indexManagement as required plugin (elastic#58915) [DOCS] Rework of main get started page (elastic#58260) [Endpoint] [Tests] fixes elastic#57946 flaky endpoint policy list test (elastic#58348) [Endpoint] add resolver middleware (elastic#58288) ...

EricDavisX · 2020-03-03T17:56:18Z

I spoke with Pedro and we're going to review the tasks list. And I see a note that Kevin Q has a pr that will cover the Functional browser tests so seeing that its all good now.

…s/kibana into alerting/fix-flaky-instance-test * 'alerting/fix-flaky-instance-test' of github.com:gmmorris/kibana: [Endpoint] Alert Details Overview (elastic#58412) Service map language icons (elastic#58633) [SIEM] [Case] Comments to case view (elastic#58315) Remove appBasePath from docs + add mock for AppMountParameters (elastic#58775) [kbn/optimizer] fix ui/* url rewrites in dist (elastic#58627)

kibanamachine · 2020-03-04T00:38:44Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 58412 or prevent reminders by adding the backport:skip label.

dplumlee added WIP Work in progress Team:Endpoint Response Endpoint Response Team labels Feb 24, 2020

dplumlee force-pushed the alert-details-overview branch from b35df0f to 83994e6 Compare February 24, 2020 23:03

peluja1012 added the Feature:Endpoint Elastic Endpoint feature label Feb 27, 2020

peluja1012 added v7.7.0 v8.0.0 labels Feb 27, 2020

peluja1012 reviewed Feb 27, 2020

View reviewed changes

x-pack/plugins/endpoint/public/applications/endpoint/view/alerts/alert_details_overview.tsx Outdated

Copy link
Copy Markdown

Contributor

peluja1012 Feb 27, 2020

Choose a reason for hiding this comment

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

🔥

peluja1012 reviewed Feb 27, 2020

View reviewed changes

dplumlee force-pushed the alert-details-overview branch from 83994e6 to d38a4ac Compare February 27, 2020 20:50

paul-tavares reviewed Feb 28, 2020

View reviewed changes

kqualters-elastic mentioned this pull request Feb 28, 2020

[Endpoint] add resolver middleware #58288

Merged

6 tasks

dplumlee added release_note:skip Skip the PR/issue when compiling release notes and removed WIP Work in progress labels Feb 28, 2020

peluja1012 approved these changes Feb 28, 2020

View reviewed changes

dplumlee force-pushed the alert-details-overview branch 2 times, most recently from 28a89f5 to d0880d5 Compare February 29, 2020 19:33

dplumlee changed the title ~~Alert Details Overview~~ [Endpoint] Alert Details Overview Mar 1, 2020

paul-tavares approved these changes Mar 2, 2020

View reviewed changes

kqualters-elastic reviewed Mar 2, 2020

View reviewed changes

dplumlee added 4 commits March 2, 2020 11:59

adds base accordions to alert detail flyout

6eace7e

adds hardcoded description blurb

a54cd58

adds more fields to alert data type

0f091d5

refactors names

1769e1d

peluja1012 and others added 8 commits March 2, 2020 12:01

Remove fields, intl some text

6452fb6

remove severity

67c8f11

update fields based on new schema

b64ca21

adding tests

fddef5d

i dont feel sorry for you at all. dont unit test middleware unless yo…

f66f7f7

…u want to use redux saga

addresses comments

3854b02

addresses comments

ce5d25b

integrates with resolver pr

5500167

dplumlee force-pushed the alert-details-overview branch from 900c43b to 5500167 Compare March 2, 2020 18:07

oatkiller suggested changes Mar 2, 2020

View reviewed changes

addresses comments

93f7cf0

dplumlee force-pushed the alert-details-overview branch from 6d946f8 to 93f7cf0 Compare March 2, 2020 21:53

marshallmain reviewed Mar 2, 2020

View reviewed changes

oatkiller approved these changes Mar 2, 2020

View reviewed changes

dplumlee merged commit ffab68d into elastic:master Mar 3, 2020

dplumlee deleted the alert-details-overview branch March 3, 2020 00:32

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 4, 2020

dplumlee mentioned this pull request Mar 4, 2020

[7.x] [Endpoint] Alert Details Overview (#58412) #59356

Closed

dplumlee added a commit to dplumlee/kibana that referenced this pull request Mar 4, 2020

[Endpoint] Alert Details Overview (elastic#58412)

b434695

dplumlee mentioned this pull request Mar 4, 2020

[7.x] [Endpoint] Alert Details Overview (#58412) #59366

Merged

dplumlee added a commit to dplumlee/kibana that referenced this pull request Mar 4, 2020

[Endpoint] Alert Details Overview (elastic#58412)

d8f903f

dplumlee added a commit that referenced this pull request Mar 4, 2020

[Endpoint] Alert Details Overview (#58412) (#59366)

af29ce9

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 4, 2020

Conversation

dplumlee commented Feb 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Feb 24, 2020

Uh oh!

elasticmachine commented Feb 27, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paul-tavares left a comment

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!

Choose a reason for hiding this comment

Uh oh!

dplumlee commented Mar 1, 2020

Uh oh!

paul-tavares left a comment

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 left a comment

Choose a reason for hiding this comment

Uh oh!

dplumlee commented Feb 24, 2020 •

edited

Loading