Task/hostlist pagination #63722

parkiino · 2020-04-16T14:56:16Z

Summary

**Parent Ticket: https://github.com/elastic/endpoint-app-team/issues/331 **

This PR drives host pagination through the URL.

Additionally addresses some wrapping/truncation problems with the hostname in host details as well as truncates all longer values in the host list so that the list is max one line.

Pagination values in the url
Properly wraps long hostnames in the host details flyout: https://github.com/elastic/endpoint-app-team/issues/333
Makes host list entries take up one row (as opposed to wrapping onto multiple lines)
Fixes a bug: https://github.com/elastic/endpoint-app-team/issues/341
Host list uses Page View component
tests using all types of wayyyys

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

…ination

elasticmachine · 2020-04-21T21:46:30Z

Pinging @elastic/endpoint-app-team (Feature:Endpoint)

elasticmachine · 2020-04-21T21:46:31Z

Pinging @elastic/endpoint-management (Team:Endpoint Management)

paul-tavares · 2020-04-22T13:30:42Z

x-pack/plugins/endpoint/public/applications/endpoint/store/hosts/host_pagination.test.ts

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


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.

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

paul-tavares · 2020-04-22T13:37:22Z

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

@@ -56,7 +56,7 @@ describe('host list middleware', () => {
    await sleep();


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.

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

paul-tavares · 2020-04-22T13:41:49Z

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

    }
    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) {


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.

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

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

paul-tavares · 2020-04-22T13:46:53Z

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

+      if (listData(state).length === 0) {
+        const { page_index: pageIndex, page_size: pageSize } = uiQueryParams(state);
+        try {
+          const response = await coreStart.http.post('/api/endpoint/metadata', {


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? :) )

not sure i'm following. HostInfo is immutable?

@paul-tavares i think i can type the response, but wouldn't it be HostResultList?

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

paul-tavares · 2020-04-22T13:52:20Z

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

+        };
+      }
+    }
+    // otherwise we are not on a host list or details page
    return {


You probably should reset the entire state as in:

return initialState();

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

paul-tavares · 2020-04-22T13:57:19Z

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

+      history.push(
+        urlFromQueryParams({
+          ...queryParams,
+          page_index: JSON.stringify(index),


(optional) You don't need stringify here and below

because the type is actually set up as string, typescript complains if i don't have stringify it here. maybe i should just turn that into a number and all will be solved?!

paul-tavares · 2020-04-22T14:13:21Z

I checked out the pull request and ran it locally. Here is some more feedback:

Closing the Details Flyout panel seems to trigger a new API request to populate the list
Should this message above the table Showing: 100 really be Total: 100? (cc/ @bfishel ) Since the table is really not showing 100.
How should we display errors on Lists? In this PR, the EUI Table error property is being used, which shows the error in the table (screen capture below (I forced the error in code, so disregard the actual message)), but in Policy List, we're using a Toaster notification. We should sync up what the expected behaviour should be - @bfishel can you help here? 😃 (ps. I like the approach that @parkiino took in screen capture below):

parkiino · 2020-04-22T14:30:32Z

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

@@ -56,7 +56,7 @@ describe('host list middleware', () => {
    await sleep();
    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' }],


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

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

kibana/x-pack/plugins/endpoint/server/routes/metadata/index.ts

Line 44 in 4ca8610

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 🤷‍♂️

parkiino · 2020-04-22T14:32:10Z

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

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


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

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

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

…ination

oatkiller · 2020-04-22T15:50:54Z

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

+              paging_properties: [{ page_index: pageIndex }, { page_size: pageSize }],
+            }),
+          });
+          response.request_page_index = Number(pageIndex);


What's the purpose of this?

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

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?

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

oatkiller · 2020-04-22T16:17:32Z

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

@@ -88,23 +88,40 @@ export type SubstateMiddlewareFactory = <Substate>(
  middleware: ImmutableMiddleware<Substate, AppAction>
 ) => Middleware<{}, GlobalState, Dispatch<AppAction | Immutable<AppAction>>>;

-export interface HostListState {
+export interface HostState {
+  /** list of host **/


oatkiller

few q's

aisantos

@parkiino - can you review these test cases and update the AC for host list pagination? I can document them in TestRail for us. I checked off the ones already in your PR. I'll execute the others manually until we can automate later:

host list url params uses page_index with an invalid value
host list url params has a page_index with a value that is more than total pagination
host list displays correct page when using url params directly
host list url params accepts only positive numbers for page_index and page_size
host list url params accepts only a value of 10, 20 or 50 for page_size (what are our defaults?)
host list url params ignores unknown search params
host list url params uses only first search param value if multiple values are present
host list url params uses defaults for any missing search params

aisantos · 2020-04-22T17:17:08Z

x-pack/plugins/endpoint/public/applications/endpoint/store/hosts/host_pagination.test.ts

+        Object {
+          "page_index": "0",
+          "page_size": "20",
+        }


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

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

aisantos · 2020-04-22T17:27:51Z

x-pack/plugins/endpoint/public/applications/endpoint/store/hosts/host_pagination.test.ts

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


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

parkiino · 2020-04-22T19:58:51Z

@aisantos

host list url params uses page_index with an invalid value
~~host list url params has a page_index with a value that is more than total pagination~~
host list displays correct page when using url params directly
host list url params accepts only positive numbers for page_index and page_size
host list url params accepts only a value of 10, 20 or 50 for page_size (10 is the default)
host list url params ignores unknown search params
host list url params uses only first search param value if multiple values are present
host list url params uses defaults for any missing search params

paul-tavares · 2020-04-22T20:32:08Z

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

@@ -56,7 +56,7 @@ describe('host list middleware', () => {
    await sleep();
    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' }],


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

kibana/x-pack/plugins/endpoint/server/routes/metadata/index.ts

Line 44 in 4ca8610

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 🤷‍♂️

paul-tavares · 2020-04-22T20:35:51Z

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

    }
    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) {


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

paul-tavares · 2020-04-22T20:41:31Z

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

+      if (listData(state).length === 0) {
+        const { page_index: pageIndex, page_size: pageSize } = uiQueryParams(state);
+        try {
+          const response = await coreStart.http.post('/api/endpoint/metadata', {


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

aisantos

Looks good @parkiino

kibanamachine · 2020-04-22T21:44:31Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 58cc53d

History

💚 Build #42511 succeeded 22eda80
💚 Build #42296 succeeded 42e547a
💚 Build #42252 succeeded 05f928f
💔 Build #42068 failed 0f69d0c
💚 Build #41691 succeeded 2fb8c40

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

kevinlog

Talked with @oatkiller , he's OK for us to move forward with this, we can address additional issues in another PR

Spoke with Rob, he's OK for us to move forward, we can make sure his questions are answered sufficiently in another PR

* hostlist pagination for endpoint security

…bana into ingest-node-pipeline/open-flyout-create-edit * 'feature/ingest-node-pipelines' of github.com:elastic/kibana: (116 commits) [Ingest Node Pipelines] More lenient treatment of on-failure value (elastic#64411) Report Deletion via UI- functional test (elastic#64031) Bump handlebars dependency from 4.5.3 to 4.7.6 (elastic#64402) [Uptime] Update TLS settings (elastic#64111) [alerting] removes usage of any throughout Alerting Services code (elastic#64161) [CANVAS] Moves notify to a canvas service (elastic#63268) [Canvas] Misc NP Stuff (elastic#63703) update apm index pattern (elastic#64232) Task/hostlist pagination (elastic#63722) [NP] Vega migration (elastic#63849) Move ensureDefaultIndexPattern into data plugin (elastic#63100) [Fleet] Fix agent status count to not include unenrolled agents (elastic#64106) Migrate graph_workspace saved object registration to Kibana platform (elastic#64157) Index pattern management UI -> TypeScript and New Platform Ready (edit_index_pattern) (elastic#64184) [ML] EuiDataGrid ml/transform components. (elastic#63447) [ML] Moving to kibana capabilities (elastic#64057) Move input_control_vis into NP (elastic#63333) remove reference to local application service in graph (elastic#64288) KQL removes leading zero and breaks query (elastic#62748) [FieldFormats] Cleanup: rename IFieldFormatType -> FieldFormatInstanceType (elastic#64193) ...

parkiino added 15 commits April 15, 2020 12:16

truncation/wrapping for hostname

ab8fe71

url pagination... still something wonky tho

9b0e3dc

page index and size checks, fixed error

fd95269

list api called when user directly navigates to details

2fb8c40

new action, list loading and error, hostlist --> host

8a5f7a7

fixed list error

f17ecfa

host list vs details loading logic

7e86f16

fixing list error types

ef20c60

reset errors when url changes

2ebf7b5

details loading stuff

3b2d2a3

uses pageview, fixes index test

ff737a7

Merge remote-tracking branch 'upstream/master' into task/hostlist-pag…

dbdec58

…ination

cleanup merge

0f69d0c

fixing up broken tests

58e637e

another test to fix

05f928f

parkiino marked this pull request as ready for review April 21, 2020 19:32

parkiino requested a review from a team as a code owner April 21, 2020 19:33

parkiino added 2 commits April 21, 2020 17:37

some pagination tests

913c428

merge master

42e547a

parkiino added Feature:Endpoint Elastic Endpoint feature release_note:skip Skip the PR/issue when compiling release notes Team:Endpoint Management v7.8.0 v8.0.0 labels Apr 21, 2020

paul-tavares reviewed Apr 22, 2020

View reviewed changes

x-pack/plugins/endpoint/public/applications/endpoint/store/hosts/middleware.ts Show resolved Hide resolved

paul-tavares reviewed Apr 22, 2020

View reviewed changes

parkiino commented Apr 22, 2020

View reviewed changes

parkiino added 2 commits April 22, 2020 11:45

add one more test using spy middleware

971352a

Merge remote-tracking branch 'upstream/master' into task/hostlist-pag…

22eda80

…ination

oatkiller reviewed Apr 22, 2020

View reviewed changes

oatkiller previously requested changes Apr 22, 2020

View reviewed changes

aisantos suggested changes Apr 22, 2020

View reviewed changes

parkiino added 2 commits April 22, 2020 15:50

add and fix tests

16a1113

removing incorrect wording showing

58cc53d

paul-tavares approved these changes Apr 22, 2020

View reviewed changes

aisantos approved these changes Apr 22, 2020

View reviewed changes

kevinlog approved these changes Apr 24, 2020

View reviewed changes

parkiino merged commit fb70f01 into elastic:master Apr 24, 2020

parkiino deleted the task/hostlist-pagination branch April 24, 2020 14:01

parkiino mentioned this pull request Apr 24, 2020

[7.x] Task/hostlist pagination (#63722) #64430

Merged

parkiino added a commit that referenced this pull request Apr 24, 2020

Task/hostlist pagination (#63722) (#64430)

a69fc3e

* hostlist pagination for endpoint security

		@@ -56,7 +56,7 @@ describe('host list middleware', () => {
		await sleep();

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

Task/hostlist pagination #63722

Task/hostlist pagination #63722

Conversation

parkiino commented Apr 16, 2020 • edited by aisantos Loading

Summary

Checklist

For maintainers

elasticmachine commented Apr 21, 2020

elasticmachine commented Apr 21, 2020

paul-tavares Apr 22, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-tavares commented Apr 22, 2020

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

oatkiller left a comment

Choose a reason for hiding this comment

aisantos left a 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

parkiino commented Apr 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aisantos left a comment

Choose a reason for hiding this comment

kibanamachine commented Apr 22, 2020

💚 Build Succeeded

History

kevinlog left a comment

Choose a reason for hiding this comment

parkiino commented Apr 16, 2020 •

edited by aisantos

Loading

paul-tavares Apr 22, 2020 •

edited

Loading

parkiino commented Apr 22, 2020 •

edited

Loading