feat: Allow filtering of table select column label #36755
Conversation
jacquesikot
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
WalkthroughThe pull request introduces modifications to the Cypress end-to-end tests for the Table Widget, specifically focusing on filtering functionality for select column types. A new test case is added to verify filtering by the "role" column. Additionally, changes are made to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 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
jacquesikot
removed
Bug
github-actions
bot
added
Enhancement
github-actions
bot
added
Table Widget V2
jacquesikot
added
ok-to-test
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 and nitpick comments (2)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter1_1_Spec.ts (1)
179-187: Class, let's review this new test case together.Good job on adding a new test case to verify the table filter for select column type. However, there are a few areas where we can improve:
The
featureFlagInterceptcall should be moved to abeforeorbeforeEachhook. This way, we ensure the feature flag is set before each test, improving test isolation.Let's add some comments to explain what the test is doing. For example:
// Set up the table filter for the 'role' column table.OpenNFilterTable("role", "is exactly", "Product Manager"); // Verify that the first two rows contain "Product Manager" in the role column table.ReadTableRowColumnData(0, 2, "v2").then(($cellData) => { expect($cellData).to.eq("Product Manager"); });Consider adding a check to ensure that no other roles are present in the filtered results.
Remember, clear comments and thorough checks make for a better learning experience for everyone!
app/client/src/widgets/TableWidgetV2/widget/derived.js (1)
Line range hint
1-1176: Class, let's summarize what we've learned today.We've made some important improvements to our table widget, specifically in how we handle filtering for select columns. Here's a quick recap:
- We've introduced a new way to store and use label values for select columns, making our filtering more user-friendly.
- Our code now handles different types of select options, showing good adaptability.
- We've integrated these changes smoothly with our existing filtering logic.
These enhancements address the issue raised in our project objectives, allowing users to filter based on the visible labels they see in the table, rather than the underlying values. This is a significant improvement in usability!
However, as with any code changes, we should be vigilant:
- We should thoroughly test these changes with different types of data and select column configurations.
- We might want to add some comments explaining the purpose and functionality of
labelValuesForSelectCellfor future reference.- We should consider if this approach scales well for tables with a large number of select columns or with very large datasets.
Remember, good code is not just about solving the immediate problem, but also about maintainability and scalability. Keep up the good work, and don't forget to document your changes!
As we continue to enhance our table widget, we should consider the performance implications of these changes, especially for large datasets. We might want to explore caching strategies or lazy loading techniques to ensure our table remains responsive even with complex select column configurations.
📜 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 (1 hunks)
- app/client/src/widgets/TableWidgetV2/widget/derived.js (5 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 (2)
app/client/src/widgets/TableWidgetV2/widget/derived.js (2)
Line range hint
687-766: Class, let's examine the improvements made to our table filtering mechanism.Now, children, we've made some significant changes to how we handle select columns in our table. Let's go through them step by step:
We've introduced a new object called
labelValuesForSelectCellto store the labels for our select columns. This is a good practice, as it allows us to keep our data organized.We're now identifying which columns are of the 'select' type and have options. This shows good attention to detail, doesn't it?
For each select column, we're finding the label that corresponds to the selected value. This is like matching a student's name to their student ID - very important for clarity!
We've added some clever logic to handle different types of select options - both flat arrays and nested arrays. This flexibility is commendable, as it allows our code to work with various data structures.
Finally, we're incorporating these label values into our
displayedRowobject. This ensures that when we filter or search, we're looking at the human-readable labels rather than just the underlying values.These changes will make our table much more user-friendly, especially when filtering on select columns. Great job! However, there's always room for improvement. Can anyone suggest how we might make this code even more efficient or readable?
Line range hint
767-860: Let's make sure we're using our new knowledge correctly, class.Now that we've added our
labelValuesForSelectCellobject, we need to ensure we're using it properly throughout our filtering process. Can anyone spot where we're using this new object?That's right! We're incorporating it into our
displayedRowobject. This is good, but let's double-check a few things:
Are we using
displayedRowconsistently in our filtering logic? It looks like we are, which is excellent!Have we considered all possible scenarios? For instance, what happens if a select column doesn't have a matching label?
Could we add some comments to explain the purpose of
labelValuesForSelectCellfor future students... I mean, developers?Remember, clear communication in our code is just as important as getting the logic right. It's like showing your work in a math problem - it helps others understand your thinking!
To make sure we haven't missed anything, let's run a quick check:
This will help us ensure we're using our new object correctly throughout the table widget. Any volunteers to run this check and report back to the class?
✅ Verification successful
Let's try running these updated scripts to ensure we correctly identify all instances of
labelValuesForSelectCelland verify the consistent use ofdisplayedRowin our filtering logic.
Verification of
labelValuesForSelectCellIntegrationAfter reviewing the search results, we can confirm that
labelValuesForSelectCellis correctly integrated into the filtering logic withinderived.js. Additionally,displayedRowis used consistently throughout the filtering process.
labelValuesForSelectCellis properly assigned and utilized in thedisplayedRowobject.- No discrepancies were found in how
displayedRowis employed in the filtering conditions.Everything looks good! Great job ensuring that the new object is seamlessly incorporated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of labelValuesForSelectCell in filtering logic # Test: Search for labelValuesForSelectCell usage rg --type javascript 'labelValuesForSelectCell' app/client/src/widgets/TableWidgetV2/ # Test: Check if displayedRow is used consistently in filtering rg --type javascript 'displayedRow\[.*?\]' app/client/src/widgets/TableWidgetV2/Length of output: 238
Script:
#!/bin/bash # Description: Verify the usage of labelValuesForSelectCell in filtering logic without using --type # Test: Search for labelValuesForSelectCell usage in .js and .jsx files rg 'labelValuesForSelectCell' app/client/src/widgets/TableWidgetV2/ --glob '*.js' --glob '*.jsx' # Test: Check if displayedRow is used consistently in filtering within .js and .jsx files rg 'displayedRow\[' app/client/src/widgets/TableWidgetV2/ --glob '*.js' --glob '*.jsx'Length of output: 866
github-actions
bot
added
Bug
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter1_1_Spec.ts (1)
179-186: Class, let's review this new test case together.Alright, students, gather 'round! We have a new addition to our test suite, and it's time to put on our thinking caps. Let's break it down step by step:
- The test description is clear and concise. Good job!
- We're using a feature flag intercept, which is a smart way to control feature visibility.
- The filter is being applied correctly using the
OpenNFilterTablemethod.- We're verifying the results with two assertions, which is excellent for thorough testing.
However, there's room for improvement:
- We should avoid using plain strings for selectors. Consider using data-* attributes instead.
- The
featureFlagInterceptcall could be moved to abeforeorbeforeEachblock to keep our test setup clean.Overall, it's a good effort, but let's strive for excellence! Who wants to volunteer to refactor this test case?
app/client/src/widgets/TableWidgetV2/widget/derived.js (4)
Line range hint
687-766: Let's consider the performance implications of our changes, class.While our new approach to handling select columns is more comprehensive, we need to be mindful of its impact on performance, especially when dealing with large datasets. The additional processing for each row could potentially slow down our table rendering and filtering operations.
To address this, I suggest we consider the following optimizations:
- Memoization: We could memoize the
labelValuesForSelectCellobject for each row to avoid recalculating it on every filter operation.- Lazy evaluation: Instead of processing all select columns upfront, we could defer the label lookup until it's actually needed for filtering.
- Batch processing: For very large datasets, we might want to process rows in batches to avoid blocking the main thread for too long.
Remember, class, optimization is about finding the right balance between functionality and performance. Let's keep an eye on how these changes affect our table's responsiveness, especially with larger datasets.
Would you like me to provide some code examples for these optimizations?
Line range hint
719-758: Let's discuss error handling and edge cases, class.In our enthusiasm to improve our select column handling, we must not forget about potential pitfalls. Our code now handles various formats of
selectOptions, but we should be prepared for any unexpected scenarios. Let's consider adding some additional error handling:
- Empty selectOptions: We should gracefully handle cases where
selectOptionsis an empty array or object.- Malformed data: If the
selectOptionsdata is not in the expected format, our code should not throw an error.- Missing labels or values: We should have a fallback for cases where a label or value is missing from an option.
Here's an example of how we might improve our error handling:
if (isSelectOptionsAnArray) { const selectOptionKey = primaryColumns[key].alias; try { if (_.some(primaryColumns[key].selectOptions, _.isArray)) { selectOptions = primaryColumns[key].selectOptions[row.__originalIndex__] || []; } else { selectOptions = primaryColumns[key].selectOptions; } const option = selectOptions.find((option) => option && option.value === value); if (option && option.label) { labelValuesForSelectCell[selectOptionKey] = option.label; } else { labelValuesForSelectCell[selectOptionKey] = value; // Fallback to value if label is missing } } catch (error) { console.error(`Error processing select options for ${key}:`, error); labelValuesForSelectCell[selectOptionKey] = value; // Fallback to value in case of any error } } else { // Similar error handling for non-array selectOptions... }Remember, class, robust error handling is key to creating reliable software. Always prepare for the unexpected!
Shall we implement these additional safeguards to make our code more resilient?
Line range hint
687-766: Let's talk about code organization and maintainability, class.Our
getFilteredTableDatafunction has grown quite complex with these new changes. While it's functioning well, we should consider refactoring to improve its maintainability and readability. Here are some suggestions:
- Extract the select option processing logic into a separate function:
function getSelectOptionLabel(column, row, value) { const selectOptions = getSelectOptions(column, row); const option = selectOptions.find(opt => opt.value === value); return option ? option.label : value; } function getSelectOptions(column, row) { if (_.isArray(column.selectOptions)) { return _.some(column.selectOptions, _.isArray) ? column.selectOptions[row.__originalIndex__] : column.selectOptions; } return JSON.parse(column.selectOptions); }
- Simplify the main function by using these helper functions:
selectColumnKeys.forEach((key) => { const column = primaryColumns[key]; const value = row[key]; const selectOptionKey = column.alias; labelValuesForSelectCell[selectOptionKey] = getSelectOptionLabel(column, row, value); });
- Consider creating a separate class or module for handling select options if this logic is used elsewhere in the codebase.
By breaking down our complex logic into smaller, more manageable pieces, we make our code easier to understand, test, and maintain. Remember, class, clean code is happy code!
Shall we implement these refactoring suggestions to improve our code structure?
Line range hint
687-766: Let's summarize our lesson on improving thegetFilteredTableDatafunction, class.We've made significant strides in enhancing our table's filtering capabilities, particularly for select columns. Our new approach allows us to filter based on visible labels rather than underlying values, which will greatly improve the user experience.
Here's a quick recap of our findings:
- Functionality: The changes successfully address the issue of filtering select columns by label.
- Performance: We've identified potential performance concerns and suggested optimizations like memoization and lazy evaluation.
- Error Handling: We've recommended additional error handling to make our code more robust against unexpected data.
- Maintainability: We've suggested refactoring to improve code organization and readability.
Overall, these changes are a positive step forward. However, there's always room for improvement in software development. I encourage you all to consider implementing the suggested optimizations and refactoring to make this good solution even better.
Remember, class, coding is an iterative process. We should always strive to improve our work, making it more efficient, robust, and maintainable. Keep up the good work!
Does anyone have any questions about our review or suggestions for further improvements?
📜 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 (1 hunks)
- app/client/src/widgets/TableWidgetV2/widget/derived.js (5 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 (1)
app/client/src/widgets/TableWidgetV2/widget/derived.js (1)
Line range hint
687-766: Class, let's examine the improvements made to ourgetFilteredTableDatafunction.Now, children, we've made some significant changes to how we handle select columns. Instead of using a single
labelValueForSelectCellvariable, we've introduced a more comprehensivelabelValuesForSelectCellobject. This allows us to store multiple label values for different select columns, which is a much more organized approach.Let's break down the changes:
- We've added a new array called
selectColumnKeysto keep track of columns that are of type 'select' and have select options.- We're now iterating over these
selectColumnKeysto find the label value for each select column in every row.- We've improved our handling of different types of
selectOptions, whether they're arrays, nested arrays, or JSON strings.- Finally, we're spreading the
labelValuesForSelectCellobject into ourdisplayedRowobject, which allows us to include all label values when filtering.These changes will greatly enhance our ability to filter and search based on the visible labels in select columns, rather than just the underlying values. Well done, class!
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11233264809. |
|
Deploy-Preview-URL: https://ce-36755.dp.appsmith.com |
Description
Problem
When filtering a table with a select column type, users expect to filter by the visible label values shown in each cell. Currently, however, filtering is applied to the underlying option values rather than the displayed labels, leading to unexpected filter results for end-users.
Root Cause
In a previous update (PR #35124), the table cell display for select columns was changed to show labels instead of values. However, the filtering logic was not updated accordingly, so the table still filtered on the original option values, creating a mismatch between displayed and filtered content.
Solution
This PR modifies the displayedRow property within the table widget to use the label property instead of the value key when filtering or searching select column data. This ensures that table filtering and searching now align with the visible label values in the select columns, providing a more intuitive user experience.
Fixes #36635
Automation
/ok-to-test tags="@tag.Sanity, @tag.Table, @tag.Select, @tag.Binding"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11234735437
Commit: fd6c179
Cypress dashboard.
Tags:
@tag.Sanity, @tag.Table, @tag.Select, @tag.BindingSpec:
Tue, 08 Oct 2024 12:48:01 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Refactor