chore: external merge request from Contributor #36575
Conversation
…ed cypress testing for the same
…to fix/bug-alert-shows-twice-for-select-widget-26696
…to fix/bug-alert-shows-twice-for-select-widget-26696
…to fix/bug-alert-shows-twice-for-select-widget-26696
…for cypress test case
…ice-for-select-widget-26696' into external-contri/fix/bug-alert-shows-twice-for-select-widget-26696
rahulbarwal
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces enhancements to the Select widget in the application. It adds a new test suite to validate event handling for dropdown interactions, ensuring alerts are displayed correctly. Additionally, modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/client/src/widgets/SelectWidget/component/index.tsx (1)
82-89: Attention, students! Let's analyze this method update.The modifications to
togglePopoverVisibilityshow great problem-solving skills. By adding theisButtonClickparameter and the conditional check, we've addressed an edge case where the method could be called twice when closing via button click. This is a prime example of defensive programming.However, let's consider a small enhancement:
Consider adding a comment explaining the edge case. This will help future developers understand the reasoning behind this check. Here's a suggested addition:
togglePopoverVisibility = (isButtonClick = false) => { + // Edge case: Prevent double-triggering when closing via button click if (isButtonClick && this.state.isOpen) return; // ... rest of the method };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts (1 hunks)
- app/client/src/widgets/SelectWidget/component/index.tsx (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts (1)
Pattern
app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
🔇 Additional comments (4)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Select/Select4_spec.ts (1)
1-18: Well done on the imports and file structure, class!The imports are neatly organized, and you've used tags for better test categorization. Good job linking the related issue too! This sets a great foundation for our test.
app/client/src/widgets/SelectWidget/component/index.tsx (3)
78-80: Well done, class! Let's examine this new method.The addition of
togglePopoverVisibilityFromButtonis a smart move. It acts as a specialized wrapper fortogglePopoverVisibility, allowing us to distinguish button-triggered toggles. This is an excellent example of the Single Responsibility Principle in action.
466-466: Class, observe this subtle yet important change in the render method.Excellent work on updating the
togglePopoverVisibilityprop to use the newtogglePopoverVisibilityFromButtonmethod. This change ensures that button clicks are handled with the new logic we just reviewed. It's a perfect example of how small changes can have a big impact on the overall functionality of our code.
Line range hint
1-524: Class dismissed! Let's recap our code review lesson.Today, we've examined some excellent improvements to our
SelectComponent. The changes we've reviewed effectively address the issue of multiple alert displays when closing the dropdown, as outlined in our PR objectives.Here's a summary of what we've learned:
- We added a new method
togglePopoverVisibilityFromButtonto handle button-specific toggle events.- We updated
togglePopoverVisibilityto prevent redundant calls in edge cases.- We modified the render method to use our new button-specific toggle method.
These changes demonstrate good problem-solving skills and adherence to solid programming principles. Remember, students, even small, focused changes can significantly improve the functionality and user experience of our applications.
Keep up the good work, and always strive for clear, efficient, and well-documented code!
| 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); | ||
| }); |
There was a problem hiding this comment.
Good effort on the test case, but let's make some improvements!
Your test case does a great job of covering the main functionality of the Select widget. However, there are a few areas where we can make it even better:
-
Instead of using
cy.get('#ToastId12'), let's use a more robust selector. Remember, we want to avoid direct element selection and use data attributes instead. -
We should use locator variables for all our selectors to make the test more maintainable.
-
The toast ID (#ToastId12) might change, making our test brittle. Let's find a more stable way to assert the toast message.
Here's how we can improve it:
it("Validate OnDropdownClose events are rendering show alert only once", () => {
// ... (previous code remains the same)
// Use a data attribute for the toast
agHelper.AssertElementVisibility(locators._toastMsg, true);
agHelper.ValidateToastMessage("Dropdown closed!");
// Use a more robust way to check for a single occurrence of the message
cy.get(locators._toastMsg)
.contains("Dropdown closed!")
.should("have.length", 1);
});Remember, class, using data attributes and locator variables makes our tests more robust and easier to maintain. Keep up the good work!
…o external-contri/fix/bug-alert-shows-twice-for-select-widget-26696
…ttps://github.com/PrasadMadine/appsmith into external-contri/fix/bug-alert-shows-twice-for-select-widget-26696
…ttps://github.com/PrasadMadine/appsmith into external-contri/fix/bug-alert-shows-twice-for-select-widget-26696
…ttps://github.com/PrasadMadine/appsmith into external-contri/fix/bug-alert-shows-twice-for-select-widget-26696
…org/appsmith into external-contri/fix/bug-alert-shows-twice-for-select-widget-26696
|
closing as no longer merged. |
Description
Issue: #26696
Original PR: #34812
EE PR: https://github.com/appsmithorg/appsmith-ee/pull/5258
Warning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11160976913
Commit: 7540edf
Cypress dashboard.
Tags:
@tag.AllSpec:
Thu, 03 Oct 2024 14:17:42 UTC