fix(tgnms-v2) Fix Policing Classification form for nodes

Summary: So this feature was straight up broken. As seen in the BEFORE video below: 1. Not able to see the Simple form values (because the code never populated the value for the input fields) 2. Not able to edit in the Custom view since the field properties were wrong. The form layout itself is wrong, theres no way for users to edit each traffic class. Also the input is broken, i can't type anything in there. # Solution So it seems as though these forms were never designed to support multiple views for a form field (i.e. simple and custom). For one, we never saved which view the user chose, so it always defaults to Simple view. After talking with Nathan, we decided to just get rid of the simple view (since to do it right it would likely require large architectural changes) and just have the Custom view by default. Reviewed By: aclave1 Differential Revision: D33852969 fbshipit-source-id: 4b37d4c442d506e3b66247aee1155e609e14aca9
terragraph · Jan 31, 2022 · 02118f9 · 02118f9
1 parent 86f2104
commit 02118f9
Show file tree

Hide file tree

Showing 4 changed files with 80 additions and 78 deletions.
diff --git a/tgnms/fbcnms-projects/tgnms/app/components/taskBasedConfig/ConfigTaskMapInput.js b/tgnms/fbcnms-projects/tgnms/app/components/taskBasedConfig/ConfigTaskMapInput.js
@@ -30,14 +30,23 @@ const useStyles = makeStyles(theme => ({
 
 export type Props = {|
   configField: string,
+  // In the event the metadata is a different path than the configField.
+  metadataField?: string,
   label?: string,
   customKeyLabel?: string,
   buttonText?: string,
   onChange?: ({}) => void,
 |};
 
 export default function ConfigTaskMapInput(props: Props) {
-  const {configField, label, buttonText, onChange, customKeyLabel} = props;
+  const {
+    configField,
+    metadataField,
+    label,
+    buttonText,
+    onChange,
+    customKeyLabel,
+  } = props;
   const classes = useStyles();
   const {configData, configMetadata, onUpdate} = useConfigTaskContext();
   const [configValue, setConfigValue] = React.useState<{
@@ -49,7 +58,7 @@ export default function ConfigTaskMapInput(props: Props) {
     configField,
   ]);
 
-  const metadata = configFieldArray.reduce(
+  const metadata = (metadataField?.split('.') ?? configFieldArray).reduce(
     (result, key) => (result ? result[key] : result),
     configMetadata,
   );

diff --git a/tgnms/fbcnms-projects/tgnms/app/components/taskBasedConfig/configTasks/QoSInterfaceConfig.js b/tgnms/fbcnms-projects/tgnms/app/components/taskBasedConfig/configTasks/QoSInterfaceConfig.js
@@ -6,14 +6,12 @@
  */
 
 import * as React from 'react';
-import ConfigOptionSelector from '../ConfigOptionSelector';
-import ConfigTaskInput from '../ConfigTaskInput';
+import ConfigTaskGroup from '../ConfigTaskGroup';
 import ConfigTaskMapInput from '../ConfigTaskMapInput';
-import useForm from '@fbcnms/tg-nms/app/hooks/useForm';
 import {useConfigTaskContext} from '@fbcnms/tg-nms/app/contexts/ConfigTaskContext';
 
-const SIMPLE_TC = ['0', '1', '2', '3'];
 const POLICING_CONFIG_FIELD = 'cpeParams.policers';
+const METADATA_FIELD = 'cpeConfig.mapVal.objVal.properties.policers';
 
 export default function QoSInterfaceConfig({
   cpeInterface,
@@ -22,64 +20,24 @@ export default function QoSInterfaceConfig({
 }) {
   const {onUpdate} = useConfigTaskContext();
   const onUpdateRef = React.useRef(onUpdate);
-
   const handlePolicingConfigChange = React.useCallback(
     change => {
       const configField = `${POLICING_CONFIG_FIELD}.${cpeInterface}`;
       onUpdateRef.current({configField, draftValue: change});
     },
     [onUpdateRef, cpeInterface],
   );
-
-  const {updateFormState} = useForm({
-    initialState: {cir: 0, eir: 0},
-    onFormUpdated: state => {
-      const formattedChange = SIMPLE_TC.reduce((res, tc) => {
-        res[tc] = state;
-        return res;
-      }, {});
-      handlePolicingConfigChange(formattedChange);
-    },
-  });
-
-  const handleSimpleChange = (key: string, value) => {
-    updateFormState({[key]: value});
-  };
-
   return (
-    <ConfigOptionSelector
+    <ConfigTaskGroup
       title={`Policing Classification for ${cpeInterface}`}
-      options={{
-        simple: {
-          name: 'Simple',
-          description:
-            'All Traffic Classes will have the same CIR (critical information rate) and EIR (excess information rate) values',
-          configGroup: (
-            <>
-              <ConfigTaskInput
-                label="CIR"
-                onChange={val => handleSimpleChange('cir', Number(val))}
-              />
-              <ConfigTaskInput
-                label="EIR"
-                onChange={val => handleSimpleChange('eir', Number(val))}
-              />
-            </>
-          ),
-        },
-        custom: {
-          name: 'Custom',
-          description: 'Configure custom CIR and EIR for each Traffic Class',
-          configGroup: (
-            <ConfigTaskMapInput
-              configField="cpeParams.policers"
-              buttonText="Add Forwarding Class"
-              customKeyLabel="Traffic Class"
-              onChange={handlePolicingConfigChange}
-            />
-          ),
-        },
-      }}
-    />
+      description={'Configure custom CIR and EIR for each Traffic Class'}>
+      <ConfigTaskMapInput
+        metadataField={METADATA_FIELD}
+        configField={`${POLICING_CONFIG_FIELD}.${cpeInterface}`}
+        buttonText="Add Forwarding Class"
+        customKeyLabel="Traffic Class"
+        onChange={handlePolicingConfigChange}
+      />
+    </ConfigTaskGroup>
   );
 }
diff --git a/...cts/tgnms/app/components/taskBasedConfig/configTasks/__tests__/QoSInterfaceConfig-test.js b/...cts/tgnms/app/components/taskBasedConfig/configTasks/__tests__/QoSInterfaceConfig-test.js
@@ -11,45 +11,82 @@ import {TestApp} from '@fbcnms/tg-nms/app/tests/testHelpers';
 import {fireEvent, render} from '@testing-library/react';
 import {mockConfigTaskContextValue} from '@fbcnms/tg-nms/app/tests/data/NetworkConfig';
 
+const onUpdateMock = jest.fn();
 jest
   .spyOn(
     require('@fbcnms/tg-nms/app/contexts/ConfigTaskContext'),
     'useConfigTaskContext',
   )
-  .mockReturnValue(mockConfigTaskContextValue());
+  .mockReturnValue(
+    mockConfigTaskContextValue({
+      onUpdate: onUpdateMock,
+      configData: [
+        {
+          field: ['cpeParams', 'policers', 'test_cpe', '0', 'cir'],
+          hasOverride: false,
+          hasTopLevelOverride: false,
+          layers: [{id: 'Base value', value: 100}],
+          metadata: {action: 'RESTART_SQUIRE', desc: 'test', type: 'STRING'},
+        },
+      ],
+      configMetadata: {
+        cpeConfig: {
+          mapVal: {
+            objVal: {
+              properties: {
+                policers: {
+                  mapVal: {objVal: {properties: {cir: {type: 'INT'}}}},
+                },
+              },
+            },
+          },
+        },
+      },
+    }),
+  );
 
 const defaultProps = {
-  cpeInterface: 'test',
+  cpeInterface: 'test_cpe',
 };
 
-test('renders', async () => {
-  const {getByText} = render(
-    <TestApp>
-      <QoSInterfaceConfig {...defaultProps} />
-    </TestApp>,
-  );
-  expect(getByText('Policing Classification for test')).toBeInTheDocument();
+beforeEach(() => {
+  onUpdateMock.mockReset();
 });
 
-test('default renders on simple format', async () => {
-  const {getByText} = render(
+test('modifications trigger an update', async () => {
+  const {getByText, getByDisplayValue} = render(
     <TestApp>
       <QoSInterfaceConfig {...defaultProps} />
     </TestApp>,
   );
+  expect(getByText('Policing Classification for test_cpe')).toBeInTheDocument();
   expect(
-    getByText(/All Traffic Classes will have the same CIR/i),
+    getByText(/Configure custom CIR and EIR for each Traffic Class/i),
   ).toBeInTheDocument();
+
+  const input = getByDisplayValue(100);
+  fireEvent.change(input, {target: {value: 200}});
+  expect(onUpdateMock).toHaveBeenCalledWith({
+    configField: 'cpeParams.policers.test_cpe',
+    draftValue: {'0': {cir: 200}},
+  });
 });
 
-test('clicking Custom switches to custom format', async () => {
+test('deletion triggers an update', async () => {
   const {getByText} = render(
     <TestApp>
       <QoSInterfaceConfig {...defaultProps} />
     </TestApp>,
   );
-  fireEvent.click(getByText('Custom'));
+  expect(getByText('Policing Classification for test_cpe')).toBeInTheDocument();
   expect(
     getByText(/Configure custom CIR and EIR for each Traffic Class/i),
   ).toBeInTheDocument();
+
+  const btn = getByText('Delete');
+  fireEvent.click(btn);
+  expect(onUpdateMock).toHaveBeenCalledWith({
+    configField: 'cpeParams.policers.test_cpe',
+    draftValue: {},
+  });
 });
diff --git a/tgnms/fbcnms-projects/tgnms/app/helpers/ConfigHelpers.js b/tgnms/fbcnms-projects/tgnms/app/helpers/ConfigHelpers.js
@@ -600,14 +600,12 @@ export function getDraftConfig<T>({
       if (draftConfigValue === '' || draftConfigValue === null) {
         unset(currentDraftConfig, configField.split('.'));
       } else if (typeof draftConfigValue === 'object') {
-        Object.keys(draftConfigValue).forEach(key => {
-          setWith(
-            currentDraftConfig,
-            [...configField.split('.'), key],
-            draftConfigValue[key],
-            Object,
-          );
-        });
+        setWith(
+          currentDraftConfig,
+          configField.split('.'),
+          draftConfigValue,
+          Object,
+        );
       } else {
         set(currentDraftConfig, configField.split('.'), draftConfigValue);
       }