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.

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 2023-08-30] [$1000] Date is incorrect after editing money request #25441

Closed
6 tasks done
luacmartins opened this issue Aug 17, 2023 · 43 comments
Closed
6 tasks done
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@luacmartins
Copy link
Contributor

luacmartins commented Aug 17, 2023

cc @mountiny

Action Performed:

  1. Request money from another user
  2. Edit the money request date
  3. Verify that the selected date is incorrect after you tap Save

Expected Result:

Date is correct

Actual Result:

Date is incorrect

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number:
Reproducible in staging?:
Reproducible in production?:
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
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:
Issue reported by:
Slack conversation:

View all open jobs on GitHub

Screen.Recording.2023-08-17.at.5.05.01.PM.mov
Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0154a57cff1cce910b
  • Upwork Job ID: 1692296957389250560
  • Last Price Increase: 2023-08-17
  • Automatic offers:
    • ShogunFire | Contributor | 26196211
@luacmartins luacmartins added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 17, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 17, 2023

Triggered auto assignment to @stephanieelliott (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot
Copy link

melvin-bot bot commented Aug 17, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@luacmartins luacmartins self-assigned this Aug 17, 2023
@luacmartins luacmartins added the External Added to denote the issue can be worked on by a contributor label Aug 17, 2023
@melvin-bot melvin-bot bot changed the title Date is incorrect after editing money request [$1000] Date is incorrect after editing money request Aug 17, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 17, 2023

Job added to Upwork: https://www.upwork.com/jobs/~0154a57cff1cce910b

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 17, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 17, 2023

Current assignee @stephanieelliott is eligible for the External assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented Aug 17, 2023

Triggered auto assignment to Contributor-plus team member for initial proposal review - @ArekChr (External)

@ShogunFire
Copy link
Contributor

ShogunFire commented Aug 17, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

After editing a money request the date is incorrect

What is the root cause of that problem?

The root cause is that we are using TransactionUtils.getCreated to get the date and in that function we transform the string representing the date to a date object using new Date(). In javascript using new Date without the time can make the date component not use the local timezone (the timezone depends of the format) which can result in having a day off.

What changes do you think we should make in order to solve the problem?

We can use the method parseISO of date-fns to parse the date like this

const createdDate = parseISO(created)

if (createdDate) {
    return format(createdDate, CONST.DATE.FNS_FORMAT_STRING);
}

What alternative solutions did you explore? (Optional)

We can add the time behind the date like this T00:00 to force it to use the local date

@mountiny
Copy link
Contributor

Wow nice @ShogunFire

We can also pass the date with a format that avoid this weird behaviour, for example we can transform our string like this yyyy/mm/dd by replacing - with /

I think this is interesting but just curios if there is anything else we can use without some special treatment. Is there some param we can pass to the Date() maybe? Or use something from the date-fns? cc @waterim

@waterim
Copy link
Contributor

waterim commented Aug 17, 2023

@ShogunFire can you please provide the string which is going to the new Date()?

@pradeepmdk
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

After editing a money request the date is incorrect

What is the root cause of that problem?

timezone issue

What changes do you think we should make in order to solve the problem?

instead of getting hard string API we can get utc format and locally we can convert the local date
currently we are using modifiedCreated
Screenshot 2023-08-18 at 4 48 04 AM

@waterim
Copy link
Contributor

waterim commented Aug 17, 2023

@ShogunFire you can try date-fns format() function to format the string

@mountiny
Copy link
Contributor

I think its 2023-08-18 for example, whatever the DatePicker component returns.

@pradeepmdk
Copy link
Contributor

@waterim already we are formatting https://github.com/Expensify/App/blob/main/src/libs/TransactionUtils.js?#L176

@ShogunFire
Copy link
Contributor

I am trying with parse from date-fns

@waterim
Copy link
Contributor

waterim commented Aug 17, 2023

@ShogunFire you can try to convert to UTC and after add an offset, or just with date-fns-tz use a UTCtoTimezone function
There are some issues with pure dates, because it applies time like “00:00” and adds an offset of your local timezone

@ShogunFire
Copy link
Contributor

ShogunFire commented Aug 17, 2023

Ok so the parse function works well for date alone but in some cases it looks like we also have the time so it fails to parse, Maybe we can try two times to parse ? Like if it fails for date alone we try with the other format

EDIT: Trying with parseISO

@pradeepmdk
Copy link
Contributor

pradeepmdk commented Aug 17, 2023

can any one clarify me modifiedCreated or created time coming from api its converted to user localtimezone?

@ShogunFire
Copy link
Contributor

Proposal

Updated

Seems to work well with parseISO method

@ShogunFire
Copy link
Contributor

@pradeepmdk The date that comes from the api is the date that we sent to the api so it is supposed to be local, the issue is it's not

@waterim
Copy link
Contributor

waterim commented Aug 17, 2023

@ShogunFire yes, parsoISO is nice one, to prevent any local offsets adding

@tienifr
Copy link
Contributor

tienifr commented Aug 18, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

Date is incorrect after editing money request

What is the root cause of that problem?

I think the way we're designing the editing of the money request is not ideal. Let's say when I select 2023-08-18 as the timezone, it will be saved as such in the db, assuming that that's the date for my local time. The problem with this approach is that my local time is not something permanent.

Let's say when I'm in timezone GMT +8, and edit the money request to 2023-08-18.

Then I move to timezone GMT -7, the money request is still 2023-08-18 because there's no timezone association in the database. This is wrong since the 2023-08-18 of my intention originally should be 2023-08-17 in my new GMT -7, and it should show 2023-08-17 in the app.

So when we're storing any date, the correct and robust way to do it is to either:

  1. Always store it with the timezone information
  2. Store it as UTC always, then convert to local time when we display it. (This is what we have done for all other dates of reports, report actions, ... also including the created of the money request)

This ambiguity in the time stored causes this issue. Since when we gets the date from the database, it has no timezone so in here, the Date constructor will assume it's UTC.

What changes do you think we should make in order to solve the problem?

  1. We need to store the date as UTC when we edit it here, by converting transactionChanges.created to our DBTime (which is UTC). The key here is to convert it to a non-ambiguous date type as soon as possible in our codebase.

For example, if I select 2023-08-18 as the edited request money date, and I'm in GMT+8 timezone:

  • Currently, it will send 2023-08-18 as the created to the back-end, which is ambiguous and will be wrong if I change my timezone
  • After the change, it should send 2023-08-17 16:00:00.000 to the back-end since it's the db time in UTC
  1. There'll be other changes in the util functions and when we display it to make sure it displays as YYYY-MM-DD properly with the new modifiedDate format.
  2. There'll be some minor changes in BE required to accept the new correct date format as well.

What alternative solutions did you explore? (Optional)

NA

@mountiny mountiny changed the title [$1000] Date is incorrect after editing money request [HOLD for payment 2023-08-30] [$1000] Date is incorrect after editing money request Sep 4, 2023
@mountiny mountiny added Daily KSv2 Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 Daily KSv2 labels Sep 4, 2023
@melvin-bot melvin-bot bot added Daily KSv2 and removed Daily KSv2 labels Sep 11, 2023
@ShogunFire
Copy link
Contributor

Friendly bump @JmillsExpensify

@melvin-bot melvin-bot bot added the Overdue label Sep 15, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 18, 2023

@JmillsExpensify, @ShogunFire, @ArekChr, @mountiny Eep! 4 days overdue now. Issues have feelings too...

@sophiepintoraetz
Copy link
Contributor

Confirming that the payment due to @ShogunFire (Pierre M) is $1500 here, correct? @JmillsExpensify @mountiny?

@melvin-bot
Copy link

melvin-bot bot commented Sep 20, 2023

@JmillsExpensify, @ShogunFire, @ArekChr, @mountiny Still overdue 6 days?! Let's take care of this!

@mountiny
Copy link
Contributor

@sophiepintoraetz that is correct, thanks for helping

@melvin-bot melvin-bot bot removed the Overdue label Sep 21, 2023
@sophiepintoraetz
Copy link
Contributor

Payment done - is there anything else keeping this issue open?

@JmillsExpensify
Copy link

Ah so sorry for the delay on this one. Don't hesitate to reach out to me via New Expensify DM.

@JmillsExpensify
Copy link

In any case, @ArekChr I don't see a BZ checklist completed for this issue? Do you mind completing one, including whether a regression test is required? Thank you!

@ArekChr
Copy link
Contributor

ArekChr commented Sep 21, 2023

  • Link to the PR: No bug identified.
  • Link to comment: n/a
  • Link to discussion: n/a
  • Determine if we should create a regression test for this bug: I don’t think we need to add a regression test here.

@melvin-bot melvin-bot bot added the Overdue label Sep 25, 2023
@JmillsExpensify
Copy link

Great thank you! All done here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

10 participants