Feature: Transfer balance #4177

parasharrajat · 2021-07-22T18:43:05Z

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

output_file.mp4

Mobile Web

Desktop

iOS

Android

…-amount

src/ONYXKEYS.js

src/languages/es.js

src/libs/actions/PaymentMethods.js

src/pages/settings/Payments/PaymentsPage.js

src/pages/settings/Payments/paymentPropTypes.js

src/pages/settings/Payments/TransferBalancePage.js

parasharrajat · 2021-08-13T20:28:58Z

cc: @stitesExpensify .Please test it to see the flow. I got no balance 💰 to see the transfer.
Questions:

What parameters does the API_method take?
How do I need to pass the Payment Mode (ACH or INSTANT)?

only this part is left and other things are done. Please review it if you have some balance in your wallet and let me know what is missing. Thanks.

parasharrajat · 2021-08-16T11:53:18Z

Please don't merge it yet

robertjchen · 2021-08-16T12:14:37Z

Yep, I'm deferring to @stitesExpensify for final review on the final two points: #4177 (comment)

parasharrajat · 2021-08-27T16:39:55Z

@stitesExpensify Do you think we can move forward now or there is still something holding it back?

stitesExpensify

Hi @parasharrajat, Sorry for the delay I've been hyperfocused on an internal project, but will now be available per usual. Just some small review comments here, but there was actually a misunderstanding when I wrote the issue that I will describe in another post.

stitesExpensify · 2021-08-17T18:27:39Z

src/libs/actions/PaymentMethods.js

+            Onyx.merge(ONYXKEYS.USER_WALLET, {balance: 0});
+            Onyx.merge(ONYXKEYS.WALLET_TRANSFER, {completed: true});
+        }).catch((error) => {
+            console.debug(`[Payments] Failed to transfer wallet balance: ${error.message}`);


Should we show something to the user here too?

I am yet to finish the API call part. I was awaiting answers to few questions #4177 (comment)

Better to actually log something to the server instead of console.debug which is only shown in development

src/pages/settings/Payments/ChooseTransferAccountPage.js

src/pages/settings/Payments/PaymentsPage.js

stitesExpensify · 2021-09-01T22:19:58Z

👋 As I mentioned, there was a misunderstanding in the original issue proposal that needs to be taken care of. My apologies for the changes.

The instant option can actually only be chosen when a debit card is selected, and the 1-3 business days option can only be chosen when using a bank account.

For this reason we want to change the choices to say

- Instant (Debit Card)
- 1-3 business days (Bank Account)

When the user loads into the page, whatever their default account is should decide which transfer option is chosen.

If the user switches the transfer type, we need to send them to the choose/add account page and show only their debit cards, or only their bank accounts (whichever is relevant). The button should also say "add debit card" or "add bank account" depending on which type of account we are showing.

If the user goes to the "which account" page and chooses an account, the transfer type should change based on the account type.

If you have any questions feel free to reach out, I'm back to being very available. Thanks again for your patience!

stitesExpensify · 2021-09-03T16:33:08Z

What parameters does the API_method take?

The method only takes 1 param, bankAccountID for bank accounts, or fundID for cards. Also since I'm not sure if I mentioned it, the method is on the normal server, not secure

How do I need to pass the Payment Mode (ACH or INSTANT)?

Like I mentioned in my above comment, we won't be passing that anymore, it will be automatic based on the account type.

stitesExpensify · 2021-09-03T18:16:40Z

Testing notes:

We want both a back button and close button on the transfer page (missing from mockup, sorry)
The fee at the bottom of the page is static, it should be based on the wallet balance, and not show up for 1-3 business days
The bank account does not have an icon, it is static as the instant icon (we will have a method for that soon)
When the balance is transferred we need to set the wallet's current balance to 0 in onyx
We should disable the transfer button if the user's currentBalance === 0

marcaaron

Sure you're still cleaning things up but have a few more high level comments.

src/components/RadioBox.js

marcaaron · 2021-12-10T21:58:24Z

src/pages/EnablePayments/index.js


 // Steps
 import OnfidoStep from './OnfidoStep';
 import AdditionalDetailsStep from './AdditionalDetailsStep';
 import TermsStep from './TermsStep';
 import ActivateStep from './ActivateStep';
+import {userWalletPropTypes} from '../settings/Payments/paymentPropTypes';


I appreciate the thought to improve it, but let's start with the necessary changes as it just makes it easier to review large features like this.

marcaaron · 2021-12-10T22:03:05Z

src/pages/settings/Payments/ChooseTransferAccountPage.js

+     * @param {Object} nativeEvent
+     * @param {String} accountID id of the selected account.
+     */
+    paymentMethodSelected(nativeEvent, accountID) {


Is the nativeEvent used?

Also this should be called something like setPaymentMethodAndNavigate()

Yeah, this is used to anchor the popover menu on the PaymentsPage.

I don't see where it's used in this method. Why is it being passed to this method?

PaymentsPage was expecting the first parameter to be an event object so I had to define one for keeping the same func defintion. But I think as per your prev comment, we should divide this into two methods and the problem will be solved.

src/pages/settings/Payments/ChooseTransferAccountPage.js

marcaaron · 2021-12-10T22:10:29Z

src/pages/settings/Payments/ChooseTransferAccountPage.js

+                            enableSelection
+                            filterType={this.props.walletTransfer.filterPaymentMethodType}
+                            selectedAccountID={this.props.walletTransfer.selectedAccountID}
+                        />


Shouldn't there be an + Add Payment Method button next to this list?

That button is handled inside the paymentMethodList

Yes it's duplicating logic see the comment below this one 😄

It is not completely duplicating the code. It is just changing the title of that button based on the payment method and we can directly use that button to navigate to the adding selected payment method type. I don't think it is good to show the popover menu if there is always going to be one menu item.

And again, the same component is used on that page.

I don't think it is good to show the popover menu if there is always going to be one menu item.

I agree, but that's not what I'm asking. We can choose to only display the popover when there are multiple. Keeping the logic all contained together is the important piece.

Anyway, this is not really negotiable. We're using this component in another PR as part of the same project and it makes 100% sense to use it here - please trust me and make the change.

Sure, I will do that change.
I will move the + Add Payment Method button to that component and use it. I think it would make more sense then. and filter it based on prop.

marcaaron · 2021-12-10T22:12:22Z

src/pages/settings/Payments/PaymentMethodList.js

+            case CONST.PAYMENT_METHODS.BANK_ACCOUNT: addPaymentMethodButtonTitle = this.props.translate('paymentMethodList.addBankAccount'); break;
+            case CONST.PAYMENT_METHODS.DEBIT_CARD: addPaymentMethodButtonTitle = this.props.translate('paymentMethodList.addDebitCard'); break;
+            default: break;
+        }


We should be using the AddPaymentMethodMenu for this.

But on this page ChooseTransferAccountPage.js, only one action is shown based on the type of paymentMethods shown.
For example,
In the list is bank accounts,
then + Add Bank Account and AddPaymentMethodMenu is a menu with all options.

Ok, well we have a component that basically does the same thing, but without being able to change the title. Maybe you can think of some ways to re-use it and the make it do whatever we need with props?

marcaaron

Ok slowly starting to understand these changes... sorry it's taking me a while. Some more comments...

src/pages/settings/Payments/PaymentMethodList.js

marcaaron · 2021-12-10T22:54:57Z

src/pages/settings/Payments/TransferBalancePage.js

+                type: CONST.PAYMENT_METHODS.BANK_ACCOUNT,
+            },
+        ];
+        PaymentMethods.startWalletTransfer(this.props.userWallet.currentBalance - Fee);


What does it mean to "start" a wallet transfer?

I needed to use the set for the first time use to I had to create separate action which usages Onyx.set and later operations are handled by Onyx.merge so for that purpose I created updateWalletTransfer. What do you suggest here?

I needed to use the set for the first time use to I had to create separate action which usages Onyx.set and later operations are handled by Onyx.merge so for that purpose I created updateWalletTransfer

Sorry, I don't get it. What does it mean to "start" a wallet transfer?

We have to set some data that is necessary to manage the component state before users interact with anything on Wallet TransferPage. What about renaming it setWalletTransferInitialData

Can we try to think of a name that communicates why we need to do this? Which state needs to be reset and how are we using it?

Oh yeah, setTransferAmountAndSelectedAccount. the three principles.

src/pages/settings/Payments/TransferBalancePage.js

parasharrajat · 2021-12-12T17:38:21Z

PR updated. The remaining changes either need more clarification or are not needed. Video is added for Web to check the new flow and the rest will be added after we are done with changes. Test steps are added as well.

marcaaron

There are quite a few more issues to resolve.

src/libs/API.js

marcaaron · 2021-12-13T19:08:59Z

src/libs/PaymentUtils.js

+    const combinedPaymentMethods = [];
+
+    _.each(bankAccountList, (bankAccount) => {
+        // Add all bank accounts besides the wallet


It's not clear why the wallet should not be in this list.

It would be better to remove the wallet wherever we don't want to include it. The caller should decide which to filter out depending on the context.

I don't really know :rofl. Picked this code from existing so no Idea. I would keep it like this for now. Maybe Brandon can help later or a follow-up PR.

engineering team is ready to answer any questions you have in Slack if you are unsure about something.

src/libs/PaymentUtils.js

marcaaron · 2021-12-13T19:10:32Z

src/libs/PaymentUtils.js

+    });
+
+    _.each(cardList, (card) => {
+        // Add all cards besides the "cash" card


Explain why the cash card is not included here.

I don't really know :rofl. Pick this code from existing so no Idea.

marcaaron · 2021-12-13T19:11:57Z

src/libs/actions/PaymentMethods.js

+    return API.TransferWalletBalance(parameters)
+        .then((response) => {
+            if (response.jsonCode !== 200) {
+                throw new Error();


What are we trying to accomplish with this throw?

src/pages/settings/Payments/TransferBalancePage.js

marcaaron · 2021-12-13T20:03:04Z

src/pages/settings/Payments/TransferBalancePage.js

+                        onCloseButtonPress={() => Navigation.dismissModal(true)}
+                    />
+                    <View style={[styles.flex1, styles.flexBasisAuto, styles.justifyContentCenter]}>
+                        <CurrentWalletBalance balanceStyles={[styles.text7XLarge]} />


Where are we getting 7X from? First time I've seen a number next to a font style.

src/pages/settings/Payments/TransferBalancePage.js

parasharrajat · 2021-12-15T23:14:07Z

src/libs/actions/PaymentMethods.js

+ * @param {Number} currentBalance
+ * @param {Number} selectedAccountID
+ */
+function saveWalletTransferAmountAndAccount(currentBalance, selectedAccountID) {


This method is used for sharing data across multiple screens.

Why are the screens not able to share this data?

Does this mean we will have something like a "local transfer" that is temporarily stored until we are ready to make an API request? Let's try to be as descriptive as possible here please. I can help write the comment but I'm confused about why we even need this.

Actually, screens change based on the Route and thus I am following the pattern of using Onyx to share data among navigation screens.

Ok, I think in that case this method name sounds too much like something that would call an API?

What about splitting these up into more specific methods like:

setWalletTransferAmount() and setWalletTransferAccountID()

With a comment like:

The Transfer Balance flow requires the use of several different screens. These methods enables us to share data about the current transfer between those screens.

Probably outside the scope of this issue, but kind of feels like we should have some distinction between "actions" that call APIs and "actions" that just set data.

Probably outside the scope of this issue, but kind of feels like we should have some distinction between "actions" that call APIs and "actions" that just set data.

I don't feel the urge to do it. Probably follow a common naming convention for such functions.

marcaaron

Leaving a few more comments because I noticed more incorrect things 🥲

I think we should maybe stop adding changes at this point and try to discuss a plan to make sure we are on the same page before proceeding further.

marcaaron · 2021-12-16T01:51:48Z

src/components/AddPaymentMethodMenu.js

+            anchorPosition: {
+                top: 0,
+                left: 0,
+            },


We need to use this component in this PR and having it internally track the anchorPosition isn't going to work.

marcaaron · 2021-12-16T01:58:58Z

src/components/AddPaymentMethodMenu.js

+            default: break;
+        }
+        return this.props.translate('paymentMethodList.addPaymentMethod');
+    }


We need to make a new component that:

Has AddPaymentMethodMenu inside it

Shows the different button options and only opens the popover when we don't want one or the other.

AddPaymentMethodMenu itself shouldn't be modified because we need the popover to open when the "Transfer Balance" button is eventually pressed. Does that make sense? There's an issue here describing what the ultimate goal is: https://github.com/Expensify/Expensify/issues/187802

I'd suggest calling it AddPaymentMethodButton

AddPaymentMethodMenu itself shouldn't be modified because we need the popover to open when the "Transfer Balance" button is eventually pressed.

I didn't get it and I don't have access to that issue. But I got the idea.

Ah I'm sorry, I forgot that you cannot see those issues.

marcaaron · 2021-12-16T02:01:20Z

src/components/AddPaymentMethodMenu.js

+                left: position.left + 20,
+            },
+        });
+    }


This would need to go into that other component.

marcaaron · 2021-12-16T02:21:49Z

src/components/AddPaymentMethodMenu.js

+        if (paymentType === CONST.PAYMENT_METHODS.BANK_ACCOUNT) {
+            Navigation.navigate(ROUTES.SETTINGS_ADD_BANK_ACCOUNT);
+            return;
+        }


None of these routes should be in this component because we will eventually need to navigate to different routes depending on the context in which this component will be used. I would suggest passing in the route as a prop to keep this component reusable.

marcaaron · 2021-12-16T02:24:56Z

src/components/MenuItem.js

@@ -124,13 +123,14 @@ const MenuItem = props => (
                            />
                        </View>
                    )}
+                    {props.shouldShowSelectedState && <SelectCircle isChecked={props.isSelected} />}


Just FYI I tried to help out a bit and moved this change to a new PR to lighten things up here.

src/libs/actions/PaymentMethods.js

marcaaron · 2021-12-16T02:35:32Z

src/libs/actions/PaymentMethods.js

+            Onyx.merge(ONYXKEYS.USER_WALLET, {balance: 0});
+            Onyx.merge(ONYXKEYS.WALLET_TRANSFER, {completed: true});
+        }).catch((error) => {
+            console.debug(`[Payments] Failed to transfer wallet balance: ${error.message}`);


Better to actually log something to the server instead of console.debug which is only shown in development

marcaaron · 2021-12-16T02:38:17Z

src/libs/actions/PaymentMethods.js

+ * @param {Number} currentBalance
+ * @param {Number} selectedAccountID
+ */
+function saveWalletTransferAmountAndAccount(currentBalance, selectedAccountID) {


Why are the screens not able to share this data?

marcaaron · 2021-12-16T02:40:31Z

src/libs/actions/PaymentMethods.js

+ * @param {Number} currentBalance
+ * @param {Number} selectedAccountID
+ */
+function saveWalletTransferAmountAndAccount(currentBalance, selectedAccountID) {


Does this mean we will have something like a "local transfer" that is temporarily stored until we are ready to make an API request? Let's try to be as descriptive as possible here please. I can help write the comment but I'm confused about why we even need this.

marcaaron · 2021-12-16T02:44:48Z

src/libs/actions/PaymentMethods.js

+/**
+ * Cancel the wallet transfer
+ */
+function cancelWalletTransfer() {


I think this would be clearer if we added "pending wallet transfer request" to all of these method names? I'm having trouble telling the difference between the action that calls the API and the one that "prepares" the data we need to make a request.

cancelWalletTransfer() sounds like an action that will call the API and cancel something.

parasharrajat · 2021-12-17T17:46:59Z

I agree, lets first discuss the complete plan before I proceed to changes. Please let me know if you have something on your mind.

parasharrajat · 2021-12-17T19:25:01Z

Proceeding to changes...

marcaaron · 2021-12-17T19:37:02Z

Here's a rough idea about how to do this that seems logical to me and can allow us to focus on smaller parts and improve things as we go...

Do a simple PR to scaffold out some of the little changes first and add all the assets, consts, pages, etc:

Add consts
Add Routes
Add blank pages for the new routes we are adding
Translations
API.js change

Move payment methods list building logic into PaymentUtils without modifying the original logic and use in PaymentMethodList. We'll test that everything is still working after the change.
Create the TransferBalancePage and have the account select feature disabled for now - we can test with just the default account and see that transfers can be initiated, errors handled, etc.
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.
Build out the ChooseTransferBalanceAccount page and hook it up to TransferBalancePage so we can select an account.

parasharrajat · 2021-12-17T20:18:44Z

Sounds like a great plan. I will break the PR now. Closing in favor of that.

stitesExpensify · 2021-12-17T23:14:10Z

I'm going to make a tracing issue for this so that we don't miss anything :)

joekaufmanexpensify · 2021-12-20T14:58:38Z

Just to make sure my understanding is correct, the plan is to add this feature across several PRs (using the steps discussed here) rather than all at once?

parasharrajat · 2021-12-20T15:59:49Z

Yup, that's correct.

joekaufmanexpensify · 2021-12-20T17:17:02Z

Sounds good! Is there somewhere I can follow along with the progress for the new PRs?

stitesExpensify · 2021-12-20T17:20:44Z

@joekaufmanexpensify I added a checklist here #3922 (comment)

joekaufmanexpensify · 2021-12-20T17:27:39Z

Great, thanks!

parasharrajat added 13 commits July 18, 2021 03:17

new: Transfer balance page added

465db11

WIP transfer amount page

a094ac4

resolve conflict and Merged latest code

6b5e413

added Choose account page

4caaeeb

navigation setup

6312af1

transfer page Finalized

981faee

Pulled latest changes

46739f1

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

360808e

…-amount

Pull latest changes

1022e5e

completed thee transfer balance flow

ebef54c

added localization

596835c

selection feature to PaymentMethodList

389ddaf

missing translations

62f8bbf

parasharrajat marked this pull request as ready for review August 13, 2021 04:38

parasharrajat requested a review from a team as a code owner August 13, 2021 04:38

MelvinBot requested review from robertjchen and removed request for a team August 13, 2021 04:38

robertjchen requested changes Aug 13, 2021

View reviewed changes

parasharrajat added 2 commits August 15, 2021 01:47

typos

38052af

Pull new changes

10b4d1b

robertjchen requested a review from stitesExpensify August 16, 2021 11:15

robertjchen previously approved these changes Aug 16, 2021

View reviewed changes

stitesExpensify reviewed Sep 1, 2021

View reviewed changes

marcaaron reviewed Dec 10, 2021

View reviewed changes

parasharrajat added 6 commits December 12, 2021 20:38

Props clean up

48c4c0d

Rename and revert extra changes

747197e

Separate Click handlers on paymentMethodList and Cleanup

eaa00c6

Clean up props and Methods

e016f45

Preselect the default account for walltet transfer

57b22d8

parameter checking

83f9bf0

parasharrajat requested a review from marcaaron December 13, 2021 17:43

marcaaron requested changes Dec 13, 2021

View reviewed changes

parasharrajat added 3 commits December 14, 2021 19:43

Rename functions

48d330d

refactor Addpaymentmethod Button

419ab92

Create Selectable versions of PaymentMethodList

5a893ae

parasharrajat commented Dec 15, 2021

View reviewed changes

refactor and cleanup

05520cb

marcaaron requested changes Dec 16, 2021

View reviewed changes

parasharrajat closed this Dec 17, 2021

parasharrajat mentioned this pull request Dec 21, 2021

[WIP]Feature: Copy Markdown/HTML support #6537

Closed

5 tasks

parasharrajat deleted the trans-amount branch November 20, 2023 13:08

Feature: Transfer balance #4177

Feature: Transfer balance #4177

Conversation

parasharrajat commented Jul 22, 2021 • edited Loading

Details

Fixed Issues

Tests | QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

parasharrajat commented Aug 13, 2021 • edited Loading

parasharrajat commented Aug 16, 2021

robertjchen commented Aug 16, 2021

parasharrajat commented Aug 27, 2021

stitesExpensify 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

stitesExpensify commented Sep 1, 2021

stitesExpensify commented Sep 3, 2021

stitesExpensify commented Sep 3, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

parasharrajat commented Dec 12, 2021

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

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

parasharrajat commented Jul 22, 2021 •

edited

Loading

parasharrajat commented Aug 13, 2021 •

edited

Loading