-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[FLEET] New Integration Policy Details page for use in Integrations section #85355
Changes from 8 commits
83a9dab
f91631a
0b36a2c
f64c17b
2dc614a
73db874
d91dc79
9e6a08c
ddb97c4
50cd6ad
65d3835
e5f7a64
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,7 @@ | |
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
import React, { useState, useEffect, useCallback, useMemo } from 'react'; | ||
import React, { useState, useEffect, useCallback, useMemo, memo } from 'react'; | ||
import { useRouteMatch, useHistory } from 'react-router-dom'; | ||
import { i18n } from '@kbn/i18n'; | ||
import { FormattedMessage } from '@kbn/i18n/react'; | ||
|
@@ -45,15 +45,24 @@ import { useUIExtension } from '../../../hooks/use_ui_extension'; | |
import { ExtensionWrapper } from '../../../components/extension_wrapper'; | ||
import { GetOnePackagePolicyResponse } from '../../../../../../common/types/rest_spec'; | ||
import { PackagePolicyEditExtensionComponentProps } from '../../../types'; | ||
import { pkgKeyFromPackageInfo } from '../../../services/pkg_key_from_package_info'; | ||
|
||
export const EditPackagePolicyPage: React.FunctionComponent = () => { | ||
export const EditPackagePolicyPage = memo(() => { | ||
const { | ||
params: { packagePolicyId }, | ||
} = useRouteMatch<{ policyId: string; packagePolicyId: string }>(); | ||
|
||
return <EditPackagePolicyForm packagePolicyId={packagePolicyId} />; | ||
}); | ||
|
||
export const EditPackagePolicyForm = memo<{ | ||
packagePolicyId: string; | ||
from?: CreatePackagePolicyFrom; | ||
}>(({ packagePolicyId, from = 'edit' }) => { | ||
const { notifications } = useStartServices(); | ||
const { | ||
agents: { enabled: isFleetEnabled }, | ||
} = useConfig(); | ||
const { | ||
params: { policyId, packagePolicyId }, | ||
} = useRouteMatch<{ policyId: string; packagePolicyId: string }>(); | ||
const history = useHistory(); | ||
const { getHref, getPath } = useLink(); | ||
|
||
|
@@ -76,16 +85,19 @@ export const EditPackagePolicyPage: React.FunctionComponent = () => { | |
GetOnePackagePolicyResponse['item'] | ||
>(); | ||
|
||
const policyId = agentPolicy?.id ?? ''; | ||
|
||
// Retrieve agent policy, package, and package policy info | ||
useEffect(() => { | ||
const getData = async () => { | ||
setIsLoadingData(true); | ||
setLoadingError(undefined); | ||
try { | ||
const [{ data: agentPolicyData }, { data: packagePolicyData }] = await Promise.all([ | ||
sendGetOneAgentPolicy(policyId), | ||
sendGetOnePackagePolicy(packagePolicyId), | ||
]); | ||
const { data: packagePolicyData } = await sendGetOnePackagePolicy(packagePolicyId); | ||
const { data: agentPolicyData } = await sendGetOneAgentPolicy( | ||
packagePolicyData?.item.policy_id ?? 'id-missing-in-package-policy' | ||
); | ||
|
||
if (agentPolicyData?.item) { | ||
setAgentPolicy(agentPolicyData.item); | ||
} | ||
|
@@ -123,7 +135,7 @@ export const EditPackagePolicyPage: React.FunctionComponent = () => { | |
setPackagePolicy(newPackagePolicy); | ||
if (packagePolicyData.item.package) { | ||
const { data: packageData } = await sendGetPackageInfoByKey( | ||
`${packagePolicyData.item.package.name}-${packagePolicyData.item.package.version}` | ||
pkgKeyFromPackageInfo(packagePolicyData.item.package) | ||
); | ||
if (packageData?.response) { | ||
setPackageInfo(packageData.response); | ||
|
@@ -150,7 +162,7 @@ export const EditPackagePolicyPage: React.FunctionComponent = () => { | |
} | ||
}; | ||
|
||
if (isFleetEnabled) { | ||
if (isFleetEnabled && policyId) { | ||
getAgentCount(); | ||
} | ||
}, [policyId, isFleetEnabled]); | ||
|
@@ -214,8 +226,32 @@ export const EditPackagePolicyPage: React.FunctionComponent = () => { | |
[updatePackagePolicy] | ||
); | ||
|
||
// Cancel url | ||
const cancelUrl = getHref('policy_details', { policyId }); | ||
// Cancel url + Success redirect Path: | ||
// if `from === 'edit'` then it links back to Policy Details | ||
// if `from === 'package-edit'` then it links back to the Integration Policy List | ||
const cancelUrl = useMemo((): string => { | ||
if (packageInfo && policyId) { | ||
return from === 'package-edit' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we expect this to expand to more pages, perhaps a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, agreed a switch will make sense if we add more types for |
||
? getHref('integration_details', { | ||
pkgkey: pkgKeyFromPackageInfo(packageInfo!), | ||
panel: 'policies', | ||
}) | ||
: getHref('policy_details', { policyId }); | ||
} | ||
return '/'; | ||
}, [from, getHref, packageInfo, policyId]); | ||
|
||
const successRedirectPath = useMemo(() => { | ||
if (packageInfo && policyId) { | ||
return from === 'package-edit' | ||
? getPath('integration_details', { | ||
pkgkey: pkgKeyFromPackageInfo(packageInfo!), | ||
panel: 'policies', | ||
}) | ||
: getPath('policy_details', { policyId }); | ||
} | ||
return '/'; | ||
}, [from, getPath, packageInfo, policyId]); | ||
|
||
// Save package policy | ||
const [formState, setFormState] = useState<PackagePolicyFormState>('INVALID'); | ||
|
@@ -237,7 +273,7 @@ export const EditPackagePolicyPage: React.FunctionComponent = () => { | |
} | ||
const { error } = await savePackagePolicy(); | ||
if (!error) { | ||
history.push(getPath('policy_details', { policyId })); | ||
history.push(successRedirectPath); | ||
notifications.toasts.addSuccess({ | ||
title: i18n.translate('xpack.fleet.editPackagePolicy.updatedNotificationTitle', { | ||
defaultMessage: `Successfully updated '{packagePolicyName}'`, | ||
|
@@ -287,7 +323,7 @@ export const EditPackagePolicyPage: React.FunctionComponent = () => { | |
}; | ||
|
||
const layoutProps = { | ||
from: 'edit' as CreatePackagePolicyFrom, | ||
from, | ||
cancelUrl, | ||
agentPolicy, | ||
packageInfo, | ||
|
@@ -363,13 +399,21 @@ export const EditPackagePolicyPage: React.FunctionComponent = () => { | |
error={ | ||
loadingError || | ||
i18n.translate('xpack.fleet.editPackagePolicy.errorLoadingDataMessage', { | ||
defaultMessage: 'There was an error loading this intergration information', | ||
defaultMessage: 'There was an error loading this integration information', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👏 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All credit goes to WebStorm 🎉 |
||
}) | ||
} | ||
/> | ||
) : ( | ||
<> | ||
<Breadcrumb policyName={agentPolicy.name} policyId={policyId} /> | ||
{from === 'package' || from === 'package-edit' ? ( | ||
<IntegrationsBreadcrumb | ||
pkgkey={pkgKeyFromPackageInfo(packageInfo)} | ||
pkgTitle={packageInfo.title} | ||
policyName={packagePolicy.name} | ||
/> | ||
) : ( | ||
<PoliciesBreadcrumb policyName={agentPolicy.name} policyId={policyId} /> | ||
)} | ||
{formState === 'CONFIRM' && ( | ||
<ConfirmDeployAgentPolicyModal | ||
agentCount={agentCount} | ||
|
@@ -424,12 +468,21 @@ export const EditPackagePolicyPage: React.FunctionComponent = () => { | |
)} | ||
</CreatePackagePolicyPageLayout> | ||
); | ||
}; | ||
}); | ||
|
||
const Breadcrumb: React.FunctionComponent<{ policyName: string; policyId: string }> = ({ | ||
const PoliciesBreadcrumb: React.FunctionComponent<{ policyName: string; policyId: string }> = ({ | ||
policyName, | ||
policyId, | ||
}) => { | ||
useBreadcrumbs('edit_integration', { policyName, policyId }); | ||
return null; | ||
}; | ||
|
||
const IntegrationsBreadcrumb = memo<{ | ||
pkgTitle: string; | ||
policyName: string; | ||
pkgkey: string; | ||
}>(({ pkgTitle, policyName, pkgkey }) => { | ||
useBreadcrumbs('integration_policy_edit', { policyName, pkgTitle, pkgkey }); | ||
return null; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems odd. Not a big fan of putting.. log-like traces into values.
you also don't technically know that that could not be a valid future policy ID.
You should check the existence of that property and value before calling sendGetOneAgentPolicy.
You're in a
try
already, could check it, and throw if it's missing (if that's the appropriate behavior).Or if it's something you move past, by grabbing a default, perhaps something like
Or if having the value itself is optional
something along those lines seems more intentional than sending intentionally fake data down the line to return.. who knows what. An error? A null? Accidentally a real object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @pzl here we can probably validate that packagePolicyData is not null and the request succeed before sending this request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also you probably want to check the
error
field of the response too, if a request fail we probably want to throw that errorThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I meant to comment on this and why I did it.
throw
ing here would not be good because the entire UI will likely crash. instead, I wanted to piggy back off the error handling in place for failed API calls. If this did happen to error, I wanted to have something that can tell us (or support team) some info about where to go look.That being said, you just pointed out that all of this is wrapped in a
try {}
block (why did I not realize that? 🤦♂️ ), so I as you suggest, doing a check and then explicitlythrow new Error()
will be a better approach.Thanks for the feedback. making the change now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @nchaulet . Good point on processing each individual API call checking for errors - in the prior code both of those would be handled (use of
Promise.all()
) but with my change they are not. Will fix