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

P2P Enable adding deposit account as payment method #6638

Merged
merged 2 commits into from
Dec 8, 2021

Conversation

marcaaron
Copy link
Contributor

@marcaaron marcaaron commented Dec 8, 2021

cc @stitesExpensify @nickmurray47 if you guys want to also take a look to make sure this will line up with any active changes for "Send Money" and "Payment Methods".

Details

Adds the ability to set up a personal bank account via the payments page

Fixed Issues

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

Tests / QA Steps

  1. Create a new account or remove all payment methods from an existing account before testing (use expensify.com site as new.expensify.com does not yet allow removing payment methods).
  2. Navigate to Settings > Payments > Add payment method and select "Bank account"
  3. Verify the Plaid modal is presented and login with Chase test credentials.
  4. Select an account from the list
  5. Tap "Save & continue" without entering a password.
  6. Verify an error message appears
  7. Enter incorrect password and verify the error shows again
  8. Enter correct password and submit again
  9. Verify you are brought back to the "Payments" view and the account is now present in the list.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

2021-12-08_08-33-41
2021-12-08_08-35-57
2021-12-08_08-36-38

Mobile Web

2021-12-08_08-47-33
2021-12-08_08-47-36
2021-12-08_08-47-36
2021-12-08_08-48-08
2021-12-08_08-48-11
2021-12-08_08-48-15
2021-12-08_08-48-32

Desktop

2021-12-08_08-53-32
2021-12-08_08-53-36
2021-12-08_08-54-01
2021-12-08_08-54-05
2021-12-08_08-54-10
2021-12-08_08-54-20

iOS

2021-12-08_09-02-00
![2021-12-08_09-02-07](https://user-images.githubusercontent.com/32969087/145268238-5ddf2a88-1492-4ae2-9969-ec
2021-12-08_09-02-37
320b4beb64.png)
2021-12-08_09-02-45
2021-12-08_09-02-59

Android

2021-12-08_09-11-48
2021-12-08_09-11-53
2021-12-08_09-12-50
2021-12-08_09-13-19

@marcaaron marcaaron self-assigned this Dec 8, 2021
@marcaaron marcaaron changed the title [WIP] [HOLD] P2P KYC Upgrade Flow [WIP] [HOLD] P2P Allow adding personal bank account Dec 8, 2021
@marcaaron marcaaron changed the title [WIP] [HOLD] P2P Allow adding personal bank account [WIP] [HOLD] P2P Enable adding deposit account as payment method Dec 8, 2021
@marcaaron marcaaron changed the title [WIP] [HOLD] P2P Enable adding deposit account as payment method [WIP] P2P Enable adding deposit account as payment method Dec 8, 2021
Refactor Add Payment method menu and link to add personal bank account flow. fix password requirement on plaid modal.

Rm console.log

Only add ability to add deposit account for now

require password param

add password requirement back to plaid form but make it optional

add some loading spinner polish
@marcaaron marcaaron force-pushed the marcaaron-kycUpgradeFlow branch from 5080dfd to 9aa0509 Compare December 8, 2021 18:40
@marcaaron marcaaron changed the title [WIP] P2P Enable adding deposit account as payment method P2P Enable adding deposit account as payment method Dec 8, 2021
@marcaaron
Copy link
Contributor Author

Putting this into reviews while I continue to test on other platforms.

@marcaaron marcaaron marked this pull request as ready for review December 8, 2021 18:40
@marcaaron marcaaron requested a review from a team as a code owner December 8, 2021 18:40
@MelvinBot MelvinBot requested review from ctkochan22 and removed request for a team December 8, 2021 18:42
@marcaaron
Copy link
Contributor Author

This is probably not a blocker, but one interesting / weird thing discovered while testing is that on Android (but not any other platform) the plaid payments list shows "Wells Fargo" and the correct icon is not displayed. :lolwut:

Copy link

@ctkochan22 ctkochan22 left a comment

Choose a reason for hiding this comment

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

Looks good and straight-forward. Would love to get another review though, still ramping up on App issues

@marcaaron marcaaron requested a review from a team December 8, 2021 21:16
@marcaaron
Copy link
Contributor Author

Sounds good, thanks @ctkochan22. Re-rolling with PullerBear to get another set of eyes. 🙇

@MelvinBot MelvinBot requested review from yuwenmemon and removed request for a team December 8, 2021 21:17
Copy link
Contributor

@stitesExpensify stitesExpensify left a comment

Choose a reason for hiding this comment

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

Code LGTM, shouldn't interfere with my projects. Leaving it to @nickmurray47 for final review

PaymentMethods.getPaymentMethods()
.then(() => {
Navigation.goBack();
Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {loading: false});
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want success growls here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dang yea that would probably be a nice touch. It's pretty strange that we just kick you back to the payments page now that I think about it. But it works for now.

Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {loading: false});
})
.catch(() => {
Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {loading: false});
Copy link
Contributor

Choose a reason for hiding this comment

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

Failure growl here

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.

Gave it a look and also looks pretty good to me!

Comment on lines +14 to +19
isVisible: PropTypes.bool.isRequired,
onClose: PropTypes.func.isRequired,
anchorPosition: PropTypes.shape({
top: PropTypes.number,
left: PropTypes.number,
}),
Copy link
Contributor

Choose a reason for hiding this comment

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

nab: prop-type doc

@yuwenmemon yuwenmemon merged commit ce94b69 into main Dec 8, 2021
@yuwenmemon yuwenmemon deleted the marcaaron-kycUpgradeFlow branch December 8, 2021 22:58
@OSBotify
Copy link
Contributor

OSBotify commented Dec 8, 2021

🚀 Deployed to staging by @yuwenmemon in version: 1.1.18-6 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 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 ✅

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