Add wallet balance transfer Page by parasharrajat · Pull Request #6878 · Expensify/App

parasharrajat · 2021-12-22T16:41:23Z

Please review @marcaaron

Details

Fixed Issues

$ #3922

Tests | QA Steps

Open Payments page.
Click on the wallet transfer.
You should be taken to the Transfer balance page.
If your wallet balance is greater than the fee and which account? has an account then you should be able to click the transfer button and it should be enabled.
Observe if Which account? is a debit card then the Instant method is selected.
Observe if Which account? is a bank account then the 1-3 business days method is selected.
Paypal should not be shown as Which account?

Tested On

Screenshots

Web | Mobile Web | Desktop

iOS

Android

parasharrajat · 2021-12-22T16:42:22Z

+function saveWalletTransferAmountAndAccount(currentBalance, selectedAccountID) {
+    Onyx.set(ONYXKEYS.WALLET_TRANSFER, {
+        transferAmount: currentBalance - CONST.WALLET.TRANSFER_BALANCE_FEE,
+        selectedAccountID,
+        filterPaymentMethodType: null,
+        loading: false,
+        completed: false,
+    });
+}
+
+/**
+ * Update selected accountID and other data for wallet transfer
+ * @param {Object} data
+ */
+function updateWalletTransferData(data) {
+    Onyx.merge(ONYXKEYS.WALLET_TRANSFER, data);
+}
+
+/**
+ * Cancel the wallet transfer
+ */
+function cancelWalletTransfer() {
+    Onyx.set(ONYXKEYS.WALLET_TRANSFER, null);
+}
+


I am not sure what would be the best way to handle these but I need these to share data across screens.

There's perhaps no other way to share this data. But the logic can be improved.

cancelWalletTransfer sounds like either:

We called the API to start a transfer and want to make a new request to cancel it

We are in the middle of a flow and have not yet called the API to start the transfer and want to cancel it

But then if we look at where this method is called we see it happens when we "confirm" a modal that says {amount} will hit your account shortly!.

If we are just sharing data about a pending transfer then maybe it's best to call this resetWalletTransferData().

But looking into it more - it seems like the primary purpose is to dismiss the confirm modal. So, why not do this instead...

function dismissWalletConfirmModal() { Onyx.merge(ONYXKEYS.WALLET_TRANSFER, {shouldShowConfirmModal: false}); }

If we need to "clear" the rest of the data like balance, accountID, etc why are we doing that when the modal button is pressed? Shouldn't this always happen when we have a successful transfer here:

App/src/libs/actions/PaymentMethods.js

Lines 112 to 119 in 11b8df1

return API.TransferWalletBalance(parameters)

.then((response) => {

if (response.jsonCode !== 200) {

throw new Error(response.message);

}

Onyx.merge(ONYXKEYS.USER_WALLET, {balance: 0});

Onyx.merge(ONYXKEYS.WALLET_TRANSFER, {completed: true, loading: false});

Navigation.navigate(ROUTES.SETTINGS_PAYMENTS);

And not just when a user "confirms"?

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

Lines 176 to 180 in 11b8df1

<ConfirmModal

title={this.props.translate('paymentsPage.allSet')}

onConfirm={PaymentMethods.cancelWalletTransfer}

isVisible={this.props.walletTransfer.completed}

prompt={this.props.translate('paymentsPage.transferConfirmText', {

It was just the shorter way to do two things at once. reset the data and hide the confirm modal.

I understand that instinct, but my feedback would be that the shorter way is not always the easiest to understand. It's usually better to write code that leaves no doubt about what is happening vs. code where we must inspect the details of every method to figure out what it does.

parasharrajat · 2021-12-22T16:43:15Z

+    getSelectedAccount() {
+        const paymentMethods = PaymentUtils.getPaymentMethods(
+            this.props.bankAccountList,
+            this.props.cardList,
+        );
+
+        const defaultAccount = _.find(
+            paymentMethods,
+            method => method.id === lodashGet(this.props, 'userWallet.walletLinkedAccountID', ''),
+        );
+        const selectedAccount = this.props.walletTransfer.selectedAccountID
+            ? _.find(
+                paymentMethods,
+                method => method.id === this.props.walletTransfer.selectedAccountID,
+            )
+            : defaultAccount;
+
+        return selectedAccount;
+    }


I don't see any selected account by default. userWallet.walletLinkedAccountID does not have any value.

I think we have to call the SetWalletLinkedAccount command to set one. @stitesExpensify would maybe know for sure.

Yeah we have to set the account manually for now. We should start defaulting them in the future though..

parasharrajat · 2021-12-22T16:44:12Z

Note: I can't test the wallet transfer, I would need your help to do that. Thanks.

marcaaron · 2021-12-22T17:59:28Z

+function saveWalletTransferAmountAndAccount(currentBalance, selectedAccountID) {
+    Onyx.set(ONYXKEYS.WALLET_TRANSFER, {
+        transferAmount: currentBalance - CONST.WALLET.TRANSFER_BALANCE_FEE,
+        selectedAccountID,
+        filterPaymentMethodType: null,
+        loading: false,
+        completed: false,
+    });
+}
+
+/**
+ * Update selected accountID and other data for wallet transfer
+ * @param {Object} data
+ */
+function updateWalletTransferData(data) {
+    Onyx.merge(ONYXKEYS.WALLET_TRANSFER, data);
+}
+
+/**
+ * Cancel the wallet transfer
+ */
+function cancelWalletTransfer() {
+    Onyx.set(ONYXKEYS.WALLET_TRANSFER, null);
+}
+


There's perhaps no other way to share this data. But the logic can be improved.

cancelWalletTransfer sounds like either:

We called the API to start a transfer and want to make a new request to cancel it

We are in the middle of a flow and have not yet called the API to start the transfer and want to cancel it

But then if we look at where this method is called we see it happens when we "confirm" a modal that says {amount} will hit your account shortly!.

If we are just sharing data about a pending transfer then maybe it's best to call this resetWalletTransferData().

But looking into it more - it seems like the primary purpose is to dismiss the confirm modal. So, why not do this instead...

function dismissWalletConfirmModal() { Onyx.merge(ONYXKEYS.WALLET_TRANSFER, {shouldShowConfirmModal: false}); }

If we need to "clear" the rest of the data like balance, accountID, etc why are we doing that when the modal button is pressed? Shouldn't this always happen when we have a successful transfer here:

App/src/libs/actions/PaymentMethods.js

Lines 112 to 119 in 11b8df1

return API.TransferWalletBalance(parameters)

.then((response) => {

if (response.jsonCode !== 200) {

throw new Error(response.message);

}

Onyx.merge(ONYXKEYS.USER_WALLET, {balance: 0});

Onyx.merge(ONYXKEYS.WALLET_TRANSFER, {completed: true, loading: false});

Navigation.navigate(ROUTES.SETTINGS_PAYMENTS);

And not just when a user "confirms"?

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

Lines 176 to 180 in 11b8df1

<ConfirmModal

title={this.props.translate('paymentsPage.allSet')}

onConfirm={PaymentMethods.cancelWalletTransfer}

isVisible={this.props.walletTransfer.completed}

prompt={this.props.translate('paymentsPage.transferConfirmText', {

marcaaron · 2021-12-22T18:02:11Z

+        PaymentMethods.saveWalletTransferAmountAndAccount(
+            this.props.userWallet.currentBalance,
+            selectedAccount ? selectedAccount.id : '',
+        );


Does it make sense to do this in the constructor()? If the currentBalance changes the user will transfer whatever we first had when the page opened and not the new balance.

Actually, the balance is transferred via the backend. API just takes the account where the user wants to transfer. But there is no other way on the app where wallet values will update automatically. The wallet is only refreshed when we open some pages and the First-time app loads.

But I agree I can subscribe to wallet wherever I need the transfer amount.

I wanted to remove the temporary data that is added for wallet transfer. we can't clear the data(set that to null) before Confirm modal is shown as the same key ONYXKEYS.WALLET_TRANSFER.shouldShowConfirmModal would be used to show the modal.

After the modal is shown and user close it. We can remove all the temporary data from Onyx by setting it to null. And by setting it to null, we close the modal as well.

@marcaaron

Sorry, I don't understand the explanation. Can you try asking this in the form of a question or point to some examples?

So we use an Onyx key to store the temporary data used for wallet transfer. And the same key is used to determine the isVisible state of ConfirmModal.

After the transfer is done, we don't need the temporary data and thus we have to set the Onyx key to null to clear it.

Confirm Modal is shown to user after API call is made and we need shouldShowConfirmModal to open this.

If we set the Onyx data to null after API call we won't be able to determine the state for ConfirmModal.

we need a trigger point to clear the Onyx data after wallet transfer i.e. a place where we can set the Key to null. I think the best place would be after user has confirmed the Transfer.

Sorry, not sure if I totally understand still. But can we solve this problem by using a new key for the modal visibility flag to separate it from the wallet data? That way we can reset or clear the data however we want and have separate controls to show/hide the modal?

we have to set the Onyx key to null to clear it

Unsure why we need to set this null - what happens if we don't do this?

Nothing. It will just remove those keys from storage but I think storage is never a problem for us. So I am correcting it now.

parasharrajat · 2021-12-28T17:48:23Z

Updating the PR asap

marcaaron · 2021-12-29T18:43:01Z

+ * @param {String} selectedAccountID
+ */
+function saveWalletTransferAccount(selectedAccountID) {
+    Onyx.set(ONYXKEYS.WALLET_TRANSFER, {


Is it possible to use a merge() here instead of a set()?

Yeah. It does not hurt. Sorry, forgot the conflict issues between set and merge.

marcaaron · 2021-12-29T18:43:52Z

+}
+
+/**
+ * Remove all the data related to wallet transfer and close the ConfirmModal


Also think we should not be doing these two things at once. Closing a modal is a very explicit action. Resetting the wallet transfer data is another specific action. Is it easy to tell that calling clearWalletTransfer() has a side effect where a modal closes?

I agree. Now, I am not worried about the storage so I am refactoring it.

marcaaron · 2021-12-29T18:52:50Z

+function saveWalletTransferAmountAndAccount(currentBalance, selectedAccountID) {
+    Onyx.set(ONYXKEYS.WALLET_TRANSFER, {
+        transferAmount: currentBalance - CONST.WALLET.TRANSFER_BALANCE_FEE,
+        selectedAccountID,
+        filterPaymentMethodType: null,
+        loading: false,
+        completed: false,
+    });
+}
+
+/**
+ * Update selected accountID and other data for wallet transfer
+ * @param {Object} data
+ */
+function updateWalletTransferData(data) {
+    Onyx.merge(ONYXKEYS.WALLET_TRANSFER, data);
+}
+
+/**
+ * Cancel the wallet transfer
+ */
+function cancelWalletTransfer() {
+    Onyx.set(ONYXKEYS.WALLET_TRANSFER, null);
+}
+


I understand that instinct, but my feedback would be that the shorter way is not always the easiest to understand. It's usually better to write code that leaves no doubt about what is happening vs. code where we must inspect the details of every method to figure out what it does.

…-amount-scaffold

parasharrajat · 2022-01-11T14:32:37Z

+function saveWalletTransferTransferredAmount(transferAmount) {
+    Onyx.merge(ONYXKEYS.WALLET_TRANSFER, {transferAmount});
+}


I had to store this transferAmount as after user click transfer amount, balance in the userWallet will be set to 0 and then in the ConfirmModal we will not have any way to show the transferred amount.

I think saveWalletTransferAmount is fine descriptiveness-wise, and not as long 😄

parasharrajat · 2022-01-11T14:41:44Z

PR Updated.

I have not added logic to set the Default Account for two reasons:

I don't know where to set it.
I don't know How to set it 😉

parasharrajat · 2022-01-11T20:53:12Z

PR updated @marcaaron @stitesExpensify

stitesExpensify

Looking good, we're getting close!

stitesExpensify · 2022-01-11T23:16:12Z

+ * Check to see if user has either a debit card or personal bank account added
+ *
+ * @param {Array} [cardList]
+ * @param {Array} [bankAccountList]


Why are we only checking for default credit here? Isn't it any bank account that is verified / open?

cc: @marcaaron

I might not understand what "default credit" means. Is it not a personal / deposit account?

I thought VBA is not a valid payment method, but maybe wrong about that too. I asked this once here.

stitesExpensify · 2022-01-11T23:19:03Z

+function saveWalletTransferTransferredAmount(transferAmount) {
+    Onyx.merge(ONYXKEYS.WALLET_TRANSFER, {transferAmount});
+}


I think saveWalletTransferAmount is fine descriptiveness-wise, and not as long 😄

stitesExpensify · 2022-01-11T23:27:05Z

-        const canTransfer = transferAmount > CONST.WALLET.TRANSFER_BALANCE_FEE;
+        const calculatedFee = PaymentUtils.calculateWalletTransferBalanceFee(this.props.userWallet.currentBalance, selectedPaymentType);
+        const transferAmount = this.props.userWallet.currentBalance - calculatedFee;
+        const canTransfer = transferAmount > calculatedFee;


Shouldn't this line either be this.props.userWallet.currentBalance > calculatedFee or transferAmount > 0?

As it is, you couldn't transfer $0.50 because 50-25 = 25 and 25 !> 25

Yeah, it is supposed to be transferAmount > 0. Thanks for checking in.

parasharrajat · 2022-01-12T17:00:39Z

Ready @marcaaron @stitesExpensify

marcaaron · 2022-01-12T19:00:26Z

+ * Call the API to transfer wallet balance.
+ * @param {Object} paymentMethod
+ * @param {String|Number} paymentMethod.methodID
+ * @param {'bankAccount'|'debitCard'} paymentMethod.type


NAB, but these docs are inconsistent with how we do things. If there are multiple types we use * and the last one should be String. I will try to propose a style guide change so it doesn't need to be mentioned.

marcaaron · 2022-01-12T19:02:02Z

+ */
+function transferWalletBalance(paymentMethod) {
+    const parameters = {
+        [paymentMethod.type === CONST.PAYMENT_METHODS.BANK_ACCOUNT ? 'bankAccountID' : 'fundID']: paymentMethod.methodID,


Please use the constants for 'bankAccountID' and 'fundID' or add them if they don't exist.

marcaaron · 2022-01-12T19:04:48Z

+            Onyx.merge(ONYXKEYS.WALLET_TRANSFER, {shouldShowConfirmModal: true, loading: false});
+            Navigation.navigate(ROUTES.SETTINGS_PAYMENTS);
+        }).catch((error) => {
+            Log.alert(`[Payments] Failed to transfer wallet balance: ${error.message}`);


Not sure about this alert and think we should remove it for now. We can see in the server logs why a request returned a jsonCode other than 200. @stitesExpensify do you agree?

marcaaron · 2022-01-12T19:06:13Z

+
+/**
+ * Close the ConfirmModal of Wallet balance transfer
+ */


Remove please.

Future reference: avoid docs that don't add any additional information - https://github.com/Expensify/App/blob/main/STYLE.md#jsdocs

marcaaron · 2022-01-12T19:08:53Z


+    mrn3: {
+        marginRight: -12,
+    },


Remove it please.

parasharrajat · 2022-01-12T20:53:42Z

All comments addressed. Ready for final review.

marcaaron

Thanks great job!

stitesExpensify

Almost there!

parasharrajat · 2022-01-13T01:00:55Z

@stitesExpensify Awaiting final review from you and merge. Hope it answers your question #6878 (comment)

stitesExpensify

Looks great!

OSBotify · 2022-01-13T01:10:25Z

✋ 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-14T20:01:02Z

🚀 Deployed to staging by @stitesExpensify in version: 1.1.29-6 🚀

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

OSBotify · 2022-01-18T22:03:34Z

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

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

parasharrajat added 7 commits December 22, 2021 21:42

Extend wallet balance to take style prop

68395a0

add wallet transfer actions

d0d742e

add prop types

e89e58e

Create wallet transfer page

a616f37

Add option to navigate to transfer balance page from payments page

7967331

fix crash

b541e5c

Add the confirm transfer dialog

1ccd2f7

parasharrajat requested a review from a team as a code owner December 22, 2021 16:41

MelvinBot requested review from stitesExpensify and removed request for a team December 22, 2021 16:41

parasharrajat commented Dec 22, 2021

View reviewed changes

parasharrajat mentioned this pull request Dec 22, 2021

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

Closed

5 tasks

clean up

11b8df1

marcaaron requested changes Dec 22, 2021

View reviewed changes

parasharrajat added 7 commits December 29, 2021 21:45

Pull latest changes from main

dce8300

Remove extra functions in Onyx data

0998c3d

Rename lint error

1408f61

Props definition correction

6105dd5

refactor transferbalance page

2464a57

Refactor Onyx Keys

a36bd7b

rename the method

2050722

parasharrajat requested a review from marcaaron December 29, 2021 17:10

more cleanup

f2c2765

marcaaron assigned parasharrajat Dec 29, 2021

marcaaron reviewed Dec 29, 2021

View reviewed changes

parasharrajat added 2 commits December 30, 2021 18:21

Refactor code to match styleguide

3225c7e

comments

66f3bfb

parasharrajat added 3 commits January 11, 2022 19:44

correct Confirmation Modal

0cea1a9

correct fee calculations

6e3df64

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

4883344

…-amount-scaffold

parasharrajat commented Jan 11, 2022

View reviewed changes

parasharrajat added 2 commits January 11, 2022 20:07

fix missing types and imports

f09a386

Fix Instant transfer Rate

f3f30ef

parasharrajat requested review from marcaaron and stitesExpensify January 11, 2022 14:41

stitesExpensify reviewed Jan 11, 2022

View reviewed changes

marcaaron mentioned this pull request Jan 12, 2022

Add default/delete popover and supporting methods #4437

Merged

5 tasks

parasharrajat added 2 commits January 12, 2022 22:25

correct translations and naming

b51397e

fix: imports and checks

3e44bc0

translations

0782a10

marcaaron requested changes Jan 12, 2022

View reviewed changes

Refactor and cleanup

efeb43c

parasharrajat requested a review from marcaaron January 12, 2022 20:23

marcaaron approved these changes Jan 12, 2022

View reviewed changes

stitesExpensify reviewed Jan 12, 2022

View reviewed changes

Comment thread src/pages/settings/Payments/TransferBalancePage.js

stitesExpensify approved these changes Jan 13, 2022

View reviewed changes

stitesExpensify merged commit 01757b2 into Expensify:main Jan 13, 2022

OSBotify mentioned this pull request Jan 14, 2022

Deploy Checklist: New Expensify 2022-01-14 #7235

Closed

66 tasks

bondydaa mentioned this pull request Feb 8, 2022

[HOLD for payment 2022-03-02] After you transfer your balance the wallet page doesn't automatically update #7626

Closed

	return API.TransferWalletBalance(parameters)
	.then((response) => {
	if (response.jsonCode !== 200) {
	throw new Error(response.message);
	}
	Onyx.merge(ONYXKEYS.USER_WALLET, {balance: 0});
	Onyx.merge(ONYXKEYS.WALLET_TRANSFER, {completed: true, loading: false});
	Navigation.navigate(ROUTES.SETTINGS_PAYMENTS);

	<ConfirmModal
	title={this.props.translate('paymentsPage.allSet')}
	onConfirm={PaymentMethods.cancelWalletTransfer}
	isVisible={this.props.walletTransfer.completed}
	prompt={this.props.translate('paymentsPage.transferConfirmText', {

Conversation

parasharrajat commented Dec 22, 2021

Details

Fixed Issues

Tests | QA Steps

Tested On

Screenshots

Web | Mobile Web | Desktop

iOS

Android

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parasharrajat commented Dec 22, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parasharrajat Dec 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parasharrajat Dec 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

parasharrajat commented Dec 28, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parasharrajat commented Jan 11, 2022

parasharrajat Dec 22, 2021 •

edited

Loading

parasharrajat Dec 29, 2021 •

edited

Loading

parasharrajat commented Jan 11, 2022 •

edited

Loading