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

Add wallet balance transfer Page #6878

Merged
merged 33 commits into from
Jan 13, 2022
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
68395a0
Extend wallet balance to take style prop
parasharrajat Dec 22, 2021
d0d742e
add wallet transfer actions
parasharrajat Dec 22, 2021
e89e58e
add prop types
parasharrajat Dec 22, 2021
a616f37
Create wallet transfer page
parasharrajat Dec 22, 2021
7967331
Add option to navigate to transfer balance page from payments page
parasharrajat Dec 22, 2021
b541e5c
fix crash
parasharrajat Dec 22, 2021
1ccd2f7
Add the confirm transfer dialog
parasharrajat Dec 22, 2021
11b8df1
clean up
parasharrajat Dec 22, 2021
dce8300
Pull latest changes from main
parasharrajat Dec 29, 2021
0998c3d
Remove extra functions in Onyx data
parasharrajat Dec 29, 2021
1408f61
Rename lint error
parasharrajat Dec 29, 2021
6105dd5
Props definition correction
parasharrajat Dec 29, 2021
2464a57
refactor transferbalance page
parasharrajat Dec 29, 2021
a36bd7b
Refactor Onyx Keys
parasharrajat Dec 29, 2021
2050722
rename the method
parasharrajat Dec 29, 2021
f2c2765
more cleanup
parasharrajat Dec 29, 2021
3225c7e
Refactor code to match styleguide
parasharrajat Dec 30, 2021
66f3bfb
comments
parasharrajat Dec 30, 2021
00fb74c
Merge branch 'main' of github.com:Expensify/Expensify.cash into trans…
parasharrajat Jan 8, 2022
66ba6bc
correct Balance calculation
parasharrajat Jan 8, 2022
437e0c3
Fix: Transfer balance method check and API call
parasharrajat Jan 8, 2022
a491cf4
Correct transfer amount format
parasharrajat Jan 8, 2022
9d8207c
added Fee consts
parasharrajat Jan 11, 2022
e46e185
Added fee calculations
parasharrajat Jan 11, 2022
0cea1a9
correct Confirmation Modal
parasharrajat Jan 11, 2022
6e3df64
correct fee calculations
parasharrajat Jan 11, 2022
4883344
Merge branch 'main' of github.com:Expensify/Expensify.cash into trans…
parasharrajat Jan 11, 2022
f09a386
fix missing types and imports
parasharrajat Jan 11, 2022
f3f30ef
Fix Instant transfer Rate
parasharrajat Jan 11, 2022
b51397e
correct translations and naming
parasharrajat Jan 12, 2022
3e44bc0
fix: imports and checks
parasharrajat Jan 12, 2022
0782a10
translations
parasharrajat Jan 12, 2022
efeb43c
Refactor and cleanup
parasharrajat Jan 12, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/components/CurrentWalletBalance.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,15 @@ const propTypes = {
currentBalance: PropTypes.number,
}),

/** Styles of the amount */
balanceStyles: PropTypes.arrayOf(PropTypes.object),

...withLocalizePropTypes,
};

const defaultProps = {
userWallet: {},
balanceStyles: [],
};

const CurrentWalletBalance = (props) => {
Expand All @@ -41,7 +45,7 @@ const CurrentWalletBalance = (props) => {
);
return (
<ExpensifyText
style={[styles.textXXXLarge, styles.pv5, styles.alignSelfCenter]}
style={[styles.textXXXLarge, styles.pv5, styles.alignSelfCenter, ...props.balanceStyles]}
>
{`${formattedBalance}`}
</ExpensifyText>
Expand Down
2 changes: 1 addition & 1 deletion src/libs/PaymentUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import * as Localize from './Localize';
* @param {Array} bankAccountList
* @param {Array} cardList
* @param {String} [payPalMeUsername='']
* @returns {Array<PaymentMethod>}
* @returns {Array<Object>}
*/
function getPaymentMethods(bankAccountList, cardList, payPalMeUsername = '') {
const combinedPaymentMethods = [];
Expand Down
63 changes: 63 additions & 0 deletions src/libs/actions/PaymentMethods.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import Growl from '../Growl';
import * as Localize from '../Localize';
import Navigation from '../Navigation/Navigation';
import * as CardUtils from '../CardUtils';
import Log from '../Log';

/**
* Calls the API to get the user's bankAccountList, cardList, wallet, and payPalMe
Expand Down Expand Up @@ -95,8 +96,70 @@ function clearDebitCardFormErrorAndSubmit() {
});
}

/**
* Call the API to transfer wallet balance.
* @param {Object} paymentMethod
* @param {String} paymentMethod.id
* @param {'bankAccount'|'debitCard'} paymentMethod.type
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

* @returns {Promise}
*/
function transferWalletBalance(paymentMethod) {
const parameters = {
[paymentMethod.type === CONST.PAYMENT_METHODS.BANK_ACCOUNT ? 'bankAccountID' : 'fundID']: paymentMethod.id,
};
Onyx.merge(ONYXKEYS.WALLET_TRANSFER, {loading: true});

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);
}).catch((error) => {
Log.alert(`[Payments] Failed to transfer wallet balance: ${error.message}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Growl.error(Localize.translateLocal('transferAmountPage.failedTransfer'));
Onyx.merge(ONYXKEYS.WALLET_TRANSFER, {loading: false});
});
}

/**
* Set the necessary data for wallet transfer
* @param {Number} currentBalance
* @param {Number} selectedAccountID
*/
function saveWalletTransferAmountAndAccount(currentBalance, selectedAccountID) {
Onyx.set(ONYXKEYS.WALLET_TRANSFER, {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove please.

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

function cancelWalletTransfer() {
Onyx.set(ONYXKEYS.WALLET_TRANSFER, null);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

cancelWalletTransfer sounds like either:

  1. We called the API to start a transfer and want to make a new request to cancel it
  2. 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:

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"?

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

export {
getPaymentMethods,
addBillingCard,
clearDebitCardFormErrorAndSubmit,
transferWalletBalance,
saveWalletTransferAmountAndAccount,
updateWalletTransferData,
cancelWalletTransfer,
};
51 changes: 46 additions & 5 deletions src/pages/settings/Payments/PaymentsPage.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React from 'react';
import {ScrollView} from 'react-native';
import {ScrollView, View} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import PropTypes from 'prop-types';
import PaymentMethodList from './PaymentMethodList';
Expand All @@ -19,18 +19,28 @@ import ONYXKEYS from '../../../ONYXKEYS';
import Permissions from '../../../libs/Permissions';
import AddPaymentMethodMenu from '../../../components/AddPaymentMethodMenu';
import CONST from '../../../CONST';
import * as Expensicons from '../../../components/Icon/Expensicons';
import MenuItem from '../../../components/MenuItem';
import * as paymentPropTypes from './paymentPropTypes';
import ConfirmModal from '../../../components/ConfirmModal';

const propTypes = {
...withLocalizePropTypes,
/** Wallet balance transfer props */
walletTransfer: paymentPropTypes.walletTransferPropTypes,

/** List of betas available to current user */
betas: PropTypes.arrayOf(PropTypes.string),

/** Are we loading payment methods? */
isLoadingPaymentMethods: PropTypes.bool,

...withLocalizePropTypes,
};

const defaultProps = {
walletTransfer: {
completed: false,
},
betas: [],
isLoadingPaymentMethods: true,
};
Expand All @@ -48,6 +58,7 @@ class PaymentsPage extends React.Component {
this.paymentMethodPressed = this.paymentMethodPressed.bind(this);
this.addPaymentMethodTypePressed = this.addPaymentMethodTypePressed.bind(this);
this.hideAddPaymentMenu = this.hideAddPaymentMenu.bind(this);
this.navigateToTransferBalancePage = this.navigateToTransferBalancePage.bind(this);
}

componentDidMount() {
Expand Down Expand Up @@ -110,6 +121,13 @@ class PaymentsPage extends React.Component {
this.setState({shouldShowAddPaymentMenu: false});
}

/**
* Navigate to Transfer wallet balance page
*/
navigateToTransferBalancePage() {
Navigation.navigate(ROUTES.SETTINGS_PAYMENTS_TRANSFER_BALANCE);
}

render() {
return (
<ScreenWrapper>
Expand All @@ -121,9 +139,19 @@ class PaymentsPage extends React.Component {
onCloseButtonPress={() => Navigation.dismissModal(true)}
/>
<ScrollView style={styles.flex1}>
{
Permissions.canUseWallet(this.props.betas) && <CurrentWalletBalance />
}
{Permissions.canUseWallet(this.props.betas) && (
<>
<View style={[styles.mv5]}>
<CurrentWalletBalance />
</View>
<MenuItem
title={this.props.translate('common.transferBalance')}
icon={Expensicons.Transfer}
onPress={this.navigateToTransferBalancePage}
shouldShowRightIcon
/>
</>
)}
<ExpensifyText
style={[styles.ph5, styles.formLabel]}
>
Expand All @@ -145,6 +173,19 @@ class PaymentsPage extends React.Component {
}}
onItemSelected={method => this.addPaymentMethodTypePressed(method)}
/>
<ConfirmModal
title={this.props.translate('paymentsPage.allSet')}
onConfirm={PaymentMethods.cancelWalletTransfer}
isVisible={this.props.walletTransfer.completed}
prompt={this.props.translate('paymentsPage.transferConfirmText', {
amount: this.props.numberFormat(
this.props.walletTransfer.transferAmount,
{style: 'currency', currency: 'USD'},
),
})}
confirmText={this.props.translate('paymentsPage.gotIt')}
shouldShowCancelButton={false}
/>
</KeyboardAvoidingView>
</ScreenWrapper>
);
Expand Down
Loading