[$250] Invoice - Receiver can send and pay their own invoice when receiver is also workspace admin

IuliiaHerets 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: v9.0.18-7 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): applausetester+kh050806@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

Go to staging.new.expensify.com
[User A] Invite User B to workspace and promote User B to admin.
[User A] Send an invoice to User B from the same workspace in Step 2.
[User B] Go to invoice room.
[User B] Click + > Send invoice.
[User B] Send an invoice in the same workspace chat as invoice receiver.
[User B] Note that User B can send and pay their own invoice. Error will show up if User B pays their own invoice as business for the second time.

Expected Result:

User B (invoice receiver and also admin of the workspace) should not be able to pay the invoice that User B sends.

Actual Result:

User B (invoice receiver and also admin of the workspace) is able to pay the invoice that User B sends. When User B pays their own invoice as business for the second time, error shows up.

Workaround:

Unknown

Platforms:

[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/user-attachments/assets/58fd96d7-77ff-4297-9b9f-15a6758806a0

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01267eba2b7c7cce57
Upwork Job ID: 1825637731328105082
Last Price Increase: 2024-11-04

Issue Owner

Current Issue Owner: @shubham1206agra

melvin-bot[bot] commented 2 months ago

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

IuliiaHerets commented 2 months ago

We think that this bug might be related to #vip-bills

IuliiaHerets commented 2 months ago

@OfstadC FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

MuaazArshad commented 2 months ago

Edited by proposal-police: This proposal was edited at 2024-09-03 16:34:05 UTC.

Proposal

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

Receiver can send and pay their own invoice when receiver is also workspace admin.

What is the root cause of that problem?

We are not checking if the user is the sender or not. https://github.com/Expensify/App/blob/e6546d658b17a8dc94aee6e8ceba1eb988397053/src/components/MoneyReportHeader.tsx#L112

https://github.com/Expensify/App/blob/e6546d658b17a8dc94aee6e8ceba1eb988397053/src/components/ReportActionItem/ReportPreview.tsx#L300

https://github.com/Expensify/App/blob/e6546d658b17a8dc94aee6e8ceba1eb988397053/src/libs/actions/IOU.ts#L6796-L6841

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

Checking for the user is the sender can help resolve this issue. we can add

 const isCurrentUserOwner = currentUserPersonalDetails?.accountID === iouReport.ownerAccountID;
    if (isCurrentUserOwner) {
        return false;
    }

in https://github.com/Expensify/App/blob/e6546d658b17a8dc94aee6e8ceba1eb988397053/src/libs/actions/IOU.ts#L6796-L6841 this line is already being used in canApproveIOU and submitExpense in the same file.

What alternative solutions did you explore? (Optional)

we can also solve this by adding this check in individual files

const shouldShowPayButton = useMemo(() => IOU.canIOUBePaid(iouReport, chatReport, policy, allTransactions) && ownerAccountD!==getCurrentUserAccountID(), [iouReport, chatReport, policy, allTransactions]);

in https://github.com/Expensify/App/blob/e6546d658b17a8dc94aee6e8ceba1eb988397053/src/components/ReportActionItem/ReportPreview.tsx#L300 and

const shouldShowPayButton = useMemo(
        () => IOU.canIOUBePaid(moneyRequestReport, chatReport, policy, transaction ? [transaction] : undefined) && moneyRequestReport.ownerAccountID !== getCurrentUserAccountID(),
        [moneyRequestReport, chatReport, policy, transaction],
    );

in https://github.com/Expensify/App/blob/e6546d658b17a8dc94aee6e8ceba1eb988397053/src/components/MoneyReportHeader.tsx#L112

melvin-bot[bot] commented 2 months ago

@OfstadC Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

OfstadC commented 2 months ago

Reviewing now

OfstadC commented 2 months ago

Sorry - I haven't been able to set this up to test it - will figure this out by EOD

OfstadC commented 2 months ago

I can confirm that User B has the option to pay the invoice they've sent. But also, i'm not sure why that's an issue if it's sent to the workspace and they are an admin on the workspace. Going to dig more

OfstadC commented 2 months ago

This is expected behavior. If the submitter is an admin, they'll have the option to pay.

OfstadC commented 2 months ago

We're actually going to fix this

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

OfstadC commented 2 months ago

Looks like we have one proposal so far for review 😃

daledah commented 2 months ago

Edited by proposal-police: This proposal was edited at 2024-08-22 11:13:36 UTC.

Proposal

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

User B (invoice receiver and also admin of the workspace) is able to pay the invoice that User B sends. When User B pays their own invoice as business for the second time, error shows up.

What is the root cause of that problem?

Here is the logic to display pay button in invoice: https://github.com/Expensify/App/blob/e6546d658b17a8dc94aee6e8ceba1eb988397053/src/libs/actions/IOU.ts#L6814-L6822
Assume A is the policy owner, B is the policy's admin.
First time, when A sends an invoice to B, then B pays as an "Bussiness". The chatReport?.invoiceReceiver?.type will be policy.
In the second time, when B send an invoices to A. The pay button displays in B's side because now, PolicyUtils.getPolicy(chatReport?.invoiceReceiver?.policyID)?.role === CONST.POLICY.ROLE.ADMIN is true.

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

In here: https://github.com/Expensify/App/blob/e6546d658b17a8dc94aee6e8ceba1eb988397053/src/libs/actions/IOU.ts#L6815-L6817 should be:

    if (iouSettled) {
        return false;
    }
    const managerID = iouReport.managerID;
    if (managerID !== userAccountID) {
        return false;
    }

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 2 months ago

@OfstadC @shubham1206agra 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!

melvin-bot[bot] commented 2 months ago

@OfstadC, @shubham1206agra Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 2 months ago

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

OfstadC commented 2 months ago

@shubham1206agra Can you review the listed proposals? Thank you!

OfstadC commented 2 months ago

@shubham1206agra if you can update by EOD please 🙏 - Thank you 😃

melvin-bot[bot] commented 2 months ago

@OfstadC, @shubham1206agra 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

shubham1206agra commented 2 months ago

@OfstadC I was finding a way to test this. Since I am blocked by subscription flow.

shubham1206agra commented 2 months ago

@OfstadC Can you ask QA to re-record this since there are so many crashes present right now in the recording?

OfstadC commented 2 months ago

Asked here 😃

kavimuru commented 2 months ago

There are 2 bugs here. Bug 1: User B (invoice receiver and also admin of the workspace) is able to pay the invoice that User B sends. Bug 2: When User B pays their own invoice as business for the second time, error shows up. Now only can repro Bug 1

https://github.com/user-attachments/assets/f1e0fe86-aebd-43c2-9440-811f323af229

shubham1206agra commented 2 months ago

@OfstadC Can you please recheck if this still needs to be fixed? Just to be sure.

melvin-bot[bot] commented 2 months ago

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

OfstadC commented 2 months ago

Yes @shubham1206agra we don't want the invoice sender to be able to pay their own invoice 😃

shubham1206agra commented 2 months ago

@daledah Can you recheck your solution? I don't think it is correct / detailed enough.

MuaazArshad commented 2 months ago

@shubham1206agra We also have my proposal https://github.com/Expensify/App/issues/47174#issuecomment-2281454348 .

shubham1206agra commented 2 months ago

@MuaazArshad Please add more details to the solution. Thanks

daledah commented 2 months ago

Can you recheck your solution? I don't think it is correct

In my solution, I'm using the managerID to determine if the current user is the receiver. If they are not (managerID !== currentUserID), the pay button is not displayed. This approach aligns with the requirement:

we don't want the invoice sender to be able to pay their own invoice

Which part of this do you think is incorrect?

MuaazArshad commented 2 months ago

Proposal Updated

Added Details
Added both possible solutions

MuaazArshad commented 2 months ago

@daledah with due respect your condition

if (managerID !== userAccountID) {
            return false;
        }

is quite wrong. if the current user is the manager, he will not be able to pay invoices sent by the workspace owner. We have to check if the current user is the owner of current report Action. The function should return false when the user is the owner not the manager.ownerAccountID === userAccountID

MuaazArshad commented 2 months ago

@shubham1206agra bump!

melvin-bot[bot] commented 1 month ago

@OfstadC @shubham1206agra this issue is now 4 weeks old, please consider:

Finding a contributor to fix the bug
Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

melvin-bot[bot] commented 1 month ago

@OfstadC, @shubham1206agra Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

OfstadC commented 1 month ago

@shubham1206agra Could you update by EOD? Thank you! 😃

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? 💸

OfstadC commented 1 month ago

Bump @shubham1206agra 😃

shubham1206agra commented 1 month ago

Sorry, but both proposals are wrong. I will bump on Slack for more proposals.

MuaazArshad commented 1 month ago

Sorry, but both proposals are wrong. I will bump on Slack for more proposals.

My proposal works perfectly. It allows the admin to pay the invoice and prevents users from paying their own invoice.

MuaazArshad commented 1 month ago

@shubham1206agra bump!

shubham1206agra commented 1 month ago

I am unable to create invoices now. Asked for help here https://expensify.slack.com/archives/C05LX9D6E07/p1726161347211499

MuaazArshad commented 1 month ago

@shubham1206agra bump!

shubham1206agra commented 1 month ago

@trjExpensify Can you tell me if we should create 2 separate Invoice rooms in this case?

Cause this line locks the payer for the entire room

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? 💸

OfstadC commented 1 month ago

CC @davidcardoza incase you can assist with this question - https://github.com/Expensify/App/issues/47174#issuecomment-2352859330

melvin-bot[bot] commented 1 month ago

@OfstadC, @shubham1206agra Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

shubham1206agra commented 1 month ago

@trjExpensify @davidcardoza Bump 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? 💸

Expensify / App