Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
199 changes: 199 additions & 0 deletions app/client/src/widgets/JSONFormWidget/component/Form.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,199 @@
import "@testing-library/jest-dom";
import { fireEvent, render, screen } from "@testing-library/react";
import { klona } from "klona";
import React from "react";
import { useForm } from "react-hook-form";
import { ROOT_SCHEMA_KEY, type SchemaItem } from "../constants";
import Form from "./Form";
import useFixedFooter from "./useFixedFooter";

jest.mock("react-hook-form", () => ({
__esModule: true,
useForm: jest.fn(),
FormProvider: jest.fn(({ children }) => <div>{children}</div>),
}));

jest.mock("./useFixedFooter", () => ({
__esModule: true,
default: jest.fn(),
}));

jest.mock("widgets/ButtonWidget/component", () => ({
__esModule: true,
BaseButton: jest.fn(({ children, ...props }) => (
<button {...props}>{children}</button>
)),
}));
Comment on lines +10 to +26

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider consolidating mock implementations.

The mock setup for multiple components could be moved to a separate test utils file for better reusability across test suites.

+ // Move to __mocks__/testUtils.ts
+ export const mockComponents = () => {
+   jest.mock("react-hook-form", () => ({
+     __esModule: true,
+     useForm: jest.fn(),
+     FormProvider: jest.fn(({ children }) => <div>{children}</div>),
+   }));
+   
+   // ... other mocks
+ };

Committable suggestion was skipped due to low confidence.


function mockUseForm(withErrors = false) {
(useForm as jest.Mock).mockReturnValue({
formState: {
errors: withErrors
? {
fieldName: {
type: "required",
message: "This field is required",
},
}
: null,
},
reset: jest.fn(),
trigger: jest.fn(),
watch: jest.fn(() => ({
subscribe: jest.fn(),
unsubscribe: jest.fn(),
})),
});
}
Comment on lines +28 to +47

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance mockUseForm to support configurable trigger behavior.

The mockUseForm function should allow configuring the trigger function's return value to properly test validation scenarios.

 function mockUseForm(withErrors = false) {
+  const mockTrigger = jest.fn().mockResolvedValue(!withErrors);
   (useForm as jest.Mock).mockReturnValue({
     formState: {
       errors: withErrors
         ? {
             fieldName: {
               type: "required",
               message: "This field is required",
             },
           }
         : null,
     },
     reset: jest.fn(),
-    trigger: jest.fn(),
+    trigger: mockTrigger,
     watch: jest.fn(() => ({
       subscribe: jest.fn(),
       unsubscribe: jest.fn(),
     })),
   });
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
function mockUseForm(withErrors = false) {
(useForm as jest.Mock).mockReturnValue({
formState: {
errors: withErrors
? {
fieldName: {
type: "required",
message: "This field is required",
},
}
: null,
},
reset: jest.fn(),
trigger: jest.fn(),
watch: jest.fn(() => ({
subscribe: jest.fn(),
unsubscribe: jest.fn(),
})),
});
}
function mockUseForm(withErrors = false) {
const mockTrigger = jest.fn().mockResolvedValue(!withErrors);
(useForm as jest.Mock).mockReturnValue({
formState: {
errors: withErrors
? {
fieldName: {
type: "required",
message: "This field is required",
},
}
: null,
},
reset: jest.fn(),
trigger: mockTrigger,
watch: jest.fn(() => ({
subscribe: jest.fn(),
unsubscribe: jest.fn(),
})),
});
}


const TEST_IDS = {
SUBMIT_BTN: "t--jsonform-submit-btn",
RESET_BTN: "t--jsonform-reset-btn",
};

const defaultProps = {
backgroundColor: "#fff",
disabledWhenInvalid: false,
fixedFooter: false,
getFormData: jest.fn(),
hideFooter: false,
isSubmitting: false,
isWidgetMounting: false,
onFormValidityUpdate: jest.fn(),
onSubmit: jest.fn(),
onreset: jest.fn(),
registerResetObserver: jest.fn(),
resetButtonLabel: "Reset",
resetButtonStyles: {},
schema: {},
scrollContents: false,
showReset: true,
stretchBodyVertically: false,
submitButtonLabel: "Submit",
submitButtonStyles: {},
title: "Test Form",
unregisterResetObserver: jest.fn(),
updateFormData: jest.fn(),
children: <input />,
};

describe("Form Component", () => {
beforeEach(() => {
const mockBodyRef = React.createRef<HTMLFormElement>();
const mockFooterRef = React.createRef<HTMLDivElement>();

(useFixedFooter as jest.Mock).mockReturnValue({
bodyRef: mockBodyRef,
footerRef: mockFooterRef,
});
});

describe("happy render path", () => {
beforeEach(() => mockUseForm());
it("renders the form title", () => {
render(<Form {...defaultProps}>Form Content</Form>);
expect(screen.getByText("Test Form")).toBeInTheDocument();
});

it("renders children inside the form body", () => {
render(<Form {...defaultProps}>Form Content</Form>);
expect(screen.getByText("Form Content")).toBeInTheDocument();
});

it("renders the submit button with correct label", () => {
const { getByTestId } = render(
<Form {...defaultProps}>Form Content</Form>,
);
const submitBtn = getByTestId(TEST_IDS.SUBMIT_BTN);

expect(submitBtn).toBeInTheDocument();
expect(submitBtn).toHaveAttribute("text", defaultProps.submitButtonLabel);

fireEvent.click(submitBtn);
expect(defaultProps.onSubmit).toHaveBeenCalled();
});

it("renders the reset button with correct label when showReset is true", () => {
const { getByTestId } = render(
<Form {...defaultProps}>Form Content</Form>,
);
const resetBtn = getByTestId(TEST_IDS.RESET_BTN);

expect(resetBtn).toBeInTheDocument();
expect(resetBtn).toHaveAttribute("text", defaultProps.resetButtonLabel);
expect(defaultProps.registerResetObserver).toHaveBeenCalled();
});

// Form updates data correctly when input values change
it("should update data when input values change", () => {
const mockUpdateFormData = jest.fn();
const mockGetFormData = jest.fn().mockReturnValue({});
const mockRegisterResetObserver = jest.fn();
const mockUnregisterResetObserver = jest.fn();
const mockOnSubmit = jest.fn();
const mockOnFormValidityUpdate = jest.fn();
const mockSchema = { [ROOT_SCHEMA_KEY]: {} as SchemaItem };
const props = klona({
...defaultProps,
updateFormData: mockUpdateFormData,
getFormData: mockGetFormData,
registerResetObserver: mockRegisterResetObserver,
unregisterResetObserver: mockUnregisterResetObserver,
onSubmit: mockOnSubmit,
onFormValidityUpdate: mockOnFormValidityUpdate,
schema: mockSchema,
});
const { getByTestId } = render(<Form {...props} />);

fireEvent.click(getByTestId(TEST_IDS.SUBMIT_BTN));
expect(mockUpdateFormData).toHaveBeenCalled();
});
Comment on lines +127 to +149

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Test implementation doesn't match its description.

The test claims to verify input value changes but only tests form submission. Either rename the test or implement proper input change verification.

-it("should update data when input values change", () => {
+it("should update form data on submit", () => {
   const mockUpdateFormData = jest.fn();
   const mockGetFormData = jest.fn().mockReturnValue({});
   const mockRegisterResetObserver = jest.fn();
   const mockUnregisterResetObserver = jest.fn();
   const mockOnSubmit = jest.fn();
   const mockOnFormValidityUpdate = jest.fn();
   const mockSchema = { [ROOT_SCHEMA_KEY]: {} as SchemaItem };
   const props = klona({
     ...defaultProps,
     updateFormData: mockUpdateFormData,
     getFormData: mockGetFormData,
     registerResetObserver: mockRegisterResetObserver,
     unregisterResetObserver: mockUnregisterResetObserver,
     onSubmit: mockOnSubmit,
     onFormValidityUpdate: mockOnFormValidityUpdate,
     schema: mockSchema,
   });
   const { getByTestId } = render(<Form {...props} />);

   fireEvent.click(getByTestId(TEST_IDS.SUBMIT_BTN));
   expect(mockUpdateFormData).toHaveBeenCalled();
 });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it("should update data when input values change", () => {
const mockUpdateFormData = jest.fn();
const mockGetFormData = jest.fn().mockReturnValue({});
const mockRegisterResetObserver = jest.fn();
const mockUnregisterResetObserver = jest.fn();
const mockOnSubmit = jest.fn();
const mockOnFormValidityUpdate = jest.fn();
const mockSchema = { [ROOT_SCHEMA_KEY]: {} as SchemaItem };
const props = klona({
...defaultProps,
updateFormData: mockUpdateFormData,
getFormData: mockGetFormData,
registerResetObserver: mockRegisterResetObserver,
unregisterResetObserver: mockUnregisterResetObserver,
onSubmit: mockOnSubmit,
onFormValidityUpdate: mockOnFormValidityUpdate,
schema: mockSchema,
});
const { getByTestId } = render(<Form {...props} />);
fireEvent.click(getByTestId(TEST_IDS.SUBMIT_BTN));
expect(mockUpdateFormData).toHaveBeenCalled();
});
it("should update form data on submit", () => {
const mockUpdateFormData = jest.fn();
const mockGetFormData = jest.fn().mockReturnValue({});
const mockRegisterResetObserver = jest.fn();
const mockUnregisterResetObserver = jest.fn();
const mockOnSubmit = jest.fn();
const mockOnFormValidityUpdate = jest.fn();
const mockSchema = { [ROOT_SCHEMA_KEY]: {} as SchemaItem };
const props = klona({
...defaultProps,
updateFormData: mockUpdateFormData,
getFormData: mockGetFormData,
registerResetObserver: mockRegisterResetObserver,
unregisterResetObserver: mockUnregisterResetObserver,
onSubmit: mockOnSubmit,
onFormValidityUpdate: mockOnFormValidityUpdate,
schema: mockSchema,
});
const { getByTestId } = render(<Form {...props} />);
fireEvent.click(getByTestId(TEST_IDS.SUBMIT_BTN));
expect(mockUpdateFormData).toHaveBeenCalled();
});

});
Comment on lines +90 to +150

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add missing test coverage for form validation triggers.

The "happy render path" suite is missing tests for form validation triggers on child component updates.

Add the following test:

it("should trigger validation on child component updates", async () => {
  const mockTrigger = jest.fn().mockResolvedValue(true);
  (useForm as jest.Mock).mockReturnValue({
    ...defaultMockFormValues,
    trigger: mockTrigger,
    formState: { errors: {} },
  });

  const { rerender } = render(
    <Form {...defaultProps}>
      <input value="initial" />
    </Form>
  );

  rerender(
    <Form {...defaultProps}>
      <input value="updated" />
    </Form>
  );

  expect(mockTrigger).toHaveBeenCalled();
});


describe("unhappy render path", () => {
it("does not render the reset button when showReset is false", () => {
mockUseForm();
const { queryByTestId } = render(
<Form {...defaultProps} showReset={false}>
Form Content
</Form>,
);

expect(queryByTestId(TEST_IDS.RESET_BTN)).not.toBeInTheDocument();
});

it("disables the submit button when disabledWhenInvalid is true and form is invalid", () => {
mockUseForm(true);
const { getByTestId } = render(
<Form {...defaultProps} disabledWhenInvalid>
Form Content
</Form>,
);

expect(getByTestId(TEST_IDS.SUBMIT_BTN)).toBeDisabled();
});

it("triggers a check, necessitating a re-render, when the children in form are updated", () => {
mockUseForm(true);
const propsToUpdate = klona({
...defaultProps,
disabledWhenInvalid: true,
});
const { getByTestId } = render(
<Form {...propsToUpdate}>Form Content</Form>,
);

expect(getByTestId(TEST_IDS.SUBMIT_BTN)).toBeDisabled();
});
Comment on lines +175 to +186

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve test assertions for child component updates.

The test lacks proper assertions to verify the trigger behavior when children are updated.

-it("triggers a check, necessitating a re-render, when the children in form are updated", () => {
+it("should trigger validation when children are updated", () => {
   mockUseForm(true);
   const propsToUpdate = klona({
     ...defaultProps,
     disabledWhenInvalid: true,
   });
-  const { getByTestId } = render(
+  const { getByTestId, rerender } = render(
     <Form {...propsToUpdate}>Form Content</Form>,
   );
+  
+  rerender(<Form {...propsToUpdate}>Updated Content</Form>);
+  
+  const mockTrigger = (useForm as jest.Mock).mock.results[0].value.trigger;
+  expect(mockTrigger).toHaveBeenCalled();
   expect(getByTestId(TEST_IDS.SUBMIT_BTN)).toBeDisabled();
 });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it("triggers a check, necessitating a re-render, when the children in form are updated", () => {
mockUseForm(true);
const propsToUpdate = klona({
...defaultProps,
disabledWhenInvalid: true,
});
const { getByTestId } = render(
<Form {...propsToUpdate}>Form Content</Form>,
);
expect(getByTestId(TEST_IDS.SUBMIT_BTN)).toBeDisabled();
});
it("should trigger validation when children are updated", () => {
mockUseForm(true);
const propsToUpdate = klona({
...defaultProps,
disabledWhenInvalid: true,
});
const { getByTestId, rerender } = render(
<Form {...propsToUpdate}>Form Content</Form>,
);
rerender(<Form {...propsToUpdate}>Updated Content</Form>);
const mockTrigger = (useForm as jest.Mock).mock.results[0].value.trigger;
expect(mockTrigger).toHaveBeenCalled();
expect(getByTestId(TEST_IDS.SUBMIT_BTN)).toBeDisabled();
});


it("should handle empty schema gracefully without errors", () => {
mockUseForm();
const mockUpdateFormData = jest.fn();
const props = klona({ ...defaultProps, schema: undefined });
const { container } = render(<Form {...props} />);

expect(container).toBeInTheDocument();
expect(mockUpdateFormData).not.toHaveBeenCalled();
});
});
});
10 changes: 9 additions & 1 deletion app/client/src/widgets/JSONFormWidget/component/Form.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ function Form<TValues = any>(
) {
const valuesRef = useRef({});
const methods = useForm();
const { formState, reset, watch } = methods;
const { formState, reset, trigger, watch } = methods;
const { errors } = formState;
const isFormInValid = !isEmpty(errors);

Expand Down Expand Up @@ -273,6 +273,12 @@ function Form<TValues = any>(
onFormValidityUpdate(!isFormInValid);
}, [onFormValidityUpdate, isFormInValid]);

useEffect(() => {
// This effect is to handle an edge-case:
// https://github.com/appsmithorg/appsmith/issues/28018
trigger();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't quite understood the solution here, why are we re-validating the whole form when children changes? Isn't it a problem of when reset button is pressed the validation persists?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashit-rath I don't think so, the problem is the validation not getting triggered when children changes. When reset button is pressed, then the validation change is triggered.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashit-rath
Let me take you with a dry run of whats happening in the original issue #28018

  1. User select red and get one input which bound to red. It is empty hence, errors has {red: required:true}.
  2. user adds value, errors are cleared. and user can submit.
  3. Now they click reset, the input is cleared and errorState has this: {red: required:true}
  4. Now user switches to blue, and gets input bound to blue, which adds {blue: required:true}
  5. Over all errorState looks liks: {red: required:true, blue: required:true}
  6. Now even if you type in blue input, the red error never goes away.

I am open to ideas if this call to trigger can be optimized. Looking forward to your ideas.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rahulbarwal and @jacquesikot for explaining this in detail, I understood the problem. I believe this is a reset problem; if you look at the RESET_OPTIONS which gets passed to onReset has keepErrors: true. Have we checked if this was set to false for the case when button is clicked; solves the problem?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ashit-rath for this input.
keepErrors: false did not work, and I think I know why:
In step 3 above, when user resets the form, the red input is visible and Here, whether we keep or discard errors does not matter.
IT is on step 4 that multiple errors enter the formState(where we are not doing the reset).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rahulbarwal Understood! Can we profile and understand the performance implication of having children as a dependency of an effect? esp. with Array data types in jsonform where the form size increases with user interaction?
Also this method trigger accepts a field name to re-trigger validation for it. So when the validation of red changes in step 4, basically we need to re-validate. I'm assuming the validation it gets from the required property is correctly set and we just need to trigger for re-validation. So there is a hook called useRegisterFieldValidity where we deal with field level error state setting/unsetting; can we use this hook to actually call trigger()?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ashit-rath, for this feedback, I've incorporated these changes. please review again.

}, [children, trigger]);

Comment thread
rahulbarwal marked this conversation as resolved.
Outdated
return (
<FormProvider {...methods}>
<StyledForm
Expand All @@ -299,6 +305,7 @@ function Form<TValues = any>(
<Button
{...resetButtonStyles}
className="t--jsonform-reset-btn"
data-testid="t--jsonform-reset-btn"
onClick={(e) => onReset(schema, e)}
text={resetButtonLabel}
type="reset"
Expand All @@ -308,6 +315,7 @@ function Form<TValues = any>(
<Button
{...submitButtonStyles}
className="t--jsonform-submit-btn"
data-testid="t--jsonform-submit-btn"
disabled={disabledWhenInvalid && isFormInValid}
loading={isSubmitting}
onClick={onSubmit}
Expand Down