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

fix: [M3-7741] - Hide error notices for $0 regions in Resize Pool and Add a Node Pool drawers #10157

Original file line number Diff line number Diff line change
Expand Up @@ -100,11 +100,13 @@ export const AddNodePoolDrawer = (props: Props) => {

const pricePerNode = getLinodeRegionPrice(selectedType, clusterRegionId)
?.monthly;
const isInvalidPricePerNode = !pricePerNode && pricePerNode !== 0;

const totalPrice =
selectedTypeInfo && pricePerNode
selectedTypeInfo && !isInvalidPricePerNode
? selectedTypeInfo.count * pricePerNode
: undefined;
const isInvalidTotalPrice = !totalPrice && totalPrice !== 0;

React.useEffect(() => {
if (open) {
Expand Down Expand Up @@ -199,7 +201,7 @@ export const AddNodePoolDrawer = (props: Props) => {
/>
)}

{selectedTypeInfo && !totalPrice && !pricePerNode && (
{selectedTypeInfo && isInvalidPricePerNode && isInvalidTotalPrice && (
<Notice
spacingBottom={16}
spacingTop={8}
Expand Down Expand Up @@ -229,7 +231,10 @@ export const AddNodePoolDrawer = (props: Props) => {
)}
<ActionsPanel
primaryButtonProps={{
disabled: !selectedTypeInfo,
disabled:
!selectedTypeInfo ||
isInvalidTotalPrice ||
isInvalidPricePerNode,
label: 'Add pool',
loading: isLoading,
onClick: handleAdd,
Expand Down
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes in the file are to account for the loading state that is now present before form data is rendered.

Original file line number Diff line number Diff line change
Expand Up @@ -26,18 +26,19 @@ describe('ResizeNodePoolDrawer', () => {
await findByText(/linode 1 GB/i);
});

it('should display a warning if the user tries to resize a node pool to < 3 nodes', () => {
const { getByText } = renderWithTheme(
it('should display a warning if the user tries to resize a node pool to < 3 nodes', async () => {
const { findByText } = renderWithTheme(
<ResizeNodePoolDrawer {...props} nodePool={smallPool} />
);
expect(getByText(/minimum of 3 nodes/i));
expect(await findByText(/minimum of 3 nodes/i));
});

it('should display a warning if the user tries to resize to a smaller node count', () => {
const { getByTestId, getByText } = renderWithTheme(
it('should display a warning if the user tries to resize to a smaller node count', async () => {
const { findByTestId, getByText } = renderWithTheme(
<ResizeNodePoolDrawer {...props} />
);
const decrement = getByTestId('decrement-button');

const decrement = await findByTestId('decrement-button');
fireEvent.click(decrement);
expect(getByText(/resizing to fewer nodes/i));
});
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { KubeNodePoolResponse, Region } from '@linode/api-v4';
import { Theme } from '@mui/material/styles';
import { makeStyles } from 'tss-react/mui';
import * as React from 'react';
import { makeStyles } from 'tss-react/mui';

import { ActionsPanel } from 'src/components/ActionsPanel/ActionsPanel';
import { CircleProgress } from 'src/components/CircleProgress';
Expand Down Expand Up @@ -107,85 +107,92 @@ export const ResizeNodePoolDrawer = (props: Props) => {
types: planType ? [planType] : [],
});

const isInvalidPricePerNode = !pricePerNode && pricePerNode !== 0;
const isInvalidTotalMonthlyPrice =
!totalMonthlyPrice && totalMonthlyPrice !== 0;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

undefined and null prices are invalid, but 0 is allowed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be good to have this clarification as a comment in the code

Copy link
Contributor

Choose a reason for hiding this comment

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

I second this. Am also thinking you could consolidate this logic with the addNodePool drawer logic in a util with your comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 6b86e9a.

return (
<Drawer
onClose={onClose}
open={open}
title={`Resize Pool: ${planType?.formattedLabel ?? 'Unknown'} Plan`}
>
{isLoadingTypes && <CircleProgress />}
<form
onSubmit={(e: React.ChangeEvent<HTMLFormElement>) => {
e.preventDefault();
handleSubmit();
}}
>
<div className={classes.section}>
{totalMonthlyPrice && (
{isLoadingTypes ? (
<CircleProgress />
) : (
<form
onSubmit={(e: React.ChangeEvent<HTMLFormElement>) => {
e.preventDefault();
handleSubmit();
}}
>
<div className={classes.section}>
Comment on lines +123 to +132
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of this diff is an indentation change caused by fixing an issue where the loading spinner was displaying above the form, instead of in its place, while isLoadingTypes is true. This was visible when I blocked network requests as described in the testing steps.

Before After
Screen.Recording.2024-02-07.at.9.01.57.AM.mov
Screen.Recording.2024-02-07.at.9.01.12.AM.mov

<Typography className={classes.summary}>
Current pool: $
{renderMonthlyPriceToCorrectDecimalPlace(totalMonthlyPrice)}/month{' '}
({pluralize('node', 'nodes', nodePool.count)} at $
{renderMonthlyPriceToCorrectDecimalPlace(totalMonthlyPrice)}
/month ({pluralize('node', 'nodes', nodePool.count)} at $
{renderMonthlyPriceToCorrectDecimalPlace(pricePerNode)}
/month)
</Typography>
)}
</div>

{error && <Notice text={error?.[0].reason} variant="error" />}

<div className={classes.section}>
<Typography className={classes.helperText}>
Enter the number of nodes you'd like in this pool:
</Typography>
<EnhancedNumberInput
min={1}
setValue={handleChange}
value={updatedCount}
/>
</div>
</div>

{error && <Notice text={error?.[0].reason} variant="error" />}

<div className={classes.section}>
{/* Renders total pool price/month for N nodes at price per node/month. */}
{pricePerNode && (
<div className={classes.section}>
<Typography className={classes.helperText}>
Enter the number of nodes you'd like in this pool:
</Typography>
<EnhancedNumberInput
min={1}
setValue={handleChange}
value={updatedCount}
/>
</div>

<div className={classes.section}>
{/* Renders total pool price/month for N nodes at price per node/month. */}
<Typography className={classes.summary}>
{`Resized pool: $${renderMonthlyPriceToCorrectDecimalPlace(
updatedCount * pricePerNode
!isInvalidPricePerNode ? updatedCount * pricePerNode : undefined
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the pricePerNode could be null or undefined, we can't multiply a non-number. renderMonthlyPriceToCorrectDecimalPlace() has logic to render a non-numeric price as $--.--.

)}/month`}{' '}
({pluralize('node', 'nodes', updatedCount)} at $
{renderMonthlyPriceToCorrectDecimalPlace(pricePerNode)}
/month)
</Typography>
</div>

{updatedCount < nodePool.count && (
<Notice important text={resizeWarning} variant="warning" />
)}

{updatedCount < 3 && (
<Notice important text={nodeWarning} variant="warning" />
)}
</div>

{updatedCount < nodePool.count && (
<Notice important text={resizeWarning} variant="warning" />
)}

{updatedCount < 3 && (
<Notice important text={nodeWarning} variant="warning" />
)}

{nodePool.count && (!pricePerNode || !totalMonthlyPrice) && (
<Notice
spacingBottom={16}
spacingTop={8}
text={PRICES_RELOAD_ERROR_NOTICE_TEXT}
variant="error"

{nodePool.count &&
(isInvalidPricePerNode || isInvalidTotalMonthlyPrice) && (
Copy link
Contributor Author

@mjac0bs mjac0bs Feb 7, 2024

Choose a reason for hiding this comment

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

Only real change in this section is the updated bools to determine whether to display the pricing error notice or not - the rest of the diff is due to indentation.

<Notice
spacingBottom={16}
spacingTop={8}
text={PRICES_RELOAD_ERROR_NOTICE_TEXT}
variant="error"
/>
)}

<ActionsPanel
primaryButtonProps={{
'data-testid': 'submit',
disabled:
updatedCount === nodePool.count ||
isInvalidPricePerNode ||
isInvalidTotalMonthlyPrice,
label: 'Save Changes',
loading: isLoading,
onClick: handleSubmit,
}}
/>
)}

<ActionsPanel
primaryButtonProps={{
'data-testid': 'submit',
disabled: updatedCount === nodePool.count,
label: 'Save Changes',
loading: isLoading,
onClick: handleSubmit,
}}
/>
</form>
</form>
)}
</Drawer>
);
};
Loading