[$250] Invoices - Invoice receiver is unable to see Category, Tag and Tax fields in invoice report

IuliiaHerets commented 3 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.21-1 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: Exp https://expensify.testrail.io/index.php?/tests/view/4864174 Email or phone of affected tester (no customers): applausetester+kh050806@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

Precondition:

Workspace has Categories, Tags and Taxes.

Go to staging.new.expensify.com
Go to FAB > Send invoice.
Enter amount. select user and send invoice.
Click on the invoice preview to go to transaction thread.
Enter data for Category, Tag and Tax.
As invoice receiver, go to invoice report.

Expected Result:

Invoice receiver should be able to see Category, Tag and Tax in the transaction thread.

Actual Result:

Invoice receiver is unable to see Category, Tag and Tax in the transaction thread. Only the system message for those fields are present for them.

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/7e2b75fc-622c-48db-aee0-3791eb58b144

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01e7476f563c3f9669
Upwork Job ID: 1826516629658939965
Last Price Increase: 2024-09-26

Issue Owner

Current Issue Owner: @johnmlee101

melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @RachCHopkins (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 3 months ago

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

IuliiaHerets commented 3 months ago

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

daledah commented 3 months ago

Edited by proposal-police: This proposal was edited at 2024-08-22 07:38:59 UTC.

Proposal

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

Invoice receiver is unable to see Category, Tag and Tax in the transaction thread. Only the system message for those fields are present for them.

What is the root cause of that problem?

In case of invoice's receiver, the don't display category because in: https://github.com/Expensify/App/blob/76158981580aa046592df17c940fb6b289b0924d/src/components/ReportActionItem/MoneyRequestView.tsx#L217 isPolicyExpenseChat is false.

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

Display tax in receiver side
- We can update: https://github.com/Expensify/App/blob/76158981580aa046592df17c940fb6b289b0924d/src/components/ReportActionItem/MoneyRequestView.tsx#L630
```
        {(shouldShowTax || (isInvoice && formattedTaxAmount)) && (
```
  to display the tax amount from receiver side.

To display the tax rate, BE needs to return an additional prop in the transaction, which allows us to get the tax rate value directly. I assume that prop is taxRate. So we can update: https://github.com/Expensify/App/blob/76158981580aa046592df17c940fb6b289b0924d/src/components/ReportActionItem/MoneyRequestView.tsx#L614
```
            {(shouldShowTax || (isInvoice && transaction.taxRate)) && (
```
and https://github.com/Expensify/App/blob/76158981580aa046592df17c940fb6b289b0924d/src/components/ReportActionItem/MoneyRequestView.tsx#L617
```
                        title={taxRateTitle ?? (isInvoice ? transaction.taxRate : '')}
```

The category/tag should not be shown on the recipient end, nor should any comment/history pertaining to the coding:
- Option 1: This should be done in BE side.

Option 2: We can modify this function: https://github.com/Expensify/App/blob/76158981580aa046592df17c940fb6b289b0924d/src/libs/ReportActionsUtils.ts#L600 so that it will early return false if the action belongs to invoice report, current user is report's manager and the action name is MODIFIEDEXPENSE.

melvin-bot[bot] commented 2 months ago

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

RachCHopkins commented 2 months ago

Oh mine is much more broken than this.

But despite it's terrible dupe display thing, I don't have the OP's problem.

RachCHopkins commented 2 months ago

@daledah I don't know how invoices should work on New Expensify, but typically the coding (category & tag) are irrelevant to the recipient and used only for internal purposes. However the tax is really important as they may need to claim it back.

I would have thought that we should also not be showing the sender changing the coding, that's kinda private data.

Is there someone who is 100% sure on how this is supposed to work?

RachCHopkins commented 2 months ago

Confirmed internally @daledah - the coding data on an invoice should be private to the company that sends it. It should not show on the recipient end, nor should any comment/history pertaining to the coding i.e. Category or Tag. The recipient should see the tax component and tax rate.

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

daledah commented 2 months ago

Proposal updated

melvin-bot[bot] commented 2 months ago

@Pujan92, @RachCHopkins Eep! 4 days overdue now. Issues have feelings too...

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

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

@Pujan92 @RachCHopkins 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

@Pujan92, @RachCHopkins 10 days overdue. I'm getting more depressed than Marvin.

melvin-bot[bot] commented 2 months ago

@Pujan92, @RachCHopkins 12 days overdue now... This issue's end is nigh!

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

RachCHopkins commented 2 months ago

@Pujan92 how are you feeling about this proposal?

melvin-bot[bot] commented 2 months ago

This issue has not been updated in over 14 days. @Pujan92, @RachCHopkins eroding to Weekly issue.

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

melvin-bot[bot] commented 2 months ago

@Pujan92 @RachCHopkins 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!

Pujan92 commented 2 months ago

When user A sends invoice from workspace W to user B without adding them to the workspace W, user B won't have that policy data. That leads to the isPolicyExpenseChat false as @daledah mentioned.

I am not sure whether the BE should provide the policy data for user B or not in this case as mostly all fields are derived based on policy conditions. If policy data is available to the user B(invoice receiver) then it will show all the details, but if we are not considering the direction of providing that data then we need to render all these fields by adding more conditions as proposed here.

Adding a stamp for the internal engineer assignment and confirmation.

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 2 months ago

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

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

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

melvin-bot[bot] commented 1 month ago

📣 @daledah You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖

daledah commented 1 month ago

@johnmlee101 My proposal requires BE to pass taxRate property to the invoice data in the receiver side, which is not viable right now. Can you help address that?

Also:

The category/tag should not be shown on the recipient end, nor should any comment/history pertaining to the coding: Option 1: This should be done in BE side.

Option 2: We can modify this function:

What do you think we should do for these two options? I prefer the first option, as the second option require changes to a function that is used widely and could potentially cause unwanted regressions. Do you agree with this?

daledah commented 1 month ago

@johnmlee101 Friendly bump on https://github.com/Expensify/App/issues/47579#issuecomment-2382330333

daledah commented 1 month ago

@johnmlee101 Bump again on https://github.com/Expensify/App/issues/47579#issuecomment-2382330333. Any updates? I will open a PR ASAP once these changes are applied on the BE side.

johnmlee101 commented 1 month ago

I'll take a look today!

RachCHopkins commented 1 month ago

How'd that looking go @johnmlee101 ?

johnmlee101 commented 3 weeks ago

Sorry, I've been side-tracked massively by some other things, I'm hoping to look at this by EOW otherwise I'll find someone who has open time

RachCHopkins commented 1 week ago

@johnmlee101 What's the plan, Stan?

johnmlee101 commented 1 week ago

Looking at this today

Expensify / App