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

Create new Payment ButtonWithMenu and add it to the Confirm Page #3866

Merged
merged 32 commits into from
Oct 4, 2021

Conversation

nickmurray47
Copy link
Contributor

@nickmurray47 nickmurray47 commented Jul 2, 2021

cc @mountiny

Details

Adds the Send Money option behind a beta to the DM and Global menu. Refactors the use of a string 'send' to be a CONST. Adds a new component ButtonWithMenu to display the list of available payment methods if a user is making a Send Money request.

Fixed Issues

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

Tests / QA Steps

If testing on staging, then you'll need to be added to the SendMoney Beta

Verify Send money appears in menus

  1. Login to account A with an existing wallet
  2. Click on the global create menu and verify the Send money option appears

Screen Shot 2021-10-01 at 12 36 54 PM

  1. Go into a DM with account B, click on the menu and verify the Send money option appears there
    Screen Shot 2021-10-01 at 12 37 02 PM

Verify dropdown menu includes payment options

  1. From the DM, click the Send Money option, enter an amount, then on the confirmation page, verify that a dropdown button menu appears with at least two options (Pay with Expensify and I'll settle up elsewhere).
  2. From account B in a separate window, add a PayPalMe address.
  3. From account A, attempt to Send Money to account B again and verify that the PayPalMe option appears from the dropdown payment options

Screen Shot 2021-10-01 at 12 37 25 PM

  1. (If testing on a real device) Add a phone number to account B, download Venmo from the app store, then verify the Venmo option appears.

Check for regressions
8. (Checking for regressions) From the DM, click the Request option, follow the full flow and make sure the confirmation button says Request $X.
Screen Shot 2021-09-22 at 4 57 52 PM

  1. (Checking for regressions) From the global create, click the Split option, follow the full flow and make sure the confirmation button says Split $X
    Screen Shot 2021-09-22 at 4 58 27 PM

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

See above

Mobile Web

Desktop

iOS

Screen Shot 2021-08-04 at 10 58 10 PM
Screen Shot 2021-08-04 at 10 57 22 PM

Android

Screen Shot 2021-08-05 at 9 51 29 AM
Screen Shot 2021-08-05 at 9 51 10 AM

@nickmurray47 nickmurray47 requested a review from a team as a code owner July 2, 2021 16:32
@nickmurray47 nickmurray47 self-assigned this Jul 2, 2021
@MelvinBot MelvinBot requested review from madmax330 and removed request for a team July 2, 2021 16:33
@nickmurray47 nickmurray47 changed the title Create new Payment ButtonWithMenu and add it to the Confirm Page [WIP] Create new Payment ButtonWithMenu and add it to the Confirm Page Jul 2, 2021
@nickmurray47 nickmurray47 changed the title [WIP] Create new Payment ButtonWithMenu and add it to the Confirm Page Create new Payment ButtonWithMenu and add it to the Confirm Page Aug 4, 2021
@nickmurray47
Copy link
Contributor Author

this is ready for review @madmax330 and @mountiny going to add you as a reviewer too

@nickmurray47 nickmurray47 requested a review from mountiny August 5, 2021 16:53
src/libs/Permissions.js Outdated Show resolved Hide resolved
@nickmurray47 nickmurray47 changed the title Create new Payment ButtonWithMenu and add it to the Confirm Page [HOLD] Create new Payment ButtonWithMenu and add it to the Confirm Page Aug 6, 2021
@nickmurray47 nickmurray47 changed the title [HOLD] Create new Payment ButtonWithMenu and add it to the Confirm Page Create new Payment ButtonWithMenu and add it to the Confirm Page Sep 14, 2021
Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

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

Overall the code looks good to me. I have left couple of comments, but also I was not sure if this is good to review of if it is on HOLD as there is hold mentioned in the PR body.

Let me know if it good for testing and final reviews. Then we can add it to the other PR and test it with it.

mountiny
mountiny previously approved these changes Sep 30, 2021
Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

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

Looks great to me! I feel like it will be way easier to test for any regressions together with the other PRs like this: #4329

@stitesExpensify
Copy link
Contributor

Some more testing problems,

  1. I can't get PayPal to show up (both accounts I'm testing with have paypal payment methods)
  2. When I choose "send money" it requests money instead

@nickmurray47
Copy link
Contributor Author

nickmurray47 commented Sep 30, 2021

Some more testing problems,

  1. I can't get PayPal to show up (both accounts I'm testing with have paypal payment methods)
  2. When I choose "send money" it requests money instead

I'm re-testing the full flow now. PayPal not showing up might be related to not having updated Auth in a while since we recently included the PayPal username in the personal details that get pulled down.

For 2. if you follow the full flow, it will "request" money at the end if that's what you're referring to. This PR will connect the API command properly and this PR just puts "send money" in as an option.

@nickmurray47
Copy link
Contributor Author

Found the issue with the PayPal address, updating shortly

@nickmurray47
Copy link
Contributor Author

nickmurray47 commented Sep 30, 2021

Screen Shot 2021-09-30 at 1 18 32 PM

PayPal appears properly now after adding it to the formatPersonalDetails function.

Also just wanted to make a note on testing makeCancellablePromise for the context in this PR when we check to see if Venmo is installed locally. On a dev simulator, we can't, unfortunately, add Venmo from the App Store so the promise will never successfully complete, but if we add a log line to makeCancellablePromise like so and quickly exit the confirmation page we can see the promise does get cancelled properly when the component unmounts.

Screen Shot 2021-09-30 at 1 51 29 PM

@nickmurray47
Copy link
Contributor Author

Ready for another round! Appreciate the patience, guys!

@shawnborton
Copy link
Contributor

Could you update the screenshots in the original comment? For example, this screenshot here:
image

Looks incorrect - but later on, the screenshot you posted here:
image

Looks good - as the menu should float above the green button and not overlap it.

@stitesExpensify
Copy link
Contributor

Also just wanted to make a note on testing makeCancellablePromise for the context in this PR when we check to see if Venmo is installed locally. On a dev simulator, we can't, unfortunately, add Venmo from the App Store so the promise will never successfully complete, but if we add a log line to makeCancellablePromise like so and quickly exit the confirmation page we can see the promise does get cancelled properly when the component unmounts.

Thanks for confirming this 👍

@stitesExpensify
Copy link
Contributor

Tests are 🟢 besides what I mentioned with it actually requesting money.

For 2. if you follow the full flow, it will "request" money at the end if that's what you're referring to. This PR will connect the API command properly and this PR just puts "send money" in as an option.

Should this be behind a beta then since it doesn't actually send money? Or are we just shipping it "broken"

@nickmurray47
Copy link
Contributor Author

nickmurray47 commented Oct 1, 2021

Should this be behind a beta then since it doesn't actually send money? Or are we just shipping it "broken"

Yup, this whole flow is behind a beta flag! Both the global create and DM menus have ...(Permissions.canUseIOUSend(this.props.betas) for the send money option so the flow will only be available to Expensify employees @stitesExpensify

@@ -560,6 +560,19 @@ class ReportActionCompose extends React.Component {
},
},
] : []),
...(Permissions.canUseIOUSend(this.props.betas) && !hasMultipleParticipants ? [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Beta flag here for the DM menu

@@ -158,6 +159,13 @@ class SidebarScreen extends Component {
text: this.props.translate('sidebarScreen.newGroup'),
onSelected: () => Navigation.navigate(ROUTES.NEW_GROUP),
},
...(Permissions.canUseIOUSend(this.props.betas) ? [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

And the other flag here

@stitesExpensify
Copy link
Contributor

lol. I was just going through the code for a final review and realized that :ohnothing: Ignore me!

@stitesExpensify
Copy link
Contributor

All you @mountiny @madmax330 !

@mountiny mountiny self-requested a review October 4, 2021 06:38
Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

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

Great job on this one @nickmurray47 and also great learning experience to see your review @stitesExpensify! I don't see anything bad in the code now, so all yours @madmax330!

@madmax330 madmax330 merged commit 612d397 into main Oct 4, 2021
@madmax330 madmax330 deleted the nmurray-buttonwithmenu branch October 4, 2021 11:02
@OSBotify
Copy link
Contributor

OSBotify commented Oct 4, 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.

@OSBotify
Copy link
Contributor

OSBotify commented Oct 4, 2021

🚀 Deployed to staging by @madmax330 in version: 1.1.4-1 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Oct 6, 2021

🚀 Deployed to production by @chiragsalian in version: 1.1.5-1 🚀

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

@luacmartins
Copy link
Contributor

luacmartins commented Feb 14, 2023

I believe this PR introduced this bug: #14305. We are storing a translated text in state and only updating defaultButtonOption in the constructor, which prevents it to update to the latest selected language.

@github-actions
Copy link
Contributor

CLA Assistant Lite bot:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


Nicholas Murray seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You can retrigger this bot by commenting recheck in this Pull Request

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.

7 participants