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>
linode · Sep 13, 2023 · c2cf937 · c2cf937
1 parent 5729b30
commit c2cf937
Show file tree

Hide file tree

Showing 7 changed files with 249 additions and 209 deletions.
diff --git a/packages/manager/.changeset/pr-9659-changed-1694440105022.md 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
@@ -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(
-      <VPCCreate />
-    );
+    const {
+      getAllByTestId,
+      getByText,
+      getAllByText,
+      queryByText,
+    } = renderWithTheme(<VPCCreate />);
     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
@@ -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<APIError[]>();
   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.
               <Link to="#"> Learn more</Link>.
-              {/* TODO: VPC - subnet learn more link here */}
+              {/* @TODO VPC: subnet learn more link here */}
             </StyledBodyTypography>
-            {subnetErrorsFromAPI
-              ? subnetErrorsFromAPI.map((apiError: APIError) => (
+            {generalSubnetErrorsFromAPI
+              ? generalSubnetErrorsFromAPI.map((apiError: APIError) => (
                   <Notice
                     key={apiError.reason}
                     spacingBottom={8}

diff --git a/packages/manager/src/utilities/formikErrorUtils.test.ts b/packages/manager/src/utilities/formikErrorUtils.test.ts
@@ -1,10 +1,14 @@
-import { handleAPIErrors } from './formikErrorUtils';
+import { handleAPIErrors, handleVPCAndSubnetErrors } from './formikErrorUtils';
 
 const errorWithoutField = [{ reason: 'Internal server error' }];
 const errorWithField = [
   { field: 'data.card_number', reason: 'Invalid credit card number' },
 ];
 
+afterEach(() => {
+  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();
+  });
+});