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

[$500] [HOLD for payment 2023-10-30] [HOLD for payment 2023-10-27] [P2P Activation] [External] [CRITICAL] Update pending Wallet state to show Enable Wallet button #29320

Closed
kevinksullivan opened this issue Oct 11, 2023 · 44 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@kevinksullivan
Copy link
Contributor

kevinksullivan commented Oct 11, 2023

Held on

Update pending Wallet state to show Enable Wallet button

Update the UI as per this mockup below. For Wallets that are in the SILVER tier. We’ll need to build off of this PR that is adding the assigned card tile with Expensify card list to the Wallet page.

Clicking the Enable wallet button will redirect to the wallet KYC flow.

image

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01d836dcba58f2381d
  • Upwork Job ID: 1719513835432079360
  • Last Price Increase: 2023-11-01
@kevinksullivan kevinksullivan added the Daily KSv2 label Oct 11, 2023
@kevinksullivan
Copy link
Contributor Author

@samh-nl here is your issue!

@MariaHCD
Copy link
Contributor

This PR is also relevant since it is redesigning the wallet page: #26406

@MariaHCD
Copy link
Contributor

@samh-nl Hopefully, this PR that is this held on should be merged soon: #26406

To clarify your question regarding the Enable wallet:

We should redirect to the enable payments route. However, is it possible to redirect to the KYCWall first? If the user clicks Enable wallet and they don't have a personal bank account, we'd want to redirect them to setup their bank account first. Once they have their bank account setup, we can direct them to the KYC flow at ROUTES.SETTINGS_ENABLE_PAYMENTS

Let me know what you think.

@samh-nl
Copy link
Contributor

samh-nl commented Oct 13, 2023

I'm subscribed to the PR for notifications.

Sure! The logic in KYCWall furthermore already handles redirecting to ROUTES.SETTINGS_ENABLE_PAYMENTS if there is a payment method but there is no gold wallet yet here.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Oct 15, 2023
@samh-nl
Copy link
Contributor

samh-nl commented Oct 15, 2023

PR is ready to be reviewed. I have also added a few questions in this comment here

@MariaHCD MariaHCD added Waiting for copy User facing verbiage needs polishing and removed Reviewing Has a PR in review labels Oct 16, 2023
@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Oct 16, 2023
@MariaHCD
Copy link
Contributor

MariaHCD commented Oct 16, 2023

@samh-nl noticed that the text displayed at the Additional Details step during Wallet KYC implies that there is a payment pending processing after the wallet is enabled:

We need to confirm the following information before we can process this payment

Screenshot 2023-10-16 at 4 15 38 PM

While this copy is true if the user triggered KYC when paying an IOU or if they triggered KYC when attempting to transfer their wallet balance, it is not relevant when a user triggers KYC when clicking on the new Enable wallet button that we'll show on the wallet page: https://docs.google.com/document/d/1gJKtqu7wXEzjsnf5QqlhKtHBRGj-bnyOaNfEfF1UT5o/edit#bookmark=id.ysre2wqb5z8p

  1. @samh-nl do you think we can show this existing copy conditionally if the KYC wall is triggered when paying an IOU or when transferring wallet balance? And the new copy if the user triggered KYC via the Enable wallet button?
  2. @ryanschaffer we'll need new copy that can be shown on the Additional Details form if the KYC flow is triggered when clicking the Enable wallet button

@samh-nl
Copy link
Contributor

samh-nl commented Oct 16, 2023

We currently use Wallet.setKYCWallSourceChatReportID to store the report ID if this initiated the KYC flow (link), we can expand this function.

@kevinksullivan
Copy link
Contributor Author

kevinksullivan commented Oct 16, 2023

Let's just update the copy in all circumstances to say:

We need to confirm the following information before you can send and receive money from your Wallet.

@kevinksullivan kevinksullivan removed the Waiting for copy User facing verbiage needs polishing label Oct 16, 2023
@MariaHCD
Copy link
Contributor

@kevinksullivan can we also get the Spanish copy 🙏🏼

@MitchExpensify
Copy link
Contributor

@kevinksullivan
Copy link
Contributor Author

Spanish translation @samh-nl

Necesitamos confirmar la siguiente información antes de que puedas enviar y recibir dinero desde tu Billetera

@samh-nl
Copy link
Contributor

samh-nl commented Oct 16, 2023

Ok perfect, I will adjust and push along with a couple changes.

@kevinksullivan
Copy link
Contributor Author

thanks!

@samh-nl
Copy link
Contributor

samh-nl commented Oct 16, 2023

Pushed and also merged main into branch.

I've got one question: the KYC wall uses PaymentUtils.hasExpensifyPaymentMethod to check if there's already a valid bank account.

However, this assumes bank accounts where isDefaultCredit() is true to be valid, but it doesn't consider if isWithdrawal() is true.
Looking at the JSDoc of both functions here, is this correct behavior in the case of enabling the wallet? In other words: are these the right bank accounts we should check for?

@samh-nl
Copy link
Contributor

samh-nl commented Oct 16, 2023

I think the we in "Can we use this account to pay other people?" refers to whether Expensify can transfer funds to it (hence the term withdrawal), so in that case it should be correct.

@MariaHCD
Copy link
Contributor

In other words: are these the right bank accounts we should check for?

Good question, we need to look for a valid deposit bank account i.e. a credit bank account. So the logic in hasExpensifyPaymentMethod is correct

@MariaHCD
Copy link
Contributor

@samh-nl, curious if you've run into this issue before: #29714

@samh-nl
Copy link
Contributor

samh-nl commented Oct 17, 2023

I think I encountered something similar at some point yesterday, and I thought perhaps I selected the wrong Plaid account, but I couldn't reproduce after that.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Oct 23, 2023
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2023-10-27] [P2P Activation] [External] [CRITICAL] Update pending Wallet state to show Enable Wallet button [HOLD for payment 2023-10-30] [HOLD for payment 2023-10-27] [P2P Activation] [External] [CRITICAL] Update pending Wallet state to show Enable Wallet button Oct 23, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 23, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.88-11 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-10-30. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Oct 27, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 30, 2023

@joelbettner, @samh-nl Whoops! This issue is 2 days overdue. Let's get this updated quick!

@MariaHCD
Copy link
Contributor

The App PR was merged and deployed last week. @samh-nl just requires payment. Adding a BZ team member here.

@MariaHCD MariaHCD added the Bug Something is broken. Auto assigns a BugZero manager. label Oct 30, 2023
@melvin-bot melvin-bot bot removed the Overdue label Oct 30, 2023
@MariaHCD
Copy link
Contributor

👋🏼 @alexpensify, we'll need payment to go out to @samh-nl for this App PR.

cc: @kevinksullivan could you clarify what the payment amount is for this issue?

@alexpensify
Copy link
Contributor

Ok, I'll wait for @kevinksullivan to share the payment summary. Thanks!

@kevinksullivan
Copy link
Contributor Author

Let's pay $500

@alexpensify alexpensify added the External Added to denote the issue can be worked on by a contributor label Nov 1, 2023
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2023-10-30] [HOLD for payment 2023-10-27] [P2P Activation] [External] [CRITICAL] Update pending Wallet state to show Enable Wallet button [$500] [HOLD for payment 2023-10-30] [HOLD for payment 2023-10-27] [P2P Activation] [External] [CRITICAL] Update pending Wallet state to show Enable Wallet button Nov 1, 2023
Copy link

melvin-bot bot commented Nov 1, 2023

Job added to Upwork: https://www.upwork.com/jobs/~01d836dcba58f2381d

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 1, 2023
Copy link

melvin-bot bot commented Nov 1, 2023

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav (External)

@alexpensify alexpensify removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 1, 2023
@alexpensify
Copy link
Contributor

@mananjadhav please disregard this, I needed an Upwork link.

@alexpensify
Copy link
Contributor

alexpensify commented Nov 1, 2023

@samh-nl - can you please apply to this Upwork job, and I can continue the payment process:

https://www.upwork.com/jobs/~01d836dcba58f2381d

Thanks!

@samh-nl
Copy link
Contributor

samh-nl commented Nov 1, 2023

I've applied on Upwork. Thanks.

@alexpensify
Copy link
Contributor

Thanks, I sent the offer in Upwork.

@samh-nl
Copy link
Contributor

samh-nl commented Nov 2, 2023

Accepted!

@alexpensify
Copy link
Contributor

All set, I've completed the payment process in Upwork!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

8 participants