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

dukenv0307 · 2024-01-19T05:15:10Z

Details

Fixed Issues

$ #33814
PROPOSAL: #33814 (comment)

Tests

Create a money request with the date as the default value
Inspect the network and verify that the created in the payload of API and the created of transaction in the response is formatted with the format YYYY-MM-DD HH:mm:ss

Verify that no errors appear in the JS console

Offline tests

Create a money request with the date as the default value
Check the transaction in OnyxDB and verify that the created of transaction formatted with the format YYYY-MM-DD HH:mm:ss

QA Steps

Create a money request with the date as the default value
Inspect the network and verify that the created in the payload of API and the created of transaction in the response is formatted with the format YYYY-MM-DD HH:mm:ss

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

android-1.mov

Android: mWeb Chrome

Screen.Recording.2024-01-19.at.11.52.41.mov

iOS: Native

iOS: mWeb Safari

Screen.Recording.2024-01-19.at.11.50.58.mov

MacOS: Chrome / Safari

Screen.Recording.2024-01-19.at.11.43.38.mov

MacOS: Desktop

Screen.Recording.2024-01-19.at.12.07.23.mov

…e is the current date

melvin-bot · 2024-01-19T05:15:38Z

@cubuspl42 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

cubuspl42 · 2024-01-19T08:27:01Z

src/libs/actions/IOU.js

@@ -870,6 +870,8 @@ 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 currentTime = DateUtils.getDBTime();
+    const currentCreated = created === format(new Date(currentTime), CONST.DATE.FNS_FORMAT_STRING) ? currentTime : created;


Isn't this equivalent to isSameDay?

Yes it's what we want to do.

cubuspl42 · 2024-01-19T08:39:17Z

src/libs/actions/IOU.js

@@ -870,6 +870,8 @@ 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 currentTime = DateUtils.getDBTime();


Please extract this logic to a function like...

/** * Conditionally add the time-of-day information to the request creation timestamp */ fun enrichMoneyRequestTimestamp(created) { // ... return currentCreated; }

@cubuspl42 I created a util for this.

cubuspl42 · 2024-01-19T10:47:21Z

src/libs/DateUtils.ts

+ */
+function enrichMoneyRequestTimestamp(created: string) {
+    const currentTime = getDBTime();
+    return isSameDay(new Date(created), new Date(currentTime)) ? currentTime : created;


Please test if it works when dropping the new Date(...) wrapping call

IsSameDay only apply type number and Date.

Oh, it does on 2.x indeed. I started a thread.

There's some active discussion, but it won't be actionable for this PR. We're good here.

cubuspl42 · 2024-01-19T10:48:08Z

src/libs/DateUtils.ts

+ * Return the date with full format if the created date is the current date.
+ * Otherwise return the created date.
+ */
+function enrichMoneyRequestTimestamp(created: string) {


Please state the return type explicitly

@cubuspl42 I updated the return type.

cubuspl42 · 2024-01-22T13:29:08Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

created-timestamp-android-compressed.mp4

Android: mWeb Chrome

iOS: Native

created-timestamp-ios-compressed.mp4

iOS: mWeb Safari

MacOS: Chrome / Safari

created-timestamp-web-converted.mp4

MacOS: Desktop

deetergp · 2024-01-22T21:00:37Z

I'm not seeing the created date on the transaction containing a full datetime stamp.

cubuspl42 · 2024-01-23T08:45:45Z

@deetergp This is definitely a problem! 😬

Was our testing approach incorrect?

Sorry if it's a trivial question, but why are you analyzing the response, not the request?

dukenv0307 · 2024-01-23T09:40:17Z

I just re-tested again and the created is still correct in both request and response. Correct me if I missed somethings

Screen.Recording.2024-01-23.at.16.39.16.mov

cubuspl42 · 2024-01-23T10:09:02Z

src/libs/DateUtils.ts

+    const currentTime = getDBTime();
+    return isSameDay(new Date(created), new Date(currentTime)) ? currentTime : created;


I think we might have some timezone issues here...

getDBTime returns a time in UTC+0, but it is cleared of the timezone information... I don't think we should use its output at all for any time calculations.

I think what happens here is a round-trip of date conversions: local -> UTC -> local, or something like that...

We should do something like this if I'm not mistaken.

const now = new Date(); return isSameDate(new Date(created), now) ? getDBTimeFromDate(now) : created;

getDBTimeFromDate is the second half of getDBTime, extracted.

cubuspl42 · 2024-01-23T10:11:09Z

@dukenv0307 Please test the PR by faking the timezone and time so that locally, the date is different from that in UTC+0.

deetergp · 2024-01-23T19:52:25Z

I'm not sure what I'm doing wrong here. I click FAB > Request Money > enter an amount > choose a recipient > the send it, and the request still only shows a YYYY-MM-DD created date.

I've double checked that I'm on the right branch and that npm i was run and didn't throw any errors.

deetergp · 2024-01-23T20:13:54Z

Oh, I think I understand what's going on. Are both of you in UTC? The issue for me is that isSameDay is returning false because the DB is in UTC, but my browser is in PST, so isSameDay is returning false:

Edit: I the time zones aren't (yet) the issue… I think that isSameDay won't return true when one side is a date and the other is a datetime.

cubuspl42 · 2024-01-24T09:02:42Z

@deetergp I'm UTC+1; I think @dukenv0307 is close.

There's no such thing as distinguished Date/DateTime in JS, I think; it's always the Date class, which represents the datetime (i.e. date + time-of-day + timezone marker). When you parse 2023-01-01 to Date, you'll get the instant of UTC midnight for that date in your local timezone.

This is a giant mess and that's why I voted to go with Yoda Time instead of date-fns. Well, I was in the minority.

dukenv0307 · 2024-01-24T09:04:09Z

@cubuspl42 I just tested and we're right. the problem is we compare two date with different timezone. Updated to fix this.

cubuspl42 · 2024-01-24T09:04:50Z

src/libs/DateUtils.ts

+    const currentTime = getDBTime();
+    return formatWithUTCTimeZone(created) === formatWithUTCTimeZone(currentTime) ? currentTime : created;


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.

@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.

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.

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

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

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

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;

@cubuspl42 I tested and updated this.

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.

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.

cubuspl42 · 2024-01-24T11:53:15Z

src/libs/DateUtils.ts

@@ -730,6 +730,23 @@ function formatToSupportedTimezone(timezoneInput: Timezone): Timezone {
    };
 }

+/**
+ * Returns the time in milliseconds of a date in the format expected by the database


Time in milliseconds?

Updated comment.

cubuspl42 · 2024-01-24T11:53:35Z

src/libs/DateUtils.ts

+/**
+ * Returns the time in milliseconds of a date in the format expected by the database
+ */
+function getDBTimeFromDate(date: Date): string {


Well, please use it inside getDBTime now

Updated to use getDBTimeFromDate in getDBTime.

cubuspl42 · 2024-01-25T10:45:56Z

src/libs/DateUtils.ts

@@ -360,12 +360,19 @@ function getMicroseconds(): number {
    return Date.now() * CONST.MICROSECONDS_PER_MS;
 }

+/**
+ * Returns the format yyyy-MM-dd HH:mm:ss of a date in the format expected by the database


Okey, let's nuke this comment (as it's a helper function related to getDBTime) and update the original comment so it makes sense.

👉 #34784 (comment)

I didn't realize that you aren't the author of the phrase I commented earlier

Sound good, updated the comment.

cubuspl42 · 2024-01-25T10:46:43Z

src/libs/DateUtils.ts

+function getDBTimeFromDate(date: Date): string {
+    return date.toISOString().replace('T', ' ').replace('Z', '');
+}
+
 /**
 * Returns the current time in milliseconds in the format expected by the database


/** * Convert the given timestamp to the "yyyy-MM-dd HH:mm:ss" format, as expected by the database * * @param [timestamp] the given timestamp (if omitted, defaults to the current time) */

cubuspl42 · 2024-01-25T11:14:29Z

I tested this with timezone spoofing and I believe the logic is timezone-proof now!

@deetergp Would you confirm it fixed the problem for you?

deetergp · 2024-01-29T19:37:40Z

@cubuspl42 Yep, it fixed the issue for me! The money request I made created a transaction in the data base with UTC timestamp of when I created it, in the expected format.

deetergp

Looks good and tests well, thanks for the changes.

OSBotify · 2024-01-29T19:43:27Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2024-01-31T02:57:41Z

🚀 Cherry-picked to staging by https://github.com/francoisl in version: 1.4.33-5 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

OSBotify · 2024-01-31T04:11:38Z

🚀 Deployed to staging by https://github.com/deetergp in version: 1.4.34-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	cancelled 🔪
🕸 web 🕸	success ✅

OSBotify · 2024-01-31T04:37:55Z

🚀 Deployed to staging by https://github.com/deetergp in version: 1.4.34-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2024-01-31T22:48:48Z

🚀 Deployed to production by https://github.com/francoisl in version: 1.4.34-1 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

format created of transaction with date time value if the created dat…

d3998d4

…e is the current date

dukenv0307 marked this pull request as ready for review January 19, 2024 05:15

dukenv0307 requested a review from a team as a code owner January 19, 2024 05:15

melvin-bot bot requested review from cubuspl42 and removed request for a team January 19, 2024 05:15

cubuspl42 suggested changes Jan 19, 2024

View reviewed changes

implement util function for get current date of money request

030d802

cubuspl42 reviewed Jan 19, 2024

View reviewed changes

add return type explicitly

1e0bb54

cubuspl42 approved these changes Jan 22, 2024

View reviewed changes

melvin-bot bot requested a review from deetergp January 22, 2024 13:29

Merge branch 'main' into fix/33814

56b9b5b

cubuspl42 reviewed Jan 23, 2024

View reviewed changes

Merge branch 'main' into fix/33814

ac00d37

convert to the same timezone before compare

120c9ce

cubuspl42 reviewed Jan 24, 2024

View reviewed changes

refactor compare date logic

e7642f6

cubuspl42 reviewed Jan 24, 2024

View reviewed changes

merge main and edit comment

4908662

cubuspl42 suggested changes Jan 25, 2024

View reviewed changes

edit the original comment

6e49257

cubuspl42 approved these changes Jan 25, 2024

View reviewed changes

dukenv0307 added 3 commits January 26, 2024 11:37

Merge branch 'main' into fix/33814

8e7dc14

merge main

b25f0c7

Merge branch 'main' into fix/33814

e3472ec

deetergp approved these changes Jan 29, 2024

View reviewed changes

deetergp merged commit 6198343 into Expensify:main Jan 29, 2024
15 checks passed

github-actions bot mentioned this pull request Jan 31, 2024

Deploy Checklist: New Expensify 2024-01-31 #35448

Closed

54 tasks

melvin-bot bot mentioned this pull request Jan 31, 2024

[HOLD for payment 2024-02-07] [$500] [WAVE 5] When inserting a manual request into the database, it should store the full YYYY-MM-DD HH:MM:SS in the created field. #33814

Closed

6 tasks

pecanoro mentioned this pull request Jun 7, 2024

Revert using timestamps for created when submitting money requests #43319

Merged

50 tasks

		const currentTime = getDBTime();
		return isSameDay(new Date(created), new Date(currentTime)) ? currentTime : created;

		const currentTime = getDBTime();
		return formatWithUTCTimeZone(created) === formatWithUTCTimeZone(currentTime) ? currentTime : created;

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

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

Conversation

dukenv0307 commented Jan 19, 2024

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Jan 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dukenv0307 Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

cubuspl42 commented Jan 22, 2024

Reviewer Checklist

Screenshots/Videos

deetergp commented Jan 22, 2024

cubuspl42 commented Jan 23, 2024

dukenv0307 commented Jan 23, 2024

Choose a reason for hiding this comment

cubuspl42 commented Jan 23, 2024

deetergp commented Jan 23, 2024 • edited Loading

deetergp commented Jan 23, 2024 • edited Loading

cubuspl42 commented Jan 24, 2024 • edited Loading

dukenv0307 commented Jan 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cubuspl42 commented Jan 25, 2024

deetergp commented Jan 29, 2024

deetergp left a comment

Choose a reason for hiding this comment

OSBotify commented Jan 29, 2024

OSBotify commented Jan 31, 2024

OSBotify commented Jan 31, 2024

OSBotify commented Jan 31, 2024

OSBotify commented Jan 31, 2024

dukenv0307 Jan 19, 2024 •

edited

Loading

deetergp commented Jan 23, 2024 •

edited

Loading

deetergp commented Jan 23, 2024 •

edited

Loading

cubuspl42 commented Jan 24, 2024 •

edited

Loading