[HOLD for payment 2024-06-28] [$250] IOU - Password-protected PDF not handled correctly when added to IOU as recipient

[lanitochka17]

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: 1.4.73-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

1. FAB > Submit expense > Manual > add amount
2. Select workspace > add merchant > submit the expense
3. Go to IOU > add password-protected PDF as a recipient

Expected Result:

During the expense submission process (FAB > Submit expense > add amount > choose workspace), when attempting to add a password-protected PDF, an error occurs stating "password-protected PDF is not supported." This error handling should also be applied when adding a password-protected PDF as the recipient in the IOU

Actual Result:

Password-protected PDF added to IOU is not handled properly

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

Add any screenshot/video evidence

Bug6479273_1715601330328.Screen_Recording_2024-05-13_at_4.40.33_AM.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0126fd5d796dca6c8e
• Upwork Job ID: 1793041772527243264
• Last Price Increase: 2024-05-21
• Automatic offers:
• dukenv0307 | Reviewer | 0
• tienifr | Contributor | 0

Issue Owner

Current Issue Owner: @kevinksullivan

[melvin-bot]

Triggered auto assignment to @kevinksullivan (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.

[lanitochka17]

@kevinksullivan FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[lanitochka17]

We think that this bug might be related to #vip-vsp

[gijoe0295]

Proposal

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

There are 2 issues:

1. We couldn't dismiss the Password-protected PDF is not supported modal in IOU confirmation screen. Although closing the modal navigates back the participant selector page, select any participant to navigate to confirmation screen again will toggle the modal again.
2. We didn't show that modal in money request view while editing the receipt.

What is the root cause of that problem?

1. In MoneyRequestConfirmationList, we only navigated back when closing the modal, we did not reset/remove the receipt from the transaction. So when we re-opened the confirmation screen later on, the invalid receipt is still there thus toggling the error modal.

App/src/components/MoneyRequestConfirmationList.tsx

Lines 1152 to 1160 in b62080a

	<ConfirmModal
	title={translate('attachmentPicker.wrongFileType')}
	onConfirm={navigateBack}
	onCancel={navigateBack}
	isVisible={isAttachmentInvalid}
	prompt={translate('attachmentPicker.protectedPDFNotSupported')}
	confirmText={translate('common.close')}
	shouldShowCancelButton={false}
	/>

2. We do not have a check for protected PDF when showing the receipt thumbnail in ReportActionItemImage within MoneyRequestView like we did in MoneyRequestConfirmationList

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

1. Reset the transaction's receipt using setMoneyRequestReceipt

App/src/libs/actions/IOU.ts

Line 410 in 7954145

function setMoneyRequestReceipt(transactionID: string, source: string, filename: string, isDraft: boolean, type?: string) {

then hide the modal by reset setIsAttachmentInvalid when confirm/cancel the modal.

2. In ReportActionItemImage, use the PDFThumbnail component to check if the PDF is password-protected and display the error modal if it is using the same approach used MoneyRequestConfirmationList:

if (enablePreviewModal && propsObj.isPDFThumbnail) {
 return (
    <>
        <PDFThumbnail
            enabled={!isAttachmentInvalid}
            onPassword={() => setIsAttachmentInvalid(true)}
        />
        <ConfirmModal
            onConfirm={() => {
                IOU.detachReceipt(transaction?.transactionID ?? '');
                setIsAttachmentInvalid(false);
            }}
            onCancel={() => {
                IOU.detachReceipt(transaction?.transactionID ?? '');
                setIsAttachmentInvalid(false);
            }}
            isVisible={isAttachmentInvalid}
        />
    </>
);

Note to use detachReceipt instead of setMoneyRequestReceipt because the transaction now is no longer optimistic.

[gijoe0295]

I think this is quite a critical issue because user couldn't dismiss the invalid receipt type modal even when they refreshed the page as mentioned in my proposal.

[melvin-bot]

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

[melvin-bot]

@kevinksullivan Still overdue 6 days?! Let's take care of this!

[kevinksullivan]

Hmm I think because it is IOU related we'd make a low priority split item. @saracouto curious if you agree!

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0126fd5d796dca6c8e

[melvin-bot]

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

[tienifr]

Proposal

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

Password-protected PDF added to IOU is not handled properly

What is the root cause of that problem?

When we create a new money request, we have this logic to show error if the PDF is password-protected.

But when we try to replace a receipt after already created the money request, there won't be any logic.

The way we're doing it is not ideal, first we still allow the user to upload the password-protected PDF, it won't fail the validation here. Then when we try to render the PDF, if it requires a password, then we show an error modal and if the modal is dismissed, the user will go back to the previous screen. This is not a good practice because:

• It shows broken UX of confirmation page in the background when the error modal is showing, we can see the area of the receipt is blank

• It has inconsistent UX with other attachment error, for other attachment error, it will show an error right after the user tries to upload, and the user will stay in the same page to upload again instead of being navigated to another page like Confirmation
• It will cause a lot of code duplication because in different places we use different elements to render the PDF, if we add the attachment modal opening logic if that rendering fails, we'll have to do it wherever we render the PDF after uploading. When we validate right at the source where we upload the PDF, we only need to validate in 1 place.

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

We should validate the password protected PDF here right where the user uploads it.

First we need to define a method to validate the password protected PDF, we can easily achieve it by using built-in feature from pdfjs. The error reason 1 is for password protected error.

import {pdfjs} from 'react-pdf';

const isPdfFilePasswordProtected = (file: FileObject) => 
    new Promise((resolve) => {
        if (!file.uri) {
            resolve(false);
        }

        pdfjs.getDocument(file.uri ?? '').onPassword = (callback, reason) => {
            // 1 is the error code for password protected PDF error
            if (reason === 1) {
                resolve(true);
                return;
            }
            resolve(false);
        };
    });

Then in here, if the file is pdf, validate password protected and show the proper error modal, similar to other attachment errors:

if (fileExtension === 'pdf') {
    return isPdfFilePasswordProtected(file).then(isProtected => {
        if (isProtected) {
            setUploadReceiptError(true, 'attachmentPicker.wrongFileType', 'attachmentPicker.protectedPDFNotSupported');
        }
        return !isProtected;
    })
}

Similar change needs to be done for native platform (if required).

Then we can remove the redundant logic and UI that validates the PDF in the confirmation page here because we would never allow uploading of password-protected PDF and setting it in Onyx in the first place.

What alternative solutions did you explore? (Optional)

We can extend the above approach to work for other cases of PDF failure too, like corrupted PDF. They will have different error reasons.

Result

Working well now, the error shows immediately right in the upload page if the user uploads password-protected PDF.

[dukenv0307]

@tienifr's solution makes sense to me.
You missed handling the case of corrupted pdf, you can fix it in isPdfFilePasswordProtected

doc.promise.catch(()=>{
            reject()
        })

or creating a new function to check if the file is corrupted. It's the expand of validateImageForCorruption

You can do it in PR phase.

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[melvin-bot]

📣 @dukenv0307 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @tienifr 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[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.0-9 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix Password-protected PDF not handled correctly #42572

If no regressions arise, payment will be issued on 2024-06-28. 🎊

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

• @tienifr requires payment automatic offer (Contributor)
• @dukenv0307 requires payment automatic offer (Reviewer)

[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:

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

[melvin-bot]

@dannymcclain, @Julesssss, @kevinksullivan, @tienifr, @dukenv0307 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[kevinksullivan]

Hi @dukenv0307 can you complete the checklist above please?

[dukenv0307]

BugZero Checklist:

• The PR that introduced the bug has been identified. Link to the PR: N/A
• 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
• 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
• Determine if we should create a regression test for this bug. Yes
• 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.

Regression test:

1. FAB > Submit expense > Manual > add amount
2. Select workspace > add merchant > submit the expense
3. Go to IOU detail page> click on the receipt placeholder to add the receipt > choose password-protected PDF
4. Verify that password-protected PDF is not supported. error modal is shown

Do we 👍 or 👎

[dukenv0307]

@kevinksullivan done

[kevinksullivan]

@dukenv0307 what does this mean?

Go to IOU > add password-protected PDF as a recipient

How would you add a PDF as a recipient?

[dukenv0307]

@kevinksullivan I updated the Regression test. Can you help take a look?

[melvin-bot]

@dannymcclain, @Julesssss, @kevinksullivan, @tienifr, @dukenv0307 Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot]

@dannymcclain, @Julesssss, @kevinksullivan, @tienifr, @dukenv0307 Still overdue 6 days?! Let's take care of this!

[melvin-bot]

@dannymcclain, @Julesssss, @kevinksullivan, @tienifr, @dukenv0307 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

[tienifr]

@tienifr requires payment automatic offer (Contributor)

@kevinksullivan I've requested payment via NewDot, no Upwork payment needed 👍

[kevinksullivan]

Looks good @dukenv0307

[kevinksullivan]

all set on payments

[JmillsExpensify]

Re-opening for the payment summary.

[kevinksullivan]

Payment summary

• @tienifr $250 (NewDot)
• @dukenv0307 $250

[JmillsExpensify]

$250 approved for @tienifr