Repository navigation
Enforce the cash-expense reimbursable rule when defaultReimbursable is absent - #101903
Conversation
…nd honor the reimbursable lock at expense creation Co-authored-by: cretadn22 <[email protected]>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
|
|
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.
|
|
🤖 Web test steps verification: ❌ not run I could not run these steps. Two blockers:
Instead I did a static pass over the diff. It surfaced a regression you should test before this merges. Regression: a workspace's reimbursable lock now leaks onto P2P expenses
Repro to try: open a workspace chat → The expense is created non-reimbursable, and the user cannot fix it because Why
|
|
@MelvinBot Please sync this PR with the main branch |
|
@MelvinBot mark this PR ready for review |
…enseNonReimbursableRule Co-authored-by: cretadn22 <[email protected]> # Conflicts: # src/pages/Share/SubmitDetailsPage.tsx
|
I merged I fixed one merge conflict in I couldn't run lint or typecheck locally, so check the CI results on the new commit. |
|
@cretadn22 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] |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
I marked this PR ready for review. CI has passed on the merge commit. The only failure is |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bdd29ae12
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| policy?.defaultReimbursable !== undefined && !!(updatedTransaction?.reimbursable ?? transactionReimbursable) !== policy.defaultReimbursable; | ||
| // An absent `defaultReimbursable` means "reimbursable", so compare against that instead of bailing out. | ||
| // Bailing out hid the toggle on exactly the expenses that drifted from a locked policy, leaving them stuck. | ||
| const isCurrentTransactionReimbursableDifferentFromPolicyDefault = !!(updatedTransaction?.reimbursable ?? transactionReimbursable) !== (policy?.defaultReimbursable ?? true); |
There was a problem hiding this comment.
When a locked policy and an expense both omit their reimbursable defaults, the rest of the application treats both omissions as true (for example, getReimbursable() and the toggle use ?? true), but this expression converts the transaction value to false with !!undefined while converting the policy value to true. An editable, valid always-reimbursable expense is therefore misclassified as drifted and exposes a toggle that can set it contrary to the locked policy; normalize the transaction side with ?? true before comparing it, including the equivalent derived checks in the confirmation and split views.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot Regarding Codex's P1 comment, why did we add ?? true after policy?.defaultReimbursable?
There was a problem hiding this comment.
We added ?? true because a missing policy.defaultReimbursable means "reimbursable" everywhere else. The backend assumes that, and so does the creation path in getDefaultReimbursableForPolicy. Without it, the toggle check would treat an unset policy as non-reimbursable, which is the mismatch that caused this bug.
Codex is right that the transaction side needs the same fallback. canShowReimbursableToggleForPolicy uses !!isReimbursable, so a missing transaction value becomes false. The App's own default is true (getReimbursable). The callers handle this differently:
MoneyRequestViewis safe, because its value already comes fromgetReimbursable.useFooterDerivedFlags(selector) andDynamicSplitExpenseEditPage(line 275) pass the raw value. On a locked, reimbursable-by-default policy, they would show the toggle wrongly.
The fix is one line in the helper: return (isReimbursable ?? true) !== (policy.defaultReimbursable ?? true);. To have me make that change and add a test for it, reply @MelvinBot implement this.
There was a problem hiding this comment.
We added ?? true so the toggle check reads a missing policy.defaultReimbursable the same way expense creation does: as "reimbursable."
The old check started with policy?.defaultReimbursable !== undefined &&. When the policy had no stored value, the check returned false and hid the toggle. That missing-value case is the one this bug is about. The creation path (IOUUtils.ts:483) treats a missing value as true, and so does the backend. So on a locked workspace, a wrong expense had no toggle and the user couldn't fix it. Replacing the bail-out with ?? true keeps the check running and compares the expense against the default that actually applies.
Codex's point is about the other side of that comparison. PolicyUtils/index.ts:1294 uses !!isReimbursable, so a missing transaction value becomes false. A locked, reimbursable-by-default policy would then show the toggle when it shouldn't. The fix is (isReimbursable ?? true) !== (policy.defaultReimbursable ?? true). Reply @MelvinBot implement this and I'll make the change and add a test.
There was a problem hiding this comment.
I made this change in 282b0b9.
canShowReimbursableToggleForPolicynow treats a missing expense value as reimbursable:(isReimbursable ?? true) !== (policy.defaultReimbursable ?? true). This fixes the confirmation footer and the split edit page.- I added two tests in
PolicyUtilsTest.ts. On a locked reimbursable policy, the toggle is hidden. On a locked non-reimbursable policy, the toggle is shown.
I couldn't run Jest or lint locally, so check the CI results on this commit.
| (isPolicyExpenseChat || isTrackExpense) && !!policy && policy?.disabledFields?.reimbursable !== true && !isManagedCardTransaction(transaction) && !isTypeInvoice; | ||
| (isPolicyExpenseChat || isTrackExpense) && | ||
| !!policy && | ||
| (policy?.disabledFields?.reimbursable !== true || isCurrentTransactionReimbursableDifferentFromPolicyDefault) && |
There was a problem hiding this comment.
@MelvinBot Could we extract (policy?.disabledFields?.reimbursable !== true || isCurrentTransactionReimbursableDifferentFromPolicyDefault) into a helper function?
There was a problem hiding this comment.
@MelvinBot Can we simplify policy?.disabledFields?.reimbursable !== true to !policy?.disabledFields?.reimbursable?
There was a problem hiding this comment.
Done in 1bd9857. I added canShowReimbursableToggleForPolicy(policy, isReimbursable) to PolicyUtils. It also checks !policy?.disabledFields?.reimbursable instead of !== true, as you asked in your follow-up. The two checks behave the same because the field is boolean | undefined.
The same check appeared in three places, so all three now call the helper:
useFooterDerivedFlags.tsMoneyRequestView.tsxDynamicSplitExpenseEditPage.tsx
I added unit tests in tests/unit/PolicyUtilsTest.ts. I couldn't run lint, typecheck, or Jest locally, so check CI on this commit.
…ToggleForPolicy Co-authored-by: cretadn22 <[email protected]>
|
@MelvinBot In #90411, new logic was introduced where the non-reimbursable option only appears after the amount in single expense reports. This causes an inconsistency with multiple expense report cases. Could you make it consistent so that non-reimbursable is displayed in both? |
…rts too Co-authored-by: cretadn22 <[email protected]>
|
I pushed a6a924a. Non-reimbursable expenses now show "Amount · … · Non-reimbursable" in both single- and multi-expense reports.
🤖 Web test steps verification results: ✅ pass On web, non-reimbursable expenses show "· Non-reimbursable" in both single- and multi-expense reports. Reimbursable expenses in a multi-expense report show no label.
|
| // "Reimbursable" is only shown for single-expense reports, where it replaces the hidden report-level breakdown. | ||
| if (isTransactionOnReport && !isCurrentTransactionReimbursable) { | ||
| amountDescription += ` ${CONST.DOT_SEPARATOR} ${Str.UCFirst(translate('iou.nonReimbursable'))}`; | ||
| } else if (isSingleTransactionReport(moneyRequestReport, visibleParentReportTransactions)) { |
There was a problem hiding this comment.
@MelvinBot Could you make it consistent so that reimbursable is displayed in both cases?
There was a problem hiding this comment.
This is done in 21f77f3, which is the current PR head. The Amount field now shows "· Reimbursable" or "· Non-reimbursable" for every expense on a report, in both single- and multi-expense reports (MoneyRequestView.tsx:795-797).
The web test passed for both report types. The results and screenshots are in this comment.
| // Flag non-reimbursable expenses on any report so single- and multi-expense reports stay consistent. | ||
| // "Reimbursable" is only shown for single-expense reports, where it replaces the hidden report-level breakdown. |
There was a problem hiding this comment.
@MelvinBot o comment needed
| // Flag non-reimbursable expenses on any report so single- and multi-expense reports stay consistent. | |
| // "Reimbursable" is only shown for single-expense reports, where it replaces the hidden report-level breakdown. |
There was a problem hiding this comment.
Removed both comment lines in a437f1e. This only deletes comments, so the code does exactly what it did before.
I didn't change the label logic in this run. Your other comment about showing "Reimbursable" in both cases is handled separately.
…enseNonReimbursableRule Co-authored-by: cretadn22 <[email protected]> # Conflicts: # tests/unit/PolicyUtilsTest.ts
|
I merged The only conflict was in
Compared with |
Co-authored-by: cretadn22 <[email protected]>
…rts too Co-authored-by: cretadn22 <[email protected]>
|
I pushed 21f77f3. The Amount field now shows "· Reimbursable" or "· Non-reimbursable" for any expense on a report, in both single- and multi-expense reports.
🤖 Web test steps verification results: ✅ pass On web, the Amount label shows the reimbursable status for every expense on a report, whether the report has one expense or two.
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-10-21.at.12.04.56.movAndroid: mWeb ChromeScreen.Recording.2026-10-21.at.11.31.22.moviOS: HybridAppScreen.Recording.2026-10-21.at.12.04.13.moviOS: mWeb SafariScreen.Recording.2026-10-21.at.11.27.23.movMacOS: Chrome / SafariScreen.Recording.2026-10-21.at.11.24.39.mov |
MelvinBot
changed the title
|
I updated the PR title and description:
The |
cretadn22
left a comment
cretadn22
left a comment
There was a problem hiding this comment.
More discussion in this thread
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #101587 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
@cretadn22 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] |
|
🚧 puneetlath 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/puneetlath in version: 9.5.16-0 🚀
|
|
No help site update needed. This PR is a bug fix that makes the Cash expenses rule work as the Workspace Rules and Split Expenses articles already describe, and no article sentence becomes incorrect. |
|
🚀 Deployed to staging by https://github.com/puneetlath in version: 9.5.18-0 🚀
|
|
No help site update needed. This PR is a bug fix that makes the Cash expenses rule work the way the Workspace Rules article already describes it, and no article mentions the Reimbursable label next to the amount. Why
|
5 participants
Explanation of Change
"Cash expenses are always non-reimbursable" was not enforced on workspaces with no stored
defaultReimbursable. The Rules page treated the missing value asfalseand showed the rule as on. Expense creation treated it astrueand created reimbursable expenses. The Reimbursable toggle was also hidden in that case, so nobody could fix the expense.This PR:
defaultReimbursableas reimbursable everywhere.getCashExpenseReimbursableModenow uses?? true, and the newgetDefaultReimbursableForPolicyinIOUUtilsis the single source for the creation-time default.MoneyRequestView,DynamicSplitExpenseEditPage, and the confirmation footer.defaultReimbursable: undefinedwith eachdisabledFields.reimbursablevalue.Out of scope: the backend does not re-check this rule when an expense is created. That fix is outside this repo.
Fixed Issues
$ #101587
PROPOSAL: #101587 (comment)
Tests
Same as QA Steps.
Offline tests
QA Steps
Preconditions: As a workspace admin in a Collect/Control workspace:
Steps to Reproduce:
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)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