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

Add default/delete popover and supporting methods #4437

Merged
merged 115 commits into from
Jan 17, 2022

Conversation

stitesExpensify
Copy link
Contributor

@stitesExpensify stitesExpensify commented Aug 6, 2021

Details

Adding the ability to set a default wallet account, and delete payment methods. The default functionality is hidden behind the wallet beta.

Fixed Issues

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

Tests

  1. Make sure you have a gold wallet
  2. Add a personal bank account to an account
  3. Add a debit card to your account
  4. Run this query to set up the account properly
update funds set additionalData='{"isBillingCard":false,"isP2PDebitCard":true}' where fundID=<INSERT ID>;
  1. Make sure you can set both types of account to the default
  2. Make sure you can delete both types of account and that the message makes sense
  3. Add a paypal.me
  4. Delete the paypal.me
  5. On an account that is not on the wallet beta, select an account and make sure you do not see the "default" button

QA Steps

  1. Make sure you have a gold wallet
  2. Add a personal bank account to an account via the add payment method button on the payments page
  3. Add a debit card to your account via the add payment method button on the payments page
  4. Make sure you can set both types of account to the default
  5. Make sure you can delete both types of account and that the message makes sense
  6. Add a paypal.me
  7. Make sure you can't set the paypal as default
  8. Delete the paypal.me
  9. On an account that is not on the wallet beta, select an account and make sure you do not see the "default" button

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

2022-01-11_17-30-17

2022-01-11_17-30-08

2022-01-11_17-29-59

2022-01-11_17-29-52

Mobile Web

2022-01-11_17-19-36

2022-01-11_17-19-23

2022-01-11_17-19-14

2022-01-11_17-18-45

Desktop

2022-01-11_17-33-12

2022-01-11_17-33-16

2022-01-11_17-33-22

2022-01-11_17-33-25

iOS

Android

@stitesExpensify
Copy link
Contributor Author

Updated!
2022-01-13_11-41-00
2022-01-13_11-28-08

@shawnborton
Copy link
Contributor

Looks great, thanks!

shawnborton
shawnborton previously approved these changes Jan 13, 2022
@marcaaron marcaaron self-requested a review January 13, 2022 18:54
marcaaron
marcaaron previously approved these changes Jan 13, 2022
@stitesExpensify stitesExpensify dismissed stale reviews from marcaaron and shawnborton via e11c3d3 January 13, 2022 22:20
@stitesExpensify
Copy link
Contributor Author

Chatted with Kevin and we want the "default" button behind the wallet beta.
2022-01-13_13-59-15

@tylerkaraszewski tylerkaraszewski removed their request for review January 17, 2022 12:30
}

function deletePayPalMe() {
NameValuePair.set(CONST.NVP.PAYPAL_ME_ADDRESS, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if this fails? We don't catch it like other cases? Maybe I am missing that part when following the flow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like it would just fail. I will make a new issue to fix that as it looks like we don't return the call in NVP.set so none of our calls to that method have a failure case.

@marcaaron marcaaron merged commit d1fd5be into main Jan 17, 2022
@marcaaron marcaaron deleted the stites-setDefaultAndDelete2 branch January 17, 2022 23:45
@OSBotify
Copy link
Contributor

✋ 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

🚀 Deployed to staging by @marcaaron in version: 1.1.30-4 🚀

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

@mvtglobally
Copy link

@Julesssss @stitesExpensify @pecanoro @shawnborton How do we get Gold account? What is the way to enable different account wallet types ?

@stitesExpensify
Copy link
Contributor Author

@kevinksullivan can we get applause a gold wallet? Or should this be internal QA?

@mvtglobally
Copy link

@stitesExpensify If you think in future we may need to validate different accounts on regular bases, we need to have QA access to such accounts somehow. If this is a one time test, you can validate internally if its faster

@kevinksullivan
Copy link
Contributor

@stitesExpensify Let's go to internal QA for now. We will need to allow for some test creds to go through KYC once the Wallet is live and we want to test that flow, but I don't think that's worth developing prior before getting it on prod with our own QA first.

@stitesExpensify
Copy link
Contributor Author

Cool, I'll test it this morning

@stitesExpensify
Copy link
Contributor Author

stitesExpensify commented Jan 19, 2022

If you think in future we may need to validate different accounts on regular bases, we need to have QA access to such accounts somehow

We will probably want to test this type of PR at some point, but for now it's under a beta :)

@stitesExpensify
Copy link
Contributor Author

QA passed

@OSBotify
Copy link
Contributor

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

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

@PauloGasparSv
Copy link
Contributor

Hi there,

I think this resulted in a bug issue #14524 where the "Make Default" and "Delete" popups were not being dismissed if another session deleted the same selected Payment Method.

Pretty sure this is a detail we missed as I didn't find any mention of it in the Design Doc.

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.

10 participants