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

upcoming: [M3-7611] - Placement Groups Detail Summary #10164

Merged
merged 11 commits into from
Feb 12, 2024
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@linode/manager": Upcoming Features
---

Create PlacementGroups Summary component ([#10164](https://github.com/linode/manager/pull/10164))
Original file line number Diff line number Diff line change
@@ -1,16 +1,20 @@
import { AFFINITY_TYPES } from '@linode/api-v4';
import { useTheme } from '@mui/material';
import * as React from 'react';
import { useHistory, useParams } from 'react-router-dom';
import { Link } from 'react-router-dom';
carrillo-erik marked this conversation as resolved.
Show resolved Hide resolved

import { CircleProgress } from 'src/components/CircleProgress';
import { DocumentTitleSegment } from 'src/components/DocumentTitle';
import { ErrorState } from 'src/components/ErrorState/ErrorState';
import { LandingHeader } from 'src/components/LandingHeader';
import { NotFound } from 'src/components/NotFound';
import { Notice } from 'src/components/Notice/Notice';
import { SafeTabPanel } from 'src/components/Tabs/SafeTabPanel';
import { TabLinkList } from 'src/components/Tabs/TabLinkList';
import { TabPanels } from 'src/components/Tabs/TabPanels';
import { Tabs } from 'src/components/Tabs/Tabs';
import { Typography } from 'src/components/Typography';
import { useFlags } from 'src/hooks/useFlags';
import {
useMutatePlacementGroup,
Expand All @@ -19,23 +23,29 @@ import {
import { getErrorStringOrDefault } from 'src/utilities/errorUtils';

import { getPlacementGroupLinodeCount } from '../utils';
import { getWarningNoticeText } from '../utils';
import { PlacementGroupsLinodes } from './PlacementGroupsLinodes/PlacementGroupsLinodes';
import { PlacementGroupsSummary } from './PlacementGroupsSummary/PlacementGroupsSummary';

export const PlacementGroupsDetail = () => {
const flags = useFlags();
const { id, tab } = useParams<{ id: string; tab?: string }>();
const history = useHistory();
const theme = useTheme();
const placementGroupId = Number(id);

const {
data: placementGroup,
error: placementGroupError,
isLoading,
} = usePlacementGroupQuery(placementGroupId, Boolean(flags.vmPlacement));

const {
error: updatePlacementGroupError,
mutateAsync: updatePlacementGroup,
reset,
} = useMutatePlacementGroup(placementGroupId);

const errorText = getErrorStringOrDefault(updatePlacementGroupError ?? '');

if (isLoading) {
Expand Down Expand Up @@ -108,7 +118,24 @@ export const PlacementGroupsDetail = () => {
>
<TabLinkList tabs={tabs} />
<TabPanels>
<SafeTabPanel index={0}>TODO VM_Placement: summary</SafeTabPanel>
<SafeTabPanel index={0}>
{!placementGroup.compliant && (
<Notice spacingBottom={20} spacingTop={24} variant="warning">
<Typography fontFamily={theme.font.bold}>
{getWarningNoticeText(placementGroup)}
{/* TODO VM_Placement: Get link location */}
<Link
className="secondaryLink"
data-testid="pg-non-compliant-notice-link"
to={'#'}
>
Learn more.
</Link>
</Typography>
</Notice>
Copy link
Contributor

Choose a reason for hiding this comment

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

The Notice should live inside the PlacementGroupSummary component

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abailly-akamai I had originally done this, however, having the <Notice /> inside the <PlacementGroupSummary /> was the reason why the unit test was failing. This led me to rethink about the component composition and I feel the <Notice /> should live outside of the <PlacementGroupSummary /> as this component is only meant to display the config details. It's a personal preference but if you believe otherwise I can make the change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that test failure should warrant this change. The notice only pertains to the summary so it should be there. You put it in the SafeTabPanel which isn't right. Let's put it back inside the summary and fix the test. I can help if you can't find the source of the failure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the changes.

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 didn't use a constant and instead incorporated the string right in the component, this seems to have solved the issue I was encountering with the unit test.

)}
<PlacementGroupsSummary placementGroup={placementGroup} />
</SafeTabPanel>
<SafeTabPanel index={1}>
<PlacementGroupsLinodes placementGroup={placementGroup} />
</SafeTabPanel>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import * as React from 'react';

import { placementGroupFactory } from 'src/factories';
import { renderWithTheme } from 'src/utilities/testHelpers';

import { PlacementGroupsSummary } from './PlacementGroupsSummary';

describe('PlacementGroups Summary', () => {
it('renders the placement group detail summary panel', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: we could also check if the tooltip exists.

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 included this check in the unit test.

const { getByText } = renderWithTheme(
<PlacementGroupsSummary
placementGroup={placementGroupFactory.build({
affinity_type: 'affinity',
capacity: 10,
compliant: true,
id: 3,
label: 'pg-3',
linode_ids: [2, 4, 6, 8, 10],
region: 'us-east',
})}
/>
);

expect(getByText('Placement Group Configuration')).toBeInTheDocument();
expect(getByText('Linodes')).toBeInTheDocument();
expect(getByText('Affinity Type')).toBeInTheDocument();
expect(getByText('Region')).toBeInTheDocument();
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
import { AFFINITY_TYPES } from '@linode/api-v4';
import Grid from '@mui/material/Unstable_Grid2';
import { styled } from '@mui/material/styles';
import * as React from 'react';

import { Box } from 'src/components/Box';
import { Paper } from 'src/components/Paper';
import { TooltipIcon } from 'src/components/TooltipIcon';
import { Typography } from 'src/components/Typography';
import { useRegionsQuery } from 'src/queries/regions';

import { PLACEMENT_GROUP_TOOLTIP_TEXT } from '../../constants';

import type { PlacementGroup } from '@linode/api-v4';

interface Props {
placementGroup: PlacementGroup;
}

export const PlacementGroupsSummary = (props: Props) => {
const { placementGroup } = props;
const { data: regions } = useRegionsQuery();

const regionLabel =
regions?.find((region) => region.id === placementGroup.region)?.label ??
placementGroup.region;
carrillo-erik marked this conversation as resolved.
Show resolved Hide resolved

return (
<Paper>
<Typography
sx={(theme) => ({ marginBottom: theme.spacing(3) })}
variant="h3"
>
Placement Group Configuration
</Typography>
<Grid container spacing={1}>
<Grid md={8} sm={12}>
<Box display="flex">
<StyledLabel>Linodes</StyledLabel>
<Typography sx={{ mx: 8 }}>
{`${placementGroup.linode_ids.length} of ${placementGroup.capacity}`}
<TooltipIcon
sxTooltipIcon={{
marginLeft: '10px',
padding: 0,
}}
status="help"
text={PLACEMENT_GROUP_TOOLTIP_TEXT}
tooltipPosition="right"
/>
</Typography>
</Box>
</Grid>
<Grid md={8} sm={12}>
<Box display="flex">
<StyledLabel>Affinity Type</StyledLabel>
<Typography sx={{ mx: 8 }}>
{AFFINITY_TYPES[placementGroup?.affinity_type]}
</Typography>
</Box>
</Grid>
<Grid md={8} sm={12}>
<Box display="flex">
<StyledLabel>Region</StyledLabel>
<Typography sx={{ mx: 8 }}>{regionLabel}</Typography>
</Box>
</Grid>
</Grid>
</Paper>
);
};

export const StyledLabel = styled(Typography, {
label: 'StyledLabel',
})(({ theme }) => ({
fontFamily: theme.font.bold,
marginRight: theme.spacing(8),
width: theme.spacing(10),
}));
2 changes: 2 additions & 0 deletions packages/manager/src/features/PlacementGroups/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,5 @@ export const MAX_NUMBER_OF_LINODES_IN_PLACEMENT_GROUP_MESSAGE =

export const PLACEMENT_GROUP_LINODES_ERROR_MESSAGE =
'There was an error loading Linodes for this Placement Group.';

export const PLACEMENT_GROUP_TOOLTIP_TEXT = `The Affinity Type and Region determine the maximum number of VMs per group`;
mjac0bs marked this conversation as resolved.
Show resolved Hide resolved
10 changes: 10 additions & 0 deletions packages/manager/src/features/PlacementGroups/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,13 @@ export const affinityTypeOptions = Object.entries(AFFINITY_TYPES).map(
value: key as CreatePlacementGroupPayload['affinity_type'],
})
);

/**
* Helper to generate the text content of the non-compliant warning notice.
*/
export const getWarningNoticeText = (placementGroup: PlacementGroup) => {
return `Placement Group ${placementGroup.label} (${
AFFINITY_TYPES[placementGroup?.affinity_type]
}) is non-compliant.
We are working to resolve compliance issues so that you can continue assigning Linodes to this Placement Group. `;
};
carrillo-erik marked this conversation as resolved.
Show resolved Hide resolved
Loading