Add a flyout to alert list. by oatkiller · Pull Request #57926 · elastic/kibana

oatkiller · 2020-02-18T22:02:04Z

Summary

Add a button to alert list that adds selected_alert query string to url. if qs is present, flyout shows up. No data loading logic added yet.

Related to https://github.com/elastic/endpoint-app-team/issues/92

Issues

The tests for this PR emit warnings. These don't effect the test. I believe they are being caused by useEffect calls in EuiDataGrid that set state right after the first render, causing a subsequent render in many cases, but that's just a guess. I . haven't been able to work out a solution with #eui yet.
https://github.com/elastic/endpoint-app-team/issues/205

This uses our custom type for createStructuredSelector, but the type isn't completely compatible w/ the implementation:
https://github.com/elastic/endpoint-app-team/issues/204

And here are a bunch of other issues created during the development of this PR:

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
Documentation was added for features that require explanation or tutorials
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

peluja1012 · 2020-02-19T16:07:30Z

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

Why is the the variable name urlPaginationData?

peluja1012 · 2020-02-19T16:08:04Z

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

Do we need to spread ...queryParams(state)?

oatkiller · 2020-02-19T17:53:12Z

x-pack/plugins/endpoint/server/services/endpoint/alert_query_builders.ts

This fn used no await so I removed async. Might look into adding lint to enforce this.

oatkiller · 2020-02-19T17:53:19Z

x-pack/plugins/endpoint/server/services/endpoint/alert_query_builders.ts

filter by alerts

oatkiller · 2020-02-19T17:53:45Z

x-pack/plugins/endpoint/server/services/endpoint/alert_query_builders.test.ts

update the test to accommodate for the new filter

oatkiller · 2020-02-19T17:53:51Z

x-pack/plugins/endpoint/server/services/endpoint/alert_query_builders.test.ts

update the test to accommodate for the new filter

oatkiller · 2020-02-19T17:54:08Z

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

If the url has a selected alert in the query string, show this.

alexk307 · 2020-02-19T18:46:36Z

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

@alexk307 worth a discussion but: https://github.com/elastic/endpoint-app-team/issues/187

alexk307 · 2020-02-19T18:48:52Z

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

why do you need to nest these functions? Why not just have one function

alexk307

LGTM, just some questions

parkiino · 2020-02-19T20:03:40Z

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

question (bc i'm still a noob at types): do we have to do this because of typescript?

this actually isn't necessary. The event.target could be any DOM node that is a descendant of event.currentTarget aka the thing that the event listener is on. Any DOM node isn't necessarily an HTMLElement. DOM nodes don't all support alertId.

In this case, I could just refer to event.currentTarget. The (synthetic) event handler is registered to an html element (the one w/ the data-* attribute) and so referring to event.currentTarget will work w/o the guard.

const handleAlertClick = useMemo(() => { return (event: React.MouseEvent<HTMLElement>) => { const alertId: string | undefined = event.currentTarget.dataset.alertId; if (alertId !== undefined) { history.push(urlWithSelectedAlert(alertId)); } }; }, [history, urlWithSelectedAlert]);

okie good to know

paul-tavares · 2020-02-19T20:32:52Z

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

I will do something similar for Policy list PR that I have pending to add pagination to the URL.
In my PR I was trying to NOT hold on to the URL information passed through from React-Router, but rather (in the reducer) do the needed processing on it that Policy needs and save the the output to the store. Is that OK? or should I follow the same approach as here?

I think I understand you here, but let me know if I'm off base.

When modeling redux state, I try to record a normalized (like database normalization: https://en.wikipedia.org/wiki/Database_normalization) representation of the relevant side effects. If some calculation is done, I do it in a selector. The selector forms the read-only model for the state. Any value that can be calculated as a result of the reduced-side effects (the state) is provided by the selector. The selector (via reselect) also handles caching / invalidating the calculated views of state.

More thoughts

Redux as a pattern separates the writing/mutation/updating parts of a model from the viewing/calculating/presenting parts of the same model.

Reducer/state

Keeps a normalized record of side effects

Keeps data that is required by selector, or data that is required in order to update state in the future (e.g. a counter)

Keeps data in a format that is easily updated when new side effects are introduced

Doesn't calculate things from state. A reducer could be run 10x before the view runs a selector

Fields of state are analogous to private instance members in a class

Selector

Like a read-only model for the state

Calculates 'views' on the state. Think about the 'presenter' model.

Selectors are functions that are called at the time they are needed, do some calculation (maybe) and then memoize it (usually.)

Selectors are like 'getters' in a class

All that having been said, I don't think the approach we have in this PR is the final goal for URL handling. I think its too prone to copy-paste errors and there is too much boilerplate. We should hit the drawing board.

@alexk307 ^

@paul-tavares i've changed my query string stuff a lot. I think it's closer to what you have now. We should be able to come up w/ a common implementation in a PR or so I think. Let me know what you think of this one.

paul-tavares · 2020-02-19T20:43:47Z

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

😱 forgot about this. I will add awareness of it to my PR

paul-tavares · 2020-02-19T20:49:59Z

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

use <FormattedDate> and/or <FormattedTime> from @kbn/i18n/react instead of a local formatter?

Also - Just FYI: in Policy list we discussed displaying date/time in relative format if within the last 1h and formatted otherwise. I have that in policy_list now, but if common requirement across all of our pages, then we should move/adjust/improve it 😄

Switched to <FormattedDate />, its equivalent

paul-tavares · 2020-02-19T20:54:28Z

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

I did not realize that history.push handled URL that started with ?. I was using useLocation() to grab the current pathname in mine. I will refactor to remove it.

history will interpret urls as relative to the current one.

// ran this on this page via console history.pushState(null, '', '?cool=true') // url is now: 'https://github.com/elastic/kibana/pull/57926/files?cool=true'

@paul-tavares i switched my implementation to use LocationDescriptionObject because its easier

paul-tavares · 2020-02-19T20:55:42Z

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

should the URL be set on this item? (allowing user's to do control click to new window or copy URL to clipboard)?

Thanks for the tip. After realizing that EuiLink returns a button, I didn't even think about trying to use href'. If href works, that'll simplify things a lot.

paul-tavares · 2020-02-19T21:01:32Z

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

Should these be named something like urlSearchParams* to be more reflective of what it is returning?

I view this as returning a relative URL. It is a string, beginning with ? and so it fits the technical def: https://tools.ietf.org/html/rfc1808#section-5

The intent is that these will be used with history.pushState or the href attribute of an a tag. These accept relative URLs and treat them as absolute after combining them with the base URL of the page.

Maybe I could explain the above in a comment. With all that being said, do you still think I should go for a diff name?

oatkiller · 2020-02-20T17:43:24Z

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

@alexk307 I missed your earlier comment, so replying here.
Selectors have a common interface, they take the state of the app (what is returned by the top level reducer) and return whatever they want. The return value should be a primitive, or treated as immutable as its used to invalidate views and things.

These selectors want to take a second param. In this case, the page number for the link they want. But since selectors always take exactly one argument, we use a thunk instead.

Helpers that take a selector, e.g. useSelector, createStructuredSelector, createSelector, etc, all still work fine, since we are still a standard (odd) selector. Its not a pattern i like to use too often, but we've got a whole bunch here (and in Resolver.)

oatkiller · 2020-02-20T17:43:36Z

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

dplumlee · 2020-02-20T17:44:26Z

x-pack/plugins/endpoint/public/applications/endpoint/store/index.ts

maybe put a comment in here saying why you need the ability to disable the middleware?

elasticmachine · 2020-02-20T18:08:03Z

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

tonymeehan · 2020-02-20T18:18:12Z

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

https://github.com/elastic/endpoint-app-team/issues/189

And created issue: https://github.com/elastic/endpoint-app-team/issues/192

…react router also, use `href` instead of a button

disable test i broke. lets rethink this

oatkiller · 2020-02-20T23:27:57Z

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

-    [history, urlFromNewPageIndexParam]
+    newPageIndex => {
+      return history.push(
+        urlFromQueryParams({


@paul-tavares doing something closer to what you have.
@dplumlee @peluja1012 thoughts?

this works for me. maybe we should also use the same querystring package that @paul-tavares is using (query-string) just to keep it consistent. I think it's in kibana already and it's a bit better than the one we're using

@oatkiller looks good but closing the flyout doesn't seem to be working when only selected_alert is present. The user gets redirected to /app/endpoint/:

I goofed this up. I'm working on another slight improvement, but I feel like I might just be getting in the way of stuff @paul-tavares is doing. Maybe worth syncing

oatkiller · 2020-02-21T17:09:29Z

@paul-tavares @peluja1012 @kqualters-elastic @dplumlee @parkiino Changed the way query strings are done, fixed tests and stuff. Comments and things might be sloppy, @dplumlee and @parkiino will fix that :)

parkiino · 2020-02-21T19:23:52Z

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

  const [visibleColumns, setVisibleColumns] = useState(() => columns.map(({ id }) => id));

+  const handleFlyoutClose = useCallback(() => {
+    const { selected_alert, ...paramsWithoutSelectedAlert } = queryParams;


are we doing url params in snake case? is that the convention? i forget...

ya https://github.com/elastic/kibana/blob/master/STYLEGUIDE.md#snake_case

…_improve-advanced-settings-save * commit '02efb01c481f9f24d8d707f06dfc68b2fb805001': (43 commits) [Endpoint] Add a flyout to alert list. (elastic#57926) Make sure index pattern has fields before parsing (elastic#58242) Sanitize workpad before sending to API (elastic#57704) [ML] Transform: Support multi-line JSON notation in advanced editor (elastic#58015) [Endpoint] Refactor Management List Tests (elastic#58148) [kbn/optimizer] include bootstrap cache key in optimizer cache key (elastic#58176) Do not refresh color scale on each lookup (elastic#57792) Updating to @elastic/lodash@3.10.1-kibana4 (elastic#54662) Trigger context (elastic#57870) [ML] Transforms: Adds clone feature to transforms list. (elastic#57837) [ML] New Platform server shim: update fields service routes (elastic#58060) [Endpoint] EMT-184: change endpoints to metadata up and down the code base. (elastic#58038) document difference between log record formats (elastic#57798) Expose elasticsearch config schema (elastic#57655) [ui/agg_response/tabify] update types for search/expressions/build_tabular_inspector_data.ts (elastic#58130) [SIEM] Cleans Cypress tests code (elastic#58134) fix: 🐛 make dev server Storybook builds work again (elastic#58188) Prevent core savedObjects plugin from being overridden (elastic#58193) Expose serverBasePath on client-side (elastic#58070) Fix legend sizing on area charts (elastic#58083) ...

* Filter alert API so it shows only Alerts instead of all documents in the index * Clicking an item in the alert list will open a flyout

…-out-of-legacy * 'master' of github.com:elastic/kibana: [SIEM] [Case] Enable case by default. Snake to camel on UI (elastic#57936) [File upload] Update remaining File Upload dependencies for NP migration (elastic#58128) Use EuiTokens for ES field types (elastic#57911) Added UI support for the default action group for Alert Type Model (elastic#57603) force savedObject API consumers to define SO type explicitly (elastic#58022) Update dependency @elastic/charts to ^17.1.1 (elastic#57634) [Endpoint] Add a flyout to alert list. (elastic#57926) Make sure index pattern has fields before parsing (elastic#58242) Sanitize workpad before sending to API (elastic#57704) [ML] Transform: Support multi-line JSON notation in advanced editor (elastic#58015) [Endpoint] Refactor Management List Tests (elastic#58148) [kbn/optimizer] include bootstrap cache key in optimizer cache key (elastic#58176) Do not refresh color scale on each lookup (elastic#57792) Updating to @elastic/lodash@3.10.1-kibana4 (elastic#54662) Trigger context (elastic#57870) [ML] Transforms: Adds clone feature to transforms list. (elastic#57837) [ML] New Platform server shim: update fields service routes (elastic#58060) [Endpoint] EMT-184: change endpoints to metadata up and down the code base. (elastic#58038)

kibanamachine · 2020-02-24T16:46:36Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: dbffe17

History

💚 Build #28184 succeeded dbffe17
💚 Build #28049 succeeded 626e2fb
💚 Build #27981 succeeded 5d26716ac51b9c3a494e79b34a88fd8fe97dc3f1
💔 Build #27956 failed 09f04268978676a8045825a6125d138a326c6815
💔 Build #27962 failed 1664c32b72393938ce66b62ec62da8bf565c97fb

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

oatkiller force-pushed the alert-list-flyout branch from 0a15730 to 82c17c4 Compare February 19, 2020 01:07

oatkiller added Feature:Resolver Security Solution Resolver feature Team:Endpoint Data Visibility Team managing the endpoint resolver release_note:skip Skip the PR/issue when compiling release notes v7.7.0 labels Feb 19, 2020

oatkiller changed the title ~~WIP: add a flyout to alert list.~~ Add a flyout to alert list. Feb 19, 2020

peluja1012 reviewed Feb 19, 2020

View reviewed changes

oatkiller commented Feb 19, 2020

View reviewed changes

x-pack/plugins/endpoint/server/services/endpoint/alert_query_builders.ts Outdated

Copy link

Contributor Author

oatkiller Feb 19, 2020

Choose a reason for hiding this comment

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

filter by alerts

oatkiller commented Feb 19, 2020

View reviewed changes

kqualters-elastic approved these changes Feb 19, 2020

View reviewed changes

dplumlee approved these changes Feb 19, 2020

View reviewed changes

alexk307 reviewed Feb 19, 2020

View reviewed changes

alexk307 approved these changes Feb 19, 2020

View reviewed changes

parkiino reviewed Feb 19, 2020

View reviewed changes

paul-tavares reviewed Feb 19, 2020

View reviewed changes

oatkiller force-pushed the alert-list-flyout branch from c0bb87b to 7dc1ecf Compare February 20, 2020 17:39

oatkiller commented Feb 20, 2020

View reviewed changes

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

Copy link

Contributor Author

oatkiller Feb 20, 2020

Choose a reason for hiding this comment

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

TODO

dplumlee reviewed Feb 20, 2020

View reviewed changes

peluja1012 added the Team:Endpoint Response Endpoint Response Team label Feb 20, 2020

tonymeehan reviewed Feb 20, 2020

View reviewed changes

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

Copy link

tonymeehan Feb 20, 2020

Choose a reason for hiding this comment

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

whitespace

oatkiller added 3 commits February 20, 2020 18:22

add a flyout

07e448d

Removing TODOs, added issue

e7c891c

https://github.com/elastic/endpoint-app-team/issues/189

Rename queryParams selector

3d6f4ab

oatkiller added 15 commits February 20, 2020 18:22

Removed sample code

26c3ca6

And created issue: https://github.com/elastic/endpoint-app-team/issues/192

DO IT

09fe7dc

return alerts from alert api.

517d796

remove copy paste

dc98ea0

Fix query builder test

f2847b4

GO FOR IT

7d18af0

add tests

9686b88

use davis technique

b5ce863

add tests that test it

eb25f76

use currentTarget cause candace said so

daafb55

use react router link. its an actual link, and its easier to test w/ …

0d67dad

…react router also, use `href` instead of a button

rewrite the tests

26a5ac2

remove todos

60db616

Add comments

66c4590

remove snapshot as it was not stable (relied on timezone)

5a30168

oatkiller force-pushed the alert-list-flyout branch from a160717 to 9b0791d Compare February 20, 2020 23:22

respect existing query params when opening or closing flyouyt

626e2fb

disable test i broke. lets rethink this

oatkiller force-pushed the alert-list-flyout branch from 9b0791d to 626e2fb Compare February 20, 2020 23:26

oatkiller commented Feb 20, 2020

View reviewed changes

oatkiller added 2 commits February 21, 2020 11:30

pedro

5b78baf

DO IT

dbffe17

parkiino reviewed Feb 21, 2020

View reviewed changes

oatkiller merged commit 02efb01 into elastic:master Feb 21, 2020

oatkiller deleted the alert-list-flyout branch February 21, 2020 19:44

oatkiller mentioned this pull request Feb 21, 2020

[7.x] [Endpoint] Add a flyout to alert list. (#57926) #58275

Merged

oatkiller pushed a commit that referenced this pull request Feb 22, 2020

[Endpoint] Add a flyout to alert list. (#57926) (#58275)

1588009

* Filter alert API so it shows only Alerts instead of all documents in the index * Clicking an item in the alert list will open a flyout

Comments

Conversation

oatkiller commented Feb 18, 2020 • edited by peluja1012 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Issues

Checklist

For maintainers

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!

alexk307 left a comment

Choose a reason for hiding this comment

Uh oh!

parkiino Feb 19, 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

More thoughts

Reducer/state

Selector

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!

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!

elasticmachine commented Feb 20, 2020

oatkiller commented Feb 18, 2020 •

edited by peluja1012

Loading

parkiino Feb 19, 2020 •

edited

Loading

peluja1012 Feb 21, 2020 •

edited

Loading