-
Notifications
You must be signed in to change notification settings - Fork 3k
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.
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
[HOLD for payment 2024-01-24] [$500] IOU - The date does not change and a message with incorrect dates is added #33248
Comments
Triggered auto assignment to @dylanexpensify ( |
Job added to Upwork: https://www.upwork.com/jobs/~01a1217caf58eef97c |
Bug0 Triage Checklist (Main S/O)
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @getusha ( |
Issue is not reproducible on my account on staging |
1 similar comment
Issue is not reproducible on my account on staging |
📣 @neoneww! 📣
|
Proposalfrom: @unicorndev-727 Please re-state the problem that we are trying to solve in this issue.Previously created date doesn't show correctly. What is the root cause of that problem?The root cause is that we didn't consider timezone when creating date here Line 2089 in bb6bc9c
What changes do you think we should make in order to solve the problem?We should update this line to.
What alternative solutions did you explore? (Optional)We need to save created and old created in originMessage in "yyyy-MM-dd'T'HH:mm:ss'Z'" when creating and updating reportAction. |
Hello |
This comment was marked as resolved.
This comment was marked as resolved.
✅ Contributor details stored successfully. Thank you for contributing to Expensify! |
ProposalPlease re-state the problem that we are trying to solve in this issue.IOU - The date does not change and a message with incorrect dates is added What is the root cause of that problem?Line 2126 in e17c313
(Edit: now this is moved to here: App/src/libs/ModifiedExpenseMessage.ts Lines 149 to 150 in 7b836cf
Here, What changes do you think we should make in order to solve the problem?use
We already applied same solution in other places (#30845, #30923) |
Contributor details |
|
Contributor details |
✅ Contributor details stored successfully. Thank you for contributing to Expensify! |
ProposalPlease re-state the problem that we are trying to solve in this issue.IOU - The date does not change and a message with incorrect dates is added What is the root cause of that problem?The problem is related to the process of creating and formatting the modified date because it created a Note: people east of GMT (most non-US) may not see this bug because There is a condition in the App/src/pages/home/report/ReportActionItem.js Lines 429 to 431 in 4a9914f
As you can see, the Lines 2123 to 2130 in 4a9914f
In this case, the For example, the What changes do you think we should make in order to solve the problem?We should use the same approach used in this issue #30303, using the method Here you can see the Lines 654 to 669 in 4a9914f
Here's an example of how we can change the code to fix this issue: function getModifiedExpenseMessage(reportAction: OnyxEntry<ReportAction>): string | undefined {
...
if (reportActionOriginalMessage?.oldCreated && reportActionOriginalMessage?.created) {
const formattedOldCreated = DateUtils.formatWithUTCTimeZone(reportActionOriginalMessage?.oldCreated, CONST.DATE.FNS_FORMAT_STRING);
return getProperSchemaForModifiedExpenseMessage(reportActionOriginalMessage?.created, formattedOldCreated, Localize.translateLocal('common.date'), false);
}
...
} Note: with this suggestion above we can improve the type checking, avoiding passing a default value (in this case Zero here at the end of the line) if What alternative solutions did you explore? (Optional)N/A POC poc-issue-33248.mp4 |
✅ Contributor details stored successfully. Thank you for contributing to Expensify! |
@getusha mind giving an update? |
I already tried to find other places. I hope this would be the last one 🙏 as part of moment > date-fns migration refactor |
Hi @youssef-lr maybe the reason you weren't able to reproduce this bug is related to the note I put in my #33248 (comment)
|
Hi, @youssef-lr I found two places with the code that could possibly have the same time zone issue. In the file App/src/libs/ModifiedExpenseMessage.ts Lines 146 to 160 in 4cfb8a2
Here in this image, we can see the value format in the The other place is in the DateUtils file where we can see many places creating a new Date object with the statement If you have any questions, please let me know. |
This is what we're trying to fix here. Your proposal is deprecated. Code was moved in #30603 |
📣 @getusha 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app! |
📣 @wlegolas 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
@wlegolas can we please standardize using the same format function everywhere? |
Sure, I'll change the files to use the same function. Thank you @youssef-lr for reviewed my proposal |
Hi @youssef-lr, @getusha I created the PR #33688 with the changes. Note: I didn't put the evidence for "Android: Native" because I've been having problems running the application in my simulator, but I'll generate this evidence and update the PR description. When I was fixing this issue I realized that the code to format the With this change, I only need to fix the issue in the If you have any questions or suggestions, feel free to bring them. |
Hi @youssef-lr and @getusha I put the last evidence for Android Native. I have a question about this implementation, in my tests, I noticed both dates Do you know why we need to convert the value from the backend to a Date object and format this Date object to the format |
@getusha @youssef-lr mind confirming |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.25-10 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2024-01-24. 🎊 For reference, here are some details about the assignees on this issue:
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
coming up! |
tomorrow! |
payments sent! |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: 1.4.13-8
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:
Issue found when executing PR #33174
Action Performed:
Expected Result:
Verify the date is changed, and a proper message is added, e.g. changed the date to 2023-12-17 (previously 2023-12-18)
Actual Result:
The date does not change and a message with incorrect dates is added
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Bug6318356_1702919179197.Recording__743.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: