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.45k stars 2.81k forks source link

[$250] Invoices - "Missing category" appears briefly and disappears after unselecting category #47858

Open lanitochka17 opened 1 month ago

lanitochka17 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: 9.0.23-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: N/A Email or phone of affected tester (no customers): applausetester+kh050806@applause.expensifail.com Issue reported by: Applause - Internal Team

Action Performed:

Precondition:

Expected Result:

"Missing category" will not appear briefly and disappear

Actual Result:

"Missing category" appears briefly and disappears

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/d980662c-2459-4a16-972f-f266b8dfdc98

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01d49423a508ae7410
  • Upwork Job ID: 1828726601829143542
  • Last Price Increase: 2024-09-04
Issue OwnerCurrent Issue Owner: @ntdiary
melvin-bot[bot] commented 1 month ago

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

lanitochka17 commented 1 month ago

@miljakljajic 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 1 month ago

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

Proposal

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

"Missing category" appears briefly and disappears

What is the root cause of that problem?

When we enable "Members must categorize all expenses" in Categories settings and Unselect the category, it'll return error missingCategory in here https://github.com/Expensify/App/blob/d2eaf93ff4ea2fc007338cf0dcf036010a88330c/src/libs/Violations/ViolationsUtils.ts#L208-L210

After that the API call is done, it updates Violations to empty.

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

We will not set optimistic data violation when it is invoice room by adding new condition in here

            if (!hasMissingCategoryViolation && policyRequiresCategories && !categoryKey && !ReportUtils.isInvoiceReport(report)) {
                newTransactionViolations.push({name: 'missingCategory', type: CONST.VIOLATION_TYPES.VIOLATION});
            }

What alternative solutions did you explore? (Optional)

We can check the isInvoiceReport condition here

 if (policy && PolicyUtils.isPaidGroupPolicy(policy) && updatedTransaction && ('tag' in transactionChanges || 'category' in transactionChanges) && !ReportUtils.isInvoiceReport(iouReport))
FitseTLT commented 1 month ago

Edited by proposal-police: This proposal was edited at 2024-08-22 14:16:41 UTC.

Proposal

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

"Missing category" appears briefly and disappears after unselecting category

What is the root cause of that problem?

We are currently creating missing category violations optimisitically for invoices too https://github.com/Expensify/App/blob/bf5d7431ac4ea662215a3ae05268f880e63f7ccd/src/libs/actions/IOU.ts#L2700-L2718 But the BE is not returning missing category violations for invoices This inconsistency also happens when we Send Invoice too; the BE doesn't return violations on send invoice but we are adding it optimistically here https://github.com/Expensify/App/blob/bf5d7431ac4ea662215a3ae05268f880e63f7ccd/src/libs/actions/IOU.ts#L1238-L1255

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

We need to align the optimistic data with what the BE returns. For instance, in this specific case we shouldn't create the optimistic violations when we are updating invoices https://github.com/Expensify/App/blob/ed04ac47da2194e279d4a4a6225280f755148bda/src/libs/actions/IOU.ts#L2700

if (
        policy &&
        PolicyUtils.isPaidGroupPolicy(policy) &&
        updatedTransaction &&
        ('tag' in transactionChanges || 'category' in transactionChanges) &&
        !ReportUtils.isInvoiceReport(iouReport)
    )

and also we should avoid the optimistic violation we are adding for send invoice in buildOnyxDataForInvoice here https://github.com/Expensify/App/blob/bf5d7431ac4ea662215a3ae05268f880e63f7ccd/src/libs/actions/IOU.ts#L1238-L1255

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 1 month ago

@miljakljajic Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

ntdiary commented 1 month ago

I tried looking for some previous issues (e.g., #45168 and design doc), but I couldn't access the doc.

@lanitochka17, @miljakljajic, could you please clarify whether the invoice transaction needs to follow the workspace settings (i.e., Members must categorize/tag all expenses)? If they do need to follow them, then the violation rules in the backend should be changed accordingly.

ntdiary commented 1 month ago

@miljakljajic, friendly bump for this comment. 🙂

miljakljajic commented 1 month ago

Apologies! Yes - workspace settings apply to both reports and invoices when invoices are enabled.

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

ntdiary commented 1 month ago

Apologies! Yes - workspace settings apply to both reports and invoices when invoices are enabled.

Great! Then I think the expected result here is that 'Missing category' should appear when we unselect the category.

This requires a backend engineer to add the related violations to the invoice transaction.

melvin-bot[bot] commented 1 month ago

@ntdiary @miljakljajic 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 3 weeks ago

@ntdiary, @miljakljajic Eep! 4 days overdue now. Issues have feelings too...

miljakljajic commented 3 weeks ago

Updating to internal as we'll need a BE engineer

trjExpensify commented 3 weeks ago

Moving this to #vip-billpay as it's invoices.

melvin-bot[bot] commented 2 weeks ago

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

miljakljajic commented 2 weeks ago

Reassigning for my parental leave - waiting for an internal volunteer. Thank you!

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 1 week ago

@ntdiary, @CortneyOfstad Huh... This is 4 days overdue. Who can take care of this?

CortneyOfstad commented 1 week ago

I'm having trouble recreating this, so going to see if this can be retested!

melvin-bot[bot] commented 1 week ago

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

CortneyOfstad commented 6 days ago

Still waiting for this to be retested!

melvin-bot[bot] commented 6 days ago

@ntdiary, @CortneyOfstad 10 days overdue. I'm getting more depressed than Marvin.

melvin-bot[bot] commented 4 days ago

@ntdiary, @CortneyOfstad 12 days overdue now... This issue's end is nigh!

CortneyOfstad commented 3 days ago

Still waiting for this to be retested!

melvin-bot[bot] commented 6 hours ago

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