-
Notifications
You must be signed in to change notification settings - Fork 357
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-7927] - Linode Create Refactor - Part 6 - Add-ons #10319
Changes from 2 commits
606df38
6a98104
d6c9ee8
bdaa95d
881bf58
7b86f37
8a453e6
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 |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@linode/api-v4": Changed | ||
--- | ||
|
||
Added jsdoc style comments to `CreateLinodeRequest` based on API documentation ([#10319](https://github.com/linode/manager/pull/10319)) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@linode/api-v4": Changed | ||
--- | ||
|
||
Allows `firewall_id` to be `null` in `CreateLinodeRequest` ([#10319](https://github.com/linode/manager/pull/10319)) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@linode/manager": Upcoming Features | ||
--- | ||
|
||
Linode Create Refactor - Part 6 - Add-ons ([#10319](https://github.com/linode/manager/pull/10319)) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
import React from 'react'; | ||
|
||
import { regionFactory } from 'src/factories'; | ||
import { makeResourcePage } from 'src/mocks/serverHandlers'; | ||
import { HttpResponse, http, server } from 'src/mocks/testServer'; | ||
import { renderWithThemeAndHookFormContext } from 'src/utilities/testHelpers'; | ||
|
||
import { Addons } from './Addons'; | ||
|
||
describe('Linode Create v2 Addons', () => { | ||
it('should render an "Add-ons" heading', () => { | ||
const { getByText } = renderWithThemeAndHookFormContext({ | ||
component: <Addons />, | ||
}); | ||
|
||
const heading = getByText('Add-ons'); | ||
|
||
expect(heading).toBeVisible(); | ||
expect(heading.tagName).toBe('H2'); | ||
}); | ||
|
||
it('renders a warning if an edge region is selected', async () => { | ||
const region = regionFactory.build({ site_type: 'edge' }); | ||
|
||
server.use( | ||
http.get('*/v4/regions', () => { | ||
return HttpResponse.json(makeResourcePage([region])); | ||
}) | ||
); | ||
|
||
const { findByText } = renderWithThemeAndHookFormContext({ | ||
component: <Addons />, | ||
useFormOptions: { defaultValues: { region: region.id } }, | ||
}); | ||
|
||
await findByText( | ||
'Backups and Private IP are currently not available for Edge regions.' | ||
); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
import React from 'react'; | ||
import { useWatch } from 'react-hook-form'; | ||
|
||
import { Divider } from 'src/components/Divider'; | ||
import { Notice } from 'src/components/Notice/Notice'; | ||
import { Paper } from 'src/components/Paper'; | ||
import { Stack } from 'src/components/Stack'; | ||
import { Typography } from 'src/components/Typography'; | ||
import { useRegionsQuery } from 'src/queries/regions/regions'; | ||
|
||
import { Backups } from './Backups'; | ||
import { PrivateIP } from './PrivateIP'; | ||
|
||
import type { CreateLinodeRequest } from '@linode/api-v4'; | ||
|
||
export const Addons = () => { | ||
const regionId = useWatch<CreateLinodeRequest, 'region'>({ name: 'region' }); | ||
|
||
const { data: regions } = useRegionsQuery(); | ||
|
||
const selectedRegion = regions?.find((r) => r.id === regionId); | ||
|
||
const isEdgeRegionSelected = selectedRegion?.site_type === 'edge'; | ||
|
||
return ( | ||
<Paper> | ||
<Stack spacing={2}> | ||
<Typography variant="h2">Add-ons</Typography> | ||
{isEdgeRegionSelected && ( | ||
<Notice | ||
text="Backups and Private IP are currently not available for Edge regions." | ||
variant="warning" | ||
/> | ||
)} | ||
<Stack divider={<Divider />} spacing={2}> | ||
<Backups /> | ||
<PrivateIP /> | ||
</Stack> | ||
</Stack> | ||
</Paper> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
import { waitFor } from '@testing-library/react'; | ||
import React from 'react'; | ||
|
||
import { accountSettingsFactory, regionFactory } from 'src/factories'; | ||
import { makeResourcePage } from 'src/mocks/serverHandlers'; | ||
import { HttpResponse, http, server } from 'src/mocks/testServer'; | ||
import { renderWithThemeAndHookFormContext } from 'src/utilities/testHelpers'; | ||
|
||
import { Backups } from './Backups'; | ||
|
||
import type { CreateLinodeRequest } from '@linode/api-v4'; | ||
|
||
describe('Linode Create V2 Backups Addon', () => { | ||
it('should render a label and checkbox', () => { | ||
const { getByLabelText } = renderWithThemeAndHookFormContext({ | ||
component: <Backups />, | ||
}); | ||
|
||
const checkbox = getByLabelText('Backups', { exact: false }); | ||
|
||
expect(checkbox).toBeEnabled(); | ||
expect(checkbox).not.toBeChecked(); | ||
}); | ||
|
||
it('should get its value from the from context', () => { | ||
bnussman-akamai marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const { | ||
getByRole, | ||
} = renderWithThemeAndHookFormContext<CreateLinodeRequest>({ | ||
component: <Backups />, | ||
useFormOptions: { defaultValues: { backups_enabled: true } }, | ||
}); | ||
|
||
const checkbox = getByRole('checkbox'); | ||
|
||
expect(checkbox).toBeEnabled(); | ||
expect(checkbox).toBeChecked(); | ||
}); | ||
|
||
it('should render special copy, be checked, and be disabled if account backups are enabled', async () => { | ||
server.use( | ||
http.get('*/v4/account/settings', () => { | ||
return HttpResponse.json( | ||
accountSettingsFactory.build({ backups_enabled: true }) | ||
); | ||
}) | ||
); | ||
|
||
const { findByText, getByRole } = renderWithThemeAndHookFormContext({ | ||
component: <Backups />, | ||
}); | ||
|
||
const checkbox = getByRole('checkbox'); | ||
|
||
await findByText('You have enabled automatic backups for your account.', { | ||
exact: false, | ||
}); | ||
|
||
expect(checkbox).toBeDisabled(); | ||
expect(checkbox).toBeChecked(); | ||
}); | ||
|
||
it('should be disabled if an edge region is selected', async () => { | ||
const region = regionFactory.build({ site_type: 'edge' }); | ||
|
||
server.use( | ||
http.get('*/v4/regions', () => { | ||
return HttpResponse.json(makeResourcePage([region])); | ||
}) | ||
); | ||
|
||
const { | ||
getByRole, | ||
} = renderWithThemeAndHookFormContext<CreateLinodeRequest>({ | ||
component: <Backups />, | ||
useFormOptions: { defaultValues: { region: region.id } }, | ||
}); | ||
|
||
const checkbox = getByRole('checkbox'); | ||
|
||
await waitFor(() => { | ||
expect(checkbox).toBeDisabled(); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,90 @@ | ||
import React from 'react'; | ||
import { useController, useWatch } from 'react-hook-form'; | ||
|
||
import { Checkbox } from 'src/components/Checkbox'; | ||
import { Currency } from 'src/components/Currency'; | ||
import { FormControlLabel } from 'src/components/FormControlLabel'; | ||
import { Link } from 'src/components/Link'; | ||
import { Stack } from 'src/components/Stack'; | ||
import { Typography } from 'src/components/Typography'; | ||
import { useRestrictedGlobalGrantCheck } from 'src/hooks/useRestrictedGlobalGrantCheck'; | ||
import { useAccountSettings } from 'src/queries/account/settings'; | ||
import { useRegionsQuery } from 'src/queries/regions/regions'; | ||
import { useTypeQuery } from 'src/queries/types'; | ||
import { getMonthlyBackupsPrice } from 'src/utilities/pricing/backups'; | ||
|
||
import { getBackupsEnabledValue } from './utilities'; | ||
|
||
import type { CreateLinodeRequest } from '@linode/api-v4'; | ||
|
||
export const Backups = () => { | ||
const { field } = useController<CreateLinodeRequest, 'backups_enabled'>({ | ||
name: 'backups_enabled', | ||
}); | ||
|
||
const isLinodeCreateRestricted = useRestrictedGlobalGrantCheck({ | ||
globalGrantType: 'add_linodes', | ||
}); | ||
|
||
const regionId = useWatch<CreateLinodeRequest, 'region'>({ name: 'region' }); | ||
const typeId = useWatch<CreateLinodeRequest, 'type'>({ name: 'type' }); | ||
|
||
const { data: type } = useTypeQuery(typeId, Boolean(typeId)); | ||
const { data: regions } = useRegionsQuery(); | ||
const { data: accountSettings } = useAccountSettings(); | ||
|
||
const backupsMonthlyPrice = getMonthlyBackupsPrice({ | ||
region: regionId, | ||
type, | ||
}); | ||
|
||
const selectedRegion = regions?.find((r) => r.id === regionId); | ||
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. I am still finding this a bit flawed. Yes, this is relatively inexpensive but because of only storing the region ID in the form state instead of the whole region object we are probably running 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, this is a great point. I was considering adding a This might be worth it considering how often we call Have any thoughts on that or other ideas? 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. Would it be possible/a good idea to break pattern and have the form state being the whole region? 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. I don't think that would be great. I think it would make things a ton more complicated with regarding types and validation 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. makes sense. then i think the best option would be a useMemo util so at least we're not recalculating every time? |
||
|
||
const isAccountBackupsEnabled = accountSettings?.backups_enabled ?? false; | ||
|
||
const isEdgeRegionSelected = selectedRegion?.site_type === 'edge'; | ||
|
||
return ( | ||
<FormControlLabel | ||
checked={getBackupsEnabledValue({ | ||
accountBackupsEnabled: isAccountBackupsEnabled, | ||
isEdgeRegion: isEdgeRegionSelected, | ||
value: field.value, | ||
})} | ||
disabled={ | ||
isEdgeRegionSelected || | ||
isLinodeCreateRestricted || | ||
isAccountBackupsEnabled | ||
} | ||
label={ | ||
<Stack sx={{ pl: 2 }}> | ||
<Stack alignItems="center" direction="row" spacing={2}> | ||
<Typography variant="h3">Backups</Typography> | ||
{backupsMonthlyPrice && ( | ||
<Typography> | ||
<Currency quantity={backupsMonthlyPrice} /> per month | ||
</Typography> | ||
)} | ||
</Stack> | ||
<Typography> | ||
{isAccountBackupsEnabled ? ( | ||
<React.Fragment> | ||
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. are we still doing fragments this way rather than 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. This was just copy paste from |
||
You have enabled automatic backups for your account. This Linode | ||
will automatically have backups enabled. To change this setting,{' '} | ||
<Link to={'/account/settings'}>click here.</Link> | ||
</React.Fragment> | ||
) : ( | ||
<React.Fragment> | ||
Three backup slots are executed and rotated automatically: a | ||
daily backup, a 2-7 day old backup, and an 8-14 day old backup. | ||
Plans are priced according to the Linode plan selected above. | ||
</React.Fragment> | ||
)} | ||
</Typography> | ||
</Stack> | ||
} | ||
control={<Checkbox />} | ||
onChange={field.onChange} | ||
/> | ||
); | ||
}; |
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.
There are other notices that will need to be added here related to cloning. I will add those when I add cloning support down the road.