fix(#16584): filterTableData source of truth #36849
Conversation
WalkthroughThe changes made in this pull request focus on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
anasKhafaga
force-pushed
the
fix/16584-table-edit-with-filter
branch
from
b5007ea
to
72f3828
Compare
There was a problem hiding this comment.
Thanks @anasKhafaga, Can you please add some automation tests to this(unit preferably) for this change.
|
Added @rahulbarwal |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| fontStyle: "REGULAR", | ||
| enableFilter: true, | ||
| enableSort: true, | ||
| isVisible: true, | ||
| isDerived: false, | ||
| label: "id", | ||
| isAscOrder: false | ||
| }, | ||
| name: { | ||
| index: 0, | ||
| width: 150, | ||
| id: "name", | ||
| alias: "name", | ||
| originalId: "name", | ||
| horizontalAlignment: "LEFT", | ||
| verticalAlignment: "CENTER", | ||
| columnType: "text", | ||
| textColor: "#231F20", | ||
| textSize: "PARAGRAPH", | ||
| fontStyle: "REGULAR", | ||
| enableFilter: true, | ||
| enableSort: true, | ||
| isVisible: true, | ||
| isDerived: false, | ||
| label: "awesome", | ||
| isAscOrder: undefined | ||
| }, | ||
| extra: { | ||
| index: 2, | ||
| width: 150, | ||
| id: "extra", | ||
| alias: "extra", | ||
| originalId: "extra", | ||
| horizontalAlignment: "LEFT", | ||
| verticalAlignment: "CENTER", | ||
| columnType: "text", | ||
| textColor: "#231F20", | ||
| textSize: "PARAGRAPH", | ||
| fontStyle: "REGULAR", | ||
| enableFilter: true, | ||
| enableSort: true, | ||
| isVisible: true, | ||
| label: "extra", | ||
| isAscOrder: undefined, | ||
| isDerived: true, | ||
| }, | ||
| }, | ||
| tableColumns: [ | ||
| { | ||
| index: 0, | ||
| width: 150, | ||
| id: "name", | ||
| horizontalAlignment: "LEFT", | ||
| verticalAlignment: "CENTER", | ||
| columnType: "text", | ||
| textColor: "#231F20", | ||
| textSize: "PARAGRAPH", | ||
| fontStyle: "REGULAR", | ||
| enableFilter: true, | ||
| enableSort: true, | ||
| isVisible: true, | ||
| isDerived: false, | ||
| label: "awesome", | ||
| isAscOrder: undefined | ||
| }, | ||
| { | ||
| index: 1, | ||
| width: 150, | ||
| id: "id", | ||
| horizontalAlignment: "LEFT", | ||
| verticalAlignment: "CENTER", | ||
| columnType: "number", | ||
| textColor: "#231F20", | ||
| textSize: "PARAGRAPH", | ||
| fontStyle: "REGULAR", | ||
| enableFilter: true, | ||
| enableSort: true, | ||
| isVisible: true, | ||
| isDerived: false, | ||
| label: "id", | ||
| isAscOrder: false | ||
| } | ||
| ], | ||
| }; | ||
|
|
||
| input.orderedTableColumns = Object.values(input.primaryColumns).sort( | ||
| (a, b) => { | ||
| return input.columnOrder[a.id] < input.columnOrder[b.id]; | ||
| }, | ||
| ); | ||
|
|
||
| const expected = [ | ||
| { id: 234, name: "Khadija Khafaga", extra: '', __originalIndex__: 2 }, | ||
| { id: 123, name: "Hamza Khafa", extra: '', __originalIndex__: 1 }, | ||
| ]; | ||
|
|
||
| let result = getFilteredTableData(input, moment, _); | ||
| console.log(result); | ||
| expect(result).toStrictEqual(expected); | ||
| }); |
There was a problem hiding this comment.
Class, let's examine this new test case carefully.
Good job on adding a new test case to validate the filtering functionality after committing an edit. However, there are a few areas where we can improve:
-
The test description is quite long. Remember, brevity is the soul of wit! Consider shortening it to something like "filters correctly after editing a value with an active filter".
-
You've included a
console.log(result)statement on line 1315. While this is helpful during development, we should remove it from our final test case. Always clean up your workspace, students! -
The expected result on lines 1309-1312 includes an empty string for the 'extra' field. Is this intentional? If so, we should add a comment explaining why this field is expected to be empty.
-
The test data includes a misspelling in the name "Hamza Khafa" (missing the last 'ga'). Let's correct this to ensure our test data is accurate.
-
Consider adding more assertions to test edge cases, such as:
- What happens if the edited value no longer matches the filter?
- How does the system handle multiple filters?
Remember, thorough testing is the foundation of robust code. Let's make these improvements to strengthen our test suite.
Here's a suggested improvement for the test case:
it("filters correctly after editing a value with an active filter", () => {
const { getFilteredTableData } = derivedProperty;
const input = {
// ... (keep the existing input setup)
};
const expected = [
{ id: 234, name: "Khadija Khafaga", extra: undefined, __originalIndex__: 2 },
{ id: 123, name: "Hamza Khafaga", extra: undefined, __originalIndex__: 1 },
];
let result = getFilteredTableData(input, moment, _);
expect(result).toStrictEqual(expected);
// Additional test cases
input.processedTableData[1].name = "Hamza Smith"; // Edit to no longer match filter
result = getFilteredTableData(input, moment, _);
expect(result).toHaveLength(1); // Only Khadija should remain
// Test with multiple filters
input.filters.push({
condition: "contains",
column: "id",
value: "23"
});
result = getFilteredTableData(input, moment, _);
expect(result).toHaveLength(1); // Only Khadija should match both filters
});This improved version addresses the issues mentioned and adds more test cases to ensure robust functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/client/src/widgets/TableWidgetV2/widget/derived.test.js (1)
1184-1298: Well done on adding this new test case, class!This test case is a valuable addition to our suite, as it verifies the correct behavior of filtering after editing a value with an active filter. The structure is clear and follows good testing practices.
However, let's make one small improvement to enhance our learning experience:
Consider adding a comment explaining why the expected result includes the row with "Hamza Anas" even though it doesn't contain "Khafaga". This will help future students understand the test case better. For example:
const expected = [ { id: 234, name: "Khadija Khafaga", __originalIndex__: 2 }, { id: 123, name: "Hamza Anas", __originalIndex__: 1 }, // Included because original data contains "Khafaga" ];Remember, clear documentation is key to maintaining a well-organized codebase!
|
@anasKhafaga Client lint and prettier check are failing for your changes. Please fix.
|
|
@rahulbarwal , |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/client/src/widgets/TableWidgetV2/widget/derived.test.js (1)
1190-1304: Well done on adding this new test case, class!This test is a valuable addition to our suite, as it verifies the correct behavior of filtering after editing a value. The structure is clear and follows good testing practices.
However, let's make one small improvement to enhance our learning experience:
Consider adding a comment explaining why the edited row (Hamza Anas) is still included in the expected result despite not containing "Khafaga". This will help future students understand the intended behavior more clearly.
For example, you could add:
// Note: The edited row (Hamza Anas) is still included because filtering uses the original databefore the
const expected = [line.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/src/widgets/TableWidgetV2/widget/derived.js (1 hunks)
- app/client/src/widgets/TableWidgetV2/widget/derived.test.js (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/widgets/TableWidgetV2/widget/derived.js
🧰 Additional context used
|
Thanks @anasKhafaga. |
|
@rahulbarwal , |
|
@anasKhafaga looks like these changes are making 2 specs fail. Can you please check and fix? |
|
@rahulbarwal ,
Those test suits evaluates the old behavior which this PR comes to solve. In the third test case of each, cypress finally uncheck the first row without saving the changes. When the 4th test case(which fails) runs it filters the checked records only and expects that the first record to be filtered out and vice versa with filtering unchecked records. The only way is to update the test cases to suit the new expected behavior but I'll be waiting for your green light to make this update. |
Thats right @anasKhafaga |
anasKhafaga
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
|
@rahulbarwal , |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/switchCell_spec.js (3)
140-145: Well done, class! This addition looks good, but let's make a small improvement.I'm pleased to see you're using data attributes for selectors. That's excellent practice! However, we can make this even better by storing the selector in a variable at the top of the file. This way, if we need to change it later, we only need to update it in one place.
Here's a gold star for following best practices, but remember: constant improvement is the key to success!
Consider adding this at the top of the file:
const DISCARD_BUTTON_SELECTOR = "button[data-test-variant='TERTIARY']";Then use it like this:
cy.get($elemClass + " " + DISCARD_BUTTON_SELECTOR).click({ force: true });
163-166: Excellent work on expanding our test coverage!You've done a splendid job adding this assertion for the second row. It's like adding another layer to our testing cake - more delicious and more thorough!
However, let's make it even clearer for your fellow students who might read this later. Remember, clear code is like a well-organized classroom - it helps everyone learn better!
Consider adding a comment explaining why we're checking the second row:
// Verify that the filter applies correctly to multiple rows cy.getTableV2DataSelector("1", "4").then((selector) => { cy.get(selector + switchSelector).should("be.checked"); });
Line range hint
1-191: A for effort, but let's aim for an A+!Class, you've done a commendable job with this test file. Your attention to detail in following our Cypress best practices is praiseworthy. However, like any good lesson plan, there's always room for improvement!
Let's consider restructuring our tests for better isolation. Remember, in the world of testing, isolation is key - just like how we separate you during exams!
Consider the following improvements:
- Move the common setup steps from the
before()hook to abeforeEach()hook. This ensures each test starts with a clean slate.- Break down the larger test cases into smaller, more focused ones. This makes it easier to identify which specific functionality is failing if a test doesn't pass.
- Use
cy.intercept()to mock API responses where applicable, ensuring our tests are not dependent on external data.Here's a homework assignment: Try implementing these changes and see how it improves our test suite's reliability and maintainability. Remember, practice makes perfect!
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/checkboxCell_spec.js (3)
148-152: Good job, class! Let's make a small improvement.The new code segment looks good and follows our guidelines. You've used a data attribute for the selector, which is excellent. However, let's make it even better by storing the selector in a variable for reusability.
Here's a suggestion to improve the code:
const discardBtnSelector = `button[data-test-variant='TERTIARY']`; cy.getTableV2DataSelector("0", "5").then(($elemClass) => { cy.get($elemClass + discardBtnSelector).click({ force: true }); // discard changes });This way, if we need to use the discard button selector again, we can easily reuse the variable.
170-173: Well done! Let's ensure consistency in our code.The new assertion is a good addition to our test case. It follows the same structure as the previous assertions and uses the correct selectors. However, to maintain consistency throughout our test suite, let's extract the checkbox selector into a constant at the top of the file.
Here's a suggestion to improve consistency:
- At the top of the file, add:
const CHECKBOX_SELECTOR = " .bp3-checkbox input[type='checkbox']";
- Then, update this assertion and all similar ones in the file:
cy.getTableV2DataSelector("1", "4").then((selector) => { cy.get(selector + CHECKBOX_SELECTOR).should("be.checked"); });This change will make our code more maintainable and consistent across all our checkbox-related assertions.
Line range hint
1-203: Great work on your test suite! Let's make it even better.Overall, your test file follows Cypress best practices and our coding guidelines. I'm impressed by your consistent use of data attributes for selectors and the absence of any cy.wait or sleep statements. However, we can further enhance our test robustness by using multiple assertions where applicable.
Here's an example of how we can improve one of our test cases:
it("1. Check if the column type checkbox appears", () => { cy.getTableV2DataSelector("0", "4").then((selector) => { cy.get(selector + CHECKBOX_SELECTOR).should("exist").and("be.visible").and("not.be.disabled"); }); // ... rest of the test case });By chaining multiple assertions, we're not only checking for the existence of the checkbox but also ensuring it's visible and enabled. This approach provides a more comprehensive test of the checkbox's state and can help catch subtle issues.
Please review your other test cases and apply this principle of multiple assertions where it makes sense. This will make our test suite more robust and informative.
Keep up the excellent work, class!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/checkboxCell_spec.js (2 hunks)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/switchCell_spec.js (2 hunks)
- app/client/src/widgets/TableWidgetV2/widget/derived.js (3 hunks)
- app/client/src/widgets/TableWidgetV2/widget/derived.test.js (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/widgets/TableWidgetV2/widget/derived.js
🧰 Additional context used
📓 Path-based instructions (2)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/checkboxCell_spec.js (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/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/switchCell_spec.js (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.test.js (2)
1184-1298: Well done on adding this important test case!Class, let's examine this new test. It's an excellent addition that verifies the filtering behavior after editing a value. This test ensures that our table can handle scenarios where edited data may no longer match the applied filter.
A few points to note:
- The test structure is clear and easy to follow.
- It covers a critical edge case in our filtering logic.
- The expected result correctly includes both the matching row and the edited row.
Keep up the good work! Remember, thorough testing like this helps prevent bugs in our application.
1453-1561: Excellent work on this complementary test case!Class, let's turn our attention to this new test. It's a valuable addition that verifies the search functionality after editing a value. This test ensures that our table can handle scenarios where edited data may no longer match the applied search key.
Here are some key observations:
- The test structure mirrors the previous one, maintaining consistency in our test suite.
- It covers an important scenario for search functionality.
- The expected result correctly includes both the matching row and the edited row.
Remember, students, comprehensive testing like this helps us catch potential issues early. Keep up this level of attention to detail in your work!
| cy.getTableV2DataSelector("0", "5").then(($elemClass) => { | ||
| const discardBtnSelector = | ||
| $elemClass + " button[data-test-variant='TERTIARY']"; | ||
| cy.get(discardBtnSelector).click({ force: true }); // discard changes |
There was a problem hiding this comment.
Please use click function from Aghelper.
| @@ -145,6 +145,11 @@ describe( | |||
| .contains("This is a test message") | |||
| .should("be.visible"); | |||
| }); | |||
| cy.getTableV2DataSelector("0", "5").then(($elemClass) => { | |||
| const discardBtnSelector = | |||
| $elemClass + " button[data-test-variant='TERTIARY']"; | |||
There was a problem hiding this comment.
Kindly add this locator in locator files related to it.
| @@ -162,6 +167,10 @@ describe( | |||
| cy.get(selector + checkboxSelector).should("be.checked"); | |||
| }); | |||
|
|
|||
| cy.getTableV2DataSelector("1", "4").then((selector) => { | |||
| cy.get(selector + checkboxSelector).should("be.checked"); | |||
There was a problem hiding this comment.
Can we please use assertHelper here ?
There was a problem hiding this comment.
I'm not sure how could we use AssertHelper module here. I can't find suitable public method to achieve this exact purpose. Could you please elaborate more? @sagar-qa007
Beside this, should I modify on all already existing assertions in this file for consistency as I only change the order of assertions but haven't integrated any new ones?
There was a problem hiding this comment.
There was a problem hiding this comment.
@rahulbarwal ,
Many thanks, it really helped me.
Could you please check the requested changes @sagar-qa007 ?
| cy.getTableV2DataSelector("0", "5").then(($elemClass) => { | ||
| const discardBtnSelector = | ||
| $elemClass + " button[data-test-variant='TERTIARY']"; | ||
| cy.get(discardBtnSelector).click({ force: true }); // discard changes |
There was a problem hiding this comment.
Try to incorporate above PR comments here.
|
Found one edge case, please check it out @anasKhafaga |
|
@rahulbarwal , |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
app/client/src/widgets/TableWidgetV2/widget/derived.js (1)
834-842: Add documentation for the dual filtering behavior.Class, while the implementation is correct, it would be helpful to add a comment explaining why we're checking both original and displayed values. This helps future maintainers understand the purpose of this dual-check approach.
Add a comment like this before the filterResult assignment:
+// Check filter condition against both original and displayed values +// This ensures filters work correctly even when the displayed value differs from the original filterResult = conditionFunction( originalRow[props.filters[i].column], props.filters[i].value, ) || conditionFunction( displayedRow[props.filters[i].column], props.filters[i].value, );app/client/src/widgets/TableWidgetV2/widget/derived.test.js (1)
1271-1385: Class, let's improve this test case!While the test verifies the core functionality, we can enhance it in a few ways:
The test data shows
Hamza AnasinprocessedTableDatabutHamza KhafagaintableData. Let's add a comment explaining this intentional difference to help future students understand the test scenario.Consider adding assertions to verify that:
- The original row data is being used for filtering
- The displayed data shows the edited value
- The order of results matches the sort order
Here's a suggested improvement:
it("should filter correctly after editing a value with an applied filter", () => { const { getFilteredTableData } = derivedProperty; + // Test scenario: Row with name "Hamza Khafaga" was edited to "Hamza Anas" + // Expected: Row should still be included in filter results based on original value const input = { tableData: [ { id: 1234, name: "Jim Doe" }, { id: 123, name: "Hamza Khafaga" }, { id: 234, name: "Khadija Khafaga" }, ], // ... rest of the test setup ... }; const expected = [ { id: 234, name: "Khadija Khafaga", __originalIndex__: 2 }, { id: 123, name: "Hamza Anas", __originalIndex__: 1 }, ]; let result = getFilteredTableData(input, moment, _); expect(result).toStrictEqual(expected); + // Verify that results are in descending order by id + expect(result[0].id).toBeGreaterThan(result[1].id); + // Verify that edited value is displayed + expect(result[1].name).toBe("Hamza Anas"); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/src/widgets/TableWidgetV2/widget/derived.js (5 hunks)
- app/client/src/widgets/TableWidgetV2/widget/derived.test.js (3 hunks)
🧰 Additional context used
🔇 Additional comments (2)
app/client/src/widgets/TableWidgetV2/widget/derived.js (2)
449-458: Well done on improving the sorting logic!Class, notice how we're now properly handling both original and modified row data during sorting. This is an excellent example of maintaining data consistency.
693-695: Excellent consistency in implementation!Just like in our sorting section, we're retrieving the original row data here. This consistent pattern makes the code more maintainable and predictable.
| isSearchKeyFound = [ | ||
| ...Object.values(_.omit(displayedRow, hiddenColumns)), | ||
| ...Object.values(_.omit(originalRow, hiddenColumns)), | ||
| ] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing the search operation.
While including both displayed and original values in the search is thorough, let's think about performance. When dealing with large datasets, concatenating all values and performing string operations could be expensive.
Consider this optimization:
-isSearchKeyFound = [
- ...Object.values(_.omit(displayedRow, hiddenColumns)),
- ...Object.values(_.omit(originalRow, hiddenColumns)),
-].join(", ").toLowerCase().includes(searchKey);
+const searchInRow = (row) => Object.values(_.omit(row, hiddenColumns))
+ .some(value => value.toString().toLowerCase().includes(searchKey));
+isSearchKeyFound = searchInRow(displayedRow) || searchInRow(originalRow);This approach stops searching as soon as a match is found, potentially improving performance with large datasets.
Committable suggestion was skipped due to low confidence.
| { id: 1234, name: "Jim Doe", __originalIndex__: 0 }, | ||
| { id: 123, name: "Hamza Anas", __originalIndex__: 1 }, | ||
| { id: 234, name: "Khadija Khafaga", __originalIndex__: 2 }, | ||
| ], | ||
| searchText: "Khafaga", | ||
| sortOrder: { column: "id", order: "desc" }, | ||
| columnOrder: ["name", "id"], | ||
| primaryColumns: { | ||
| id: { | ||
| index: 1, | ||
| width: 150, | ||
| id: "id", | ||
| alias: "id", | ||
| originalId: "id", | ||
| horizontalAlignment: "LEFT", | ||
| verticalAlignment: "CENTER", | ||
| columnType: "number", | ||
| textColor: "#231F20", | ||
| textSize: "PARAGRAPH", | ||
| fontStyle: "REGULAR", | ||
| enableFilter: true, | ||
| enableSort: true, | ||
| isVisible: true, | ||
| isDerived: false, | ||
| label: "id", | ||
| isAscOrder: false, | ||
| }, | ||
| name: { | ||
| index: 0, | ||
| width: 150, | ||
| id: "name", | ||
| alias: "name", | ||
| originalId: "name", | ||
| horizontalAlignment: "LEFT", | ||
| verticalAlignment: "CENTER", | ||
| columnType: "text", | ||
| textColor: "#231F20", | ||
| textSize: "PARAGRAPH", | ||
| fontStyle: "REGULAR", | ||
| enableFilter: true, | ||
| enableSort: true, | ||
| isVisible: true, | ||
| isDerived: false, | ||
| label: "awesome", | ||
| isAscOrder: undefined, | ||
| }, | ||
| }, | ||
| tableColumns: [ | ||
| { | ||
| index: 0, | ||
| width: 150, | ||
| id: "name", | ||
| horizontalAlignment: "LEFT", | ||
| verticalAlignment: "CENTER", | ||
| columnType: "text", | ||
| textColor: "#231F20", | ||
| textSize: "PARAGRAPH", | ||
| fontStyle: "REGULAR", | ||
| enableFilter: true, | ||
| enableSort: true, | ||
| isVisible: true, | ||
| isDerived: false, | ||
| label: "awesome", | ||
| isAscOrder: undefined, | ||
| }, | ||
| { | ||
| index: 1, | ||
| width: 150, | ||
| id: "id", | ||
| horizontalAlignment: "LEFT", | ||
| verticalAlignment: "CENTER", | ||
| columnType: "number", | ||
| textColor: "#231F20", | ||
| textSize: "PARAGRAPH", | ||
| fontStyle: "REGULAR", | ||
| enableFilter: true, | ||
| enableSort: true, | ||
| isVisible: true, | ||
| isDerived: false, | ||
| label: "id", | ||
| isAscOrder: false, | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| input.orderedTableColumns = Object.values(input.primaryColumns).sort( | ||
| (a, b) => { | ||
| return input.columnOrder[a.id] < input.columnOrder[b.id]; | ||
| }, | ||
| ); | ||
|
|
||
| const expected = [ | ||
| { id: 234, name: "Khadija Khafaga", __originalIndex__: 2 }, | ||
| { id: 123, name: "Hamza Anas", __originalIndex__: 1 }, | ||
| ]; | ||
|
|
||
| let result = getFilteredTableData(input, moment, _); | ||
|
|
||
| expect(result).toStrictEqual(expected); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Students, let's discuss code duplication!
This test case is very similar to the previous one, just using searchText instead of filters. We should consolidate these tests to reduce duplication and make our test suite more maintainable.
Consider using a parameterized test or combining the cases:
describe("filtering after editing a value", () => {
const baseInput = {
tableData: [
{ id: 1234, name: "Jim Doe" },
{ id: 123, name: "Hamza Khafaga" },
{ id: 234, name: "Khadija Khafaga" },
],
processedTableData: [
{ id: 1234, name: "Jim Doe", __originalIndex__: 0 },
{ id: 123, name: "Hamza Anas", __originalIndex__: 1 },
{ id: 234, name: "Khadija Khafaga", __originalIndex__: 2 },
],
// ... common properties ...
};
const expected = [
{ id: 234, name: "Khadija Khafaga", __originalIndex__: 2 },
{ id: 123, name: "Hamza Anas", __originalIndex__: 1 },
];
it("should work with filters", () => {
const input = {
...baseInput,
filters: [{
condition: "contains",
column: "name",
value: "Khafaga",
}],
};
expect(getFilteredTableData(input, moment, _)).toStrictEqual(expected);
});
it("should work with search", () => {
const input = {
...baseInput,
searchText: "Khafaga",
};
expect(getFilteredTableData(input, moment, _)).toStrictEqual(expected);
});
});
|
Hey @anasKhafaga There are other failures which are unrelated, so we can ignore those. |
|
@rahulbarwal , |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/client/src/widgets/TableWidgetV2/widget/derived.js (5 hunks)
🔇 Additional comments (2)
app/client/src/widgets/TableWidgetV2/widget/derived.js (2)
696-698: Excellent implementation of dual-value search!Class, this is a good example of thorough search implementation. The code now correctly searches through both displayed and original values, ensuring no data is missed during filtering.
Also applies to: 803-806
449-461: Good addition of original data retrieval in sorting logic!Class, notice how we're properly maintaining data consistency by considering both displayed and original values during sorting. This is essential for the edit functionality to work correctly with filtered data.
Let's verify the sorting behavior with edited values:
✅ Verification successful
Let me search for test cases and sorting implementation to verify our sorting logic. As a teacher, I believe in thorough verification before drawing conclusions!
Class, let's look more closely at the test cases and implementation to verify our sorting behavior with edited values!
Class, after reviewing the test cases and implementation, I can confidently provide my assessment of the sorting logic changes!
The sorting implementation correctly handles both displayed and original values! ✓
The code demonstrates proper handling of data consistency by:
- Maintaining original row data through
__originalIndex__- Merging displayed and original values during sorting:
{ ...a, ...originalA }- Including test coverage specifically for edited table data sorting
This implementation ensures that sorting works correctly whether we're dealing with edited or original values, which is exactly what we want for our table widget!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential sorting edge cases with edited values # Look for test files that verify sorting behavior rg -l "sort.*edit" --type=testLength of output: 61
Script:
#!/bin/bash # Let's search for test files more broadly fd "test" -e js -e ts -e jsx -e tsx | grep -i "table" # Then let's look for sorting-related tests rg -l "sort" $(fd "test" -e js -e ts -e jsx -e tsx | grep -i "table") # Also check for any sorting implementation in the TableWidgetV2 rg -A 5 "sort" "app/client/src/widgets/TableWidgetV2"Length of output: 62912
Script:
#!/bin/bash # Let's examine the test cases specifically related to sorting with edited values rg -A 10 "validates generated filtered edited table data" app/client/src/widgets/TableWidgetV2/widget/derived.test.js # Also check the implementation of sorting with edited values ast-grep --pattern 'if (!selectColumnKeysWithSortByLabel.length) { $$$ [processedA, processedB] = [ { ...a, ...originalA }, { ...b, ...originalB }, ]; $$$ }'Length of output: 770
| filterResult = | ||
| conditionFunction( | ||
| originalRow[props.filters[i].column], | ||
| props.filters[i].value, | ||
| ) || | ||
| conditionFunction( | ||
| displayedRow[props.filters[i].column], | ||
| props.filters[i].value, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Let's make the filter condition check more readable!
Class, while the logic is correct, we can make it more maintainable by extracting the condition check into a helper function.
Consider this refactor to improve readability:
-filterResult =
- conditionFunction(
- originalRow[props.filters[i].column],
- props.filters[i].value,
- ) ||
- conditionFunction(
- displayedRow[props.filters[i].column],
- props.filters[i].value,
- );
+const checkCondition = (row, column, value) =>
+ conditionFunction(row[column], value);
+
+filterResult =
+ checkCondition(originalRow, props.filters[i].column, props.filters[i].value) ||
+ checkCondition(displayedRow, props.filters[i].column, props.filters[i].value);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| filterResult = | |
| conditionFunction( | |
| originalRow[props.filters[i].column], | |
| props.filters[i].value, | |
| ) || | |
| conditionFunction( | |
| displayedRow[props.filters[i].column], | |
| props.filters[i].value, | |
| ); | |
| const checkCondition = (row, column, value) => | |
| conditionFunction(row[column], value); | |
| filterResult = | |
| checkCondition(originalRow, props.filters[i].column, props.filters[i].value) || | |
| checkCondition(displayedRow, props.filters[i].column, props.filters[i].value); |
|
Is PR ready for merging or we still have further steps? @rahulbarwal |
|
Hey @anasKhafaga Once it passes, we will merge this PR. |
Description
When a filter is applied to the table data, the current behavior results in filter updates without validating or saving the updated value of the row being edited. This leads to a situation where users are unable to save or discard changes until the filters are cleared. The proposed fix ensures that the original row is retrieved from the table data and used in filtering, allowing editing and filtering to work as expected.
Here's a screen record for the solution
Motivation:
The problem arises in scenarios where table rows are editable and filters are applied. This change ensures that users can continue editing table rows without being forced to clear filters first.
Context:
This change is required to improve user experience and fix the broken editing functionality when filters are active. The change ensures that editable values in filtered rows are correctly handled and saved.
Fixes https://www.loom.com/share/335d0c61817646a0903d581adf73064e
Automation
/ok-to-test tags="table-widget,filter,edit"
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Style