Add freezing for Approved, Paid and/or Done splits during updating other splits - #100281
Add freezing for Approved, Paid and/or Done splits during updating other splits#100281ZhenjaHorbach wants to merge 9 commits into
Conversation
… statusNum to use actual statuses for splits
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.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d8a7c1e7e
ℹ️ 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".
| const itemTransaction = allTransactions?.[transactionKey] ?? searchResultsData?.[transactionKey]; | ||
| const reportKey = `${ONYXKEYS.COLLECTION.REPORT}${itemTransaction?.reportID}` as const; | ||
| const itemReport = allReports?.[reportKey] ?? searchResultsData?.[reportKey] ?? fallbackReport; | ||
| if (isSplitExpenseFrozen(itemReport)) { |
There was a problem hiding this comment.
Why don't we use a function like isSplitExpenseEditable in DynamicSplitExpensePage?
There was a problem hiding this comment.
I'm not sure about it
Since isSplitExpenseEditable is a broad permission gate for the whole flow
But we need a narrow rule: when the report is already approved/paid/closed and the amount is fixed
There was a problem hiding this comment.
Yeah, I'm not sure about it as well.
But if a split expense isn't editable while its amount can still change when I delete another split expense, that feels a bit odd.
@lakchote WDYT?
There was a problem hiding this comment.
But okay
I think we can use isSplitAction here
But I would prefer to leave isSplitExpenseFrozen guard here also, since isSplitAction short-circuits true for selfDM regardless of status, so it alone wouldn't catch a selfDM report that's somehow approved/settled/closed even if it's an edge case
|
I found a bug. I think this is a BE bug. The original transaction isn't available after "clear cache and restart" when the two split transactions are in separate reports. cc @lakchote 2026-09-08.09-31-06.mp4 |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da7368376e
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfe1710673
ℹ️ 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".
|
@MelvinBot review, find any potential bugs |
|
🤖 Reviewed the diff. The core approach (derive frozen-ness live from each split's own report instead of a cached 1. The snapshot fallback can silently miss, re-opening the exact bug this PR fixes
Failure: the split's report is missing from the Onyx 2. Per diem gets the same dead-end that the card guard exists to prevent
Failure: per diem expense, splits A (Approved) and B (open). Remove is shown on B. Removing it leaves A frozen at its old amount, sum < total → "total amount less than original" on Save, and there's nothing left to edit to fix it. The user is stuck. 3. The frozen split is still tappable and editable when its report only exists in the search snapshot
So in the scenario your new test adds (Approved report present only in the snapshot), the Approved split still renders as editable: the user can open it and hit Remove, which drops it from the draft entirely. Redistribution is frozen but the row isn't. Worth routing both through the same resolver. 4.
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c5655f293
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6cafbc58e5
ℹ️ 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".
| if (!liveTransaction) { | ||
| return original; | ||
| } | ||
| const liveAmount = hasValidModifiedAmount(liveTransaction) ? Number(liveTransaction.modifiedAmount) : (liveTransaction.amount ?? 0); |
There was a problem hiding this comment.
Normalize the live amount before restoring frozen splits
For a normal workspace expense, the transaction stored in Onyx uses the report-facing sign (for example, the new fixtures store amount: -5000), while the corresponding SplitExpense uses 5000. Copying the raw amount or modifiedAmount here therefore flips the frozen split negative whenever another split is added, removed, or edited. Cash splits below the original total are allowed through save validation, so this incorrect value can be submitted rather than merely producing a warning; restore the amount using the same sign conversion used by initSplitExpenseItemData/getTransactionDetails.
Useful? React with 👍 / 👎.
| allPolicies, | ||
| parentReport, | ||
| ); | ||
| const frozenSplitsContext = {frozenSplitTransactionIDs, searchResultsData: currentSearchResults?.data}; |
There was a problem hiding this comment.
Block saving a split after it becomes frozen
When the selected split's report becomes approved or paid while this per-split editor is open, the hook adds its ID to this context, but the context is only used by the Remove path. The Save button still calls updateSplitExpenseField without any frozen-ID guard, copying the dirty amount, tax, and other fields into the parent draft, after which the main save can update the finalized split. Fresh evidence after the earlier direct-edit guard is that updateSplitExpenseAmountField is now protected, while this separate updateSplitExpenseField path remains unprotected.
Useful? React with 👍 / 👎.
| if (!liveTransaction) { | ||
| return original; | ||
| } | ||
| const liveAmount = hasValidModifiedAmount(liveTransaction) ? Number(liveTransaction.modifiedAmount) : (liveTransaction.amount ?? 0); |
There was a problem hiding this comment.
Should we use getAmount here instead?
Explanation of Change
Fixed Issues
$ #99190
PROPOSAL:
Tests
Precondition:
Admin creates a manual expense in the workspace chat and splits it into 2 expenses
One of the splits is moved to a new report and Approved (or Paid)
Steps:
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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
2026-09-04.11.31.24.mov