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

[Fleet] disable upgrade action when package does not have upgrade available #111510

Merged
Merged
Show file tree
Hide file tree
Changes from 5 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 @@ -45,16 +45,25 @@ import {
PackagePolicyActionsMenu,
} from '../../../../../components';

import type { PackagePolicyAndAgentPolicy } from './use_package_policies_with_agent_policy';
import { usePackagePoliciesWithAgentPolicy } from './use_package_policies_with_agent_policy';
import { Persona } from './persona';

interface PackagePoliciesPanelProps {
name: string;
version: string;
}

interface InMemoryPackagePolicyAndAgentPolicy {
packagePolicy: InMemoryPackagePolicy;
agentPolicy: GetAgentPoliciesResponseItem;
}

const AddAgentButton = styled(EuiButtonIcon)`
margin-left: ${(props) => props.theme.eui.euiSizeS};
`;

const IntegrationDetailsLink = memo<{
packagePolicy: PackagePolicyAndAgentPolicy['packagePolicy'];
packagePolicy: InMemoryPackagePolicyAndAgentPolicy['packagePolicy'];
}>(({ packagePolicy }) => {
const { getHref } = useLink();
return (
Expand All @@ -71,10 +80,6 @@ const IntegrationDetailsLink = memo<{
);
});

interface PackagePoliciesPanelProps {
name: string;
version: string;
}
export const PackagePoliciesPage = ({ name, version }: PackagePoliciesPanelProps) => {
const { search } = useLocation();
const history = useHistory();
Expand Down Expand Up @@ -109,12 +114,12 @@ export const PackagePoliciesPage = ({ name, version }: PackagePoliciesPanelProps
const updatableIntegrationRecord = updatableIntegrations.get(
packagePolicy.package?.name ?? ''
);

const hasUpgrade =
!!updatableIntegrationRecord &&
updatableIntegrationRecord.policiesToUpgrade.some(
({ id }) => id === packagePolicy.policy_id
({ pkgPolicyId }) => pkgPolicyId === packagePolicy.id
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is line is the fix to disable the button

Copy link
Contributor

Choose a reason for hiding this comment

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

So policy_id was an incorrect field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, updatableIntegration.policiesToUpgrade contains information about the agent policy (Default policy in this case) and the package policy (which has an upgrade available), where id is the ID of the agent policy and pkgPolicyId is the ID of the package policy.

Previously we were incorrectly comparing the agent policy IDs so every package policy in an agent policy would show as needing to upgrade. I have changed it so that we compare the package policy ID so that only the out of date packages show as needing an upgrade.

);

return {
agentPolicy,
packagePolicy: {
Expand Down Expand Up @@ -143,7 +148,7 @@ export const PackagePoliciesPage = ({ name, version }: PackagePoliciesPanelProps
}, [history]);

const handleTableOnChange = useCallback(
({ page }: CriteriaWithPagination<PackagePolicyAndAgentPolicy>) => {
({ page }: CriteriaWithPagination<InMemoryPackagePolicyAndAgentPolicy>) => {
setPagination({
currentPage: page.index + 1,
pageSize: page.size,
Expand Down Expand Up @@ -196,7 +201,7 @@ export const PackagePoliciesPage = ({ name, version }: PackagePoliciesPanelProps
};
}, [name, version, getHref, agentEnrollmentFlyoutExtension]);

const columns: Array<EuiTableFieldDataColumnType<PackagePolicyAndAgentPolicy>> = useMemo(
const columns: Array<EuiTableFieldDataColumnType<InMemoryPackagePolicyAndAgentPolicy>> = useMemo(
() => [
{
field: 'packagePolicy.name',
Expand All @@ -213,16 +218,6 @@ export const PackagePoliciesPage = ({ name, version }: PackagePoliciesPanelProps
defaultMessage: 'Version',
}),
render(_version, { agentPolicy, packagePolicy }) {
const updatableIntegrationRecord = updatableIntegrations.get(
packagePolicy.package?.name ?? ''
);

const hasUpgrade =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

InMemoryPackagePolicy has a hasUpgrade property so we don't need to duplicate the logic here

!!updatableIntegrationRecord &&
updatableIntegrationRecord.policiesToUpgrade.some(
({ pkgPolicyId }) => pkgPolicyId === packagePolicy.id
);

return (
<EuiFlexGroup gutterSize="s" alignItems="center">
<EuiFlexItem grow={false}>
Expand All @@ -235,7 +230,7 @@ export const PackagePoliciesPage = ({ name, version }: PackagePoliciesPanelProps
</EuiText>
</EuiFlexItem>

{hasUpgrade && (
{packagePolicy.hasUpgrade && (
<EuiFlexItem grow={false}>
<EuiButton
size="s"
Expand Down Expand Up @@ -274,7 +269,7 @@ export const PackagePoliciesPage = ({ name, version }: PackagePoliciesPanelProps
truncateText: true,
align: 'left',
width: '8ch',
render({ packagePolicy, agentPolicy }: PackagePolicyAndAgentPolicy) {
render({ packagePolicy, agentPolicy }: InMemoryPackagePolicyAndAgentPolicy) {
const count = agentPolicy?.agents ?? 0;

return (
Expand Down Expand Up @@ -327,7 +322,7 @@ export const PackagePoliciesPage = ({ name, version }: PackagePoliciesPanelProps
defaultMessage: 'Last Updated',
}),
truncateText: true,
render(updatedAt: PackagePolicyAndAgentPolicy['packagePolicy']['updated_at']) {
render(updatedAt: InMemoryPackagePolicyAndAgentPolicy['packagePolicy']['updated_at']) {
return (
<span className="eui-textTruncate" title={updatedAt}>
<FormattedRelative value={updatedAt} />
Expand Down Expand Up @@ -358,7 +353,7 @@ export const PackagePoliciesPage = ({ name, version }: PackagePoliciesPanelProps
},
},
],
[getHref, updatableIntegrations, viewDataStep]
[getHref, viewDataStep]
);

const noItemsMessage = useMemo(() => {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import React from 'react';

import { act } from '@testing-library/react';

import type { AgentPolicy, InMemoryPackagePolicy } from '../types';
import { createIntegrationsTestRendererMock } from '../mock';

import { PackagePolicyActionsMenu } from './package_policy_actions_menu';

function renderMenu({
agentPolicy,
packagePolicy,
showAddAgent = false,
isOpen = true,
}: {
agentPolicy: AgentPolicy;
packagePolicy: InMemoryPackagePolicy;
showAddAgent?: boolean;
isOpen?: boolean;
}) {
const renderer = createIntegrationsTestRendererMock();

const utils = renderer.render(
<PackagePolicyActionsMenu
agentPolicy={agentPolicy}
packagePolicy={packagePolicy}
showAddAgent={showAddAgent}
upgradePackagePolicyHref=""
isOpen={isOpen}
key="test1"
/>
);

return { utils };
}

function createMockAgentPolicy(props: Partial<AgentPolicy> = {}): AgentPolicy {
return {
id: 'some-uuid1',
namespace: 'default',
monitoring_enabled: [],
name: 'Test Policy',
description: '',
is_default: false,
is_preconfigured: false,
status: 'active',
is_managed: false,
revision: 1,
updated_at: '',
updated_by: 'elastic',
package_policies: [],
...props,
};
}

function createMockPackagePolicy(
props: Partial<InMemoryPackagePolicy> = {}
): InMemoryPackagePolicy {
return {
id: 'some-uuid2',
name: 'mock-package-policy',
description: '',
created_at: '',
created_by: '',
updated_at: '',
updated_by: '',
policy_id: '',
enabled: true,
output_id: '',
namespace: 'default',
inputs: [],
revision: 1,
hasUpgrade: false,
...props,
};
}

test('Should disable upgrade button if package does not have upgrade', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nchaulet these test were much simpler to write :D

const agentPolicy = createMockAgentPolicy();
const packagePolicy = createMockPackagePolicy({ hasUpgrade: false });
const { utils } = renderMenu({ agentPolicy, packagePolicy });
await act(async () => {
const upgradeButton = utils.getByText('Upgrade package policy').closest('button');
expect(upgradeButton).toBeDisabled();
});
});

test('Should enable upgrade button if package does has upgrade', async () => {
const agentPolicy = createMockAgentPolicy();
const packagePolicy = createMockPackagePolicy({ hasUpgrade: true });
const { utils } = renderMenu({ agentPolicy, packagePolicy });
await act(async () => {
const upgradeButton = utils.getByText('Upgrade package policy').closest('button');
expect(upgradeButton).not.toBeDisabled();
});
});

test('Should not be able to delete integration from a managed policy', async () => {
const agentPolicy = createMockAgentPolicy({ is_managed: true });
const packagePolicy = createMockPackagePolicy();
const { utils } = renderMenu({ agentPolicy, packagePolicy });
await act(async () => {
expect(utils.queryByText('Delete integration')).toBeNull();
});
});

test('Should be able to delete integration from a non-managed policy', async () => {
const agentPolicy = createMockAgentPolicy({ is_managed: false });
const packagePolicy = createMockPackagePolicy();
const { utils } = renderMenu({ agentPolicy, packagePolicy });
await act(async () => {
expect(utils.queryByText('Delete integration')).not.toBeNull();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,21 @@ export const PackagePolicyActionsMenu: React.FunctionComponent<{
packagePolicy: InMemoryPackagePolicy;
viewDataStep?: EuiStepProps;
showAddAgent?: boolean;
isOpen?: boolean;
upgradePackagePolicyHref: string;
}> = ({ agentPolicy, packagePolicy, viewDataStep, showAddAgent, upgradePackagePolicyHref }) => {
}> = ({
agentPolicy,
packagePolicy,
viewDataStep,
showAddAgent,
upgradePackagePolicyHref,
isOpen = false,
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 have added this isOpen prop to aid testing, I could've fired a click event but I didn't think it was unreasonable for the component to have this prop?

Copy link
Member

Choose a reason for hiding this comment

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

@hop-dev nitpick what do you think of naming this defaultIsOpen as the way I saw it it's the default value for the state correct?

}) => {
const [isEnrollmentFlyoutOpen, setIsEnrollmentFlyoutOpen] = useState(false);
const { getHref } = useLink();
const hasWriteCapabilities = useCapabilities().write;
const refreshAgentPolicy = useAgentPolicyRefresh();
const [isActionsMenuOpen, setIsActionsMenuOpen] = useState(false);
const [isActionsMenuOpen, setIsActionsMenuOpen] = useState(isOpen);

const onEnrollmentFlyoutClose = useMemo(() => {
return () => setIsEnrollmentFlyoutOpen(false);
Expand Down Expand Up @@ -84,6 +92,7 @@ export const PackagePolicyActionsMenu: React.FunctionComponent<{
disabled={!packagePolicy.hasUpgrade}
icon="refresh"
href={upgradePackagePolicyHref}
key="packagePolicyUpgrade"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

a warning was being logged because this menu item didn't have a key

>
<FormattedMessage
id="xpack.fleet.policyDetails.packagePoliciesTable.upgradeActionTitle"
Expand Down Expand Up @@ -135,7 +144,7 @@ export const PackagePolicyActionsMenu: React.FunctionComponent<{
<ContextMenuActions
isOpen={isActionsMenuOpen}
items={menuItems}
onChange={(isOpen) => setIsActionsMenuOpen(isOpen)}
onChange={(open) => setIsActionsMenuOpen(open)}
/>
</>
);
Expand Down