Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Invalid Date Display in Table Widget's Date Column When Using Unix Timestamp (ms) #36455

Open
wants to merge 7 commits into
base: release
Choose a base branch
from
+128 −5
Open

fix: Invalid Date Display in Table Widget's Date Column When Using Unix Timestamp (ms) #36455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5ab2834
adjust moment inputFormat for Unix ms + allow validate unix ms in tra…
jacquesikot Sep 19, 2024
1a4ebe9
add enum for MomentDateInputFormat
jacquesikot Sep 19, 2024
c97031f
Merge branch 'release' of https://github.com/appsmithorg/appsmith int…
jacquesikot Sep 19, 2024
6b6b7a8
add base test
jacquesikot Sep 19, 2024
94ef32d
improve test
jacquesikot Sep 19, 2024
da89194
improve Date_column_editing_2_spec
jacquesikot Sep 20, 2024
2621b9a
complete test
jacquesikot Sep 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 109 additions & 0 deletions app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/Date_column_editing_2_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,5 +92,114 @@ describe(
expect($textData).to.include("YYYY-MM-DDTHH:mm:ss.SSSZ"),
);
});

it("4. should allow input format of Unix Timestamp(ms) and not throw Invalid Value error when inline editing", () => {
// Get string format for tomorrow date - Sat Sep 21 2024 format
const tomorrow = new Date();
tomorrow.setDate(tomorrow.getDate() + 1);
const formattedTomorrowDateVerbose = tomorrow
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please add this in helper function with better parameters to make a good usage of it?

.toLocaleDateString("en-US", {
weekday: "short",
year: "numeric",
month: "short",
day: "2-digit",
})
.replace(/,/g, "");

// Get strig format for tomorrow date - 2024-09-21
const formattedTomorrowDateYYYYMMDD = tomorrow
.toISOString()
.split("T")[0];

EditorNavigation.SelectEntityByName("Table1", EntityType.Widget);

propPane.NavigateBackToPropertyPane();

// Update table data
propPane.UpdatePropertyFieldValue(
"Table data",
`
{{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please existing fixture here? or atleast add this in fixture folder and update in test.

[
{
unix: 1726713837918,
role: 1,
id: 1,
name: "Alice Johnson",
email: "[email protected]",
age: 28,
gender: 2,
},
{
unix: 1726713837918,
role: 2,
id: 2,
name: "Bob Smith",
email: "[email protected]",
age: 34,
gender: 1
},
{
unix: 1726713837918,
role: 3,
id: 3,
name: "Charlie Brown",
email: "[email protected]",
age: 25,
gender: 3
},
{
unix: 1726713837918,
role: 2,
id: 4,
name: "Diana Prince",
email: "[email protected]",
age: 30,
gender: 2
},
{
unix: 1726713837918,
role: 1,
id: 5,
name: "Evan Williams",
email: "[email protected]",
age: 27,
gender: 1
}
]
}}
`,
);

// Change column to date
table.ChangeColumnType("unix", "Date", "v2");

// Edit column
table.EditColumn("unix", "v2");

// Update date format property
propPane.ToggleJSMode("Date format", true);
propPane.UpdatePropertyFieldValue("Date format", "Milliseconds");

// Update display format property
propPane.ToggleJSMode("Display format", true);
propPane.UpdatePropertyFieldValue("Display format", "YYYY-MM-DD");

// Toggle editable
propPane.TogglePropertyState("Editable", "On");

// Click unix cell edit
table.ClickOnEditIcon(0, 2);

// Click on specific date within
agHelper.GetNClick(
`${table._dateInputPopover} [aria-label='${formattedTomorrowDateVerbose}']`,
);

// Check that date is set in column
table
.ReadTableRowColumnData(0, 2, "v2")
.then((val) => expect(val).to.equal(formattedTomorrowDateYYYYMMDD));
});
},
);
14 changes: 11 additions & 3 deletions app/client/src/widgets/TableWidgetV2/component/cellComponents/DateCell.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,11 @@ import DateComponent from "widgets/DatePickerWidget2/component";
import { TimePrecision } from "widgets/DatePickerWidget2/constants";
import type { RenderDefaultPropsType } from "./PlainTextCell";
import styled from "styled-components";
import { EditableCellActions } from "widgets/TableWidgetV2/constants";
import {
DateInputFormat,
EditableCellActions,
MomentDateInputFormat,
} from "widgets/TableWidgetV2/constants";
import { ISO_DATE_FORMAT } from "constants/WidgetValidation";
import moment from "moment";
import { BasicCell } from "./BasicCell";
Expand Down Expand Up @@ -218,6 +222,7 @@ export const DateCell = (props: DateComponentProps) => {
}, [value, props.outputFormat]);

const onDateSelected = (date: string) => {
let momentAdjustedInputFormat = inputFormat;
if (isNewRow) {
updateNewRowValues(alias, date, date);

Expand All @@ -235,8 +240,11 @@ export const DateCell = (props: DateComponentProps) => {
setShowRequiredError(false);
setHasFocus(false);

const formattedDate = date ? moment(date).format(inputFormat) : "";

if (inputFormat === DateInputFormat.MILLISECONDS)
momentAdjustedInputFormat = MomentDateInputFormat.MILLISECONDS;
Comment on lines +243 to +244
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is rationale behind this condition? Its unclear; add comment.
also, we have an if statement, lets put it inside block scope {}.

const formattedDate = date
? moment(date).format(momentAdjustedInputFormat)
: "";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Query : Blank value return means error will come or what will be the output?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

empty value will come. essentially, the date itself is not provided, it is a falsey value(null/undefined/""/0)

onDateSave(rowIndex, alias, formattedDate, onDateSelectedString);
};

Expand Down
4 changes: 4 additions & 0 deletions app/client/src/widgets/TableWidgetV2/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,10 @@ export enum DateInputFormat {
MILLISECONDS = "Milliseconds",
}

export enum MomentDateInputFormat {
MILLISECONDS = "x",
}

export const defaultEditableCell: EditableCell = {
column: "",
index: -1,
Expand Down
6 changes: 4 additions & 2 deletions app/client/src/widgets/TableWidgetV2/widget/reactTableUtils/transformDataPureFn.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,16 @@ export const transformDataPureFn = (
const type = _.isArray(column.metaProperties.inputFormat)
? column.metaProperties.inputFormat[rowIndex]
: column.metaProperties.inputFormat;

if (
type !== DateInputFormat.EPOCH &&
type !== DateInputFormat.MILLISECONDS
) {
inputFormat = type;
moment(value as MomentInput, inputFormat);
} else if (!isNumber(value)) {
} else if (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition and the condition above is confusing, can we simplify this logic?
Also, the top of this function there is a comment stating the need to unit test this, while we are at it, lets take care of that as well.
One suggestion, right now the variable type is of any, which is not ideal, either declare an interface for it, or explain in comments what all values can it assume.

!isNumber(value) &&
type !== DateInputFormat.MILLISECONDS
) {
isValidDate = false;
}
} catch (e) {
Expand Down
Loading