Fix error on Android when submitting a tracked expense #54628
Conversation
Reviewer Checklist
Screenshots/VideosAndroid: Nativeabc.mp4abc2.mp4Android: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
|
@CyberAndrii can you plz merge main |
…on-android-when-submitting-an-expense
@Pujan92 merged and tested again |
There was a problem hiding this comment.
One minor comment @CyberAndrii, otherwise looks good to me.
src/libs/HttpUtils.ts
Outdated
| if (Array.isArray(value)) { | ||
| if (value.every(isValid)) { | ||
| return; | ||
| } | ||
| } else if (isValid(value)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
| if (Array.isArray(value)) { | |
| if (value.every(isValid)) { | |
| return; | |
| } | |
| } else if (isValid(value)) { | |
| return; | |
| } | |
| if (isValid(value)) { | |
| return; | |
| } |
I think simply isValid(value) should be fine here, otherwise it can be a nested-level array too.
There was a problem hiding this comment.
Added support for nested arrays.
There was a problem hiding this comment.
Sorry but I was thinking of only do the validation for TopLevel, can you plz revert to the previous one where you applied it to one level bottom? Bcoz instead of making this complex, prev one seems to be the suitable for mostly all requests.
There was a problem hiding this comment.
I think it is fine as it is now, as it covers all possible cases. Note that the isTopLevel param wasn't added specifically for nested arrays. Files are not allowed within arrays, so even if we make it one level deep, it will be pretty much the same code just a bit reordered
function validateFormDataParameter(command: string, key: string, value: unknown) {
// eslint-disable-next-line @typescript-eslint/no-shadow
const isValid = (value: unknown) => value === null || typeof value !== 'object';
// Native platforms only require the value to include the `uri` property.
// Optionally, it can also have a `name` and `type` props.
// On other platforms, the value must be an instance of `Blob`.
// eslint-disable-next-line @typescript-eslint/no-shadow
const isValidTopLevel = (value: unknown) => isValid(value) || isNativePlatform ? 'uri' in value && !!value.uri : value instanceof Blob;
if (Array.isArray(value)) {
if (value.every(isValid)) {
return;
}
} else if (isValidTopLevel(value)) {
return;
}
// eslint-disable-next-line no-console
console.warn(`An unsupported value was passed to command '${command}' (parameter: '${key}'). Only Blob and primitive types are allowed.`);
}
| ...reportInformation, | ||
| ...policyParams, | ||
| ...transactionParams, | ||
| linkedTrackedExpenseReportAction: undefined, |
There was a problem hiding this comment.
Why do we need this change?
There was a problem hiding this comment.
It was accidentally added when that code was refactored to use the spread operator. See the discussion in https://github.com/Expensify/App/pull/52221/files#r1880255805. It will cause an error if we don't unset it because it is an {} object.
Native platforms only require the value to include the `uri` property. Optionally, it can also have a `name` and `type` props. On other platforms, the value must be an instance of `Blob`.
There was a problem hiding this comment.
@CyberAndrii Only a partial revert based on this comment I expect, otherwise LGTM!
|
Great job! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
CyberAndrii
deleted the
45086-fix-error-on-android-when-submitting-an-expense
branch
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.0.84-0 🚀
|
|
Failing on android with original bug id #45086 |
I am unable to reproduce this on Also, I think this might be unrelated to this PR as the error in #45086 appears after approximately 40 seconds, whereas in this video it appears instantly. |
|
Agree with @CyberAndrii as it seems to be the other issue. Logs will help here @kavimuru |
|
FYI: #54358 (comment) |
|
Logs |
|
Ok, managed to reproduce with this repro steps:
VideoScreen.Recording.2025-01-13.at.21.24.45.mov`ConvertTrackedExpenseToRequest` responseIt appears that some information is missing on the frontend because the chat was never opened. So I think we should create a new issue to address this bug. And for testing #45086 - make sure to pre-open the chat with the user to whom you’re submitting the expense. cc @izarutskaya |
|
Created new issue #55199 |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.84-7 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.84-7 🚀
|
Explanation of Change
Fixes an error on Android that appears when tracking a Distance expense, submitting a tracked expense to someone, categorizing it, or sharing it with an accountant.
It was caused by sending the receipt as an object
{ ... }(not aFile/Blob), which is not supported byXMLHttpRequest/FormData.Fixed Issues
$ #45086
PROPOSAL: #45086 (comment)
Tests
Same as QA Steps
Offline tests
N/A
QA Steps
Unexpected error submitting/deleting this expenseerror appears on the expensesUnexpected error submitting/deleting this expenseerror appears on the expensesNote: If you see
Receipt scan incomplete. Please verify details manually.error, it is related to#App/53839PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))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
45086.android.native.mp4
Android: mWeb Chrome
45086.android.chrome.mp4
iOS: Native
45086.ios.native.mp4
iOS: mWeb Safari
45086.ios.safari.mp4
MacOS: Chrome / Safari
45086.macos.safari.mov
MacOS: Desktop
45086.macos.desktop.mov