Adjusting IOUModal and adding createAndPayIOU#4329
Conversation
|
added myself as a reviewer to track this |
|
I had a look at the code, looks good so far (ignoring the style problems). I'll be off on Monday, if you need someone to review on Monday, I'll suggest you to add another reviewer and remove me ;) |
|
@nickmurray47 For the linting errors, I believe this issue: https://github.com/Expensify/Expensify/issues/169175 would be good to implement alongside as it will refactor the I had couple of questions for things which were not clear to me. I will write them as comments to the code so I do not link the lines here which would be clumsy! |
|
Refactored the |
…e use of crypto.getRandomValues() function
|
@nickmurray47 Alright, so I have tried to look into what we can do in order to get the submitterPaypalMeAddress and phoneNumber for the other participants of the reports and I believe the best option it update this Auth command, to be more precise to update this Account class function to also return the Same for phone number as secondary login, we need to return it from this query otherwise we would have to create a new command and add API calls which we should avoid. I would like to know your opinion before starting to work on this. Thank you for feedback! |
This comment has been minimized.
This comment has been minimized.
|
@nickmurray47 Yes, but those are details of the signed in user. I thought the |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Updated! Sorry for the delay, have been traveling, been online/offline for the past few days and doing work on the move. *will be back online later today and tomorrow morning too A few things to note:
|
mountiny
left a comment
There was a problem hiding this comment.
@nickmurray47 Great refactorings here. It is way cleaner!
I have tested it with the Web-E branch linked, but I run into a problem, where I can use the Pay with Expensify option from account with GOLD wallet if someone requested money from me, but if I use the Send Money flow, the Pay with Expensify option does not show up for me.
It works in case when I should pay for requested amount. Does the account B also need GOLD wallet? Would you be able to check if it works now for you locally. Thank you!
|
@vitHoracek Ah yeah, just ran into the same thing with testing down here in Mexico! That was an oversight where for sending money via Expensify Wallet we were checking that the user's local currency was USD and for paying a requested IOU we were instead checking that the IOU report's currency was USD. Updated the Send Money flow so that we check the IOU report's currency. |
| sendMoney(paymentMethodType) { | ||
| const hasGoldWallet = this.props.userWallet.tierName && this.props.userWallet.tierName === CONST.WALLET.TIER_NAME.GOLD; | ||
|
|
||
| // 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, | ||
| }); |
There was a problem hiding this comment.
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
sendMoneyfromIOUModal. I couldn't think of a case where we'd want to expose asendMoneyaction elsewhere. - One of the original reasons for having a
sendMoneyaction 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 asendMoneyaction would be to wrappayIOUReportin a promise there then callNavigation.navigatefrom a.then. It seemed simpler to me to just pass an additional paramshouldRedirectToChatReporttopayIOUReportto handle the redirect aftereffect. - By doing this too, we can also avoid repeating passing the same params through an additional function.
There was a problem hiding this comment.
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?
Line 281 in ca34f19
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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;|
updated and retested |
marcaaron
left a comment
There was a problem hiding this comment.
Looks great, thanks for the changes and patience with the review ✅
Full disclosure: I normally test every PR locally, but didn't test this one. Looks like @mountiny has + I'm assuming we are still under the beta and have plenty of time to iron out any issues.
| sendMoney(paymentMethodType) { | ||
| const hasGoldWallet = this.props.userWallet.tierName && this.props.userWallet.tierName === CONST.WALLET.TIER_NAME.GOLD; | ||
|
|
||
| // 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, | ||
| }); |
There was a problem hiding this comment.
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?
Line 281 in ca34f19
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.
There was a problem hiding this comment.
@nickmurray47 Great explanation. I cannot approve this PR as I am the original author, thank you for taking this over once the university hit me 🙇
I wanted to test it with a new GOLD wallet account, but I ran into some problems with setting it up. I will need to resolve that, but as Marc mentioned, since this is in beta, we have room for some edge cases coming up later.
aldo-expensify
left a comment
There was a problem hiding this comment.
The code looks good to me, just left a small comment on one condition.
Shouldn't this pull request be labeled with [HOLD] until this https://github.com/Expensify/Web-Expensify/pull/32181 is merged?
Some comments about my experience testing it in case you see anything to improve there:
- Struggled a while trying to figure out why I couldn't see the "Pay with Expensify" option. It was because it only appears for USD currency. This is not explicit in the App, but maybe we don't care because this is only being used in the US for now?
- When you "Send money", it looks like this:
| updateComment(comment) { | ||
| this.setState({ | ||
| sendMoney(paymentMethodType) { | ||
| const hasGoldWallet = this.props.userWallet.tierName && this.props.userWallet.tierName === CONST.WALLET.TIER_NAME.GOLD; |
There was a problem hiding this comment.
The section this.props.userWallet.tierName of this condition is unnecessary
Hey @mountiny , I was able to test locally with a new GOLD wallet account using the SO. If you are having some problems with it, maybe I can help. |
|
Thanks for the reviews! @aldo-expensify to answer your questions:
The PRs need each other to work. I wanted to merge this frontend PR first because this PR doesn't risk breaking anything - it only materially changes the
This is true, the GOLD wallets are only available for US people since only they can pass our KYC verifications (subject to hopefully change in the future). I think we will have an additional follow-up polish PR to make that explicitly clear to international users.
Yes, we are creating an additional "action" in Auth. I will also need to create a quick Auth polish PR that doesn't do this. |
Let's create some follow-up issues to track this stuff? Nice observations @aldo-expensify |
|
Follow-up issues mentioned in main tracking issue, going to self-merge this guy |
|
Triggered auto assignment to @nkuoch ( |
|
@nickmurray47 looks like this was merged without passing tests. Please add a note explaining why this was done and remove the |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
Not an emergency, all tests had passed |
|
🚀 Deployed to staging by @nickmurray47 in version: 1.1.23-2 🚀
|
|
Internal QA 👍 |
|
🚀 Deployed to production by @francoisl in version: 1.1.24-8 🚀
|
Pullerbear @aldo-expensify
cc @nickmurray47
Details
Pass necessary params,
reportID = 0, paymentMethodType, amount, currency, requestorPhoneNumber, requestorPayPalMeAddress, comment, requestorEmailfor SendMoney flow to work.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/169171
Tests
If testing, need to pull and merge this Web-E branch
Sending Money
Send moneyoption appearsSend moneyoption appears thereSend Moneyoption, enter an amount in USD, then on the confirmation page, verify that a dropdown button menu appears with at least two options (Pay with ExpensifyandI'll settle up elsewhere).Pay with Expensifyand verify an IOU with that amount is sent.I'll settle up elsewhereand verify an IOU with that amount in USD is sent.QA
Check for regressions
(Checking for regressions) From the DM, click the
Requestoption, follow the full flow and make sure the confirmation button saysRequest $X.(Checking for regressions) From the global create, click the
Splitoption, follow the full flow and make sure the confirmation button saysSplit $XTested On
Screenshots
Web
Mobile Web
Desktop
iOS
Android