Skip to content
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

Attempt at fixing race #4416

Merged
merged 4 commits into from
Aug 5, 2021
Merged

Attempt at fixing race #4416

merged 4 commits into from
Aug 5, 2021

Conversation

chiragsalian
Copy link
Contributor

@chiragsalian chiragsalian commented Aug 5, 2021

cc @parasharrajat for some reason I cant add you as a reviewer.

Details

Fixed Issues

$ #4388 (comment)

Tests

  1. Update this line to be const timezone = {automatic: true, selected: 'Asia/Kolkata'};.
  2. Tested logging out and logging in on simulator and web and confirmed i didn't see any Cannot read property 'replace' of undefined during log out and log in.

QA Steps

Steps mentioned in issue - #4388 (comment)

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@chiragsalian chiragsalian self-assigned this Aug 5, 2021
@chiragsalian chiragsalian requested a review from a team as a code owner August 5, 2021 01:39
@MelvinBot MelvinBot requested review from Luke9389 and removed request for a team August 5, 2021 01:39
Onyx.connect({
key: ONYXKEYS.MY_PERSONAL_DETAILS,
callback: (val) => {
if (!val) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides moving the block i also added this bit so that when we log out if this is null it just returns early since there is no reason to proceed with val is empty.

Copy link
Member

@parasharrajat parasharrajat Aug 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one more thing, this should not run when the user is logged out. But it's fine you have null check on val which should cover this case as well.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2021

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

@chiragsalian
Copy link
Contributor Author

chiragsalian commented Aug 5, 2021

oh jest is failing. But its passing for me locally 🤔
weird, let me restart it and see if it helps.

Edit: Its failing for me locally as weird. Weird i could've sworn i ran npm run test yesterday and it passed. Investigating.

@chiragsalian
Copy link
Contributor Author

Looks like I was able to resolve the problem with jest. I had to move the blocks back to authScreens since there were more race conditions occurring in jest causing it to fail. Atm having the code in authScreens with an early return seems like an easy fix so I think it's best to try it out first.

Copy link
Contributor

@NikkiWines NikkiWines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

@chiragsalian
Copy link
Contributor Author

Luke seems to be offline and this is a deploy blocker. Self merging so that we can test if this resolves the blocker.

@chiragsalian chiragsalian merged commit 969f9d2 into main Aug 5, 2021
@chiragsalian chiragsalian deleted the chirag-race-fix-attempt branch August 5, 2021 18:31
github-actions bot pushed a commit that referenced this pull request Aug 5, 2021
Attempt at fixing race

(cherry picked from commit 969f9d2)
@OSBotify
Copy link
Contributor

OSBotify commented Aug 5, 2021

🚀 Cherry-picked to staging in version: 1.0.82-4🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Aug 6, 2021

🚀 Deployed to production in version: 1.0.82-7🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Aug 6, 2021

🚀 Deployed to staging in version: 1.0.82-8🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @francoisl in version: 1.0.83-1 🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants