App icon indicating copy to clipboard operation
App copied to clipboard

[$250] Invoice - 0.00 displayed on Pay button for invoice receiver

Open IuliiaHerets opened this issue 1 year ago β€’ 80 comments

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 9.0.52-5 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5113365 Issue reported by: Applause Internal Team

Action Performed:

  1. Go to https://staging.new.expensify.com/ and log in
  2. Create a workspace if there is no any
  3. Navigate to the workspace settings > More features
  4. Enable the Invoice option
  5. Click on FAB > Send invoice
  6. Select an invoice receiver
  7. Enter the company info and send the invoice
  8. Log in with the invoice receiver account in incognito
  9. Navigate to the invoice room

Expected Result:

The correct amount is displayed on the Pay button

Actual Result:

0.00 is displayed on the Pay button

Workaround:

Unknown

Platforms:

  • [ ] Android: Standalone
  • [ ] Android: HybridApp
  • [ ] Android: mWeb Chrome
  • [ ] iOS: Standalone
  • [x] iOS: HybridApp
  • [x] iOS: mWeb Safari
  • [x] MacOS: Chrome / Safari
  • [ ] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/d265440b-b54a-44c7-b8c4-bc5581f0b9ba

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021849917912281837951
  • Upwork Job ID: 1849917912281837951
  • Last Price Increase: 2024-11-29
Issue OwnerCurrent Issue Owner: @MitchExpensify
Issue OwnerCurrent Issue Owner: @NikkiWines

IuliiaHerets avatar Oct 23 '24 08:10 IuliiaHerets

Triggered auto assignment to @MitchExpensify (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] avatar Oct 23 '24 08:10 melvin-bot[bot]

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

IuliiaHerets avatar Oct 23 '24 08:10 IuliiaHerets

@MitchExpensify 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

IuliiaHerets avatar Oct 23 '24 08:10 IuliiaHerets

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

Proposal

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

0.00 displayed on Pay button for invoice receiver

What is the root cause of that problem?

We get the settlement amount using getSettlementAmount: https://github.com/Expensify/App/blob/f251dbcdfdafc492d9a9908cbf40e4630b7b6d9d/src/components/ReportActionItem/ReportPreview.tsx#L237

We get the total reimbursableSpend to be displayed in the pay button form getMoneyRequestSpendBreakdown:

https://github.com/Expensify/App/blob/f251dbcdfdafc492d9a9908cbf40e4630b7b6d9d/src/components/ReportActionItem/ReportPreview.tsx#L133

We calculate reimbursableSpend using the formula: https://github.com/Expensify/App/blob/f251dbcdfdafc492d9a9908cbf40e4630b7b6d9d/src/libs/ReportUtils.ts#L2761

But for a invoice report nonReimbursableSpend is going to be equal to the totalDisplaySpend as for invoices we request the amount from the user and not ask for reimbursement. So the value goes to zero and that is what is displayed on the FE. Screenshot 2024-10-23 at 3 04 02β€―PM

This causes the 0 amount error

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

We should check if the current report is an invoice report and for that case should directly pass the totalDisplaySpend as that would be the requested invoice amount:

const reimbursableSpend = isInvoiceReport(report) ? totalDisplaySpend : totalDisplaySpend - nonReimbursableSpend;

We also need to update the below: https://github.com/Expensify/App/blob/b87ec1ffed2887cf53cfb3bf25a34f691313a9bd/src/libs/actions/IOU.ts#L6798

To:

    let total = (iouReport?.total ?? 0) - (isInvoiceReport ? 0 : iouReport?.nonReimbursableTotal ?? 0);

And then in MoneyReportView we calculate the formattedOutOfPocketAmount in the wrong way, it should be totalDisplaySpend - nonReimbursableSpend as that is what is owed for reimbursement.

https://github.com/Expensify/App/blob/cd3f30f73a18c29532d6886b4bb3744fd700ab9d/src/components/ReportActionItem/MoneyReportView.tsx#L62



    const formattedOutOfPocketAmount = CurrencyUtils.convertToDisplayString(totalDisplaySpend - nonReimbursableSpend, report.currency);

What alternative solutions did you explore? (Optional)

Or when we get settlement amount below: https://github.com/Expensify/App/blob/f251dbcdfdafc492d9a9908cbf40e4630b7b6d9d/src/components/ReportActionItem/ReportPreview.tsx#L535

We can check if the current report is invoice and if so then call getDisplayAmount() for that case

twilight2294 avatar Oct 23 '24 09:10 twilight2294

Edited by proposal-police: This proposal was edited at 2024-11-04 16:27:36 UTC.

Proposal

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

0.00 displayed on Pay button for invoice receiver

What is the root cause of that problem?

Since the nonReimbursableTotal for invoice reports is the same as it's total, the reimbursableSpend, which is calculated from the ReportPreview.getMoneyRequestSpendBreakdown function, will be 0.

https://github.com/Expensify/App/blob/5e975f5aedac0d150984282ada45eed1693bcdb6/src/libs/ReportUtils.ts#L2760-L2761

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

Let's create a condition in the ReportPreview and MoneyReportHeader components to display the total instead of the reimbursableSpend if the report is an invoice report.

https://github.com/Expensify/App/blob/5e975f5aedac0d150984282ada45eed1693bcdb6/src/components/ReportActionItem/ReportPreview.tsx#L246

const formattedAmount = CurrencyUtils.convertToDisplayString(
         ReportUtils.isInvoiceReport(moneyRequestReport) ? moneyRequestReport.total : reimbursableSpend, 
         iouReport?.currency
);

https://github.com/Expensify/App/blob/aedbbde0317b720151a75dbe0b6a7ddea52a20cd/src/components/MoneyReportHeader.tsx#L171

return CurrencyUtils.convertToDisplayString(
         isInvoiceRoom ? totalDisplaySpend : reimbursableSpend, 
         iouReport?.currency
);

Finally, let's change how we calculate total in the getPayMoneyRequestParams function, like below, to only use the value of iouReport?.total by excluding the value of iouReport?.nonReimbursableTotal in the calculation only when if the report is an invoice report.

https://github.com/Expensify/App/blob/b87ec1ffed2887cf53cfb3bf25a34f691313a9bd/src/libs/actions/IOU.ts#L6798

let total = (iouReport?.total ?? 0) - (!isInvoiceReport && (iouReport?.nonReimbursableTotal ?? 0));

Tony-MK avatar Oct 23 '24 22:10 Tony-MK

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

melvin-bot[bot] avatar Oct 25 '24 20:10 melvin-bot[bot]

Lets fix!

MitchExpensify avatar Oct 25 '24 20:10 MitchExpensify

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

melvin-bot[bot] avatar Oct 25 '24 20:10 melvin-bot[bot]

Edited by proposal-police: This proposal was edited at 2024-11-05 11:05:55 UTC.

Proposal

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

Invoice - 0.00 displayed on Pay button for invoice receiver

What is the root cause of that problem?

reimbursable spend is calculated by subtracting non-reimbursable from the total https://github.com/Expensify/App/blob/a86ee35a6c9706d8acb35202acb4cdaa1cc769e7/src/libs/ReportUtils.ts#L2776 but for invoice nonReimbursableSpend is equal to the total

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

We should add an exception for invoices and set nonReimbursableSpend 0, so:

nonReimbursableSpend = isInvoiceReport(report) ? 0 : nonReimbursableSpend;
            const reimbursableSpend = totalDisplaySpend - nonReimbursableSpend;

then also here https://github.com/Expensify/App/blob/a86ee35a6c9706d8acb35202acb4cdaa1cc769e7/src/libs/actions/IOU.ts#L6784

    let total = (iouReport?.total ?? 0) - (isInvoiceReport ? 0 : iouReport?.nonReimbursableTotal ?? 0);

What alternative solutions did you explore? (Optional)

If we don't want to update the nonReimbursableSpend We can update only the reimbursableSpend

            const reimbursableSpend = totalDisplaySpend - isInvoiceReport(report) ? 0 : nonReimbursableSpend;

and we disable showing break down in money report view if it is invoice report because for invoices the total amount is all reimbursable https://github.com/Expensify/App/blob/cd3f30f73a18c29532d6886b4bb3744fd700ab9d/src/components/ReportActionItem/MoneyReportView.tsx#L60

    const shouldShowBreakdown = !ReportUtils.isInvoiceReport(report) && nonReimbursableSpend && reimbursableSpend && shouldShowTotal;

we can also use nonReimbursableSpend!==totalDisplaySpend condition instead

FitseTLT avatar Oct 25 '24 22:10 FitseTLT

Reviewing...

dukenv0307 avatar Oct 28 '24 14:10 dukenv0307

@FitseTLT @twilight2294 Do you think it's BE bug since it returns nonReimbursableTotal same as total

dukenv0307 avatar Oct 29 '24 05:10 dukenv0307

No, Non-reimbursable expenses are costs that an organization or employer will not compensate for.

For invoices we ask the end user to pay the amount and hence it i same as total, @dukenv0307 my proposal here solves the bug correctly

twilight2294 avatar Oct 29 '24 06:10 twilight2294

@twilight2294 If so, we still need to fix the total here like @FitseTLT's suggestion.

Here's the result when I apply your solution

Screenshot 2024-10-29 at 15 40 38

dukenv0307 avatar Oct 29 '24 08:10 dukenv0307

@dukenv0307 , @FitseTLT ' RCA and solution is essential the exact same as my proposal with one extra case. I feel I was the first one to propose the changes required to fix the originally reported bug so i should be assigned to this bug. Our contributor guidelines also state:

Before submitting a proposal on an issue, be sure to read any other existing proposals. ALL NEW PROPOSALS MUST BE DIFFERENT FROM EXISTING PROPOSALS.

If at all that extra case is important, i can fix that in my PR and can share say ~$25 out of my compensation as the solution is same and it's just a minor change during PR phase.

And also as @FitseTLT suggests to change the nonReimbursableSpend, nonReimbursableSpend shouldn't be changed here, as it is a non-zero value in the Backend so essentially he is saying that BE is giving a wrong value than frontend. so his solution is a workaround (hacky solution) than a clean solution depending on the type of report. My solution deals with the way we get the total amount for the spend, but @FitseTLT proposal suggests to change the nonReimbursableSpend altogether which is in contrast with the BE value.

twilight2294 avatar Oct 29 '24 13:10 twilight2294

@dukenv0307 I am surprised how you are taking this matter into a discussion because the missed point is important and will obviously create a regression and also you shouldn't forget the suggested changes were

const reimbursableSpend = isInvoiceReport(report) ? totalDisplaySpend : totalDisplaySpend - nonReimbursableSpend;

which will only fix reimbursableSpend but doesn't fix the value for nonReimbursableSpend here https://github.com/Expensify/App/blob/f251dbcdfdafc492d9a9908cbf40e4630b7b6d9d/src/libs/ReportUtils.ts#L2764 that would cause the out-of-pocket amount to be displayed in money report image

FitseTLT avatar Oct 29 '24 14:10 FitseTLT

Thanks for all your proposal. You guys provide the correct RCA.

I'm not sure the nonReimbursableSpend returned from BE is correct or not, if we don't use it anywhere, it should be 0.

In case it's correct, I like @FitseTLT's proposal since it does not only fix the displayed amount in pay button, but also the amount in action (after pay).

πŸŽ€πŸ‘€πŸŽ€ C+ reviewed

dukenv0307 avatar Oct 30 '24 03:10 dukenv0307

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

melvin-bot[bot] avatar Oct 30 '24 03:10 melvin-bot[bot]

@NikkiWines I don't think @FitseTLT 's solution is correct:

@FitseTLT suggests to change the nonReimbursableSpend, nonReimbursableSpend shouldn't be changed here, as it is a non-zero value in the Backend so essentially he is saying that BE is giving a wrong value than frontend. so his solution is a workaround (hacky solution) than a clean solution depending on the type of report. My solution deals with the way we get the total amount for the spend, but @FitseTLT proposal suggests to change the nonReimbursableSpend altogether which is in contrast with the BE value.

twilight2294 avatar Oct 30 '24 03:10 twilight2294

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

melvin-bot[bot] avatar Nov 01 '24 16:11 melvin-bot[bot]

Sorry for the delay. I'm not sure either solution is entirely correct to be honest.

@twilight2294 you're correct in your statement here:

For invoices we ask the end user to pay the amount and hence it i same as total

But your solution doesn't fully resolve the issue, since (as @FitseTLT outlined) there are a couple of other cases where we end up having a display mismatches due to the same type of totalDisplaySpend - nonReimbursableSpend calculation.

It's true that this issue was opened for this particular flow, but we prefer more comprehensive solutions that solve the problem in its entirety, if there's the same bug in multiple flows/areas.

@FitseTLT, I don't think we want to modify nonReimbursableSpend here - @twilight2294 is right that value correct, it's the logic used to determine totalDisplaySpend (or total) that's incorrect (in multiple locations)

Ideally, I think we'd add a case or adjustment in all the relevant areas to adjust the totalSpend/total calculation, based on whether or not the report is an invoice - instead of adjusting the value for reimbursableSpend/ nonReimbursableSpend themselves. Hope this makes sense!

NikkiWines avatar Nov 01 '24 20:11 NikkiWines

I agree with you @NikkiWines !, how should we move forward here ?

twilight2294 avatar Nov 01 '24 21:11 twilight2294

@FitseTLT, I don't think we want to modify nonReimbursableSpend here - @twilight2294 is right that value correct, it's the logic used to determine totalDisplaySpend (or total) that's incorrect (in multiple locations)

@NikkiWines @twilight2294 is not correct. If the value of nonReimbursableSpend is not going to be fixed then the money report view will show out of pocket(non reimbursable) and Company Spend(reimbursable) value equal to the total which is wrong and confusing and that should be fixed too. image

FitseTLT avatar Nov 01 '24 21:11 FitseTLT

Proposal

Updated

Ideally, I think we'd add a case or adjustment in all the relevant areas to adjust the totalSpend/total calculation, based on whether or not the report is an invoice - instead of adjusting the value for reimbursableSpend/ nonReimbursableSpend themselves. Hope this makes sense!

According this comment, the solution for my proposal already did not adjust or modify the value for reimbursableSpend/ nonReimbursableSpend in the getMoneyRequestSpendBreakdown function.

Tony-MK avatar Nov 04 '24 16:11 Tony-MK

Not overdue, we're discussing the solution and the ball seems to be in @NikkiWines 's court right now

MitchExpensify avatar Nov 04 '24 17:11 MitchExpensify

@NikkiWines, @MitchExpensify, @dukenv0307 Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] avatar Nov 04 '24 18:11 melvin-bot[bot]

What are your thoughts on next steps here @dukenv0307 ?

MitchExpensify avatar Nov 04 '24 23:11 MitchExpensify

I agree with you @NikkiWines !, how should we move forward here ?

@twilight2294 The best thing to do would be to update your proposal, as @Tony-MK has done, to account for a new approach.

@NikkiWines @twilight2294 is not correct. If the value of nonReimbursableSpend is not going to be fixed then the money report view will show out of pocket(non reimbursable) and Company Spend(reimbursable) value equal to the total which is wrong and confusing and that should be fixed too.

@FitseTLT I think there may be a misunderstanding. I agree that we need to update the view to show the correct value for out of pocket spend when the report is an invoice - but I think the correct way to do that is by accounting for whether or not the report is an invoice when we're determining the amount to display in the component itself - not by changing the nonReimbursableSpend value that we're returning from the backend. Hopefully that makes sense?

According this https://github.com/Expensify/App/issues/51312#issuecomment-2452582144, the solution for my https://github.com/Expensify/App/issues/51312#issuecomment-2433575175 already did not adjust or modify the value for reimbursableSpend/ nonReimbursableSpend in the getMoneyRequestSpendBreakdown function.

@dukenv0307, can you review @Tony-MK's updated proposal, please πŸ™‡ However, @Tony-MK, I don't think we'd want to update convertToDisplayString to do anything over than convert the inputted value to a formatted string, as that muddies the purpose of the function.

NikkiWines avatar Nov 05 '24 02:11 NikkiWines

@NikkiWines I'm checking the new proposal, will give the updates soon

dukenv0307 avatar Nov 05 '24 02:11 dukenv0307

Updated proposal

Updated proposal to cover extra case as per this comment

twilight2294 avatar Nov 05 '24 08:11 twilight2294

@dukenv0307, can you review @Tony-MK's updated proposal, please πŸ™‡ However, @Tony-MK, I don't think we'd want to update convertToDisplayString to do anything over than convert the inputted value to a formatted string, as that muddies the purpose of the function.

I believe totalDisplaySpend is already a number but there are already several functions where totalDisplaySpend is used with convertToDisplayString like in my proposal such as getReportAutomaticallyForwardedMessage, getIOUForwardedMessage and getIOUReportActionMessage.

Tony-MK avatar Nov 05 '24 08:11 Tony-MK