From 86c083e48c8975a794653a5a74e2be3f68ff45e4 Mon Sep 17 00:00:00 2001 From: PrasadMadine Date: Tue, 9 Jul 2024 13:47:54 +0530 Subject: [PATCH 1/6] fixed the bug alert message is showing twice in select widget and added cypress testing for the same --- .../ClientSide/Widgets/Select/Select4_spec.ts | 37 +++++++++++++++++++ .../widgets/SelectWidget/component/index.tsx | 2 +- 2 files changed, 38 insertions(+), 1 deletion(-) create mode 100644 app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts diff --git a/app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts b/app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts new file mode 100644 index 00000000000..8e31985c240 --- /dev/null +++ b/app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts @@ -0,0 +1,37 @@ +import { + agHelper, + draggableWidgets, + deployMode, + entityExplorer, + locators, + propPane, +} from "../../../../../support/Objects/ObjectsCore"; + +describe( + "Select widget tests", + { tags: ["@tag.Widget", "@tag.Select"] }, + function () { + before(() => { + entityExplorer.DragDropWidgetNVerify(draggableWidgets.SELECT); + }); + + + it("Validate OnDropdownClose events are rendering show alert only once", () => { + propPane.EnterJSContext( + "onDropdownClose", + "{{showAlert('Dropdown closed!','success')}}", + true, + ); + propPane.ToggleJSMode("onDropdownClose", false); + deployMode.DeployApp(locators._widgetInDeployed(draggableWidgets.SELECT)); + agHelper.GetNClick(locators._widgetInDeployed(draggableWidgets.SELECT)); + agHelper.AssertElementVisibility( + locators._selectOptionValue("Red"), + true, + ); + agHelper.GetNClick(locators._selectOptionValue("Red")); + agHelper.ValidateToastMessage("Dropdown closed!"); + }); + }, +); + \ No newline at end of file diff --git a/app/client/src/widgets/SelectWidget/component/index.tsx b/app/client/src/widgets/SelectWidget/component/index.tsx index fba3aee71ec..26dc1995f3b 100644 --- a/app/client/src/widgets/SelectWidget/component/index.tsx +++ b/app/client/src/widgets/SelectWidget/component/index.tsx @@ -177,7 +177,7 @@ class SelectComponent extends React.Component< }; handleCloseList = () => { if (this.state.isOpen) { - this.togglePopoverVisibility(); + // this.togglePopoverVisibility(); if (!this.props.selectedIndex) return; return this.handleActiveItemChange( this.props.options[this.props.selectedIndex], From ebf1d7dfc4e140e3451a45d6187637d366c14153 Mon Sep 17 00:00:00 2001 From: PrasadMadine Date: Fri, 12 Jul 2024 13:30:26 +0530 Subject: [PATCH 2/6] Fix: updated the description of cypress test for select widget --- .../e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts b/app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts index 8e31985c240..ae973ad9fa0 100644 --- a/app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts +++ b/app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts @@ -8,7 +8,7 @@ import { } from "../../../../../support/Objects/ObjectsCore"; describe( - "Select widget tests", + "Select widget tests validating OnDropdownClose events are rendering show alert only once", { tags: ["@tag.Widget", "@tag.Select"] }, function () { before(() => { From 98d07b2d90536df80c7985ff03cc7f11309be7e4 Mon Sep 17 00:00:00 2001 From: PrasadMadine Date: Wed, 25 Sep 2024 16:29:55 +0530 Subject: [PATCH 3/6] fix: bug with different approach and added alerts and count of alert for cypress test case --- .../ClientSide/Widgets/Select/Select4_spec.ts | 26 +++++++++++++++---- .../widgets/SelectWidget/component/index.tsx | 20 +++++++++----- 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts b/app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts index ae973ad9fa0..5ec12f86cd8 100644 --- a/app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts +++ b/app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts @@ -1,12 +1,13 @@ import { - agHelper, + agHelper, draggableWidgets, deployMode, entityExplorer, locators, propPane, } from "../../../../../support/Objects/ObjectsCore"; - + +// Issue link: https://github.com/appsmithorg/appsmith/issues/26696 describe( "Select widget tests validating OnDropdownClose events are rendering show alert only once", { tags: ["@tag.Widget", "@tag.Select"] }, @@ -14,8 +15,7 @@ describe( before(() => { entityExplorer.DragDropWidgetNVerify(draggableWidgets.SELECT); }); - - + it("Validate OnDropdownClose events are rendering show alert only once", () => { propPane.EnterJSContext( "onDropdownClose", @@ -23,15 +23,31 @@ describe( true, ); propPane.ToggleJSMode("onDropdownClose", false); + propPane.EnterJSContext( + "onDropdownOpen", + "{{showAlert('Dropdown opened!','success')}}", + true, + ); + propPane.ToggleJSMode("onDropdownOpen", false); + propPane.EnterJSContext( + "onOptionChange", + "{{showAlert('Option changed!','success')}}", + true, + ); + propPane.ToggleJSMode("onOptionChange", false); deployMode.DeployApp(locators._widgetInDeployed(draggableWidgets.SELECT)); agHelper.GetNClick(locators._widgetInDeployed(draggableWidgets.SELECT)); + agHelper.ValidateToastMessage("Dropdown opened!"); agHelper.AssertElementVisibility( locators._selectOptionValue("Red"), true, ); agHelper.GetNClick(locators._selectOptionValue("Red")); + agHelper.ValidateToastMessage("Option changed!"); agHelper.ValidateToastMessage("Dropdown closed!"); + cy.get("#ToastId12 > .Toastify__toast-body") + .contains("Dropdown closed!") + .should("have.length", 1); }); }, ); - \ No newline at end of file diff --git a/app/client/src/widgets/SelectWidget/component/index.tsx b/app/client/src/widgets/SelectWidget/component/index.tsx index ddcf4be5279..2391d892c13 100644 --- a/app/client/src/widgets/SelectWidget/component/index.tsx +++ b/app/client/src/widgets/SelectWidget/component/index.tsx @@ -78,11 +78,12 @@ class SelectComponent extends React.Component< togglePopoverVisibility = () => { if (this.state.isOpen) { this.handleOnDropdownClose(); - } else { - this.handleOnDropdownOpen(); + this.setState( + (prev) => { + return { ...prev, isOpen: false }; + }, + ); } - - this.setState({ isOpen: !this.state.isOpen }); }; handleActiveItemChange = (activeItem: DropdownOption | null) => { @@ -188,7 +189,7 @@ class SelectComponent extends React.Component< }; handleCloseList = () => { if (this.state.isOpen) { - // this.togglePopoverVisibility(); + this.togglePopoverVisibility(); if (!this.props.selectedIndex) return; return this.handleActiveItemChange( @@ -455,7 +456,14 @@ class SelectComponent extends React.Component< hideCancelIcon={this.props.hideCancelIcon} isRequired={this.props.isRequired} spanRef={this.spanRef} - togglePopoverVisibility={this.togglePopoverVisibility} + togglePopoverVisibility={() => { + if(!this.state.isOpen){ + this.handleOnDropdownOpen(); + this.setState((prev) => { + return { ...prev, isOpen: !prev.isOpen }; + }); + } + }} tooltipText={tooltipText} value={this.props.value?.toString()} /> From ad953b3720572ee10e5eeac5036c1e61914e2ce1 Mon Sep 17 00:00:00 2001 From: PrasadMadine Date: Thu, 26 Sep 2024 12:35:26 +0530 Subject: [PATCH 4/6] fix: Addressed the review comments --- .../widgets/SelectWidget/component/index.tsx | 25 ++++++++----------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/app/client/src/widgets/SelectWidget/component/index.tsx b/app/client/src/widgets/SelectWidget/component/index.tsx index 2391d892c13..848719f68e8 100644 --- a/app/client/src/widgets/SelectWidget/component/index.tsx +++ b/app/client/src/widgets/SelectWidget/component/index.tsx @@ -75,14 +75,16 @@ class SelectComponent extends React.Component< } }; - togglePopoverVisibility = () => { - if (this.state.isOpen) { + togglePopoverVisibility = (isButtonClick = false) => { + if (!isButtonClick && this.state.isOpen) { this.handleOnDropdownClose(); - this.setState( - (prev) => { - return { ...prev, isOpen: false }; - }, - ); + this.setState({ isOpen: false }); + } else if (isButtonClick) { + this.handleOnDropdownOpen(); + this.setState((prev) => ({ + ...prev, + isOpen: !prev.isOpen, + })); } }; @@ -456,14 +458,7 @@ class SelectComponent extends React.Component< hideCancelIcon={this.props.hideCancelIcon} isRequired={this.props.isRequired} spanRef={this.spanRef} - togglePopoverVisibility={() => { - if(!this.state.isOpen){ - this.handleOnDropdownOpen(); - this.setState((prev) => { - return { ...prev, isOpen: !prev.isOpen }; - }); - } - }} + togglePopoverVisibility={() => this.togglePopoverVisibility(true)} tooltipText={tooltipText} value={this.props.value?.toString()} /> From f8da8fc94857bef0234122a02a9ae4579e16fd99 Mon Sep 17 00:00:00 2001 From: PrasadMadine Date: Thu, 26 Sep 2024 15:46:15 +0530 Subject: [PATCH 5/6] fix: addressed the comments suggested by coderabbit ai --- .../widgets/SelectWidget/component/index.tsx | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/app/client/src/widgets/SelectWidget/component/index.tsx b/app/client/src/widgets/SelectWidget/component/index.tsx index 848719f68e8..08cef98a4ae 100644 --- a/app/client/src/widgets/SelectWidget/component/index.tsx +++ b/app/client/src/widgets/SelectWidget/component/index.tsx @@ -75,17 +75,22 @@ class SelectComponent extends React.Component< } }; + togglePopoverVisibilityFromButton = () => { + this.togglePopoverVisibility(true); + }; + togglePopoverVisibility = (isButtonClick = false) => { - if (!isButtonClick && this.state.isOpen) { + // This is an edge case, this method gets called twice if user closes it by clicking on the `SelectButton` + // which in turn triggers handleOnDropdownClose twice, to solve we have this exception to tell if click event is from button + if (isButtonClick && this.state.isOpen) return; + + if (this.state.isOpen) { this.handleOnDropdownClose(); - this.setState({ isOpen: false }); - } else if (isButtonClick) { + } else { this.handleOnDropdownOpen(); - this.setState((prev) => ({ - ...prev, - isOpen: !prev.isOpen, - })); } + + this.setState({ isOpen: !this.state.isOpen }); }; handleActiveItemChange = (activeItem: DropdownOption | null) => { @@ -458,7 +463,7 @@ class SelectComponent extends React.Component< hideCancelIcon={this.props.hideCancelIcon} isRequired={this.props.isRequired} spanRef={this.spanRef} - togglePopoverVisibility={() => this.togglePopoverVisibility(true)} + togglePopoverVisibility={this.togglePopoverVisibilityFromButton} tooltipText={tooltipText} value={this.props.value?.toString()} /> From b7d13576859e30a3794d6fa842b0f2e234980abf Mon Sep 17 00:00:00 2001 From: PrasadMadine Date: Fri, 27 Sep 2024 15:40:14 +0530 Subject: [PATCH 6/6] fix: Added unit test cases --- .../ClientSide/Widgets/Select/Select4_spec.ts | 53 ------------------- .../SelectWidget/component/index.test.tsx | 36 +++++++++++++ 2 files changed, 36 insertions(+), 53 deletions(-) delete mode 100644 app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts create mode 100644 app/client/src/widgets/SelectWidget/component/index.test.tsx diff --git a/app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts b/app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts deleted file mode 100644 index 5ec12f86cd8..00000000000 --- a/app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts +++ /dev/null @@ -1,53 +0,0 @@ -import { - agHelper, - draggableWidgets, - deployMode, - entityExplorer, - locators, - propPane, -} from "../../../../../support/Objects/ObjectsCore"; - -// Issue link: https://github.com/appsmithorg/appsmith/issues/26696 -describe( - "Select widget tests validating OnDropdownClose events are rendering show alert only once", - { tags: ["@tag.Widget", "@tag.Select"] }, - function () { - before(() => { - entityExplorer.DragDropWidgetNVerify(draggableWidgets.SELECT); - }); - - it("Validate OnDropdownClose events are rendering show alert only once", () => { - propPane.EnterJSContext( - "onDropdownClose", - "{{showAlert('Dropdown closed!','success')}}", - true, - ); - propPane.ToggleJSMode("onDropdownClose", false); - propPane.EnterJSContext( - "onDropdownOpen", - "{{showAlert('Dropdown opened!','success')}}", - true, - ); - propPane.ToggleJSMode("onDropdownOpen", false); - propPane.EnterJSContext( - "onOptionChange", - "{{showAlert('Option changed!','success')}}", - true, - ); - propPane.ToggleJSMode("onOptionChange", false); - deployMode.DeployApp(locators._widgetInDeployed(draggableWidgets.SELECT)); - agHelper.GetNClick(locators._widgetInDeployed(draggableWidgets.SELECT)); - agHelper.ValidateToastMessage("Dropdown opened!"); - agHelper.AssertElementVisibility( - locators._selectOptionValue("Red"), - true, - ); - agHelper.GetNClick(locators._selectOptionValue("Red")); - agHelper.ValidateToastMessage("Option changed!"); - agHelper.ValidateToastMessage("Dropdown closed!"); - cy.get("#ToastId12 > .Toastify__toast-body") - .contains("Dropdown closed!") - .should("have.length", 1); - }); - }, -); diff --git a/app/client/src/widgets/SelectWidget/component/index.test.tsx b/app/client/src/widgets/SelectWidget/component/index.test.tsx new file mode 100644 index 00000000000..b85523ddb58 --- /dev/null +++ b/app/client/src/widgets/SelectWidget/component/index.test.tsx @@ -0,0 +1,36 @@ +import React from "react"; +import { render, screen } from "@testing-library/react"; +import SelectComponent, { type SelectComponentProps } from "./index"; +import userEvent from "@testing-library/user-event"; + +const mockProps: SelectComponentProps = { + borderRadius: "", + compactMode: false, + dropDownWidth: 0, + height: 0, + isFilterable: false, + isLoading: false, + isValid: false, + labelText: "", + onFilterChange: jest.fn(), + onOptionSelected: jest.fn(), + onDropdownClose: jest.fn(), + onDropdownOpen: jest.fn(), + options: [], + serverSideFiltering: false, + width: 0, + widgetId: "", +}; + +describe("SelectComponent", () => { + it("should call onDropdownClose only once when select button is clicked twice", () => { + render(); + const dropdownButton = screen.getByRole("button"); + userEvent.click(dropdownButton); + + expect(mockProps.onDropdownOpen).toHaveBeenCalledTimes(1); + userEvent.click(dropdownButton); + + expect(mockProps.onDropdownClose).toHaveBeenCalledTimes(1); + }); +});