Ensure we're always including timezone in myPersonalDetails #2040

NikkiWines · 2021-03-24T17:37:45Z

cc: @jasperhuangg since you made this original issue for this one.

Details

Fixes a whitescreen issue where new users were blocked from using the product.
Adds a migration so we always get the timezone in the correct format.

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/158295

Tests

To test that personalDetails are getting set correctly:

Go to https://www.emailondeck.com/ to get a temporary email.
Use the temporary email to register for Expensify.cash
Use the validation link sent to the temporary email address.
Set a password.
After logging in, confirm everything works as anticipated and you don't get an error in the JS console.

To test timezone migration:

Log into E.cash
Open the JS console navigate to Application, select Local Storage, and search for myPersonalDetails
Manually modify your timezone in your myPersonalDetails so that it's a string like so
OLD

"timezone": {
  "automatic": true,
  "selected": "America/Los_Angeles"
}

NEW

"timezone": "America/Los_Angeles"

Set your log levels for the console drawer to include Verbose
Refresh the page and confirm you see [Migrate Onyx] Ran migration ReformatTimezone logged.
Check your myPersonalDetails onyx key again and confirm it is an object matching the OLD format above.

Tested On

Screenshots

No platform-specific changes here, no screenshots necessary.

add timezone migration

…ki-dateutils-whitescreen

NikkiWines · 2021-03-24T21:49:22Z

🤔 this test is failing for me on master as well... Gonna post in eng-chat

AndrewGable · 2021-03-24T21:51:58Z

You can fix your local error by

brew tap wix/brew
brew install applesimutils

I am not sure what is wrong with the remote version yet

NikkiWines · 2021-03-24T22:00:22Z

@AndrewGable, works on master after running those two commands! 🎉

NikkiWines · 2021-03-24T22:13:44Z

Was still broken on my branch, but should be fixed now 🎉

src/libs/migrations/ReformatTimezone.js

jasperhuangg · 2021-03-25T00:39:16Z

src/libs/migrations/ReformatTimezone.js

+                    return resolve();
+                }
+
+                // Replace the old timezone with the default timezone


Hey @NikkiWines! I'm confused as to why we're setting the old string timezone to the DEFAULT_TIME_ZONE when migrating?

My current understanding is this: the migration isn't supposed to actually change the timezone, it's just meant to change its format so that when you pick it up in DateUtil.js you can be sure that it's an object (to prevent all that conditional logic you had before when reading it from Onyx). Why would we default it to America/Los Angeles here?

This is a good point. I opted to use the default because we technically only have half the user's timezone NVP information if it's in string format. We'd be missing the automatic: true/false portion and using the default value instead just seemed like a simple solution rather than trying to guess half of their NVP value.

However, in retrospect, I think it's fine to use the old timezone value for timezone.selected and default to true, since the user can always modify that in their settings in E.cash.

…plicable

NikkiWines · 2021-03-25T01:29:25Z

Updated! Also updated the testing steps to include steps for testing the migration

isagoico · 2021-03-26T03:45:04Z

@NikkiWines I was trying to qa this and I don't think I'm editing correctly the myPersonalDetails because I'm getting this error.

Could you check this string? (It's the last one I tried I tried several possible combinations but all displayed the same error 🤯)

{"login":"isagoicotest+mar5@gmail.com","avatar":"https://d2k5nsl2zxldvw.cloudfront.net/images/avatars/avatar_6.png","displayName":"isagoicotest+mar5@gmail.com","pronouns":"",timezone: "America/Los_Angeles","firstName":"","lastName":""}

NikkiWines · 2021-03-26T15:56:10Z

@isagoico you were missing quotes around timezone. This should work:

{"login":"isagoicotest+mar5@gmail.com","avatar":"https://d2k5nsl2zxldvw.cloudfront.net/images/avatars/avatar_6.png","displayName":"isagoicotest+mar5@gmail.com","pronouns":"","timezone":"America/Los_Angeles","firstName":"","lastName":""}

isagoico · 2021-03-26T16:29:09Z

@NikkiWines I got the correct Onyx Log but I think it broke the page

NikkiWines · 2021-03-26T16:43:21Z

Hmm, that's not good. Because that's the error this was intended to fix. Can you show me the results of your myPersonalDetails key?

It also might useful to retry logging into the same account in incognito mode, to see if the error is reproducable then.

Also to be clear have you been able to successfully complete the To test that personalDetails are getting set correctly steps with this account?

NikkiWines · 2021-03-26T17:05:08Z

~~OH, I know it is!~~ Wait... maybe I don't.

isagoico · 2021-03-26T17:06:53Z

I'm getting the same issue in a Incognito window.
For the personalDetails steps. I created a new account and followed the steps and didn't see a JS error 🤔

NikkiWines · 2021-03-26T17:24:07Z

And your myPersonalDetails key contains the correctly updated information?

isagoico · 2021-03-26T17:31:18Z

It displays the OLD format

{"login":"isagoicotest+mar5@gmail.com","avatar":"https://d2k5nsl2zxldvw.cloudfront.net/images/avatars/avatar_6.png","displayName":"Isabella  Goicoechea","pronouns":"","timezone":{"automatic":true,"selected":"America/Los_Angeles"},"firstName":"Isabella ","lastName":"Goicoechea"

NikkiWines · 2021-03-26T17:43:43Z

@isagoico you might need to click away from it first, then navigate back to it to see the updated values:

I can reproduce this locally, but I'm wondering if it's a function of the way we're testing this.

It's weird to me that we run the migration, and I see the updated value in my onyx key (though the default is set to true which makes me think we're overriding it somewhere else) but that we're still getting the formatting error. Seems like a race condition and to be fair most users wouldn't been changing their onyx key

NikkiWines · 2021-03-26T17:47:51Z

Either way, I think it would be a pretty small percentage of users who would be experiencing the old timezone issue, since we've had the new timezone format out for a couple of week nows. We're basically manually replicating the old timezone issue to test this.

I can make another PR to remove the migration and go back to our old timezone check in DateUtils, but I'm not sure this should block the deploy since I think it would impact a small number of users.

isagoico · 2021-03-26T17:47:56Z

Ohh I was confused. Before refreshing the page it does display the updated MyPersonalDetails after editing it. After I refreshed and the JS error appears it displayed the OLD MyPersonalDetails.

NikkiWines · 2021-03-26T18:16:09Z

OK - here's what's happening so far:

Running the steps in To test timezone migration results in the following error
The myPersonalDetails key in local storage IS updated to an object.
Doing a hard-refresh after seeing this page does resolve the issue

Here's TLDR; of what led up to this.

When we first starting using E.cash add the timezone value to the myPersonalDetails key as a string: "timezone":"Los_Angeles"
Soon after we wanted to use the whole timezone NVP, so we changed it to "timezone": {"automatic": true, "selected": "Los_Angeles"}
- This was before we had migrations, so we had to add a bunch of manual checks all over the place
- This change went out 2 weeks ago
We now finally have migrations in place, so I made one for timezones, but to test it we have to simulate having an old timezone string by manually editing the myPersonalDetails key. From this PR we see that something goes wrong when we do that as we see Ran migration ReformatTimezome but we're getting a formatting error for the timezone even though it should be in the correct format.
Since we update the myPersonalDetails key every 30 minutes here for someone to still have the old key they would've had to meet a very niche set of conditions.
- They'd have to have manually modified it like we did to try to test this.
- They would've had to somehow not had their myPersonalDetails key updated in the last 2 weeks
Note that we know this doesn't impact NEW users since @isagoico was able to successfully complete the To test that personalDetails are getting set correctly steps for this PR which involves creating a brand-new user.

I'm going to reproduce this error on staging, then let it sit for 30min to confirm that the issue will be resolved after 30min when the myPersonalDetails key gets automatically updated. I'll also try to reproduce it on mobile and run the same test there.

NikkiWines · 2021-03-26T18:46:32Z

Hmm, so, unfortunately, looks like things won't self-resolve after 30 minutes, which makes this more serious if someone were to encounter it.

I still think the number of users who would meet the criteria is super small, but I'm less hesitant to move ahead with the deploy.

Sidenote: weirdly I can only reproduce this issue locally if I have the merge commit checked out. If I try to use the latest master it works without issue.

Edit: Looks like it works on master because we modified DateUtils.js to no longer rely on the timezone for getLocalMomentFromTimestamp in this PR

NikkiWines · 2021-03-26T21:48:17Z

Retested on staging now that the timezone migration has been removed, no more formatting error + whitescreen

Screen.Recording.2021-03-26.at.2.41.51.PM.mov

NikkiWines · 2021-03-26T21:49:21Z

@isagoico would you mind going through these steps once more 🦸‍♀️

To test that personalDetails are getting set correctly:

Go to https://www.emailondeck.com/ to get a temporary email.
Use the temporary email to register for Expensify.cash
Use the validation link sent to the temporary email address.
Set a password.
After logging in, confirm everything works as anticipated and you don't get an error in the JS console.

isagoico · 2021-03-26T22:07:15Z

I posted on the wrong PR. Here's what I found:

Staging link does work and I'm able to set the password but there's a JS error in console after login in.

I also used the production link (by mistake) and I'm unable to set up a password here's the screenshot with the JS

NikkiWines · 2021-03-26T22:17:14Z

Interesting, I'm unable to reproduce that issue on a new account sawyer6@bedroomsod.com on staging:

NikkiWines · 2021-03-26T22:21:52Z

Just tried it again on a new account using the link that gets you to set your password in E.com then log in, still no JS console error.

isagoico · 2021-03-26T22:29:32Z

Just tried again and no JS error 🎉

ensure we're always including timezone in myPersonalDetails

815c55f

add timezone migration

NikkiWines requested a review from jasperhuangg March 24, 2021 17:37

NikkiWines requested a review from a team as a code owner March 24, 2021 17:37

NikkiWines self-assigned this Mar 24, 2021

botify requested review from sketchydroide and removed request for a team March 24, 2021 17:38

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

07cd70c

…ki-dateutils-whitescreen

skip migration is personalDetails key is empty

1fa1e20

NikkiWines removed the request for review from sketchydroide March 24, 2021 23:26

jasperhuangg requested changes Mar 25, 2021

View reviewed changes

use users old timezone selection and update to use constants where ap…

f3cfc16

…plicable

jasperhuangg approved these changes Mar 25, 2021

View reviewed changes

jasperhuangg merged commit 12aaf9d into master Mar 25, 2021

jasperhuangg deleted the nikki-dateutils-whitescreen branch March 25, 2021 02:49

OSBotify mentioned this pull request Mar 25, 2021

Deploy Checklist: Expensify.cash 2021-03-19 #1926

Closed

85 tasks

AndrewGable mentioned this pull request Mar 25, 2021

Issues signing up, Error on new account sign in #2073

Closed

NikkiWines mentioned this pull request Mar 26, 2021

Remove timezone migration #2115

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure we're always including timezone in myPersonalDetails #2040

Ensure we're always including timezone in myPersonalDetails #2040

NikkiWines commented Mar 24, 2021 •

edited

Loading

NikkiWines commented Mar 24, 2021 •

edited

Loading

AndrewGable commented Mar 24, 2021

NikkiWines commented Mar 24, 2021

NikkiWines commented Mar 24, 2021

jasperhuangg Mar 25, 2021

NikkiWines Mar 25, 2021

NikkiWines commented Mar 25, 2021

isagoico commented Mar 26, 2021

NikkiWines commented Mar 26, 2021

isagoico commented Mar 26, 2021

NikkiWines commented Mar 26, 2021 •

edited

Loading

NikkiWines commented Mar 26, 2021 •

edited

Loading

isagoico commented Mar 26, 2021

NikkiWines commented Mar 26, 2021

isagoico commented Mar 26, 2021

NikkiWines commented Mar 26, 2021

NikkiWines commented Mar 26, 2021

isagoico commented Mar 26, 2021

NikkiWines commented Mar 26, 2021

NikkiWines commented Mar 26, 2021 •

edited

Loading

NikkiWines commented Mar 26, 2021

NikkiWines commented Mar 26, 2021

isagoico commented Mar 26, 2021

NikkiWines commented Mar 26, 2021

NikkiWines commented Mar 26, 2021

isagoico commented Mar 26, 2021

Ensure we're always including timezone in myPersonalDetails #2040

Ensure we're always including timezone in myPersonalDetails #2040

Conversation

NikkiWines commented Mar 24, 2021 • edited Loading

Details

Fixed Issues

Tests

To test that personalDetails are getting set correctly:

To test timezone migration:

Tested On

Screenshots

NikkiWines commented Mar 24, 2021 • edited Loading

AndrewGable commented Mar 24, 2021

NikkiWines commented Mar 24, 2021

NikkiWines commented Mar 24, 2021

jasperhuangg Mar 25, 2021

Choose a reason for hiding this comment

NikkiWines Mar 25, 2021

Choose a reason for hiding this comment

NikkiWines commented Mar 25, 2021

isagoico commented Mar 26, 2021

NikkiWines commented Mar 26, 2021

isagoico commented Mar 26, 2021

NikkiWines commented Mar 26, 2021 • edited Loading

NikkiWines commented Mar 26, 2021 • edited Loading

isagoico commented Mar 26, 2021

NikkiWines commented Mar 26, 2021

isagoico commented Mar 26, 2021

NikkiWines commented Mar 26, 2021

NikkiWines commented Mar 26, 2021

isagoico commented Mar 26, 2021

NikkiWines commented Mar 26, 2021

NikkiWines commented Mar 26, 2021 • edited Loading

NikkiWines commented Mar 26, 2021

NikkiWines commented Mar 26, 2021

To test that personalDetails are getting set correctly:

isagoico commented Mar 26, 2021

NikkiWines commented Mar 26, 2021

NikkiWines commented Mar 26, 2021

isagoico commented Mar 26, 2021

NikkiWines commented Mar 24, 2021 •

edited

Loading

NikkiWines commented Mar 24, 2021 •

edited

Loading

NikkiWines commented Mar 26, 2021 •

edited

Loading

NikkiWines commented Mar 26, 2021 •

edited

Loading

NikkiWines commented Mar 26, 2021 •

edited

Loading