Update timezone to update if user has it set to automatic #2318

NikkiWines · 2021-04-08T23:35:07Z

cc: @iwiznia

Details

Fixed Issues

Fixes https://expensify.slack.com/archives/C01GTK53T8Q/p1617920244466600?thread_ts=1617784393.408700&cid=C01GTK53T8Q

Tests

Open your developer console (opt + cmd + j for chrome) and go to the Sources tab
Set a breakpoint at if (_.isObject(timezone) && timezone.automatic && timezone.selected !== currentTimezone) { in AuthScreens.js

Video to set breakpoint

Untitled.mov

Reload the page
When the point stops at the debugger, manually update your timezone by running the following in the console

timezone = {automatic: true, selected: "Africa/Tripoli"};

Continue execution
Navigate to the Application tab the developer console, click on the value under Local Storage on the left hand side.

Video to find personalDetails

navToPersonalDetails.mov

Search for myPersonalDetails, and click on the value.
Confirm it's update to your current timezone.

QA Steps

Repeat test steps (Web only QA)

Tested On

Screenshots

N/A no UI changes.

src/libs/Navigation/AppNavigator/AuthScreens.js

thienlnam · 2021-04-09T16:37:29Z

src/libs/Navigation/AppNavigator/AuthScreens.js

@@ -56,11 +60,13 @@ const modalScreenListeners = {

 const propTypes = {
    network: PropTypes.shape({isOffline: PropTypes.bool}),
+    myPersonalDetails: PropTypes.objectOf(personalDetailsPropType),


this prop is never passed in and will always be a default prop right? In that case, should we just modify it in componentDidMount?

Ahh this is a good point, I think maybe this logic is better suited to personalDetails.js

…ki-automatic-timezone

iwiznia · 2021-04-09T17:57:58Z

src/libs/actions/PersonalDetails.js

@@ -87,6 +88,11 @@ function formatPersonalDetails(personalDetailsList) {
        const pronouns = lodashGet(personalDetailsResponse, 'pronouns', '');
        const timezone = lodashGet(personalDetailsResponse, 'timeZone', CONST.DEFAULT_TIME_ZONE);

+        // Update the users timezone if they have it set to automatic
+        if (login === currentUserEmail && _.isObject(timezone) && timezone.automatic) {


This is not really updating the NVP, IMO it should update the NVP. Also, it feels a bit weird that we are doing this logic here... we will be guessing the timezone all the time we format your details? Doesn't it make more sense to do this once on app init?

Oh, yeah good point, I should make sure I sent the NVP as well.

As for doing it all the time, we only call personalDetails.fetch() in Authscreens.js in two places. Once on init, and then every 30 min to update the details as necessary. Since we want this to be automatic, I don't see an issue with it running every 30 min, but maybe I am too lenient.

I don't know, feels weird to me that we do this here... you call formatPersonalDetails and all of a sudden an NVP is set... it's weird.

Yeah, I can understand that - you wouldn't expect a function called formatXYZ to set any values. I could move this logic into it's own function and call it in fetch() which isn't much better, but makes a little more sense since the value from the DB might be outdated if the user has moved to a new timezone.

That's where I had it before, but there were issues because we don't necessarily have the personalDetails at that point. I suppose we could make PersonalDetails.fetch() return a promise and when that resolves run the logic for updating the timezone if it's set to automatic.

I think we should not use the personal details at all there, just get the NVP from your own account.

Hmm, I think that'll potentially cause some issues since we are already fetching personalDetails (which contains the timezone) here. We'd need to be sure that the myPersonalDetails and personalDetails onyx keys are updated correctly after we modify the timezone.

Based on your suggestion, I think we'd end up doing something like this in AuthScreens.js (adding some small modifications to NameValuePairs.get as well).

NameValuePair.get(CONST.NVP.TIMEZONE, CONST.DEFAULT_TIMEZONE) .then((value) => { const timezone = value; if (timezone.automatic) { timezone.selected = moment.tz.guess(true); NameValuePair.set(CONST.NVP.TIMEZONE, timezone); PersonalDetails.fetch(); } });

Is that along the lines of what you were thinking?

No, not really since actions should not return a promise, you need to subscribe to the onyx key, no?

Ok, chatted 1:1 and I messed around with this a bit. Works now!

thienlnam

Looks good! I like that now it only updates if it doesn't match the current timezone. I just had a small comment

src/libs/Navigation/AppNavigator/AuthScreens.js

NikkiWines · 2021-04-12T16:02:45Z

updated!

thienlnam

Thanks! LGTM

kidroca · 2021-04-13T11:34:55Z

src/libs/Navigation/AppNavigator/AuthScreens.js

+Onyx.connect({
+    key: ONYXKEYS.MY_PERSONAL_DETAILS,
+    callback: (val) => {
+        const timezone = lodashGet(val, 'timezone', {});
+        const currentTimezone = moment.tz.guess(true);
+
+        // If the current timezone is different than the user's timezone, and their timezone is set to automatic
+        // then update their timezone.
+        if (_.isObject(timezone) && timezone.automatic && timezone.selected !== currentTimezone) {
+            timezone.selected = currentTimezone;
+            PersonalDetails.setPersonalDetails({timezone});
+        }
+    },
+});
+


Why is this block defined here?

The code here would run as soon as this file is imported and it's not related to the actual AtuhScreens component lifecycle (E.g. the subscription would happen even before you login and AtuhScreens are rendered)

It could have been added to PersonalDetails along with the existing static Onix.connect calls there?

https://github.com/Expensify/Expensify.cash/blob/9b0938441611f90ed8611513954258227dabbd35/src/libs/actions/PersonalDetails.js#L13-L23

I noticed this while resolving a merge conflict in AtuhScreens in #2346

There is a larger conversation here about the reasoning for moving this logic to AuthScreens.js.

The linked conversations is about setting the timezone should not be a side effect of the format function and should be moved out of it.

Ok it was moved out but you can literally copy this whole block, put it anywhere and it will still work

https://github.com/Expensify/Expensify.cash/blob/87c3b3dbf5045b196eacf2d89554a3cd2d74586f/src/libs/Navigation/AppNavigator/AuthScreens.js#L47-L60

I just don't get why it's in AuthScreens it's not related to anything in this file.

That's why I suggested that it can be housed in PersonalDetails below the other Onyx.connect calls there

Oh, that's a good point. It indeed can be anywhere. I am fine with putting it in the PersonalDetails file

Ah, I see. Yeah, it could be in PersonalDetails.js. @kidroca if you feel strongly about it you can modify it in your PR (or a future PR), or I will modify it in my next E.cash change.

I could, but then git would annotate me as the author of the code, while I don't know how it actually works. It would be best if the author (or at least someone involved in this) moves it

update timezone on login/page reload if user has it set to automatic

f86a477

NikkiWines self-assigned this Apr 8, 2021

NikkiWines requested a review from a team as a code owner April 8, 2021 23:35

MelvinBot requested review from thienlnam and removed request for a team April 8, 2021 23:35

thienlnam reviewed Apr 9, 2021

View reviewed changes

NikkiWines added 2 commits April 9, 2021 10:10

Merge branch 'master' of github.com:Expensify/Expensify.cash into nik…

c1b5894

…ki-automatic-timezone

move auto-update timezone logic to personalDetails.js

cd3f644

iwiznia requested changes Apr 9, 2021

View reviewed changes

NikkiWines added 2 commits April 9, 2021 11:06

ensure we update timezone nvp in addition to onyx key

5138c72

set timezone inside callback

a019e1e

iwiznia previously approved these changes Apr 9, 2021

View reviewed changes

thienlnam reviewed Apr 10, 2021

View reviewed changes

src/libs/Navigation/AppNavigator/AuthScreens.js Outdated Show resolved Hide resolved

use currentTimezone where applicable

87c3b3d

NikkiWines dismissed iwiznia’s stale review via 87c3b3d April 12, 2021 16:02

thienlnam approved these changes Apr 12, 2021

View reviewed changes

thienlnam merged commit 0874880 into master Apr 12, 2021

thienlnam deleted the nikki-automatic-timezone branch April 12, 2021 16:39

OSBotify mentioned this pull request Apr 12, 2021

Deploy Checklist: Expensify.cash 2021-04-09 #2332

Closed

8 tasks

kidroca reviewed Apr 13, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update timezone to update if user has it set to automatic #2318

Update timezone to update if user has it set to automatic #2318

NikkiWines commented Apr 8, 2021 •

edited

Loading

thienlnam Apr 9, 2021

NikkiWines Apr 9, 2021

iwiznia Apr 9, 2021

NikkiWines Apr 9, 2021

NikkiWines Apr 9, 2021 •

edited

Loading

iwiznia Apr 9, 2021

NikkiWines Apr 9, 2021

NikkiWines Apr 9, 2021

iwiznia Apr 9, 2021

NikkiWines Apr 9, 2021

iwiznia Apr 9, 2021

NikkiWines Apr 9, 2021

thienlnam left a comment •

edited

Loading

NikkiWines commented Apr 12, 2021

thienlnam left a comment

kidroca Apr 13, 2021 •

edited

Loading

NikkiWines Apr 13, 2021 •

edited

Loading

kidroca Apr 13, 2021

iwiznia Apr 13, 2021

NikkiWines Apr 13, 2021

kidroca Apr 14, 2021

Update timezone to update if user has it set to automatic #2318

Update timezone to update if user has it set to automatic #2318

Conversation

NikkiWines commented Apr 8, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikkiWines Apr 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thienlnam left a comment • edited Loading

Choose a reason for hiding this comment

NikkiWines commented Apr 12, 2021

thienlnam left a comment

Choose a reason for hiding this comment

kidroca Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

NikkiWines Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikkiWines commented Apr 8, 2021 •

edited

Loading

NikkiWines Apr 9, 2021 •

edited

Loading

thienlnam left a comment •

edited

Loading

kidroca Apr 13, 2021 •

edited

Loading

NikkiWines Apr 13, 2021 •

edited

Loading