[HOLD for payment 2024-04-25] [$500] Send money - Missing bank account option when paying with Expensify

izarutskaya commented 1 month ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: 1.4.55-0 Reproducible in staging?: Y Reproducible in production?: Y Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team

Action Performed:

Go to staging.new.expensify.com.
Go to 1:1 DM
Click + > Send money
Select USD currency and enter an amount > Click Next
Click Pay with Expensify.

Expected Result:

Bank account option will be present in Pay with Expensify menu.

Actual Result:

Bank account option is not present in Pay with Expensify menu.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

https://github.com/Expensify/App/assets/115492554/dbff58e5-666f-4c5d-ae2f-6349ed714699

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0113f6d0636b87cae3
Upwork Job ID: 1772440160316952576
Last Price Increase: 2024-04-02
Automatic offers:

jjcoffee | Reviewer | 0
bernhardoj | Contributor | 0

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @stephanieelliott (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

izarutskaya commented 1 month ago

We think this issue might be related to the #vip-vsb.

godofoutcasts94 commented 1 month ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Send money - Missing bank account option when paying with Expensify

What is the root cause of that problem?

Here : https://github.com/Expensify/App/blob/51ef6c1bd1cd3d981db744bff1fcff9e7c670c0c/src/components/AddPaymentMethodMenu.js#L69-L73 we need to add Send Report Request as well and here as well https://github.com/Expensify/App/blob/ee5ab95f2c893738fb48756a9002e9d9dadd9a31/src/components/AddPaymentMethodMenu.tsx#L72

What changes do you think we should make in order to solve the problem?

Here : https://github.com/Expensify/App/blob/51ef6c1bd1cd3d981db744bff1fcff9e7c670c0c/src/components/AddPaymentMethodMenu.js#L69-L73 we can add this ReportUtils.isMoneyRequest in one of the OR condition like this :

   const canUseBusinessBankAccount =
        ReportUtils.isExpenseReport(iouReport) ||
        ReportUtils.isMoneyRequest ||
        (ReportUtils.isIOUReport(iouReport) && !ReportActionsUtils.hasRequestFromCurrentAccount(lodashGet(iouReport, 'reportID', 0), lodashGet(session, 'accountID', 0)));

we need to add this - ReportUtils.isMoneyRequest here as well https://github.com/Expensify/App/blob/51ef6c1bd1cd3d981db744bff1fcff9e7c670c0c/src/components/AddPaymentMethodMenu.js#L69-L73 so, that we can Personal Bank Account option as well

What alternative solutions did you explore? (Optional)

Alternatively, like the way we have shouldShowPersonalBankAccountOption here and all the other places similar to what we have done for shouldShowPersonalBankAccountOption, we need to add a type like that for Business account as well, something like shouldShowBusinessAccountOption and pass it to settlement button.

Result

Working Video

https://github.com/Expensify/App/assets/158435970/2a1a5d4d-6870-47c5-bea5-a7e489274d70

godofoutcasts94 commented 1 month ago

Updated Proposal

bernhardoj commented 1 month ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

The bank account option isn't available when sending money to a user.

What is the root cause of that problem?

The bank account page will show if it's an expense or an IOU report. https://github.com/Expensify/App/blob/82065804528dc66daaa6ba652ab2dea84684c619/src/components/AddPaymentMethodMenu.js#L71-L75

However, on the money request confirmation page, the expense/IOU report doesn't exist. Each send money will create a new IOU report. Also, in our case, we can only send money to a user and not a workspace, so the expected result would be to show the personal bank account option.

What changes do you think we should make in order to solve the problem?

We need to pass shouldShowPersonalBankAccountOption to the confirmation page settlement button. https://github.com/Expensify/App/blob/82065804528dc66daaa6ba652ab2dea84684c619/src/components/MoneyTemporaryForRefactorRequestConfirmationList.js#L622-L624

The props is passed down deeper to the AddPaymentMethodMenu component.

this is what we do in here too

GandalfGwaihir commented 1 month ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Missing bank account option when paying with Expensify

What is the root cause of that problem?

we use AddPaymentMethodMenu to display payment options in any report. https://github.com/Expensify/App/blob/51ef6c1bd1cd3d981db744bff1fcff9e7c670c0c/src/components/AddPaymentMethodMenu.js#L69-L76

Here a prop shouldShowPersonalBankAccountOption is passed to the AddPaymentMethodMenu to show personal bank account.

Then in MoneyTemporaryForRefactorRequestConfirmationList we pass down props to the settlement button:

https://github.com/Expensify/App/blob/82065804528dc66daaa6ba652ab2dea84684c619/src/components/MoneyTemporaryForRefactorRequestConfirmationList.js#L622-L624

Here as this is shared among many iou type, we don't explicitly pass shouldShowPersonalBankAccountOption as we don't want to show it on workspaces.

Also, we dont have the condition to show the business account for send requests.

What changes do you think we should make in order to solve the problem?

we need to pass shouldShowPersonalBankAccountOption:

So the updated settlement button code would be:

 const button = shouldShowSettlementButton ? ( 
     <SettlementButton 
+       shouldShowPersonalBankAccountOption
         pressOnEnter

Then we also need to show the business button for send 1:1 requests as well. for that we need to update the condition to:

First we need to define a new prop isSendRequest in AddPaymentMethodMenu and then update the canUseBusinessBankAccount to :

const canUseBusinessBankAccount =
    isSendRequest || ReportUtils.isExpenseReport(iouReport ?? {}) || (isIOUReport && !ReportActionsUtils.hasRequestFromCurrentAccount(iouReport?.reportID ?? '', session?.accountID ?? 0));

This would be a optional prop with default value as false to avoid regression at other places. we need to pass this prop in MoneyTemporaryForRefactorRequestConfirmationList as well as MoneyRequestConfirmationList

This prop will be true for iouType === CONST.IOU.TYPE.SEND

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 1 month ago

Job added to Upwork: https://www.upwork.com/jobs/~0113f6d0636b87cae3

melvin-bot[bot] commented 1 month ago

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

stephanieelliott commented 1 month ago

Hey @jjcoffee, we have a few proposals for review when you get a chance!

jjcoffee commented 1 month ago

@godofoutcasts94's proposal is unclear and has an incomplete solution, which doesn't appear to work. I'd recommend working on expanding your proposals in future to make it clearer both for yourself and for the reviewers.

The other two are pretty similar proposals, but @bernhardoj's proposal is the first to get the correct RCA and solution. We don't need to check for isTypeSend as @GandalfGwaihir suggests, as we already do that in shouldShowSettlementButton.

https://github.com/Expensify/App/blob/82065804528dc66daaa6ba652ab2dea84684c619/src/components/MoneyTemporaryForRefactorRequestConfirmationList.js#L619

:ribbon::eyes::ribbon: C+ reviewed

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @arosiclair, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

godofoutcasts94 commented 1 month ago

Hey @jjcoffee! Updated Proposal with Working Video

jjcoffee commented 1 month ago

@godofoutcasts94 The expected result here is to show the personal bank account option, as the other two proposals implement. You also have ReportUtils.isMoneyRequest in your solution without passing anything to it, which is why I recommend digging into the details for your future proposals. Best of luck!

godofoutcasts94 commented 1 month ago

@godofoutcasts94 The expected result here is to show the personal bank account option, as the other two proposals implement. You also have ReportUtils.isMoneyRequest in your solution without passing anything to it, which is why I recommend digging into the details for your future proposals. Best of luck!

but in the issue description, there is no mention about Personal or Business Bank Account Option. Its just saying bank account option

arosiclair commented 1 month ago

@bernhardoj's propsal LGTM though I'm not sure if bank accounts are actually supposed to be an option for the Send Money flow currently. I'm asking internally here.

melvin-bot[bot] commented 1 month ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

arosiclair commented 1 month ago

Alright just concluded that conversation. We should list both Personal and Business bank accounts as options when using Pay with Expensify in the send money flow. Any of those can be linked to your wallet for payment.

That means both @bernhardoj and @godofoutcasts94's proposals are actually incomplete. Can either of you update your proposals and can you re-review @jjcoffee? I think we can take whichever is updated and acceptable first.

godofoutcasts94 commented 1 month ago

updated proposal

cc - @arosiclair

godofoutcasts94 commented 1 month ago

Updated proposal with alternative solution as well.

bernhardoj commented 1 month ago

@arosiclair wouldn't it be redundant to show both business and personal bank account? Both options will navigate the user to add a personal bank account.

arosiclair commented 1 month ago

@arosiclair wouldn't it be redundant to show both business and personal bank account? Both options will navigate the user to add a personal bank account.

I don't think this is true. The business bank account option should create a workspace and start the VBBA flow here.

GandalfGwaihir commented 1 month ago

Updated proposal

Updated proposal as per updated discussion.

bernhardoj commented 1 month ago

@arosiclair it will create a workspace and start the VBBA if we do it on an existing iou report, but when sending money, we are always creating a new iou report, and it's not possible to send money to workspace. So, it will navigate to addBankAccountRoute instead. https://github.com/Expensify/App/blob/7177173f4b686a32bfcb94f07f0541b2f865f31b/src/components/KYCWall/BaseKYCWall.tsx#L137

And addBankAccountRoute will be the add personal bank account route. https://github.com/Expensify/App/blob/6d06a30801b063ff4b7dddb8e77b8196d8761b6e/src/libs/ReportUtils.ts#L890-L892

arosiclair commented 4 weeks ago

@arosiclair it will create a workspace and start the VBBA if we do it on an existing iou report, but when sending money, we are always creating a new iou report, and it's not possible to send money to workspace. So, it will navigate to addBankAccountRoute instead.

Hmm that sounds like another bug then. I would expect a workspace to be created in both cases.

Alright, let's just focus on just the personal bank account in this issue and then I'll open a follow up to give special attention to the send money flow using a business bank account. Since @bernhardoj had the best proposal for the personal bank account, I'll hire them.

melvin-bot[bot] commented 4 weeks ago

📣 @jjcoffee 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 4 weeks ago

📣 @bernhardoj 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

bernhardoj commented 4 weeks ago

PR is ready

cc: @jjcoffee

stephanieelliott commented 3 weeks ago

PR has been reviewed, currently undergoing QA

stephanieelliott commented 2 weeks ago

PR is on staging

melvin-bot[bot] commented 2 weeks ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 2 weeks ago

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

https://github.com/Expensify/App/pull/39619

If no regressions arise, payment will be issued on 2024-04-25. :confetti_ball:

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

@jjcoffee requires payment automatic offer (Reviewer)
@bernhardoj requires payment automatic offer (Contributor)

melvin-bot[bot] commented 2 weeks ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[ ] [@jjcoffee] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@jjcoffee] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
[ ] [@jjcoffee] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
[ ] [@jjcoffee] Determine if we should create a regression test for this bug.
[ ] [@jjcoffee] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[ ] [@stephanieelliott] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

melvin-bot[bot] commented 1 week ago

Triggered auto assignment to @zanyrenney (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

stephanieelliott commented 1 week ago

Reapplying the Bug label to get another BZ member on this while I am OOO til May 2. Thanks @zanyrenney, only thing to do here is issue payment on 4/25!

zanyrenney commented 1 week ago

payment summary

@jjcoffee PAID $500 ON UW automatic offer (Reviewer) @bernhardoj PAID $500 ON UW automatic offer (Contributor)

Expensify / App