From c2cf937097e2a73d61b38e78e7d6ada8c644e71f Mon Sep 17 00:00:00 2001 From: Connie Liu <139280159+coliu-akamai@users.noreply.github.com> Date: Wed, 13 Sep 2023 16:36:37 -0400 Subject: [PATCH] change: [poc] - Better subnet validation behavior on VPCCreate page (#9659) * begin POC * need to test this + tests will fail rn * fix bugs and remove unneeded code * update naming and docs * changeset * way cleaner logic for subnet error handling ;-; * update variable to better name * Update packages/manager/src/features/VPCs/VPCCreate/VPCCreate.tsx Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com> * Update packages/manager/src/features/VPCs/VPCCreate/VPCCreate.tsx Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com> * Update packages/manager/src/features/VPCs/VPCCreate/VPCCreate.tsx Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com> * Update packages/manager/src/utilities/formikErrorUtils.ts Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com> * Update packages/manager/src/utilities/formikErrorUtils.ts Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com> * update comments to explain logic * simplify logic --------- Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com> --- .../pr-9659-changed-1694440105022.md | 5 + .../VPCs/VPCCreate/VPCCreate.test.tsx | 26 ++-- .../src/features/VPCs/VPCCreate/VPCCreate.tsx | 126 +++++++++++------- .../src/utilities/formikErrorUtils.test.ts | 100 +++++++++++++- .../manager/src/utilities/formikErrorUtils.ts | 54 ++++++++ .../manager/src/utilities/subnets.test.ts | 109 +-------------- packages/manager/src/utilities/subnets.ts | 38 +----- 7 files changed, 249 insertions(+), 209 deletions(-) create mode 100644 packages/manager/.changeset/pr-9659-changed-1694440105022.md diff --git a/packages/manager/.changeset/pr-9659-changed-1694440105022.md b/packages/manager/.changeset/pr-9659-changed-1694440105022.md new file mode 100644 index 00000000000..fd4754a0f5e --- /dev/null +++ b/packages/manager/.changeset/pr-9659-changed-1694440105022.md @@ -0,0 +1,5 @@ +--- +"@linode/manager": Changed +--- + +Update VPCCreate validation for subnets ([#9659](https://github.com/linode/manager/pull/9659)) diff --git a/packages/manager/src/features/VPCs/VPCCreate/VPCCreate.test.tsx b/packages/manager/src/features/VPCs/VPCCreate/VPCCreate.test.tsx index 2308c4e2991..89d88b96ac3 100644 --- a/packages/manager/src/features/VPCs/VPCCreate/VPCCreate.test.tsx +++ b/packages/manager/src/features/VPCs/VPCCreate/VPCCreate.test.tsx @@ -29,11 +29,15 @@ describe('VPC create page', () => { getAllByText('Create VPC'); }); - // test fails due to new default value for subnet ip addresses + // test fails due to new default value for subnet ip addresses - if we remove default text and just + // have a placeholder, we can put this test back in it.skip('should require vpc labels and region and ignore subnets that are blank', async () => { - const { getAllByTestId, getByText, queryByText } = renderWithTheme( - - ); + const { + getAllByTestId, + getByText, + getAllByText, + queryByText, + } = renderWithTheme(); const createVPCButton = getByText('Create VPC'); const subnetIP = getAllByTestId('textfield-input'); expect(createVPCButton).toBeInTheDocument(); @@ -43,14 +47,10 @@ describe('VPC create page', () => { }); const regionError = getByText('Region is required'); expect(regionError).toBeInTheDocument(); - const labelError = getByText('Label is required'); - expect(labelError).toBeInTheDocument(); + const labelErrors = getAllByText('Label is required'); + expect(labelErrors).toHaveLength(1); const badSubnetIP = queryByText('The IPv4 range must be in CIDR format'); expect(badSubnetIP).not.toBeInTheDocument(); - const badSubnetLabel = queryByText( - 'Label is required. Must only be ASCII letters, numbers, and dashes' - ); - expect(badSubnetLabel).not.toBeInTheDocument(); }); it('should add and delete subnets correctly', async () => { @@ -96,10 +96,8 @@ describe('VPC create page', () => { 'The IPv4 range must be in CIDR format' ); expect(badSubnetIP).toBeInTheDocument(); - const badSubnetLabel = screen.getByText( - 'Label is required. Must only be ASCII letters, numbers, and dashes' - ); - expect(badSubnetLabel).toBeInTheDocument(); + const badLabels = screen.getAllByText('Label is required'); + expect(badLabels).toHaveLength(2); }); it('should have a default value for the subnet ip address', () => { diff --git a/packages/manager/src/features/VPCs/VPCCreate/VPCCreate.tsx b/packages/manager/src/features/VPCs/VPCCreate/VPCCreate.tsx index 4e316322aec..1fb026d321f 100644 --- a/packages/manager/src/features/VPCs/VPCCreate/VPCCreate.tsx +++ b/packages/manager/src/features/VPCs/VPCCreate/VPCCreate.tsx @@ -21,12 +21,14 @@ import { TextField } from 'src/components/TextField'; import { useGrants, useProfile } from 'src/queries/profile'; import { useRegionsQuery } from 'src/queries/regions'; import { useCreateVPCMutation } from 'src/queries/vpcs'; -import { handleAPIErrors } from 'src/utilities/formikErrorUtils'; +import { + SubnetError, + handleVPCAndSubnetErrors, +} from 'src/utilities/formikErrorUtils'; import scrollErrorIntoView from 'src/utilities/scrollErrorIntoView'; import { DEFAULT_SUBNET_IPV4_VALUE, SubnetFieldState, - validateSubnets, } from 'src/utilities/subnets'; import { MultipleSubnetInput } from './MultipleSubnetInput'; @@ -42,64 +44,71 @@ const VPCCreate = () => { const { data: grants } = useGrants(); const { data: regions } = useRegionsQuery(); const { isLoading, mutateAsync: createVPC } = useCreateVPCMutation(); - const [subnetErrorsFromAPI, setSubnetErrorsFromAPI] = React.useState< - APIError[] - >(); + const [ + generalSubnetErrorsFromAPI, + setGeneralSubnetErrorsFromAPI, + ] = React.useState(); const [generalAPIError, setGeneralAPIError] = React.useState< string | undefined >(); const userCannotAddVPC = profile?.restricted && !grants?.global.add_vpcs; - const createSubnetsPayload = () => { - const subnetPayloads: CreateSubnetPayload[] = []; + // When creating the subnet payloads, we also create a mapping of the indexes of the subnets that appear on + // the UI to the indexes of the subnets that the API will receive. This enables users to leave subnets blank + // on the UI and still have any errors returned by the API correspond to the correct subnet + const createSubnetsPayloadAndMapping = () => { + const subnetsPayload: CreateSubnetPayload[] = []; + const subnetIdxMapping = {}; + let apiSubnetIdx = 0; - for (const subnetState of values.subnets) { - const { ip, label } = subnetState; + for (let i = 0; i < values.subnets.length; i++) { + const { ip, label } = values.subnets[i]; if (ip.ipv4 || label) { - subnetPayloads.push({ ipv4: ip.ipv4, label }); + subnetsPayload.push({ ipv4: ip.ipv4, label }); + subnetIdxMapping[i] = apiSubnetIdx; + apiSubnetIdx++; } } - return subnetPayloads; + return { + subnetsPayload, + visualToAPISubnetMapping: subnetIdxMapping, + }; }; - const validateVPCSubnets = () => { - const validatedSubnets = validateSubnets(values.subnets, { - ipv4Error: 'The IPv4 range must be in CIDR format', - labelError: - 'Label is required. Must only be ASCII letters, numbers, and dashes', + const combineErrorsAndSubnets = ( + errors: {}, + visualToAPISubnetMapping: {} + ) => { + return values.subnets.map((subnet, idx) => { + const apiSubnetIdx: number | undefined = visualToAPISubnetMapping[idx]; + // If the subnet has errors associated with it, include those errors in its state + if ((apiSubnetIdx || apiSubnetIdx === 0) && errors[apiSubnetIdx]) { + const errorData: SubnetError = errors[apiSubnetIdx]; + return { + ...subnet, + labelError: errorData.label ?? '', + // @TODO VPC: IPv6 error handling + ip: { + ...subnet.ip, + ipv4Error: errorData.ipv4 ?? '', + }, + }; + } else { + return subnet; + } }); - - if ( - validatedSubnets.some( - (subnet) => subnet.labelError || subnet.ip.ipv4Error - ) - ) { - setFieldValue('subnets', validatedSubnets); - return false; - } else { - setFieldValue( - 'subnets', - validatedSubnets.map((subnet) => { - delete subnet.labelError; - delete subnet.ip.ipv4Error; - return { ...subnet }; - }) - ); - return true; - } }; const onCreateVPC = async () => { - if (!validateVPCSubnets()) { - return; - } - setSubmitting(true); setGeneralAPIError(undefined); - const subnetsPayload = createSubnetsPayload(); + const { + subnetsPayload, + visualToAPISubnetMapping, + } = createSubnetsPayloadAndMapping(); const createVPCPayload: CreateVPCPayload = { ...values, @@ -110,19 +119,38 @@ const VPCCreate = () => { const response = await createVPC(createVPCPayload); history.push(`/vpcs/${response.id}`); } catch (errors) { - const apiSubnetErrors = errors.filter( - (error: APIError) => error.field && error.field.includes('subnets') + const generalSubnetErrors = errors.filter( + (error: APIError) => + // Both general and specific subnet errors include 'subnets' in their error field. + // General subnet errors come in as { field: subnets.some_field, ...}, whereas + // specific subnet errors come in as { field: subnets[some_index].some_field, ...}. So, + // to avoid specific subnet errors, we filter out errors with a field that includes '[' + error.field && + error.field.includes('subnets') && + !error.field.includes('[') ); - if (apiSubnetErrors) { - setSubnetErrorsFromAPI(apiSubnetErrors); + + if (generalSubnetErrors) { + setGeneralSubnetErrorsFromAPI(generalSubnetErrors); } - handleAPIErrors( + const indivSubnetErrors = handleVPCAndSubnetErrors( errors.filter( - (error: APIError) => !error.field?.includes('subnets') || !error.field + // ignore general subnet errors: !(the logic of filtering for only general subnet errors) + (error: APIError) => + !error.field?.includes('subnets') || + !error.field || + error.field.includes('[') ), setFieldError, setGeneralAPIError ); + + // must combine errors and subnet data to avoid indexing weirdness when deleting a subnet + const subnetsAndErrors = combineErrorsAndSubnets( + indivSubnetErrors, + visualToAPISubnetMapping + ); + setFieldValue('subnets', subnetsAndErrors); } setSubmitting(false); @@ -243,10 +271,10 @@ const VPCCreate = () => { allow for controlled access to VPC resources. Subnets within a VPC are routable regardless of the address spaces they are in. Learn more. - {/* TODO: VPC - subnet learn more link here */} + {/* @TODO VPC: subnet learn more link here */} - {subnetErrorsFromAPI - ? subnetErrorsFromAPI.map((apiError: APIError) => ( + {generalSubnetErrorsFromAPI + ? generalSubnetErrorsFromAPI.map((apiError: APIError) => ( { + jest.clearAllMocks(); +}); + const setFieldError = jest.fn(); const setError = jest.fn(); @@ -24,3 +28,97 @@ describe('handleAPIErrors', () => { expect(setError).toHaveBeenCalledWith(errorWithoutField[0].reason); }); }); + +const subnetMultipleErrorsPerField = [ + { + reason: 'not expected error for label', + field: 'subnets[0].label', + }, + { + reason: 'expected error for label', + field: 'subnets[0].label', + }, + { + reason: 'not expected error for label', + field: 'subnets[3].label', + }, + { + reason: 'expected error for label', + field: 'subnets[3].label', + }, + { + reason: 'not expected error for ipv4', + field: 'subnets[3].ipv4', + }, + { + reason: 'expected error for ipv4', + field: 'subnets[3].ipv4', + }, +]; + +const subnetErrors = [ + { + reason: 'Label required', + field: 'subnets[1].label', + }, + { + reason: 'bad label', + field: 'subnets[2].label', + }, + { + reason: 'cidr ipv4', + field: 'subnets[2].ipv4', + }, + { + reason: 'needs an ip', + field: 'subnets[4].ipv4', + }, + { + reason: 'needs an ipv6', + field: 'subnets[4].ipv6', + }, +]; + +describe('handleVpcAndConvertSubnetErrors', () => { + it('converts API errors for subnets into a map of subnet index to SubnetErrors', () => { + const errors = handleVPCAndSubnetErrors( + subnetErrors, + setFieldError, + setError + ); + expect(Object.keys(errors)).toHaveLength(3); + expect(Object.keys(errors)).toEqual(['1', '2', '4']); + expect(errors[1]).toEqual({ label: 'Label required' }); + expect(errors[2]).toEqual({ label: 'bad label', ipv4: 'cidr ipv4' }); + expect(errors[4]).toEqual({ ipv4: 'needs an ip', ipv6: 'needs an ipv6' }); + }); + + it('takes the last error to display if a subnet field has multiple errors associated with it', () => { + const errors = handleVPCAndSubnetErrors( + subnetMultipleErrorsPerField, + setFieldError, + setError + ); + expect(Object.keys(errors)).toHaveLength(2); + expect(errors[0]).toEqual({ label: 'expected error for label' }); + expect(errors[3]).toEqual({ + label: 'expected error for label', + ipv4: 'expected error for ipv4', + }); + }); + + it('passes errors without the subnet field to handleApiErrors', () => { + const errors = handleVPCAndSubnetErrors( + errorWithField, + setFieldError, + setError + ); + expect(Object.keys(errors)).toHaveLength(0); + expect(errors).toEqual({}); + expect(setFieldError).toHaveBeenCalledWith( + 'card_number', + errorWithField[0].reason + ); + expect(setError).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/manager/src/utilities/formikErrorUtils.ts b/packages/manager/src/utilities/formikErrorUtils.ts index 0881cc4770a..72899c97f50 100644 --- a/packages/manager/src/utilities/formikErrorUtils.ts +++ b/packages/manager/src/utilities/formikErrorUtils.ts @@ -66,3 +66,57 @@ export const handleAPIErrors = ( } }); }; + +export interface SubnetError { + label?: string; + ipv4?: string; + ipv6?: string; +} + +/** + * Handles given API errors and converts any specific subnet related errors into a usable format; + * Returns a map of subnets' indexes to their @interface SubnetError + * Example: errors = [{ reason: 'error1', field: 'subnets[1].label' }, + * { reason: 'error2', field: 'subnets[1].ipv4' }, + * { reason: 'not a subnet error so will not appear in return obj', field: 'label'}, + * { reason: 'error3', field: 'subnets[4].ipv4' }] + * returns: { + * 1: { label: 'error1', ipv4: 'error2' }, + * 4: { ipv4: 'error3'} + * } + * + * @param errors the errors from the API + * @param setFieldError function to set non-subnet related field errors + * @param setError function to set (non-subnet related) general API errors + */ +export const handleVPCAndSubnetErrors = ( + errors: APIError[], + setFieldError: (field: string, message: string) => void, + setError?: (message: string) => void +) => { + const subnetErrors = {}; + const nonSubnetErrors: APIError[] = []; + + errors.forEach((error) => { + if (error.field && error.field.includes('subnets[')) { + const [subnetIdx, field] = error.field.split('.'); + const idx = parseInt( + subnetIdx.substring(subnetIdx.indexOf('[') + 1, subnetIdx.indexOf(']')), + 10 + ); + + // if there already exists some previous error for the subnet at index idx, we + // just add the current error. Otherwise, we create a new entry for the subnet. + if (subnetErrors[idx]) { + subnetErrors[idx] = { ...subnetErrors[idx], [field]: error.reason }; + } else { + subnetErrors[idx] = { [field]: error.reason }; + } + } else { + nonSubnetErrors.push(error); + } + }); + + handleAPIErrors(nonSubnetErrors, setFieldError, setError); + return subnetErrors; +}; diff --git a/packages/manager/src/utilities/subnets.test.ts b/packages/manager/src/utilities/subnets.test.ts index 56e1adfc63c..0a9b3eb9ad4 100644 --- a/packages/manager/src/utilities/subnets.test.ts +++ b/packages/manager/src/utilities/subnets.test.ts @@ -1,9 +1,4 @@ -import { - DEFAULT_IPV4_ERROR, - DEFAULT_LABEL_ERROR, - calculateAvailableIPv4s, - validateSubnets, -} from './subnets'; +import { calculateAvailableIPv4s } from './subnets'; describe('calculateAvailableIPv4s', () => { it('should return a number if the input is a valid IPv4 with a mask', () => { @@ -33,105 +28,3 @@ describe('calculateAvailableIPv4s', () => { expect(badMask2).toBe(undefined); }); }); - -describe('validateSubnets', () => { - const emptyIP = { - ip: { ipv4: '', ipv4Error: '' }, - label: 'empty ip', - labelError: '', - }; - - const badIP = { - ip: { ipv4: 'bad ip', ipv4Error: '' }, - label: '', - labelError: '', - }; - - const missingMask = { - ip: { ipv4: '10.0.0.0', ipv4Error: '' }, - label: 'needs a mask', - labelError: '', - }; - - const missingLabel = { - ip: { ipv4: '10.0.0.0/24', ipv4Error: '' }, - label: '', - labelError: '', - }; - - const goodSubnets = [ - { - ip: { ipv4: '', ipv4Error: '' }, - label: '', - labelError: '', - }, - { - ip: { ipv4: '10.0.0.0/24', ipv4Error: '' }, - label: 'good subnet', - labelError: '', - }, - ]; - - it('should use the error messages passed in and error for ip and label', () => { - const erroredSubnets = validateSubnets([badIP], { - ipv4Error: 'this is a bad ip', - labelError: 'this is a bad label', - }); - - expect(erroredSubnets).toHaveLength(1); - expect(erroredSubnets).toStrictEqual([ - { - ip: { - ipv4: 'bad ip', - ipv4Error: 'this is a bad ip', - }, - label: '', - labelError: 'this is a bad label', - }, - ]); - }); - - it('should return error for IPs', () => { - const erroredSubnets = validateSubnets([missingMask, emptyIP]); - expect(erroredSubnets).toHaveLength(2); - expect(erroredSubnets).toStrictEqual([ - { - ip: { - ipv4: '10.0.0.0', - ipv4Error: DEFAULT_IPV4_ERROR, - }, - label: 'needs a mask', - labelError: '', - }, - { - ip: { - ipv4: '', - ipv4Error: DEFAULT_IPV4_ERROR, - }, - label: 'empty ip', - labelError: '', - }, - ]); - }); - - it('should return error for label', () => { - const erroredSubnets = validateSubnets([missingLabel]); - expect(erroredSubnets).toHaveLength(1); - expect(erroredSubnets).toStrictEqual([ - { - ip: { - ipv4: '10.0.0.0/24', - ipv4Error: '', - }, - label: '', - labelError: DEFAULT_LABEL_ERROR, - }, - ]); - }); - - it('should not return error messages', () => { - const subnets = validateSubnets(goodSubnets); - expect(subnets).toHaveLength(2); - expect(subnets).toStrictEqual(goodSubnets); - }); -}); diff --git a/packages/manager/src/utilities/subnets.ts b/packages/manager/src/utilities/subnets.ts index 836564bad7c..8a6ab64087a 100644 --- a/packages/manager/src/utilities/subnets.ts +++ b/packages/manager/src/utilities/subnets.ts @@ -1,4 +1,4 @@ -import { determineIPType, vpcsValidateIP } from '@linode/validation'; +import { determineIPType } from '@linode/validation'; export const DEFAULT_SUBNET_IPV4_VALUE = '10.0.4.0/24'; export const RESERVED_IP_NUMBER = 4; @@ -74,39 +74,3 @@ export const calculateAvailableIPv4s = ( return SubnetMaskToAvailIPv4s[mask]; }; - -export const DEFAULT_LABEL_ERROR = 'Label is required'; -export const DEFAULT_IPV4_ERROR = 'The IPv4 range must be in CIDR format'; - -// TODO: VPC - add error validation for ipv6 when that becomes supported -export const validateSubnets = ( - subnets: SubnetFieldState[], - options?: { - ipv4Error?: string; - ipv6Error?: string; - labelError?: string; - } -): SubnetFieldState[] => { - return subnets.map((subnet) => { - if (subnet.label || subnet.ip.ipv4) { - const errorSubnet: SubnetFieldState = { ...subnet, labelError: '' }; - if (!subnet.label) { - errorSubnet['labelError'] = options?.labelError ?? DEFAULT_LABEL_ERROR; - } - if ( - determineIPType(subnet.ip.ipv4 ?? '') !== 'ipv4' || - !vpcsValidateIP({ - mustBeIPMask: false, - shouldHaveIPMask: true, - value: subnet.ip.ipv4, - }) - ) { - const errorIP = { ...subnet.ip, ipv4Error: '' }; - errorIP['ipv4Error'] = options?.ipv4Error ?? DEFAULT_IPV4_ERROR; - errorSubnet['ip'] = errorIP; - } - return errorSubnet; - } - return subnet; - }); -};