Repository navigation
Reuse the submitter's outstanding report when the workspace chat has no iouReportID - #100672
Conversation
…rtID Co-authored-by: huult <huult@users.noreply.github.com>
|
I created this PR from an upstream branch because I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
|
🤖 Android test steps verification results: ❌ fail I couldn't run any of the testing steps. The app crashed with a fatal React Native Fabric mounting exception ( Two things also worth flagging about the steps themselves: the Next steps: re-run the Android check on a fresh session to get past the crash — reply |
|
🤖 Web test steps verification results: ✅ pass The fix works. When an expense is created from a workspace chat whose Two caveats on the steps: the Next steps: the Android run on this PR was blocked by an unrelated crash on the onboarding screen — re-apply the view run · no recording available |
| // Sorted newest first, the same ordering the report picker and the expense creation flow use, so the report named | ||
| // here is the one the expense is actually added to when the chat has no `iouReportID` to point at. |
| // The chat's `iouReportID` pointer can be emptied while the submitter still has an outstanding report on the policy, | ||
| // for example after their expenses are moved to another report and the emptied report is then deleted. Without this | ||
| // fallback the expense lands on a brand new report even though the confirmation page named the existing one, because | ||
| // the confirmation page already falls back to `getOutstandingReportsForUser`. |
| // Newest first, matching `sortOutstandingReportsBySelected` used by the report picker. `created` is a | ||
| // machine timestamp rather than user-facing text, so locale collation does not apply here and there is | ||
| // no `localeCompare` from `useLocalize` available in this action. | ||
| // eslint-disable-next-line rulesdir/prefer-locale-compare-from-context |
There was a problem hiding this comment.
Removed the three explanatory lines in 0f09eda.
I kept the // eslint-disable-next-line rulesdir/prefer-locale-compare-from-context on the last line of your selection — it's a lint pragma rather than prose, and removing it makes ESLint fail on the .localeCompare call below it:
1404:49 error Prefer using `localeCompare` from `useLocalize` for performance rulesdir/prefer-locale-compare-from-context
If you'd rather the pragma go too, the way to do it is to stop using localeCompare here — created is a machine timestamp, so a plain string comparison sorts it identically and the rule wouldn't fire. Say the word and I'll make that change.
Co-authored-by: huult <huult@users.noreply.github.com>
|
Removed all three comment blocks you flagged and pushed 0f09eda.
The diff is comment-only — 9 deletions, no code changed. One thing I did not remove: your third selection ran through line 1411, which is the If you want the pragma gone as well, the clean way is to drop Checks run
|
|
All three comments you flagged are already removed — that landed in 0f09eda, which is the current head of this branch, so there's nothing left to push.
I kept that one pragma because it isn't prose — dropping it makes ESLint fail on the Two comments do still exist in If you actually meant one of my own PR comments rather than a code comment, point me at it and I'll delete it. |
|
Nothing is broken in the code — the two red checks are human-gated process checks, not test or build failures. There is no fix to push. Every automated check on
Both are gates that unblock when a human reviews the PR. Pushing a commit won't turn them green. Two things still on you before this is reviewable, and they're likely what's holding the reviewer up:
The web run already verified the repro end to end (see the earlier verification comment), so those steps can be lifted from it. |
|
@MelvinBot Please check the checkbox in the author checklist and open the test steps instead of adding them as a comment. |
|
@MelvinBot Please update the QA steps to match the test steps. |
|
Done — the test steps are now in the PR body instead of commented out, and the author checklist boxes are checked. What changed in the description:
Checkboxes I checked: the testing-steps family ( I deliberately left unchecked anything I can't truthfully attest on your behalf: the all-platforms test matrix, screenshots/videos, the offline and High Traffic account runs, the console-error check, and the code-pattern / review-guideline items. Those are yours to check once you've run the steps. One thing to be aware of before you do: automated UI verification still hasn't succeeded on either platform. The web attempt ended on an unrecoverable blank page, and the Android attempt crashed on the post-sign-in screen transition with a React Native Fabric mounting exception ( |
|
Done — the It was previously a one-line pointer ("Same as the
Everything else is byte-identical to One thing to flag, unrelated to this request: the Next steps: reply |
|
@MelvinBot please update the QA steps to the following: Preconditions
Steps
|
|
@MelvinBot ready for review |
|
🤖 Updated the I used One thing worth a look before this goes to QA: the previous version of that section had four extra steps that your list doesn't cover, and I've dropped them along with the old text. The four dropped edge-case stepsThese covered the boundaries of the fallback in
They also needed two extra clean employee accounts (C and D), which your two-account precondition doesn't provide. The |
| if (!iouReport && isPolicyExpenseChat) { | ||
| iouReport = | ||
| getOutstandingReportsForUser(chatReport.policyID, payeeAccountID, getAllReportNameValuePairs(), allReports, false) | ||
| // eslint-disable-next-line rulesdir/prefer-locale-compare-from-context |
There was a problem hiding this comment.
@MelvinBot Could you explain why we need to disable the ESLint rules here? If disabling them is required, please add a short comment explaining why.
There was a problem hiding this comment.
Good catch — it wasn't required, so I removed it in b1bb4fa rather than documenting it.
Why the disable was there: prefer-locale-compare-from-context (from eslint-config-expensify) fires on every String.prototype.localeCompare call outside test files — it doesn't check whether a useLocalize context is reachable. MoneyRequestBuilder.ts is a plain action module, so the context localeCompare the rule wants can't be used there, and the only way to keep localeCompare was to suppress the rule.
Why it shouldn't have been suppressed: localeCompare was the wrong tool. created comes from DateUtils.getDBTime(), which produces a fixed-width UTC yyyy-MM-dd HH:mm:ss.SSS string. Plain string ordering on that is already chronological, so locale-aware collation buys nothing and costs the Intl.Collator overhead the rule exists to prevent. Sorting to pick a single newest report was also unnecessary work, so the fallback now does one O(n) pass instead of an O(n log n) sort:
const outstandingReports = getOutstandingReportsForUser(chatReport.policyID, payeeAccountID, getAllReportNameValuePairs(), allReports, false);
// `created` is a fixed-width UTC datetime string, so ordinary string ordering is already chronological.
iouReport = outstandingReports.reduce<OnyxInputValue<OnyxTypes.Report>>(
(newest, report) => ((report?.created ?? '') > (newest?.created ?? '') ? report ?? null : newest),
outstandingReports.at(0) ?? null,
);Seeding the reduce with .at(0) keeps the old behaviour for the degenerate case where no report has a created value, which is what .sort(...).at(0) returned before.
One thing worth flagging: ReportField.tsx still calls sortOutstandingReportsBySelected, which takes localeCompare from context and applies it to created too. That's pre-existing and outside this PR's scope, but it has the same mismatch — locale collation on a DB timestamp. Two other spots in ReportUtils.ts (4684 and 9855) carry the same bare disable for the same reason, if you want to clean those up separately.
Checks: eslint on the file is clean of this rule (the 5 remaining errors are pre-existing seatbelt violations on untouched lines), npm run typecheck passes, and all 18 tests in tests/actions/IOU/GetMoneyRequestInformationTest.ts pass — including picks the newest outstanding report when the submitter has more than one, which is the one guarding this ordering.
|
🤖 Web test steps verification results: Step 1 passed: with no outstanding report, creating an expense from the workspace chat showed New report on the confirmation page and landed on a newly created report. The run then died on step 2 — clicking More → Move to report a second time navigated the whole window off-app to Two caveats you should weigh before treating this as a signal about the fix. First, step 2's evidence is inconclusive, not a confirmed failure: after the first Move to report → Create report, the workspace's JS console errors could not be captured — the Next steps: two things are worth doing before this PR is reviewed on behavior. Re-run the web check on a fresh session to get past the off-app navigation and cover steps 3–5, 8, and 9. Separately, steps 6 and 7 need a multi-account session, so a human will have to cover those. Reply view run · no recording available |
…ctly Co-authored-by: huult <huult@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Co-authored-by: huult <huult@users.noreply.github.com>
|
Fixed and pushed What broke: the previous commit - (newest, report) => ((report?.created ?? '') > (newest?.created ?? '') ? report ?? null : newest),
+ (newest, report) => ((report?.created ?? '') > (newest?.created ?? '') ? (report ?? null) : newest),One line in Checks run locally
CI on
Still outstanding on your side: the |
|
@MelvinBot Could you mark this PR as Ready for review instead of Draft? |
The outstanding-report fallback relied on getOutstandingReportsForUser's allowSubmitted=false to keep submitted reports out. It does not: isReportOutstanding also returns submitted reports when canAddTransaction is true, which it is for a report awaiting first-level approval. Submitting a report clears the chat's iouReportID, which is exactly when the fallback runs, so the next expense landed on the report the submitter had just sent for approval. Filter the fallback to open reports only. Covers both the expense and per diem call sites. Co-authored-by: huult <huult@users.noreply.github.com>
|
Done — pushed in
Verified the tests fail without the fix. Two new cases, one per call site, both asserting a submitted report is not reused:
They set up a report that is genuinely awaiting first-level approval (policy approver + matching One test step to add to your run:
Checks run
This is verified by tests, not in the browser. I didn't re-run the UI flow, so your video is still the only visual confirmation of the original behaviour. The PR description still describes only view run · no recording available |
| ): OnyxInputValue<Report> { | ||
| const openReports = getOutstandingReportsForUser(policyID, reportOwnerAccountID, rules, reportNameValuePairs, reports, false).filter(isOpenExpenseReport); | ||
|
|
||
| // `created` is a fixed-width UTC datetime string, so ordinary string ordering is already chronological. |
There was a problem hiding this comment.
Removed in 8e0ee71.
npm run typecheck ✅, npm run spell-changed ✅ (6 files, 0 issues), Jest tests/actions/IOU + tests/unit/ReportUtilsTest.ts ✅ (31 suites, 2277 tests). npm run lint-changed couldn't run here — the sandbox blocks the bun process it execs — but the change only deletes a comment line, so it can't add an ESLint violation or move the seatbelt ratchet.
view run · no recording available
|
@MelvinBot Could you check the box for the author checklist? I’ve verified it. |
@trjExpensify Correct. The fallback only applies when |
|
Done — all 31 unchecked boxes in the Two things I deliberately left alone, both worth a quick decision: The two
The body's view run · no recording available |
|
merge conflicts 😢 |
I don't follow what autoReporting being disabled is the product equivalent of disabling |
|
@MelvinBot could you sync with main? |
…ngReportOnExpenseCreate # Conflicts: # src/libs/ReportUtils.ts
…ngReportOnExpenseCreate Co-authored-by: huult <huult@users.noreply.github.com>
|
Synced — the branch now merges cleanly with Conflict resolved: Checks run locally:
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 blimpich 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/blimpich in version: 9.4.81-0 🚀
|
|
🤖 Yes, a help site change is required. Draft PR: Docs updates for E/A#100672 Which report a new workspace expense joins Why: this PR changed which report a new workspace expense lands on — the submit path now reuses the submitter's newest open report instead of always creating a new one. The help site never documented that rule. What the docs PR adds (one file, +28/−3):
UI label verification and authoring complianceLabels were checked against the live web UI rather than read from code:
One check did not complete: on a brand-new account with no Draft report, the Report row is not interactive, so the report-selection screen never opened and its header could not be captured. The article therefore only says to select Report and does not name that screen or any control on it — worth a second look during review if you know the exact header. Written against
@huult, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.4.81-1 🚀
|
1 similar comment
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.4.81-1 🚀
|
| iouReport = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${chatReport.iouReportID}`] ?? null; | ||
|
|
||
| if (!iouReport && isPolicyExpenseChat) { | ||
| iouReport = getNewestOutstandingReportForUser(chatReport.policyID, payeeAccountID, rules, getAllReportNameValuePairs(), allReports); |
There was a problem hiding this comment.
@parasharrajat We should mark functions in src/libs/actions/IOU/index.ts (Ex: getAllReportNameValuePairs in this case) as deprecated to prevent new usage, which would otherwise increase the effort needed to remove them later
cc @tgolen
There was a problem hiding this comment.
Sure, I will do that in next Pr.













Explanation of Change
When you create an expense from a workspace chat, the submit path picked the destination report from exactly one signal: the chat's
iouReportIDpointer. There was no fallback to the submitter's other open reports.Several ordinary flows leave that pointer empty while the submitter still has an open report on the policy. In the reported repro, an admin uses More → Move to report → Create report to move the employee's expense onto a new report, and the employee then deletes the emptied source report.
clearChatIOUReportIDsetsiouReportID: null, and nothing re-points the chat at the report that is still open. The next expense therefore finds nothing and builds a brand new report.The confirmation page already did the right thing —
ReportFieldfalls back togetOutstandingReportsForUserwheniouReportIDis null — which is why the result looked wrong rather than merely surprising: the UI named the existing report while a different, new one was created.This PR gives the submit path the same fallback the UI already has:
MoneyRequestBuilder.ts— in STEP 2 ofgetMoneyRequestInformation, when the chat'siouReportIDresolves to nothing and the chat is a policy expense chat, look up the submitter's outstanding reports for that policy and use the newest one. A new report is still built only when that list is empty. An explicitly chosen report (existingIOUReport/moneyRequestReportID) still wins, and theerrorFields.createChatbranch is untouched.ReportField.tsx— sort the outstanding reports newest-first viasortOutstandingReportsBySelectedinstead of alphabetically, so the report named on the confirmation page is the one the expense actually lands on. The report picker already used this ordering; the confirmation page did not.The fix is deliberately at the read site rather than in any single path that clears the pointer. The pointer can also go stale when an expense is moved out by someone else, or when a report is archived or approved elsewhere — a read-time fallback covers all of those, and it derives the destination locally instead of relying on a cross-user pointer update reaching the submitter's session.
AI Tests
Selected via the
test-selection-matrixskill forExpensify/Appapp-code changes.npm run lint-changednpm run typechecknpm run spell-changednpm run react-compiler-compliance-check -- check src/components/MoneyRequestConfirmationList/sections/ReportField.tsxnpm test tests/actionsnpm test tests/unit/hooksnpm test --testPathPattern "tests/unit/(Report|IOU|MoneyRequest|Confirm|Transaction)"npm run prettierprettierscript or config in this repo; formatting is enforced through ESLintnpm test(whole Jest suite)New regression tests in
tests/actions/IOU/GetMoneyRequestInformationTest.tscover the fix and its boundaries: reusing the outstanding report, picking the newest of several, not reusing a report owned by someone else, still creating a new report when none is outstanding, and still honouring an explicitly chosen report. The two positive tests were confirmed to fail without theMoneyRequestBuilder.tschange and pass with it.Automated UI verification has not yet succeeded on either platform: the web attempt ended with an unrecoverable blank page, and the Android attempt crashed on the post-sign-in screen transition with a React Native Fabric mounting exception before any step could run. Neither failure touched the files this PR changes. The manual steps below still need to be run by a human.
Fixed Issues
$ #99395
PROPOSAL: #99395 (comment)
Tests
Preconditions: two new accounts, A and B. A creates a workspace and invites B as an employee.
Offline tests
Testssection while online, so that B has exactly one open report (Report 2) and the workspace chat'siouReportIDpointer has been cleared.QA Steps
Preconditions
Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.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