[HOLD for payment 2024-06-11] [$250] Expense - Undefined displayed in violation message instead of amount when tax enabled

[lanitochka17]

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: 1.4.71.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
Issue reported by: Applause - Internal Team

Action Performed:

recondition: user is a member of a collect policy with enabled taxes

1. Go to https://staging.new.expensify.com/
   and log in as member
2. Navigate to the workspace chat and submit expense > $200
3. Submit the report
4. Log in as the admin
5. Navigate to the workspace chat
6. Open the submitted report and wait for violations to appear

Expected Result:

The violation message includes exceeding amount

Actual Result:

The violation message "Expense exceeds auto approval limit of undefined" is displayed

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6473730_1715085447589.Recording__459.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~016679daf410f80dd0
• Upwork Job ID: 1788683822954524672
• Last Price Increase: 2024-05-23
• Automatic offers:
• fedirjh | Reviewer | 102440626
• gijoe0295 | Contributor | 102440630

Issue Owner

Current Issue Owner: @kevinksullivan

[melvin-bot]

Triggered auto assignment to @kevinksullivan (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]

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

[lanitochka17]

We think that this bug might be related to #wave-collect - Release 1

[melvin-bot]

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

[melvin-bot]

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

[kevinksullivan]

feels like collect polish to me, so moving it to that project.

[roitman-g]

Could anyone provide the steps for enabling the recondition - "user is a member of a collect policy with enabled taxes"?

[melvin-bot]

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

[gijoe0295]

Proposal

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

Based on #42173:

Expected Result:

The violation correctly states what the auto-approval limit is. This violation should also appear in the amount field.

Actual Result:

The violation incorrectly states an undefined auto-approval limit. The violation is showing under the receipt.

We have 2 issues:

1. The auto-approval limit amount is undefined
2. The violation should ALSO show under Amount field

What is the root cause of that problem?

1. The data in transactionViolations for overAutoApprovalLimit is null. This is a BE issue.
2. In [HOLD for payment 2024-05-09] [Violations] [$500] Implement Receipt Audit Feature / Note type violations #36288, we expected to show the notice type violations under the receipt and overAutoApprovalLimit is of type notice. Moreover, error text for each field is evaluated by getErrorForField which internally uses getViolationsByField which only filters out violation type:

App/src/components/ReportActionItem/MoneyRequestView.tsx

Lines 408 to 409 in bd6595f

	brickRoadIndicator={getErrorForField('amount') ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : undefined}
	errorText={getErrorForField('amount')}

App/src/hooks/useViolations.ts

Line 53 in 57751f1

const filteredViolations = violations.filter((violation) => violation.type === CONST.VIOLATION_TYPES.VIOLATION);

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

Latest expectation:

• 'receiptRequired', 'receiptNotSmartScanned', 'modifiedDate', 'cashExpenseWithNoReceipt', 'smartscanFailed' maps to receipt field and thus shows in ReceiptAudit
• Other notices show in their corresponding fields as per

  App/src/hooks/useViolations.ts

  Line 13 in 57751f1

  const violationFields: Record<ViolationName, ViolationField> = {

Waiting for expected behavior confirmation

1. Fix the BE
2. I think there might be confusion here when we want to show the notice violation in both receipt audit and amount field. I want to confirm expectation on this but if so, we can:

• Introduce a new param types to getViolationsForField in useViolations to optionally filter violations by specified type. Then in violationsByField we won't need to filter violation type only and move the filter logic to getViolationsForField based on the above types param:

const getViolationsForField = (field, data?, types = ['violation']) => {
    const fieldViolations = violationsByField.get(field) ?? [];
    const currentViolations = fieldViolations.filter((violation) => types?.includes(violation.type));
    ...
}

• Use it in MoneyRequestView where we want to shows notice type as well

getViolationsForField(field, data, ['violation', 'notice']);

[gijoe0295]

@kevinksullivan Could you please update the OP to reflect the complete expected results outlined in #42173?

@JmillsExpensify overAutoApprovalLimit is of type notice and as per #36288, we want to show notice violation under receipt audit only. Is there any misunderstanding here? Could you confirm the expected behavior?

[fedirjh]

Could anyone provide the steps for enabling the recondition - "user is a member of a collect policy with enabled taxes"?

@roitman-g Please check this thread: https://expensify.slack.com/archives/C01GTK53T8Q/p1708422807620599

[fedirjh]

@gijoe0295 Did you manage to replicate this issue? I'm unable to replicate it.

[gijoe0295]

@fedirjh I enabled Expense Violations and set Members > Expense Approvals > Manually approve all expenses over in OD and the violation did appear in OD reports but not in ND. But fortunately an expense came out of nowhere with that violation so I could test.

I guess both the submitter and the approver need to be added to violations beta in the backend. Maybe @kevinksullivan could help. My test emails are [email protected] and [email protected].

[fedirjh]

I guess both the submitter and the approver need to be added to violations beta in the backend.

@gijoe0295 Yes, that's right. I was only able to reproduce the issue after being added to the violations beta.

cc @cead22 Could you please take a look at this issue? It seems like the backend does not return the correct data. @gijoe0295 has asked a valid question in #41771 (comment). Can you also review it?

[cead22]

You're right. I'll submit a back end fix to send formattedLimit in the data

[cead22]

Looking closer, this is a notice that is being shown under the receipt, because I think we're showing all notices in that place, but maybe what we need to do here right now is to not show that. What do you think @JmillsExpensify

Also, I think the setup for this isn't about taxes, but about having auto approval enabled and an auto approval limit set

[melvin-bot]

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

[gijoe0295]

We already covered violation type violations by getViolationsForField:

App/src/components/ReportActionItem/MoneyRequestView.tsx

Lines 396 to 397 in 182fb9f

	{shouldShowNotesViolations && <ReceiptAuditMessages notes={receiptNoticeViolations} />}
	{canUseViolations && <ViolationMessages violations={getViolationsForField('receipt')} />}

And RECEIPT_REQUIRED is of type violation so we would have duplicated messages like this:

So I added notice type violation condition to the filter here.

[cead22]

Should we remove this line instead {canUseViolations && <ViolationMessages violations={getViolationsForField('receipt')} />}, and that way we don't need violation.type === CONST.VIOLATION_TYPES.NOTICE &&?

[gijoe0295]

@cead22 Thanks, that sounds much better! What's about modifiedDate? Is it of type violation or notice? Just want to make sure we wouldn't show duplicated messages for it (one under receipt, the other under date field).

[cead22]

modifiedDate is a notice

[melvin-bot]

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

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.78-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Only show receipt-related violations in receipt audit #42661

If no regressions arise, payment will be issued on 2024-06-11. 🎊

For reference, here are some details about the assignees on this issue:

• @fedirjh requires payment automatic offer (Reviewer)
• @gijoe0295 requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@fedirjh] The PR that introduced the bug has been identified. Link to the PR:
• [@fedirjh] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@fedirjh] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@fedirjh] Determine if we should create a regression test for this bug.
• [@fedirjh] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@kevinksullivan] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[gijoe0295]

@fedirjh @kevinksullivan We're good for payment.

[kevinksullivan]

Hey @fedirjh can you finish out the steps above?

[kevinksullivan]

Payments made, just waiting on @fedirjh for final steps and then we're all set

[kevinksullivan]

Moving to weekly since we're only held on steps here.

[fedirjh]

BugZero Checklist:

• Link to the PR: N/A This looks like a feature request rather than a bug.
• Link to comment: N/A
• Link to discussion: N/A
• Determine if we should create a regression test for this bug: Y

Regression Test Proposal

Precondition: Have a Control workspace in which both the admin and the employee are in violation betas.

1. In OldDot, enable Expenses > Expense Violations and set Members > Expense Approvals > Manually approve all expenses over to an arbitrary amount
2. In NewDot, as employee, submit an expense whose amount is larger than the limit in step 1 that workspace
3. As admin, verify after a while, no violation shows under receipt section in transaction thread report
4. In OldDot, go to Expenses > Expense Violations and set Receipt Required Amount to a specific amount
5. In NewDot, as employee, submit an expense without receipt whose amount is larger than the limit set in step 4 to that workspace
6. As admin, verify after a while, a violation shows under receipt section in transaction thread report saying Receipt required over x amount.

• Do we agree 👍 or 👎