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

Refactor Settlement Button for reuse in both Details and Confirm screens #6651

Merged
merged 2 commits into from
Dec 9, 2021

Conversation

marcaaron
Copy link
Contributor

@marcaaron marcaaron commented Dec 8, 2021

Details

One part Send Money clean up step. One part prep for adding KYC triggers to all these flows.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/187943

Tests / QA Steps

This PR should not change any existing behavior. Still wrote up some regression tests here for where the flow is currently at to make sure I'm not blowing anything up.

Setup

  • Create two test accounts both without phone logins or paypal added.
  • Do this before each of the following tests for each platform tested or remove the phone logins / paypal info before moving on to the next test/platform.
  • Make sure Venmo is not installed

Send Money case

  1. Have User A create a chat with User B
  2. Select "Send Money" from the + menu
  3. Enter amount and continue
  4. Tap the down caret
  5. Verify you see both the "Settle up elsewhere" and "Pay with Expensify" options (Note: must be on the beta to see the Expensify option)
  6. In User B account add a paypal option via "Settings > Payments"
  7. Refresh User A and try to send money again - verify the "Pay with PayPal.me" option appears.
  8. Test all options and make sure you end up on the "Verify identity" page (other options are still in development)
  9. Filter logs for [IOU] and verify that you see this with the correct payment method selected
[info] [IOU] Sending money via: Expensify - ""

Additional mobile only steps:

  1. As User B add a phone login
  2. As User A verify the venmo option is not available when sending money (unless venmo is installed)
  3. Install Venmo and verify the option is now available
  4. The option is not yet enabled so selecting it should open the "Verify Identity" page for now.

IOU case

  1. Have User A create a chat with User B
  2. Select "Request Money" from the + menu
  3. Enter amount and press "Request"
  4. As User B enter the chat and open the request details
  5. Verify that "Pay with Expensify" and settle up elsewhere are the only options.
  6. Select "Pay with Expensify" and verify it shows a modal about not having a gold wallet.
  7. Select "Settle up" and verify it closes out the request.
  8. As User A add a paypal account and create a new request
  9. As User B find the request and verify the paypal option now exists. Select it and verify it opens up a new tab out to paypal and the request is settled.

Additional mobile only steps:

  1. As User A add a phone login and create a new request
  2. As User B verify the venmo option is not available (unless venmo is installed)
  3. Install Venmo and verify the option is now available
  4. Settle up via Venmo and verify the request is closed out.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@marcaaron marcaaron self-assigned this Dec 8, 2021
Use recipient because it explains the significance of props.participants[0] better

Define whether to use venmo or paypal via props;

Clean up currency code changes

lesss changes
@marcaaron marcaaron force-pushed the marcaaron-payWithExpensifyButton branch from 575de5b to 2f679df Compare December 8, 2021 22:59
@marcaaron marcaaron marked this pull request as ready for review December 8, 2021 23:55
@marcaaron marcaaron requested a review from a team as a code owner December 8, 2021 23:55
@marcaaron marcaaron changed the title [WIP] Refactor Settlement Button for reuse in both Details and Confirm screens Refactor Settlement Button for reuse in both Details and Confirm screens Dec 8, 2021
@MelvinBot MelvinBot requested review from marcochavezf and removed request for a team December 8, 2021 23:55
Copy link
Contributor

@nickmurray47 nickmurray47 left a comment

Choose a reason for hiding this comment

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

Wow, thanks for the refactor! This looks great

@marcaaron
Copy link
Contributor Author

Thanks @nickmurray47 gonna merge this so we can unblock some of the other issues. @marcochavezf let me know if you have any thoughts and I can create a follow up to address them!

@marcaaron marcaaron merged commit 6734c9b into main Dec 9, 2021
@marcaaron marcaaron deleted the marcaaron-payWithExpensifyButton branch December 9, 2021 20:08
@OSBotify
Copy link
Contributor

OSBotify commented Dec 9, 2021

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@marcochavezf
Copy link
Contributor

@marcaaron The changes look good, I found a minor issue though. My currency by default is MXN and when I change it to USD doesn't seem to reflect in the SettlementButton component and thus the "Pay with Expensify" doesn't appear for me:

Screen.Recording.2021-12-09.at.15.28.49.mov

@marcaaron
Copy link
Contributor Author

Oh no, hmm did I break it? Can you check and see how it works on production?

@marcaaron
Copy link
Contributor Author

It looks like changing the currency there will update the IOU currency

/**
* Confirms the selection of currency and sets values in Onyx
* @return {void}
*/
confirmCurrencySelection() {
IOU.setIOUSelectedCurrency(this.state.toggledCurrencyCode);
Navigation.goBack();
}

Maybe @nickmurray47 knows what the expected behavior should be eventually.

But seems like we were referring to the local currency before and not the IOU currency (at least for the IOU send feature)

if (this.props.localCurrencyCode === CONST.CURRENCY.USD && Permissions.canUsePayWithExpensify(this.props.betas) && Permissions.canUseWallet(this.props.betas)) {
confirmationButtonOptions.push({text: this.props.translate('iou.settleExpensify'), icon: Expensicons.Wallet});
}

That code is before my changes so I was just trying to keep the same functionality here - but perhaps you are right that it was incorrect.

@nickmurray47
Copy link
Contributor

Yes, the Pay With Expensify option is only available for USD so nothing broke. Only USD users can fill out KYC and get a GOLD wallet.

@marcaaron
Copy link
Contributor Author

Ah got it. So in that case, did we get this part wrong over here?

<SettlementButton
isLoading={this.props.iou.loading}
onPress={paymentMethodType => this.performIOUPayment(paymentMethodType)}
recipientPhoneNumber={this.getSubmitterPhoneNumber()}
shouldShowPaypal={Boolean(lodashGet(this.props, 'iouReport.submitterPayPalMeAddress'))}
currency={lodashGet(this.props, 'iouReport.currency')}

When paying the IOU we use the currency on the "iou report", but should actually be looking at the user's local currency?

@nickmurray47
Copy link
Contributor

nickmurray47 commented Dec 10, 2021

Ah, sorry missed this earlier.

should actually be looking at the user's local currency?

Yes, we should be using the user's local currency.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @marcaaron in version: 1.1.19-5 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @Julesssss in version: 1.1.21-1 🚀

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

@sakluger
Copy link
Contributor

Heads up @marcaaron @marcochavezf! This PR caused a bug here: #12291.

@sakluger
Copy link
Contributor

@jasperhuangg asked me to mention that we should be sure that all reusable components react accordingly to any prop updates.

@marcaaron
Copy link
Contributor Author

Statute of limitations. This PR is almost a year old 😄

@jasperhuangg do you have a summary somewhere?

Which props changed broke stuff? Is there some way we can avoid this in the future?

@jasperhuangg
Copy link
Contributor

Looks like we cleared this up in the other issue, read on from here for more context

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants