feat: Allow table search to include label and value for tables with select column type #36061
Conversation
jacquesikot
added
Table Widget
ok-to-test
jacquesikot
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
WalkthroughThe changes enhance the functionality of the TableV2 component by integrating feature flags and improving the search capabilities for select widget columns. A new dataset structure is introduced for testing, and the search functionality now correctly identifies both label and value entries in select columns. These modifications aim to resolve issues related to searching within data tables. Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
Poem
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter1_1_Spec.ts (1)
191-230: Great job on the new test case! Here are a few suggestions to make it even better:
- Consider extracting the column options data into a separate constant for better readability and maintainability.
- Use more descriptive variable names for the
afterSearchvariables to clarify their purpose.- Add comments to explain the purpose of each search scenario (label search and value search) for better understanding.
Overall, the test case is well-structured, covers an important scenario, and follows the best practices for Cypress testing. It ensures that the search functionality works correctly for tables with select column types when the feature flag is enabled.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter1_1_Spec.ts (3 hunks)
- app/client/src/widgets/TableWidgetV2/widget/derived.js (1 hunks)
Additional context used
Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter1_1_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 not posted (7)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter1_1_Spec.ts (4)
1-1: Imports look good!The new import statements for
featureFlagIntercept,EditorNavigation, andEntityTypeare correctly added and will be useful for integrating feature flags and editor navigation functionalities into the test suite.Also applies to: 12-14
16-60: ThedemoTableDataconstant is well-structured and comprehensive.The
demoTableDataconstant provides a realistic and diverse dataset for testing the TableV2 component. The data structure and attributes cover various scenarios, making it suitable for thorough testing.
Line range hint
1-230: Excellent adherence to Cypress best practices and additional instructions!The code follows the recommended practices for Cypress testing, such as:
- Avoiding
cy.wait,cy.pause, andagHelper.sleep().- Using locator variables instead of plain strings.
- Utilizing data-* attributes for selectors.
- Avoiding XPaths, attributes, and complex CSS paths.
- Performing login, logout, and signup via API calls.
- Not using
it.only.- Avoiding
afterandafterEachhooks in test cases.- Using multiple assertions for
expectstatements.- Avoiding the use of strings for assertions.
- Using a unique filename.
- Not using
agHelper.Sleeporthis.Sleep.Keep up the great work in following the best practices and writing clean, maintainable test code!
Line range hint
1-230: Outstanding work on the Cypress test code!The code is well-structured, follows best practices, and provides comprehensive coverage of important scenarios. The test cases are readable, maintainable, and adhere to the additional instructions provided.
I appreciate your attention to detail and efforts in writing high-quality test code. Keep up the excellent work, and feel free to reach out if you have any further questions or need assistance with your Cypress tests.
app/client/src/widgets/TableWidgetV2/widget/derived.js (3)
591-616: Great work on enhancing the search for select columns! 👍The code changes correctly retrieve the label value for select columns and store it in the
labelValueForSelectCellvariable. This will allow the search to match against the label value displayed to the user.
619-619: Nice addition to thedisplayedRowobject.Including the
labelValueForSelectCellin thedisplayedRowobject ensures that the label value is considered during the search.
Line range hint
1-589:Also applies to: 620-1000
|
@jacquesikot Please check failure. |
github-actions
bot
added
Bug
There was a problem hiding this comment.
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 (5 hunks)
Additional context used
Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/Select1_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 not posted (3)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/Select1_spec.ts (3)
89-96: The data structure change is consistent with the previous update.This code segment also wraps the array of objects with an additional set of curly braces, maintaining consistency with the change observed in the previous code segment. This reinforces the assumption that the modification is part of a broader update to align with a new specification.
Line range hint
182-197: LGTM! The data structure change is consistent.This code segment follows the same pattern of wrapping the array of objects with an additional set of curly braces. It aligns with the previous changes and maintains consistency throughout the file.
Line range hint
45-60: Verify the impact of the data structure change.The input data structure for the property control options has been modified by wrapping the array of objects with an additional set of curly braces. While this change seems intentional, it's crucial to ensure that the rest of the application correctly handles this new structure.
Please run the following script to verify that the data structure change does not introduce any regressions:
Verification successful
Let's correct the regex pattern by escaping the curly braces and re-run the script to verify the impact of the data structure change. This will help us ensure that the new data structure is handled correctly throughout the application.
Let's adjust our approach by specifying the file extension directly using the
--globoption. This will help us search through the TypeScript files correctly and verify the impact of the data structure change.
No issues found with the data structure change.
The search did not reveal any instances of the
cy.updateCodeInputfunction being used with the updated data structure pattern elsewhere in the codebase. This suggests that the change is isolated to the file under review and does not impact other parts of the application. Therefore, no regressions are expected from this modification.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the table component correctly handles the updated input data structure. # Test: Search for usages of the property control options data. Expect: No errors related to the data structure change. rg --type typescript $'cy\.updateCodeInput\(\s*".+",\s*`\s*{{\s*\[\s*{' -A 10Length of output: 245
Script:
#!/bin/bash # Description: Verify the table component correctly handles the updated input data structure. # Test: Search for usages of the property control options data. Expect: No errors related to the data structure change. rg --type typescript $'cy\\.updateCodeInput\\(\\s*".+",\\s*`\\s*\\{\\{\\s*\\[\\s*\\{' -A 10Length of output: 116
Script:
#!/bin/bash # Description: Verify the table component correctly handles the updated input data structure. # Test: Search for usages of the property control options data. Expect: No errors related to the data structure change. rg --glob '*.ts' $'cy\\.updateCodeInput\\(\\s*".+",\\s*`\\s*\\{\\{\\s*\\[\\s*\\{' -A 10Length of output: 77
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10680871261. |
|
Deploy-Preview-URL: https://ce-36061.dp.appsmith.com |
Description
Problem
The search functionality in the table widget does not properly search through the label values in columns with the select type. Currently, it only searches using the underlying value property, leading to inconsistent and inaccurate search results.
Root Cause
A recent PR updated the table widget to display label values instead of the raw values in select columns. However, this change inadvertently caused the search logic to overlook these label values, continuing to search only by the value property.
Solution
To address this, the search functionality in the derived.js file within tablev2 has been updated. The solution involves retrieving the label value associated with the select column's value property and passing this label into the search logic. This ensures that the search functionality accurately reflects what is displayed in the table, providing correct search results based on the label values.
Fixes #35406
Automation
/ok-to-test tags="@tag.Sanity, @tag.Widget, @tag.Table, @tag.Binding"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10817317106
Commit: f921efc
Cypress dashboard.
Tags:
@tag.Sanity, @tag.Widget, @tag.Table, @tag.BindingSpec:
Wed, 11 Sep 2024 19:44:20 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Chores