Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
97c8e99
Allow hiding the Add payment method button on Payment Method list
parasharrajat Jan 13, 2022
8660d64
Added ChooseTransferAccontpage
parasharrajat Jan 13, 2022
05f8a7c
set navigation between choose account and transfer page
parasharrajat Jan 13, 2022
ff85915
add methods
parasharrajat Jan 13, 2022
967e4ba
Merge branch 'main' of github.com:Expensify/Expensify.cash into trans…
parasharrajat Jan 13, 2022
00f4106
Apply filters correctly
parasharrajat Jan 13, 2022
93fdbb6
Reset data on every transfer
parasharrajat Jan 13, 2022
12c62e0
make filtering optional for PaymentMethodList
parasharrajat Jan 13, 2022
e3f1b4f
UI tweaks
parasharrajat Jan 13, 2022
b480379
rename method names
parasharrajat Jan 13, 2022
ec1782c
translations fixed
parasharrajat Jan 13, 2022
cc5e4a5
Refactored names, clean up and comments
parasharrajat Jan 13, 2022
c43bd3a
remove unused imports
parasharrajat Jan 13, 2022
0136230
rename and props correction
parasharrajat Jan 14, 2022
7751d73
select default account
parasharrajat Jan 15, 2022
0bb65ca
Merge branch 'main' of github.com:Expensify/Expensify.cash into trans…
parasharrajat Jan 18, 2022
cd1ddba
Adopt changes from latest changes
parasharrajat Jan 18, 2022
0dcf5f8
fix: PaymentMethodlist filtering
parasharrajat Jan 18, 2022
8eacf33
more refactoring
parasharrajat Jan 18, 2022
39b6327
preselect default account
parasharrajat Jan 18, 2022
41feaa9
Merge branch 'main' of github.com:Expensify/Expensify.cash into trans…
parasharrajat Jan 20, 2022
0043cec
Fix code to get the selected Account
parasharrajat Jan 21, 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
4 changes: 2 additions & 2 deletions src/languages/en.js
Original file line number Diff line number Diff line change
Expand Up @@ -381,8 +381,8 @@ export default {
},
paymentMethodList: {
addPaymentMethod: 'Add payment method',
addDebitCard: 'Add debit card',
addBankAccount: 'Add bank account',
addNewDebitCard: 'Add new debit card',
addNewBankAccount: 'Add new bank account',
accountLastFour: 'Account ending in',
cardLastFour: 'Card ending in',
addFirstPaymentMethod: 'Add a payment method to send and receive payments directly in the app.',
Expand Down
4 changes: 2 additions & 2 deletions src/languages/es.js
Original file line number Diff line number Diff line change
Expand Up @@ -381,8 +381,8 @@ export default {
},
paymentMethodList: {
addPaymentMethod: 'Agrega método de pago',
addDebitCard: 'Agregar tarjeta de débito',
addBankAccount: 'Agregar cuenta de banco',
addNewDebitCard: 'Agregar nueva tarjeta de débito',
addNewBankAccount: 'Agregar nueva cuenta de banco',
accountLastFour: 'Cuenta con terminación',
cardLastFour: 'Tarjeta con terminacíon',
addFirstPaymentMethod: 'Añade un método de pago para enviar y recibir pagos directamente desde la aplicación.',
Expand Down
2 changes: 1 addition & 1 deletion src/libs/PaymentUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ function formatPaymentMethods(bankAccountList, cardList, payPalMeUsername = '',
combinedPaymentMethods.push({
title: card.addressName,
description: formattedCardNumber,
methodID: card.cardNumber,
methodID: card.fundID,
icon,
iconSize,
key: `card-${card.cardNumber}`,
Expand Down
33 changes: 24 additions & 9 deletions src/libs/actions/PaymentMethods.js
Original file line number Diff line number Diff line change
Expand Up @@ -184,10 +184,10 @@ function clearDebitCardFormErrorAndSubmit() {
* Call the API to transfer wallet balance.
* @param {Object} paymentMethod
* @param {*} paymentMethod.methodID
* @param {String} paymentMethod.type
* @param {String} paymentMethod.accountType
*/
function transferWalletBalance(paymentMethod) {
const paymentMethodIDKey = paymentMethod.type === CONST.PAYMENT_METHODS.BANK_ACCOUNT
const paymentMethodIDKey = paymentMethod.accountType === CONST.PAYMENT_METHODS.BANK_ACCOUNT
? CONST.PAYMENT_METHOD_ID_KEYS.BANK_ACCOUNT
: CONST.PAYMENT_METHOD_ID_KEYS.DEBIT_CARD;
const parameters = {
Expand All @@ -209,13 +209,10 @@ function transferWalletBalance(paymentMethod) {
});
}

/**
* Set the transfer account and reset the transfer data for Wallet balance transfer
* @param {String} selectedAccountID
*/
function saveWalletTransferAccountAndResetData(selectedAccountID) {
function resetWalletTransferData() {
Onyx.merge(ONYXKEYS.WALLET_TRANSFER, {
selectedAccountID,
selectedAccountType: '',
selectedAccountID: null,
filterPaymentMethodType: null,
loading: false,
shouldShowConfirmModal: false,
Expand All @@ -229,6 +226,22 @@ function saveWalletTransferAmount(transferAmount) {
Onyx.merge(ONYXKEYS.WALLET_TRANSFER, {transferAmount});
}

/**
* @param {String} selectedAccountType
* @param {String} selectedAccountID
*/
function saveWalletTransferAccountTypeAndID(selectedAccountType, selectedAccountID) {
Onyx.merge(ONYXKEYS.WALLET_TRANSFER, {selectedAccountType, selectedAccountID});
}

/**
* Toggles the user's selected type of payment method (bank account or debit card) on the wallet transfer balance screen.
* @param {String} filterPaymentMethodType
*/
Comment thread
marcaaron marked this conversation as resolved.
function saveWalletTransferMethodType(filterPaymentMethodType) {
Onyx.merge(ONYXKEYS.WALLET_TRANSFER, {filterPaymentMethodType});
}

function dismissWalletConfirmModal() {
Onyx.merge(ONYXKEYS.WALLET_TRANSFER, {shouldShowConfirmModal: false});
}
Expand All @@ -243,7 +256,9 @@ export {
continueSetup,
clearDebitCardFormErrorAndSubmit,
transferWalletBalance,
saveWalletTransferAccountAndResetData,
resetWalletTransferData,
saveWalletTransferAmount,
saveWalletTransferAccountTypeAndID,
saveWalletTransferMethodType,
dismissWalletConfirmModal,
};
99 changes: 86 additions & 13 deletions src/pages/settings/Payments/ChooseTransferAccountPage.js
Original file line number Diff line number Diff line change
@@ -1,27 +1,100 @@
import React from 'react';
import {withOnyx} from 'react-native-onyx';
import {View} from 'react-native';
import HeaderWithCloseButton from '../../../components/HeaderWithCloseButton';
import ScreenWrapper from '../../../components/ScreenWrapper';
import Navigation from '../../../libs/Navigation/Navigation';
import withLocalize, {withLocalizePropTypes} from '../../../components/withLocalize';
import KeyboardAvoidingView from '../../../components/KeyboardAvoidingView';
import CONST from '../../../CONST';
import PaymentMethodList from './PaymentMethodList';
import * as PaymentMethods from '../../../libs/actions/PaymentMethods';
import ROUTES from '../../../ROUTES';
import MenuItem from '../../../components/MenuItem';
import * as Expensicons from '../../../components/Icon/Expensicons';
import compose from '../../../libs/compose';
import ONYXKEYS from '../../../ONYXKEYS';
import walletTransferPropTypes from './walletTransferPropTypes';
import styles from '../../../styles/styles';

const propTypes = {
/** Wallet transfer propTypes */
walletTransfer: walletTransferPropTypes,

...withLocalizePropTypes,
};

const ChooseTransferAccountPage = props => (
<ScreenWrapper>
<KeyboardAvoidingView>
<HeaderWithCloseButton
title={props.translate('chooseTransferAccountPage.chooseAccount')}
shouldShowBackButton
onBackButtonPress={() => Navigation.goBack()}
onCloseButtonPress={() => Navigation.dismissModal()}
/>
</KeyboardAvoidingView>
</ScreenWrapper>
);
const defaultProps = {
walletTransfer: {},
};

const ChooseTransferAccountPage = (props) => {
/**
* Go back to transfer balance screen with the selected bank account set
* @param {Object} event Click event object
* @param {String} accountType of the selected account type
* @param {Object} account of the selected account data
*/
const selectAccountAndNavigateBack = (event, accountType, account) => {
PaymentMethods.saveWalletTransferAccountTypeAndID(
accountType,
accountType === CONST.PAYMENT_METHODS.BANK_ACCOUNT
? account.bankAccountID
: account.fundID,
);
Navigation.navigate(ROUTES.SETTINGS_PAYMENTS_TRANSFER_BALANCE);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

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

};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.


/**
* @param {String} paymentType
*/
const navigateToAddPaymentMethodPage = () => {
if (props.walletTransfer.filterPaymentMethodType === CONST.PAYMENT_METHODS.DEBIT_CARD) {
Navigation.navigate(ROUTES.SETTINGS_ADD_DEBIT_CARD);
return;
}
Navigation.navigate(ROUTES.SETTINGS_ADD_BANK_ACCOUNT);
};

return (
<ScreenWrapper>
<KeyboardAvoidingView>
<HeaderWithCloseButton
title={props.translate('chooseTransferAccountPage.chooseAccount')}
shouldShowBackButton
onBackButtonPress={() => Navigation.goBack()}
onCloseButtonPress={() => Navigation.dismissModal()}
/>
<View style={[styles.mt6, styles.flexShrink1, styles.flexBasisAuto]}>
<PaymentMethodList
onPress={selectAccountAndNavigateBack}
shouldShowSelectedState
filterType={props.walletTransfer.filterPaymentMethodType}
selectedMethodID={props.walletTransfer.selectedAccountID}
shouldShowAddPaymentMethodButton={false}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

/>
</View>
<MenuItem
onPress={navigateToAddPaymentMethodPage}
title={props.walletTransfer.filterPaymentMethodType === CONST.PAYMENT_METHODS.BANK_ACCOUNT
? props.translate('paymentMethodList.addNewBankAccount')
: props.translate('paymentMethodList.addNewDebitCard')}
icon={Expensicons.Plus}
/>
</KeyboardAvoidingView>
</ScreenWrapper>
);
};

ChooseTransferAccountPage.propTypes = propTypes;
ChooseTransferAccountPage.defaultProps = defaultProps;
ChooseTransferAccountPage.displayName = 'ChooseTransferAccountPage';

export default withLocalize(ChooseTransferAccountPage);
export default compose(
withLocalize,
withOnyx({
walletTransfer: {
key: ONYXKEYS.WALLET_TRANSFER,
},
}),
)(ChooseTransferAccountPage);
20 changes: 20 additions & 0 deletions src/pages/settings/Payments/PaymentMethodList.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,13 @@ const propTypes = {
cardID: PropTypes.number,
})),

/** Whether the add Payment button be shown on the list */
shouldShowAddPaymentMethodButton: PropTypes.bool,

/** Type to filter the payment Method list */
filterType: PropTypes.oneOf([CONST.PAYMENT_METHODS.DEBIT_CARD, CONST.PAYMENT_METHODS.BANK_ACCOUNT, '']),

/** User wallet props */
userWallet: PropTypes.shape({
/** The ID of the linked account */
walletLinkedAccountID: PropTypes.number,
Expand All @@ -66,6 +73,8 @@ const defaultProps = {
},
isLoadingPayments: false,
isAddPaymentMenuActive: false,
shouldShowAddPaymentMethodButton: true,
filterType: '',
};

class PaymentMethodList extends Component {
Expand All @@ -82,6 +91,11 @@ class PaymentMethodList extends Component {
*/
createPaymentMethodList() {
let combinedPaymentMethods = PaymentUtils.formatPaymentMethods(this.props.bankAccountList, this.props.cardList, this.props.payPalMeUsername, this.props.userWallet);

if (!_.isEmpty(this.props.filterType)) {
combinedPaymentMethods = _.filter(combinedPaymentMethods, paymentMethod => paymentMethod.accountType === this.props.filterType);
}

combinedPaymentMethods = _.map(combinedPaymentMethods, paymentMethod => ({
...paymentMethod,
type: MENU_ITEM,
Expand All @@ -96,6 +110,10 @@ class PaymentMethodList extends Component {
});
}

if (!this.props.shouldShowAddPaymentMethodButton) {
return combinedPaymentMethods;
}

combinedPaymentMethods.push({
type: MENU_ITEM,
title: this.props.translate('paymentMethodList.addPaymentMethod'),
Expand Down Expand Up @@ -132,6 +150,8 @@ class PaymentMethodList extends Component {
iconWidth={item.iconSize}
badgeText={item.isDefault ? this.props.translate('paymentMethodList.defaultPaymentMethod') : null}
wrapperStyle={item.wrapperStyle}
shouldShowSelectedState={this.props.shouldShowSelectedState}
isSelected={this.props.selectedMethodID === item.methodID}
/>
);
}
Expand Down
2 changes: 1 addition & 1 deletion src/pages/settings/Payments/PaymentsPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ class PaymentsPage extends React.Component {
</>
)}
<Text
style={[styles.ph5, styles.formLabel]}
style={[styles.ph5, styles.mt6, styles.formLabel]}
>
{this.props.translate('paymentsPage.paymentMethodsTitle')}
</Text>
Expand Down
51 changes: 40 additions & 11 deletions src/pages/settings/Payments/TransferBalancePage.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import React from 'react';
import {View, ScrollView} from 'react-native';
import PropTypes from 'prop-types';
import {withOnyx} from 'react-native-onyx';
import lodashGet from 'lodash/get';
import ONYXKEYS from '../../../ONYXKEYS';
import HeaderWithCloseButton from '../../../components/HeaderWithCloseButton';
import ScreenWrapper from '../../../components/ScreenWrapper';
Expand All @@ -24,6 +23,7 @@ import walletTransferPropTypes from './walletTransferPropTypes';
import * as PaymentMethods from '../../../libs/actions/PaymentMethods';
import * as PaymentUtils from '../../../libs/PaymentUtils';
import userWalletPropTypes from '../../EnablePayments/userWalletPropTypes';
import ROUTES from '../../../ROUTES';

const propTypes = {
/** User's wallet information */
Expand Down Expand Up @@ -96,35 +96,62 @@ class TransferBalancePage extends React.Component {
},
];

this.saveTransferAmountAndBalance = this.saveTransferAmountAndBalance.bind(this);
this.getSelectedPaymentMethodAccount = this.getSelectedPaymentMethodAccount.bind(this);

PaymentMethods.resetWalletTransferData();
const selectedAccount = this.getSelectedPaymentMethodAccount();
PaymentMethods.saveWalletTransferAccountAndResetData(selectedAccount ? selectedAccount.id : '');

// Select the default payment account when page is opened,
// so that user can see that preselected on choose transfer account page
if (!selectedAccount || !selectedAccount.isDefault) {
return;
}

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

}

/**
* Get the selected/default payment method account for wallet transfer
* @returns {Object|undefined}
*/
getSelectedPaymentMethodAccount() {
const paymentMethods = PaymentUtils.formatPaymentMethods(this.props.bankAccountList, this.props.cardList, '', this.props.userWallet);
const accountID = this.props.walletTransfer.selectedAccountID || lodashGet(this.props, 'userWallet.walletLinkedAccountID', '');
return _.find(paymentMethods, method => method.methodID === accountID);
const paymentMethods = PaymentUtils.formatPaymentMethods(
this.props.bankAccountList,
this.props.cardList,
'',
this.props.userWallet,
);

const defaultAccount = _.find(paymentMethods, method => method.isDefault);
const selectedAccount = _.find(
paymentMethods,
method => method.accountType === this.props.walletTransfer.selectedAccountType
&& method.methodID === this.props.walletTransfer.selectedAccountID,
);
return selectedAccount || defaultAccount;
}

/**
* @param {Number} transferAmount
* @param {Object} selectedAccount
*/
saveTransferAmountAndBalance(transferAmount, selectedAccount) {
saveTransferAmountAndStartTransfer(transferAmount, selectedAccount) {
PaymentMethods.saveWalletTransferAmount(transferAmount);
PaymentMethods.transferWalletBalance(selectedAccount);
Comment thread
marcaaron marked this conversation as resolved.
}

/**
* @param {String} filterPaymentMethodType
*/
navigateToChooseTransferAccount(filterPaymentMethodType) {
PaymentMethods.saveWalletTransferMethodType(filterPaymentMethodType);
Navigation.navigate(ROUTES.SETTINGS_PAYMENTS_CHOOSE_TRANSFER_ACCOUNT);
}

render() {
const selectedAccount = this.getSelectedPaymentMethodAccount();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

const selectedPaymentType = selectedAccount && selectedAccount.type === CONST.PAYMENT_METHODS.BANK_ACCOUNT
const selectedPaymentType = selectedAccount && selectedAccount.accountType === CONST.PAYMENT_METHODS.BANK_ACCOUNT
? CONST.WALLET.TRANSFER_METHOD_TYPE.ACH
: CONST.WALLET.TRANSFER_METHOD_TYPE.INSTANT;

Expand Down Expand Up @@ -162,6 +189,7 @@ class TransferBalancePage extends React.Component {
...(selectedPaymentType === paymentType.key
&& styles.transferBalanceSelectedPayment),
}}
onPress={() => this.navigateToChooseTransferAccount(paymentType.type)}
/>
))}
<Text
Expand All @@ -182,6 +210,7 @@ class TransferBalancePage extends React.Component {
...styles.mrn5,
...styles.ph0,
}}
onPress={() => this.navigateToChooseTransferAccount(selectedAccount.accountType)}
/>
)}
<Text
Expand Down Expand Up @@ -210,7 +239,7 @@ class TransferBalancePage extends React.Component {
pressOnEnter
isLoading={this.props.walletTransfer.loading}
isDisabled={isButtonDisabled}
onPress={() => this.saveTransferAmountAndBalance(transferAmount, selectedAccount)}
onPress={() => this.saveTransferAmountAndStartTransfer(transferAmount, selectedAccount)}
text={this.props.translate(
'transferAmountPage.transfer',
{
Expand Down