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

Adds KYC Wall trigger for Transfer Balance action #7211

Merged
merged 8 commits into from
Jan 20, 2022

Conversation

marcaaron
Copy link
Contributor

@marcaaron marcaaron commented Jan 13, 2022

cc @stitesExpensify @nickmurray47

Details

  • Adds the KYC checks to attempts to transfer a balance
  • Fixes up the positioning of the "add payment method" popover

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/187802

Tests

Note: In order to simulate a user instantly passing Onfido + IDOlogy we need to take a few additional steps that are explained here.

  1. Start with an account that has a gold wallet + payment method added i.e. able to "Send Money"
  2. Send money to an account that does not have a gold wallet
  3. As the recipient, navigate to the "Settings > Payments" screen and tap "Transfer Balance"
  4. Verify that a popup appears asking you to add a "Bank account" or "Debit card"
    2022-01-20_09-36-11
  5. Select Bank Account + add a test bank account
  6. Verify you are navigated to the "Verify identity" flow next
    2022-01-20_09-37-03
  7. Go through Onfido
  8. Enter the required information in Additional details step (follow second answer here)
  9. Verify that you are brought to the balance transfer page
    2022-01-20_09-43-34
  • Verify that no errors appear in the JS console

QA Steps

Internal QA ... maybe Production QA? Not sure if Onfido works on staging.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@marcaaron marcaaron self-assigned this Jan 13, 2022
@marcaaron marcaaron marked this pull request as ready for review January 18, 2022 20:04
@marcaaron marcaaron requested a review from a team as a code owner January 18, 2022 20:04
@marcaaron marcaaron changed the title [WIP] Adds KYC Wall trigger for Transfer Balance action Adds KYC Wall trigger for Transfer Balance action Jan 18, 2022
@MelvinBot MelvinBot requested review from marcochavezf and removed request for a team January 18, 2022 20:04
nickmurray47
nickmurray47 previously approved these changes Jan 19, 2022
Copy link
Contributor

@nickmurray47 nickmurray47 left a comment

Choose a reason for hiding this comment

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

not able to test right now because of some Auth/VM issues but code LGTM too!

@stitesExpensify
Copy link
Contributor

Will test and merge this morning

@marcochavezf
Copy link
Contributor

I got this error in step 3:

Screen Shot 2022-01-19 at 13 04 29

I'm going to test again with a different account.

@marcochavezf
Copy link
Contributor

marcochavezf commented Jan 19, 2022

Getting the same issue with another account with Gold wallet. Seems the userWalletproperty is missing here:

const paymentMethods = PaymentUtils.formatPaymentMethods(
this.props.bankAccountList,
this.props.cardList,
);

Because it's undefined in PaymentUtils.formatPaymentMethods:

Screen Shot 2022-01-19 at 13 36 07

@marcaaron
Copy link
Contributor Author

Uh hmm,

const paymentMethods = PaymentUtils.formatPaymentMethods(
this.props.bankAccountList,
this.props.cardList,
);

We are not passing it here

@marcaaron
Copy link
Contributor Author

Pushed a change to fix this here. Gonna re-test myself (last time I checked I had no issues accessing).

@marcaaron
Copy link
Contributor Author

Hmm, I'm really confused why this is not working all of a sudden... the data is there

2022-01-19_10-48-33

I have a feeling this is related to the IndexedDB change...

2022-01-19_10-52-12
2022-01-19_10-52-39

@marcaaron
Copy link
Contributor Author

Ok it took me way too long to figure out that we need to pass a paypal me name to the formatPaymentMethods() method 😂

The SvgChase() error + propTypes were unrelated, but also fixing them here to avoid future confusion.

@marcaaron
Copy link
Contributor Author

Updated.

@@ -30,7 +30,7 @@ const propTypes = {
userWallet: userWalletPropTypes.userWallet,

/** Array of bank account objects */
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB: Ah I think we should update that this is no longer an array but an object, no?

@@ -45,7 +45,7 @@ const propTypes = {
})),

/** Array of card objects */
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB: Same here? Which can be an Object (or Dictionary) of card objects?

@marcaaron
Copy link
Contributor Author

Updated.

@marcochavezf
Copy link
Contributor

The Transfer Balance page is working fine now :D but for some reason I'm stuck in step 5; the button Instant (Debit Card) is selected but the Transfer button is disabled also I can't select the other option 1-3 Business Days (Bank Account)

Screen.Recording.2022-01-20.at.11.33.28.mov

@marcaaron
Copy link
Contributor Author

for some reason I'm stuck in step 5; the button Instant (Debit Card) is selected but the Transfer button is disabled also I can't select the other option 1-3 Business Days (Bank Account)

Which step 5? The QA for this PR is to just make sure you can get to the "Transfer Balance" page and looks like you were able to do it :)

@marcaaron
Copy link
Contributor Author

I'll add some pics to show what I'm looking for here.

@marcaaron
Copy link
Contributor Author

Just tested the flow again and added screenshots. @marcochavezf let me know if everything looks good to you!

@marcochavezf
Copy link
Contributor

Oh I see, thanks! I was using an account that was skipping the "Verify identity" step

Copy link
Contributor

@stitesExpensify stitesExpensify left a comment

Choose a reason for hiding this comment

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

Nice!

@stitesExpensify stitesExpensify merged commit 5ae1d91 into main Jan 20, 2022
@stitesExpensify stitesExpensify deleted the marcaaron-transferBalanceKYC branch January 20, 2022 20:54
@OSBotify
Copy link
Contributor

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

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @stitesExpensify in version: 1.1.31-2 🚀

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

@mvtglobally
Copy link

@marcaaron @marcochavezf @nickmurray47 @stitesExpensify This PR is ready to QA internally

@marcaaron
Copy link
Contributor Author

Thanks, there is an issue where we can't access the production API so we will need to test this on production. This feature is still behind a beta so we can check this off I think.

#7371

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @AndrewGable in version: 1.1.32-0 🚀

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.

6 participants