fix: IOU Scan - In dark mode, the damaged PDF - file is barely visible. #40607
Conversation
Signed-off-by: Krishna Gupta <[email protected]>
melvin-bot
bot
requested review from
jayeshmangwani
and removed request for
a team
|
@jayeshmangwani 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] |
|
@jayeshmangwani, when selecting a corrupt pdf file on native devices we don't get error and empty white background is shown. I'm looking for a solution for that. |
|
@Krishna2323 When you push the changes for native devices, please ping me. |
Signed-off-by: Krishna Gupta <[email protected]>
Signed-off-by: Krishna Gupta <[email protected]>
|
@jayeshmangwani, I have updated the code for native devices, can you pls check and let me know, I haven't updated the recordings for native yet. Demo for for native devices: scan_corrupt_pdf_error_native.mp4 |
|
@Expensify/design Please confirm the styling of the 
|
|
I would think instead of showing text like we are, we'd show our default fallback receipt view, something like this that @dannymcclain had suggested previously: Though I think I would change out the icon to use some kind of broken image icon, something like this but in our brand style: |
|
@shawnborton I thought in a different issue we had settled on not allowing someone to upload a corrupted image/PDF? We show the standard modal that says "Error uploading attachment..." This was the mock for that:
Am I misremembering something? |
|
You are definitely right. So yeah, that begs the question, how did we trigger this particular "Failed to load PDF" scenario? |
|
@shawnborton @dannymcclain To reproduce this issue, I am following this steps:
corrupted-pdf.movCorrupt pdf |
|
@dannymcclain @shawnborton, Yep, you're correct here. I worked on preventing corrupted images from being uploaded, but that was specifically for images. There might be another GitHub issue addressing prevention of corrupt PDFs from being uploaded. PR. |
|
Here is the issue for preventing corrupted pdf's from being uploaded. |
|
Yeah, I feel like a much better solution is to prevent a corrupted PDF from being uploaded in the first place. |
|
@hayata-suenaga @shawnborton If we are going to prevent a corrupted PDF from being uploaded, then we can close this PR and close/hold the issue in favor of #34032 |
|
That's where my head is at... it seems like we won't ever be able to get to this scenario once we prevent corrupted images/files from being uploaded. |
I don't think we know the image/file is corrupt purely on the client, so isn't it possible you'll run into this when uploading a corrupt file offline? |
|
Hmm good question, who can confirm that? If that's the case, I like doing something like my suggestion here. |
Yeah I agree. @shawnborton do you think any of these icons would work for that? (Receipt Slash and Document Slash are already in the system—Image Slash is one that I just made based off of the Image icon and Receipt Slash icon.)
|
|
Yeah I think any of these would be great! I think I like doing something super generic in case this is a pattern we'd repeat not only for PDF-specific receipts but for all receipts in general that are corrupt. So that being said, I lean towards the first option? |
|
Yeah first option makes sense to me—that way we could use it for any file type of receipt that borked. |
I'm not familiar with how we're handling the PDF upload on the client side, but we try to display a preview of the PDF before the user attempts to send it. If the PDF is corrupted, we know that when we try to display the preview. @Krishna2323, could you confirm that you seem to be working on the issue to prevent the corrupt PDF from being uploaded? 
Regardless, I think it's a good idea to still implement the fallback view if the attachment is corrupted though. |
|
@hayata-suenaga, I've worked on preventing corrupt images from being uploaded, and it seems to be functioning well across all platforms. As for the PDF issue, it's slated to be addressed in this issue. The proposal selected to resolve the problem doesn't appear to offer a straightforward solution for detecting corrupt PDFs across all platforms. Therefore, I agree with your suggestion to proceed with implementing the fallback view. Additionally, I've noticed that some PDFs preview correctly on the confirmation list but then display an error message stating that the PDF can't be loaded. You can try with this pdf: This works fine until we submit the request and go to details page but as soon as we refresh it shows |
|
Ok, so I guess, we agree on showing fallback for corrupted files, will start implementing the changes today and update. |
| const sizeStyles = [styles.w100, styles.h100]; | ||
| const [hasError, setHasError] = useState(false); |
There was a problem hiding this comment.
hasError variable name doesn't look good here. Instead we can use something like failedToLoad @hayata-suenaga @Krishna2323
There was a problem hiding this comment.
failedToLoad sounds good 😄
| <View style={[sizeStyles, styles.alignItemsCenter, styles.justifyContentCenter]}> | ||
| {enabled && ( | ||
| <View style={[sizeStyles, !hasError && styles.alignItemsCenter, styles.justifyContentCenter]}> | ||
| {enabled && !hasError && ( |
There was a problem hiding this comment.
Is there any reason you added different conditions when rendering the PDF component and thumbnail?
Why are we checking the hasError in native only and not other platforms?
App/src/components/PDFThumbnail/index.tsx
Line 57 in 31caba2
There was a problem hiding this comment.
I think I forgot to add that condition, now updated.
| fill={theme.icon} | ||
| /> | ||
| </View> | ||
| )} |
There was a problem hiding this comment.
The lines of code below are common in both files; we can create a common file for this.
<View style={[styles.justifyContentCenter, styles.pdfErrorPlaceholder, styles.alignItemsCenter]}>
<Icon
src={Expensicons.ReceiptSlash}
width={variables.receiptPlaceholderIconWidth}
height={variables.receiptPlaceholderIconHeight}
fill={theme.icon}
/>
</View>|
|
||
| return ( | ||
| <View style={[style, styles.overflowHidden]}> | ||
| <View style={[sizeStyles, styles.alignItemsCenter, styles.justifyContentCenter]}> | ||
| {enabled && ( | ||
| <View style={[sizeStyles, !hasError && styles.alignItemsCenter, styles.justifyContentCenter]}> |
There was a problem hiding this comment.
Why conditional alignItemsCenter styling here
There was a problem hiding this comment.
The component is used in other places as well, and if I recall correctly, applying alignItemsCenter by default was causing styling issues in other places.
| <View style={[style, styles.overflowHidden]}> | ||
| <View style={[styles.w100, styles.h100, styles.alignItemsCenter, styles.justifyContentCenter]}>{enabled && thumbnail}</View> | ||
| <View style={[style, styles.h100, styles.overflowHidden]}> | ||
| <View style={[styles.w100, styles.h100, !hasError && styles.alignItemsCenter, !hasError && styles.justifyContentCenter]}> |
There was a problem hiding this comment.
Why are conditional alignItemsCenter and justifyContentCenter styling passed here?
Instead of checking hasError twice for both stylings, please merge them.
- !hasError && styles.alignItemsCenter, !hasError && styles.justifyContentCenter
+ !hasError && {...styles.alignItemsCenter, ...styles.justifyContentCenter}Signed-off-by: Krishna Gupta <[email protected]>
Signed-off-by: Krishna Gupta <[email protected]>
| <View style={[styles.w100, styles.h100, styles.alignItemsCenter, styles.justifyContentCenter]}>{enabled && thumbnail}</View> | ||
| <View style={[style, styles.h100, styles.overflowHidden]}> | ||
| <View style={[styles.w100, styles.h100, !failedToLoad && {...styles.alignItemsCenter, ...styles.justifyContentCenter}]}> | ||
| {enabled && failedToLoad && thumbnail} |
There was a problem hiding this comment.
These conditionals contradict each other. Why should we show a thumbnail when the failedToLoad condition is true??
There was a problem hiding this comment.
thats incorrect, updated now. Thanks for the catch.
| ); | ||
|
|
||
| return ( | ||
| <View style={[style, styles.overflowHidden]}> | ||
| <View style={[styles.w100, styles.h100, styles.alignItemsCenter, styles.justifyContentCenter]}>{enabled && thumbnail}</View> | ||
| <View style={[style, styles.h100, styles.overflowHidden]}> |
There was a problem hiding this comment.
Here, we Added the styles.h100 parent View Component; why is this change necessary? It was not to the parent View Component before our changes. We don't want to add any visual regression from the PR, so we wanted to make sure it is needed here
There was a problem hiding this comment.
I found a bug when adding styles.h100 directly, added a condition now.
Signed-off-by: Krishna Gupta <[email protected]>
|
@Krishna2323 Please Update Tests Section something like this: 1. Open App > Press FAB Menu > Submit expense > Select the Scan tab
2. Upload corrupt pdf(attached below) > Select any participant
3. Verify That We can see the Attachment error Modal, and in the background, we can see the [Receipt Slash Image](https://github.com/Expensify/App/pull/40607#issuecomment-2082744344)Below the Tests Section, attach the corrupt pdf file for testing. Also, can you help me understand how I can test these steps? I can test the following: 4. Verify transactions preview also shows receipt slash component
5. Click on transactions preview and verify receipt slash component is also displayed on transaction preview
6. Open transaction details page and verify receipt slash component in place of receipt place holder |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid.movAndroid: mWeb ChromemWeb-chrome.moviOS: NativeiOS.moviOS: mWeb SafarimWeb-safari.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
There was a problem hiding this comment.
Added a comment for updating the Tests section, code changes LGTM
All yours @hayata-suenaga
hayata-suenaga
changed the title
|
@hayata-suenaga I think we are good to merge this PR; I can see a few PRs merged recently |
|
The merge freeze is being gradually being lifted with the urgent PRs being merged first I don't think this PR needs to be merged urgently, so let's wait until we have complete merge freeze lift 😄 |
|
@hayata-suenaga, friendly bump to merge this, if we can. |
hayata-suenaga
changed the title
|
✋ 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 production by https://github.com/luacmartins in version: 1.4.81-11 🚀
|
Details
Fixed Issues
$ #39356
PROPOSAL: #39356 (comment)
Tests
Attachment errorOffline tests
Attachment errorQA Steps
Attachment errorPR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.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
android_native.mp4
Android: mWeb Chrome
android_chrome.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.mp4
MacOS: Desktop
desktop_app.mp4