fix: pass iouReport instead of chatReport for smartscan field check#86727
Conversation
…smartscan iouReport lookup
melvin-bot
bot
requested review from
heyjennahay and
thesahindia
and removed request for
a team
|
@thesahindia Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
@thesahindia, @aimane-chnaif will review this since it's related to a PR they authored - see here |
🤖 MelvinBot Code ReviewOverall: This is a well-targeted fix. The root cause is correctly identified and the fix is sound. All CI checks pass. What the PR does rightThe core bug: The fix correctly:
Non-blocking observations
Cat fact: A group of cats is called a "clowder," while a group of kittens is called a "kindle." |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-04-01.at.4.06.15.pm.mov |
|
production: (the bug already exists in production. showing RBR) 
staging: (showing "Fix") 
this branch: 
|
aimane-chnaif
left a comment
aimane-chnaif
left a comment
There was a problem hiding this comment.
@MelvinBot can you help with automated tests to prevent such bugs in the future?
Just provide code diff.
|
Here's a test diff that covers the exact regression scenario from this PR — ensuring Add the following --- a/tests/unit/ReportUtilsTest.ts
+++ b/tests/unit/ReportUtilsTest.ts
@@ -35,6 +35,8 @@
buildOptimisticIOUReportAction,
+ getReportActionWithSmartscanError,
+ hasSmartscanError,
buildOptimisticReportPreview,Then add the test block (at the end of the top-level + describe('getReportActionWithSmartscanError', () => {
+ const chatReportID = '100';
+ const expenseReportID = '200';
+ const transactionID = '300';
+ const iouReportActionID = '400';
+
+ const chatReport: Report = {
+ ...LHNTestUtils.getFakeReport(),
+ reportID: chatReportID,
+ type: CONST.REPORT.TYPE.CHAT,
+ };
+
+ const expenseReport: Report = {
+ ...LHNTestUtils.getFakeReport(),
+ reportID: expenseReportID,
+ type: CONST.REPORT.TYPE.EXPENSE,
+ parentReportID: chatReportID,
+ ownerAccountID: currentUserAccountID,
+ statusNum: CONST.REPORT.STATUS_NUM.OPEN,
+ };
+
+ // Transaction with $0 amount but valid merchant and created date.
+ // On an expense report this should NOT be a smartscan error (amount is
+ // irrelevant; only merchant matters). On a chat report the old code path
+ // would incorrectly flag getAmount(txn) === 0 as a missing field.
+ const transaction: Transaction = {
+ ...createRandomTransaction(Number(transactionID)),
+ transactionID,
+ reportID: expenseReportID,
+ amount: 0,
+ merchant: 'Coffee Shop',
+ modifiedMerchant: '',
+ created: testDate,
+ };
+
+ // Money‑request (IOU) action that lives inside the expense report's actions
+ const iouAction = {
+ ...createRandomReportAction(Number(iouReportActionID)),
+ reportActionID: iouReportActionID,
+ actionName: CONST.REPORT.ACTIONS.TYPE.IOU as const,
+ actorAccountID: currentUserAccountID,
+ originalMessage: {
+ IOUTransactionID: transactionID,
+ IOUReportID: expenseReportID,
+ type: CONST.IOU.REPORT_ACTION_TYPE.CREATE,
+ amount: 0,
+ currency: CONST.CURRENCY.USD,
+ comment: '',
+ participantAccountIDs: [currentUserAccountID],
+ },
+ };
+
+ // REPORT_PREVIEW action that sits in the chat report and links to the expense report
+ const reportPreviewAction = buildOptimisticReportPreview(chatReport, expenseReport as Report, '', transaction);
+
+ beforeAll(async () => {
+ await Onyx.multiSet({
+ [ONYXKEYS.SESSION]: {email: currentUserEmail, accountID: currentUserAccountID},
+ [`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`]: transaction,
+ [`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${expenseReportID}`]: {
+ [iouReportActionID]: iouAction,
+ },
+ });
+ return waitForBatchedUpdates();
+ });
+
+ it('should NOT flag $0 manual expense as smartscan error when expense report is resolved via reports collection', () => {
+ // With the reports collection the function can look up the actual
+ // expense report (type 'expense') via IOUReportID. For expense
+ // reports areRequiredFieldsEmpty checks isMerchantMissing (not
+ // amount), so a $0 expense with a valid merchant is fine.
+ const reportsCollection = {
+ [`${ONYXKEYS.COLLECTION.REPORT}${expenseReportID}`]: expenseReport,
+ };
+
+ const result = getReportActionWithSmartscanError([reportPreviewAction], chatReport, reportsCollection);
+ expect(result).toBeUndefined();
+ expect(hasSmartscanError([reportPreviewAction], chatReport, reportsCollection)).toBe(false);
+ });
+
+ it('should flag smartscan error when expense report has a missing merchant', async () => {
+ // Replace the transaction with one that has an empty merchant
+ const txnMissingMerchant: Transaction = {
+ ...transaction,
+ merchant: '',
+ modifiedMerchant: '',
+ };
+ await Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, txnMissingMerchant);
+ await waitForBatchedUpdates();
+
+ const reportsCollection = {
+ [`${ONYXKEYS.COLLECTION.REPORT}${expenseReportID}`]: expenseReport,
+ };
+
+ expect(hasSmartscanError([reportPreviewAction], chatReport, reportsCollection)).toBe(true);
+
+ // Restore original transaction for subsequent tests
+ await Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, transaction);
+ await waitForBatchedUpdates();
+ });
+
+ it('should NOT flag settled (reimbursed) expense reports even with missing fields', async () => {
+ const settledExpenseReport: Report = {
+ ...expenseReport,
+ statusNum: CONST.REPORT.STATUS_NUM.REIMBURSED,
+ };
+
+ // Even with missing merchant, a settled report should not show error
+ const txnMissingMerchant: Transaction = {
+ ...transaction,
+ merchant: '',
+ modifiedMerchant: '',
+ };
+ await Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, txnMissingMerchant);
+ await waitForBatchedUpdates();
+
+ const reportsCollection = {
+ [`${ONYXKEYS.COLLECTION.REPORT}${expenseReportID}`]: settledExpenseReport,
+ };
+
+ expect(hasSmartscanError([reportPreviewAction], chatReport, reportsCollection)).toBe(false);
+
+ // Restore
+ await Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, transaction);
+ await waitForBatchedUpdates();
+ });
+ });What these tests cover
The first test is the critical regression guard — if someone removes the Cat fact: Cats can rotate their ears 180 degrees, using 32 muscles in each ear — compared to just 6 muscles in a human ear. |
|
@sosek108 please add unit test above. Otherwise looks good. |
NikkiWines
left a comment
NikkiWines
left a comment
There was a problem hiding this comment.
lgtm aside from @aimane-chnaif's comment above
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@aimane-chnaif @NikkiWines tests added |
NikkiWines
left a comment
NikkiWines
left a comment
There was a problem hiding this comment.
@sosek108 typescript checks are failing
Otherwise lgtm
tests/unit/ReportUtilsTest.ts
Outdated
|
|
||
| it('should flag smartscan error when expense report has a missing merchant', async () => { | ||
| // Replace the transaction with one that has an empty merchant | ||
| const txnMissingMerchant: Transaction = { |
There was a problem hiding this comment.
| const txnMissingMerchant: Transaction = { | |
| const transactionMissingMerchant: Transaction = { |
Avoid using abbreviated variable names (even in tests)
tests/unit/ReportUtilsTest.ts
Outdated
| }; | ||
|
|
||
| // Even with missing merchant, a settled report should not show error | ||
| const txnMissingMerchant: Transaction = { |
There was a problem hiding this comment.
| const txnMissingMerchant: Transaction = { | |
| const transactionMissingMerchant: Transaction = { |
|
@NikkiWines fixed |
NikkiWines
left a comment
NikkiWines
left a comment
There was a problem hiding this comment.
@sosek108 could you merge main please? otherwise looks good
|
Done, let's wait for checks to pass |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @NikkiWines has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/NikkiWines in version: 9.3.54-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. This is a purely internal bug fix that corrects which report object (IOU/expense report vs. chat report) is passed to the smartscan field validation logic. The changes are confined to internal utilities (
The fix simply corrects an incorrect "Fix" badge from appearing on chat previews in the LHN — the badge itself is existing documented behavior that was just triggering incorrectly. |
|
Deploy Blocker #87343 was identified to be related to this PR. |
|
This PR failing because of the issue #87343 |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.3.54-7 🚀
|
6 participants
Explanation of Change
getReportActionWithMissingSmartscanFields(and the relatedshouldShowRBRForMissingSmartscanFields/hasSmartscanError/getReportActionWithSmartscanErrorchain) was receiving the chat report and forwarding it tohasMissingSmartscanFields. That utility evaluatesareRequiredFieldsEmptywithisFromExpenseReportderived from the report it receives — when given the chat report instead of the expense (IOU) report,isFromExpenseReportis alwaysfalse, which makes a $0 amount incorrectly pass the smartscan error check and show a "Fix" badge in the LHN even when there is no real error.The fix threads
reports: OnyxCollection<Report>through the call chain (generateReportAttributes→getAllReportErrors→getAllReportActionsErrorsAndReportActionThatRequiresAttention→hasSmartscanError/getReportActionWithSmartscanError) so thatgetReportActionWithSmartscanErrorcan look up the actual IOU/expense report byIOUReportIDand pass it toshouldShowRBRForMissingSmartscanFields/getReportActionWithMissingSmartscanFields. When the IOU report is not found in the collection it falls back to the original report, preserving existing behaviour for non-preview actions.Fixed Issues
$ #86109
PROPOSAL:
Tests
Offline tests
N/A
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari