-
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
[C+ Checklist Needs Completion] [$500] Profile - Incorrect city name in automatic timezone selection #30468
Comments
Triggered auto assignment to @greg-schroeder ( |
Job added to Upwork: https://www.upwork.com/jobs/~01bb82fb7cffd6353f |
Bug0 Triage Checklist (Main S/O)
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.We are unable to find Kiev as a timezone when selecting the time zone manually What is the root cause of that problem?Kiev is not defined in TIMEZONES.js, only Kyiv is defined. What changes do you think we should make in order to solve the problem?Add 'Europe/Kiev' to TIMEZONES.js
They both are valid names for the city and are valid time zones. Verified by running this in the javascript console:
What alternative solutions did you explore? (Optional) |
ProposalPlease re-state the problem that we are trying to solve in this issue.Incorrect city name in automatic timezone selection What is the root cause of that problem?We use a constant in which the names of the time zones do not match the time zones from the https://github.com/Expensify/App/blob/53699cfa5aae38f8a95859ce102ff026cf9ce407/src/TIMEZONES.js What changes do you think we should make in order to solve the problem?I think the easiest way to avoid such problems is Line 1 in 53699cf
https://raw.githubusercontent.com/leon-do/Timezones/main/timezone.json And use time zones from the Intl library because for automatic value we also useIntl library (Intl.DateTimeFormat().resolvedOptions().timeZone )
For this we can use and we can call here instead using TIMEZONES constant App/src/pages/settings/Profile/TimezoneSelectPage.js Lines 40 to 49 in 546f27e
As alternative: To be sure that the data matches What alternative solutions did you explore? (Optional)We can just update zone names in TIMEZONES constants |
ProposalPlease re-state the problem that we are trying to solve in this issue.Incorrect city name in automatic timezone selection What is the root cause of that problem?The list of timezone is missing from Line 1 in bb32cc5
What changes do you think we should make in order to solve the problem?We will create timezoneBackwardMap from tz_database const timezoneBackwardMap = {
'Africa/Accra': 'Africa/Abidjan',
'Africa/Addis_Ababa': 'Africa/Nairobi',
.....
}; At function getUserTimezone if timezone not exits on TIMEZONES.ts we will get backward link from timezoneBackwardMap const getUserTimezone = (currentUserPersonalDetails) => {
const timezone = lodashGet(currentUserPersonalDetails, 'timezone', CONST.DEFAULT_TIME_ZONE);
if(TIMEZONES.includes(timezone)) {
return timezone;
}
return timezoneBackwardMap[timezone]
}; Then At function updateAutomaticTimezone also check if timezone not exits on TIMEZONES.ts we will get backward link from timezoneBackwardMap const parameters: UpdateAutomaticTimezoneParams = {
timezone: TIMEZONES.includes(timezone.selected) ? JSON.stringify(timezone) : JSON.stringify({ selected: timezoneBackwardMap[timezone.selected], automatic: timezone.automatic }),
}; What alternative solutions did you explore? (Optional) |
ProposalPlease re-state the problem that we are trying to solve in this issue.The relevant timezone is not included in the configured data. What is the root cause of that problem?The timezone list is not being updated. What changes do you think we should make in order to solve the problem?In JavaScript, you can use the moment-timezone library to get a list of time zones. To automatically update the timezones.js file with the list of time zones obtained from moment.tz.names(), you have several options. One way is to create a script that dynamically generates the TIMEZONES.js file When you run this script, it will generate (or update) the TIMEZONES.js file at the specified path. If the moment-timezone library is updated with new time zones, you can re-run this script to automatically update TIMEZONES.js. Instead of running this script manually, you could also use some automation tool (e.g., a cron job or a CI/CD pipeline) to run it periodically. example
|
📣 @girafferz! 📣
|
Contributor details |
✅ Contributor details stored successfully. Thank you for contributing to Expensify! |
@sobitneupane, @greg-schroeder Whoops! This issue is 2 days overdue. Let's get this updated quick! |
Awaiting proposal review |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
Looks like we're waiting for some proposals to be updated after factoring in the comment above, I think |
@sobitneupane @greg-schroeder 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! |
Looks like @sobitneupane is still in the review phase! |
Reviews continue on linked PR |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.23-4 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-17. 🎊 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:
|
Issue reported by: Applause Paid @suneox, and you can make a manual request Sobit. All that's left is to complete the checklist above! |
Bump @sobitneupane :) |
Bump again so we can get this one closed out! |
Sorry for the delay @greg-schroeder I was mostly unavailable last week. I will try to get it done this week (probably tomorrow) |
No problem! Thanks Sobit! |
This issue is not solely caused by the linked PR. Auto-selecting timezone is returning outdated timezone. Discussion: #30468 (comment) and #30468 (comment)
It is an edge case that was missed while testing the PR.
Nope. It was reported by Applause - Internal Team. So, I believe the regression test for this issue is already covered. |
Thanks! closing |
$500 approved for @sobitneupane based on this summary. |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: 1.3.91-6
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:
Action Performed:
Pre-condition: User logged in and located in Ukraine
Expected Result:
Name of the city in the manual and automatic time zone selection will be identical
Actual Result:
Name of the city in the automatic timezone selection is Kiev, but if you try to find this timezone manually you can't find anything. Because timezone name in manual selection is Kyiv
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
Windows: Chrome
Bug6251961_1698333607223.chrome_PVhOb4WtPO.mp4
MacOS: Desktop
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: