#30468 parse supported timezone #33186

suneox · 2023-12-15T18:48:04Z

Details

Fixed Issues

$ #30468
PROPOSAL: #30468 (comment)

Tests

Preconditions: simulate location or change timezone to Ukraine

Web config: Config Europe/Kiev location then turn on sensor
IOS config: https://www.makeuseof.com/how-to-change-time-zone-iphone/
Android config: https://support.google.com/android/answer/2841106?hl=en

Open Settings
Click on Profile
Click on the Timezone
Turn on "Automatically determine your location." and observe the timezone
Turn off "Automatically determine your location."
Click on the timezone selection
Verify the timezone at step 4 exists in the list

Verify that no errors appear in the JS console

Offline tests

QA Steps

Preconditions: simulate location or change timezone to Ukraine

Web config: Config Europe/Kiev location then turn on sensor
IOS config: https://www.makeuseof.com/how-to-change-time-zone-iphone/
Android config: https://support.google.com/android/answer/2841106?hl=en

Open Settings
Click on Profile
Click on the Timezone
Turn on "Automatically determine your location." and observe the timezone
Turn off "Automatically determine your location."
Click on the timezone selection
Verify the timezone at step 4 exists in the list

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

MacOS: Desktop

desktop.mp4

Android: mWeb Chrome

android-chrome.mp4

Android: Native

iOS: mWeb Safari

ios-safari.MP4

iOS: Native

ios-native.mp4

MacOS: Chrome / Safari

web-chrome.mp4

melvin-bot · 2023-12-15T18:48:09Z

@sobitneupane 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]

suneox · 2023-12-18T04:06:44Z

these change work on your side?

Yes It works I also capture in Screenshots/Videos section

suneox · 2023-12-18T15:56:23Z

Hi @sobitneupane have you got any feedback on my PR

sobitneupane · 2023-12-19T06:18:06Z

src/components/withCurrentUserPersonalDetails.tsx

+            () =>
+                accountPersonalDetails
+                    ? {...accountPersonalDetails, accountID, timezone: accountPersonalDetails.timezone ? DateUtils.parseTimezone(accountPersonalDetails.timezone) : undefined}
+                    : {},


@suneox Can you please help me understand the reason for this change. I think we should change the timezone to supported format before saving it to Onyx. Is there any possible scenario where personalDetails retrieved from storage has unsupported timezone?

This change to handle case user has save the timezone is not supported to BE and user do not anything action to re-update timezone. So when get the timezone from BE we also should parse to supported timezone to display

Thanks @suneox for the prompt reply.

user has save the timezone is not supported to BE and user do not anything action to re-update timezone.

I didn't get you. Can you please add more details. User saved timezone will be from the list of TIMEZONES.ts, right? Will there be any case where the saved timezone is not supported by BE? Please let me know if I am missing something.

That mean a user has saved an unsupported timezone to BE by set Automatically Timezone BEFORE merge this PR and this timezone not available in the list of TIMEZONES.ts, so this change to support return supported timezone from currentUserPersonalDetails if user do not change timezone after merge this PR

I don't think we need to consider the situation as we are still in development phase and the issue was not reported before. So, I think we are safe to remove the change. Please let me know if you disagree.

sobitneupane · 2023-12-19T06:26:16Z

src/libs/DateUtils.ts

+ * function parse timezone to supported timezone
+ * @returns Timezone
+ */
+function parseTimezone(timezoneInput: Timezone): Timezone {


I think we should name it formatToSupportedTimezone or convertToSupportedTimezone?

sobitneupane · 2023-12-19T06:29:54Z

@suneox Let's add the test steps for the case of App Crash (that you mention here) as well

sobitneupane · 2023-12-19T06:33:22Z

src/libs/actions/PersonalDetails.ts

@@ -296,7 +297,7 @@ function updateAutomaticTimezone(timezone: Timezone) {
    };

    const parameters: UpdateAutomaticTimezoneParams = {
-        timezone: JSON.stringify(timezone),
+        timezone: JSON.stringify(DateUtils.parseTimezone(timezone)),


Looks like you have not updated the timezone saved in Onyx.

https://github.com/Expensify/App/pull/33186/files#diff-1372b8700afa9b26d89dfe0575dcfaaf399d62b9b0555f30863e8c19c5609df9R310

Yes I have missed let me update

suneox · 2023-12-19T07:05:47Z

@suneox Let's add the test steps for the case of App Crash (that you mention here) as well

Prepare:

Login same account into web app and native app,
turn off automatically location on application (Setting -> Profile -> Timezone) on both web, native app

Reproduce Step:

Change system timezone to Kyiv Ukraine (Setting -> General -> Date & Time -> turn off set automatically)
turn on automatically location on the web application then turn off
Open native app and access report has IOU

trim.80B223C7-DCFF-47C7-A71C-FDD95C780B75.MOV

suneox · 2023-12-20T08:32:50Z

I have updated code review

sobitneupane

I am still seeing the issue @suneox

Screen.Recording.2023-12-21.at.13.43.43.mov

Some issues:

Timezone is switching between unformatted and formatted timezone when user toggles 'automatically determine location'
Timezone resets back to unformatted timezone.

suneox · 2023-12-21T11:24:30Z

Hi @sobitneupane
I have found an issue, we have another API also re-update unsupported timezone, maybe it have updated recently

after UpdateAutomaticTimezone

So let me also update an api OpenProfile using supported timezone

suneox · 2023-12-21T13:22:38Z

Hi @sobitneupane
I have updated the supported timezone for API OpenProfile

30468_3.mp4

sobitneupane

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

Screen.Recording.2023-12-22.at.10.39.51.mov

Android: mWeb Chrome

iOS: Native

Screen.Recording.2023-12-22.at.09.56.29.mov

iOS: mWeb Safari

Screen.Recording.2023-12-22.at.09.10.49.mov

MacOS: Chrome / Safari

Screen.Recording.2023-12-22.at.08.35.35.mov

MacOS: Desktop

Screen.Recording.2023-12-22.at.08.48.45.mov

suneox · 2023-12-28T00:40:48Z

Hi, @amyevans
Have you got any feedback on this PR?

amyevans

Thanks for your patience while I was OOO. Just one change requested!

amyevans · 2024-01-02T22:18:21Z

src/TIMEZONES.ts

@@ -419,3 +420,133 @@ export default [
    'Pacific/Wake',
    'Pacific/Wallis',
 ] as const;
+
+const timezoneBackwardMap: Record<string, (typeof TIMEZONES)[number]> = {


Could you add a comment explaining briefly where these values come from (link to wikipedia page)/what the purpose of the map is? That way people curious in the future don't have to follow the GH blame trail.

…ct-timezone

amyevans

A couple suggestions regarding the comments, let me know if they make sense or if we should continue to refine.

amyevans · 2024-01-03T20:03:44Z

src/TIMEZONES.ts

@@ -419,3 +420,137 @@ export default [
    'Pacific/Wake',
    'Pacific/Wallis',
 ] as const;
+
+/**
+ * On some devices the timezone backward doesn't support so when a user using multiple devices has automatic timezone setting will not display in the list timezone selection


Suggested change

* On some devices the timezone backward doesn't support so when a user using multiple devices has automatic timezone setting will not display in the list timezone selection

The timezones supported in browser and on native devices differ, so we must map each timezone to its supported equivalent.

amyevans · 2024-01-03T20:04:02Z

src/TIMEZONES.ts

+
+/**
+ * On some devices the timezone backward doesn't support so when a user using multiple devices has automatic timezone setting will not display in the list timezone selection
+ * List timezone backward parse from https://en.wikipedia.org/wiki/List_of_tz_database_time_zones


Suggested change

* List timezone backward parse from https://en.wikipedia.org/wiki/List_of_tz_database_time_zones

* Data sourced from https://en.wikipedia.org/wiki/List_of_tz_database_time_zones

…ct-timezone

suneox · 2024-01-04T06:08:11Z

A couple suggestions regarding the comments, let me know if they make sense or if we should continue to refine.

@amyevans Your the comment suggestion is better than mine so I have updated it

amyevans

Looks good!

The only remaining thing is the author checklist check is failing, looks like the template was updated recently: https://github.com/Expensify/App/pull/30699/files. @suneox Could you update your checklist to include those new checks please?

suneox · 2024-01-04T17:26:31Z

Looks good!

The only remaining thing is the author checklist check is failing, looks like the template was updated recently: https://github.com/Expensify/App/pull/30699/files. @suneox Could you update your checklist to include those new checks please?

@amyevans the current code change doesn't update any style and I have updated the checklist

OSBotify · 2024-01-04T18:34:26Z

✋ 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-09T05:36:18Z

🚀 Deployed to staging by https://github.com/amyevans in version: 1.4.23-0 🚀

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

OSBotify · 2024-01-10T06:03:33Z

🚀 Deployed to production by https://github.com/thienlnam in version: 1.4.23-4 🚀

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

Expensify#30468 parse supported timezone

a0b6d95

suneox requested a review from a team as a code owner December 15, 2023 18:48

melvin-bot bot requested review from sobitneupane and removed request for a team December 15, 2023 18:48

suneox mentioned this pull request Dec 15, 2023

[C+ Checklist Needs Completion] [$500] Profile - Incorrect city name in automatic timezone selection #30468

Closed

6 tasks

sobitneupane reviewed Dec 19, 2023

View reviewed changes

suneox added 3 commits December 19, 2023 16:21

Expensify#30468 update code review

1e82fd2

revert format timezone for currentUserPersonalDetails

ac96c6c

revert formated timezone for currentUserPersonalDetails

df07ac9

sobitneupane reviewed Dec 21, 2023

View reviewed changes

openprofile update supported timezone

2a677a1

sobitneupane approved these changes Dec 22, 2023

View reviewed changes

melvin-bot bot requested a review from amyevans December 22, 2023 04:13

amyevans requested changes Jan 2, 2024

View reviewed changes

suneox added 2 commits January 3, 2024 08:14

mention for timezone backward

c508ab1

Merge remote-tracking branch 'upstream/main' into fix/30468-auto-sele…

27f0eae

…ct-timezone

amyevans reviewed Jan 3, 2024

View reviewed changes

suneox added 2 commits January 4, 2024 10:18

Merge remote-tracking branch 'upstream/main' into fix/30468-auto-sele…

3a6e65a

…ct-timezone

update code review

e3b7c1f

amyevans approved these changes Jan 4, 2024

View reviewed changes

amyevans merged commit adf68dc into Expensify:main Jan 4, 2024

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

Deploy Checklist: New Expensify 2024-01-09 #34121

Closed

87 tasks

suneox mentioned this pull request Jun 2, 2024

[HOLD for payment 2024-06-28] [$250] Timezone in the ND app is not updated after moving to another timezone location #34035

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#30468 parse supported timezone #33186

#30468 parse supported timezone #33186

suneox commented Dec 15, 2023 •

edited

Loading

melvin-bot bot commented Dec 15, 2023

suneox commented Dec 18, 2023

suneox commented Dec 18, 2023

sobitneupane Dec 19, 2023

suneox Dec 19, 2023

sobitneupane Dec 19, 2023 •

edited

Loading

suneox Dec 19, 2023 •

edited

Loading

sobitneupane Dec 19, 2023

sobitneupane Dec 19, 2023

sobitneupane commented Dec 19, 2023

sobitneupane Dec 19, 2023

suneox Dec 19, 2023

suneox commented Dec 19, 2023

suneox commented Dec 20, 2023

sobitneupane left a comment •

edited

Loading

suneox commented Dec 21, 2023

suneox commented Dec 21, 2023

sobitneupane left a comment •

edited

Loading

suneox commented Dec 28, 2023

amyevans left a comment

amyevans Jan 2, 2024

amyevans left a comment

amyevans Jan 3, 2024

amyevans Jan 3, 2024

suneox commented Jan 4, 2024

amyevans left a comment

suneox commented Jan 4, 2024

OSBotify commented Jan 4, 2024

OSBotify commented Jan 9, 2024

OSBotify commented Jan 10, 2024

	* On some devices the timezone backward doesn't support so when a user using multiple devices has automatic timezone setting will not display in the list timezone selection
	The timezones supported in browser and on native devices differ, so we must map each timezone to its supported equivalent.

	* List timezone backward parse from https://en.wikipedia.org/wiki/List_of_tz_database_time_zones
	* Data sourced from https://en.wikipedia.org/wiki/List_of_tz_database_time_zones

#30468 parse supported timezone #33186

#30468 parse supported timezone #33186

Conversation

suneox commented Dec 15, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Dec 15, 2023

suneox commented Dec 18, 2023

suneox commented Dec 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sobitneupane Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

suneox Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sobitneupane commented Dec 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suneox commented Dec 19, 2023

suneox commented Dec 20, 2023

sobitneupane left a comment • edited Loading

Choose a reason for hiding this comment

suneox commented Dec 21, 2023

suneox commented Dec 21, 2023

sobitneupane left a comment • edited Loading

Choose a reason for hiding this comment

Reviewer Checklist

Screenshots/Videos

suneox commented Dec 28, 2023

amyevans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amyevans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suneox commented Jan 4, 2024

amyevans left a comment

Choose a reason for hiding this comment

suneox commented Jan 4, 2024

OSBotify commented Jan 4, 2024

OSBotify commented Jan 9, 2024

OSBotify commented Jan 10, 2024

suneox commented Dec 15, 2023 •

edited

Loading

sobitneupane Dec 19, 2023 •

edited

Loading

suneox Dec 19, 2023 •

edited

Loading

sobitneupane left a comment •

edited

Loading

sobitneupane left a comment •

edited

Loading