Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.67k stars 2.93k forks source link

[HOLD for payment 2024-11-07] [CVP] [$250] If you click "Pay with Expensify" and the payment fails (e.g., due to a bank account setup error), the option disappears, leaving only "Pay Elsewhere." #49576

Closed m-natarajan closed 3 weeks ago

m-natarajan commented 2 months 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: 9.0.39-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @ryanschaffer Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1726864011581859

Action Performed:

  1. Create new test account.
  2. Submit an expense to a new account that hasn’t been created yet
  3. Log in to account 2 and try to pay the expense submitted from account 1
  4. Mess up the bank flow
  5. Try to restart the payment flow and see that the only option is Pay Elsewhere

    Expected Result:

    If you try to restart the payment flow a 2nd time, it works the same it did the first time.

    Actual Result:

    The 2nd time, you don’t get the Pay with Expensify option and only the Pay Elsewhere option is available.

    Workaround:

    Unknown

    Platforms:

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

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

Screenshots/Videos

image

https://github.com/user-attachments/assets/05e88105-baa0-41ad-995e-04105b40df5d

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021838883742527613450
  • Upwork Job ID: 1838883742527613450
  • Last Price Increase: 2024-09-25
Issue OwnerCurrent Issue Owner: @joekaufmanexpensify
melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @dylanexpensify (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.

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

Ace0328 commented 2 months ago

To resolve this issue, you should ensure that the resendTransaction method is correctly defined and implemented in the appropriate file within your codebase, and that the import statement or class extension is set up correctly if it is inherited from a parent class or imported from a library. You should also ensure that there are no typos or errors in the method name, and that all required dependencies are correctly installed and referenced.

melvin-bot[bot] commented 2 months ago

📣 @Ace0328! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
Ace0328 commented 2 months ago

Email:davidhe791@gmail.com Upwork Profile link:https://www.upwork.com/freelancers/~017675b79a86fc30f6

Ace0328 commented 2 months ago

@.*** Upwork Profile link:https://www.upwork.com/freelancers/~017675b79a86fc30f6

Message ID: @.***>

shahinyan11 commented 2 months ago

@m-natarajan Hi. The issue does not reproducible with the steps described in Action Performed: section. There is not a button with the "Pay $xx with Expensify" label . Could you provide a video showing the entire flow of actions?

mananjadhav commented 2 months ago

Proposal

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

We're disabling the wallet options for the expense report. Hence we don't see "Pay with Expensify" for the workspace payments.

What is the root cause of that problem?

After clicking the "Pay with Expensify" option and selecting the "Business Bank Account", we move the payment to a new workspace and the payment type becomes CONST.REPORT.TYPE.EXPENSE. The "Pay with Expensify" option is dependent on the "canUseWallet" as shown in the code below.

https://github.com/Expensify/App/blob/43c2409e5e740cfd52f2b4a43b00957e3dbe25c2/src/components/SettlementButton/index.tsx#L98-L108

The flag canUseWallet is false isExpenseReport is true which is in our case.

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

const canUseWallet = !isInvoiceReport && currency === CONST.CURRENCY.USD;

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 2 months ago

@akinwale, @dylanexpensify Eep! 4 days overdue now. Issues have feelings too...

ryanschaffer commented 2 months ago

I just retested this. Another way to make the Pay with Expensify option disappear is to just exit out of the Plaid flow. I was testing this flow, realized I wasn't on staging so reloaded on staging and Pay Elsewhere was the only option

allroundexperts commented 2 months ago

@mananjadhav's proposal looks good to me. However, before proceeding, I'd like the assigned engineer to confirm that the user can use the wallet if its an expense report?

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 2 months ago

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

trjExpensify commented 2 months ago

Hm, I don't think the wallet has anything to do with expense reports, that's a P2P IOUReport feature.

I actually think the crux of the problem is that the workspace created has reimbursementChoice=manual on creation because a VBBA hasn't been added to the workspace successfully yet to set that to reimbursementChoice=yes for in-app payments on workspaces.

So we need to decide if we want to show the "Pay with Expensify" button in the dropdown for USD workspaces if reimbursementChoice=manual. If that option in the pay button is clicked, launch the connect bank account VBBA flow, and when successfully added it will update to reimbursementChoice=yes.

CC: @JmillsExpensify I think we should do this, but let me know what you think.

JmillsExpensify commented 2 months ago

Hm, I don't think the wallet has anything to do with expense reports, that's a P2P IOUReport feature.

Agreed

So we need to decide if we want to show the "Pay with Expensify" button in the dropdown for USD workspaces if reimbursementChoice=manual. If that option in the pay button is clicked, launch the connect bank account VBBA flow, and when successfully added it will update to reimbursementChoice=yes.

Also agree here. Let's do it.

neil-marcellini commented 2 months ago

Ok. @allroundexperts @mananjadhav would you please investigate Tom's theory for the root cause and come up with a solution that doesn't relate to changing the canUseWallet permissions? Maybe that's actually a poor variable name in the code? Please update your proposal and let us know.

We are still seeking new proposals.

mananjadhav commented 2 months ago

I don't have the context for this, but I'll look up and propose a solution.

dylanexpensify commented 2 months ago

TY @mananjadhav!

Ace0328 commented 2 months ago

Haha, thanks!

On Fri, Oct 4, 2024 at 1:55 AM Dylan Courtney @.***> wrote:

TY @mananjadhav https://github.com/mananjadhav!

— Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/49576#issuecomment-2393189368, or unsubscribe https://github.com/notifications/unsubscribe-auth/BKIVFGBPIV6MW2LJO3USDCTZZZJZNAVCNFSM6AAAAABOUTD4Y2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJTGE4DSMZWHA . You are receiving this because you were mentioned.Message ID: @.***>

melvin-bot[bot] commented 1 month ago

@neil-marcellini @allroundexperts @dylanexpensify this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

dylanexpensify commented 1 month ago

@mananjadhav any updates?

dylanexpensify commented 1 month ago

bump @mananjadhav

mananjadhav commented 1 month ago

Sorry I didn't get a chance to check this yet. I'll try to by tomorrow. I am not aware about the reimbursementChoice, so I'll need to check the code and workout a solution.

melvin-bot[bot] commented 1 month ago

@neil-marcellini, @allroundexperts, @dylanexpensify Eep! 4 days overdue now. Issues have feelings too...

mananjadhav commented 1 month ago

Okay I tried following @trjExpensify's comment here. I was unnecessarily complicating it thinking we need to update the reimbursementChoice when we start the bank flow.

So we need to decide if we want to show the "Pay with Expensify" button in the dropdown for USD workspaces if reimbursementChoice=manual.

If we're good with this, then the only change we need to make is this line

We should remove policy?.reimbursementChoice !== CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_MANUAL check and that should work. I'll write up a formal proposal.

https://github.com/user-attachments/assets/2ac7c1ae-29ee-409c-ae2d-4163617aaaa5

mananjadhav commented 1 month ago

Proposal

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

We're disabling the wallet options in the workspace chat. Hence we don't see "Pay with Expensify".

What is the root cause of that problem?

After clicking the "Pay with Expensify" option and selecting the "Business Bank Account", we move the payment to a new workspace. By default the reimbursementChoice is manual for the workspace. In the code below, Pay with Expensify option is hidden if the reimbursementChoice is manual..

https://github.com/Expensify/App/blob/5f5688f02cd53fec9d3749f341203b52f8de1a1c/src/components/SettlementButton/index.tsx#L72

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

So we need to decide if we want to show the "Pay with Expensify" button in the dropdown for USD workspaces if reimbursementChoice=manual.

Based on the previous comment here, then the only change we need to make is this line

We should remove policy?.reimbursementChoice !== CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_MANUAL check and that should work. I have attached the video in the previous comment.

What alternative solutions did you explore? (Optional)

NA

trjExpensify commented 1 month ago

When REIMBURSEMENT_NO is the reimbursementChoice for say a track user just categorising their receipts on a workspace, wouldn't we end up showing the Pay with Expensify button or is that logic elsewhere?

mananjadhav commented 1 month ago

tl;dr, REIMBURSEMENT_NO is taken care of by evaluating outside the component and passed in as props.

Explanation:

The settlement button payment options has two conditions. One about manual that I mentioned, second is the prop shouldHidePaymentOptions

https://github.com/Expensify/App/blob/7746ea31493d47aa779797e1df1e0e76d2d6ed0d/src/components/SettlementButton/index.tsx#L72

If we check the props being passed in ReportPreview, you'll see it's dependent on shouldShowPayButton.

https://github.com/Expensify/App/blob/7746ea31493d47aa779797e1df1e0e76d2d6ed0d/src/components/ReportActionItem/ReportPreview.tsx#L534

The shouldShowPayButton is then defined using canIOUBePaid where the check for REIMBURSEMENT_NO exists.

https://github.com/Expensify/App/blob/7746ea31493d47aa779797e1df1e0e76d2d6ed0d/src/libs/actions/IOU.ts#L6992-L6999

Ace0328 commented 1 month ago

Thanks, I'll check it out!

On Tue, Oct 15, 2024 at 9:54 AM Manan @.***> wrote:

tl;dr, REIMBURSEMENT_NO is taken care of by evaluating outside the component and passed in as props.

Explanation:

The settlement button payment options has two conditions. One about manual that I mentioned, second is the prop shouldHidePaymentOptions

https://github.com/Expensify/App/blob/7746ea31493d47aa779797e1df1e0e76d2d6ed0d/src/components/SettlementButton/index.tsx#L72

If we check the props being passed in ReportPreview, you'll see it's dependent on shouldShowPayButton.

https://github.com/Expensify/App/blob/7746ea31493d47aa779797e1df1e0e76d2d6ed0d/src/components/ReportActionItem/ReportPreview.tsx#L534

The shouldShowPayButton is then defined https://github.com/Expensify/App/blob/7746ea31493d47aa779797e1df1e0e76d2d6ed0d/src/components/ReportActionItem/ReportPreview.tsx#L324 using canIOUBePaid where the check for REIMBURSEMENT_NO exists.

https://github.com/Expensify/App/blob/7746ea31493d47aa779797e1df1e0e76d2d6ed0d/src/libs/actions/IOU.ts#L6992-L6999

— Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/49576#issuecomment-2414541393, or unsubscribe https://github.com/notifications/unsubscribe-auth/BKIVFGDV2GBMZULTRFS3NY3Z3VCFDAVCNFSM6AAAAABOUTD4Y2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMJUGU2DCMZZGM . You are receiving this because you were mentioned.Message ID: @.***>

trjExpensify commented 1 month ago

tl;dr, REIMBURSEMENT_NO is taken care of by evaluating outside the component and passed in as props.

Great then!

trjExpensify commented 1 month ago

@allroundexperts can you progress the review today? Let's get this fixed!

allroundexperts commented 1 month ago

Sure thing!

trjExpensify commented 1 month ago

How'd you get on?

allroundexperts commented 1 month ago

My day hasn't finished yet 😄. I'll review this shortly in about 2 hours.

allroundexperts commented 1 month ago

Okay. I managed to check @mananjadhav's proposal. Given we have an agreement that we should show the Pay with Expensify regardless if the reimbursementChoice is manual or not, @mananjadhav's proposal of removing the condition should work well. Let's go with there proposal.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 1 month ago

Current assignee @neil-marcellini is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @joekaufmanexpensify (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.

dylanexpensify commented 1 month ago

Hey @joekaufmanexpensify! I'm heading out on parental leave so reassigning this! TY!

joekaufmanexpensify commented 1 month ago

Sure, happy to help!

melvin-bot[bot] commented 1 month ago

@neil-marcellini @allroundexperts @joekaufmanexpensify this issue is now 4 weeks old, please consider:

Thanks!

joekaufmanexpensify commented 1 month ago

I think we're just waiting for @neil-marcellini to sign off on the selected proposal above

joekaufmanexpensify commented 1 month ago

Dm'ed Neil about proposal

neil-marcellini commented 1 month ago

Okay. I managed to check @mananjadhav's proposal. Given we have an agreement that we should show the Pay with Expensify regardless if the reimbursementChoice is manual or not, @mananjadhav's proposal of removing the condition should work well. Let's go with there proposal.

🎀 👀 🎀 C+ reviewed

Sounds good to me.

mananjadhav commented 1 month ago

Thanks, will raise the PR.

joekaufmanexpensify commented 1 month ago

Great. TY!

joekaufmanexpensify commented 1 month ago

Pending PR

mananjadhav commented 1 month ago

Will be pushed in 2-3 hours, just testing changes locally.

joekaufmanexpensify commented 1 month ago

Sounds great. TY!

joekaufmanexpensify commented 1 month ago

PR in review

melvin-bot[bot] commented 1 month ago

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