Repository navigation
Conversation
…stance-rate # Conflicts: # src/components/SelectionList/ListItem/SplitListItem.tsx # src/libs/PolicyDistanceRatesUtils.ts
… Split when the rate is on another workspace
…stance-rate # Conflicts: # src/pages/iou/DynamicSplitExpensePage.tsx
- Split a nonzero expense whose rate is now $0 with its historical rate - Give a $0 split of a positive rate expense no distance, and only accept $0 splits on a $0 rate - Put the remainder of a $0 rate distance on the first split, like Make splits even - Lock a split's amount only when the split itself is on a $0 rate - Look up the distance rate across workspaces before offering Split
…ility, edited amounts, distance error copy
…-rate # Conflicts: # src/components/MoneyRequestHeaderSecondaryActions.tsx # src/libs/ReportSecondaryActionUtils.ts
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7ed2e21a7
ℹ️ 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 isRateBeforeZeroRateChange = selectedRate.rate !== fallbackMileageRate.rate && (selectedRate.rate === 0 || fallbackMileageRate.rate === 0); | ||
| if (isRateBeforeZeroRateChange && customUnitRateID === fallbackMileageRate.customUnitRateID && isUsableSplitRate(fallbackMileageRate.rate)) { |
There was a problem hiding this comment.
Preserve historical rates for independently rated splits
When editing an existing distance split whose child was assigned a rate different from the parent, a later workspace change of that child rate to or from $0 is not handled: this condition only substitutes a historical rate when the child's ID equals the parent's fallback ID. A child created at $0 and later changed positive is removed from zeroRateSplitTransactionIDs, so its valid zero amount blocks every save; the reverse transition leaves a nonzero amount locked to a live $0 rate. Infer the historical rate from the child transaction/split's own amount and quantity rather than limiting the transition logic to the parent's rate ID.
Useful? React with 👍 / 👎.
|
Update: working on issues i found while I reviewed the code |
|
@FitseTLT can @marufsharifi begin the review? |
|
Nope still working |
|
@marufsharifi @NicolasBonet bugs 2 and 3 are edge cases where it is impossible to determine if an expense is really zero rate at the time of creation. Case 1 — exact-zero collision (BUG#2): Not fixable with this math: once the live rate is exactly $0 and the stored amount is also $0, "always was $0" and "was positive but rounded/changed to $0" produce identical inputs, so there's nothing left to distinguish them. Case 2 — fully-excluded collision (BUG#3): Also not fixable with this math: once Both need the rate value persisted at creation time rather than reconstructed from amount/quantity/rate after the fact. One workaround I found is parsing from the frozen merchant text of the transaction but I can't be 100% sure if it is a perfect solution. My question is if allowing splits for $0 amount distance expenses is the fact that we can split distances of the original transaction what if we allow spliting distance expense of zero amount with non-zero distance as a whole. This would simplify a lot of our code and be certain it will for all cases. Let me know your opinons otherwise I will apply the calculation from |
|
@marufsharifi back to you |
|
@FitseTLT, could you please merge the main. thanks. |
|
reviewing. |
|
@marufsharifi do you think you can complete the review today? |
|
@NicolasBonet, could you please share your thoughts here: #104756 (comment), thanks. |
|
@FitseTLT could you please elaborate or exemplify what this means?
|
|
Ok @NicolasBonet What I am saying is the logic to determine if the expense was created with 0 rate or not doesn't always work? Like for the cases of Bug 2 and 3 @marufsharifi reported. Workaround: the only workaround is to calculate from the merchant of the transaction but I am not sure how sustainable that fix is. Proposal: instead what I propose is because we are allowing split with zero amount based on the logic that the distances can be split why don't we allow split of 0-amount transactions whether they have a zero rate or not. WDYT |
|
I think that works, thoughts @trjExpensify ? |
Does that mean allow regular $0 expenses to be split? (CC: @garrettmknight @lakchote to this convo on split btw) |
|
@trjExpensify I read @FitseTLT's proposal as distance only. A $0 distance expense still has a distance to split, whether the rate was $0 or the amount was edited down to $0, so we don't have to tell those two cases apart. A regular $0 expense would normally split into parts that are all $0, so each part could only differ by category, tag or date. Creating those splits already works on the backend, it's only blocked in the app. Removing a part later can fail, though. If another part is already on an approved, paid or closed report and a third one can still be edited, what's left adds up to $0 and the backend refuses it. The app only shows "Unexpected error editing this expense. Please try again later." So if we allow it, that check needs to change too on the backend. |
@lakchote So does this change be made for Zero-amount distance expenses too. This issue is scoped on zero amount distance expenses. If I got your point correct other split fields can be updated when one is already approved. Like here 2026-10-30.19-39-26.mp4 |
Sorry @FitseTLT, I don't understand this sentence. |
In change, I meant the BE change that has to be made @lakchote is claiming; Should the BE change be made for distance expenses too or we can continue on working on the FE changes in this pr without BE change. But the main point is what should we do next here 😄 |
|
@FitseTLT no, the backend change isn't needed for distance expenses. The check that refuses remaining splits adding up to $0 skips them, so you can keep going with the App changes here. |
Explanation of Change
Lets workspaces use a $0 distance rate, for mileage that's tracked but not reimbursed.
validateRateValueaccepts 0 and only rejects negative rates.lowRateErrornow reads "Rate can't be negative".!ratebecomesrate === undefined). This removes the "Pending..." merchant, the false "Rate out of policy" error and the disabled rate field. A distance of 0 stays blocked.Saving a $0 split also needs the Auth change in https://github.com/Expensify/Auth/pull/25159. Without it,
SplitTransactionreturns402 Distance rate missing from policy.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/685239
PROPOSAL: N/A
Tests
0.-1...0.and saves as $0.00.0.Offline tests
QA Steps
Same as tests. Step 24 requires https://github.com/Expensify/Auth/pull/25159 to be deployed.
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