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

Add a flyout to alert list. #57926

Merged
merged 23 commits into from
Feb 21, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -7,33 +7,47 @@
import { Store, createStore, applyMiddleware } from 'redux';
import { History } from 'history';
import { alertListReducer } from './reducer';
import { AlertListState } from '../../types';
import { AlertListState, AlertingIndexUIQueryParams } from '../../types';
import { alertMiddlewareFactory } from './middleware';
import { AppAction } from '../action';
import { coreMock } from 'src/core/public/mocks';
import { createBrowserHistory } from 'history';
import { urlFromNewPageSizeParam, uiQueryParams, urlFromNewPageIndexParam } from './selectors';
import { uiQueryParams } from './selectors';
import { urlFromQueryParams } from '../../view/alerts/url_from_query_params';

describe('alert list pagination', () => {
let store: Store<AlertListState, AppAction>;
let coreStart: ReturnType<typeof coreMock.createStart>;
let history: History<never>;
let queryParams: () => AlertingIndexUIQueryParams;
/**
* Update the history with a new `AlertingIndexUIQueryParams`
*/
let historyPush: (params: AlertingIndexUIQueryParams) => void;
beforeEach(() => {
coreStart = coreMock.createStart();
history = createBrowserHistory();

const middleware = alertMiddlewareFactory(coreStart);
store = createStore(alertListReducer, applyMiddleware(middleware));

history.listen(location => {
store.dispatch({ type: 'userChangedUrl', payload: location });
});

queryParams = () => uiQueryParams(store.getState());

historyPush = (nextQueryParams: AlertingIndexUIQueryParams): void => {
return history.push(urlFromQueryParams(nextQueryParams));
};
});
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 });
historyPush({ ...queryParams(), page_size: '1' });
});
it('should modify the url correctly', () => {
const actualPaginationQuery = uiQueryParams(store.getState());
expect(actualPaginationQuery).toMatchInlineSnapshot(`
expect(queryParams()).toMatchInlineSnapshot(`
Object {
"page_size": "1",
}
Expand All @@ -42,13 +56,10 @@ describe('alert list pagination', () => {

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 });
historyPush({ ...queryParams(), page_index: '1' });
});
it('should modify the url in the correct order', () => {
const actualPaginationQuery = uiQueryParams(store.getState());
expect(actualPaginationQuery).toMatchInlineSnapshot(`
expect(queryParams()).toMatchInlineSnapshot(`
Object {
"page_index": "1",
"page_size": "1",
Expand All @@ -60,28 +71,23 @@ describe('alert list pagination', () => {

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 });
historyPush({ ...queryParams(), page_index: '1' });
});
it('should modify the url correctly', () => {
const actualPaginationQuery = uiQueryParams(store.getState());
expect(actualPaginationQuery).toMatchInlineSnapshot(`
expect(queryParams()).toMatchInlineSnapshot(`
Object {
"page_index": "1",
}
`);
});

describe('and then a new page size is passed', () => {
// TODO, move this test to react land, since thats where this logic lives now. is that good?
xdescribe('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 });
historyPush({ ...queryParams(), page_size: '1' });
});
it('should modify the url correctly and reset index to `0`', () => {
const actualPaginationQuery = uiQueryParams(store.getState());
expect(actualPaginationQuery).toMatchInlineSnapshot(`
expect(queryParams()).toMatchInlineSnapshot(`
Object {
"page_size": "1",
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import qs from 'querystring';
import querystring from 'querystring';
import {
createSelector,
createStructuredSelector as createStructuredSelectorWithBadType,
Expand Down Expand Up @@ -52,7 +52,7 @@ export const uiQueryParams: (
const data: AlertingIndexUIQueryParams = {};
if (location) {
// Removes the `?` from the beginning of query string if it exists
const query = qs.parse(location.search.slice(1));
const query = querystring.parse(location.search.slice(1));

/**
* Build an AlertingIndexUIQueryParams object with keys from the query.
Expand Down Expand Up @@ -89,64 +89,6 @@ export const apiQueryParams: (
})
);

/**
* Returns a function that takes in a new page size and returns a new query param string
*/
export const urlFromNewPageSizeParam: (
state: AlertListState
) => (newPageSize: number) => string = createSelector(uiQueryParams, paramData => {
return newPageSize => {
const queryParams: AlertingIndexUIQueryParams = { ...paramData };
queryParams.page_size = newPageSize.toString();

/**
* Reset the page index when changing page size.
*/
if (queryParams.page_index !== undefined) {
delete queryParams.page_index;
}
return '?' + qs.stringify(queryParams);
};
});

/**
* Returns a function that takes in a new page index and returns a new query param string
*/
export const urlFromNewPageIndexParam: (
state: AlertListState
) => (newPageIndex: number) => string = createSelector(uiQueryParams, paramData => {
return newPageIndex => {
const queryParams: AlertingIndexUIQueryParams = { ...paramData };
queryParams.page_index = newPageIndex.toString();
return '?' + qs.stringify(queryParams);
};
});

/**
* Returns a url like the current one, but with a new alert id.
*/
export const urlWithSelectedAlert: (
state: AlertListState
) => (alertID: string) => string = createSelector(uiQueryParams, paramData => {
return (alertID: string) => {
const queryParams = { ...paramData };
queryParams.selected_alert = alertID;
return '?' + qs.stringify(queryParams);
};
});

/**
* Returns a url like the current one, but with no alert id
*/
export const urlWithoutSelectedAlert: (state: AlertListState) => string = createSelector(
uiQueryParams,
urlPaginationData => {
const queryParams = { ...urlPaginationData };
delete queryParams.selected_alert;
return '?' + qs.stringify(queryParams);
}
);

export const hasSelectedAlert: (state: AlertListState) => boolean = createSelector(
uiQueryParams,
({ selected_alert: selectedAlert }) => selectedAlert !== undefined
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
import { i18n } from '@kbn/i18n';
import { useHistory, Link } from 'react-router-dom';
import { FormattedDate } from 'react-intl';
import { urlFromQueryParams } from './url_from_query_params';
import { AlertData } from '../../../../../common/types';
import * as selectors from '../../store/alerts/selectors';
import { useAlertListSelector } from './hooks/use_alerts_selector';
Expand Down Expand Up @@ -82,28 +83,39 @@ export const AlertIndex = memo(() => {
}, []);

const { pageIndex, pageSize, total } = useAlertListSelector(selectors.alertListPagination);
const urlFromNewPageSizeParam = useAlertListSelector(selectors.urlFromNewPageSizeParam);
const urlWithSelectedAlert = useAlertListSelector(selectors.urlWithSelectedAlert);
const urlWithoutSelectedAlert = useAlertListSelector(selectors.urlWithoutSelectedAlert);
const urlFromNewPageIndexParam = useAlertListSelector(selectors.urlFromNewPageIndexParam);
const alertListData = useAlertListSelector(selectors.alertListData);
const hasSelectedAlert = useAlertListSelector(selectors.hasSelectedAlert);
const queryParams = useAlertListSelector(selectors.uiQueryParams);

const onChangeItemsPerPage = useCallback(
newPageSize => history.push(urlFromNewPageSizeParam(newPageSize)),
[history, urlFromNewPageSizeParam]
newPageSize => {
const newQueryParms = { ...queryParams };
newQueryParms.page_size = newPageSize;
delete newQueryParms.page_index;
const relativeURL = urlFromQueryParams(newQueryParms);
return history.push(relativeURL);
},
[history, queryParams]
);

const onChangePage = useCallback(
newPageIndex => history.push(urlFromNewPageIndexParam(newPageIndex)),
[history, urlFromNewPageIndexParam]
newPageIndex => {
return history.push(
urlFromQueryParams({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@peluja1012 peluja1012 Feb 21, 2020

Choose a reason for hiding this comment

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

@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/:
close_flyout_bug mov

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

...queryParams,
page_index: newPageIndex,
})
);
},
[history, queryParams]
);

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

const handleFlyoutClose = useCallback(() => {
history.push(urlWithoutSelectedAlert);
}, [history, urlWithoutSelectedAlert]);
const { selected_alert, ...paramsWithoutSelectedAlert } = queryParams;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

history.push(urlFromQueryParams(paramsWithoutSelectedAlert));
}, [history, queryParams]);

const datesForRows: Map<AlertData, Date> = useMemo(() => {
return new Map(
Expand All @@ -123,7 +135,10 @@ export const AlertIndex = memo(() => {

if (columnId === 'alert_type') {
return (
<Link data-testid="alertTypeCellLink" to={urlWithSelectedAlert('TODO')}>
<Link
data-testid="alertTypeCellLink"
to={urlFromQueryParams({ ...queryParams, selected_alert: 'TODO' })}
>
{i18n.translate(
'xpack.endpoint.application.endpoint.alerts.alertType.maliciousFileDescription',
{
Expand Down Expand Up @@ -173,7 +188,7 @@ export const AlertIndex = memo(() => {
}
return null;
};
}, [alertListData, datesForRows, pageSize, total, urlWithSelectedAlert]);
}, [alertListData, datesForRows, pageSize, queryParams, total]);

const pagination = useMemo(() => {
return {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* 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 querystring from 'querystring';
import { AlertingIndexUIQueryParams } from '../../types';

/**
* Return a relative URL for `AlertingIndexUIQueryParams`.
* usage:
*
* ```ts
* // Replace this with however you get state, e.g. useSelector in react
* const queryParams = selectors.uiQueryParams(store.getState())
*
* // same as current url, but page_index is now 3
* const relativeURL = urlFromQueryParams({ ...queryParams, page_index: 3 })
*
* // now use relativeURL in the 'href' of a link, the 'to' of a react-router-dom 'Link' or history.push, history.replace
* ```
*/
export function urlFromQueryParams(queryParams: AlertingIndexUIQueryParams): string {
const search = querystring.stringify(queryParams);
return search === '' ? '' : '?' + search;
}