Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
65 commits
Select commit Hold shift + click to select a range
2d770ad
Add paymentType and createAndPayIOU to IOUModal
mountiny Jul 30, 2021
2b3c4f7
Add an optional comment parameter to payIOUReport function
mountiny Aug 2, 2021
ffbbddf
Add polyfil for crypto.getRandomValues
mountiny Aug 2, 2021
ff11fb3
Refactor the confirm function
mountiny Aug 2, 2021
84a8d0c
Bind the refactored function
mountiny Aug 2, 2021
61c76ef
Add IOU_TYPE global contant
mountiny Aug 3, 2021
4bd4f9a
Change the IOU type string to use global constants
mountiny Aug 3, 2021
8b17843
Add requestorEmail as optional parametr to payIOUReport and update th…
mountiny Aug 5, 2021
8dda6b4
Add requestor email to the payIOUReport function call
mountiny Aug 5, 2021
cc50eda
Add sendMoney beta and refactor the iou_type const
mountiny Aug 6, 2021
7a3d074
Merge main and resolve conflicts
mountiny Sep 16, 2021
a20bdef
Resolve conflicts I have missed
mountiny Sep 16, 2021
409b848
Add the phone number and paypalme address to the filtered data
mountiny Sep 16, 2021
bef057a
Include paypalme address and phoneNumber in the newIOUReportDetails
mountiny Sep 16, 2021
26f87d6
Fix lint errors
mountiny Sep 16, 2021
9126504
Add the phonenumber and paypal address to the personalDetails
mountiny Sep 23, 2021
b4a2c30
Update proptypes with phoneNumber and paypalMeAddress
mountiny Sep 23, 2021
307440e
Add the phoneNumber and payPalMeAddress to options factory
mountiny Sep 23, 2021
4457af8
Clean up the code
mountiny Sep 23, 2021
341901e
Use different object key
mountiny Sep 23, 2021
eda967b
Update props name
mountiny Sep 23, 2021
61b0a14
Merge main and resolve conflicts
mountiny Oct 5, 2021
5dee09f
Remove double line
mountiny Oct 5, 2021
9669dd5
Remove duplicate keys
mountiny Oct 5, 2021
eadda9b
Create IOU utils and include idempotency key generater helper function
mountiny Oct 5, 2021
421fd84
Remove blank line
mountiny Oct 5, 2021
bab792b
Update the import
mountiny Oct 5, 2021
c55e984
Fix js styles
mountiny Oct 6, 2021
6bbe303
Merge branch 'main' into vit-createAndPayIOU
mountiny Oct 7, 2021
761d4ad
Change to paymentType from iouType
mountiny Oct 7, 2021
f9728e3
Add payment type to the dropdown menu button
mountiny Oct 8, 2021
75e221d
Use paymentType instead of type
mountiny Oct 8, 2021
bbe044c
Make paymentType required
mountiny Oct 8, 2021
8c710a3
Update comments
mountiny Oct 8, 2021
0642a21
Use default payment type as Elsewhere
mountiny Oct 8, 2021
2a21ec4
Make the payment type not required as we dont need PaymentType for sp…
mountiny Oct 8, 2021
383cb6b
Update comment for payment options
mountiny Oct 8, 2021
a91d03e
Polish comments
mountiny Oct 8, 2021
e18a4bf
JSON stringify the newIOUReportDetails data
mountiny Oct 11, 2021
58cc279
Pass the entire payment option and set payment type later
mountiny Oct 11, 2021
77f5ca2
Move the payment options up to IOUModal
mountiny Oct 12, 2021
634d46a
Fix styles
mountiny Oct 12, 2021
f1da312
Move around the functions to satisfy the linter
mountiny Oct 12, 2021
bbbc077
Add the updateComment function which was removed in last commit y acc…
mountiny Oct 12, 2021
6cda8c1
Add in required reportID for paying from IOUDetails Modal
Oct 13, 2021
d3e64d6
Merge main
Oct 27, 2021
1517e50
Merge main with conflicts
Oct 28, 2021
86e8b4f
Merge and fix conflicts in IOUConfirmation and IOUModal
Nov 29, 2021
ad644f3
Merge with conflict
Dec 9, 2021
9c70be0
Merge main changes in IOUModal
Dec 10, 2021
fc9125e
Updated from Settlement Button refactor
Dec 13, 2021
58da027
Replace cryptovalues with Str.guid
Dec 14, 2021
84ed8cc
Clean up + navigate back to report
Dec 14, 2021
92bbf3a
Final cleanup + simplify logic
Dec 14, 2021
5ee20f7
Style
Dec 14, 2021
2054d30
Review comments
Dec 15, 2021
7cfa7ab
Lint
Dec 15, 2021
f43b988
Use new IOU.sendMoney
Dec 21, 2021
821c90c
Cleanup
Dec 21, 2021
3cc4fa6
Revert stashed change
Dec 21, 2021
40b1e55
Fix indentation
Dec 21, 2021
2db4946
Have sendMoney call payIOUReport
Dec 25, 2021
c4ca01e
Fix typo
Dec 25, 2021
73937b2
Update currency
Dec 25, 2021
ca34f19
Ugh, fix another comment typo
Dec 25, 2021
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
7 changes: 5 additions & 2 deletions src/components/IOUConfirmationList.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ const propTypes = {
/** Callback to inform parent modal of success */
onConfirm: PropTypes.func.isRequired,

/** Callback to to parent modal to send money */
onSendMoney: PropTypes.func.isRequired,

// Callback to update comment from IOUModal
onUpdateComment: PropTypes.func,

Expand Down Expand Up @@ -149,7 +152,7 @@ class IOUConfirmationList extends Component {
onPress(value) {
if (this.props.iouType === CONST.IOU.IOU_TYPE.SEND) {
Log.info(`[IOU] Sending money via: ${value}`);
this.props.onConfirm();
this.props.onSendMoney(value);
} else {
Log.info(`[IOU] Requesting money via: ${value}`);
this.props.onConfirm(this.getSplits());
Expand Down Expand Up @@ -387,7 +390,7 @@ class IOUConfirmationList extends Component {
onPress={this.onPress}
shouldShowPaypal={Boolean(recipient.payPalMeAddress)}
recipientPhoneNumber={recipient.phoneNumber}
currency={this.props.localCurrencyCode}
currency={this.props.iou.selectedCurrencyCode}
/>
) : (
<ButtonWithMenu
Expand Down
2 changes: 2 additions & 0 deletions src/libs/API.js
Original file line number Diff line number Diff line change
Expand Up @@ -530,6 +530,7 @@ function Graphite_Timer(parameters) {
* @param {Object} parameters
* @param {Number} parameters.reportID
* @param {String} parameters.paymentMethodType
* @param {Object} [parameters.newIOUReportDetails]
* @returns {Promise}
*/
function PayIOU(parameters) {
Expand All @@ -541,6 +542,7 @@ function PayIOU(parameters) {
/**
* @param {Object} parameters
* @param {Number} parameters.reportID
* @param {Object} [parameters.newIOUReportDetails]
* @returns {Promise}
*/
function PayWithWallet(parameters) {
Expand Down
33 changes: 25 additions & 8 deletions src/libs/actions/IOU.js
Original file line number Diff line number Diff line change
Expand Up @@ -246,25 +246,36 @@ function buildPayPalPaymentUrl(amount, submitterPayPalMeAddress, currency) {
* @param {String} params.paymentMethodType - one of CONST.IOU.PAYMENT_TYPE
* @param {Number} params.amount
* @param {String} params.currency
* @param {String} [params.submitterPhoneNumber] - used for Venmo
* @param {String} [params.submitterPayPalMeAddress]
* @param {String} [params.requestorPhoneNumber] - used for Venmo
* @param {String} [params.requestorPayPalMeAddress]
* @param {String} [params.newIOUReportDetails] - Extra details required only for send money flow
* @param {Boolean} [params.shouldRedirectToChatReport]
*/
function payIOUReport({
chatReportID, reportID, paymentMethodType, amount, currency, submitterPhoneNumber, submitterPayPalMeAddress,
chatReportID,
reportID,
paymentMethodType,
amount,
currency,
requestorPhoneNumber,
requestorPayPalMeAddress,
newIOUReportDetails,
shouldRedirectToChatReport = false,
}) {
Onyx.merge(ONYXKEYS.IOU, {loading: true, error: false});

const payIOUPromise = paymentMethodType === CONST.IOU.PAYMENT_TYPE.EXPENSIFY
? API.PayWithWallet({reportID})
: API.PayIOU({reportID, paymentMethodType});
? API.PayWithWallet({reportID, newIOUReportDetails})
: API.PayIOU({reportID, paymentMethodType, newIOUReportDetails});

// Build the url for the user's platform of choice if they have
// selected something other than a manual settlement or Expensify Wallet e.g. Venmo or PayPal.me
let url;
if (paymentMethodType === CONST.IOU.PAYMENT_TYPE.PAYPAL_ME) {
url = buildPayPalPaymentUrl(amount, submitterPayPalMeAddress, currency);
url = buildPayPalPaymentUrl(amount, requestorPayPalMeAddress, currency);
}
if (paymentMethodType === CONST.IOU.PAYMENT_TYPE.VENMO) {
url = buildVenmoPaymentURL(amount, submitterPhoneNumber);
url = buildVenmoPaymentURL(amount, requestorPhoneNumber);
}

asyncOpenURL(payIOUPromise
Expand All @@ -291,7 +302,13 @@ function payIOUReport({
}
Onyx.merge(ONYXKEYS.IOU, {error: true});
})
.finally(() => Onyx.merge(ONYXKEYS.IOU, {loading: false})),
.finally(() => {
Onyx.merge(ONYXKEYS.IOU, {loading: false});

if (shouldRedirectToChatReport) {
Navigation.navigate(ROUTES.REPORT);
Comment thread
nickmurray47 marked this conversation as resolved.
}
}),
url);
}

Expand Down
77 changes: 60 additions & 17 deletions src/pages/iou/IOUModal.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ class IOUModal extends Component {
this.addParticipants = this.addParticipants.bind(this);
this.createTransaction = this.createTransaction.bind(this);
this.updateComment = this.updateComment.bind(this);
this.sendMoney = this.sendMoney.bind(this);
const participants = lodashGet(props, 'report.participants', []);
const participantsWithDetails = _.map(OptionsListUtils.getPersonalDetailsForLogins(participants, props.personalDetails), personalDetails => ({
login: personalDetails.login,
Expand All @@ -121,8 +122,6 @@ class IOUModal extends Component {
payPalMeAddress: lodashGet(personalDetails, 'payPalMeAddress', ''),
phoneNumber: lodashGet(personalDetails, 'phoneNumber', ''),
}));
this.isSendRequest = props.iouType === CONST.IOU.IOU_TYPE.SEND;
this.hasGoldWallet = props.userWallet.tierName && props.userWallet.tiername === CONST.WALLET.TIER_NAME.GOLD;

this.state = {
previousStepIndex: 0,
Expand Down Expand Up @@ -193,14 +192,15 @@ class IOUModal extends Component {
*/
getTitleForStep() {
const currentStepIndex = this.state.currentStepIndex;
const isSendingMoney = this.props.iouType === CONST.IOU.IOU_TYPE.SEND;
if (currentStepIndex === 1 || currentStepIndex === 2) {
const formattedAmount = this.props.numberFormat(
this.state.amount, {
style: 'currency',
currency: this.props.iou.selectedCurrencyCode,
},
);
if (this.isSendRequest) {
if (isSendingMoney) {
return this.props.translate('iou.send', {
amount: formattedAmount,
});
Expand All @@ -212,7 +212,7 @@ class IOUModal extends Component {
);
}
if (currentStepIndex === 0) {
if (this.isSendRequest) {
if (isSendingMoney) {
return this.props.translate('iou.sendMoney');
}
return this.props.translate(this.props.hasMultipleParticipants ? 'iou.splitBill' : 'iou.requestMoney');
Expand All @@ -221,6 +221,22 @@ class IOUModal extends Component {
return this.props.translate(this.steps[currentStepIndex]) || '';
}

/**
* Update comment whenever user enters any new text
*
* @param {String} comment
*/
updateComment(comment) {
this.setState({
comment,
});
}

/**
* Update participants whenever user selects the payment recipient
*
* @param {Array} participants
*/
addParticipants(participants) {
this.setState({
participants,
Expand All @@ -247,35 +263,61 @@ class IOUModal extends Component {
if (this.state.currentStepIndex >= this.steps.length - 1) {
return;
}

this.setState(prevState => ({
previousStepIndex: prevState.currentStepIndex,
currentStepIndex: prevState.currentStepIndex + 1,
}));
}

/**
* Update comment whenever user enters any new text
* Checks if user has a GOLD wallet then creates a paid IOU report on the fly
*
* @param {String} comment
* @param {String} paymentMethodType
*/
updateComment(comment) {
this.setState({
sendMoney(paymentMethodType) {
const hasGoldWallet = this.props.userWallet.tierName && this.props.userWallet.tierName === CONST.WALLET.TIER_NAME.GOLD;

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.

The section this.props.userWallet.tierName of this condition is unnecessary


// If the user is trying to send money, then they need to upgrade to a GOLD wallet
if (!hasGoldWallet) {
Navigation.navigate(ROUTES.IOU_ENABLE_PAYMENTS);
return;
}

const amount = Math.round(this.state.amount * 100);
const currency = this.props.iou.selectedCurrencyCode;
const comment = this.state.comment;

const newIOUReportDetails = JSON.stringify({
amount,
currency,
requestorEmail: this.state.participants[0].login,
comment,
idempotencyKey: Str.guid(),
});

IOU.payIOUReport({
chatReportID: lodashGet(this.props, 'route.params.reportID', ''),
reportID: 0,
paymentMethodType,
amount,
currency,
requestorPayPalMeAddress: this.state.participants[0].payPalMeAddress,
requestorPhoneNumber: this.state.participants[0].phoneNumber,
comment,
newIOUReportDetails,
shouldRedirectToChatReport: true,
});
Comment on lines +278 to 310

@nickmurray47 nickmurray47 Dec 25, 2021

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.

Basically moved the minimal sendMoney logic as an action from libs/action/IOU.js to here in IOUModal. Reason being:

  • We are really only supposed to call sendMoney from IOUModal. I couldn't think of a case where we'd want to expose a sendMoney action elsewhere.
  • One of the original reasons for having a sendMoney action was that we wanted the aftereffect of redirecting the user back to the dm of the user they're paying and it breaks app philosophy to call a .then() on any actions inside views. The way I thought I'd accomplish this originally inside a sendMoney action would be to wrap payIOUReport in a promise there then call Navigation.navigate from a .then. It seemed simpler to me to just pass an additional param shouldRedirectToChatReport to payIOUReport to handle the redirect aftereffect.
  • By doing this too, we can also avoid repeating passing the same params through an additional function.

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.

Great summary of the thought process here. Thanks!

Happy to leave things as they are for now, but just wanted to give this one last exploration as problems like these will pop up in the future and how we handle them sets the precedent for others to follow.

We are really only supposed to call sendMoney from IOUModal. I couldn't think of a case where we'd want to expose a sendMoney action elsewhere.

It might be reasonable to extract this into an "action" regardless of code re-use benefits. It will help separate the "action" logic from the view, keep things organized, and we get the benefit of replacing shouldRedirectToChatReport with a .finally() in an IOU.sendMoney() method.

we wanted the aftereffect of redirecting the user back to the dm of the user they're paying and it breaks app philosophy to call a .then() on any actions inside views

Could we still follow this philosophy if we create a new action? Why does this rule exist?

wrap payIOUReport in a promise there then call Navigation.navigate from a .then.

What if we returned the promise here instead of wrapping it with a promise?

asyncOpenURL(payIOUPromise

Calling .then() like so should not break any app philosophy

function sendMoneyAndNavigateToReport(params) {
    payIOUReport(params)
        .finally(() => {
            Navigation.navigate(ROUTES.REPORT);        
        });
}

It seemed simpler to me to just pass an additional param shouldRedirectToChatReport to payIOUReport to handle the redirect aftereffect.

An advantage of navigating from outside this method (instead of within it) is that we pass the responsibility to the caller of the action and reduce the overall responsibility of the action itself. When will shouldRedirectToChatReport be true? Only when we are sending money. Does it depend on anything else in payIOUReport? Only the promise - which could be returned.

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.

It might be reasonable to extract this into an "action" regardless of code re-use benefits. It will help separate the "action" logic from the view, keep things organized, and we get the benefit of replacing shouldRedirectToChatReport with a .finally() in an IOU.sendMoney() method.

I agree with this.

Could we still follow this philosophy if we create a new action? Why does this rule exist?

Yes we could, and I believe this rule exists so we don't introduce any side effects within views (views should be responsible for displaying data is my understanding).

What if we returned the promise here instead of wrapping it with a promise?

I guess I mostly struggled with how to make this work nicely with asyncOpenURL since that method doesn't return a promise. Wouldn't we have to wrap asyncOpenURL in another promise anyways or can we simply return it and attach the .finally handler to it?

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.

Sorry bout that, didn't notice asyncOpenURL was not returning a promise - but can think of two solutions here:

  • Always return the promise passed to asyncOpenURL() instead of void
export default function asyncOpenURL(promise, url) {
    if (!url) {
        return promise;
    }

    return promise.then((params) => {
        Linking.openURL(typeof url === 'string' ? url : url(params));
    });
}
  • Attach all the handlers to the promise and do asyncOpenURL then return it
    payIOUPromise.then((response) => {...});
    asyncOpenURL(payIOUPromise, url);
    return payIOUPromise;

}

/**
* Create the IOU transaction
*
* @param {Array} [splits]
*/
createTransaction(splits) {
const reportID = lodashGet(this.props, 'route.params.reportID', '');

// If the user is trying to send money, then they need to upgrade to a GOLD wallet
if (this.isSendRequest && !this.hasGoldWallet) {
Navigation.navigate(ROUTES.IOU_ENABLE_PAYMENTS);
return;
}

// Only splits from a group DM has a reportID
// Check if reportID is a number
if (splits && CONST.REGEX.NUMBER.test(reportID)) {
Expand All @@ -290,11 +332,12 @@ class IOUModal extends Component {
});
return;
}

if (splits) {
IOU.createIOUSplit({
comment: this.state.comment,

// should send in cents to API
// Send in cents to API.
amount: Math.round(this.state.amount * 100),
currency: this.props.iou.selectedCurrencyCode,
splits,
Expand All @@ -305,7 +348,7 @@ class IOUModal extends Component {
IOU.createIOUTransaction({
comment: this.state.comment,

// should send in cents to API
// Send in cents to API.
amount: Math.round(this.state.amount * 100),
currency: this.props.iou.selectedCurrencyCode,
debtorEmail: OptionsListUtils.addSMSDomainIfPhoneNumber(this.state.participants[0].login),
Expand Down Expand Up @@ -398,13 +441,13 @@ class IOUModal extends Component {
>
<IOUConfirmPage
onConfirm={this.createTransaction}
onSendMoney={this.sendMoney}
hasMultipleParticipants={this.props.hasMultipleParticipants}
participants={this.state.participants}
iouAmount={this.state.amount}
comment={this.state.comment}
onUpdateComment={this.updateComment}
iouType={this.props.iouType}
localCurrencyCode={this.props.myPersonalDetails.localCurrencyCode}
isGroupSplit={this.steps.length === 2}
/>
</AnimatedStep>
Expand Down
8 changes: 4 additions & 4 deletions src/pages/iou/steps/IOUConfirmPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ const propTypes = {
/** Callback to inform parent modal of success */
onConfirm: PropTypes.func.isRequired,

/** Callback to to parent modal to send money */
onSendMoney: PropTypes.func.isRequired,

/** Callback to update comment from IOUModal */
onUpdateComment: PropTypes.func,

Expand All @@ -19,8 +22,6 @@ const propTypes = {
/** IOU amount */
iouAmount: PropTypes.string.isRequired,

localCurrencyCode: PropTypes.string,

/** Selected participants from IOUMOdal with login */
participants: PropTypes.arrayOf(PropTypes.shape({
login: PropTypes.string.isRequired,
Expand Down Expand Up @@ -49,7 +50,6 @@ const defaultProps = {
onUpdateComment: null,
comment: '',
iouType: CONST.IOU.IOU_TYPE.REQUEST,
localCurrencyCode: CONST.CURRENCY.USD,
};

const IOUConfirmPage = props => (
Expand All @@ -60,8 +60,8 @@ const IOUConfirmPage = props => (
onUpdateComment={props.onUpdateComment}
iouAmount={props.iouAmount}
onConfirm={props.onConfirm}
onSendMoney={props.onSendMoney}
iouType={props.iouType}
localCurrencyCode={props.localCurrencyCode}
isGroupSplit={props.isGroupSplit}
/>
);
Expand Down
2 changes: 2 additions & 0 deletions src/pages/iou/steps/IOUParticipantsPage/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ const propTypes = {
isPinned: PropTypes.bool,
isUnread: PropTypes.bool,
reportID: PropTypes.number,
phoneNumber: PropTypes.string,
payPalMeAddress: PropTypes.string,
})),

/* Onyx Props */
Expand Down