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

Format created of transaction with date time value if the created date is the current date #34784

Merged
merged 12 commits into from
Jan 29, 2024
Merged

Format created of transaction with date time value if the created date is the current date #34784

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
d3998d4
format created of transaction with date time value if the created dat…
dukenv0307 Jan 19, 2024
030d802
implement util function for get current date of money request
dukenv0307 Jan 19, 2024
1e0bb54
add return type explicitly
dukenv0307 Jan 19, 2024
56b9b5b
Merge branch 'main' into fix/33814
dukenv0307 Jan 23, 2024
ac00d37
Merge branch 'main' into fix/33814
dukenv0307 Jan 24, 2024
120c9ce
convert to the same timezone before compare
dukenv0307 Jan 24, 2024
e7642f6
refactor compare date logic
dukenv0307 Jan 24, 2024
4908662
merge main and edit comment
dukenv0307 Jan 25, 2024
6e49257
edit the original comment
dukenv0307 Jan 25, 2024
8e7dc14
Merge branch 'main' into fix/33814
dukenv0307 Jan 26, 2024
b25f0c7
merge main
dukenv0307 Jan 26, 2024
e3472ec
Merge branch 'main' into fix/33814
dukenv0307 Jan 26, 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
10 changes: 10 additions & 0 deletions src/libs/DateUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -730,6 +730,15 @@ function formatToSupportedTimezone(timezoneInput: Timezone): Timezone {
};
}

/**
* Return the date with full format if the created date is the current date.
* Otherwise return the created date.
*/
function enrichMoneyRequestTimestamp(created: string): string {
const currentTime = getDBTime();
return formatWithUTCTimeZone(created) === formatWithUTCTimeZone(currentTime) ? currentTime : created;
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I won't happily approve more smelly approaches to date operations. Let's stick with the best thing we got, using date-fns as much as possible.

You'll have to convince me why that's not the good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cubuspl42 We already have formatWithUTCTimeZone function and this function use date-fns-tz lib as well so I think it's good to use this function.

Copy link
Contributor

Choose a reason for hiding this comment

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

getDBTime creates a Date, converts it to string, screws up the ISO format, then we pass it to formatWithUTCTimeZone, which again parses the string to Date, dumps it to string, and then we compare it.

It's a nightmare.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, is this even correct? Aren't we ultimately forced to use the "DB time" format?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So timezone we should use. the current user timezone or UTC timezone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To avoid the created string invalid date we can use isValid function of date-fns as well

Copy link
Contributor

Choose a reason for hiding this comment

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

Okey, let's try...

const now = new Date();
const createdDate = parse(created, CONST.DATE.FNS_FORMAT_STRING, now);
return isSameDate(createdDate, now) ? getDBTimeFromDate(now) : created;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cubuspl42 I tested and updated this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Time zones are the worst. Just to clarify, all datetimes that get saved to our back end should be stored as UTC strings in the YYYY-MM-DD HH:MM:SS format. But the kicker is that when displaying them back to the user, we probably need to show it relative to their time zone.

Copy link
Contributor

Choose a reason for hiding this comment

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

Timezones are the worst, true, but we're not helping ourselves by making very questionable decisions like passing dates around as strings, converting Dates to string to Dates to strings to Date..., not using separate types for instants and dates and datetimes, etc., etc.

}

const DateUtils = {
formatToDayOfWeek,
formatToLongDateWithWeekday,
Expand Down Expand Up @@ -774,6 +783,7 @@ const DateUtils = {
getWeekEndsOn,
isTimeAtLeastOneMinuteInFuture,
formatToSupportedTimezone,
enrichMoneyRequestTimestamp,
};

export default DateUtils;
10 changes: 6 additions & 4 deletions src/libs/actions/IOU.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -870,6 +870,7 @@ function createDistanceRequest(report, participant, comment, created, category,
// If the report is an iou or expense report, we should get the linked chat report to be passed to the getMoneyRequestInformation function
const isMoneyRequestReport = ReportUtils.isMoneyRequestReport(report);
const currentChatReport = isMoneyRequestReport ? ReportUtils.getReport(report.chatReportID) : report;
const currentCreated = DateUtils.enrichMoneyRequestTimestamp(created);

const optimisticReceipt = {
source: ReceiptGeneric,
Expand All @@ -881,7 +882,7 @@ function createDistanceRequest(report, participant, comment, created, category,
comment,
amount,
currency,
created,
currentCreated,
merchant,
userAccountID,
currentUserEmail,
Expand All @@ -906,7 +907,7 @@ function createDistanceRequest(report, participant, comment, created, category,
createdIOUReportActionID,
reportPreviewReportActionID: reportPreviewAction.reportActionID,
waypoints: JSON.stringify(validWaypoints),
created,
created: currentCreated,
category,
tag,
billable,
Expand Down Expand Up @@ -1275,14 +1276,15 @@ function requestMoney(
// If the report is iou or expense report, we should get the linked chat report to be passed to the getMoneyRequestInformation function
const isMoneyRequestReport = ReportUtils.isMoneyRequestReport(report);
const currentChatReport = isMoneyRequestReport ? ReportUtils.getReport(report.chatReportID) : report;
const currentCreated = DateUtils.enrichMoneyRequestTimestamp(created);
const {payerAccountID, payerEmail, iouReport, chatReport, transaction, iouAction, createdChatReportActionID, createdIOUReportActionID, reportPreviewAction, onyxData} =
getMoneyRequestInformation(
currentChatReport,
participant,
comment,
amount,
currency,
created,
currentCreated,
merchant,
payeeAccountID,
payeeEmail,
Expand All @@ -1305,7 +1307,7 @@ function requestMoney(
amount,
currency,
comment,
created,
created: currentCreated,
merchant,
iouReportID: iouReport.reportID,
chatReportID: chatReport.reportID,
Expand Down
Loading