[HOLD - #32109] [$500] Attachment – The corrupted PDF can be downloaded, but no download indication is displayed

[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.5-3
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
Expensify/Expensify Issue URL:
Issue reported by: Applause-Internal Team
Slack conversation: @

Action Performed:

1. Go to https://staging.new.expensify.com/
2. Log in with any Gmail account.
3. On a conversation, click on the + button in the compose box
4. Click on add attachment
5. Upload the following corrupted PDF
6. Verify that Failed to load PDF file. message appears when previewing a corrupt PDF file
7. Click on send.
8. Click on “Failed to load PDF file.”

Expected Result:

The corrupted file should have the same preview component and download component as .mp4/docs.

Actual Result:

The corrupted PDF can be downloaded by clicking on it, but no download indication is displayed.

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

Bug6294834_1701276250661.Corrupted_PDF.mp4

Corrupted PDF.pdf

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01a226f1589481a1b1
• Upwork Job ID: 1729917217011929088
• Last Price Increase: 2023-11-29

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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]

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

[ZhenjaHorbach]

Proposal

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

Attachment – The corrupted PDF can be downloaded, but no download indication is displayed

What is the root cause of that problem?

The main problem is that since we are using the onPress function by default

App/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.js

Lines 48 to 62 in 67b3b59

	<PressableWithoutFeedback
	style={props.style}
	onPress={() => {
	if (isDownloading) {
	return;
	}
	Download.setDownload(sourceID, true);
	fileDownload(sourceURLWithAuth, fileName).then(() => Download.setDownload(sourceID, false));
	}}
	onPressIn={props.onPressIn}
	onPressOut={props.onPressOut}
	onLongPress={(event) => showContextMenuForReport(event, anchor, report.reportID, action, checkIfContextMenuActive, ReportUtils.isArchivedRoom(report))}
	accessibilityLabel={fileName}
	role={CONST.ACCESSIBILITY_ROLE.BUTTON}
	>

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

Since we clearly have no information that our file is broken
We can obtain this information using a special method onLoadError

App/src/components/PDFView/index.js

Lines 286 to 297 in a0b2197

	<Document
	error={<Text style={this.props.errorLabelStyles}>{this.props.translate('attachmentView.failedToLoadPDF')}</Text>}
	loading={<FullScreenLoadingIndicator />}
	file={this.props.sourceURL}
	options={{
	cMapUrl: 'cmaps/',
	cMapPacked: true,
	}}
	externalLinkTarget="_blank"
	onLoadSuccess={this.onDocumentLoadSuccess}
	onPassword={this.initiatePasswordChallenge}
	>

So we can add new code here

    const [isLoadError, setLoadError] = React.useState(false);

    const onLoadError = () => {
        setLoadError(true);
    };

    const isDownloading = props.download && props.download.isDownloading || isLoadError;

App/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.js

Line 43 in 67b3b59

const isDownloading = props.download && props.download.isDownloading;

And pass new param onLoadError here

App/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.js

Lines 63 to 68 in 67b3b59

	<AttachmentView
	source={sourceURLWithAuth}
	file={{name: fileName}}
	shouldShowDownloadIcon
	shouldShowLoadingSpinnerIcon={isDownloading}
	/>

And then update code here and add onLoadError

                        onLoadError={() => {
                            this.props.onLoadError();
                        }}

App/src/components/PDFView/index.js

Lines 286 to 297 in 67b3b59

	<Document
	error={<Text style={this.props.errorLabelStyles}>{this.props.translate('attachmentView.failedToLoadPDF')}</Text>}
	loading={<FullScreenLoadingIndicator />}
	file={this.props.sourceURL}
	options={{
	cMapUrl: 'cmaps/',
	cMapPacked: true,
	}}
	externalLinkTarget="_blank"
	onLoadSuccess={this.onDocumentLoadSuccess}
	onPassword={this.initiatePasswordChallenge}
	>

What alternative solutions did you explore? (Optional)

As an alternative we can use onLoadSuccess instead onLoadError

App/src/components/PDFView/index.js

Line 295 in 67b3b59

onLoadSuccess={this.onDocumentLoadSuccess}

As soon as we successfully download the element we will enable the download

App/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.js

Line 43 in 67b3b59

const isDownloading = props.download && props.download.isDownloading;

---

As an alternative, we can update the render method to disable events coming from parent components

like

        return this.props.onPress ? (
            <PressableWithoutFeedback
                onPress={(event) => {
                    if (this.state.isLoadError) {
                        // OR !this.state.isLoadSuccess
                        event.preventDefault();
                        event.stopPropagation();
                    }
                    this.props.onPress();
                }}
                style={[styles.flex1, styles.flexRow, styles.alignSelfStretch, {backgroundColor: 'red'}, {height: 200, width: 200}]}
                role={CONST.ACCESSIBILITY_ROLE.IMAGEBUTTON}
                accessibilityLabel={this.props.fileName || this.props.translate('attachmentView.unknownFilename')}
            >
                {this.renderPDFView()}
            </PressableWithoutFeedback>
        ) : (
            <PressableWithoutFeedback
                onPress={(event) => {
                    if (this.state.isLoadError) {
                        // OR !this.state.isLoadSuccess
                        event.preventDefault();
                        event.stopPropagation();
                    }
                }}
                style={[styles.flex1, styles.flexRow, styles.alignSelfStretch, {backgroundColor: 'red'}, {height: 200, width: 200}]}
                accessibilityLabel={this.props.fileName || this.props.translate('attachmentView.unknownFilename')}
            >
                {this.renderPDFView()}
            </PressableWithoutFeedback>
        );

App/src/components/PDFView/index.js

Lines 329 to 339 in 67b3b59

	<PressableWithoutFeedback
	onPress={this.props.onPress}
	style={[styles.flex1, styles.flexRow, styles.alignSelfStretch]}
	role={CONST.ACCESSIBILITY_ROLE.IMAGEBUTTON}
	accessibilityLabel={this.props.fileName || this.props.translate('attachmentView.unknownFilename')}
	>
	{this.renderPDFView()}
	</PressableWithoutFeedback>
	) : (
	this.renderPDFView()
	);

[ishpaul777]

i think this can be handled with #32109 Download feature should be indicated as if by mistake user deletes the file there is no way to get back without this feature and this can be a frustration for user, there should be a indication that user can download when clicking the preview.

Why should we merge this with #32109 ?

The root cause for #32109 is related to resizing of the inverted list item dynamicaly if the item is not resized (we show the loading indicator for pdf preview and currupted pdf preview same size both issue can be resolved.

POC from #32109 (comment) (design specs can be discussed with design team)

Screen.Recording.2023-11-29.at.3.17.08.PM.mov

cc- @parasharrajat

[saranshbalyan-1234]

Proposal

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

The corrupted PDF can be downloaded, but no download indication is displayed

What is the root cause of that problem?

We are using PressableWithoutFeedback component even before PDF is loaded or failed to load here

App/src/components/PDFView/index.js

Lines 326 to 328 in e87d35d

	return this.props.onPress ? (
	<PressableWithoutFeedback
	onPress={this.props.onPress}

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

by default we should not use the PressableWithoutFeedback component,
We should create a new state i.e. pdfLoaded which we will initiate as false, that we will set true during onLoadSucess like this

    onDocumentLoadSuccess(pdf) {
        const {numPages} = pdf;

        Promise.all(
            _.times(numPages, (index) => {
                const pageNumber = index + 1;

                return pdf.getPage(pageNumber).then((page) => page.getViewport({scale: 1}));
            }),
        ).then((pageViewports) => {
            this.setState({
                pageViewports,
                numPages,
                shouldRequestPassword: false,
                isPasswordInvalid: false,
               pdfLoaded:true
            });
        });
    }

and then

 render() {
        return this.props.onPress && this.state.pdfLoaded ?

Note: the reason why we should not use onLoadError is because, this would turn off the PressableWithoutFeedback component once there is an error, that means by default you can use this to download while its loading
What we have to do is we have to enable press once the pdf is loaded succesfully

What alternative solutions did you explore? (Optional)

N/A

[tienifr]

Proposal

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

Attachment – The corrupted PDF can be downloaded, but no download indication is displayed

What is the root cause of that problem?

We always allow onPress if there's a onPress function here:

App/src/components/PDFView/index.js

Lines 328 to 330 in 398590a

	return this.props.onPress ? (
	<PressableWithoutFeedback
	onPress={this.props.onPress}

That's why the PDF file is always downloadable.

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

The OP expectation is to show download indicator for corrupted file because it is downloadable. In other words, we should render the default attachment view with clip and download icons if the PDF is corrupted.

1. To detect the file error, we should introduce a new state called pdfError; and a function prop onLoadError for AttachmentViewPdf component which would consquently be passed to react-pdf, to set pdfError. This is what we've been doing for image file:

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

Line 84 in ff18fdd

const [imageError, setImageError] = useState(false);

2. We should add another condition to render PDF preview here that the file does not have error, so that it would fallback to default attachment view:

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

Line 124 in ff18fdd

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

3. Remove the error text here to avoid flickering.

4. For file name preview here, we should render Failed to load PDF if pdfError. We should also add some styles here to indicate an error not a real file name.

As such, in case the pdf is loaded with error, it would fallback to default attachment view like this (just for reference purpose, we should wait for design team to weigh in here):

or:

What alternative solutions did you explore? (Optional)

We can forbid downloading corrupted PDF file by detecting whether the document is loaded successfully (by onLoadSuccess) and only allow onPress function to be triggered then.

[anup2230]

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

The corrupted PDF can be downloaded, but no download indication is displayed

What is the root cause of that problem?

We are using PressableWithoutFeedback component before PDF is loaded or failed to load in ~/PDFView/index.js.

render() {
        return this.props.onPress ? (
            <PressableWithoutFeedback
                onPress={this.props.onPress}

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

We can use the available state 'failedToLoadPDF' as an additional condition before using the PassibleWithoutFeedback component. Currently, the 'faileToLoadPDF' state is being set to false given that the pdf failed to load. We can utilize this and make a minimal change to solve this issue.

render() {
        return this.props.onPress && !this.state.failedToLoadPDF ? (
            <PressableWithoutFeedback
                onPress={this.props.onPress}

This should only enable the download onClick when the PDF is loaded properly, utilizing the 'failedtoLoadPDF' state.

[melvin-bot]

📣 @anup2230! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[anup2230]

Contributor details
Your Expensify account email: [email protected]
Upwork Profile Link: https://www.upwork.com/freelancers/~0125eea53463402da3

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[strepanier03]

@akinwale - When you have some time we have some proposals.

[akinwale]

After reviewing the proposals, I recommend moving forward with @tienifr's proposal as it follows the existing model used for image attachments, and includes improvements for the display of the failed status.

🎀 👀 🎀 C+ reviewed.

[melvin-bot]

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

[ishpaul777]

@chiragsalian I do have proposal #32109 (comment) in #32109 (proposed before this issue is created), that will solve this issue too, Please take a look and see if combining issues makes more sense

[parasharrajat]

@akinwale Could you please request a design review here as this involves design changes?

[chiragsalian]

@ishpaul777 yeah it looks like your proposal was made slightly earlier and sorta solves this issue too. I do like @tienifr proposal showing a cross with failed to load PDF file but i think that visual improvment might be a different issue. I like solving for displaying the corrupted PDT and having a download indication which you @ishpaul777 seem to solve too.

Thoughts on closing this issue in favor for #32109 since it looks like the proposal there should solve this issue too right, @akinwale @parasharrajat, or am i missing something here?

[strepanier03]

Still on hold.

[strepanier03]

Hold GH is still open.

[strepanier03]

HOLD GH still open.

[strepanier03]

HOLD Gh still open.

[strepanier03]

Hold GH looks like it's going to move forward soon as we've decided on the behavior.

[strepanier03]

HOLD GH is still open, pausing here.

[strepanier03]

still on hold

[strepanier03]

HOLD GH is still open.

[chiragsalian]

Looks like the linked GH issue is closed. @akinwale @parasharrajat @strepanier03, can someone test and confirm if this issue still exists. Also can someone confirm if this is an internal or external issue.

[ishpaul777]

This issue is fixed! We are good to close this one