Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.33k stars 2.76k forks source link

[$250] Timezone - The automatic time zone is selected with the incorrect spelling "Kiev" #16893

Closed kbecciv closed 1 year ago

kbecciv commented 1 year ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Issue found when executing PR https://github.com/Expensify/App/pull/16777

Action Performed:

Preconditional: user should be located in Europe/Ukraine

  1. Go to URL https://staging.new.expensify.com/
  2. Login with any account
  3. Go to Settings -> Profile -> Timezone
  4. Disable the option "Automatically determine your location"
  5. Click on the timezone selector below
  6. In the search input, search for Kyiv
  7. Enable the option "Automatically determine your location"

Expected Result:

The time zone is set with the correct spelling - Europa/Kyiv

Fix this upstream in moment repo

Actual Result:

A time zone with incorrect spelling is being set - Europe/Kiev

Workaround:

Unknown

Platforms:

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

Version Number: 1.2.94.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

https://user-images.githubusercontent.com/93399543/229640824-5b08cc09-2920-4cbe-af6f-fcd2dba35509.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~010a8578e77393b86c
  • Upwork Job ID: 1643659043634610176
  • Last Price Increase: 2023-04-13
MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

Bug0 Triage Checklist (Main S/O)

strepanier03 commented 1 year ago

Working through this now.

strepanier03 commented 1 year ago

Saving - https://en.wikipedia.org › wiki › KyivNotKiev

strepanier03 commented 1 year ago

I cannot test this so I raised in Slack here to get a buddy check test going.

If no one responds by the time I get back to GHs tomorrow I'll just move forward.

thienlnam commented 1 year ago

I imagine this is coming from the moment-timezone API, I don't think we're modifying it on our end

Prince-Mendiratta commented 1 year ago

@strepanier03 It isn't necessary to be located in Ukraine to test this issue, you can test the automatic timezone resolution by setting your device timezone to Europe/Kyiv.

As for the RCA and solution to this issue, it is as pointed out by @thienlnam, it relies on moment-timezone API. It is because of 2 issues:

  1. We are using an outdated version of moment-timezone, which makes use of an outdated version of the IANA database (2020), which is the underlying database for moment-timezone.
  2. Even on the latest database version, it is possible for the timezone to be set to kiev as the database includes old data as well for backward compatibility. I asked for more info in this github issue. As it turns out, there are more deprecated names present in the database which we will have to manually filter out from the list, if required. See https://www.github.com/moment/moment-timezone/issues/227 for an old discussion about this in Moment Timezone, and https://www.github.com/tc39/proposal-temporal/issues/2509 for a (much longer) discussion for TC39's Temporal proposal

If this can be made external, I can send in a proposal for this issue.

strepanier03 commented 1 year ago

Thank you for the hint about testing @Prince-Mendiratta and for the comments. And thanks for @thienlnam as well.

I'm going to send this external and we'll take it from there.

MelvinBot commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~010a8578e77393b86c

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

Triggered auto assignment to @mountiny (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

Prince-Mendiratta commented 1 year ago

Proposal

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

In this issue, we can see that when the location is determined automatically when the user is based on Ukraine, the location shows Kiev instead of Kyiv.

What is the root cause of that problem?

Has been pointed out in the comment above, the Intl API itself has the timezone as Kiev instead of Kyiv. It isn't possible to modify that database directly.

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

I propose that we change the name of the place specifically to "Kyiv" using the replace function on the Profile Details page.

thienlnam commented 1 year ago

Sorry, I should have clarified - I don't think we should handle this on our end @strepanier03

Let's reach out to moment API and have them make this change

MelvinBot commented 1 year ago

@strepanier03, @mountiny, @thesahindia Eep! 4 days overdue now. Issues have feelings too...

mountiny commented 1 year ago

@thesahindia Would you want to take this one and create issue/ get in touch with moment-timezone api about this?

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

Upwork job price has been updated to $250

mountiny commented 1 year ago

Whoever wants to fix this, lets fix this upstream so the main taksk would be to manage that discussion in the moment API

strepanier03 commented 1 year ago

@mountiny - Is this mostly just an email and continued follow-up with moment API about making the change? If so, is this something I should do since we haven't had confirmation @thesahindia that they want to own this one?

Not sure if we need to write code of some sort for this or if I could try to get them to make the change without needing that knowledge?

thesahindia commented 1 year ago

Apologies for missing the earlier comment. Unfortunately, I am currently swamped with work and am unable to look into this but I'll do my best to address it when I have the time. Alternatively, it is possible that another contributor may be able to help before then or maybe some other C+ ( I will ask at the C+ channel)

Not sure if we need to write code of some sort for this or if I could try to get them to make the change without needing that knowledge?

@strepanier03, we can log this issue in their repo and wait but it will take too much time so we will need to raise a PR ourself.

s77rt commented 1 year ago

@thesahindia I will take this as C+

MelvinBot commented 1 year ago

@strepanier03 @mountiny @thesahindia this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

thesahindia commented 1 year ago

@strepanier03, please assign @s77rt

MelvinBot commented 1 year ago

📣 @s77rt You have been assigned to this job by @thienlnam! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

thienlnam commented 1 year ago

Moving to weekly - @s77rt is going to reach out to moment to get this resolved on their end

mountiny commented 1 year ago

Thanks! Was travelling to San Francisco so a bit slower with responses now

@s77rt are you interested in taking on the communication with moment API? I think it will be easier if its handled by C+ here. we can make this external if you dont want to

thienlnam commented 1 year ago

Oh my bad - thought I was assigned to this issue lmao, going to turn off notifications but feel free to mention me if needed

s77rt commented 1 year ago

@mountiny Yes I will take it.

mountiny commented 1 year ago

Thanks! it might just be along back and forth, but best thing is to try to fix this in the library

strepanier03 commented 1 year ago

Thank you, everyone. If something comes up that I can help with let me know!

mountiny commented 1 year ago

Thank you! I assume this will be a lengthy one so making it monthly, @s77rt please link any issues in the repo or similar once you have it thanks!

s77rt commented 1 year ago

I'm not sure if this is really on moment. Apparently they use Intl API here https://github.com/moment/moment-timezone/blob/b155ef316889ca7c8ad9f24d28fef5c1621f8c0c/moment-timezone.js#L361.

Intl.DateTimeFormat().resolvedOptions().timeZone
> 'Europe/Kiev'
s77rt commented 1 year ago

@mountiny ^ Was there any discussion on this that we decided to blame moment :sweat_smile:?

mountiny commented 1 year ago

Hmm ok, I checked the old issue https://github.com/Expensify/App/issues/16620 and in retrospect is this important to fix? Is this breaking anything?

s77rt commented 1 year ago

I think this kind of issues will be auto fixed over time e.g. once Intl API starts returning the correct timezone. I think all what we can do if we insist on fixing this now is to replace outdated timezones with new ones, e.g.:

const outDatedTimezones = {
    'Europe/Kiev': 'Europe/Kyiv',
}
tz = Intl.DateTimeFormat().resolvedOptions().timeZone
tz = outDatedTimezones[tz] || tz
> 'Europe/Kyiv'
mountiny commented 1 year ago

Honestly this feels like such a minor thing, but yeah can you make a PR for it, or @Prince-Mendiratta and you can review/ test it. What do you think?

This would be interim before the old name is fully deprecated.

s77rt commented 1 year ago

@mountiny Are we going to fork moment-timezone?

mountiny commented 1 year ago

Noo definitely no, can we do that in App? Probably closer to what @Prince-Mendiratta suggested above.

s77rt commented 1 year ago

@mountiny We can do that on E/App too we will have to call the replace function whenever moment.tz.guess() is called. But maybe a patch is better here.

mountiny commented 1 year ago

I feel like that is overkill for this issue

I would rather do nothing than those options we have, this is not causing any real issues right now.

@kbecciv could you note somewhere that both options of spelling are possible in the list? Thanks

s77rt commented 1 year ago

@mountiny I meant a patch using patch-package and not a fork. I think that's at least better then having to call another function every after moment.tz.guess() on the App.

mountiny commented 1 year ago

I know what you meant but i still feel like like in general its something to maintain for little to know benefit. Again this is not really broken, its just two spellings and this issue only got raised because of specific steps in one PR.

I will go ahead and close this issue to focus on more important fixes.

Thanks everyone for help here and if you think there is a problem this addresses and we should fix this feel free to comment, thanks!