[HOLD for payment 2023-11-22] [SMARTSCAN] [$500] HIGH: Web - Scan - Failed to load PDF file uploaded in request money scan #27680

kbecciv · 2023-09-18T14:23:55Z

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Action Performed:

Open an report
Request money and open scan tab
Upload a pdf
Open scan report
Click on pdf preview

Expected Result:

The pdf should be loaded.

Actual Result:

The pdf is not loaded correctly.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Version Number: 1.3.71.4
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Untitled.mp4

Recording.4564.mp4

Expensify/Expensify Issue URL:
Issue reported by: @Krishna2323
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1694901873701159

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01fcbb8f9acc8d0c31
Upwork Job ID: 1703776880779624448
Last Price Increase: 2023-10-09

melvin-bot · 2023-09-18T14:24:00Z

Job added to Upwork: https://www.upwork.com/jobs/~01fcbb8f9acc8d0c31

melvin-bot · 2023-09-18T14:24:01Z

Triggered auto assignment to @sonialiap (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot · 2023-09-18T14:24:04Z

Bug0 Triage Checklist (Main S/O)

This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

melvin-bot · 2023-09-18T14:24:04Z

Triggered auto assignment to @garrettmknight (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

melvin-bot · 2023-09-18T14:24:12Z

Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane (External)

ewanmellor · 2023-09-18T19:48:46Z

Proposal

Please re-state the problem that we are trying to solve in this issue.

Opening a scanned PDF from a scan report fails.

What is the root cause of that problem?

The thumbnail image shown in the bug report video is a ReportActionItemImage. This is being passed the image to display by MoneyRequestView

App/src/components/ReportActionItem/MoneyRequestView.js

Line 93 in 6be8fca

    
           receiptURIs = ReceiptUtils.getThumbnailAndImageURIs(transaction.receipt.source, transaction.filename);

The problem is that for a PDF, this is the preview image, not the PDF. When ReportActionItemImage handles the click on the preview image, it navigates to /r/:id/attachment?source=/:id.png, which renders AttachmentModal > AttachmentView.

AttachmentView is smart enough to be able to know to open the PDF viewer (AttachmentViewPdf) but unfortunately it is doing this to handle the case where the source is a blob: URL for drag-n-drop. No consideration has been given to the case where the source is only a preview PNG.

App/src/components/Attachments/AttachmentView/index.js

Line 91 in 6be8fca

    
           if (Str.isPDF(source) || (file && Str.isPDF(file.name || translate('attachmentView.unknownFilename')))) {

AttachmentViewPdf then assumes that the given source is a valid PDF, and tries to render it. Since it is a PNG, it fails.

What changes do you think we should make in order to solve the problem?

ReportActionItemImage needs to handle the case where the image is a preview only, and the actual attachment is something else. We need to pass the proper attachment filepath so that the AttachmentModal can be given the correct info, and we'll end up opening AttachmentViewPdf with a valid source.

garrettmknight · 2023-09-19T02:36:27Z

Unnassigning since @sonialiap was first!

Talha345 · 2023-09-21T23:33:21Z

Proposal

Please re-state the problem that we are trying to solve in this issue.

When a PDF is used for request money via the scan feature, it is not loaded for preview in the attachment modal via the request money report.

What is the root cause of that problem?

The MoneyRequestView passes an image and thumbnail to ReportActionItemImage for preview by calculating the thumbnail and image URL using the getThumbnailAndImageURIs helper method below.

App/src/components/ReportActionItem/MoneyRequestView.js

Lines 92 to 95 in 6be8fca

    
           if (hasReceipt) { 
        
               receiptURIs = ReceiptUtils.getThumbnailAndImageURIs(transaction.receipt.source, transaction.filename); 
        
               hasErrors = TransactionUtils.hasMissingSmartscanFields(transaction); 
        
           }

The ReportActionItemImage then displays the thumbnail or image and the image source URL is used to open the attachment.

In case of PDF, the getThumbnailAndImageURIs method returns the following response:

{thumbnail: null, image} , where the image is the URL of just a placeholder image i.e ReceiptGeneric.

App/src/libs/ReceiptUtils.js

Line 52 in 6be8fca

let image = ReceiptGeneric;

Therefore, when the AttachmentView tries to display the attachment, the following if clause succeeds because the file.name has a .pdf extension but infact the source is an image URL and therefore the PDF is not loaded.

App/src/components/Attachments/AttachmentView/index.js

Line 91 in 6be8fca

    
           if (Str.isPDF(source) || (file && Str.isPDF(file.name || translate('attachmentView.unknownFilename')))) {

What changes do you think we should make in order to solve the problem?

We can add the following if clause to getThumbnailAndImageURIs helper method.This way for a PDF the thumbnail uses the URL of the placeholder image i.e ReceiptGeneric but the image source contains the actual path of the PDF file and hence it is displayed correctly.

    if (fileExtension === 'pdf')
    {
        return {thumbnail: image, image: path};
    }

In order to display the thumbnail image in RHN while requesting money we would have to update the image source here with the following change

App/src/components/MoneyRequestConfirmationList.js

Lines 442 to 446 in 6be8fca

    
           {!_.isEmpty(props.receiptPath) ? ( 
        
               <Image 
        
                   style={styles.moneyRequestImage} 
        
                   source={{uri: ReceiptUtils.getThumbnailAndImageURIs(props.receiptPath, props.receiptSource).image}} 
        
               />

const imgURLs = ReceiptUtils.getThumbnailAndImageURIs(props.receiptPath, props.receiptSource);
 const imgSource = imgURLs.thumbnail || imgURLs.image;
 
   <Image
                    style={styles.moneyRequestImage}
                    source={{uri: imgSource}}
                />

What alternative solutions did you explore? (Optional)

N/A

Result:

Screen.Recording.2023-09-22.at.01.32.00.mov

pradeepmdk · 2023-09-22T02:14:27Z

Proposal

Please re-state the problem that we are trying to solve in this issue.

Failed to load PDF file uploaded in request money scan

What is the root cause of that problem?

here we are returning the static image even pdf.

App/src/libs/ReceiptUtils.js

Line 64 in 6be8fca

return {thumbnail: null, image};

static image used in preview

App/src/components/ReportActionItem/ReportActionItemImage.js

Line 62 in 6be8fca

const route = ROUTES.getReportAttachmentRoute(report.reportID, imageSource);

What changes do you think we should make in order to solve the problem?

we should change this

App/src/libs/ReceiptUtils.js

Line 64 in 6be8fca

return {thumbnail: null, image};

to
return {thumbnail: image, image: path}; this will work for other type
now we are focusing the only pdf so

if (fileExtension === 'pdf')
    {
        return {thumbnail: image, image: path};
    }

and here we can create const for pdf

App/src/CONST.ts

Line 1152 in 02ad2fc

FILE_TYPES: {

so that all static image show in the thumbnail and the original path take to preview
and when we change this we should change ThumbnailImage component props allow image as reference number

because in native device local import as reference number we already did this some of component
like this

App/src/components/ReportActionItem/ReportActionItemImage.js

Lines 18 to 19 in 6be8fca

    
           /** URI for the image or local numeric reference for the image  */ 
        
           image: PropTypes.oneOfType([PropTypes.string, PropTypes.number]).isRequired,

when we change this it will create regression here

App/src/components/MoneyRequestConfirmationList.js

Line 445 in 6be8fca

    
           source={{uri: ReceiptUtils.getThumbnailAndImageURIs(props.receiptPath, props.receiptSource).image}}

so here we need to change

let receiptPathImage;
    if (!_.isEmpty(props.receiptPath)) {
       const image =  ReceiptUtils.getThumbnailAndImageURIs(props.receiptPath, props.receiptSource)
       receiptPathImage = image.thumbnail || image.image
    }

......
source={{uri: receiptPathImage}}

here thumbnail is always an image except in this case

 if (isReceiptImage && (path.startsWith('blob:') || path.startsWith('file:'))) {
        return {thumbnail: null, image: path};
    }

sonialiap · 2023-09-22T05:39:05Z

@sobitneupane looks like we have proposals rolling in, any thoughts on the ones we currently have?

sobitneupane · 2023-09-25T08:43:54Z

Thanks for the proposal @ewanmellor

We need to pass the proper attachment filepath so that the AttachmentModal can be given the correct info

Your solution should include how you are going to achieve the result.

sobitneupane · 2023-09-25T09:00:37Z

Thanks for the proposal @Talha345.

With the change you suggested, I'm observing a loading indicator instead of the thumbnail.

sobitneupane · 2023-09-25T09:02:17Z

@pradeepmdk Thanks for the proposal.

What change are you suggesting in ThumbnailImage and how and what regression will it cause regression? Can we return {thumbnail: image, image: path}; only if file is pdf?

sobitneupane · 2023-09-25T09:53:38Z

DO we have any issue with docx?

Talha345 · 2023-09-25T09:57:24Z

Thanks for the proposal @Talha345.

With the change you suggested, I'm observing a loading indicator instead of the thumbnail.

Hi @sobitneupane, make sure you are adding the if clause at the correct place within the method like:

Seems to be working fine for me, however I noticed that the thumbnail image was not loading in the RHN after choosing the PDF file during request money with my change so we need to make another change at some place and I have updated my proposal accordingly.

Talha345 · 2023-09-25T10:05:05Z

DO we have any issue with docx?

@sobitneupane With docx, currently we are displaying the docx generic image because we do not have the capability to display docx attachments even when we send them in the chat.We only preview images and PDFs

Screen.Recording.2023-09-25.at.12.03.35.mov

pradeepmdk · 2023-11-25T16:31:51Z

@sonialiap any update for payment

sonialiap · 2023-11-27T09:10:44Z

@pradeepmdk fix = $500 - paid ✔️
@sobitneupane review = $500 - please request in ND
@Krishna2323 report = $50 - paid ✔️

sonialiap · 2023-11-27T09:11:05Z

@sobitneupane please complete the checklist

rlinoz · 2023-11-30T20:36:41Z

I think the comment above is still valid? @sobitneupane

sonialiap · 2023-12-01T13:51:33Z

Yes, still need the checklist competed :)

sonialiap · 2023-12-04T13:15:57Z

@sobitneupane bump on completing the checklist

melvin-bot · 2023-12-08T11:06:32Z

@rlinoz, @pradeepmdk, @sonialiap, @Gonals, @sobitneupane Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

sobitneupane · 2023-12-08T11:07:26Z

Sorry for the delay. I will get it done asap.

sobitneupane · 2023-12-08T11:35:34Z

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:

#24235 was the PR which introduced the feature of SmartScan to newDot. The PR mainly focused on images as receipt. So, Rather than a bug, I will say it was work on progress.

[@sobitneupane] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

N/A

[@sobitneupane] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

N/A

[@sobitneupane] Determine if we should create a regression test for this bug.

Yes.

[@sobitneupane] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

#27680 (comment)

sobitneupane · 2023-12-08T11:37:00Z

Regression Test Proposal:

Request money and open scan tab
Upload a pdf
Open scanned report
Click on pdf preview
Verify that pdf is loaded.

Do we agree 👍 or 👎

sonialiap · 2023-12-11T12:54:55Z

Thanks, Sobit! Submitting regression test and closing this out

sobitneupane · 2023-12-15T09:54:44Z

#27680 (comment)

Requested payment on newDot.

JmillsExpensify · 2023-12-15T23:15:46Z

$500 payment approved for @sobitneupane based on this comment.

trjExpensify · 2023-12-20T22:11:01Z

👋 Heads up! @pradeepmdk @sobitneupane @Gonals this seems to have broken again:

Any chance you'd be willing to help us diagnose so we can create a follow-up issue/PR to fix it, or maybe encapsulate it into this issue?

CC: @RachCHopkins

sobitneupane · 2023-12-21T10:56:29Z

Found the cause.

App/src/components/ReportActionItem/ReportActionItemImage.js

Line 87 in 880254c

isAuthTokenRequired={!isLocalFile}

isLocalFile is true even when authToken is required (making isAuthTokenRequired false for non-local files). We didn't face the issue before because we weren't using AttachmentModal component in ReportActionItemImage.

@pradeepmdk Will you be willing to create PR to fix the issue.

trjExpensify · 2023-12-21T12:29:21Z

Niceee sleuthing! ❤️ @pradeepmdk could you let us know today if you'd be up for fixing that?

Talha345 · 2023-12-21T12:41:35Z

@trjExpensify @sobitneupane I would be available to fix this follow up issue if @pradeepmdk is not available anymore as I was also involved in this during the proposal and discussion phase! 😊

trjExpensify · 2023-12-21T13:39:50Z

Nice, let's go for it! Fresh issue here: #33412

kbecciv added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 18, 2023

melvin-bot bot changed the title ~~Web - Scan - Failed to load PDF file uploaded in request money scan~~ [$500] Web - Scan - Failed to load PDF file uploaded in request money scan Sep 18, 2023

melvin-bot bot assigned sonialiap Sep 18, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 18, 2023

melvin-bot bot assigned garrettmknight Sep 18, 2023

melvin-bot bot assigned sobitneupane Sep 18, 2023

garrettmknight removed their assignment Sep 19, 2023

melvin-bot bot added the Overdue label Sep 21, 2023

melvin-bot bot added Overdue and removed Overdue labels Sep 22, 2023

melvin-bot bot removed the Overdue label Sep 25, 2023

sobitneupane mentioned this issue Sep 25, 2023

[HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] [$500] MEDIUM: New Feature: Support file uploads on mobile (e.g. PDFs, docx, etc.) #23228

Closed

melvin-bot bot removed the Overdue label Nov 25, 2023

melvin-bot bot added the Overdue label Nov 29, 2023

melvin-bot bot removed the Overdue label Nov 30, 2023

melvin-bot bot added the Overdue label Dec 4, 2023

melvin-bot bot added Overdue and removed Overdue labels Dec 4, 2023

melvin-bot bot removed the Overdue label Dec 8, 2023

melvin-bot bot added the Overdue label Dec 11, 2023

melvin-bot bot removed the Overdue label Dec 11, 2023

sonialiap closed this as completed Dec 11, 2023

trjExpensify mentioned this issue Dec 20, 2023

[HOLD for payment 2024-03-14] [$500] [MEDIUM] Smartscan: Show PDF receipt thumbnails #31432

Closed

6 tasks

trjExpensify mentioned this issue Dec 21, 2023

[Awaiting Payment 26th Jane] [$500] PDF receipt doesn't load in the receipt modal #33412

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HOLD for payment 2023-11-22] [SMARTSCAN] [$500] HIGH: Web - Scan - Failed to load PDF file uploaded in request money scan #27680

[HOLD for payment 2023-11-22] [SMARTSCAN] [$500] HIGH: Web - Scan - Failed to load PDF file uploaded in request money scan #27680

kbecciv commented Sep 18, 2023 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Sep 18, 2023

melvin-bot bot commented Sep 18, 2023

melvin-bot bot commented Sep 18, 2023

melvin-bot bot commented Sep 18, 2023

melvin-bot bot commented Sep 18, 2023

ewanmellor commented Sep 18, 2023

garrettmknight commented Sep 19, 2023

Talha345 commented Sep 21, 2023 •

edited

Loading

pradeepmdk commented Sep 22, 2023 •

edited

Loading

sonialiap commented Sep 22, 2023

sobitneupane commented Sep 25, 2023

sobitneupane commented Sep 25, 2023

sobitneupane commented Sep 25, 2023

sobitneupane commented Sep 25, 2023

Talha345 commented Sep 25, 2023

Talha345 commented Sep 25, 2023 •

edited

Loading

pradeepmdk commented Nov 25, 2023

sonialiap commented Nov 27, 2023 •

edited

Loading

sonialiap commented Nov 27, 2023

rlinoz commented Nov 30, 2023

sonialiap commented Dec 1, 2023

sonialiap commented Dec 4, 2023

melvin-bot bot commented Dec 8, 2023

sobitneupane commented Dec 8, 2023

sobitneupane commented Dec 8, 2023 •

edited

Loading

sobitneupane commented Dec 8, 2023

sonialiap commented Dec 11, 2023

sobitneupane commented Dec 15, 2023

JmillsExpensify commented Dec 15, 2023

trjExpensify commented Dec 20, 2023

sobitneupane commented Dec 21, 2023 •

edited

Loading

trjExpensify commented Dec 21, 2023

Talha345 commented Dec 21, 2023

trjExpensify commented Dec 21, 2023

[HOLD for payment 2023-11-22] [SMARTSCAN] [$500] HIGH: Web - Scan - Failed to load PDF file uploaded in request money scan #27680

[HOLD for payment 2023-11-22] [SMARTSCAN] [$500] HIGH: Web - Scan - Failed to load PDF file uploaded in request money scan #27680

Comments

kbecciv commented Sep 18, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

melvin-bot bot commented Sep 18, 2023

melvin-bot bot commented Sep 18, 2023

melvin-bot bot commented Sep 18, 2023

Bug0 Triage Checklist (Main S/O)

melvin-bot bot commented Sep 18, 2023

melvin-bot bot commented Sep 18, 2023

ewanmellor commented Sep 18, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

garrettmknight commented Sep 19, 2023

Talha345 commented Sep 21, 2023 • edited Loading

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

What alternative solutions did you explore? (Optional)

Result:

pradeepmdk commented Sep 22, 2023 • edited Loading

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

sonialiap commented Sep 22, 2023

sobitneupane commented Sep 25, 2023

sobitneupane commented Sep 25, 2023

sobitneupane commented Sep 25, 2023

sobitneupane commented Sep 25, 2023

Talha345 commented Sep 25, 2023

Talha345 commented Sep 25, 2023 • edited Loading

pradeepmdk commented Nov 25, 2023

sonialiap commented Nov 27, 2023 • edited Loading

sonialiap commented Nov 27, 2023

rlinoz commented Nov 30, 2023

sonialiap commented Dec 1, 2023

sonialiap commented Dec 4, 2023

melvin-bot bot commented Dec 8, 2023

sobitneupane commented Dec 8, 2023

sobitneupane commented Dec 8, 2023 • edited Loading

sobitneupane commented Dec 8, 2023

sonialiap commented Dec 11, 2023

sobitneupane commented Dec 15, 2023

JmillsExpensify commented Dec 15, 2023

trjExpensify commented Dec 20, 2023

sobitneupane commented Dec 21, 2023 • edited Loading

trjExpensify commented Dec 21, 2023

Talha345 commented Dec 21, 2023

trjExpensify commented Dec 21, 2023

kbecciv commented Sep 18, 2023 •

edited by melvin-bot bot

Loading

Talha345 commented Sep 21, 2023 •

edited

Loading

pradeepmdk commented Sep 22, 2023 •

edited

Loading

Talha345 commented Sep 25, 2023 •

edited

Loading

sonialiap commented Nov 27, 2023 •

edited

Loading

sobitneupane commented Dec 8, 2023 •

edited

Loading

sobitneupane commented Dec 21, 2023 •

edited

Loading