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

Task/hostlist pagination #63722

Merged
merged 21 commits into from
Apr 24, 2020
Merged
Show file tree
Hide file tree
Changes from 17 commits
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 @@ -4,14 +4,19 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { HostListPagination, ServerApiError } from '../../types';
import { ServerApiError } from '../../types';
import { HostResultList, HostInfo } from '../../../../../common/types';

interface ServerReturnedHostList {
type: 'serverReturnedHostList';
payload: HostResultList;
}

interface ServerFailedToReturnHostList {
type: 'serverFailedToReturnHostList';
payload: ServerApiError;
}

interface ServerReturnedHostDetails {
type: 'serverReturnedHostDetails';
payload: HostInfo;
Expand All @@ -22,13 +27,8 @@ interface ServerFailedToReturnHostDetails {
payload: ServerApiError;
}

interface UserPaginatedHostList {
type: 'userPaginatedHostList';
payload: HostListPagination;
}

export type HostAction =
| ServerReturnedHostList
| ServerFailedToReturnHostList
| ServerReturnedHostDetails
| ServerFailedToReturnHostDetails
| UserPaginatedHostList;
| ServerFailedToReturnHostDetails;
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
/*
* 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 { CoreStart } from 'kibana/public';
import { DepsStartMock, depsStartMock } from '../../mocks';
import { AppAction, HostState, HostIndexUIQueryParams } from '../../types';
import { Immutable } from '../../../../../common/types';
import { History, createBrowserHistory } from 'history';
import { hostMiddlewareFactory } from './middleware';
import { applyMiddleware, Store, createStore } from 'redux';
import { hostListReducer } from './reducer';
import { coreMock } from 'src/core/public/mocks';
import { urlFromQueryParams } from '../../view/hosts/url_from_query_params';
import { uiQueryParams } from './selectors';

describe('host list pagination: ', () => {
let store: Store<Immutable<HostState>, Immutable<AppAction>>;
let fakeCoreStart: jest.Mocked<CoreStart>;
let depsStart: DepsStartMock;
let history: History<never>;
let queryParams: () => HostIndexUIQueryParams;

let historyPush: (params: HostIndexUIQueryParams) => void;
beforeEach(() => {
fakeCoreStart = coreMock.createStart();
depsStart = depsStartMock();
history = createBrowserHistory();

const middleware = hostMiddlewareFactory(fakeCoreStart, depsStart);
store = createStore(hostListReducer, applyMiddleware(middleware));

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

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

historyPush = (nextQueryParams: HostIndexUIQueryParams): void => {
return history.push(urlFromQueryParams(nextQueryParams));
};
});

describe('when a new page size is passed', () => {
beforeEach(() => {
historyPush({ ...queryParams(), page_size: '20' });
});
it('should modify the url correctly', () => {
expect(queryParams()).toMatchInlineSnapshot(`
Object {
"page_index": "0",
"page_size": "20",
}

Choose a reason for hiding this comment

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

@parkiino - are these values the defaults we are using? It would be good to test the defaults for this test case and have a separate test case for invalid values that revert to defaults.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the defaults are going to be page_index: 0, and page_size: 10

`);
});
});
describe('when an invalid page size is passed', () => {
Copy link
Contributor

@paul-tavares paul-tavares Apr 22, 2020

Choose a reason for hiding this comment

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

Are there any other variations you should be testing? Example: what if multiple page_size params are in the URL? what if page_size is -1? or a string (jsdkfjkds)?

Also - might be worth testing that the parameters get correcly passed down to the middleware/API.

Choose a reason for hiding this comment

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

Yes, I agree with @paul-tavares - @parkiino we should test the following invalid values as well:

  • -1
  • invalid UUID
  • valid UUID + special characters
  • valid UUID + JS embed
  • valid UUID + unicode
  • empty string

If it's too much of a lift, I can also test these examples manually and create another ticket to automate these tests in the future. I will also manually test when page_index is more than what we expect to make sure it results in the same bug I found in the policy list URL pagination

beforeEach(() => {
historyPush({ ...queryParams(), page_size: '1' });
});
it('should modify the page size in the url to the default page size', () => {
expect(queryParams()).toEqual({ page_index: '0', page_size: '10' });
});
});

describe('when a new page index is passed', () => {
beforeEach(() => {
historyPush({ ...queryParams(), page_index: '2' });
});
it('should modify the page index in the url correctly', () => {
expect(queryParams()).toEqual({ page_index: '2', page_size: '10' });
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,12 @@

import { createStore, Dispatch, Store } from 'redux';
import { HostAction, hostListReducer } from './index';
import { HostListState } from '../../types';
import { HostState } from '../../types';
import { listData } from './selectors';
import { mockHostResultList } from './mock_host_result_list';

describe('HostList store concerns', () => {
let store: Store<HostListState>;
let store: Store<HostState>;
let dispatch: Dispatch<HostAction>;
const createTestStore = () => {
store = createStore(hostListReducer);
Expand All @@ -37,6 +37,11 @@ describe('HostList store concerns', () => {
pageIndex: 0,
total: 0,
loading: false,
error: undefined,
details: undefined,
detailsLoading: false,
detailsError: undefined,
location: undefined,
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { coreMock } from '../../../../../../../../src/core/public/mocks';
import { History, createBrowserHistory } from 'history';
import { hostListReducer, hostMiddlewareFactory } from './index';
import { HostResultList, Immutable } from '../../../../../common/types';
import { HostListState } from '../../types';
import { HostState } from '../../types';
import { AppAction } from '../action';
import { listData } from './selectors';
import { DepsStartMock, depsStartMock } from '../../mocks';
Expand All @@ -20,7 +20,7 @@ describe('host list middleware', () => {
let fakeCoreStart: jest.Mocked<CoreStart>;
let depsStart: DepsStartMock;
let fakeHttpServices: jest.Mocked<HttpSetup>;
type HostListStore = Store<Immutable<HostListState>, Immutable<AppAction>>;
type HostListStore = Store<Immutable<HostState>, Immutable<AppAction>>;
let store: HostListStore;
let getState: HostListStore['getState'];
let dispatch: HostListStore['dispatch'];
Expand All @@ -41,7 +41,7 @@ describe('host list middleware', () => {
dispatch = store.dispatch;
history = createBrowserHistory();
});
test('handles `userChangedUrl`', async () => {
it('handles `userChangedUrl`', async () => {
const apiResponse = getEndpointListApiResponse();
fakeHttpServices.post.mockResolvedValue(apiResponse);
expect(fakeHttpServices.post).not.toHaveBeenCalled();
Expand All @@ -56,7 +56,7 @@ describe('host list middleware', () => {
await sleep();
Copy link
Contributor

Choose a reason for hiding this comment

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

we should remove the sleeps and instead use the new utility that allows you to wait for a given action to be dispatched - in this case, its probably the serverReturnedHostListData action.

expect(fakeHttpServices.post).toHaveBeenCalledWith('/api/endpoint/metadata', {
body: JSON.stringify({
paging_properties: [{ page_index: 0 }, { page_size: 10 }],
paging_properties: [{ page_index: '0' }, { page_size: '10' }],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the page_index and page_size are strings because they are coming from the url now

Copy link
Contributor

Choose a reason for hiding this comment

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

This does not seem valid. The Server Schema for the metadata POST request indicates these should be numbers.
See

page_size: schema.number({ defaultValue: 10, min: 1, max: 10000 }),

if you are sending strings to the server now (in the browser) and that is succeeding, then perhaps the platform translates it back to a number 🤷‍♂️

}),
});
expect(listData(getState())).toEqual(apiResponse.hosts.map(hostInfo => hostInfo.metadata));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,34 +4,62 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { isOnHostPage, hasSelectedHost, uiQueryParams, listData } from './selectors';
import { HostState } from '../../types';
import { ImmutableMiddlewareFactory } from '../../types';
import { pageIndex, pageSize, isOnHostPage, hasSelectedHost, uiQueryParams } from './selectors';
import { HostListState } from '../../types';

export const hostMiddlewareFactory: ImmutableMiddlewareFactory<HostListState> = coreStart => {
export const hostMiddlewareFactory: ImmutableMiddlewareFactory<HostState> = coreStart => {
return ({ getState, dispatch }) => next => async action => {
next(action);
const state = getState();
if (
(action.type === 'userChangedUrl' &&
isOnHostPage(state) &&
hasSelectedHost(state) !== true) ||
action.type === 'userPaginatedHostList'
action.type === 'userChangedUrl' &&
isOnHostPage(state) &&
hasSelectedHost(state) !== true
) {
const hostPageIndex = pageIndex(state);
const hostPageSize = pageSize(state);
const response = await coreStart.http.post('/api/endpoint/metadata', {
body: JSON.stringify({
paging_properties: [{ page_index: hostPageIndex }, { page_size: hostPageSize }],
}),
});
response.request_page_index = hostPageIndex;
dispatch({
type: 'serverReturnedHostList',
payload: response,
});
const { page_index: pageIndex, page_size: pageSize } = uiQueryParams(state);
try {
const response = await coreStart.http.post('/api/endpoint/metadata', {
body: JSON.stringify({
paging_properties: [{ page_index: pageIndex }, { page_size: pageSize }],
}),
});
response.request_page_index = Number(pageIndex);
dispatch({
type: 'serverReturnedHostList',
payload: response,
});
} catch (error) {
dispatch({
type: 'serverFailedToReturnHostList',
payload: error,
});
}
}
if (action.type === 'userChangedUrl' && hasSelectedHost(state) !== false) {
// If user navigated directly to a host details page, load the host list
if (listData(state).length === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If a list API call was done, but there were no hosts in the list, then this if() would alway match. Its unlikely to happen now, but when we introduce the KQL bar, then I can see this being an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's true. i'm not sure what to do at that point once the kql bar is introduced. maybe it will be possible to additionally check if filters are active at that point? otherwise, for now i think this makes the most sense for the details page

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, this would be where perhaps we should track wether the list has been loaded (once) at least. Not a big deal. I guess we can re-evaluate when we introduce filtering

const { page_index: pageIndex, page_size: pageSize } = uiQueryParams(state);
try {
const response = await coreStart.http.post('/api/endpoint/metadata', {
Copy link
Contributor

Choose a reason for hiding this comment

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

I was going to suggest that we add typing to response by doing:

Suggested change
const response = await coreStart.http.post('/api/endpoint/metadata', {
const response = await coreStart.http.post<HostInfo>('/api/endpoint/metadata', {

But I just realized that you then get an error because HostInfo type has been made Imutable (cc/ @oatkiller - maybe making everthing Immutable is the best approach? :) )

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure i'm following. HostInfo is immutable?

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 i think i can type the response, but wouldn't it be HostResultList?

Copy link
Contributor

Choose a reason for hiding this comment

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

My mistake - @parkiino you are correct - you should use HostResultList type here :)

body: JSON.stringify({
paging_properties: [{ page_index: pageIndex }, { page_size: pageSize }],
}),
});
response.request_page_index = Number(pageIndex);
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the api doesn't return the page index that the ui would use so we manually set it in the ui

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider typing response with a type that's shared with the server. If the server should be returning this value, why don't you just change the code there instead of here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the server returns what would originally be returned from an elastic search query, which is the page index within elastic i think? or something like that. so it seemed to make more sense to be consistent with that

dispatch({
type: 'serverReturnedHostList',
payload: response,
});
} catch (error) {
dispatch({
type: 'serverFailedToReturnHostList',
payload: error,
});
paul-tavares marked this conversation as resolved.
Show resolved Hide resolved
}
}

// call the host details api
const { selected_host: selectedHost } = uiQueryParams(state);
try {
const response = await coreStart.http.get(`/api/endpoint/metadata/${selectedHost}`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,23 +4,27 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { HostListState, ImmutableReducer } from '../../types';
import { Immutable } from '../../../../../common/types';
import { HostState, ImmutableReducer } from '../../types';
import { AppAction } from '../action';
import { isOnHostPage, hasSelectedHost } from './selectors';

const initialState = (): HostListState => {
const initialState = (): HostState => {
return {
hosts: [],
pageSize: 10,
pageIndex: 0,
total: 0,
loading: false,
detailsError: undefined,
error: undefined,
details: undefined,
detailsLoading: false,
detailsError: undefined,
location: undefined,
};
};

export const hostListReducer: ImmutableReducer<HostListState, AppAction> = (
export const hostListReducer: ImmutableReducer<HostState, AppAction> = (
state = initialState(),
action
) => {
Expand All @@ -38,30 +42,77 @@ export const hostListReducer: ImmutableReducer<HostListState, AppAction> = (
pageSize,
pageIndex,
loading: false,
error: undefined,
};
} else if (action.type === 'serverFailedToReturnHostList') {
return {
...state,
error: action.payload,
loading: false,
};
} else if (action.type === 'serverReturnedHostDetails') {
return {
...state,
details: action.payload.metadata,
detailsLoading: false,
detailsError: undefined,
};
} else if (action.type === 'serverFailedToReturnHostDetails') {
return {
...state,
detailsError: action.payload,
detailsLoading: false,
};
} else if (action.type === 'userPaginatedHostList') {
return {
} else if (action.type === 'userChangedUrl') {
const newState: Immutable<HostState> = {
...state,
...action.payload,
loading: true,
location: action.payload,
};
} else if (action.type === 'userChangedUrl') {
const isCurrentlyOnListPage = isOnHostPage(newState) && !hasSelectedHost(newState);
const wasPreviouslyOnListPage = isOnHostPage(state) && !hasSelectedHost(state);
const isCurrentlyOnDetailsPage = isOnHostPage(newState) && hasSelectedHost(newState);
const wasPreviouslyOnDetailsPage = isOnHostPage(state) && hasSelectedHost(state);

// if on the host list page for the first time, return new location and load list
if (isCurrentlyOnListPage) {
if (!wasPreviouslyOnListPage) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we currently refresh the list if the user closes a host details flyout

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the logic that decides to set loading to true here should be used to decide when to actually make the request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as opposed to in the middleware? not really sure i understand what you mean

return {
...state,
location: action.payload,
loading: true,
error: undefined,
detailsError: undefined,
};
}
} else if (isCurrentlyOnDetailsPage) {
// if previous page was the list or another host details page, load host details only
if (wasPreviouslyOnDetailsPage || wasPreviouslyOnListPage) {
return {
...state,
location: action.payload,
detailsLoading: true,
error: undefined,
detailsError: undefined,
};
} else {
// if previous page was not host list or host details, load both list and details
return {
...state,
location: action.payload,
loading: true,
detailsLoading: true,
error: undefined,
detailsError: undefined,
};
}
}
// otherwise we are not on a host list or details page
return {
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably should reset the entire state as in:

return initialState();

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 know there's something going on to move the location out of all the individual reducers but that hasn't merged yet right? initially i did it that way so that the location would still be updated in the host reducer

...state,
location: action.payload,
error: undefined,
detailsError: undefined,
};
}

return state;
};
Loading