[HOLD for payment 2024-08-01] [$250] Scan - No error pop-up and scan expense with corrupted PDF can be submitted via QAB

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: v1.4.82-1
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4622999
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

1. Go to staging.new.expensify.com
2. Go to workspace chat.
3. Go to + > Submit expense > Scan.
4. Upload a corrupted PDF (attached below).
5. Note that it shows error pop-up and the expense is not submitted.
6. Submit a manual request in the workspace chat.
7. Go to FAB > Submit expense under QAB > Scan.
8. Upload the same corrupted PDF.

Expected Result:

App will show error pop-up and the scan expense is not submitted.

Actual Result:

There is inconsistency in submitting corrupted PDF via FAB and QAB.
App shows error pop-up and the expense is not submitted when submitting via FAB.
App does not show error pop-up and the scan expense is submitted when submitting via QAB.

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6511609_1718252208027.20240613_120908.mp4

LARGE PDF FILE corrupted.pdf

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~013830a17b8e9e0b20
• Upwork Job ID: 1801414678071173121
• Last Price Increase: 2024-06-14
• Automatic offers:
• suneox | Reviewer | 102912100
• Krishna2323 | Contributor | 102912103

Issue Owner

Current Issue Owner: @VictoriaExpensify

[melvin-bot]

Triggered auto assignment to @VictoriaExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[izarutskaya]

We think this issue might be related to the #collect project.

[Krishna2323]

Proposal

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

Scan - No error pop-up and scan expense with corrupted PDF can be submitted via QAB

What is the root cause of that problem?

The pdf error is detected when it is rendered on the confirmation page but in case of QAB, the confirmation page is skipped and we don't get the error alert. This also happens with for protected pdf's.

App/src/components/MoneyRequestConfirmationList.tsx

Lines 1115 to 1128 in bce61c0

	<PDFThumbnail
	// eslint-disable-next-line @typescript-eslint/non-nullable-type-assertion-style
	previewSourceURL={resolvedReceiptImage as string}
	// We don't support scanning password protected PDF receipt
	enabled={!isAttachmentInvalid}
	onPassword={() => {
	setIsAttachmentInvalid(true);
	setInvalidAttachmentPromt(translate('attachmentPicker.protectedPDFNotSupported'));
	}}
	onLoadError={() => {
	setInvalidAttachmentPromt(translate('attachmentPicker.errorWhileSelectingCorruptedAttachment'));
	setIsAttachmentInvalid(true);
	}}
	/>

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

If we want to validate the pdf on scan page then we would need to make few changes.

1. Create a state for storing the pdf file object.

const [pdfFile, setPdfFile] = useState<null | FileObject>(null);

2. Update setReceiptAndNavigate function, first we should accept a new parameter to check if the pdf is already validated, then check if the file is a pdf file or not and if so then call setPdfFile(file); to set the file and return from setReceiptAndNavigate.

App/src/pages/iou/request/step/IOURequestStepScan/index.tsx

Lines 418 to 434 in 1f7ebfc

	const setReceiptAndNavigate = (file: FileObject) => {
	validateReceipt(file).then((isFileValid) => {
	if (!isFileValid) {
	return;
	}
	// Store the receipt on the transaction object in Onyx
	const source = URL.createObjectURL(file as Blob);
	// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
	IOU.setMoneyRequestReceipt(transactionID, source, file.name || '', action !== CONST.IOU.ACTION.EDIT);
	if (action === CONST.IOU.ACTION.EDIT) {
	updateScanAndNavigate(file, source);
	return;
	}
	navigateToConfirmationStep(file, source);
	});
	};

    const setReceiptAndNavigate = (file: FileObject, isPdfValidated?: boolean) => {
        validateReceipt(file).then((isFileValid) => {
            if (!isFileValid) {
                return;
            }
            if (Str.isPDF(file.name ?? '') && !isPdfValidated) {
                setPdfFile(file);
                return;
            }
            // Store the receipt on the transaction object in Onyx
            const source = URL.createObjectURL(file as Blob);
            // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
            IOU.setMoneyRequestReceipt(transactionID, source, file.name || '', action !== CONST.IOU.ACTION.EDIT);

            if (action === CONST.IOU.ACTION.EDIT) {
                updateScanAndNavigate(file, source);
                return;
            }
            navigateToConfirmationStep(file, source);
        });
    };

3. Update PDFThumbnail to accept a new prop onLoadSuccess and pass it to onLoadSuccess prop of Document.
   

   App/src/components/PDFThumbnail/index.tsx

   Line 21 in 1f7ebfc

   <Document

4. Render the pdf component without displaying it and trigger the alert modals when callbacks triggers.

            {pdfFile && (
                <View style={{position: 'absolute', opacity: 0}}>
                    <PDFThumbnail
                        previewSourceURL={pdfFile.uri ?? ''}
                        onPassword={() => {
                            setUploadReceiptError(true, 'attachmentPicker.attachmentError', 'attachmentPicker.errorWhileSelectingCorruptedAttachment');
                        }}
                        onLoadSuccess={() => {
                            setPdfFile(null);
                            setReceiptAndNavigate(pdfFile, true);
                        }}
                        onLoadError={() => {
                            setUploadReceiptError(true, 'attachmentPicker.attachmentError', 'attachmentPicker.errorWhileSelectingCorruptedAttachment');
                        }}
                    />
                </View>
            )}

Note: There are some differences between PDFThumbnail component for native and web to there will be minor changes required accordingly.

In this PR, we added the functionality to detect the corrupted pdf files on confirmation page, so if we go with the new solution then we would need to clean up that component as well.

Test Branch

What alternative solutions did you explore? (Optional)

Result

Monosnap.screencast.2024-06-13.21-00-13.mp4

[VictoriaExpensify]

Agree this inconsistency should be fixed

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~013830a17b8e9e0b20

[melvin-bot]

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

[suneox]

@Krishna2323 proposal looks good to me.
Solution summary: We will validate the file after it is selected instead of validate on another place

🎀 👀 🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @grgia, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[truph01]

Proposal

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

There is inconsistency in submitting corrupted PDF via FAB and QAB.
App shows error pop-up and the expense is not submitted when submitting via FAB.
App does not show error pop-up and the scan expense is submitted when submitting via QAB.

What is the root cause of that problem?

• When creating expense by FAB, after uploading the file, we render the file via this logic:
  

  App/src/components/MoneyRequestConfirmationList.tsx

  Lines 1115 to 1127 in 43cd0f7

  	<PDFThumbnail
  	// eslint-disable-next-line @typescript-eslint/non-nullable-type-assertion-style
  	previewSourceURL={resolvedReceiptImage as string}
  	// We don't support scanning password protected PDF receipt
  	enabled={!isAttachmentInvalid}
  	onPassword={() => {
  	setIsAttachmentInvalid(true);
  	setInvalidAttachmentPromt(translate('attachmentPicker.protectedPDFNotSupported'));
  	}}
  	onLoadError={() => {
  	setInvalidAttachmentPromt(translate('attachmentPicker.errorWhileSelectingCorruptedAttachment'));
  	setIsAttachmentInvalid(true);
  	}}

  
  in the confirmation page, so there is an error popup appears in this case.

• But when creating expense by QAB, after uploading file, we skip confirmation page, that leads to there is no way to check whether the file is corrupted pdf file.

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

• We need to validate the corrupted PDF file right after user picks the file, rather than rendering it then need to wait until the onLoadError triggers. To do it, we can create a function isCorruptedPdfFile (can be used in both web and native):

import { PDFDocument } from 'pdf-lib';

async function isCorruptedPdfFile(file) {
  try {
    const arrayBuffer = await file.arrayBuffer();
    await PDFDocument.load(arrayBuffer);
    return false; // PDF is not corrupted
  } catch (error) {
  if (error.message.includes('password')) {
      return false;
    }
    return true;
  }
}

export default isCorruptedPdfFile;

then use it in this logic:

App/src/pages/iou/request/step/IOURequestStepScan/index.tsx

Line 192 in 43cd0f7

function validateReceipt(file: FileObject) {

if (fileExtension === 'pdf') {
    return isCorruptedPdfFile(file)
        .then((isCorruptedPdf: boolean) => {
            if (isCorruptedPdf) {
                setUploadReceiptError(true, 'attachmentPicker.attachmentError', 'attachmentPicker.errorWhileSelectingCorruptedAttachment'
                );
                return false;
            }
            return true;
        });
}

and

App/src/pages/iou/request/step/IOURequestStepScan/index.native.tsx

Line 164 in 43cd0f7

const validateReceipt = (file: FileObject) => {

if (fileExtension === 'pdf') {
    return isCorruptedPdfFile(file)
        .then((isCorruptedPdf: boolean) => {
            if (isCorruptedPdf) {
                Alert.alert('attachmentPicker.attachmentError', 'attachmentPicker.errorWhileSelectingCorruptedAttachment');
                return false;
            }
            return true;
        });
}

• If we want to handle password file, we can create isPasswordFile:

import { PDFDocument } from 'pdf-lib';

async function isPasswordFile(file) {
  try {
    const arrayBuffer = await file.arrayBuffer();
    await PDFDocument.load(arrayBuffer);
    return false; // PDF is not password protected
  } catch (error) {
    if (error.message.includes('password')) {
      return true; // PDF is password protected
    }
    return false;
  }
}

export default isPasswordFile;

• And use it like we use isCorruptedPdfFile

What alternative solutions did you explore? (Optional)

[truph01]

@suneox Can you help check my proposal above? TY

[suneox]

@suneox Can you help check my proposal above? TY

We have another proposal: it also validates files after they are selected without rendering. However, I have some concerns about this approach.

• Can it handle files with passwords and extract error types? Due to scanning receipts also does not support PDF files with passwords.
• Ability to run on native devices, and How does it perform on native devices when loading large files? This is relevant because PDFThumbnail on native devices uses native code provided by react-native-pdf

@truph01 Could you please clarify some point above? And thank you for your proposal 🌟

[truph01]

@suneox Thanks for your feedback.

Can it handle files with passwords and extract error types? Due to scanning receipts also does not support PDF files with passwords.

• I wonder that why do we need to handle password pdf case? I think it is not a scope of this issue

[suneox]

• I wonder that why do we need to handle password pdf case? I think it is not a scope of this issue

@truph01 We have to make it consistent at this comment

So the scope in this issue also handle password case, due to at confirmation page also validate pdf password.

[truph01]

@suneox I think that the "inconsistency" mentioned in this #43645 (comment) is about:

There is inconsistency in submitting corrupted PDF via FAB and QAB.
App shows error pop-up and the expense is not submitted when submitting via FAB.
App does not show error pop-up and the scan expense is submitted when submitting via QAB.

[suneox]

@suneox I think that the "inconsistency" mentioned in this #43645 (comment) is about:

I believe the scope of this issue should prevent uploading a PDF file with a password. This is because submit expense also prevents such files. When editing, we should also prevent uploading PDF files with passwords. If we don’t do this, the backend won’t be able to scan PDFs with passwords.

@VictoriaExpensify I'd like to confirm that the scope of this issue involves handling corrupted files and files has password

[melvin-bot]

@suneox, @grgia, @VictoriaExpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[grgia]

I believe the scope of this issue should prevent uploading a PDF file with a password. This is because submit expense also prevents such files. When editing, we should also prevent uploading PDF files with passwords. If we don’t do this, the backend won’t be able to scan PDFs with passwords.

@suneox I agree that we should make all pdf error cases consistent as part of this issue. Does that answer your question?

[truph01]

@grgia There are two types of pdf files we should not allow user to upload: Protected pdf and corrupt pdf. I think the @suneox 's question is:

• The scope of this issue only involve handling corrupted pdf or the protected pdf as well?

[grgia]

@truph01 we should cover both cases

[grgia]

Thank you @suneox, all yours @Krishna2323 !

[Krishna2323]

Will raise PR within 2 days.

[suneox]

@Krishna2323 still working on PR

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.11-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: Scan - No error pop-up and scan expense with corrupted PDF can be submitted via QAB  #44778

If no regressions arise, payment will be issued on 2024-08-01. 🎊

For reference, here are some details about the assignees on this issue:

• @suneox requires payment automatic offer (Reviewer)
• @Krishna2323 requires payment automatic offer (Contributor)

[melvin-bot]

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:

• [@suneox] The PR that introduced the bug has been identified. Link to the PR:
• [@suneox] 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:
• [@suneox] 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:
• [@suneox] Determine if we should create a regression test for this bug.
• [@suneox] 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.
• [@VictoriaExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[suneox]

BugZero Checklist

• [@suneox] The PR that introduced the bug has been identified. Link to the PR: N/A: This is a new implementation to handle an edge case.
• [@suneox] 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
• [@suneox] 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
• [@suneox] Determine if we should create a regression test for this bug. N/A

[VictoriaExpensify]

Contributor: @Krishna2323 paid $250 via Upwork

Contributor+: @suneox paid $250 via Upwork

Upwork job here!

[VictoriaExpensify]

Thanks for your work @suneox and @Krishna2323! 🙌