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.

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

Alert list frontend pagination #57142

Merged
merged 12 commits into from
Feb 18, 2020
55 changes: 33 additions & 22 deletions x-pack/plugins/endpoint/public/applications/endpoint/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,22 @@ import * as React from 'react';
import ReactDOM from 'react-dom';
import { CoreStart, AppMountParameters } from 'kibana/public';
import { I18nProvider, FormattedMessage } from '@kbn/i18n/react';
import { Route, BrowserRouter, Switch } from 'react-router-dom';
import { Provider } from 'react-redux';
import { Route, Switch, BrowserRouter, useLocation } from 'react-router-dom';
import { Provider, useDispatch } from 'react-redux';
import { Store } from 'redux';
import { memo } from 'react';
import { appStoreFactory } from './store';
import { AlertIndex } from './view/alerts';
import { ManagementList } from './view/managing';
import { PolicyList } from './view/policy';
import { AppAction } from './store/action';
import { EndpointAppLocation } from './types';

/**
* This module will be loaded asynchronously to reduce the bundle size of your plugin's main bundle.
*/
export function renderApp(coreStart: CoreStart, { appBasePath, element }: AppMountParameters) {
coreStart.http.get('/api/endpoint/hello-world');

const store = appStoreFactory(coreStart);

ReactDOM.render(<AppRoot basename={appBasePath} store={store} />, element);
Expand All @@ -31,6 +33,13 @@ export function renderApp(coreStart: CoreStart, { appBasePath, element }: AppMou
};
}

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

Choose a reason for hiding this comment

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

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.

return <>{children}</>;
});

interface RouterProps {
basename: string;
store: Store;
Expand All @@ -40,25 +49,27 @@ const AppRoot: React.FunctionComponent<RouterProps> = React.memo(({ basename, st
<Provider store={store}>
<I18nProvider>
<BrowserRouter basename={basename}>
<Switch>
<Route
exact
path="/"
render={() => (
<h1 data-test-subj="welcomeTitle">
<FormattedMessage id="xpack.endpoint.welcomeTitle" defaultMessage="Hello World" />
</h1>
)}
/>
<Route path="/management" component={ManagementList} />
<Route path="/alerts" component={AlertIndex} />
<Route path="/policy" exact component={PolicyList} />
<Route
render={() => (
<FormattedMessage id="xpack.endpoint.notFound" defaultMessage="Page Not Found" />
)}
/>
</Switch>
<RouteCapture>
<Switch>
<Route
exact
path="/"
render={() => (
<h1 data-test-subj="welcomeTitle">
<FormattedMessage id="xpack.endpoint.welcomeTitle" defaultMessage="Hello World" />
</h1>
)}
/>
<Route path="/management" component={ManagementList} />
<Route path="/alerts" render={() => <AlertIndex />} />
<Route path="/policy" exact component={PolicyList} />
<Route
render={() => (
<FormattedMessage id="xpack.endpoint.notFound" defaultMessage="Page Not Found" />
)}
/>
</Switch>
</RouteCapture>
</BrowserRouter>
</I18nProvider>
</Provider>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { Immutable } from '../../../../../common/types';
import { AlertListData } from '../../types';

interface ServerReturnedAlertsData {
type: 'serverReturnedAlertsData';
payload: AlertListData;
readonly type: 'serverReturnedAlertsData';
readonly payload: Immutable<AlertListData>;
}

export type AlertAction = ServerReturnedAlertsData;
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { Store, createStore, applyMiddleware } from 'redux';
import { History } from 'history';
import { alertListReducer } from './reducer';
import { AlertListState } from '../../types';
import { alertMiddlewareFactory } from './middleware';
import { AppAction } from '../action';
import { coreMock } from 'src/core/public/mocks';
import { AlertResultList } from '../../../../../common/types';
import { isOnAlertPage } from './selectors';
import { createBrowserHistory } from 'history';

describe('alert list tests', () => {
let store: Store<AlertListState, AppAction>;
let coreStart: ReturnType<typeof coreMock.createStart>;
let history: History<never>;
beforeEach(() => {
coreStart = coreMock.createStart();
history = createBrowserHistory();
const middleware = alertMiddlewareFactory(coreStart);
store = createStore(alertListReducer, applyMiddleware(middleware));
});
describe('when the user navigates to the alert list page', () => {
beforeEach(() => {
coreStart.http.get.mockImplementation(async () => {
const response: AlertResultList = {
alerts: [
{
'@timestamp': new Date(1542341895000),
agent: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we want to have less silly data?

Copy link
Contributor

Choose a reason for hiding this comment

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

id: 'ced9c68e-b94a-4d66-bb4c-6106514f0a2f',
version: '3.0.0',
},
event: {
action: 'open',
},
file_classification: {
malware_classification: {
score: 3,
},
},
host: {
hostname: 'HD-c15-bc09190a',
ip: '10.179.244.14',
os: {
name: 'Windows',
},
},
thread: {},
},
],
total: 1,
request_page_size: 10,
request_page_index: 0,
result_from_index: 0,
};
return response;
});

// Simulates user navigating to the /alerts page
store.dispatch({
type: 'userChangedUrl',
payload: {
...history.location,
pathname: '/alerts',
},
});
});

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

it('should return alertListData', () => {
const actualResponseLength = store.getState().alerts.length;
expect(actualResponseLength).toEqual(1);
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { Store, createStore, applyMiddleware } from 'redux';
import { History } from 'history';
import { alertListReducer } from './reducer';
import { AlertListState } from '../../types';
import { alertMiddlewareFactory } from './middleware';
import { AppAction } from '../action';
import { coreMock } from 'src/core/public/mocks';
import { createBrowserHistory } from 'history';
import {
urlFromNewPageSizeParam,
paginationDataFromUrl,
urlFromNewPageIndexParam,
} from './selectors';

describe('alert list pagination', () => {
let store: Store<AlertListState, AppAction>;
let coreStart: ReturnType<typeof coreMock.createStart>;
let history: History<never>;
beforeEach(() => {
coreStart = coreMock.createStart();
history = createBrowserHistory();
const middleware = alertMiddlewareFactory(coreStart);
store = createStore(alertListReducer, applyMiddleware(middleware));
});
describe('when the user navigates to the alert list page', () => {
describe('when a new page size is passed', () => {
beforeEach(() => {
const urlPageSizeSelector = urlFromNewPageSizeParam(store.getState());
history.push(urlPageSizeSelector(1));
store.dispatch({ type: 'userChangedUrl', payload: history.location });
});
it('should modify the url correctly', () => {
const actualPaginationQuery = paginationDataFromUrl(store.getState());
expect(actualPaginationQuery).toMatchInlineSnapshot(`
Object {
"page_size": "1",
}
`);
});

describe('and then a new page index is passed', () => {
beforeEach(() => {
const urlPageIndexSelector = urlFromNewPageIndexParam(store.getState());
history.push(urlPageIndexSelector(1));
store.dispatch({ type: 'userChangedUrl', payload: history.location });
});
it('should modify the url in the correct order', () => {
const actualPaginationQuery = paginationDataFromUrl(store.getState());
expect(actualPaginationQuery).toMatchInlineSnapshot(`
Object {
"page_index": "1",
"page_size": "1",
}
`);
});
});
});

describe('when a new page index is passed', () => {
beforeEach(() => {
const urlPageIndexSelector = urlFromNewPageIndexParam(store.getState());
history.push(urlPageIndexSelector(1));
store.dispatch({ type: 'userChangedUrl', payload: history.location });
});
it('should modify the url correctly', () => {
const actualPaginationQuery = paginationDataFromUrl(store.getState());
expect(actualPaginationQuery).toMatchInlineSnapshot(`
Object {
"page_index": "1",
}
`);
});

describe('and then a new page size is passed', () => {
beforeEach(() => {
const urlPageSizeSelector = urlFromNewPageSizeParam(store.getState());
history.push(urlPageSizeSelector(1));
store.dispatch({ type: 'userChangedUrl', payload: history.location });
});
it('should modify the url correctly and reset index to `0`', () => {
const actualPaginationQuery = paginationDataFromUrl(store.getState());
expect(actualPaginationQuery).toMatchInlineSnapshot(`
Object {
"page_index": "0",
"page_size": "1",
}
`);
});
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,19 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { parse } from 'query-string';
import { HttpFetchQuery } from 'src/core/public';
import { HttpFetchQuery } from 'kibana/public';
import { AlertResultList } from '../../../../../common/types';
import { AppAction } from '../action';
import { MiddlewareFactory, AlertListData } from '../../types';

export const alertMiddlewareFactory: MiddlewareFactory = coreStart => {
const qp = parse(window.location.search.slice(1), { sort: false });
import { MiddlewareFactory, AlertListState } from '../../types';
import { isOnAlertPage, paginationDataFromUrl } from './selectors';

export const alertMiddlewareFactory: MiddlewareFactory<AlertListState> = coreStart => {
return api => next => async (action: AppAction) => {
next(action);
if (action.type === 'userNavigatedToPage' && action.payload === 'alertsPage') {
const response: AlertListData = await coreStart.http.get('/api/endpoint/alerts', {
query: qp as HttpFetchQuery,
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,
});
api.dispatch({ type: 'serverReturnedAlertsData', payload: response });
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ const initialState = (): AlertListState => {
request_page_index: 0,
result_from_index: 0,
total: 0,
location: undefined,
};
};

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

Choose a reason for hiding this comment

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

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.

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 suppose, only time the action is dispatched currently is if the app is already confirmed to be on the alerts page

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@peluja1012 peluja1012 Feb 18, 2020

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed 👍

return {
...state,
location: action.payload,
};
}

return state;
Expand Down
Loading