Fix issue with Split reverting was not successful for expense with negative amount#84705
Conversation
|
@ShridharGoel 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✅ Changes either increased or maintained existing code coverage, great job!
|
trjExpensify
left a comment
There was a problem hiding this comment.
PR for a WN project. 👍
Reviewer Checklist
Screenshots/VideosAndroid: mWeb ChromeiOS: HybridAppScreen.Recording.2026-04-10.at.6.21.24.PM.moviOS: mWeb SafariScreen.Recording.2026-04-10.at.6.40.42.PM.movMacOS: Chrome / SafariScreen.Recording.2026-04-10.at.6.38.23.PM.mov |
|
Bug: Mixed-sign negative splits are still validated with signed comparisons in Screen.Recording.2026-03-12.at.12.21.35.AM.mov |
When original amount is negative and the last split is positive But in case the last split is negative and bigger than the original(which is also negative) |
|
Actually, we have a BE issue CC:@lakchote 2026-03-11.20.53.34.mov |
|
Still waiting BE fixes |
|
this should be fixed now @ZhenjaHorbach |
|
@ShridharGoel |
|
Bug: Shows -43 as being greater than -22 (probably comparing absolute values) Screen.Recording.2026-03-29.at.6.57.31.PM.mov |
Oh @lakchote @garrettmknight @trjExpensify On one side, we need to show less than the original value error (-43 < -22) |
@lakchote @garrettmknight @trjExpensify |
|
Based on the earlier decision in this comment and the backend fix that followed, same-sign negative totals are validated by magnitude, so -43 vs -22 is treated as 43 > 22, not as a -43 < -22 “less than” case. The current behavior seems correct, but the copy is confusing for negative totals. If we want clearer wording, I’d suggest something specific for them, like "Total split amount exceeds the original expense amount". Thoughts @garrettmknight? |
Yes |
I'm not convinced we need to update the copy:
I'm not sure making it more general will actually help in this case, and I'm not sure how we could be more specific. |
At the moment, we show |
|
Looks like it's @MelvinBot can you confirm the behavior for reverting a split in general is:
|
|
Yes, that flow is correct. When you edit a split and remove all splits except one, the app detects this as a "reverse split operation" and calls The detection logic is in So the expected behavior for those steps is:
This is the same behavior on staging, prod, and in this PR's branch. |
|
Melv is wrong - we don't revert to the original expense amount, nor is that the intention. So I think the behavior is right from my testing on prod and staging. The thing we need to change is any test that indicates the amount should be changed. Does that make sense @ZhenjaHorbach ? Can you test and confirm you're seeing the same as me in this comment? |
I'm confused 😅 We show the original amount. Ex. -$10 As a result, we just show the warning, but we are able to revert, and the reverted transaction will have -$5 amount CC: @lakchote |
|
Oh, now I get what you're saying about the abs values.
This makes sense then. The copy stays the same, but you're able to save the edit/revert the expense. |
So in this case, we will show |
Yeah, I think that's right. |
|
Okay then |
|
In this case, I need to make a few improvements |
|
Okay But found a new one when we added a new split 2026-04-07.16.39.16.mov |
|
@ShridharGoel |
|
Noticed that after the revert, the expense shows as a duplicate. Maybe we need to check this from the backend side? |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #82575 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
Probably yes |
|
@ZhenjaHorbach can you please add qa steps? |
stitesExpensify
left a comment
There was a problem hiding this comment.
LGTM but there is one issue in the errors and there are no QA steps
| }); | ||
|
|
||
| describe('mixed-sign splits (some negative, positive total)', () => { | ||
| it('returns "greater" warning when difference > 0', () => { |
There was a problem hiding this comment.
This is incorrect right? We are returning "LESS"?
|
@ZhenjaHorbach can you clarify why you decided to remove the test instead of just changing it to greater? |
Because we already have 2 tests for less and greater for the mixed-sign splits cases |
|
🚧 @stitesExpensify 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. |

Explanation of Change
Fixed Issues
$ #82575
PROPOSAL:
Tests
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 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
2026-03-10.10.50.28.mov