-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
fix: table save row button onclick function #35412
Conversation
WalkthroughThe recent modifications enhance the Cypress test suite for the Table widget's one-click binding feature and the filterable property of the table widget. By improving test organization, readability, and robustness of interactions, these updates ensure comprehensive test coverage while maintaining the core functionality. This facilitates easier maintenance and greater reliability of the test code. Changes
Assessment against linked issues
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/TableWidget/mysql_spec.ts (1 hunks)
- app/client/src/widgets/TableWidgetV2/component/header/banner/AddNewRowBanner.tsx (1 hunks)
Files skipped from review due to trivial changes (1)
- app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/TableWidget/mysql_spec.ts
Additional comments not posted (1)
app/client/src/widgets/TableWidgetV2/component/header/banner/AddNewRowBanner.tsx (1)
72-73
: Good job! Preventing event propagation is crucial in complex UIs.The addition of
event.stopPropagation()
in theonClick
handler ensures that the click event does not bubble up to parent elements, which can prevent unintended behaviors and improve the user experience.
/ci-test-limit |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/10247779852. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/cypress/limited-tests.txt (1 hunks)
Files skipped from review due to trivial changes (1)
- app/client/cypress/limited-tests.txt
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10247779852. |
/ci-test-limit |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/10248857569. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10248857569. |
/ci-test-limit |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/10250890007. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10250890007. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove all static wait ie agHelper.Sleep and the execute this spec multiple times on EE and CE that will give you clear idea if removing static wait has removed all flakiness. @AmanAgarwal041
app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/TableWidget/mysql_spec.ts
Outdated
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/TableWidget/mysql_spec.ts
Outdated
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/TableWidget/mysql_spec.ts
Outdated
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/TableWidget/mysql_spec.ts
Outdated
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/TableWidget/mysql_spec.ts
Outdated
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/TableWidget/mysql_spec.ts
Outdated
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/TableWidget/mysql_spec.ts
Outdated
Show resolved
Hide resolved
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/10288935324. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10288933666. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10288933741. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10288935324. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10288933774. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10288934833. |
…146-table-one-click
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/Select1_spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/Select1_spec.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/Select1_spec.ts
Additional context used
Learnings (1)
Common learnings
Learnt from: sagar-qa007 PR: appsmithorg/appsmith#35412 File: app/client/cypress/e2e/Regression/ClientSide/OneClickBinding/TableWidget/mysql_spec.ts:99-99 Timestamp: 2024-08-06T05:59:19.000Z Learning: In Cypress tests within the `app/client/cypress` directory, avoid using `agHelper.Sleep`, `this.Sleep`, and other related sleep functions to prevent non-deterministic behaviors and ensure tests are more reliable and maintainable. Instead, use Cypress methods like `cy.get()`, `cy.contains()`, and `cy.intercept()` to wait for specific conditions.
Description
The
onClick
method was attached to both button and parent child : https://github.com/appsmithorg/appsmith/blob/release/app/client/src/widgets/ButtonWidget/component/index.tsx#L204-L232Hence, whenever we clicked on the button using the UI, since there was a check of loading state inside the onclick handler it was not triggering twice as before running the onclick of parent it was updating the reference value of loading inside it.
But if we trigger the button click using cypress or the
document.querySelector
from console, it used to execute the addnewrow action twice, thereby creating two rows. Hence added a stopPropagation function in the onclick handler, which restricts the onclick to propagate to the parent.Fixes #35146
EE runs - https://github.com/appsmithorg/appsmith-ee/pull/4818
or
Fixes
Issue URL
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.Widget, @tag.Datasource, @tag.Binding"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10301142371
Commit: 40a81cb
Cypress dashboard.
Tags:
@tag.Widget, @tag.Datasource, @tag.Binding
Spec:
Thu, 08 Aug 2024 12:44:05 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests