[Endpoint] Policy list support for URL pagination state #63291

paul-tavares · 2020-04-10T21:01:41Z

Summary

Drive pagination for the Policy List (page index + page size) via URL search params (page_index=1&page_size=10). This enables a user to click the browser back/forward buttons and be shown the correct set of data on the Policy list view.

This PR also includes a Redux "spy" middleware test utility to avoid having to use setTimeout based sleeps in Unit Test cases. With this in place:

fixes Failing test: X-Pack Jest Tests.x-pack/plugins/endpoint/public/applications/endpoint/store/policy_list - policy list store concerns it sets isLoading when userPaginatedPolicyListTable #58896
fixes Failing test: X-Pack Jest Tests.x-pack/plugins/endpoint/public/applications/endpoint/store/policy_list - policy list store concerns it sets isLoading when userNavigatedToPage #58972

🤞

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

elasticmachine · 2020-04-10T21:01:44Z

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

elasticmachine · 2020-04-10T21:01:45Z

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

paul-tavares · 2020-04-10T21:05:33Z

x-pack/plugins/endpoint/public/applications/endpoint/store/policy_list/test_mock_utils.ts

+ *   // do assertion
+ * });
+ */
+export const createSpyMiddleware = <S = GlobalState>(): MiddlewareActionSpyHelper<S> => {


@parkiino , @kevinlog , @oatkiller , @peluja1012 , @dplumlee , @bkimmel , @kqualters-elastic - FYI. If you find that you have the same need for this type of utility while creating test cases for middleware or views that trigger middleware, we can elevate this utility to be in a higher level folder and re-use it. Its currently built in a generic way, so it should adopt to other middleware testing cases.

@oatkiller - I'm going to try to jest.fn() on the middleware idea next. Did not include it here because I did not have a need currently for it. 😃

FYI - I made the middleware dispatch function a jest mocked function and exposed its mock object via the dispatchSpy property in the returned object.

this is greatttt

kevinlog · 2020-04-13T11:52:44Z

x-pack/plugins/endpoint/public/applications/endpoint/store/policy_list/test_mock_utils.ts

+        const timeout = setTimeout(() => {
+          watchers.delete(watch);
+          reject(err);
+        }, 4500);


would it ever make sense to make this timeout configurable? Not necessary now, but wondering if we would ever adjust jest's default.

Yeah, for sure the createSpyMiddleware factory can be improved in the future to take in the timeout. I actually looked to see if there was a way to access Jest's timeout value at runtime so that I could use it, but was not able to find it (there is one to set it, but not retrieve it).

… multiple times

…st-url-pagination-redo

…st-url-pagination-redo # Conflicts: # x-pack/plugins/endpoint/public/applications/endpoint/store/policy_list/middleware.ts # x-pack/plugins/endpoint/public/applications/endpoint/store/routing/action.ts # x-pack/plugins/endpoint/public/applications/endpoint/view/use_page_id.ts

paul-tavares · 2020-04-13T17:47:18Z

jenkins test this

oatkiller · 2020-04-13T17:57:48Z

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

+  return state.location?.pathname === '/policy';
+};
+
+export const routeLocation = (state: PolicyListState) => state.location;


you could drop export if you'd like

True. I will remove it in next commit

oatkiller · 2020-04-13T18:04:51Z

x-pack/plugins/endpoint/public/applications/endpoint/store/policy_list/test_mock_utils.ts

+   *
+   * @param actionType
+   */
+  waitForAction: (actionType: Pick<AppAction, 'type'>['type']) => Promise<void>;


you should be able to write AppAction['type'] here and get the same behavior

thanks. Will simplify it. (not sure why I decided to go the long/hard way on this one).

oatkiller · 2020-04-13T18:06:22Z

x-pack/plugins/endpoint/public/applications/endpoint/store/policy_list/test_mock_utils.ts

+   * Also - do not hold on to references to this property value if `jest.clearAllMocks()` or
+   * `jest.resetAllMocks()` is called between usages of the value.
+   */
+  dispatchSpy: jest.Mock<Dispatch<AppAction>>['mock'];


if this isn't always set, maybe make the property dispatchSpy? instead? that way we'll check for validity before using it (and hopefully avoid an error.)

yeah, thats another alternative. But would that be a worst developer experience? It would imply that we would need to either check the property every time we try to use it (utils!.dispatchSpy) or in destructing it, like dispatchSpy = utils!.dispatchUtils which could lead to an error if done prior to store creation.

oatkiller · 2020-04-13T18:10:10Z

x-pack/plugins/endpoint/public/applications/endpoint/store/policy_list/test_mock_utils.ts

+
+    actionSpyMiddleware: api => {
+      return next => {
+        spyDispatch = jest.fn(action => {


could you create jest.fn where it is defined and here do

spyDispatch.mockImplementation(action => { // ... })

would that allow you to remove the comment about an error occurring if you access the mock calls out of sequence?

That was my initial thought (in order to avoid the check in the dispatchSpy property getter), but dispatch needs access to next() thus it needs to be defined within this context.

oatkiller · 2020-04-13T18:16:08Z

x-pack/plugins/endpoint/public/applications/endpoint/store/policy_list/test_mock_utils.ts

+        spyDispatch = jest.fn(action => {
+          next(action);
+          // If we have action watchers, then loop through them and call them with this action
+          if (watchers.size > 0) {


would this work the same if you didn't have if (watchers.size > 0)

ahh - good point. I'll remove the condition and have only the for loop

oatkiller · 2020-04-13T18:16:32Z

x-pack/plugins/endpoint/public/applications/endpoint/store/policy_list/test_mock_utils.ts

+              watch(action);
+            }
+          }
+          return action;


are you using this?

I'm not using it, but needed to satisfy the type defined by jest.Mock<>. Without it, I got type errors

…st-url-pagination-redo

kibanamachine · 2020-04-13T20:24:37Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: fb2a5bc

History

💚 Build #40462 succeeded 0cb54f6
💔 Build #40427 failed 0cb54f6
💚 Build #40407 succeeded 4e12f0b
💚 Build #40245 succeeded 0148aba

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

) * store changes to support pagination via url * Fix storing location when pagination happens * Initial set of tests * Redux spy middleware and async utility * Add better types to `waitForAction` * Add more docs * fix urlSearchParams selector to account for array of values * full set of tests for policy list store concerns * More efficient redux spy middleware (no more sleep()) * Set spy middleware `dispatch` to a `jest.fn` and expose `mock` info. * Fix url param selector to return first param value when it is defined multiple times * Removed PageId and associated hook * clean up TODO items * Fixes post-merge frm `master` * Address code review comments

* master: (132 commits) document code splitting for client code (elastic#62593) Escape single quotes surrounded by double quotes (elastic#63229) [Endpoint] Update cli mapping to match endpoint package (elastic#63372) update in-app links to metricbeat configuration docs (elastic#63295) investigation notes field (documentation / metadata) (elastic#63386) [Maps] fix bug where toggling Scaling type does not re-fetch data (elastic#63326) [Alerting] set correct parameter for unauthented email action (elastic#63086) [Telemetry] force staging urls in tests (elastic#63356) Migrate legacy maps service to NP & update refs (elastic#60942) Fix task manager query to return tasks to retry (elastic#63360) [Endpoint] Policy list support for URL pagination state (elastic#63291) [Canvas] Migrate saved object mappings and migrations to Kibana Platform (elastic#58891) [DOCS] Add ILM tutorial (elastic#59502) [Maps] Add SOURCE_TYPES enumeration (elastic#62975) [Maps] update geospatial filters to use geo_shape query for geo_point fields (elastic#62966) Move away from npStart for embeddables in canvas (elastic#62680) Use MapInput type from Maps plugin (elastic#61539) Update to pagination for workpad and templates (elastic#62050) [SIEM] Fix AlertsTable id (elastic#63368) Consistent terminology around cypress test data (elastic#63279) ...

* master: document code splitting for client code (elastic#62593) Escape single quotes surrounded by double quotes (elastic#63229) [Endpoint] Update cli mapping to match endpoint package (elastic#63372) update in-app links to metricbeat configuration docs (elastic#63295) investigation notes field (documentation / metadata) (elastic#63386) [Maps] fix bug where toggling Scaling type does not re-fetch data (elastic#63326) [Alerting] set correct parameter for unauthented email action (elastic#63086) [Telemetry] force staging urls in tests (elastic#63356) Migrate legacy maps service to NP & update refs (elastic#60942) Fix task manager query to return tasks to retry (elastic#63360) [Endpoint] Policy list support for URL pagination state (elastic#63291) [Canvas] Migrate saved object mappings and migrations to Kibana Platform (elastic#58891) [DOCS] Add ILM tutorial (elastic#59502) [Maps] Add SOURCE_TYPES enumeration (elastic#62975) [Maps] update geospatial filters to use geo_shape query for geo_point fields (elastic#62966) Move away from npStart for embeddables in canvas (elastic#62680)

* store changes to support pagination via url * Fix storing location when pagination happens * Initial set of tests * Redux spy middleware and async utility * Add better types to `waitForAction` * Add more docs * fix urlSearchParams selector to account for array of values * full set of tests for policy list store concerns * More efficient redux spy middleware (no more sleep()) * Set spy middleware `dispatch` to a `jest.fn` and expose `mock` info. * Fix url param selector to return first param value when it is defined multiple times * Removed PageId and associated hook * clean up TODO items * Fixes post-merge frm `master` * Address code review comments

aisantos · 2020-06-12T03:45:40Z

Regressed test cases for url pagination state for Policy List:

Policy List displays displays default page_index and page_size when no url params T5737
Policy List display correct page when using url params directly T5739
Policy List url params accepts only positive numbers for page_index and page_size T5740
Policy List url params accepts only a value of 10, 20 or 50 for page_size T5741
Policy List url params ignores unknown search params T5472
Policy List url params uses only last param value if multiple values are present T5473
Policy List url params uses defaults for any missing search params T5744
Policy List url params uses an invalid page_index value T5916
Policy List url params has a page_index with a value that is more than total pagination T5917

paul-tavares added 9 commits April 10, 2020 10:03

store changes to support pagination via url

c32339f

Fix storing location when pagination happens

87bb350

Initial set of tests

30eacb8

Redux spy middleware and async utility

aff13f6

Add better types to waitForAction

7a3f4c3

Add more docs

95f3f76

fix urlSearchParams selector to account for array of values

7b78efd

full set of tests for policy list store concerns

b596537

More efficient redux spy middleware (no more sleep())

0148aba

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

paul-tavares requested a review from a team as a code owner April 10, 2020 21:01

paul-tavares self-assigned this Apr 10, 2020

paul-tavares commented Apr 10, 2020

View reviewed changes

kevinlog reviewed Apr 13, 2020

View reviewed changes

kevinlog approved these changes Apr 13, 2020

View reviewed changes

paul-tavares added 5 commits April 13, 2020 09:33

Set spy middleware dispatch to a jest.fn and expose mock info.

6bc16ab

Fix url param selector to return first param value when it is defined…

c643dcb

… multiple times

Removed PageId and associated hook

caf4ed3

clean up TODO items

f312771

Merge remote-tracking branch 'upstream/master' into EMT-182-policy-li…

4e12f0b

…st-url-pagination-redo

parkiino approved these changes Apr 13, 2020

View reviewed changes

paul-tavares added 2 commits April 13, 2020 11:38

Fixes post-merge frm master

0cb54f6

oatkiller reviewed Apr 13, 2020

View reviewed changes

oatkiller approved these changes Apr 13, 2020

View reviewed changes

oatkiller reviewed Apr 13, 2020

View reviewed changes

paul-tavares added 2 commits April 13, 2020 14:44

Address code review comments

1edfffc

Merge remote-tracking branch 'upstream/master' into EMT-182-policy-li…

fb2a5bc

…st-url-pagination-redo

paul-tavares merged commit 48fd5c0 into elastic:master Apr 13, 2020

paul-tavares deleted the EMT-182-policy-list-url-pagination-redo branch April 13, 2020 20:26

paul-tavares mentioned this pull request Apr 13, 2020

[7.x] [Endpoint] Policy list support for URL pagination state (#63291) #63402

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Endpoint] Policy list support for URL pagination state #63291

[Endpoint] Policy list support for URL pagination state #63291

paul-tavares commented Apr 10, 2020

elasticmachine commented Apr 10, 2020

elasticmachine commented Apr 10, 2020

paul-tavares Apr 10, 2020

paul-tavares Apr 13, 2020

parkiino Apr 13, 2020

kevinlog Apr 13, 2020

paul-tavares Apr 13, 2020

paul-tavares commented Apr 13, 2020

oatkiller Apr 13, 2020

paul-tavares Apr 13, 2020

oatkiller Apr 13, 2020

paul-tavares Apr 13, 2020

oatkiller Apr 13, 2020

paul-tavares Apr 13, 2020

oatkiller Apr 13, 2020

paul-tavares Apr 13, 2020

oatkiller Apr 13, 2020

paul-tavares Apr 13, 2020

oatkiller Apr 13, 2020

paul-tavares Apr 13, 2020

kibanamachine commented Apr 13, 2020

aisantos commented Jun 12, 2020

[Endpoint] Policy list support for URL pagination state #63291

[Endpoint] Policy list support for URL pagination state #63291

Conversation

paul-tavares commented Apr 10, 2020

Summary

Checklist

elasticmachine commented Apr 10, 2020

elasticmachine commented Apr 10, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Apr 13, 2020

💚 Build Succeeded

History

aisantos commented Jun 12, 2020