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.33k stars 2.76k forks source link

[$250] Receipt audit shows "Receipt ° Verified √" when there's a violations related to SmartScan #46791

Open m-natarajan opened 1 month ago

m-natarajan 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.16-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: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @cead22 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1722635347734459

Action Performed:

Expected Result:

When "Date differs from scanned receipt" and/or "amount greater than scanned receipt" are visible, the receipt audit component shouldn't show "Receipt ° Verified √" Instead, it should say "Receipt ° Issue found" or "Receipt ° Issues found" However, we should not show "Date differs from scanned receipt" or "Amount greater than scanned receipts" under the receipt (ie, in the ReceiptAuditMessages component) We should show the "Date differs from scanned receipt" under the date field, and "Amount greater than scanned receipt" under the amount field

Actual Result:

You see "Receipt ° Verified √" when one of the two mentioned violations are present

Workaround:

unknown

Platforms:

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

Screenshots/Videos

https://github.com/user-attachments/assets/d27b8943-3af8-40a9-8edc-7992365c7ff5

image

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01925fef567df5c3b7
  • Upwork Job ID: 1820487169849591150
  • Last Price Increase: 2024-08-05
Issue OwnerCurrent Issue Owner: @cead22
melvin-bot[bot] commented 1 month ago

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

bernhardoj commented 1 month ago

Proposal

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

Receipt audit shows verified even though there is a receipt violation.

What is the root cause of that problem?

The "Issue found" will be shown if there is a violation that is included in receiptViolationNames array. https://github.com/Expensify/App/blob/2eb65f8a666d0337ff6fd0d2ea0e7669b9d6aa6e/src/components/ReportActionItem/MoneyRequestView.tsx#L377-L384

Both "Date differs from scanned receipt" and "Amount greater than scanned receipt" violations are not in the array, so the "Verified" is shown. https://github.com/Expensify/App/blob/2eb65f8a666d0337ff6fd0d2ea0e7669b9d6aa6e/src/components/ReportActionItem/MoneyRequestView.tsx#L433-L434 https://github.com/Expensify/App/blob/2eb65f8a666d0337ff6fd0d2ea0e7669b9d6aa6e/src/components/ReceiptAudit.tsx#L23-L28

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

We can just add both CONST.VIOLATIONS.MODIFIED_AMOUNT and CONST.VIOLATIONS.MODIFIED_DATE to receiptViolationNames, but it will also show the violation below the receipt which we don't want.

image

The violation below receipt is shown by ReceiptAuditMessages. https://github.com/Expensify/App/blob/2eb65f8a666d0337ff6fd0d2ea0e7669b9d6aa6e/src/components/ReportActionItem/MoneyRequestView.tsx#L502

So, I propose to rename receiptViolationNames to receiptImageViolationNames and add a new array called receiptFieldViolationNames which contains the new violation name. Then, we will also have a new array called receiptImageViolations which contains the image-only violation (the current one).

const receiptImageViolationNames: OnyxTypes.ViolationName[] = [
    CONST.VIOLATIONS.RECEIPT_REQUIRED,
    CONST.VIOLATIONS.RECEIPT_NOT_SMART_SCANNED,
    CONST.VIOLATIONS.CASH_EXPENSE_WITH_NO_RECEIPT,
    CONST.VIOLATIONS.SMARTSCAN_FAILED,
];
const receiptFieldViolationNames: OnyxTypes.ViolationName[] = [
    CONST.VIOLATIONS.MODIFIED_AMOUNT,
    CONST.VIOLATIONS.MODIFIED_DATE,
    // other violation names if needed
]
const [receiptImageViolations, receiptViolations] = useMemo(() => {
    const imageViolations = [];
    const allViolations = [];

    for (const violation of (transactionViolations ?? [])) {
        const isReceiptFieldViolation = receiptFieldViolationNames.includes(violation.name);
        const isReceiptImageViolation = receiptImageViolationNames.includes(violation.name);
        if (isReceiptFieldViolation || isReceiptImageViolation) {
            const violationMessage = ViolationsUtils.getViolationTranslation(violation, translate);
            allViolations.push(violationMessage);
            if (isReceiptImageViolation) {
                imageViolations.push(violationMessage);
            }
        }
    }
    return [imageViolations, allViolations];
}, []);

Then, we pass the receiptImageViolations to ReceiptAuditMessage so it will only show the violation message from receiptImageViolationNames. https://github.com/Expensify/App/blob/2eb65f8a666d0337ff6fd0d2ea0e7669b9d6aa6e/src/components/ReportActionItem/MoneyRequestView.tsx#L502

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

cead22 commented 1 month ago

@thesahindia any thoughts on @bernhardoj 's proposal?

thesahindia commented 1 month ago

@bernhardoj's proposal looks good to me!

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 1 month ago

Current assignee @cead22 is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

cead22 commented 1 month ago

@bernhardoj do you think you'll have a PR up for this today?

bernhardoj commented 1 month ago

Yes, I'll work on the PR in a few minutes.

cead22 commented 1 month ago

Awesome, thank you!

bernhardoj commented 1 month ago

PR is ready

bernhardoj commented 2 weeks ago

The PR was deployed to prod 2 weeks ago, so this should be ready for payment.

Requested in ND.

JmillsExpensify commented 2 weeks ago

Awaiting payment summary

melvin-bot[bot] commented 1 week ago

This issue has not been updated in over 15 days. @cead22, @muttmuure, @bernhardoj, @thesahindia eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

muttmuure commented 1 week ago

$250 - @bernhardoj $250 - @thesahindia

JmillsExpensify commented 1 week ago

$250 approved for @bernhardoj

cead22 commented 2 days ago

Anything else we need to do before we close this one?

garrettmknight commented 2 days ago

$250 approved for @thesahindia

garrettmknight commented 2 days ago

@thesahindia can you complete the checklist?

thesahindia commented 1 day ago

"Date differs from scanned receipt" and "Amount greater than scanned receipt" violations weren't in the receiptViolationNames array. Because of that we were seeing "Verified".

We can add a test case for them, here are the steps :-

  1. Scan a real receipt that will SmartScan successfully
  2. Wait for it to SmartScan
  3. Once SmartScan finishes, change the date/amount of the receipt
  4. Verify you see a violation under the date field "date differs from scanned receipt"
  5. Verify you see a violation under the amount field "Amount greater than scanned receipt"
  6. Verify you see "Receipt ° Issue found" above the receipt