fix: Reject offline new report#70960
Conversation
…ct-offline-new-report
src/libs/actions/IOU.ts
Outdated
| // Add all report actions to the new report | ||
| optimisticData.push({ | ||
| onyxMethod: Onyx.METHOD.SET, | ||
| key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${rejectedToReportID}`, | ||
| value: newReportActions, | ||
| }); |
There was a problem hiding this comment.
I think the newReportActions should be added to the transaction thread rather than the expense report.
With the expense report, we only add CREATED action and IOU action, which corresponds with the rejected transaction.
For reference, you check check what BE returns in online, so we can handle the optimistical data properly in offline.
…ct-offline-new-report
…ct-offline-new-report
…ct-offline-new-report
src/libs/actions/IOU.ts
Outdated
| key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${rejectedToReportID}`, | ||
| value: newReportActions, |
There was a problem hiding this comment.
I saw that you pushes the: rejected this expense action, rejection reason comment, moved this expense action to the new expense report. Does it match the data returned by BE?
Codecov Report❌ Patch coverage is
... and 8 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
|
What's the deal here now. Is @truph01 reviewing? |
|
@truph01 took over the PR as implementer in my absence as I was out. I am partially back and hence reviewing this PR. I reviewed this PR yesterday. Now going to test it. |
|
I tested a few scenarios and it works fine. Here's a happy path. web-offline-reject-2.movOne question if we keep rejecting the expenses offline, instead of adding it to the new offline report we are creating two new offline reports. I can see BE also returns 2 offline reports. So I think this behavior is fine? web-offline-reject-1.mov |
|
@truph01 I was testing the actions for the new offline report. May be some of these are not related to the PR. But following actions didn't work.
web-offline-reject-menu-actions.movOne more question (even I had this when I was working on it), the offline report title format is: |
@garrettmknight is this to do with the ASAP submit rollout perhaps? 🤔 If an
Can you confirm which ones are related, so we focus on them if they're blocking the PR?
I think this is a different project, coming with moving custom report title formulas to Auth. |
The steps look fine. Let me create a new workspace and then try. |
@mananjadhav I just tested, and it looks like this behavior has also appeared in prod. So I think we can skip it in this PR |
I fixed this bug via this commit. |
|
@mananjadhav Have you had a chance to review this one? |
|
+1, can we please keep this moving? Thanks! P.S @truph01 conflict now! |
|
I tested the flow again for web all the scenarios. All the scenarios are fixed except for the 2 offline rejects. I think once this is fixed, we're good with the PR. Rest all the I tested and seem to be fixed. @truph01 Now the preview shows correctly with 2 offline expenses, but when I open the report, it shows only the first rejected expense. web-rejected-multiple-offline-expense.mov |
@mananjadhav I just posted a commit to fix it. Result: Screen.Recording.2025-10-30.at.16.04.02.mov |
|
I can be online in 2 hours to test this. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid-offline-reject.movAndroid: mWeb Chromemweb-chrome-offline-reject.moviOS: HybridAppiOS: mWeb Safarimweb-safari-offline-reject.movMacOS: Chrome / Safariweb-offline-reject.movMacOS: Desktopdesktop-offline-reject.mov |
|
@truph01 The recent changes has caused some issues. The new optimistic report is empty. I also DMed you over the weekend. desktop-reject-offline.movmweb-safari-reject-offline.movweb-offline-reject.mov |
|
The recent fixes worked. Testing again all platforms. |
mananjadhav
left a comment
There was a problem hiding this comment.
I have issues with my iOS build. But I retested on all the other platforms. I think it's good to merge.
|
Great work on this one, thank you both @truph01 @mananjadhav! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/lakchote in version: 9.2.43-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.2.43-2 🚀
|
| total: (movedToReport?.total ?? 0) - transactionAmount, | ||
| }, | ||
|
|
||
| const [, , iouAction, ,] = buildOptimisticMoneyRequestEntities({ |
There was a problem hiding this comment.
We need to set createdIOUReportActionID with reportActionID of the iouAction here as well, otherwise it will cause FE sends empty value for createdIOUReportActionID, hence BE re-generate another createdIOUReportActionID, then cause duplicate report preview. More details here #76982 (comment)
| { | ||
| onyxMethod: Onyx.METHOD.SET, | ||
| key: `${ONYXKEYS.COLLECTION.REPORT}${rejectedToReportID}`, | ||
| value: { | ||
| pendingFields: null, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Coming from #78577 checklist: we should update pendingFields using the MERGE method instead of SET the entire object.
Explanation of Change
Fixed Issues
$ #70569
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 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
Screen.Recording.2025-10-23.at.00.22.27.mov
Android: mWeb Chrome
Screen.Recording.2025-10-23.at.00.18.36.mov
iOS: Native
I encountered an error when building IOS. I will add the video later once it is fixed.
iOS: mWeb Safari
Screen.Recording.2025-10-23.at.00.17.25.mov
MacOS: Chrome / Safari
Screen.Recording.2025-10-22.at.23.48.22.mov
MacOS: Desktop
Screen.Recording.2025-10-23.at.00.14.37.mov