Incorporate Add Bank Account / KYC checks into Send Money + Pay request flow #6659

marcaaron · 2021-12-09T00:53:52Z

Details

Tracking issue and additional context: https://github.com/Expensify/Expensify/issues/185320

This PR adds a way to effectively "block" a specific user action that requires a gold wallet and then to "continue" that action once they successfully make it through these flows. Since the flows all happen on different pages/screens it is tricky to enable the "continue" part of whatever user action they were taking. By using a "pay wall" style component we can save a reference to the component whenever it is mounted and call it's defined "continue" callback.

Holding on Return promise in payIOUReport / Fix AdditionalDetailsStep #6992 as the AdditionalDetailsStep was recently broken and the test steps here can't be completed until this is fixed.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/187804
$ https://github.com/Expensify/Expensify/issues/187803

Tests

Start with an account that has a SILVER wallet
Attempt to Send Money to someone else
Verify you are asked to add a payment method with a popover
Select bank account and verify you are brought to the Plaid flow and go through it
On success verify, you are next brought to the KYC flow and follow the second set of instructions here which explain how to test the ActivateWallet flow on dev -> https://stackoverflow.com/c/expensify/questions/10607
Once you successfully get through this flow the original action you wanted to take (send money) should succeed
Repeat with a new account but this time test paying a request
Logout after adding a bank account + passing KYC checks then log back in and verify that you do not hit the KYC wall anymore.

QA Steps

Internal QA

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

…Button later

marcaaron · 2021-12-10T01:37:04Z

I'm at an interesting spot with this stuff. A few things are becoming clear...

These two issues are basically the same with the main difference being that when we push one of the 3 screens (enable payments, add bank, add debit) we'll want it to go on top of the correct stack that we're already on:
- https://github.com/Expensify/Expensify/issues/187804
- https://github.com/Expensify/Expensify/issues/187803
I think that means we'll need unique routes for all of this stuff and need to pass these in to the SettlementButton
The logic for the SettlementButton ideally should work independent of the SettlementButton itself so it can also work with the Transfer Balance button. @parasharrajat is working on that over in Feature: Transfer balance #4177
There's another tricky part of this to handle which is basically... what happens when we successfully do any of these and need to return to the previous screen and trigger the original event? The possibilities are...
- add a debit card -> move the user to KYC flow
- add a bank account -> move the user to KYC flow
- enable payments -> continue sending money, transferring balance, or paying request
Now the fun part... each time we do one of those we will dismiss the page by calling goBack() or navigating away on success or failure.
However, the screen underneath will need some logic to continue whatever the user was trying to do before.

I'm unsure the best way to achieve the "continue" logic just yet, but have a few of ideas...

Navigate back to the previous page with some params like {previousRoute: 'bankAccount', success: true} then wait for these route params in componentDidUpdate(), clear them out, and trigger an onContinue() callback which will basically perform the original action again and route us to the next place.
Use some kind of subscription to trigger an event and subscribe to the event where we place the popover then call the onContinue() callback. Instead of "navigating with params" we will navigate() or goBack() and also publish an event like EVENT.CONTINUE_PAYMENTS_SETUP.
Similar to 2 we can set a global onEnablePaymentsContinueCallback() when our component mounts and clear it out when the component unmounts. Then we can simply call this callback whenever we successfully complete any of the three actions.
Onyx based solution. Set some params for the success or failure of any of the actions we tried to take after they are successful or unsuccessful. In the previous screen check to see if these params did not exist before but are now updated in componentDidUpdate(). Handle them once then clear them out.
Navigate to these routes, but use the screen name and params instead of the linkTo() method. This should allow us to pass a callback as a param when navigating.

~~The last option seems to be the easiest so that's what I'll try first.~~ Actually ended up not liking that last option because ideally there will only ever be one way to "navigate" and we have avoided "navigate with a callback" thus far.

marcaaron · 2021-12-14T00:38:56Z

Solution 3 ended up being the one that makes the most sense to me.

marcaaron · 2022-01-03T21:03:43Z

Found a better solution than everything else that I thought of which is to create a "pay wall" type component where if the user has not completed checks or added payment method we force them to complete the flow and then "continue" whatever the original action is after. The component uses a ref similar to how the Growl component/interface works.

marcaaron · 2022-01-04T21:28:57Z

This is on HOLD still but taking off draft to get some initial thoughts on the overall implementation etc.

marcaaron · 2022-01-06T23:02:37Z

Before merging @marcaaron, also what is the expectation of how this flow should work when someone already has a GOLD wallet? They shouldn't have to add a bank account, right?

No, you must add a bank account or debit card in order to do any of these things otherwise you can't pay with the wallet.

… issue.

nickmurray47 · 2022-01-06T23:35:03Z

cool, just wanted to make sure

nickmurray47

👍 will leave to you @danieldoglas

danieldoglas · 2022-01-07T21:00:37Z

I'm reviewing and testing this. Not really used to this flow so taking some time.

marcaaron · 2022-01-07T21:04:27Z

Gonna also do some quick tests on the platforms. Just realized this is adding new UI and so should be tested everywhere and have screenshots.

marcaaron · 2022-01-07T21:04:59Z

@danieldoglas please let me know if there are any questions!

danieldoglas

Leaving this as a comment so we can discuss my two comments before approving, but overall LGTM

danieldoglas · 2022-01-07T21:35:17Z

src/libs/Navigation/AppNavigator/ModalStackNavigators.js

+{
+    Component: AddPersonalBankAccountPage,
+    name: 'IOU_Send_Add_Bank_Account',
+},
+{
+    Component: AddDebitCardPage,
+    name: 'IOU_Send_Add_Debit_Card',
+},
 {
    Component: EnablePaymentsPage,
-    name: 'IOU_Enable_Payments',
+    name: 'IOU_Send_Enable_Payments',
 }]);

 const IOUDetailsModalStackNavigator = createModalStackNavigator([{
    Component: IOUDetailsModal,
    name: 'IOU_Details_Root',
+},
+{
+    Component: AddPersonalBankAccountPage,
+    name: 'IOU_Details_Add_Bank_Account',
+},
+{
+    Component: AddDebitCardPage,
+    name: 'IOU_Details_Add_Debit_Card',
+},
+{
+    Component: EnablePaymentsPage,
+    name: 'IOU_Details_Enable_Payments',


As discussed with @marcaaron , I think we're adding some complexity in the code duplicating the AddDebitCardPage and AddPersonalBankAccountPage in two stacks.

If we duplicate them here, we need to pass them as parameters in IOUConfirmationList, SettlementButtton and IOUDetailsModal instead of just using them in KYCWall.

If we can simplify it using only two routes, that would help us in maintenance in the future.

What do you think @nickmurray47 ?

Also, this is NAB.

I like the idea of using generic routes for these then they will exist outside of the underlying stack.

But I think in that case we should not have both < and x in their headers because they would both do the same thing.

This wasn't really ironed out in the doc so I followed the existing convention (we already had IOU_Enable_Payments set up with a unique route).

danieldoglas · 2022-01-07T21:37:39Z

src/CONST.js

+    ADD_PAYMENT_MENU_POSITION_Y: 226,
+    ADD_PAYMENT_MENU_POSITION_X: 356,


Do we want to work with fixed values here independent of the platform and screen size? Just to be sure this will be how we're imagining in every device.

This is a bit of temp code and a follow up created to address this here

marcaaron · 2022-01-07T21:49:10Z

When testing on iOS seems like the an error appears...

Kind of confused because I have not added and ScrollView in this PR so going to check against main to see if the issue exists there.

marcaaron · 2022-01-07T21:52:10Z

Hmm yes the VirtualizedList warning exists on main as well. We should maybe create an issue to look into this - but can ignore it as we are still under beta and it wasn't introduced in this PR.

marcaaron · 2022-01-07T22:18:59Z

Ah I found a bug with some of the logic while testing and updated it. If you add a bank account and then log out and then try to send an IOU again after logging in we will not check the most recent list of payment methods and end up asking the user to enter one again.

@nickmurray47 mind re-reviewing?

nickmurray47 · 2022-01-07T22:41:54Z

Yeah, can take another look! catching up on context right now

marcaaron · 2022-01-07T22:59:46Z

The ScrollView thing is being handled here #7088

nickmurray47

Nice, LGTM! Thanks for adding the screenshots in for the other platforms, I should've asked for those in the beginning.

Do you also mind adding just one last step to QA:
8. Logout after adding a bank account, log back in and verify that you do not hit the pay wall

danieldoglas

LGTM

OSBotify · 2022-01-10T19:44:44Z

✋ 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 · 2022-01-12T05:54:29Z

🚀 Deployed to staging by @danieldoglas in version: 1.1.27-2 🚀

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

OSBotify · 2022-01-12T11:32:34Z

🚀 Deployed to staging by @danieldoglas in version: 1.1.27-3 🚀

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

OSBotify · 2022-01-12T18:54:37Z

🚀 Deployed to staging by @danieldoglas in version: 1.1.27-3 🚀

platform	result
🤖 android 🤖	skipped 🚫
🖥 desktop 🖥	skipped 🚫
🍎 iOS 🍎	skipped 🚫
🕸 web 🕸	skipped 🚫

mvtglobally · 2022-01-12T20:35:45Z

@marcaaron @danieldoglas @nickmurray47 PR is ready to be QA-ed internally

marcaaron · 2022-01-12T22:50:43Z

@mvtglobally This can be checked off. It's behind a beta and has been tested on staging.

OSBotify · 2022-01-14T19:59:54Z

🚀 Deployed to production by @Jag96 in version: 1.1.29-5 🚀

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

move logic out of IOUModal since it must be re-used in the Settlement…

0fa7a0c

…Button later

marcaaron self-assigned this Dec 9, 2021

marcaaron added 7 commits December 8, 2021 15:19

rm userWallet

f2d7fb3

something something anchorposition is weird

0089dba

fix conflicts

a34ad57

Move the popover to the Settlement Button

c306681

remove unneeded new line

ea66c54

add IOU Details routes

81da411

fix event

94c49ae

marcaaron added 5 commits December 9, 2021 16:07

rm comment

e5f8fe7

idk if this is the right direction here

75df6c5

undo bad idea

7a38aa1

Add continue setup flow

cc83c3c

Only progress after setting wallet account data

ba5df99

marcaaron added 8 commits December 20, 2021 10:17

Make it so the KYCWall can be re-used for Transfer Balance flow

d43fd33

Merge remote-tracking branch 'origin' into marcaaron-checkPaymentMethods

210b171

remove growl

6636535

remove unused import

e5c42be

add propTypes

526bdac

Use a ref instead of a callback

28c54e7

Fix conflicts

0d42f58

Fix PaymentUtils bad import

0ea560f

marcaaron changed the title ~~[WIP] Incorporate Add Bank Account / KYC checks into Send Money flow~~ [WIP] Incorporate Add Bank Account / KYC checks into Send Money + Pay request flow Jan 3, 2022

marcaaron changed the title ~~[WIP] Incorporate Add Bank Account / KYC checks into Send Money + Pay request flow~~ [HOLD #6992] Incorporate Add Bank Account / KYC checks into Send Money + Pay request flow Jan 3, 2022

marcaaron changed the title ~~[HOLD #6992] Incorporate Add Bank Account / KYC checks into Send Money + Pay request flow~~ [HOLD PR 6992] Incorporate Add Bank Account / KYC checks into Send Money + Pay request flow Jan 3, 2022

marcaaron marked this pull request as ready for review January 4, 2022 21:28

marcaaron dismissed nickmurray47’s stale review via 3c2c00c January 6, 2022 23:00

marcaaron added 3 commits January 6, 2022 13:28

Add constants for add payment menu position. Will handle in follow up…

8519f49

… issue.

Merge remote-tracking branch 'origin' into marcaaron-checkPaymentMethods

16b5a4e

add ROUTES

6e4afbc

nickmurray47 previously approved these changes Jan 7, 2022

View reviewed changes

danieldoglas reviewed Jan 7, 2022

View reviewed changes

Load payment methods whenever KYCWall is used

5f88dff

marcaaron dismissed nickmurray47’s stale review via 5f88dff January 7, 2022 22:17

nickmurray47 approved these changes Jan 7, 2022

View reviewed changes

danieldoglas approved these changes Jan 10, 2022

View reviewed changes

danieldoglas merged commit 6803d00 into main Jan 10, 2022

danieldoglas deleted the marcaaron-checkPaymentMethods branch January 10, 2022 19:43

OSBotify mentioned this pull request Jan 12, 2022

Deploy Checklist: New Expensify 2022-01-12 #7144

Closed

54 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Incorporate Add Bank Account / KYC checks into Send Money + Pay request flow #6659

Incorporate Add Bank Account / KYC checks into Send Money + Pay request flow #6659

marcaaron commented Dec 9, 2021 •

edited

Loading

marcaaron commented Dec 10, 2021 •

edited

Loading

marcaaron commented Dec 14, 2021

marcaaron commented Jan 3, 2022

marcaaron commented Jan 4, 2022

marcaaron commented Jan 6, 2022

nickmurray47 commented Jan 6, 2022

nickmurray47 left a comment

danieldoglas commented Jan 7, 2022

marcaaron commented Jan 7, 2022

marcaaron commented Jan 7, 2022

danieldoglas left a comment

danieldoglas Jan 7, 2022

marcaaron Jan 7, 2022

danieldoglas Jan 7, 2022

marcaaron Jan 7, 2022

marcaaron commented Jan 7, 2022

marcaaron commented Jan 7, 2022

marcaaron commented Jan 7, 2022

nickmurray47 commented Jan 7, 2022

marcaaron commented Jan 7, 2022

nickmurray47 left a comment

danieldoglas left a comment

OSBotify commented Jan 10, 2022

OSBotify commented Jan 12, 2022

OSBotify commented Jan 12, 2022

OSBotify commented Jan 12, 2022

mvtglobally commented Jan 12, 2022

marcaaron commented Jan 12, 2022

OSBotify commented Jan 14, 2022

		ADD_PAYMENT_MENU_POSITION_Y: 226,
		ADD_PAYMENT_MENU_POSITION_X: 356,

Incorporate Add Bank Account / KYC checks into Send Money + Pay request flow #6659

Incorporate Add Bank Account / KYC checks into Send Money + Pay request flow #6659

Conversation

marcaaron commented Dec 9, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

marcaaron commented Dec 10, 2021 • edited Loading

marcaaron commented Dec 14, 2021

marcaaron commented Jan 3, 2022

marcaaron commented Jan 4, 2022

marcaaron commented Jan 6, 2022

nickmurray47 commented Jan 6, 2022

nickmurray47 left a comment

Choose a reason for hiding this comment

danieldoglas commented Jan 7, 2022

marcaaron commented Jan 7, 2022

marcaaron commented Jan 7, 2022

danieldoglas left a comment

Choose a reason for hiding this comment

danieldoglas Jan 7, 2022

Choose a reason for hiding this comment

marcaaron Jan 7, 2022

Choose a reason for hiding this comment

danieldoglas Jan 7, 2022

Choose a reason for hiding this comment

marcaaron Jan 7, 2022

Choose a reason for hiding this comment

marcaaron commented Jan 7, 2022

marcaaron commented Jan 7, 2022

marcaaron commented Jan 7, 2022

nickmurray47 commented Jan 7, 2022

marcaaron commented Jan 7, 2022

nickmurray47 left a comment

Choose a reason for hiding this comment

danieldoglas left a comment

Choose a reason for hiding this comment

OSBotify commented Jan 10, 2022

OSBotify commented Jan 12, 2022

OSBotify commented Jan 12, 2022

OSBotify commented Jan 12, 2022

mvtglobally commented Jan 12, 2022

marcaaron commented Jan 12, 2022

OSBotify commented Jan 14, 2022

marcaaron commented Dec 9, 2021 •

edited

Loading

marcaaron commented Dec 10, 2021 •

edited

Loading