Add ChooseTransferAccountPage and allow selecting accounts for wallet transfer #7200

parasharrajat · 2022-01-13T03:08:02Z

Details

Fixed Issues

$ #3922

Tests | QA Steps

Open Payments page.
Click TransferBalance Link.
Confirm that the transfer button is disabled if the following conditions are true.
i. Transfer Amount is less than $ 0.30.
ii. Which Account is not selected.
Click instant Mode.
It should take you to the new page where you will select an account. This page should show you Debit cards only and Add Debit Card Button.
Clicking + Add Debit Card Button opens a new page to add Debit Card.
From the Select Account page, click on the any debit card.
You should be taken to the old page where you can see the selected debit card as Which account?
If your balance is > Fee then Transfer Button should be active and it should show the transfer amount - fee.
Click 1-3 Business Days Mode.
It should take you to the new page where you will select an account. This page should show you Bank Accounts only and Add bank account Button.
Clicking + Add bank Account Button opens a new page to add bank Account.
From the Select Account page, click on any bank account.
You should be taken to the old page where you can see the selected bank account card as Which account?
If your balance is > Fee then Transfer Button should be active and it should show the transfer amount - fee.
If your balance is > Fee and which account is set.
i. Click Transfer button.
ii. Now you should be taken to the Payments Page and a confirm modal should be shown.

Tested On

Screenshots

Web | Desktop

output_file.mp4

Mobile Web

iOS

Android

…-amount-scaffold

parasharrajat · 2022-01-13T16:25:02Z

I didn't do these steps

Create a new component that wraps AddPaymentMethodMenu (without modifying this component too much) and call it “AddBalanceTransferAccountButton”

Create a version of the PaymentMethodList that is selectable. It should not duplicate the logic, but wrap PaymentMethodList.

I used MenuItem directly as there was no need to add Popover and disabled the add Payment method button from the paymentMethodList via a prop. There was no new logic needed for the SelectablePaymentMethodList so I used passed the props to filter the list and show the checkboxes. I don't see any other way of creating a component from PaymentMethodList without passing these props.

parasharrajat · 2022-01-13T16:41:57Z

cc: @marcaaron

src/libs/actions/PaymentMethods.js

src/pages/settings/Payments/ChooseTransferAccountPage.js

src/pages/settings/Payments/TransferBalancePage.js

parasharrajat · 2022-01-13T17:38:27Z

Updated.

src/libs/actions/PaymentMethods.js

stitesExpensify

Testing went pretty well, it's looking good! One bug I found was that when you click add debit card, it always sends you to the add bank account page

src/pages/settings/Payments/ChooseTransferAccountPage.js

parasharrajat · 2022-01-14T01:42:48Z

Updated @stitesExpensify @marcaaron .

stitesExpensify

Code looks pretty good to me. I found 2 more bugs while testing though.

If you have 2 accounts and go to the transfer page, then click the ACH button, neither account is selected even though one was selected on the main page

If you have a debit card set as your default, it is not shown on the main transfer page

parasharrajat · 2022-01-17T17:53:24Z

How can I set the default Payment Method? @stitesExpensify

stitesExpensify · 2022-01-17T17:59:43Z

The userWallet key in onyx can be manually modified for now to have values like this:

{"availableBalance":12345,"bankAccountID":8,"currentBalance":12345,"currentStep":"","tier":1,"tierName":"SILVER","walletLinkedAccountID":1,"walletLinkedAccountType":"debitCard"}

You can substitute the walletLinkedAccountID and walletLinkedAccountType to be whatever you want to test with

…-amount-scaffold

parasharrajat · 2022-01-18T18:13:12Z

Ok. Fixed both issues.

I was never using the default account on Choose Account screen.
I was using the wrong methodID for debit cards. It should be fundID.

Updated. cc: @stitesExpensify

stitesExpensify · 2022-01-19T01:09:23Z

I'm still running into some issues while testing. Debit cards seem to work, but if I make a bank account my default the selected account won't change

2022-01-18_18-06-34.mp4

parasharrajat · 2022-01-19T18:23:09Z

Interesting issue. I can't think of the reason why this is happening but I have two accounts now and I will try to test it.

…-amount-scaffold

parasharrajat · 2022-01-21T01:44:04Z

Finally, fixed. Thanks for catching these issues.

I was doing the wrong matching to get the SelectedAccount. It was picking the Defaultaccount if that comes before in the position the selected account in the payment method Array. Took me some time to figure it out.

Lesson learned: Always criticize your own code.

Ready for review @marcaaron @stitesExpensify

parasharrajat · 2022-01-21T20:23:35Z

This is ready @marcaaron @stitesExpensify . Sorry for the ping.

marcaaron · 2022-01-21T20:35:22Z

No worries, but there are guidelines here about when it is appropriate to ping if you need a reminder.

marcaaron

Approving since the code is acceptable. Left some non blocking comments and ideas for improvements that can potentially be addressed in follow ups.

marcaaron · 2022-01-24T18:17:09Z

src/pages/settings/Payments/ChooseTransferAccountPage.js

+                : account.fundID,
+        );
+        Navigation.navigate(ROUTES.SETTINGS_PAYMENTS_TRANSFER_BALANCE);
+    };


NAB, this method isn't using props so we are redeclaring it on every render. Not that this will make a huge difference performance-wise, but it feels like this is not the correct scope to declare it.

Yeah, I agree but I didn't see any example of such a functional component. Where should I move this. Outside the component in the same file?

Yeah outside the component in the same file would work. Also maybe a good sign that it should be an action instead of a component method.

marcaaron · 2022-01-24T18:17:39Z

src/pages/settings/Payments/ChooseTransferAccountPage.js

+                ? account.bankAccountID
+                : account.fundID,
+        );
+        Navigation.navigate(ROUTES.SETTINGS_PAYMENTS_TRANSFER_BALANCE);


If we are going back can we just do Navigation.goBack() ?

Whatever we do maybe it should be consistent because we are using Navigation.goBack() here

App/src/pages/settings/Payments/ChooseTransferAccountPage.js

Line 65 in 0043cec

onBackButtonPress={() => Navigation.goBack()}

marcaaron · 2022-01-24T18:22:26Z

src/pages/settings/Payments/ChooseTransferAccountPage.js

+                        shouldShowSelectedState
+                        filterType={props.walletTransfer.filterPaymentMethodType}
+                        selectedMethodID={props.walletTransfer.selectedAccountID}
+                        shouldShowAddPaymentMethodButton={false}


NAB, but it would be better to eventually find the case where we DO want to show an add payment method button and show one next to the list instead of inside of it. Now we have this list that must specify "no thanks, I don't need this". And in this case we just shows a very similar button below which could be confusing.

Maybe a good follow up issue to create and have someone fix up.

Yeah, I was thinking th same but then it would unnecessary refactor for this PR so I left it as it is.

marcaaron · 2022-01-24T18:54:55Z

src/pages/settings/Payments/TransferBalancePage.js

+        PaymentMethods.saveWalletTransferAccountTypeAndID(
+            selectedAccount.accountType,
+            selectedAccount.methodID,
+        );


NAB, the more I look at this the more wrong it seems. We are saving data that can be derived from looking at other data and just sort of seems like stuff this component shouldn't be responsible for.

Maybe it says "when I am first loaded then set the most likely default payment method", but I don't think it would be this component's responsibility to filter everything.

Yeah that is true. Let me see if I can clean this up bit.

marcaaron · 2022-01-24T18:59:40Z

src/pages/settings/Payments/TransferBalancePage.js

+        PaymentMethods.saveWalletTransferMethodType(filterPaymentMethodType);
+        Navigation.navigate(ROUTES.SETTINGS_PAYMENTS_CHOOSE_TRANSFER_ACCOUNT);
+    }
+
    render() {
        const selectedAccount = this.getSelectedPaymentMethodAccount();


NAB, feels like we are unnecessarily filtering through all the payment methods to derive this and giving a lot of responsibility to this component.

e.g. if we are initializing the payment method type in walletTransfer. accountType and it is based on the value in getSelectedPaymentMethodAccount() then we should be able to just look in walletTransfer? Or no?

When we come back from Choose Transfer Account screen. This helps in picking the selected method. Either we should do that in componentDidUpdate or here.

Yeah I'm not sure that's easy to see or why that would be the case. It maybe works, but the whole interaction between the different sets of data and why we are filtering them on every render is confusing.

stitesExpensify

Code looks good, and it tests well! Nice job 😄

OSBotify · 2022-01-24T23:01:07Z

✋ 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-25T20:33:47Z

🚀 Deployed to staging by @stitesExpensify in version: 1.1.32-1 🚀

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

OSBotify · 2022-02-01T18:04:51Z

🚀 Deployed to production by @roryabraham in version: 1.1.33-3 🚀

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

parasharrajat added 8 commits January 13, 2022 07:36

Allow hiding the Add payment method button on Payment Method list

97c8e99

Added ChooseTransferAccontpage

8660d64

set navigation between choose account and transfer page

05f8a7c

add methods

ff85915

Merge branch 'main' of github.com:Expensify/Expensify.cash into trans…

967e4ba

…-amount-scaffold

Apply filters correctly

00f4106

Reset data on every transfer

93fdbb6

make filtering optional for PaymentMethodList

12c62e0

parasharrajat mentioned this pull request Jan 13, 2022

[HOLD for payment 2022-02-08] [HOLD for payment 2022-01-25] Add the Transfer Balance Page #3922

Closed

5 tasks

parasharrajat added 2 commits January 13, 2022 08:45

UI tweaks

e3f1b4f

rename method names

b480379

parasharrajat marked this pull request as ready for review January 13, 2022 16:14

parasharrajat requested a review from a team as a code owner January 13, 2022 16:14

MelvinBot requested review from stitesExpensify and removed request for a team January 13, 2022 16:14

translations fixed

ec1782c

marcaaron requested changes Jan 13, 2022

View reviewed changes

Refactored names, clean up and comments

cc5e4a5

parasharrajat requested a review from marcaaron January 13, 2022 17:41

remove unused imports

c43bd3a

marcaaron reviewed Jan 13, 2022

View reviewed changes

src/libs/actions/PaymentMethods.js Outdated Show resolved Hide resolved

stitesExpensify reviewed Jan 14, 2022

View reviewed changes

src/pages/settings/Payments/ChooseTransferAccountPage.js Outdated Show resolved Hide resolved

rename and props correction

0136230

parasharrajat requested review from stitesExpensify and marcaaron January 14, 2022 01:42

stitesExpensify requested changes Jan 14, 2022

View reviewed changes

select default account

7751d73

parasharrajat added 5 commits January 18, 2022 22:43

Merge branch 'main' of github.com:Expensify/Expensify.cash into trans…

0bb65ca

…-amount-scaffold

Adopt changes from latest changes

cd1ddba

fix: PaymentMethodlist filtering

0dcf5f8

more refactoring

8eacf33

preselect default account

39b6327

parasharrajat added 2 commits January 21, 2022 05:13

Merge branch 'main' of github.com:Expensify/Expensify.cash into trans…

41feaa9

…-amount-scaffold

Fix code to get the selected Account

0043cec

parasharrajat requested a review from stitesExpensify January 21, 2022 20:22

marcaaron approved these changes Jan 24, 2022

View reviewed changes

stitesExpensify approved these changes Jan 24, 2022

View reviewed changes

stitesExpensify merged commit 921fa4f into Expensify:main Jan 24, 2022

OSBotify mentioned this pull request Jan 25, 2022

Deploy Checklist: New Expensify 2022-01-25 #7395

Closed

46 tasks

kavimuru mentioned this pull request Jan 25, 2022

Payments - Options "instant debit card" and "1-3 business days" aren't displayed when clicking "Transfer Balance" #7398

Closed

rushatgabhane mentioned this pull request May 25, 2023

[HOLD for payment 2023-05-16] [$1000] Inconsistent margin above 'Payment methods' title #17861

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ChooseTransferAccountPage and allow selecting accounts for wallet transfer #7200

Add ChooseTransferAccountPage and allow selecting accounts for wallet transfer #7200

parasharrajat commented Jan 13, 2022 •

edited

Loading

parasharrajat commented Jan 13, 2022 •

edited

Loading

parasharrajat commented Jan 13, 2022

parasharrajat commented Jan 13, 2022

stitesExpensify left a comment

parasharrajat commented Jan 14, 2022

stitesExpensify left a comment

parasharrajat commented Jan 17, 2022 •

edited

Loading

stitesExpensify commented Jan 17, 2022

parasharrajat commented Jan 18, 2022

stitesExpensify commented Jan 19, 2022

parasharrajat commented Jan 19, 2022

parasharrajat commented Jan 21, 2022 •

edited

Loading

parasharrajat commented Jan 21, 2022 •

edited

Loading

marcaaron commented Jan 21, 2022

marcaaron left a comment

marcaaron Jan 24, 2022

parasharrajat Jan 24, 2022

marcaaron Jan 25, 2022

marcaaron Jan 24, 2022

marcaaron Jan 24, 2022

marcaaron Jan 24, 2022

parasharrajat Jan 24, 2022

marcaaron Jan 24, 2022

parasharrajat Jan 24, 2022

marcaaron Jan 24, 2022

marcaaron Jan 24, 2022

parasharrajat Jan 24, 2022

marcaaron Jan 25, 2022

stitesExpensify left a comment

OSBotify commented Jan 24, 2022

OSBotify commented Jan 25, 2022

OSBotify commented Feb 1, 2022

Add ChooseTransferAccountPage and allow selecting accounts for wallet transfer #7200

Add ChooseTransferAccountPage and allow selecting accounts for wallet transfer #7200

Conversation

parasharrajat commented Jan 13, 2022 • edited Loading

Details

Fixed Issues

Tests | QA Steps

Tested On

Screenshots

Web | Desktop

Mobile Web

iOS

Android

parasharrajat commented Jan 13, 2022 • edited Loading

parasharrajat commented Jan 13, 2022

parasharrajat commented Jan 13, 2022

stitesExpensify left a comment

Choose a reason for hiding this comment

parasharrajat commented Jan 14, 2022

stitesExpensify left a comment

Choose a reason for hiding this comment

parasharrajat commented Jan 17, 2022 • edited Loading

stitesExpensify commented Jan 17, 2022

parasharrajat commented Jan 18, 2022

stitesExpensify commented Jan 19, 2022

parasharrajat commented Jan 19, 2022

parasharrajat commented Jan 21, 2022 • edited Loading

parasharrajat commented Jan 21, 2022 • edited Loading

marcaaron commented Jan 21, 2022

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stitesExpensify left a comment

Choose a reason for hiding this comment

OSBotify commented Jan 24, 2022

OSBotify commented Jan 25, 2022

OSBotify commented Feb 1, 2022

parasharrajat commented Jan 13, 2022 •

edited

Loading

parasharrajat commented Jan 13, 2022 •

edited

Loading

parasharrajat commented Jan 17, 2022 •

edited

Loading

parasharrajat commented Jan 21, 2022 •

edited

Loading

parasharrajat commented Jan 21, 2022 •

edited

Loading