Skip to content

Commit

Permalink
fix: mandatory date column enforcement (#35613)
Browse files Browse the repository at this point in the history
## Description
**Problem:**
When the `isRequired` property is set for date columns in the table
widget, the validation doesn't work as expected. Users can add new rows
without filling in the date field, even though it is marked as required.
This results in rows being added with missing date values, which can
lead to incomplete or invalid data entries.

**Root Cause:**
The validation logic for the date column is currently handled within the
DateCell component. However, the isRequired validation functionality was
not implemented within this component. Additionally, the general
validation logic in the `getEditableCellValidity` function, located in
the derived.js file, does not account date cells in its isRequired
validation.

**Solution:**
To fix this issue:

**Enhance the `getEditableCellValidity` function:** Extend the existing
validation logic in getEditableCellValidity to include the date cell in
its validation, specifically checking for the isRequired property.

**Integrate with DateCell validation:** Ensure that the isRequired
validation is properly executed in conjunction with the existing date
validations inside the DateCell component. This will enforce the
requirement and prevent new rows from being added if the date field is
left empty.

**Test Plan**

https://www.notion.so/appsmith/Test-Plan-Date-Column-Marked-as-Required-Doesn-t-Enforce-Mandatory-Entry-When-Adding-New-Table-Row-c73b764af60842a188cba056bdda6d79?pvs=4

Fixes #34258

## Automation

/ok-to-test tags="@tag.All"

### 🔍 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/10453174231>
> Commit: 40fe2ea
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10453174231&attempt=1"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.All`
> Spec:
> <hr>Mon, 19 Aug 2024 13:17:23 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


## Summary by CodeRabbit

- **New Features**
- Introduced validation for "Date" column types, ensuring that required
fields are correctly enforced.

- **Enhancements**
- Improved validation logic for more accurate user feedback in date
cells.
  - Expanded support for validating "date" columns in the table widget.

- **Bug Fixes**
- Enhanced error handling to ensure proper indication of cell validity
based on new validation criteria.
  - Updated visual feedback for cell editor state in the UI.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
  • Loading branch information
jacquesikot authored Aug 20, 2024
1 parent 7c96221 commit 6628635
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 27 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,18 @@ describe("Validation flow", { tags: ["@tag.Widget", "@tag.Table"] }, () => {
cy.get(`.t--inlined-cell-editor-has-error`).should("exist");
_.propPane.UpdatePropertyFieldValue("Max", "");

// check that date isRequired validation is working
cy.get(commonlocators.changeColType).last().click();
cy.get(".t--dropdown-option").children().contains("Date").click();
cy.wait("@updateLayout");
cy.enterTableCellValue(0, 0, "");
cy.get(`.t--inlined-cell-editor-has-error`).should("exist");

// revert to Number for remainder of tests
cy.get(commonlocators.changeColType).last().click();
cy.get(".t--dropdown-option").children().contains("Number").click();
cy.wait("@updateLayout");

cy.get(".t--discard-new-row").click({ force: true });
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ describe(
agHelper.AssertCSS(
table._editCellEditor,
"border",
"1px solid rgb(255, 255, 255)",
"1px solid rgb(242, 43, 43)",
);
agHelper
.GetText(`${table._tableRow1Child3} ${locators._inputField}`)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ export const DateCell = (props: DateComponentProps) => {
isCellEditable,
isCellEditMode,
isCellVisible,
isEditableCellValid,
isHidden,
isNewRow,
isRequired,
Expand All @@ -195,6 +196,10 @@ export const DateCell = (props: DateComponentProps) => {
const [isValid, setIsValid] = useState(true);
const [showRequiredError, setShowRequiredError] = useState(false);
const contentRef = useRef<HTMLDivElement>(null);
const isCellCompletelyValid = useMemo(
() => isEditableCellValid && isValid,
[isEditableCellValid, isValid],
);

const valueInISOFormat = useMemo(() => {
if (typeof value !== "string") return "";
Expand Down Expand Up @@ -272,16 +277,16 @@ export const DateCell = (props: DateComponentProps) => {
accentColor={accentColor}
allowCellWrapping={allowCellWrapping}
className={`${hasFocus ? FOCUS_CLASS : ""} t--inlined-cell-editor ${
!isValid && "t--inlined-cell-editor-has-error"
!isCellCompletelyValid && "t--inlined-cell-editor-has-error"
}`}
compactMode={compactMode}
isEditableCellValid={isValid}
isEditableCellValid={isCellCompletelyValid}
paddedInput
textSize={textSize}
verticalAlignment={verticalAlignment}
>
<ErrorTooltip
isOpen={showRequiredError && !isValid}
isOpen={showRequiredError && !isCellCompletelyValid}
message={
validationErrorMessage ||
createMessage(INPUT_WIDGET_DEFAULT_VALIDATION_ERROR)
Expand Down
2 changes: 1 addition & 1 deletion app/client/src/widgets/TableWidgetV2/widget/derived.js
Original file line number Diff line number Diff line change
Expand Up @@ -851,7 +851,7 @@ export default {
};

let editableColumns = [];
const validatableColumns = ["text", "number", "currency"];
const validatableColumns = ["text", "number", "currency", "date"];

if (props.isAddRowInProgress) {
Object.values(props.primaryColumns)
Expand Down
42 changes: 20 additions & 22 deletions app/client/src/widgets/TableWidgetV2/widget/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2654,31 +2654,29 @@ class TableWidgetV2 extends BaseWidget<TableWidgetProps, WidgetState> {
value: string,
onSubmit?: string,
) => {
if (this.isColumnCellValid(alias)) {
const { commitBatchMetaUpdates } = this.props;

this.pushTransientTableDataActionsUpdates({
[ORIGINAL_INDEX_KEY]: this.getRowOriginalIndex(rowIndex),
[alias]: value,
});
const { commitBatchMetaUpdates } = this.props;

if (onSubmit && this.props.editableCell?.column) {
//since onSubmit is truthy this makes action truthy as well, so we can push this event
this.pushOnColumnEvent({
rowIndex: rowIndex,
action: onSubmit,
triggerPropertyName: "onSubmit",
eventType: EventType.ON_SUBMIT,
row: {
...this.props.filteredTableData[rowIndex],
[this.props.editableCell.column]: value,
},
});
}
this.pushTransientTableDataActionsUpdates({
[ORIGINAL_INDEX_KEY]: this.getRowOriginalIndex(rowIndex),
[alias]: value,
});

commitBatchMetaUpdates();
this.clearEditableCell();
if (onSubmit && this.props.editableCell?.column) {
//since onSubmit is truthy this makes action truthy as well, so we can push this event
this.pushOnColumnEvent({
rowIndex: rowIndex,
action: onSubmit,
triggerPropertyName: "onSubmit",
eventType: EventType.ON_SUBMIT,
row: {
...this.props.filteredTableData[rowIndex],
[this.props.editableCell.column]: value,
},
});
}

commitBatchMetaUpdates();
this.clearEditableCell();
};
pushClearEditableCellsUpdates = () => {
const { pushBatchMetaUpdates } = this.props;
Expand Down

0 comments on commit 6628635

Please sign in to comment.