-
Notifications
You must be signed in to change notification settings - Fork 3.5k
fix: No message that expense is on hold when split expense offline #75980
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
|
|
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-12-02.at.8.09.43.PM.movAndroid: mWeb ChromeScreen.Recording.2025-12-02.at.8.14.03.PM.moviOS: HybridAppScreen.Recording.2025-12-02.at.8.09.17.PM.moviOS: mWeb SafariScreen.Recording.2025-12-02.at.8.14.35.PM.movMacOS: Chrome / SafariScreen.Recording.2025-12-02.at.7.54.05.PM.mov |
|
@paulnjs I think we'll also need backend fix for the issue. Can you help comment in the parent issue to answer the following questions? BE issue: transaction thread has duplicate hold message. StagingScreen.Recording.2025-11-25.at.7.47.01.PM.movThis PRScreen.Recording.2025-11-25.at.7.26.33.PM.mov |
|
@paulnjs In this PR, I think we should align offline behavior with BE - show on hold messages in transaction thread. |
trjExpensify
left a comment
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.
This PR doesn't look right to me. When you split a held expense:
- The "child" expenses should inherit the held status
- The hold system message and reason should be inside each of the transaction threads of the held expense.
It looks like that largely works on staging:
2025-11-25_12-49-05.mp4
The only bugs appear to be:
- A duplicate hold reason in the transaction thread on being split. (@lakchote is fixing that in the BE in this PR)
- The expense previews in the report preview component says "Review required" instead of "This expense was put on hold." even though that message appears to be within the character limit to show 🤔
|
@trjExpensify Thanks for the review! I agree all you mentioned, just one more offline issue - no hold system message and reason in transaction threads, see #75980 (comment) |
|
As in, copying the message to the child split expense threads when offline doesn't happen? Only when you come back online, do they appear? |
Yes, that's right, see staging video from #75980 (comment) |
|
@paulnjs Friendly bump |
|
@eh2077 Ya, i'm investigating. I will ping you when it's done |
|
@trjExpensify @eh2077 After testing more cases, I realized that BE will duplicate all messages from original expense to the split expenses, not just the hold system message or hold reason. Then duplicate the violation for each expense. If we want to create all messages for the split expenses optimistically, we need to pass all new actionIDs to BE. Is it possible? |
|
@paulnjs Do you mean we need BE change? If so, can you sort out following things?
|
It’s Transaction_Split API
On BE side, we will duplicate all messages from original expense to the split expenses, so if we want to do that optimistically, we have to generate the new reportActionIDs for each split expense in FE side. That means we need to pass them to the payload of Transaction_Split API
BE is returning the reportActions for each split correctly. What we need to do now is to generate the actions optimistically and pass them to BE. |
|
@paulnjs you can provide the optimistic hold report action ID and hold report action comment:
where For copied user comments, I recommend letting those sync from the server response rather than passing optimistic IDs - the complexity and race condition risks outweigh the UX benefit. If someone adds a comment to the original thread between when FE reads the number of comments and when the split executes, the ID mapping breaks. Comments added while offline would be missed. FE would need to know exactly how many comments exist before splitting. This data can easily be stale. Also it's different from hold actions, hold status directly affects what actions are available on the expense (can't submit, shows warning, etc). That's worth optimistic updates. Copied comments are just informational. |
|
@lakchote This is what I see in the
Could you help to confirm? And did BE handle this? |
|
@paulnjs, yes, this is correct. BE handles these params already. |
MarioExpensify
left a comment
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.
Awesome, thank you @paulnjs
|
@lakchote is ooo until Jan 2nd, we'll be able to merge once we get his approval. |
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.
LGTM, waiting for your approval now @trjExpensify
trjExpensify
left a comment
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.
👍
|
Thanks for the bump, @lakchote. Good to merge! |
|
🚀 Deployed to staging by https://github.com/lakchote in version: 9.2.95-0 🚀
|
[CP Staging] Revert "Merge pull request #75980 from paulnjs/paulnjs-fix/74682"
|
Reverted as it caused a blocker: #79004 |
* main: (267 commits) Update Mobile-Expensify submodule version to 9.2.95-2 Update version to 9.2.95-2 revert 6020367 fix wrong messages Update Mobile-Expensify submodule version to 9.2.95-1 Update version to 9.2.95-1 Revert "Merge pull request #75980 from paulnjs/paulnjs-fix/74682" fix for taxes Update Mobile-Expensify submodule version to 9.2.95-0 Update version to 9.2.95-0 Remove expense type from the amount row label in MoneyRequestView Prettier fix Fix after merge Add translations fix: remove redundant change fix iOS crash when attempting to insert an empty buffer fix: extract onPress handler to useCallback in ApprovalWorkflowEditor fixing long email displayed in the approver description fix: typecheck - category Revert changes in NumberWithSymbolForm ... # Conflicts: # src/languages/de.ts # src/languages/fr.ts # src/languages/it.ts # src/languages/ja.ts # src/languages/pt-BR.ts # src/languages/zh-hans.ts
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.2.95-5 🚀
|
|
🚀 Deployed to staging by https://github.com/lakchote in version: 9.2.96-1 🚀
|
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.2.96-6 🚀
|
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.2.96-6 🚀
|
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.2.96-6 🚀
|
|
🚀 Deployed to staging by https://github.com/lakchote in version: 9.2.97-0 🚀
|
|
🚀 Deployed to staging by https://github.com/lakchote in version: 9.2.99-0 🚀
|

Explanation of Change
Fixed Issues
$#74682
PROPOSAL:#74682 (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.mov
Android: mWeb Chrome
android_chorme.mov
iOS: Native
ios.mov
iOS: mWeb Safari
safari.mov
MacOS: Chrome / Safari
chorme.mov