Repository navigation
Add retry button and logic - #99902
Conversation
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
|
|
||
| // These carry state the transaction does not store - GPS points, rates, units - so they are excluded rather | ||
| // than retried with those fields silently dropped. |
There was a problem hiding this comment.
❌ CONSISTENCY-15 (docs)
This comment sets off a parenthetical with spaced hyphens used as em dashes (does not store - GPS points, rates, units - so they). Comments should read as plain sentences without dash-delimited asides.
Rewrite without the dashes, e.g.:
// A distance, per diem, or time request carries state the transaction alone does not store, such as GPS points,
// rates, and units, so it is excluded rather than retried with those fields silently dropped.Reviewed at: f9367e2 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9367e2787
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 668eeed463
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }, | ||
| policyParams, | ||
| transactionParams: { | ||
| amount: transaction.amount ?? 0, |
There was a problem hiding this comment.
For a nonzero workspace expense, the persisted transaction stores amount with the expense-report sign, while requestMoney() expects the original input sign and negates it again in MoneyRequestBuilder.ts:1545 before writing Onyx. Passing transaction.amount unchanged therefore sends a negative amount to RequestMoney and replaces the local transaction with a positive amount; an ALREADY_CREATED response only applies success data and does not restore that value, while repeated failed retries alternate the sign. Reverse the stored expense-report sign when rebuilding both amount and taxAmount.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed by reading the transaction back through getTransactionDetails, which resolves the expense-report sign for both amount and taxAmount and is the same helper that duplicate uses to rebuild a request from an existing transaction
| participant, | ||
| }, | ||
| policyParams, | ||
| transactionParams: { |
There was a problem hiding this comment.
When receipt upload fails while submitting an existing tracked expense, the failed operation is the action === SUBMIT branch that calls convertTrackedExpenseToRequest, but this payload omits action, linkedTrackedExpenseReportAction, and linkedTrackedExpenseReportID, so the retry enters the ordinary RequestMoney branch instead. Because the tracked transaction already exists under the reused ID, this can be treated as already created without ever converting/submitting it. Unlike the already-fixed TRACK_EXPENSE case, fresh evidence is that MoneyRequestBuilder.ts:1066 tags failures from this conversion path as MONEY_REQUEST, so the new render gate still offers the retry; preserve the conversion parameters or exclude this flow.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I would say its out of scope here - the gate already excludes any tracked expense whose receipt reached the server, since that leaves a remote receipt.source, so the remaining window is a tracked expense whose receipt never uploaded at all. The conversion path needs its own handling - we might work on follow up on that later
|
Translations updated, @brunovjk what's your status? thanks |
|
The "add payment card" modal is stuck; I click to dismiss it, but it keeps coming back: Screen.Recording.2026-09-24.at.10.53.27.movThe strange thing is that after pulling that PR and switching back to |
|
@brunovjk I don’t see it on my side. This PR doesn’t touch any payment card or billing code. Have you tried a fresh account, or signing out and clearing the app cache? |
| } | ||
|
|
||
| retryReceiptUpload(receiptRetryContext, () => | ||
| clearReceiptUploadError({ |
There was a problem hiding this comment.
Could we consider clearing the receipt error only after requestMoney() succeeds? I'm wondering if clearing it in onBeforeDispatch could leave the user without the retry option if the dispatch fails synchronously. Is that a possible edge case here? Thanks.
There was a problem hiding this comment.
Good point. Now the error is cleared only after requestMoney() is called. If building the request throws, the receipt error stays and the user still has Try again and Save. I added a test for that.
If the retry fails on the server, failureData writes a new receipt error, so Try again comes back anyway.
|
One small style question: should |
Yes. I created another simulator, and it worked. It worked well in the initial tests; I'll test it on the other platforms later. 99902_android_native.mov |
|
@brunovjk Good catch, I removed it by mistake. Added it back, and also the one in |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp99902_android_native.movAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari99902_web_chrome.mov |
There was a problem hiding this comment.
LGTM.
NAB: @BartekObudzinski @rinej, should we update the PR description to clarify that the Try again button should not appear on Web/mWeb? Thank you!
|
@MelvinBot please update the PR description to clarify that the Try again button should not appear on Web/mWeb |
|
Updated the PR description. The Explanation of Change section now states that Try again is native only (iOS and Android) and never appears on Web or mWeb. It also adds:
I left the Tests and QA sections unchanged. |
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Thanks for pushing this ahead, looks in a good shape now
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.95-0 🚀
|
|
Yes, the help site needs one update. I opened a draft PR for it: Docs updates for E/A#99902 Update receipt upload failure FAQ for Try again The "What should I do if my receipt upload fails?" FAQ in Attach-and-edit-receipts-on-expenses.md:173 still showed the old message and the Save receipt and Delete expense buttons. The update:
Verification notes
@rinej, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
Deploy Blocker #102494 was identified to be related to this PR. |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.4.95-4 🚀
|
11 participants
Explanation of Change
When a receipt upload fails,
SequentialQueueappliesfailureDataand drops the request, so the expense is stranded with no way to resend it.This PR adds a Try again action to the receipt error row. It rebuilds the original
RequestMoneycall from what the failure left in Onyx — the transaction, its report, and the file still on disk — and dispatches it again under the sametransactionID, so a retry resends the same expense instead of creating a duplicate. On success the error is cleared and the row goes away.Following the design feedback in this PR, the error row was also simplified:
Upload failed. Try again or save for later.Receipt · Issue foundheader above it already shows one.Try again is native only (iOS and Android). It never appears on Web or mWeb. On native, the receipt is kept on the device in
ReceiptStorage, so it can be resent even after a restart. On Web and mWeb, the receipt only exists as a blob URL in the open tab and is gone after a reload, so there is nothing to resend.isRetrySupportedisfalseon web, so the row there always shows Save on its own.On native, Try again is also rendered only when the upload can actually be replayed.
canBuildRetryPayloadreturnsfalsefor distance, per diem, and time requests (they carry state the transaction alone does not store, such as waypoints, rates, and units), for track expenses and replaced receipts, for test receipts, and when the receipt source is no longer a local file.Whenever the row shows Save on its own (always on Web/mWeb, and in the native cases above), the copy is
Upload failed. Save your receipt to keep it., so the message never mentions a button that isn't there.Fixed Issues
$ #100846
PROPOSAL:
Tests
On iOS:
Offline tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
iOSWithNetworkFailing.mov
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari