[CP Staging] Fix distance split system message showing generic text#83542
Conversation
Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
|
@MelvinBot Mark it as ready |
|
Done! PR has been marked as ready for review. ✅ |
melvin-bot
Bot
requested review from
trjExpensify
and removed request for
a team and
trjExpensify
|
@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] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-02-26.at.2.29.11.PM.movAndroid: mWeb ChromeScreen.Recording.2026-02-26.at.2.30.52.PM.moviOS: HybridAppScreen.Recording.2026-02-26.at.2.13.47.PM.moviOS: mWeb SafariScreen.Recording.2026-02-26.at.2.23.25.PM.movMacOS: Chrome / Safari
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const hasModifiedCreated = 'created' in transactionChanges; | ||
| const hasModifiedAmount = 'amount' in transactionChanges; | ||
| // For split transactions, the merchant and amount are already computed in transactionChanges, | ||
| // so we can build a valid optimistic MODIFIED_EXPENSE even when waypoints are pending. |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The expression 'amount' in transactionChanges is already captured as hasModifiedAmount on line 4560, just two lines above. Reusing the existing variable improves consistency with the surrounding code pattern and avoids duplication.
Suggested fix:
const hasSplitDistanceMessageFields = !!isSplitTransaction && 'merchant' in transactionChanges && hasModifiedAmount;Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Good catch — fixed in 487427c. Now reuses hasModifiedAmount instead of duplicating 'amount' in transactionChanges.
There was a problem hiding this comment.
@MelvinBot Let's update based on the above, and we can put 'merchant' in transactionChanges in a variable as well.
There was a problem hiding this comment.
Done in 6b5f5d3. Extracted 'merchant' in transactionChanges into a hasModifiedMerchant variable alongside the other hasModified* declarations, and removed the duplicate definition that was further down in the same function.
There was a problem hiding this comment.
This was already addressed — hasModifiedMerchant was extracted as a variable at line 4561 (alongside the other hasModified* declarations), and the duplicate declaration further down was removed. The hasSplitDistanceMessageFields line now reads:
const hasSplitDistanceMessageFields = !!isSplitTransaction && hasModifiedMerchant && hasModifiedAmount;Both hasModifiedMerchant and hasModifiedAmount are reused from the existing variable declarations rather than using inline 'merchant' in transactionChanges / 'amount' in transactionChanges expressions.
|
Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
Moves the 'merchant' in transactionChanges check into a named variable alongside the other hasModified* declarations, and removes the duplicate definition that was further down in the same function. Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
|
@ShridharGoel I already have a PR with split fixes |
- Added unit tests verifying the hasSplitDistanceMessageFields logic: regular distance expenses skip optimistic report action when waypoints are pending, split transactions with merchant+amount create it, and split transactions missing merchant or amount correctly skip it. - Updated the Step 3 comment in getUpdateMoneyRequestParams to reflect the split transaction exception for waypoint updates. Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
…sage' into claude-fixDistanceSplitSystemMessage Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
|
Addressed both items: 1. Tests for the split-distance waypoint branch Added
These tests verify the 2. Updated the "we're updating the waypoints" comment The comment at the top of Step 3 in
And two additional lines were added explaining why split transactions are the exception:
|
|
@ZhenjaHorbach Oh, but I've already reviewed and created recordings/screenshots also. Maybe let's handle this one here itself? |
Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
|
Anyways, this one also checks using a better condition + has tests now. What do you think? |
|
Fixed the failing |
OnyxEntry<T> is defined as T | undefined (not T | null), so passing null causes TypeScript errors. Replace null with undefined for policy, policyTagList, policyCategories, and iouReportNextStep in all test cases. Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
|
Fixed the failing TypeScript check. The issue was that The previous Prettier failure was already fixed by the earlier commit. |
|
The failing check was This was already fixed in commit |
lakchote
changed the title
…mMessage [CP Staging] Fix distance split system message showing generic text (cherry picked from commit e75afbb) (cherry-picked to staging by puneetlath)
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Cherry-picked to staging by https://github.com/puneetlath in version: 9.3.26-6 🚀
|
|
🚧 @lakchote 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 production by https://github.com/puneetlath in version: 9.3.26-8 🚀
|
|
🚀 Cherry-picked to staging by https://github.com/puneetlath in version: 9.3.27-0 🚀
|
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.3.27-8 🚀
|
5 participants
Explanation of Change
When editing waypoints on a distance split expense, the optimistic
MODIFIED_EXPENSEreport action was being skipped becausegetUpdateMoneyRequestParamsdetected pending waypoints (hasPendingWaypoints = true) and deferred to the server to create the action after MapBox processing. This works for regular distance expenses but not for split child transactions, where theSplitTransactionAPI does not generate the distance-specificMODIFIED_EXPENSEaction. As a result, the system message showed a generic "changed the expense" instead of the detailed distance message.This fix adds a
hasSplitDistanceMessageFieldscheck that allows the optimisticMODIFIED_EXPENSEto be created when:isSplitTransactionis true)merchantandamountare present intransactionChangesFor split transactions, the merchant (distance string like "10.00 mi @ $0.70 / mi") and amount are already computed in
transactionChangesfrom the split data, so valid optimistic data IS available. The existinggetModifiedExpenseOriginalMessageandModifiedExpenseMessage.getForDistanceRequestwill then generate the correct detailed message.Fixed Issues
$ #83500
PROPOSAL: #83500 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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