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

fix: crash when value is undefined #7782

Merged
merged 1 commit into from
Feb 22, 2022

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Feb 16, 2022

Details

ComponentDidUpdate was allowing the undefined value to be pass down to native TextInput value which was causing an issue. So I added a check if that value prop is undefined (~not provided), skip the update.

Fixed Issues

$ #7769
$ #7774
$ #7775
$ #7869

Tests

  1. Open the Payments page and tried to set a payment method as default and the password popup should come up and no errors.
  • Verify that no errors appear in the JS console

QA Steps

  1. Go to the payments page.
  2. Click any existing payment method to set it to default.
  3. Click set default and no error should pop up.
  • Verify that no errors appear in the JS console

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web | Desktop

image image

Mobile Web

image

iOS

image

Android

image

@parasharrajat parasharrajat requested a review from a team as a code owner February 16, 2022 18:20
@MelvinBot MelvinBot requested review from Beamanator and removed request for a team February 16, 2022 18:20
@parasharrajat
Copy link
Member Author

Updating screens shortly.....testing more issues.

@parasharrajat
Copy link
Member Author

I will update the IOS screen tomorrow. But I am pretty sure it is working.

@parasharrajat
Copy link
Member Author

Could you please test it on IOS? I am getting a build error on IOS. I don't know what changed.

@parasharrajat
Copy link
Member Author

Ok, App is built now but I am facing App freezing issue When I click setDefultmethod on the Payment Option Menu. On the other hand, I don't know how to test the complete VBA flow. I am stuck on the captcha step.

Can someone please test IOS? Otherwise, this is ready.

cc: @Beamanator

@Julesssss
Copy link
Contributor

Sorry, @parasharrajat I tried to verify this for you but I've had a lot of problems trying to verify this, none of which are related to the PR 😩

First I created some bank accounts with the generator script. Then I created an Expensify wallet. Next, I upgraded it to Gold. But I'm still not able to link a bank account to a wallet... All three of my accounts show that I'm using an invalid bank account when setting them as the default, but the error doesn't explain why.

I modified the Auth code in order to give me the exact reason why, but now I'm getting 666 after updating and need to reprovision as updateRepos w/ build didn't work either. My last attempt will be to restart my machine, which I'll do after sending this.

@parasharrajat
Copy link
Member Author

parasharrajat commented Feb 18, 2022

I was able to fix the App freezing issue and Test it and it is not crashing the app. I will attach the screen shotly.

@Beamanator
Copy link
Contributor

Sorry guys I'm pretty heads down in a design doc I didn't have time to test last weekend and may not have time today either :'( If @Julesssss is having many problems we can ping another engineer to help test

@sketchydroide
Copy link
Contributor

I'll try testing this today

@nkuoch
Copy link
Contributor

nkuoch commented Feb 22, 2022

Just FYI, I've been using this fix in the PRs I am working on because I had the bug, and it works well. (#7804 and #7813) ... and would love to see it merged ASAP so I can merge it in my PRs :)

@parasharrajat
Copy link
Member Author

Thanks, @nkuoch for your input.

@timszot
Copy link
Contributor

timszot commented Feb 22, 2022

Alright, so this looks to solve our 3 deploy blocker issues. @nkuoch you said you've been testing this in relation to your own changes, which is great. Do you have time to review/merge this PR since the two assigned haven't had a chance to get to it so we can unblock the deploy?

@github-actions
Copy link
Contributor

⚠️ ⚠️ 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.

@nkuoch nkuoch merged commit 8582db0 into Expensify:main Feb 22, 2022
OSBotify pushed a commit that referenced this pull request Feb 22, 2022
fix: crash when value is `undefined`
(cherry picked from commit 8582db0)
@OSBotify
Copy link
Contributor

🚀 Cherry-picked to staging by @nkuoch in version: 1.1.39-2 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

@@ -59,7 +59,8 @@ class BaseTextInput extends Component {

componentDidUpdate() {
// Activate or deactivate the label when value is changed programmatically from outside
if (this.value === this.props.value) {
// Only update when value prop is provided
if (this.props.value === undefined || this.value === this.props.value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB _.isUndefined(this.props.value)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I will do that in #6584 during refactoring.

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @timszot in version: 1.1.39-3 🚀

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.

8 participants