-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[CP Staging] Fix buildOptimisticIOUReportAction returning an existing reportActionID when it should generate a new one #73041
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ID when it should generate a new one
|
LGTM |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-10-23.at.01.00.59.movAndroid: mWeb ChromeScreen.Recording.2025-10-23.at.00.57.49.moviOS: HybridAppI have error when building IOS app. I will add video later once it is fixed iOS: mWeb SafariScreen.Recording.2025-10-23.at.00.57.01.movMacOS: Chrome / SafariScreen.Recording.2025-10-23.at.00.51.35.movMacOS: DesktopScreen.Recording.2025-10-23.at.00.59.41.mov |
|
@chuckdries we have ESLint errors, could you fix them? |
| isAttachmentOnly: false, | ||
| originalMessage, | ||
| reportActionID: linkedExpenseReportAction?.reportActionID ?? reportActionID ?? rand64(), | ||
| reportActionID: reportActionID ?? rand64(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chuckdries Can we apply the new change only when isMovingTransactionFromTrackExpense is true? Something like:
| reportActionID: reportActionID ?? rand64(), | |
| reportActionID: isMovingTransactionFromTrackExpense(action) ? rand64() : (linkedExpenseReportAction?.reportActionID ?? reportActionID ?? rand64()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually would prefer to use the change I committed because it restores the behavior to what we know worked before the PR that broke it, and the functionality of that PR works fine in my testing with my change applied. Paging @ZhenjaHorbach - can you provide more insight into why you added linkedExpenseReportAction?.reportActionID ?? here, and whether it's ever necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just read the description of the issue, and yes, I think using reportActionID from linkedExpenseReportAction was a bad idea, since it affected not only the split flow
However, I believe we should still pass reportActionID from saveSplitTransactions to avoid potential issues with IOUReportActions — although, to be honest, I don’t quite remember what exactly a random reportActionID could cause
And I didn't use reportActionID since this PR was being developed in parallel with my PR, where reportActionID parameter was added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, given the description @chuckdries provided and the testing that works, I believe we should be able to proceed and maintain the existing behavior before #63738.
Given it affects real customers in production, I think we want to CP it to staging and production:

|
The eslint errors are for translateLocal being deprecated, which we just added yesterday. They're irrelevant to this PR, but no PR will be allowed to modify this file until we eslint-disable each of these calls, so I'll go ahead and do that here to unblock us. |
…isticIOUReportAction-improperly-reuses-reportActionID
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
😮💨 the codecov affected lines are only counted as changes because of the eslint comments, no actual coverage changes occurred with this PR |
…isticIOUReportAction-improperly-reuses-reportActionID
|
LGTM |
|
🚧 @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, Desktop, and Web. Happy testing! 🧪🧪
|
|
@Beamanator 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] |
|
Regression is completed. The issue was fixed in the current build. |
|
Proceeding with the merge, as we need to fix this in production as soon as possible. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
…UReportAction-improperly-reuses-reportActionID [CP Staging/Production] Fix buildOptimisticIOUReportAction returning an existing reportActionID when it should generate a new one (cherry picked from commit 173dee9) (cherry-picked to staging by mountiny)
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.2.36-6 🚀
|
1 similar comment
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.2.36-6 🚀
|
|
@lakchote do we need to check this PR in stg build too? |
|
@IuliiaHerets yes please. Just do not test on Android as the build failed due to external reasons for now. |
|
@lakchote QA steps are passed on Desktop, Windows 10/Chrome, Android 16/Chrome and iPhone 13/iOS 18.6, build v9.2.36-6 11.12.41.mp4az_recorder_20251023_152611.mp4IMG_3521.MP42025-10-23.11.17.00.mp42025-10-23.11.15.25.mp42025-10-23.11.14.15.mp4 |
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.2.36-6 🚀
|
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.2.36-7 🚀
|
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.2.37-1 🚀
|
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.2.37-8 🚀
|
Explanation of Change
This change reverts this line of #63738. ConvertTrackedExpenseToRequest and AddTrackedExpenseToPolicy both expect the reportActionID that is returned from
buildOptimisticIOUReportActionto not exist yet, it's supposed to be optimistic. Both of those commands fail in their happy paths with a unique constraint violation because this function was returning an existing reportActionID rather than a fresh one.CC @lakchote @ZhenjaHorbach @eh2077 Since y'all worked on the PR that introduced this change, would appreciate an extra pair of eyes to make sure reverting this line doesn't break any functionality that I'm not aware of
Fixed Issues
$ #72536
$ https://github.com/Expensify/Expensify/issues/555438
$ https://github.com/Expensify/Expensify/issues/555351
PROPOSAL:
Tests
AddTrackedExpenseToPolicy
AddTrackedExpenseToPolicyrequest does not say400 Unique Constraints Violationin the preview tabAddTrackedExpenseToPolicy.fixed.mp4
ConvertTrackedExpenseToRequest
ConvertTrackedExpenseToRequestrequest does not say400 Unique Constraints Violationin the preview tabConvertTrackedExpenseToRequest.fixed.mp4
Split expenses flow
Note: I'm copying these instructions from the PR that introduced this bug. I performed them locally, except they mention enabling the
newDotUpdateSplitsbeta, which I couldn't find any code at all that references that, so I skipped that step.Split.flows.work.mp4
Offline tests
N/A
QA Steps
Submit expense from selfDM to workspace, confirm that the expense actually moves
Submit expense from selfDM to another person, confirm that the expense actually moves
Split Expenses flow
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
MacOS: Desktop