fix: Incorrect updation of selctedRowIndex when primary column is set. #36393
Conversation
…ed to select row using original index, simple index will suffice.
WalkthroughThe pull request introduces a modification to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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
Bug
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10919585530. |
|
Deploy-Preview-URL: https://ce-36393.dp.appsmith.com |
…o fix-36080/selectedRowIndex-getting-updated-unnecessarily
…o fix-36080/selectedRowIndex-getting-updated-unnecessarily
…o fix-36080/selectedRowIndex-getting-updated-unnecessarily
|
🔴 There's new test files in JS, please port to TS and re-trigger Cypress tests:
|
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10937002988. |
|
🔴 There's new test files in JS, please port to TS and re-trigger Cypress tests:
|
…o fix-36080/selectedRowIndex-getting-updated-unnecessarily
rahulbarwal
added
ok-to-test
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_FilteredTableData_spec.ts (1 hunks)
- app/client/cypress/fixtures/tableV2FilteringWithPrimaryColumnJSObjectWidthData.ts (1 hunks)
Additional context used
Path-based instructions (2)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_FilteredTableData_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.
app/client/cypress/fixtures/tableV2FilteringWithPrimaryColumnJSObjectWidthData.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 not posted (1)
app/client/cypress/fixtures/tableV2FilteringWithPrimaryColumnJSObjectWidthData.ts (1)
1-84: Great work on creating this test data fixture! 👍Class, let's take a closer look at the
tableDataJSObjectconstant. This JavaScript object contains an array of employee records, which will be very useful for testing the filtering and display functionality of a table component.Each employee record follows a consistent structure, with properties like
employeeId,name,department,position,location, andsalary. This well-organized data will make it easy to validate the behavior of the table when filtering based on the primary column.Having a diverse set of test data, covering various departments and locations, is essential for thorough testing. With these ten employee records, you'll be able to simulate different scenarios and ensure that the table component handles them correctly.
Remember, using realistic test data is crucial for catching edge cases and ensuring a robust user experience. Keep up the excellent work in creating comprehensive test fixtures like this one!
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_FilteredTableData_spec.ts
Outdated
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_FilteredTableData_spec.ts
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_FilteredTableData_spec.ts
Outdated
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_FilteredTableData_spec.ts
Show resolved
Hide resolved
…o fix-36080/selectedRowIndex-getting-updated-unnecessarily
rahulbarwal
added
ok-to-test
…o fix-36080/selectedRowIndex-getting-updated-unnecessarily
rahulbarwal
deleted the
fix-36080/selectedRowIndex-getting-updated-unnecessarily
branch
appsmithorg#36393) ## Description <ins> Problem statement </ins> 1. Table should have a primary column set. 2. The table should be filtered(via search or client side filters) 3. there should be a selectedRow in table a. One way to ensure is add a text widget and bind it to table's `selectedRowIndex` property. If you now run a query that updates the data behind the table: the `selectedRow` is updated to (seemingly) random row and selectedRowIndex is updated as well <ins> Rootcause </ins> We have a mechanism of preserving `selectedRow` when data updates happen. Underlying logic gets the previous selected row and tries to find it in the new data using the primary key. * If it finds it, it updates the selectedRowIndex to the `__original_index__` of the found row i.e. its position in the original data before any filters were applied. <ins> Solution </ins> We update the selectedRowIndex to the index in the new filtered data. <ins> How to test </ins> 1. Create a table with a primary key 2. Add a text widget and bind it to table's `selectedRowIndex` property 3. Filter the table 4. Click on a row to select it 5. Run a query that updates the data behind the table 6. Assert that the selected row is the same and that the selectedRowIndex is updated to the new index Fixes appsmithorg#36080 _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.Table, @tag.Binding, @tag.Sanity" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/10953679202> > Commit: 1a2d644 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10953679202&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Table, @tag.Binding, @tag.Sanity` > Spec: > <hr>Fri, 20 Sep 2024 06:30:08 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit - **Improvements** - Enhanced the efficiency of the TableWidgetV2 by streamlining the method for retrieving the index of selected rows. - Improved code readability and maintainability without altering the overall functionality. - **New Features** - Introduced new test cases to validate filtering and searching functionalities within the Table Widget V2. - Added a JSON configuration for a data-driven table interface, supporting sorting, filtering, and dynamic data binding. - Introduced fixture data for testing, allowing for comprehensive validation of table behavior under various conditions. - Added a JavaScript object containing employee data to facilitate filtering and display in the table. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Problem statement
a. One way to ensure is add a text widget and bind it to table's
selectedRowIndexproperty.If you now run a query that updates the data behind the table: the
selectedRowis updated to (seemingly) random row and selectedRowIndex is updated as wellRootcause
We have a mechanism of preserving
selectedRowwhen data updates happen.Underlying logic gets the previous selected row and tries to find it in the new data using the primary key.
__original_index__of the found row i.e. its position in the original data before any filters were applied.Solution
We update the selectedRowIndex to the index in the new filtered data.
How to test
selectedRowIndexpropertyFixes #36080
or
Fixes
Issue URLWarning
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.Table, @tag.Binding, @tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10953679202
Commit: 1a2d644
Cypress dashboard.
Tags:
@tag.Table, @tag.Binding, @tag.SanitySpec:
Fri, 20 Sep 2024 06:30:08 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
Improvements
New Features