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

Update react-native-onyx with the latest version #3423

Merged
merged 1 commit into from
Jun 8, 2021

Conversation

kidroca
Copy link
Contributor

@kidroca kidroca commented Jun 8, 2021

@marcaaron @Jag96 @roryabraham

Details

Update E.cash to use the react-native-onyx with with in memory cache

Fixed Issues

Fixes #2762

Tests

This update affects the app as a whole. It should become faster and more responsive
There is a known issue where selecting a chat might seem to hang for a bit, details here: Expensify/react-native-onyx#76 (comment)

There are no specific steps to do otherwise, the entire app should be tested

QA Steps

Same as above

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Expensify.cash.-.Google.Chrome.2021-06-08.14-30-14.mp4

Mobile Web

Screen.Recording.2021-06-08.at.14.47.08.mov

Desktop

Screen.Recording.2021-06-08.at.14.55.09.mp4

iOS

Screen.Recording.2021-06-08.at.14.51.42.mov

Android

Android.Emulator.-.Pixel_2_API_29_5554.2021-06-08.15-00-15.mp4

@kidroca kidroca requested a review from a team as a code owner June 8, 2021 12:01
@MelvinBot MelvinBot requested review from mountiny and removed request for a team June 8, 2021 12:02
@kidroca
Copy link
Contributor Author

kidroca commented Jun 8, 2021

Stumbled on some problems while testing, but I don't think they are related to the cache changes

IOUs showing 0.00 [all platforms]

After a fresh install and login all my IOUs we're displaying 0.00
After refresh they will display the correct amount

Screen.Recording.2021-06-08.at.14.18.02.mp4
  • initial value is 0.00
  • after clicking "View Details" the value is fixed
  • pressing on the badge while it reads 0.00 results in exception
    • the badge in the header inside a chat
    • as well as the badge for a conversation in the LHN

A similar issue happens if someone else made an IOU with me while I'm using the app - the new IOU will appear in LHN with a value of 0.00 it's fixed only after "View Details" or restarting the app

Request Money does not allow to type a note [mWeb

For some reason the field immediately loses focus

Request.Money.Bug.-.Not.able.to.add.a.note.mov

Are any of these problems known?

@kidroca
Copy link
Contributor Author

kidroca commented Jun 8, 2021

Maybe we should first merge the changes here and update this PR: Expensify/react-native-onyx#79
Or we can open a follow-up

@kidroca
Copy link
Contributor Author

kidroca commented Jun 8, 2021

I found out what's causing the "IOUs showing 0.00" here: Expensify/react-native-onyx#79 (comment)

I'm working on a fix

@mountiny
Copy link
Contributor

mountiny commented Jun 8, 2021

Great that you found it! I assume we shall wait for that bug to be fixed before merging this. I will try to test it out locally in case any other problem comes up.

@kidroca
Copy link
Contributor Author

kidroca commented Jun 8, 2021

I've pushed the fix to the other PR (Expensify/react-native-onyx#79), and verified it's working:

Screen.Recording.2021-06-08.at.18.32.36.mov

@marcaaron
Copy link
Contributor

Merged so you can update the sha here again with ebde257d2ebd6f72fe56af6ee3f4630d47c16dc5

@kidroca kidroca force-pushed the kidroca/onyx-improvements-cache branch from d6039a5 to d9b869d Compare June 8, 2021 19:47
@kidroca
Copy link
Contributor Author

kidroca commented Jun 8, 2021

Updated with the latest Onyx version

Copy link
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

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

LGTM and tests well ✅

I can see that the issue on mobile web where the reason textbox isn't working is present on both production and staging, so I don't think this is related to this PR (posted in slack here)

@Jag96 Jag96 merged commit 6835401 into Expensify:main Jun 8, 2021
@OSBotify
Copy link
Contributor

OSBotify commented Jun 8, 2021

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@mountiny mountiny removed their request for review June 8, 2021 23:50
@OSBotify
Copy link
Contributor

OSBotify commented Jun 8, 2021

🚀 Deployed to staging in version: 1.0.65-1🚀

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

@kidroca kidroca deleted the kidroca/onyx-improvements-cache branch June 9, 2021 15:24
@trjExpensify trjExpensify added the DeployBlockerCash This issue or pull request should block deployment label Jun 9, 2021
@trjExpensify
Copy link
Contributor

Marking this as a deploy blocker, so it's correctly reflected here. Thank you both @kidroca and @Julesssss for getting PRs up to fix the IOU regressions today!

@Julesssss
Copy link
Contributor

We've closed the linked issues, so marking this as resolved in the deploy checklist.

@Julesssss Julesssss removed the DeployBlockerCash This issue or pull request should block deployment label Jun 14, 2021
@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.68-4🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 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.

Improvements to Onyx.get
7 participants