Protei/PromucFlow_constructor
2
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

fix: Refactor validation logic in TableWidgetV2 to improve clarity and correctness (#40679)

Browse Source 
## Description
<ins>Problem</ins>

The table widget skipped validations for number columns when users typed
quickly and pressed Enter, accepting invalid values despite min/max
constraints.

<ins>Root cause</ins>

The validation logic had an early exit if the value was empty and the
column wasn't required. This incorrectly allowed empty values even when
min/max constraints were set. Due to async evaluations and timing
differences, a quick backspace followed by a new input and Enter led to
premature validation success.

<ins>Solution</ins>

This PR handles a refactor of the validation logic in `TableWidgetV2` to
improve clarity and correctness.
- Consolidates checks for required fields and regex validations.  
- Ensures column type validations accurately reflect constraints like
min/max.
- Prevents premature validation passes caused by async evaluation race
conditions.

Fixes #`Issue Number`  
_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"

### 🔍 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/15160290045>
> Commit: b3022119656954765a79b1e03c5af2a4338a89c4
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=15160290045&attempt=1"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.Table`
> Spec:
> <hr>Wed, 21 May 2025 11:35:27 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

- **Bug Fixes**
- Enhanced validation for editable table cells to better enforce
required fields and regex patterns.
- Improved handling of empty values in required and non-required cells
for more accurate validation feedback.
- **Tests**
- Refactored and reorganized validation test suites for improved clarity
and maintainability without changing validation behavior.
- Updated end-to-end tests to improve timing and synchronization by
removing fixed waits and relying on automatic retries.
- Adjusted inline editing validation tests to reflect updated error
visibility expectations during editing.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
Rahul Barwal 2025-05-21 18:45:24 +05:30 committed by GitHub
parent a13e09f33f
commit d64361e225
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 483 additions and 1304 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

37
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/AddNewRow2_spec.js
View File

@ -18,36 +18,25 @@ describe(
_.table.toggleColumnEditableViaColSettingsPane("step", "v2", true, false);
_.propPane.UpdatePropertyFieldValue("Valid", "{{editedValue === '#1'}}");
cy.wait(500);
cy.get(`.t--inlined-cell-editor-has-error`).should("not.exist");
cy.enterTableCellValue(0, 0, "22");
cy.wait(500);
cy.get(`.t--inlined-cell-editor-has-error`).should("exist");
cy.enterTableCellValue(0, 0, "#1");
cy.wait(500);
cy.get(`.t--inlined-cell-editor-has-error`).should("not.exist");
cy.enterTableCellValue(0, 0, "22");
cy.get(`.t--inlined-cell-editor-has-error`).should("exist");
_.propPane.UpdatePropertyFieldValue("Valid", "");
_.propPane.UpdatePropertyFieldValue("Regex", "^#1$");
cy.wait(500);
cy.get(`.t--inlined-cell-editor-has-error`).should("not.exist");
cy.get(`.t--inlined-cell-editor-has-error`).should("exist");
cy.enterTableCellValue(0, 0, "22");
cy.wait(500);
cy.get(`.t--inlined-cell-editor-has-error`).should("exist");
cy.enterTableCellValue(0, 0, "#1");
cy.wait(500);
cy.get(`.t--inlined-cell-editor-has-error`).should("not.exist");
_.propPane.UpdatePropertyFieldValue("Regex", "");
_.propPane.TogglePropertyState("Required", "On");
cy.enterTableCellValue(0, 0, "22");
cy.wait(500);
cy.get(`.t--inlined-cell-editor-has-error`).should("not.exist");
cy.enterTableCellValue(0, 0, "#1");
cy.wait(500);
cy.get(`.t--inlined-cell-editor-has-error`).should("not.exist");
cy.enterTableCellValue(0, 0, "");
cy.wait(500);
cy.get(`.t--inlined-cell-editor-has-error`).should("exist");
cy.get(commonlocators.changeColType).last().click();
@ -56,37 +45,26 @@ describe(
_.propPane.UpdatePropertyFieldValue("Min", "5");
cy.enterTableCellValue(0, 0, "6");
cy.wait(500);
cy.get(`.t--inlined-cell-editor-has-error`).should("not.exist");
cy.enterTableCellValue(0, 0, "7");
cy.wait(500);
cy.get(`.t--inlined-cell-editor-has-error`).should("not.exist");
cy.enterTableCellValue(0, 0, "4");
cy.wait(500);
cy.get(`.t--inlined-cell-editor-has-error`).should("exist");
cy.enterTableCellValue(0, 0, "3");
cy.wait(500);
cy.get(`.t--inlined-cell-editor-has-error`).should("exist");
cy.enterTableCellValue(0, 0, "8");
cy.wait(500);
cy.get(`.t--inlined-cell-editor-has-error`).should("not.exist");
_.propPane.UpdatePropertyFieldValue("Min", "");
_.propPane.UpdatePropertyFieldValue("Max", "5");
cy.enterTableCellValue(0, 0, "6");
cy.wait(500);
cy.get(`.t--inlined-cell-editor-has-error`).should("exist");
cy.enterTableCellValue(0, 0, "7");
cy.wait(500);
cy.get(`.t--inlined-cell-editor-has-error`).should("exist");
cy.enterTableCellValue(0, 0, "4");
cy.wait(500);
cy.get(`.t--inlined-cell-editor-has-error`).should("not.exist");
cy.enterTableCellValue(0, 0, "3");
cy.wait(500);
cy.get(`.t--inlined-cell-editor-has-error`).should("not.exist");
cy.enterTableCellValue(0, 0, "8");
cy.wait(500);
cy.get(`.t--inlined-cell-editor-has-error`).should("exist");
_.propPane.UpdatePropertyFieldValue("Max", "");
@ -113,31 +91,24 @@ describe(
cy.editTableCell(0, 0);
cy.enterTableCellValue(0, 0, "3");
cy.wait(500);
cy.get(`.t--inlined-cell-editor-has-error`).should("exist");
cy.enterTableCellValue(0, 0, "2");
cy.wait(500);
cy.get(`.t--inlined-cell-editor-has-error`).should("not.exist");
cy.discardTableCellValue(0, 0);
cy.wait(500);
cy.get(".t--add-new-row").click();
cy.wait(1000);
cy.enterTableCellValue(0, 0, "3");
cy.wait(500);
cy.get(`.t--inlined-cell-editor-has-error`).should("exist");
cy.enterTableCellValue(0, 0, "2");
cy.wait(500);
cy.get(`.t--inlined-cell-editor-has-error`).should("exist");
cy.enterTableCellValue(0, 0, "1");
cy.wait(500);
cy.get(`.t--inlined-cell-editor-has-error`).should("not.exist");
cy.get(".t--discard-new-row").click({ force: true });
});
it("2.3. should test that validation is working for more than one add new row cell at a time", () => {
_.propPane.UpdatePropertyFieldValue("Valid", "{{editedValue === 1}}");
cy.get("[data-testid='t--property-pane-back-btn']").click();
cy.wait(500);
_.propPane.NavigateBackToPropertyPane();
_.table.toggleColumnEditableViaColSettingsPane("task", "v2", true, false);
_.propPane.UpdatePropertyFieldValue(
"Valid",

16
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/inline_editing_validations_spec.js
View File

@ -87,8 +87,7 @@ describe(
_.propPane.TogglePropertyState("Editable", "On");
_.propPane.UpdatePropertyFieldValue("Regex", "^#1$");
cy.editTableCell(0, 0);
cy.wait(500);
cy.get(`.t--inlined-cell-editor-has-error`).should("not.exist");
cy.get(`.t--inlined-cell-editor-has-error`).should("exist");
cy.enterTableCellValue(0, 0, "22");
cy.get(`.t--inlined-cell-editor-has-error`).should("exist");
cy.enterTableCellValue(0, 0, "#1");
@ -104,8 +103,7 @@ describe(
"{{editedValue === '#1'}}",
);
cy.editTableCell(0, 0);
cy.wait(500);
cy.get(`.t--inlined-cell-editor-has-error`).should("not.exist");
cy.get(`.t--inlined-cell-editor-has-error`).should("exist");
cy.enterTableCellValue(0, 0, "22");
cy.get(`.t--inlined-cell-editor-has-error`).should("exist");
cy.enterTableCellValue(0, 0, "#1");
@ -118,7 +116,6 @@ describe(
_.propPane.TogglePropertyState("Editable", "On");
_.propPane.TogglePropertyState("Required", "On");
cy.editTableCell(0, 0);
cy.wait(500);
cy.get(`.t--inlined-cell-editor-has-error`).should("exist");
cy.enterTableCellValue(0, 0, "22");
cy.get(`.t--inlined-cell-editor-has-error`).should("not.exist");
@ -146,8 +143,7 @@ describe(
_.propPane.UpdatePropertyFieldValue("Min", "5");
cy.editTableCell(0, 0);
cy.wait(500);
cy.get(`.t--inlined-cell-editor-has-error`).should("not.exist");
cy.get(`.t--inlined-cell-editor-has-error`).should("exist");
cy.enterTableCellValue(0, 0, "6");
cy.get(`.t--inlined-cell-editor-has-error`).should("not.exist");
cy.enterTableCellValue(0, 0, "7");
@ -172,7 +168,6 @@ describe(
_.propPane.UpdatePropertyFieldValue("Max", "5");
cy.editTableCell(0, 0);
cy.wait(500);
cy.get(`.t--inlined-cell-editor-has-error`).should("not.exist");
cy.enterTableCellValue(0, 0, "6");
cy.get(`.t--inlined-cell-editor-has-error`).should("exist");
@ -198,7 +193,6 @@ describe(
"You got error mate!!",
);
cy.editTableCell(0, 0);
cy.wait(1000);
cy.enterTableCellValue(0, 0, "123");
cy.get(".bp3-overlay.error-tooltip .bp3-popover-content").should(
"contain",
@ -224,12 +218,12 @@ describe(
cy.get(`.t--inlined-cell-editor`).should("exist");
cy.get(`.t--inlined-cell-editor-has-error`).should("exist");
cy.saveTableCellValue(0, 0);
cy.wait(500);
cy.get(`.t--inlined-cell-editor`).should("exist");
cy.get(`.t--inlined-cell-editor-has-error`).should("exist");
cy.get(widgetsPage.toastAction).should("not.exist");
cy.enterTableCellValue(0, 0, "#1");
cy.saveTableCellValue(0, 0);
cy.wait(500);
cy.get(`.t--inlined-cell-editor`).should("not.exist");
cy.get(`.t--inlined-cell-editor-has-error`).should("not.exist");
cy.get(widgetsPage.toastAction).should("be.visible");
@ -276,7 +270,7 @@ describe(
},
);
it("should check that save/discard button is disabled when there is a validation error", () => {
it("9. should check that save/discard button is disabled when there is a validation error", () => {
cy.openPropertyPane("tablewidgetv2");
cy.editColumn("step");
_.propPane.TogglePropertyState("Editable", "On");

1648
app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getEditableCellValidity.test.js
View File

File diff suppressed because it is too large Load Diff

86
app/client/src/widgets/TableWidgetV2/widget/derived.js
View File

@ -1199,62 +1199,60 @@ export default {
const validationMap = {};
editableColumns.forEach((validationObj) => {
const editedColumn = validationObj[0];
const value = validationObj[1];
editableColumns.forEach(([editedColumn, value]) => {
let isValid = true;
if (editedColumn && editedColumn.validation) {
const validation = editedColumn.validation;
/* General validations */
/**
* General validations
* 1. isColumnEditableCellValid
* 2. regex
* 3. isColumnEditableCellRequired
* 4. number/currency min/max
*/
if (
!validation.isColumnEditableCellRequired &&
!_.isNil(validation.isColumnEditableCellValid) &&
!validation.isColumnEditableCellValid
) {
isValid = false;
} else if (
validation.regex &&
!createRegex(validation.regex).test(value)
) {
isValid = false;
} else if (
validation.isColumnEditableCellRequired &&
(value === "" || _.isNil(value))
) {
validationMap[editedColumn.alias] = true;
isValid = false;
} else {
switch (editedColumn.columnType) {
case "number":
case "currency":
if (
!_.isNil(validation.min) &&
validation.min !== "" &&
validation.min > value
) {
isValid = false;
}
return;
} else if (
(!_.isNil(validation.isColumnEditableCellValid) &&
!validation.isColumnEditableCellValid) ||
(validation.regex && !createRegex(validation.regex).test(value)) ||
(validation.isColumnEditableCellRequired &&
(value === "" || _.isNil(value)))
) {
validationMap[editedColumn.alias] = false;
if (
!_.isNil(validation.max) &&
validation.max !== "" &&
validation.max < value
) {
isValid = false;
}
return;
}
/* Column type related validations */
switch (editedColumn.columnType) {
case "number":
case "currency":
if (
!_.isNil(validation.min) &&
validation.min !== "" &&
validation.min > value
) {
validationMap[editedColumn.alias] = false;
return;
}
if (
!_.isNil(validation.max) &&
validation.max !== "" &&
validation.max < value
) {
validationMap[editedColumn.alias] = false;
return;
}
break;
break;
}
}
}
validationMap[editedColumn.alias] = true;
validationMap[editedColumn.alias] = isValid;
});
return validationMap;