From 76d345d5508848885f1057c3f3664331e6b0d1c3 Mon Sep 17 00:00:00 2001
From: Peter Kulko <93188219+pkulkoraccoongang@users.noreply.github.com>
Date: Fri, 13 Oct 2023 11:01:50 +0300
Subject: [PATCH] fix: avoid duplicate onChange calls in FormRadioSet and
 FormCheckboxSet (#2705)

* fix: fixed duplicate calls in FormRadioSet and FormCheckboxSet

* refactor: added new tests

* refactor: refactoring after review

(cherry picked from commit a395042548cbc66c81ae43269414afb040df8f3f)
---
 package.json                              |  2 +-
 src/Form/FormCheckbox.jsx                 | 20 ++++---
 src/Form/FormRadio.jsx                    | 63 +++++++++++------------
 src/Form/tests/FormCheckboxSet.test.jsx   | 19 +++++++
 src/Form/tests/FormRadioSet.test.jsx      | 19 +++++++
 src/Menu/__snapshots__/Menu.test.jsx.snap |  1 +
 6 files changed, 83 insertions(+), 41 deletions(-)

diff --git a/package.json b/package.json
index 7524637f55e..e9411aa66a3 100644
--- a/package.json
+++ b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "@edx/paragon",
-  "version": "20.18.1",
+  "version": "20.46.3",
   "description": "Accessible, responsive UI component library based on Bootstrap.",
   "main": "dist/index.js",
   "module": "dist/index.js",
diff --git a/src/Form/FormCheckbox.jsx b/src/Form/FormCheckbox.jsx
index 9a9b736b899..d2e1951ba52 100644
--- a/src/Form/FormCheckbox.jsx
+++ b/src/Form/FormCheckbox.jsx
@@ -62,7 +62,7 @@ const FormCheckbox = React.forwardRef(({
   floatLabelLeft,
   ...props
 }, ref) => {
-  const { getCheckboxControlProps, hasCheckboxSetProvider } = useCheckboxSetContext();
+  const { hasCheckboxSetProvider } = useCheckboxSetContext();
   const { hasFormGroupProvider, useSetIsControlGroupEffect, getControlProps } = useFormGroupContext();
   useSetIsControlGroupEffect(true);
   const shouldActAsGroup = hasFormGroupProvider && !hasCheckboxSetProvider;
@@ -70,14 +70,11 @@ const FormCheckbox = React.forwardRef(({
     ...getControlProps({}),
     role: 'group',
   } : {};
-  const checkboxInputProps = getCheckboxControlProps({
-    ...props,
-    className: controlClassName,
-  });
-  const control = React.createElement(controlAs, { ...checkboxInputProps, ref });
+
+  const control = React.createElement(controlAs, { ...props, className: controlClassName, ref });
   return (
     <FormGroupContextProvider
-      controlId={checkboxInputProps.id}
+      controlId={props.id}
       isInvalid={isInvalid}
       isValid={isValid}
     >
@@ -85,7 +82,7 @@ const FormCheckbox = React.forwardRef(({
         className={classNames('pgn__form-checkbox', className, {
           'pgn__form-control-valid': isValid,
           'pgn__form-control-invalid': isInvalid,
-          'pgn__form-control-disabled': checkboxInputProps.disabled,
+          'pgn__form-control-disabled': props.disabled,
           'pgn__form-control-label-left': !!floatLabelLeft,
         })}
         {...groupProps}
@@ -107,6 +104,8 @@ const FormCheckbox = React.forwardRef(({
 });
 
 FormCheckbox.propTypes = {
+  /** Specifies id of the FormCheckbox component. */
+  id: PropTypes.string,
   /** Specifies contents of the component. */
   children: PropTypes.node.isRequired,
   /** Specifies class name to append to the base element. */
@@ -123,10 +122,14 @@ FormCheckbox.propTypes = {
   isValid: PropTypes.bool,
   /** Specifies control element. */
   controlAs: PropTypes.elementType,
+  /** Specifies whether the floating label should be aligned to the left. */
   floatLabelLeft: PropTypes.bool,
+  /** Specifies whether the `FormCheckbox` is disabled. */
+  disabled: PropTypes.bool,
 };
 
 FormCheckbox.defaultProps = {
+  id: undefined,
   className: undefined,
   controlClassName: undefined,
   labelClassName: undefined,
@@ -135,6 +138,7 @@ FormCheckbox.defaultProps = {
   isValid: false,
   controlAs: CheckboxControl,
   floatLabelLeft: false,
+  disabled: false,
 };
 
 export { CheckboxControl };
diff --git a/src/Form/FormRadio.jsx b/src/Form/FormRadio.jsx
index 9840f6ab530..4021e408d54 100644
--- a/src/Form/FormRadio.jsx
+++ b/src/Form/FormRadio.jsx
@@ -40,42 +40,37 @@ const FormRadio = React.forwardRef(({
   isInvalid,
   isValid,
   ...props
-}, ref) => {
-  const { getRadioControlProps } = useRadioSetContext();
-  const radioInputProps = getRadioControlProps({
-    ...props,
-    className: controlClassName,
-  });
-  return (
-    <FormGroupContextProvider
-      controlId={radioInputProps.id}
-      isInvalid={isInvalid}
-      isValid={isValid}
+}, ref) => (
+  <FormGroupContextProvider
+    controlId={props.id}
+    isInvalid={isInvalid}
+    isValid={isValid}
+  >
+    <div
+      className={classNames('pgn__form-radio', className, {
+        'pgn__form-control-valid': isValid,
+        'pgn__form-control-invalid': isInvalid,
+        'pgn__form-control-disabled': props.disabled,
+      })}
     >
-      <div
-        className={classNames('pgn__form-radio', className, {
-          'pgn__form-control-valid': isValid,
-          'pgn__form-control-invalid': isInvalid,
-          'pgn__form-control-disabled': radioInputProps.disabled,
-        })}
-      >
-        <RadioControl {...radioInputProps} ref={ref} />
-        <div>
-          <FormLabel className={labelClassName}>
-            {children}
-          </FormLabel>
-          {description && (
-            <FormControlFeedback hasIcon={false}>
-              {description}
-            </FormControlFeedback>
-          )}
-        </div>
+      <RadioControl ref={ref} className={controlClassName} {...props} />
+      <div>
+        <FormLabel className={labelClassName}>
+          {children}
+        </FormLabel>
+        {description && (
+        <FormControlFeedback hasIcon={false}>
+          {description}
+        </FormControlFeedback>
+        )}
       </div>
-    </FormGroupContextProvider>
-  );
-});
+    </div>
+  </FormGroupContextProvider>
+));
 
 FormRadio.propTypes = {
+  /** Specifies id of the FormRadio component. */
+  id: PropTypes.string,
   /** Specifies contents of the component. */
   children: PropTypes.node.isRequired,
   /** Specifies class name to append to the base element. */
@@ -90,15 +85,19 @@ FormRadio.propTypes = {
   isInvalid: PropTypes.bool,
   /** Specifies whether to display component in valid state, this affects styling. */
   isValid: PropTypes.bool,
+  /** Specifies whether the `FormRadio` is disabled. */
+  disabled: PropTypes.bool,
 };
 
 FormRadio.defaultProps = {
+  id: undefined,
   className: undefined,
   controlClassName: undefined,
   labelClassName: undefined,
   description: undefined,
   isInvalid: false,
   isValid: false,
+  disabled: false,
 };
 
 export { RadioControl };
diff --git a/src/Form/tests/FormCheckboxSet.test.jsx b/src/Form/tests/FormCheckboxSet.test.jsx
index 6194ccf2056..4dcba0e9d24 100644
--- a/src/Form/tests/FormCheckboxSet.test.jsx
+++ b/src/Form/tests/FormCheckboxSet.test.jsx
@@ -149,4 +149,23 @@ describe('FormCheckboxSet', () => {
       expect(checkboxNode.props().defaultChecked).toBe(true);
     });
   });
+
+  it('checks if onClick is called once in FormCheckboxSet', () => {
+    const handleChange = jest.fn();
+    const { getByLabelText } = render(
+      <FormGroup controlId="my-field">
+        <FormLabel>Which color?</FormLabel>
+        <FormCheckboxSet
+          name="colors"
+          onChange={handleChange}
+        >
+          <FormCheckbox value="red">Red</FormCheckbox>
+          <FormCheckbox value="green">Green</FormCheckbox>
+        </FormCheckboxSet>
+      </FormGroup>,
+    );
+
+    userEvent.click(getByLabelText('Red'));
+    expect(handleChange).toHaveBeenCalledTimes(1);
+  });
 });
diff --git a/src/Form/tests/FormRadioSet.test.jsx b/src/Form/tests/FormRadioSet.test.jsx
index fce088541b4..2eb86698653 100644
--- a/src/Form/tests/FormRadioSet.test.jsx
+++ b/src/Form/tests/FormRadioSet.test.jsx
@@ -102,4 +102,23 @@ describe('FormRadioSet', () => {
     const radioNode = wrapper.find('input[type="radio"]').first();
     expect(radioNode.props().name).toBe('trees');
   });
+
+  it('checks if onClick is called once in FormRadioSet', () => {
+    const handleChange = jest.fn();
+    const { getByLabelText } = render(
+      <FormGroup>
+        <FormLabel>Which color?</FormLabel>
+        <FormRadioSet
+          name="colors"
+          onChange={handleChange}
+        >
+          <FormRadio value="red">Red</FormRadio>
+          <FormRadio value="green">Green</FormRadio>
+        </FormRadioSet>
+      </FormGroup>,
+    );
+
+    userEvent.click(getByLabelText('Red'));
+    expect(handleChange).toHaveBeenCalledTimes(1);
+  });
 });
diff --git a/src/Menu/__snapshots__/Menu.test.jsx.snap b/src/Menu/__snapshots__/Menu.test.jsx.snap
index ced5b214032..75f4f1b1cf2 100644
--- a/src/Menu/__snapshots__/Menu.test.jsx.snap
+++ b/src/Menu/__snapshots__/Menu.test.jsx.snap
@@ -126,6 +126,7 @@ exports[`Menu Item renders correctly renders as expected with menu items 1`] = `
   >
     <input
       className="pgn__form-checkbox-input"
+      disabled={false}
       id="form-field1"
       type="checkbox"
     />