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.

Sign up for GitHub

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

Jump to bottom

[Bug]: selectedRowIndex getting updated unnecessarily on data reload. #36080

Closed
1 task done
rahulbarwal opened this issue Sep 3, 2024 · 0 comments · Fixed by #36393
Closed
1 task done

[Bug]: selectedRowIndex getting updated unnecessarily on data reload. #36080

rahulbarwal opened this issue Sep 3, 2024 · 0 comments · Fixed by #36393
Assignees
@rahulbarwal
Labels
Bug Something isn't working High This issue blocks a user from building or impacts a lot of users Needs Triaging Needs attention from maintainers to triage Production QA Pod Issues under the QA Pod QA Needs QA attention Table Widget V2 Issues related to Table Widget V2 Widgets & Accelerators Pod Issues related to widgets & Accelerators Widgets Product This label groups issues related to widgets

Comments

@rahulbarwal
Copy link
Contributor

Is there an existing issue for this?

  • I have searched the existing issues

Description

Original problem: #34346 (comment)

Slack conversation 1
Slack conversation 2

Steps To Reproduce

  1. Drag and drop a table with server side searching and pagination
  2. Drop another table and have its data dependent on selectedRow of first table.
  3. Bind the updates(add/delete) to second table such that on success data of both the tables are updated.
  4. Insert(or delete) some data into the second table.
  5. On success it reloads both the tables.

Actual: observe that selectedRowIndex is updated in the first table
Expected: since the data in first table has not changed, the selectedRowIndex not update.

Public Sample App

No response

Environment

Production

Severity

High (Blocker to building or releasing)

Issue video log

No response

Version

v1.38.1
@rahulbarwal rahulbarwal added Bug Something isn't working Needs Triaging Needs attention from maintainers to triage Widgets & Accelerators Pod Issues related to widgets & Accelerators labels Sep 3, 2024
@rahulbarwal rahulbarwal self-assigned this Sep 3, 2024
@Nikhil-Nandagopal Nikhil-Nandagopal added High This issue blocks a user from building or impacts a lot of users Production labels Sep 3, 2024
@github-actions github-actions bot removed the Widgets & Accelerators Pod Issues related to widgets & Accelerators label Sep 3, 2024
@rahulbarwal rahulbarwal added the Widgets & Accelerators Pod Issues related to widgets & Accelerators label Sep 6, 2024
@github-actions github-actions bot removed the Widgets & Accelerators Pod Issues related to widgets & Accelerators label Sep 6, 2024
@rahulbarwal rahulbarwal added the Table Widget V2 Issues related to Table Widget V2 label Sep 6, 2024
@github-actions github-actions bot added Widgets Product This label groups issues related to widgets Widgets & Accelerators Pod Issues related to widgets & Accelerators labels Sep 6, 2024
@rahulbarwal rahulbarwal mentioned this issue Sep 18, 2024
Merged
2 tasks
@appsmith-bot appsmith-bot added the QA Needs QA attention label Sep 20, 2024
@github-actions github-actions bot added the QA Pod Issues under the QA Pod label Sep 20, 2024
Shivam-z pushed a commit to Shivam-z/appsmith that referenced this issue Sep 26, 2024
@rahulbarwal @Shivam-z
fix: Incorrect updation of selctedRowIndex when primary column is set. (
4bc8d35 
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 -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rahulbarwal rahulbarwal

Labels
Bug Something isn't working High This issue blocks a user from building or impacts a lot of users Needs Triaging Needs attention from maintainers to triage Production QA Pod Issues under the QA Pod QA Needs QA attention Table Widget V2 Issues related to Table Widget V2 Widgets & Accelerators Pod Issues related to widgets & Accelerators Widgets Product This label groups issues related to widgets
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: Incorrect updation of selctedRowIndex when primary column is set. appsmithorg/appsmith
3 participants
@Nikhil-Nandagopal @rahulbarwal @appsmith-bot