Alert list frontend pagination #57142

dplumlee · 2020-02-07T18:52:48Z

Summary

Connects the alert list frontend to the backend api to pass alert data and pagination control. Updates the frontend routing to use and modify the url query for navigation and REST api control

Checklist

Delete any items that are not applicable to this PR.

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, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

dplumlee · 2020-02-11T23:21:32Z

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

+          alerts: [
+            {
+              '@timestamp': new Date(0),
+              agent: {


do we want to have less silly data?

I think so. You can find some sample values for these fields here https://github.com/elastic/kibana/blob/master/x-pack/plugins/endpoint/server/test_data/all_alerts_data.json.

dplumlee · 2020-02-11T23:21:59Z

x-pack/plugins/endpoint/public/applications/endpoint/lib/flattened_promise.ts

+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+export function flattenedPromise<T>(): [Promise<T>, PromiseResolver<T>, PromiseRejector<T>] {


maybe a comment explaining what this is/why it's needed

dplumlee · 2020-02-11T23:25:20Z

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

+export const routingMiddlewareFactory: MiddlewareFactory = (coreStart, history) => {
+  const [promise, resolve] = flattenedPromise();
+
+  return {


@paul-tavares just to follow up with the comment @peluja1012 left on your pr, this is the new route action/middleware approach we're taking, would like to hear your thoughts

dplumlee · 2020-02-11T23:30:16Z

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

@@ -65,14 +67,14 @@ const AppRoot: React.FunctionComponent<RouterProps> = React.memo(({ basename, st
              );
            }}
          />
-          <Route path="/alerts" component={AlertIndex} />
+          <Route path="/alerts" render={() => <AlertIndex />} />


switched for consistency purposes and to be able to pass props down if/when needed

@kqualters-elastic needs to pass down props and we can keep consistency with the rest of the app

madirey · 2020-02-11T23:41:41Z

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

+    });
+
+    it('should return alertListData', () => {
+      const actual = store.getState().alerts.length;


Good test, but the name actual seems a little ambiguous.

madirey · 2020-02-11T23:47:10Z

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

+      next(action);
+      const state = api.getState();
+      if (action.type === 'userChangedUrl' && isOnAlertPage(state)) {
+        const response: AlertResultList = await coreStart.http.post(`/api/endpoint/alerts`, {


You can always do this later, but we're going to be removing POST support for this API. If you want to try changing it now, it probably will just require changing this to .get and then changing the the line below to query: paginationDataFromUrl(state) as HttpFetchQuery), instead of body: JSON.stringify(paginationDataFromUrl(state)), ... I'm fine with waiting until the next PR though, whatever you feel like.

peluja1012 · 2020-02-12T07:25:20Z

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

@@ -0,0 +1,12 @@
+/*


We should move this file to the view/alerts folder, as hooks are associated with views. Also I think it might be better to rename the file to something like use_alerts_selector.ts because we will probably add more hooks in the future and I don't think we should keep them all in one file.

peluja1012 · 2020-02-12T07:35:28Z

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

+
+    it("should recognize it's on the alert list page", () => {
+      const actual = isOnAlertPage(store.getState());
+      expect(actual).toBeTruthy();


I think .toBe(true) applies better to this case because we expect isOnAlertPage to return either true or false.

peluja1012 · 2020-02-12T07:37:18Z

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

+import { AlertResultList } from '../../../../../common/types';
+import { isOnAlertPage } from './selectors';
+
+describe('alert list middleware and selectors', () => {


We should update this now that you split up the tests.

peluja1012 · 2020-02-12T07:37:27Z

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

+  urlFromNewPageIndexParam,
+} from './selectors';
+
+describe('alert list middleware and selectors', () => {


We should update this now that you split up the tests.

peluja1012 · 2020-02-12T07:59:50Z

x-pack/test/functional/apps/endpoint/alert_list.ts

+    it('loads the Alert List Page', async () => {
+      await testSubjects.existOrFail('alertListPage');
+    });
+    it('includes policy list table', async () => {


Let's update this description 😉

peluja1012 · 2020-02-12T08:11:13Z

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

-      { id: 'timestamp' },
-      { id: 'archived' },
-      { id: 'malware_score' },
+      { id: 'Alert Type' },


You can probably keep the id's the same as they were before. To provide friendly names to the column headers you can use the EuiDataGridColumn display property. So you could do something like:

{ id: 'event_type', display: i18n.translate( 'xpack.endpoint.application.endpoint.alerts.eventType', { defaultMessage: 'Event Type', } }

oatkiller · 2020-02-13T22:09:16Z

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

+      const state = api.getState();
+      if (action.type === 'userChangedUrl' && isOnAlertPage(state)) {
+        const response: AlertResultList = await coreStart.http.get(`/api/endpoint/alerts`, {
+          query: paginationDataFromUrl(state) as HttpFetchQuery,


@oatkiller Cause I told him to 🙈

oatkiller · 2020-02-13T22:11:30Z

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

+ */
+export const paginationDataFromUrl = (state: AlertListState): qs.ParsedUrlQuery => {
+  // Removes the `?` from the beginning of query string if it exists
+  return state.location ? qs.parse(state.location.search.slice(1)) : {};


i think you need to call out / validate each value here. otherwise a malicious actor can give an unwitting nerd a url like somekibana.net/endpoint?badStuff=encodedHacks and then the nerd clicks the url and a request goes to kibana w/ unwitting nerd's creds but with whatever params were provided by malicious actor @dplumlee

@oatkiller We are doing pretty comprehensive validation on the backend FYI. So maybe not necessary? Always helps to have an extra layer, I s'pose.

I was also wondering this. I think once we start to grab data from the URL which is open to manipulation, we should do some sort of manipulation in order to avoid front-end code injections. I'm ok if we defer that to another Issue, but we should come up with a common approach.

paul-tavares · 2020-02-14T14:31:03Z

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

+const RouteCapture = memo(({ children }) => {
+  const location: EndpointAppLocation = useLocation();
+  const dispatch: (action: AppAction) => unknown = useDispatch();
+  dispatch({ type: 'userChangedUrl', payload: location });


I had mentioned this in my prior comment and just wanted to again make everyone aware - this will dispatch the action potentially BEFORE the component for the matched route is rendered. I think that is ok, but just wanted everyone to be aware of that behaviour.

paul-tavares · 2020-02-14T14:40:36Z

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

-      applyMiddleware(alertMiddlewareFactory(coreStart), appSagaFactory(coreStart))
+      applyMiddleware(
+        substateMiddlewareFactory(s => s.alertList, alertMiddlewareFactory(coreStart).middleware),
+        appSagaFactory(coreStart)


Just FYI. This is actually a bug @parkiino fixed at one point (should just be the sagaReduxMiddleware variable that was created on line 52). That being said, its also going away since Candace refactored it to use Middleware and remove Sagas

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

paul-tavares · 2020-02-14T14:49:50Z

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


-  const json = useSelector(selectors.alertListData);
+  const [visibleColumns, setVisibleColumns] = useState(() => columns.map(({ id }) => id));


Why use of local state and not redux store?

i dont know if we had a particular reason for not putting it in store beyond we didn't consider it part of the feature and it was just a side effect already built into the eui DataGrid component

paul-tavares · 2020-02-14T14:54:43Z

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

+    return {
+      pageIndex,
+      pageSize,
+      pageSizeOptions: [10, 50, 100],


We should all sync up on the values to use for page size. @parkiino and I are using [10, 20, 50], which I think we discussed with Bonnie.

paul-tavares · 2020-02-14T14:56:58Z

x-pack/plugins/endpoint/public/applications/endpoint/lib/flattened_promise.ts

@@ -0,0 +1,22 @@
+/*


Is this still used? should it be deleted?

paul-tavares · 2020-02-14T21:08:11Z

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

@@ -27,6 +28,11 @@ export const alertListReducer: Reducer<AlertListState, AppAction> = (
      ...state,
      ...action.payload,
    };
+  } else if (action.type === 'userChangedUrl') {


you are updating the Alerts store every time the URL changes - is that intentional? or did you forget to use isOnAlertsList()?

Also, when the user leaves the alert list page, the store for it should be reset.

i suppose, only time the action is dispatched currently is if the app is already confirmed to be on the alerts page

The action is dispatched anytime the URL changes - so navigating to /management or /policy will trigger the action.

@paul-tavares We didn't want to force other teams to use this approach so we are storing the location in the alerts piece of the store. If all of us want to use this, we can store the location at the top level of the store. The idea is to update location every time the url changes.

I created an issue for clearing the state when navigating away https://github.com/elastic/endpoint-app-team/issues/183

@peluja1012 I actually thought we had agreed to use this new approach instead of usePageId() when we talked through the revised implementation around using location provided by react-router. At least I was planning on refactoring the Policy/Management pages to use userChangedUrl instead once this merged in.

Not sure if storing it globally would facilitate things though - since currently we're building our reducers/middleware to NOT be aware of global state. Accessing the location information would be an issue thus why each concern needs to store it, no?

@paul-tavares Oh that's nice. I didn't realize we were all moving forward with this approach. It would certainly be easier the way the code is laid out now to have each concern store the location. I don't know if I like the fact that we'll need a copied and pasted reducer clause to update the location in each concern though. I would rather have location be global. We could refactor the code to allow for a global location piece of state or create a reducer helper to handle updating the location in each concern. Either way, I'd like to do this work in a separate PR. This PR is currently blocking work on Alert Details and Alerts Filtering, so I would like to get it merged as soon as we can.

agreed 👍

paul-tavares · 2020-02-14T21:13:50Z

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

+ * Returns a boolean based on whether or not the user is on the alerts page
+ */
+export const isOnAlertPage = (state: AlertListState): boolean => {
+  return state.location ? state.location.pathname.endsWith('/alerts') : false;


(optional I would suggest you do a strict === as in state.location.pathname === '/alerts'

kibanamachine · 2020-02-18T20:18:59Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 376da43

History

💚 Build #26949 succeeded d50c1c9c0e39ddf629fdb1cd4241b5916993ad45
💔 Build #26677 failed bd6d56a501618c445f67b85cf3a730fd729053ad
💔 Build #26668 failed 0afda20d62caa45eacf34f5830162f060290c2d1
💔 Build #26665 failed bac280e06daaec146c8a0cde4cd87ecf37cc6434
💔 Build #26277 failed 4cacdcb73fc49a398254c6becd1f843cf22f30fa

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

dplumlee added WIP Work in progress v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 Team:Endpoint Response Endpoint Response Team Feature:Endpoint Elastic Endpoint feature labels Feb 7, 2020

dplumlee force-pushed the alert-list-frontend-pagination branch from 96b3d0c to 4cd62dc Compare February 7, 2020 19:01

peluja1012 mentioned this pull request Feb 11, 2020

[Endpoint] Policy List UI route and initial view #56918

Merged

3 tasks

dplumlee force-pushed the alert-list-frontend-pagination branch 3 times, most recently from 997c0c7 to f6457a0 Compare February 11, 2020 23:19

dplumlee commented Feb 11, 2020

View reviewed changes

dplumlee removed the WIP Work in progress label Feb 11, 2020

dplumlee requested review from paul-tavares, oatkiller, peluja1012, kqualters-elastic, madirey and alexk307 February 11, 2020 23:26

dplumlee commented Feb 11, 2020

View reviewed changes

madirey reviewed Feb 11, 2020

View reviewed changes

peluja1012 reviewed Feb 12, 2020

View reviewed changes

oatkiller reviewed Feb 13, 2020

View reviewed changes

paul-tavares reviewed Feb 14, 2020

View reviewed changes

x-pack/plugins/endpoint/public/applications/endpoint/types.ts Show resolved Hide resolved

paul-tavares reviewed Feb 14, 2020

View reviewed changes

dplumlee force-pushed the alert-list-frontend-pagination branch 2 times, most recently from 84c66af to d50c1c9 Compare February 14, 2020 19:55

paul-tavares reviewed Feb 14, 2020

View reviewed changes

dplumlee added 12 commits February 18, 2020 13:22

front end basic pagination w/o url

4ac7212

url now mirrors pagination status

54861c9

moves logic into selectors

1fd5d96

starts to add tests

bb92691

refactors selector hooks

5a8fc38

adds more tests and refactors code for app consistency

cd53db7

splits tests into more specific files

e1da0a8

adds functional tests

7129486

addresses comments

8ed289e

switches router/url logic

2224a25

clean up code

b41a158

addresses comments

376da43

dplumlee force-pushed the alert-list-frontend-pagination branch from 08600f4 to 376da43 Compare February 18, 2020 18:23

dplumlee merged commit f6dc674 into elastic:master Feb 18, 2020

dplumlee mentioned this pull request Feb 18, 2020

[7.x] Alert list frontend pagination (#57142) #57928

Merged

dplumlee added a commit to dplumlee/kibana that referenced this pull request Feb 18, 2020

Alert list frontend pagination (elastic#57142)

f0a78d9

dplumlee deleted the alert-list-frontend-pagination branch February 18, 2020 22:39

dplumlee added a commit that referenced this pull request Feb 19, 2020

Alert list frontend pagination (#57142) (#57928)

07bf0df


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

Alert list frontend pagination #57142

Alert list frontend pagination #57142

Conversation

dplumlee commented Feb 7, 2020 • edited Loading

Summary

Checklist

For maintainers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madirey Feb 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peluja1012 Feb 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oatkiller Feb 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peluja1012 Feb 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Feb 18, 2020

💚 Build Succeeded

History

dplumlee commented Feb 7, 2020 •

edited

Loading

madirey Feb 11, 2020 •

edited

Loading

peluja1012 Feb 12, 2020 •

edited

Loading

oatkiller Feb 13, 2020 •

edited

Loading

peluja1012 Feb 18, 2020 •

edited

Loading