Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DO NOT MERGE] TESTING EXTERNAL SCRIPT: external merge request from Contributor #36476

Closed
wants to merge 20 commits into from
Closed
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
20ba6f0
chore:remove space b/w form and CTA onboarding page
Shivam-z Aug 29, 2024
f3d043a
chore:added RadioButtonControl unit test case
Shivam-z Sep 5, 2024
230a8a7
fix/removed hardcoded height value and adjusted spacing
Shivam-z Sep 19, 2024
61fcaa2
Update JSONtoForm.tsx
Shivam-z Sep 20, 2024
240943f
Merge remote-tracking branch 'external-Shivam-z/Task-#30523/chore-rem…
AmanAgarwal041 Sep 23, 2024
8dcc55b
Merge branch 'release' of github.com:Shivam-z/appsmith into Task-#305…
Shivam-z Sep 26, 2024
9c0fc9d
Merge branch 'Task-#30523/chore-remove_unnecessary_space_between_form…
AmanAgarwal041 Sep 26, 2024
e5f4bab
chore: revert back the height value for DSEditorWrapper, which was le…
Shivam-z Sep 30, 2024
3f309d6
Merge branch 'Task-#30523/chore-remove_unnecessary_space_between_form…
AmanAgarwal041 Sep 30, 2024
7342984
Merge branch 'release' of github.com:appsmithorg/appsmith into chore/…
AmanAgarwal041 Sep 30, 2024
5039a65
Merge branch 'release' of github.com:Shivam-z/appsmith into Task-#305…
Shivam-z Oct 1, 2024
4e7ef9c
fix:replaced dropdown class with radio group in gsScopeOptions
Shivam-z Oct 1, 2024
4682913
Merge branch 'release' of github.com:appsmithorg/appsmith into chore/…
AmanAgarwal041 Oct 3, 2024
fe24dbd
Merge branch 'release' of github.com:appsmithorg/appsmith into chore/…
AmanAgarwal041 Oct 3, 2024
5106609
Merge branch 'Task-#30523/chore-remove_unnecessary_space_between_form…
AmanAgarwal041 Oct 3, 2024
af42b63
fix:client linting errors
Shivam-z Oct 7, 2024
243f312
Merge branch 'Task-#30523/chore-remove_unnecessary_space_between_form…
AmanAgarwal041 Oct 7, 2024
80d3fc5
Merge branch 'release' of github.com:appsmithorg/appsmith into chore/…
AmanAgarwal041 Oct 7, 2024
2b47472
fix: linting error in radioButtonControl test
Shivam-z Oct 8, 2024
78acdfa
Merge branch 'Task-#30523/chore-remove_unnecessary_space_between_form…
AmanAgarwal041 Oct 8, 2024
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
2 changes: 1 addition & 1 deletion app/client/cypress/support/Pages/DataSources.ts
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ export class DataSources {
_globalSearchInput = ".t--global-search-input";
_gsScopeDropdown =
"[data-testid^='datasourceStorages.'][data-testid$='.datasourceConfiguration.authentication.scopeString']";
_gsScopeOptions = ".ads-v2-select__dropdown .rc-select-item-option";
_gsScopeOptions = ".ads-v2-radio-group";
_queryTimeout = "//input[@name='actionConfiguration.timeoutInMillisecond']";
_getStructureReq = "/api/v1/datasources/*/structure?ignoreCache=true";
_editDatasourceFromActiveTab = (dsName: string) =>
Expand Down
110 changes: 110 additions & 0 deletions app/client/src/components/formControls/RadioButtonControl.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
import React from "react";
import RadioButtonControl from "./RadioButtonControl";
import { render, waitFor, screen } from "test/testUtils";
import { Provider } from "react-redux";
import { reduxForm } from "redux-form";
import configureStore from "redux-mock-store";
import store from "store";
import "@testing-library/jest-dom";

const mockStore = configureStore([]);

function TestForm(props: any) {
return <div>{props.children}</div>;
}

const ReduxFormDecorator = reduxForm({
form: "TestForm",
})(TestForm);

const mockOptions = [
{ label: "Option 1", value: "option1", children: "Option 1" },
{ label: "Option 2", value: "option2", children: "Option 2" },
{ label: "Option 3", value: "option3", children: "Option 3" },
];

let radioButtonProps = {
options: mockOptions,
configProperty: "actionConfiguration.testPath",
controlType: "PROJECTION",
label: "Columns",
id: "column",
formName: "",
isValid: true,
initialValue: "option1",
};

describe("RadioButtonControl", () => {
beforeEach(() => {
let store: any;
store = mockStore();
});
it("should render RadioButtonControl and options properly", async () => {
render(
<Provider store={store}>
<ReduxFormDecorator>
<RadioButtonControl {...radioButtonProps} />
</ReduxFormDecorator>
</Provider>,
);
const radioButton = await waitFor(async () =>
screen.getByTestId("actionConfiguration.testPath"),
);
expect(radioButton).toBeInTheDocument();

const options = screen.getAllByRole("radio");
expect(options).toHaveLength(3);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Excellent work on your first test, class!

You've done a splendid job ensuring that our RadioButtonControl renders correctly. Your test checks for the presence of the radio button and verifies the number of options. Bravo!

However, let's challenge ourselves to go a step further. Can anyone tell me how we might improve this test?

Consider adding assertions to check if the labels of the radio buttons match the expected values. This will make our test even more robust! Here's an example of how you could do this:

mockOptions.forEach((option, index) => {
  expect(screen.getByLabelText(option.label)).toBeInTheDocument();
});

Who wants to add this to our test? It'll earn you extra credit!


it("should show the default selected option", async () => {
radioButtonProps = {
...radioButtonProps,
};

render(
<Provider store={store}>
<ReduxFormDecorator>
<RadioButtonControl {...radioButtonProps} />
</ReduxFormDecorator>
</Provider>,
);
const radioButton = await waitFor(async () =>
screen.getByTestId("actionConfiguration.testPath"),
);
expect(radioButton).toBeInTheDocument();

const options = screen.getAllByRole("radio");
expect(options[0]).toBeChecked();
expect(options[1]).not.toBeChecked();
expect(options[2]).not.toBeChecked();
});

it("should select the option when clicked", async () => {
radioButtonProps = {
...radioButtonProps,
};

render(
<Provider store={store}>
<ReduxFormDecorator>
<RadioButtonControl {...radioButtonProps} />
</ReduxFormDecorator>
</Provider>,
);
const radioButton = await waitFor(async () =>
screen.getByTestId("actionConfiguration.testPath"),
);
expect(radioButton).toBeInTheDocument();

const options = screen.getAllByRole("radio");
expect(options[0]).toBeChecked();
expect(options[1]).not.toBeChecked();
expect(options[2]).not.toBeChecked();

options[1].click();

expect(options[0]).not.toBeChecked();
expect(options[1]).toBeChecked();
expect(options[2]).not.toBeChecked();
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's review our final test, class!

I'm impressed with your thoroughness in testing the user interaction. You've successfully simulated clicking on a different option and verified that the selection changes accordingly. This is exactly the kind of real-world scenario we want to test!

However, I have a challenge for you to make this test even better. Can anyone guess what it might be?

Let's consider testing all possible selections, not just one. This will ensure our component behaves correctly regardless of which option is chosen. Here's how we could improve this:

mockOptions.forEach((option, index) => {
  options[index].click();
  mockOptions.forEach((_, i) => {
    if (i === index) {
      expect(options[i]).toBeChecked();
    } else {
      expect(options[i]).not.toBeChecked();
    }
  });
});

Who would like to implement this enhancement? It's an excellent opportunity to practice your testing skills!

69 changes: 69 additions & 0 deletions app/client/src/components/formControls/RadioButtonControl.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
import React from "react";
import type { ControlProps } from "./BaseControl";
import BaseControl from "./BaseControl";
import type { ControlType } from "constants/PropertyControlConstants";
import type { WrappedFieldInputProps, WrappedFieldMetaProps } from "redux-form";
import { Field } from "redux-form";
import { Radio, RadioGroup, type SelectOptionProps } from "@appsmith/ads";
import styled from "styled-components";

class RadioButtonControl extends BaseControl<RadioButtonControlProps> {
getControlType(): ControlType {
return "RADIO_BUTTON";
}
render() {
return (
<Field
component={renderComponent}
name={this.props.configProperty}
props={{ ...this.props }}
type="radiobutton"
/>
Comment on lines +16 to +21
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Correct the way props are passed to the Field component

Class, let's pay attention to how we're passing props to the Field component from redux-form. Currently, we're nesting our control's props under a props key by using props={{ ...this.props }}. This might cause issues when accessing props in renderComponent. Instead, we should spread this.props directly into the Field component so that they are passed correctly.

Apply this diff to fix the issue:

 <Field
    component={renderComponent}
    name={this.props.configProperty}
-   props={{ ...this.props }}
+   {...this.props}
    type="radiobutton"
 />
📝 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
<Field
component={renderComponent}
name={this.props.configProperty}
props={{ ...this.props }}
type="radiobutton"
/>
<Field
component={renderComponent}
name={this.props.configProperty}
{...this.props}
type="radiobutton"
/>

);
}
}

type renderComponentProps = RadioButtonControlProps & {
input?: WrappedFieldInputProps;
meta?: WrappedFieldMetaProps;
options?: Array<{ label: string; value: string }>;
};
Comment on lines +26 to +30
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure type consistency for the options property

Class, let's make sure we're maintaining consistency in our type definitions. The options property in renderComponentProps is currently defined as optional and uses an inline type. However, in RadioButtonControlProps, it's defined as required and uses SelectOptionProps[]. To avoid confusion and improve maintainability, we should use SelectOptionProps[] in both places and ensure options is consistently treated as required or optional.

Apply this diff to align the types:

 type renderComponentProps = RadioButtonControlProps & {
   input?: WrappedFieldInputProps;
   meta?: WrappedFieldMetaProps;
-  options?: Array<{ label: string; value: string }>;
 };

Committable suggestion was skipped due to low confidence.


const StyledRadioGroup = styled(RadioGroup)({
display: "flex",
flexDirection: "column",
gap: "16px",
marginTop: "16px",
});

function renderComponent(props: renderComponentProps) {
const onChangeHandler = (value: string): any => {
if (typeof props.input?.onChange === "function") {
props.input.onChange(value);
}
};

const options = props.options || [];
const defaultValue = props.initialValue as string;

return (
<StyledRadioGroup
data-testid={props.input?.name}
defaultValue={defaultValue}
onChange={onChangeHandler}
Comment on lines +47 to +53
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Make the RadioGroup a controlled component by using value instead of defaultValue

Class, to ensure that our RadioGroup stays in sync with the form state managed by redux-form, we should use value instead of defaultValue, and assign it to props.input?.value. This makes the RadioGroup a controlled component, which is essential for proper form behavior.

Apply this diff to make the component controlled:

- const defaultValue = props.initialValue as string;

 return (
   <StyledRadioGroup
     data-testid={props.input?.name}
-    defaultValue={defaultValue}
+    value={props.input?.value as string}
     onChange={onChangeHandler}
   >
📝 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
const defaultValue = props.initialValue as string;
return (
<StyledRadioGroup
data-testid={props.input?.name}
defaultValue={defaultValue}
onChange={onChangeHandler}
return (
<StyledRadioGroup
data-testid={props.input?.name}
value={props.input?.value as string}
onChange={onChangeHandler}

>
{options.map((option) => {
return (
<Radio key={option.value} value={option.value}>
{option.label}
</Radio>
);
})}
</StyledRadioGroup>
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Now, class, let's examine this renderComponent function. It's doing well, but there's room for improvement!

The function correctly handles the onChange event and maps options to Radio components. The use of defaultValue for setting an initial selection is appropriate.

However, let's think about how we can make this function more robust. Can anyone suggest some improvements?

  1. Add error handling for cases where options are not provided or are empty.
  2. Consider memoizing the Radio components to optimize performance, especially for large option lists.
  3. Add accessibility attributes (aria-label, aria-describedby) to enhance the component's usability for screen readers.

Here's an example of how you might implement these suggestions:

import React, { useMemo } from 'react';

function renderComponent(props: renderComponentProps) {
  const onChangeHandler = (value: string): void => {
    props.input?.onChange?.(value);
  };

  const options = props.options || [];
  const defaultValue = props.initialValue as string;

  if (options.length === 0) {
    console.warn('No options provided for RadioButtonControl');
    return null;
  }

  const memoizedRadios = useMemo(() => 
    options.map((option) => (
      <Radio 
        key={option.value} 
        value={option.value}
        aria-label={option.label}
      >
        {option.label}
      </Radio>
    )),
    [options]
  );

  return (
    <StyledRadioGroup
      data-testid={props.input?.name}
      defaultValue={defaultValue}
      onChange={onChangeHandler}
      aria-label={props.label}
    >
      {memoizedRadios}
    </StyledRadioGroup>
  );
}

export interface RadioButtonControlProps extends ControlProps {
options: SelectOptionProps[];
}

export default RadioButtonControl;
22 changes: 11 additions & 11 deletions app/client/src/pages/Editor/SaaSEditor/DatasourceForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -662,17 +662,6 @@ class DatasourceSaaSEditor extends JSONtoForm<Props, State> {
>
{(!viewMode || createFlow || isInsideReconnectModal) && (
<>
{/* This adds information banner when creating google sheets datasource,
this info banner explains why appsmith requires permissions from users google account */}
{datasource && isGoogleSheetPlugin && createFlow ? (
<AuthMessage
actionType={ActionType.DOCUMENTATION}
calloutType="info"
datasource={datasource}
description={googleSheetsInfoMessage}
pageId={pageId}
/>
) : null}
{/* This adds error banner for google sheets datasource if the datasource is unauthorised */}
{datasource &&
isGoogleSheetPlugin &&
Expand All @@ -688,6 +677,17 @@ class DatasourceSaaSEditor extends JSONtoForm<Props, State> {
? map(sections, this.renderMainSection)
: null}
{""}
{/* This adds information banner when creating google sheets datasource,
this info banner explains why appsmith requires permissions from users google account */}
{datasource && isGoogleSheetPlugin && createFlow ? (
<AuthMessage
actionType={ActionType.DOCUMENTATION}
calloutType="info"
datasource={datasource}
description={googleSheetsInfoMessage}
pageId={pageId}
/>
) : null}
Comment on lines +680 to +690
Copy link
Contributor

Choose a reason for hiding this comment

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

Class, pay attention to this important change!

The developer has made a thoughtful modification to improve the user experience when creating a Google Sheets datasource. They've added an informative banner that explains why Appsmith requires permissions from the user's Google account. This is an excellent addition to enhance transparency and user trust.

However, I'd like to see you improve the code's readability. Let's break this down into smaller, more manageable pieces:

-                      {datasource && isGoogleSheetPlugin && createFlow ? (
-                        <AuthMessage
-                          actionType={ActionType.DOCUMENTATION}
-                          calloutType="info"
-                          datasource={datasource}
-                          description={googleSheetsInfoMessage}
-                          pageId={pageId}
-                        />
-                      ) : null}
+                      {renderGoogleSheetsInfoBanner()}

Then, create a new method renderGoogleSheetsInfoBanner:

renderGoogleSheetsInfoBanner() {
  if (this.props.datasource && isGoogleSheetPluginDS(this.props.pluginPackageName) && this.props.createFlow) {
    return (
      <AuthMessage
        actionType={ActionType.DOCUMENTATION}
        calloutType="info"
        datasource={this.props.datasource}
        description={createMessage(GOOGLE_SHEETS_INFO_BANNER_MESSAGE)}
        pageId={this.props.pageId}
      />
    );
  }
  return null;
}

This refactoring will make the render method cleaner and easier to understand. Remember, class, clean code is happy code!

</>
)}
{viewMode &&
Expand Down
7 changes: 7 additions & 0 deletions app/client/src/utils/formControl/FormControlRegistry.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ import FormTemplateControl from "components/formControls/FormTemplateControl";
import type { FormTemplateControlProps } from "components/formControls/FormTemplateControl";
import MultiFilePickerControl from "components/formControls/MultiFilePickerControl";
import type { MultipleFilePickerControlProps } from "components/formControls/MultiFilePickerControl";
import type { RadioButtonControlProps } from "components/formControls/RadioButtonControl";
import RadioButtonControl from "components/formControls/RadioButtonControl";

/**
* NOTE: If you are adding a component that uses FormControl
Expand Down Expand Up @@ -183,6 +185,11 @@ class FormControlRegistry {
},
},
);
FormControlFactory.registerControlBuilder(formControlTypes.RADIO_BUTTON, {
buildPropertyControl(controlProps: RadioButtonControlProps): JSX.Element {
return <RadioButtonControl {...controlProps} />;
},
});
}
}

Expand Down
1 change: 1 addition & 0 deletions app/client/src/utils/formControl/formControlTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,5 @@ export default {
PROJECTION: "PROJECTION",
FORM_TEMPLATE: "FORM_TEMPLATE",
MULTIPLE_FILE_PICKER: "MULTIPLE_FILE_PICKER",
RADIO_BUTTON: "RADIO_BUTTON",
};
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
{
"label": "Permissions | Scope",
"configProperty": "datasourceConfiguration.authentication.scopeString",
"controlType": "DROP_DOWN",
"controlType": "RADIO_BUTTON",
"options": [
{
"label": "Read / Write / Delete | Selected google sheets",
Expand Down
Loading