From 2cfe0b0facda39c1a0436d3a403b362e9c3b3b42 Mon Sep 17 00:00:00 2001 From: Anas Khafaga <58850182+anasKhafaga@users.noreply.github.com> Date: Wed, 30 Oct 2024 15:03:01 +0300 Subject: [PATCH] fix(#16584): filterTableData source of truth (#36849) ## Description > **TL;DR**: This PR addresses is related to #16584 where editing a table row with applied filters becomes impossible without clearing the filters 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](https://drive.google.com/file/d/1JuP_UN_B1vzz_oMeR1ojjPscF_mB3A4x/view?usp=sharing) 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" ### :mag: 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? - [x] Yes - [ ] No ## Summary by CodeRabbit - **New Features** - Improved filtering accuracy in the TableWidgetV2 by using original row data for evaluations. - **Bug Fixes** - Enhanced functionality to ensure that filtering conditions are evaluated correctly with original data. - **Tests** - Added new test cases to validate filtering functionality after edits in the TableWidgetV2. - Expanded test coverage for checkbox and switch interactions to ensure accurate filtering behavior, including new interactions with a discard button. - **Style** - Minor adjustments to comments and formatting for better readability. --- .../TableV2/columnTypes/checkboxCell_spec.js | 21 +- .../TableV2/columnTypes/switchCell_spec.js | 12 +- .../widgets/TableWidgetV2/widget/derived.js | 56 +++- .../TableWidgetV2/widget/derived.test.js | 313 ++++++++++++++++++ 4 files changed, 378 insertions(+), 24 deletions(-) diff --git a/app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/checkboxCell_spec.js b/app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/checkboxCell_spec.js index ece9e034ff..f73ac9eb8f 100644 --- a/app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/checkboxCell_spec.js +++ b/app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/checkboxCell_spec.js @@ -145,6 +145,7 @@ describe( .contains("This is a test message") .should("be.visible"); }); + _.agHelper.ClickButton("Discard", { index: 0 }); // discard changes }); it("4. Verify filter condition", () => { @@ -159,7 +160,17 @@ describe( // filter and verify checked rows cy.getTableV2DataSelector("0", "4").then((selector) => { - cy.get(selector + checkboxSelector).should("be.checked"); + _.agHelper.AssertExistingCheckedState( + selector + checkboxSelector, + "true", + ); + }); + + cy.getTableV2DataSelector("1", "4").then((selector) => { + _.agHelper.AssertExistingCheckedState( + selector + checkboxSelector, + "true", + ); }); // Filter and verify unchecked rows @@ -170,10 +181,10 @@ describe( cy.get(publishPage.applyFiltersBtn).click(); cy.getTableV2DataSelector("0", "4").then((selector) => { - cy.get(selector + checkboxSelector).should("not.be.checked"); - }); - cy.getTableV2DataSelector("1", "4").then((selector) => { - cy.get(selector + checkboxSelector).should("not.be.checked"); + _.agHelper.AssertExistingCheckedState( + selector + checkboxSelector, + "false", + ); }); }); diff --git a/app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/switchCell_spec.js b/app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/switchCell_spec.js index 7288e635b5..8625c88984 100644 --- a/app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/switchCell_spec.js +++ b/app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/switchCell_spec.js @@ -137,6 +137,7 @@ describe( cy.wait(100); agHelper.ValidateToastMessage("This is a test message"); }); + agHelper.ClickButton("Discard", { index: 0 }); // discard changes }); it("4. Verify filter condition", () => { @@ -151,7 +152,11 @@ describe( // filter and verify checked rows cy.getTableV2DataSelector("0", "4").then((selector) => { - cy.get(selector + switchSelector).should("be.checked"); + agHelper.AssertExistingCheckedState(selector + switchSelector, "true"); + }); + + cy.getTableV2DataSelector("1", "4").then((selector) => { + agHelper.AssertExistingCheckedState(selector + switchSelector, "true"); }); // Filter and verify unchecked rows @@ -162,10 +167,7 @@ describe( cy.get(publishPage.applyFiltersBtn).click(); cy.getTableV2DataSelector("0", "4").then((selector) => { - cy.get(selector + switchSelector).should("not.be.checked"); - }); - cy.getTableV2DataSelector("1", "4").then((selector) => { - cy.get(selector + switchSelector).should("not.be.checked"); + agHelper.AssertExistingCheckedState(selector + switchSelector, "false"); }); }); diff --git a/app/client/src/widgets/TableWidgetV2/widget/derived.js b/app/client/src/widgets/TableWidgetV2/widget/derived.js index defec50570..c62e58ac6f 100644 --- a/app/client/src/widgets/TableWidgetV2/widget/derived.js +++ b/app/client/src/widgets/TableWidgetV2/widget/derived.js @@ -446,25 +446,42 @@ export default { sortedTableData = transformedTableDataForSorting.sort((a, b) => { if (_.isPlainObject(a) && _.isPlainObject(b)) { + let [processedA, processedB] = [a, b]; + + if (!selectColumnKeysWithSortByLabel.length) { + const originalA = (props.tableData ?? + transformedTableDataForSorting)[a.__originalIndex__]; + const originalB = (props.tableData ?? + transformedTableDataForSorting)[b.__originalIndex__]; + + [processedA, processedB] = [ + { ...a, ...originalA }, + { ...b, ...originalB }, + ]; + } + if ( - isEmptyOrNil(a[sortByColumnOriginalId]) || - isEmptyOrNil(b[sortByColumnOriginalId]) + isEmptyOrNil(processedA[sortByColumnOriginalId]) || + isEmptyOrNil(processedB[sortByColumnOriginalId]) ) { /* push null, undefined and "" values to the bottom. */ - return isEmptyOrNil(a[sortByColumnOriginalId]) ? 1 : -1; + return isEmptyOrNil(processedA[sortByColumnOriginalId]) ? 1 : -1; } else { switch (columnType) { case "number": case "currency": return sortByOrder( - Number(a[sortByColumnOriginalId]) > - Number(b[sortByColumnOriginalId]), + Number(processedA[sortByColumnOriginalId]) > + Number(processedB[sortByColumnOriginalId]), ); case "date": try { return sortByOrder( - moment(a[sortByColumnOriginalId], inputFormat).isAfter( - moment(b[sortByColumnOriginalId], inputFormat), + moment( + processedA[sortByColumnOriginalId], + inputFormat, + ).isAfter( + moment(processedB[sortByColumnOriginalId], inputFormat), ), ); } catch (e) { @@ -489,8 +506,8 @@ export default { } default: return sortByOrder( - a[sortByColumnOriginalId].toString().toLowerCase() > - b[sortByColumnOriginalId].toString().toLowerCase(), + processedA[sortByColumnOriginalId].toString().toLowerCase() > + processedB[sortByColumnOriginalId].toString().toLowerCase(), ); } } @@ -676,6 +693,9 @@ export default { const finalTableData = sortedTableData.filter((row) => { let isSearchKeyFound = true; + const originalRow = (props.tableData ?? sortedTableData)[ + row.__originalIndex__ + ]; const columnWithDisplayText = Object.values(props.primaryColumns).filter( (column) => column.columnType === "url" && column.displayText, ); @@ -780,7 +800,10 @@ export default { }; if (searchKey) { - isSearchKeyFound = Object.values(_.omit(displayedRow, hiddenColumns)) + isSearchKeyFound = [ + ...Object.values(_.omit(displayedRow, hiddenColumns)), + ...Object.values(_.omit(originalRow, hiddenColumns)), + ] .join(", ") .toLowerCase() .includes(searchKey); @@ -811,10 +834,15 @@ export default { ConditionFunctions[props.filters[i].condition]; if (conditionFunction) { - filterResult = conditionFunction( - displayedRow[props.filters[i].column], - props.filters[i].value, - ); + filterResult = + conditionFunction( + originalRow[props.filters[i].column], + props.filters[i].value, + ) || + conditionFunction( + displayedRow[props.filters[i].column], + props.filters[i].value, + ); } } catch (e) { filterResult = false; diff --git a/app/client/src/widgets/TableWidgetV2/widget/derived.test.js b/app/client/src/widgets/TableWidgetV2/widget/derived.test.js index e7ba48a1a3..e12cb35ff3 100644 --- a/app/client/src/widgets/TableWidgetV2/widget/derived.test.js +++ b/app/client/src/widgets/TableWidgetV2/widget/derived.test.js @@ -580,6 +580,93 @@ describe("Validates getFilteredTableData Properties", () => { expect(result).toStrictEqual(expected); }); + it("validates generated filtered edited table data to be sorted correctly based on column type", () => { + const { getFilteredTableData } = derivedProperty; + const input = { + processedTableData: [ + { id: 123, name: "BAC", __originalIndex__: 0 }, + { id: 1234, name: "ABC", __originalIndex__: 1 }, + { id: 234, name: "CAB", __originalIndex__: 2 }, + ], + tableData: [ + { id: 123, name: "BAC" }, + { id: 1234, name: "ABC" }, + { id: 234, name: "CAB" }, + ], + sortOrder: { column: "name", order: "asc" }, + 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, + computedValue: ["BAC", "ABC", "AAB"], + }, + }, + }; + + input.orderedTableColumns = Object.values(input.primaryColumns).sort( + (a, b) => { + return input.columnOrder[a.id] < input.columnOrder[b.id]; + }, + ); + + const expected = [ + { + id: 1234, + name: "ABC", + __originalIndex__: 1, + }, + { + id: 123, + name: "BAC", + __originalIndex__: 0, + }, + { + id: 234, + name: "AAB", + __originalIndex__: 2, + }, + ]; + + let result = getFilteredTableData(input, moment, _); + + expect(result).toStrictEqual(expected); + }); + it("validates generated filtered table data with null values to be sorted correctly", () => { const { getFilteredTableData } = derivedProperty; const input = { @@ -1181,6 +1268,122 @@ describe("Validates getFilteredTableData Properties", () => { expect(result).toStrictEqual(expected); }); + it("should filter correctly after editing a value with an applied filter", () => { + const { getFilteredTableData } = derivedProperty; + const input = { + 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 }, + ], + filters: [ + { + condition: "contains", + column: "name", + value: "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); + }); + it("validates generated sanitized table data with valid property keys", () => { const { getProcessedTableData } = derivedProperty; @@ -1333,6 +1536,116 @@ describe("Validates getFilteredTableData Properties", () => { expect(result).toStrictEqual(expected); }); + + it("filters correctly after editing a value with an applied search key", () => { + const { getFilteredTableData } = derivedProperty; + const input = { + 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 }, + ], + 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); + }); }); describe("Validate getSelectedRow function", () => {