[Endpoint] Add Endpoint empty states for onboarding #69626

kevinlog · 2020-06-19T21:40:49Z

Summary

This PR adds an onboarding flow for the empty Host list.

On Endpoint:

Adds an empty state which gets users to add a policy
Following a policy, it will prompt users to deploy an Agent with Endpoint Security enabled if not already

On Ingest:

Adds a query param openEnrollmentFlyout to the Agent config details page to open the deployment flyout to allow us to deep link to it
Enables us to share state to be able to route back to Security after the user closes the deployment flyout using the same method as @paul-tavares previous PR: [Ingest Manager][Endpoint] Add Endpoint Create Policy flow with Ingest #68955

Testing:

Mostly manual with some unit tests added to verify the states load when appropriate
Our functional tests for this page are disabled right now, when they are re-enabled, I'll add functional tests in a later PR

No Policy prompt

Add Policy in Ingest

No Endpoint prompt

Enroll Agent in Ingest

Endpoint

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

…st-empty-states

kevinlog · 2020-06-24T15:38:22Z

x-pack/plugins/ingest_manager/public/applications/ingest_manager/hooks/use_intra_app_state.tsx

- if (location.pathname === intraAppState.forRoute && !wasHandled.has(intraAppState)) {
+
+ if (
+ location.pathname + location.search === intraAppState.forRoute &&


Looking for feedback on this, I'm thinking when we need search params, we pass them in separately and then keep this comparison to be by pathname only

I agree that this should not trigger based off of .search string - only pathname. The page component is the one that should handle retrieving any URL .search params and process on them.

…st-empty-states

kevinlog · 2020-06-24T19:47:29Z

x-pack/plugins/security_solution/public/management/pages/endpoint_hosts/view/index.tsx

@@ -252,6 +325,54 @@ export const HostList = () => {
 ];
 }, [formatUrl, queryParams, search]);

+ const renderTableOrEmptyState = useMemo(() => {
+ if (listData && listData.length > 0) {


TODO, the flow works, but some oddities in loading cause "pop ins", gonna introduce some additional loading states

paul-tavares

Some feedback :)

paul-tavares · 2020-06-24T20:13:56Z

x-pack/plugins/ingest_manager/public/applications/ingest_manager/hooks/use_intra_app_state.tsx

- if (location.pathname === intraAppState.forRoute && !wasHandled.has(intraAppState)) {
+
+ if (
+ location.pathname + location.search === intraAppState.forRoute &&


I agree that this should not trigger based off of .search string - only pathname. The page component is the one that should handle retrieving any URL .search params and process on them.

paul-tavares · 2020-06-24T20:23:25Z

x-pack/plugins/ingest_manager/public/applications/ingest_manager/types/intra_app_route_state.ts

+ */
+export interface AgentConfigDetailsDeployAgentAction {
+ /** On cancel, navigate to the given app */
+ onCancelNavigateTo?: Parameters<ApplicationStart['navigateToApp']>;


There are a few buttons/action on the flyout, but you only have onCancel defined here. Is this right? they probably would (in our case) do the same thing, but what about the "proceed" button?

If there is really no need for more than one param, then I would rename it to something else - maybe onDoneNavigateTo??

yea, right now, everything is just controlled by "Cancel", including the "Continue" option. That name makes sense, will change.

paul-tavares · 2020-06-24T20:28:14Z

x-pack/plugins/security_solution/public/management/pages/endpoint_hosts/view/index.tsx

+ const handleCreatePolicyClick = useNavigateToAppEventHandler<CreateDatasourceRouteState>(
+ 'ingestManager',
+ {
+ path: `#/integrations${packageInfo ? `/endpoint-${packageInfo.version}/add-datasource` : ''}`,


Just FYI - I'm changing some of this in my PR that is up, so you may end up with some conflicts when the time comes

I saw your PR, will take care of it 👍

elasticmachine · 2020-06-25T13:00:16Z

Pinging @elastic/ingest-management (Team:Ingest Management)

elasticmachine · 2020-06-25T13:01:08Z

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

kevinlog · 2020-06-25T13:01:29Z

@elasticmachine merge upstream

paul-tavares · 2020-06-25T13:11:27Z

x-pack/plugins/ingest_manager/public/applications/ingest_manager/types/intra_app_route_state.ts

+ * Supported routing state for the agent config details page routes with deploy agents action
+ */
+export interface AgentConfigDetailsDeployAgentAction {
+ /** On cancel, navigate to the given app */


Update comment (remove "cancel")

paul-tavares · 2020-06-25T13:12:57Z

x-pack/plugins/security_solution/public/management/components/management_empty_state.tsx

+ () => [
+ {
+ title: i18n.translate('xpack.securitySolution.endpoint.policyList.stepOneTitle', {
+ defaultMessage: 'Head over to Ingest Manager.',


I know we're still nailing down the labels, but remove period at the end? ( 😬 ) (some for labels below)

I'll update all of the labels after we finalize the content for the empty states

paul-tavares · 2020-06-25T13:16:08Z

x-pack/plugins/security_solution/public/management/components/management_empty_state.tsx

+ return (
+ <div data-test-subj={dataTestSubj}>
+ {loading ? (
+ <EuiProgress size="xs" color="accent" className="essentialAnimation" />


I take back my prior comment about using the Progress bar - I thought you had content inside of it, so that it would just show a small animation a top of the content that scrolled across the width of the area (similar to the animation on tables while loading)

For this use case, where all the user sees is a "busy animation", I would suggest going with the Loading component

paul-tavares · 2020-06-25T13:19:03Z

x-pack/plugins/security_solution/public/management/pages/endpoint_hosts/store/middleware.ts

+
+ // No hosts, so we should check to see if there are policies for onboarding
+ if (hostResponse && hostResponse.hosts.length === 0) {
+ const http = coreStart.http;


maybe this should be defined at top - below line 17 - and used trough out this module

paul-tavares · 2020-06-25T13:31:28Z

x-pack/plugins/security_solution/public/management/pages/endpoint_hosts/store/reducer.ts

@@ -132,6 +165,8 @@ export const hostListReducer: ImmutableReducer<HostState, AppAction> = (
 error: undefined,
 detailsError: undefined,
 policyResponseError: undefined,
+ loading: true,


I'm actually confused by this return here (unrelated to your change), which may be causing the state to be updated on every URL change.

I think the return here should be to reset the state for hosts if the user is leaving the Endpoints page (list and details) and if not leaving, then do nothing (just return state).

so maybe something like:

if ( (wasPreviouslyOnListPage || wasPreviouslyOnDetailsPage) && !isCurrentlyOnListPage && !iscurrentlyOnDetailsPage ) { return initialHostListState(); } return state;

I'm actually removing this as it's what's causing the test failures

paul-tavares · 2020-06-25T13:52:04Z

x-pack/plugins/ingest_manager/public/applications/ingest_manager/hooks/use_intra_app_state.tsx

- if (location.pathname === intraAppState.forRoute && !wasHandled.has(intraAppState)) {
+
+ // Compare the routes only, strip any query params
+ const forBaseRoute = intraAppState.forRoute.split('?')[0];


maybe move the parsing of the hash to line 31 since it would mean it only gets parsed once

jfsiii · 2020-06-25T15:07:27Z

x-pack/plugins/ingest_manager/public/applications/ingest_manager/hooks/use_intra_app_state.tsx

@@ -28,7 +28,7 @@ export const IntraAppStateProvider = memo<{
 }>(({ kibanaScopedHistory, children }) => {
 const internalAppToAppState = useMemo<IntraAppState>(() => {
 return {
- forRoute: kibanaScopedHistory.location.hash.substr(1),
+ forRoute: kibanaScopedHistory.location.hash.substr(1).split('?')[0],


It's not clear to me what's happening here, and I'm a bit uneasy with the chain of string operations on the url.

After playing around a bit I think I see the intent and, IIUC, I'd prefer something more explicit/safe like URL which is available in the browser and node

const route = kibanaScopedHistory.location.hash.substr(1); const url = new URL(route, kibanaScopedHistory.location); ... forRoute: url.pathname

@jfsiii @paul-tavares I solved this by just passing in the url base path as well from the state so that we can control it from there. It saves us from having to parse potential query params and is more exact. let me know your thoughts

jfsiii · 2020-06-25T15:12:28Z

...gest_manager/public/applications/ingest_manager/sections/agent_config/details_page/index.tsx

+ const openEnrollmentFlyoutOpenByDefault =
+ queryString.parse(useLocation().search).openEnrollmentFlyout === 'true';


Can we use URLSearchParams here, instead?

Suggested change

const openEnrollmentFlyoutOpenByDefault =

queryString.parse(useLocation().search).openEnrollmentFlyout === 'true';

const queryParams = new URLSearchParams(useLocation().search);

const openEnrollmentFlyoutOpenByDefault = queryParams.get('openEnrollmentFlyout') === 'true';

FYI - If I remember correctly, the last time I tried to use URLSearchParams I had issues in the Jest UT environment because I think jsdom did support it. I don't believe we have UT for these UIR components, so it maybe find to use it for now

I just pulled in the URLSearchParams as opposed to query-string and yes this doesn't run in Jest UT right now afaik. I plan to use functional tests when our host tests are re-enabled, so we'll see what happens.

jfsiii

Did an initial scan and left some notes about how we deal with URLs. I can come back later and do a deeper review.

…st-empty-states

kevinlog · 2020-06-25T21:29:49Z

@jfsiii addressed your initial comments, let me know what else you find. Thanks for helping out!

paul-tavares · 2020-06-26T12:36:10Z

x-pack/plugins/ingest_manager/public/applications/ingest_manager/hooks/use_intra_app_state.tsx

 return {
- forRoute: kibanaScopedHistory.location.hash.substr(1),
- routeState: kibanaScopedHistory.location.state as AnyIntraAppRouteState,
+ forRoute: intraAppState && intraAppState.baseRoute ? intraAppState.baseRoute : '',


I'll defer to @jfsiii here, but personally I think we should parse the hash and remove the search params from it (if any), for a few reason:

This "bridge" solution here is temporary and should be looked at as a work-around to the fact that Hash router is still being used. So when this is removed (when Ingest moves to using BrowserRouter and the ScoppedHistory provided by Kibana's mount params), everything should just work normally (™️ famous words 😬 )

It feels wrong to have the Route interface state params define what basePath the state applies to, being that we're already redirecting to a specific location. I feel that the Route state params should be meant for the View that it displays, not for the intermediate process that bridges the two routing approaches

Adding this param in the state will further couple the provider with Ingest in having to know/define the baseRoute for the Route

@jfsiii @paul-tavares my original apprehension to using new URL() is that I didn't see a way to get the protocol required to create the object without accessing window.location or just faking one. I wasn't seeing it in the kibanaScopedHistory object anywhere.

After thinking about it, my understanding is that our goal here is to just use a library for URL parsing to be more safe, which makes complete sense. So, I propose we just fake the protocol so that we're able to properly create the URL object and strip out the pathname

I'm doing this locally which is working right now. As @paul-tavares said, we could back this out when we're no longer using a hashRouter.

forRoute: (new URL(`https://localhost${kibanaScopedHistory.location.hash.substr(1)}`)).pathname

@kevinlog The URL constructor has the signature

const url = new URL(url [, base])

If we have a relative URL and are only after the pathname I think we can follow the example from

kibana/x-pack/plugins/ingest_manager/public/applications/ingest_manager/sections/epm/hooks/use_links.tsx

Lines 10 to 11 in 7c22204

const removeRelativePath = (relativePath: string): string =>

new URL(relativePath, 'http://example.com').pathname;

e.g.

const route = kibanaScopedHistory.location.hash.substr(1); // url will have protocol, port, etc from second arg, but we're ignoring them const url = new URL(route, 'http://any.tld.works');

@jfsiii sounds good, I built the URL as above - it's all working as expected

jfsiii · 2020-06-26T18:01:49Z

x-pack/plugins/ingest_manager/public/applications/ingest_manager/hooks/use_intra_app_state.tsx

 routeState: kibanaScopedHistory.location.state as AnyIntraAppRouteState,
 };
- }, [kibanaScopedHistory.location.hash, kibanaScopedHistory.location.state]);
+ }, [kibanaScopedHistory.location.state, kibanaScopedHistory.location.hash]);


spiderman_meme.jpg

jfsiii

Approving in the spirit of iterative improvement and since my concerns were addressed but I only did a "linter"-type review. I didn't check it out and run it locally or think much about the way the parts interact.

…st-empty-states

kevinlog · 2020-06-26T20:18:27Z

@elasticmachine merge upstream

kibanamachine · 2020-06-26T22:06:19Z

💚 Build Succeeded

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

Build metrics

✅ unchanged

History

💔 Build #56826 failed 9689d04
💚 Build #56794 succeeded 18dd954
💚 Build #56547 succeeded 8399f0e
💔 Build #56504 failed f364e74
💔 Build #56326 failed 310b8ef

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

* master: (59 commits) [Lens] Fix broken test (elastic#70117) [SIEM] Import timeline fix (elastic#65448) [SECURITY SOLUTION][INGEST] UX update for ingest manager edit/create datasource for endpoint (elastic#70079) [Telemetry] Collector Schema (elastic#64942) [Endpoint] Add Endpoint empty states for onboarding (elastic#69626) Hide unused resolver buttons (elastic#70112) [Security] `Investigate in Resolver` Timeline Integration (elastic#70111) [Discover] Improve styling of graphs in sidebar (elastic#69440) [Metrics UI] Fix EuiTheme type issue (elastic#69735) skip failing suite (elastic#70104) (elastic#70103) [ENDPOINT] Hide the Timeline Flyout while on the Management Pages (elastic#69998) [SIEM][CASE] Persist callout when dismissed (elastic#68372) [SIEM][Exceptions] - Cleaned up and updated exception list item comment structure (elastic#69532) [Maps] remove indexing state from redux (elastic#69765) Add API integration test for deleting data streams. (elastic#70020) renames SIEM to Security Solution (elastic#70070) Adding saved_objects_page in OSS (elastic#69900) [Lens] Use accordion menus in field list for available and empty fields (elastic#68871) Dynamic uiActions & license support (elastic#68507) [SIEM] Update readme for timeline apis (elastic#67038) ...

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…bana into alerting/consumer-based-rbac * 'alerting/consumer-based-rbac' of github.com:gmmorris/kibana: (25 commits) [Lens] Fix broken test (elastic#70117) [SIEM] Import timeline fix (elastic#65448) [SECURITY SOLUTION][INGEST] UX update for ingest manager edit/create datasource for endpoint (elastic#70079) [Telemetry] Collector Schema (elastic#64942) [Endpoint] Add Endpoint empty states for onboarding (elastic#69626) Hide unused resolver buttons (elastic#70112) [Security] `Investigate in Resolver` Timeline Integration (elastic#70111) [Discover] Improve styling of graphs in sidebar (elastic#69440) [Metrics UI] Fix EuiTheme type issue (elastic#69735) skip failing suite (elastic#70104) (elastic#70103) [ENDPOINT] Hide the Timeline Flyout while on the Management Pages (elastic#69998) [SIEM][CASE] Persist callout when dismissed (elastic#68372) [SIEM][Exceptions] - Cleaned up and updated exception list item comment structure (elastic#69532) [Maps] remove indexing state from redux (elastic#69765) Add API integration test for deleting data streams. (elastic#70020) renames SIEM to Security Solution (elastic#70070) Adding saved_objects_page in OSS (elastic#69900) [Lens] Use accordion menus in field list for available and empty fields (elastic#68871) Dynamic uiActions & license support (elastic#68507) [SIEM] Update readme for timeline apis (elastic#67038) ...

kevinlog added 12 commits June 19, 2020 17:39

add policy empty state to Host list, placeholder for add endpoint prompt

f9982f1

add empty endpoint state

a319fc1

fix types

d203642

fix test, hide total when empty state

08afb5a

Merge branch 'master' of github.com:kevinlog/kibana into task/host-li…

899f0cf

…st-empty-states

fix types

4316454

fix test

08bbc88

add policy selection to onboarding

6260e5e

begin unit tests

1c55b23

additional tests

f5e3999

add URL and roundtrip

eb4b259

fix bug, add type

b9129eb

kevinlog commented Jun 24, 2020

View reviewed changes

kevinlog added 3 commits June 24, 2020 11:56

refactor middleware

a93a1fa

fix selection bug

3206882

Merge branch 'master' of github.com:kevinlog/kibana into task/host-li…

439e8e9

…st-empty-states

kevinlog changed the title ~~add policy empty state to Host list, placeholder for add endpoint prompt~~ [Endpoint] Add Endpoint empty states for onboarding Jun 24, 2020

kevinlog commented Jun 24, 2020

View reviewed changes

paul-tavares reviewed Jun 24, 2020

View reviewed changes

kevinlog added 2 commits June 25, 2020 08:19

refactor route path check

e683508

make loading smoother, fix types

7f656f8

kevinlog marked this pull request as ready for review June 25, 2020 13:00

kevinlog requested a review from a team as a code owner June 25, 2020 13:00

kevinlog requested a review from a team June 25, 2020 13:00

kevinlog requested a review from a team as a code owner June 25, 2020 13:00

botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Jun 25, 2020

kevinlog added release_note:skip Skip the PR/issue when compiling release notes v7.9.0 v8.0.0 labels Jun 25, 2020

kevinlog added the Team:Endpoint Management label Jun 25, 2020

Merge branch 'master' into task/host-list-empty-states

71690bf

paul-tavares approved these changes Jun 25, 2020

View reviewed changes

address comments, fix tests

310b8ef

jfsiii reviewed Jun 25, 2020

View reviewed changes

kevinlog added 2 commits June 25, 2020 13:29

Merge branch 'master' of github.com:kevinlog/kibana into task/host-li…

3529c1c

…st-empty-states

address comments

f364e74

fix test

8399f0e

paul-tavares reviewed Jun 26, 2020

View reviewed changes

ph requested a review from jfsiii June 26, 2020 13:39

use URL, take out baseRoute

18dd954

jfsiii reviewed Jun 26, 2020

View reviewed changes

jfsiii approved these changes Jun 26, 2020

View reviewed changes

Merge branch 'master' of github.com:kevinlog/kibana into task/host-li…

9689d04

…st-empty-states

Merge branch 'master' into task/host-list-empty-states

d425f46

ph assigned kevinlog Jun 26, 2020

kevinlog merged commit 5236335 into elastic:master Jun 26, 2020

kevinlog deleted the task/host-list-empty-states branch June 26, 2020 22:08

kevinlog mentioned this pull request Jun 26, 2020

[7.x] [Endpoint] Add Endpoint empty states for onboarding (#69626) #70133

Merged

kevinlog added a commit that referenced this pull request Jun 29, 2020

[Endpoint] Add Endpoint empty states for onboarding (#69626) (#70133)

2c48a90

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

kevinlog mentioned this pull request Jun 29, 2020

[Ingest Manager][SECURITY SOLUTION] adjust config reassign link and add roundtrip to Reassignment flow #70208

Merged

7 tasks

		const openEnrollmentFlyoutOpenByDefault =
		queryString.parse(useLocation().search).openEnrollmentFlyout === 'true';

	const removeRelativePath = (relativePath: string): string =>
	new URL(relativePath, 'http://example.com').pathname;

[Endpoint] Add Endpoint empty states for onboarding #69626

[Endpoint] Add Endpoint empty states for onboarding #69626

Conversation

kevinlog commented Jun 19, 2020 • edited Loading

Summary

Checklist

For maintainers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jun 25, 2020

elasticmachine commented Jun 25, 2020

kevinlog commented Jun 25, 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

Choose a reason for hiding this comment

jfsiii left a comment

Choose a reason for hiding this comment

kevinlog commented Jun 25, 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

jfsiii left a comment

Choose a reason for hiding this comment

kevinlog commented Jun 26, 2020

kibanamachine commented Jun 26, 2020

💚 Build Succeeded

Build metrics

History

kevinlog commented Jun 19, 2020 •

edited

Loading