Allow receipt recovery if the upload fails #29790
Conversation
Gonals
changed the title
melvin-bot
bot
requested review from
marcochavezf
and removed request for
a team
|
@marcochavezf 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] |
Deploying with
|
| Latest commit: |
0d010f9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://50255374.helpdot.pages.dev |
| Branch Preview URL: | https://alberto-recoverreceipt.helpdot.pages.dev |
Gonals
changed the title
|
I'm having some trouble uploading receipts on mobile, but this is ready for testing! |
Ok, just iOS native left, as my pods are missing the permissions module for reasons unknown 🤷 In any case, feel free to go ahead and test. This seems to work well across platforms! |
|
Ah, @Gonals.. I just realised that there's a small design addition when dismissing the error that we landed on with replace receipt flow where we're also wanting to do similar to not lose the receipt. It's the "are you sure?" modal confirmation on dismissing the error to be extra sure, illustrated here. @tienifr is working on a PR for that issue in the replace receipt flow to not lose the receipt there on replacing it. Not sure if he needs to wait until this is merged to re-use parts of it, and then maybe aslo include adding that additional modal in this flow as well actually? CC: @hoangzinh |
The don't need to wait on this. This flow is error-based, while theirs is simply adding the link in the modal, so they don't really intersect. Similarly, we can't really add the modal here. I |
src/libs/actions/IOU.js
Outdated
| * @param {Object} transaction | ||
| * @returns {Object} | ||
| */ | ||
| function getReceiptError(transaction) { |
There was a problem hiding this comment.
The idea is to also use the same method for the 2 last blocks (line 1436 and 1461)
There was a problem hiding this comment.
Yep. I'm having issues testing today (onyx is doing weird stuff). I'll try again tomorrow!
| @@ -585,6 +585,9 @@ export default { | |||
| invalidSplit: 'La suma de las partes no equivale al monto total', | |||
| other: 'Error inesperado, por favor inténtalo más tarde', | |||
| genericCreateFailureMessage: 'Error inesperado solicitando dinero, Por favor, inténtalo más tarde', | |||
| receiptFailureMessage: 'El recibo no se subió. ', | |||
| saveFileMessage: 'Guarda el archivo ', | |||
There was a problem hiding this comment.
Please update this word "Guarda" to "Download" as well for consistency.
cc: @marcochavezf for Spanish help
There was a problem hiding this comment.
I'm from Spain! I decided to leave both as they are on purpose 😁
| @@ -583,6 +583,9 @@ export default { | |||
| invalidSplit: 'La suma de las partes no equivale al monto total', | |||
| other: 'Error inesperado, por favor inténtalo más tarde', | |||
| genericCreateFailureMessage: 'Error inesperado solicitando dinero, Por favor, inténtalo más tarde', | |||
| receiptFailureMessage: 'El recibo no se subió. ', | |||
| saveFileMessage: 'Guarda el archivo ', | |||
| loseFileMessage: 'o descarta este error y piérdelo', | |||
There was a problem hiding this comment.
While updating above, please also apply this suggestion 🙂
|
@thienlnam 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] |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
| ))} | ||
| {_.map(sortedMessages, (message, i) => | ||
| isReceiptError(message) ? ( | ||
| <PressableWithoutFeedback |
There was a problem hiding this comment.
@Gonals I'm working on DotIndicatorMessage TS migration PR and while resolving conflicts I noticed this logic change:
isReceiptError(message) ? (
<PressableWithoutFeedback
...👇 With these lines sortedMessages is an array of strings, so I don't see how isReceiptError will ever return true for the code above, therefore the whole logic with PressableWithoutFeedback is unnecessary in my opinion 🤔
const sortedMessages = _.chain(props.messages)
.keys()
.sortBy()
.map((key) => props.messages[key])
// Using uniq here since some fields are wrapped by the same OfflineWithFeedback component (e.g. WorkspaceReimburseView)
// and can potentially pass the same error.
.uniq()
.map((message) => Localize.translateIfPhraseKey(message))
.value();There was a problem hiding this comment.
I tried to follow the steps from the description but I get a different error:
There was a problem hiding this comment.
Ah actually it works but it is kind of hacky 😅
ReceiptError object is passed to this function as a 'message', the error is thrown inside try block but it is caught in the catch block and the whole object is returned instead of a string as function signature suggests. I think it would makes things more readable if we were to change the logic here/or use a different component for Receipt errors. cc @Gonals @nkuoch @0xmiroslav
/**
* Return translated string for given error.
*/
function translateIfPhraseKey(message: string | [string, Record<string, unknown> & {isTranslated?: true}] | []): string {
if (!message || (Array.isArray(message) && message.length === 0)) {
return '';
}
try {
// check if error message has a variable parameter
const [phrase, variables] = Array.isArray(message) ? message : [message];
// This condition checks if the error is already translated. For example, if there are multiple errors per input, we handle translation in ErrorUtils.addErrorMessage due to the inability to concatenate error keys.
if (variables?.isTranslated) {
return phrase;
}
return translateLocal(phrase as TranslationPaths, variables as never);
} catch (error) {
return Array.isArray(message) ? message[0] : message;
}
}
|
🚀 Deployed to staging by https://github.com/nkuoch in version: 1.4.3-0 🚀
|
|
🚀 Deployed to staging by https://github.com/nkuoch in version: 1.4.3-0 🚀
|
|
I am not sure how QA team will test this as we prevent csv files uploading in advance. |
|
@Gonals We are blocked as per @0xmiroslav comment. How should we proceed with this PR? bandicam.2023-11-24.01-36-22-748.mp4 |
| <Text | ||
| key={i} | ||
| style={styles.offlineFeedback.text} | ||
| > | ||
| <Text style={[styles.optionAlternateText, styles.textLabelSupporting]}>{Localize.translateLocal('iou.error.receiptFailureMessage')}</Text> | ||
| <Text style={[styles.optionAlternateText, styles.textLabelSupporting, styles.link]}>{Localize.translateLocal('iou.error.saveFileMessage')}</Text> | ||
| <Text style={[styles.optionAlternateText, styles.textLabelSupporting]}>{Localize.translateLocal('iou.error.loseFileMessage')}</Text> | ||
| </Text> |
There was a problem hiding this comment.
@Gonals The error text next to red dot indicators is meant to be red to be consistent with other error messages in the app.
With your current change here, the error text will revert back to light-greyish green.
I just worked on an issue fixing this and just luckily stumbled on the regression from the video of another issue I was looking at.
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.4.3-11 🚀
|
| style={styles.offlineFeedback.text} | ||
| > | ||
| <Text style={[styles.optionAlternateText, styles.textLabelSupporting]}>{Localize.translateLocal('iou.error.receiptFailureMessage')}</Text> | ||
| <Text style={[styles.optionAlternateText, styles.textLabelSupporting, styles.link]}>{Localize.translateLocal('iou.error.saveFileMessage')}</Text> |
There was a problem hiding this comment.
Coming from this issue #39330, we should use either Pressable component or TextLink here, instead of wrapping the whole text inside a Pressable.
Details
Fixed Issues
$ #28884
PROPOSAL:
Tests
+->Request Money->Scanand upload a file that will fail in the backend. A CSV should do the trick.Offline tests
No changes
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)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