izarutskaya commented 10 months ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com 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:

Go to https://staging.new.expensify.com/
Log in with any Gmail account.
On a conversation, click on the + button in the compose box
Click on add attachment
Upload the following corrupted PDF
Verify that Failed to load PDF file. message appears when previewing a corrupt PDF file
Click on send.
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
[x] Android: mWeb Chrome
[ ] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

https://github.com/Expensify/App/assets/115492554/dee95e1a-5a73-4526-b828-eafb04df47c0

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[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

Bug0 Triage Checklist (Main S/O)

[x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[x] 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
[x] 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.
[x] 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.
[x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

melvin-bot[bot] commented 10 months ago

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

ZhenjaHorbach commented 10 months ago

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

https://github.com/Expensify/App/blob/67b3b59a782cb13c4e53a7686a52345b754f4ef7/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.js#L48-L62

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

https://github.com/Expensify/App/blob/a0b219711080751d6826bb2330ddd85c1641d7bf/src/components/PDFView/index.js#L286-L297

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;

https://github.com/Expensify/App/blob/67b3b59a782cb13c4e53a7686a52345b754f4ef7/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.js#L43

And pass new paramonLoadError here

https://github.com/Expensify/App/blob/67b3b59a782cb13c4e53a7686a52345b754f4ef7/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.js#L63-L68

And then update code here and add onLoadError

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

https://github.com/Expensify/App/blob/67b3b59a782cb13c4e53a7686a52345b754f4ef7/src/components/PDFView/index.js#L286-L297

What alternative solutions did you explore? (Optional)

As an alternative we can use onLoadSuccess instead onLoadError https://github.com/Expensify/App/blob/67b3b59a782cb13c4e53a7686a52345b754f4ef7/src/components/PDFView/index.js#L295

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

https://github.com/Expensify/App/blob/67b3b59a782cb13c4e53a7686a52345b754f4ef7/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.js#L43

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>
        );

https://github.com/Expensify/App/blob/67b3b59a782cb13c4e53a7686a52345b754f4ef7/src/components/PDFView/index.js#L329-L339

ishpaul777 commented 10 months ago

i think this can be handled with https://github.com/Expensify/App/issues/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 https://github.com/Expensify/App/issues/32109 ?

The root cause for https://github.com/Expensify/App/issues/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 https://github.com/Expensify/App/issues/32109#issuecomment-1831562662 (design specs can be discussed with design team)

https://github.com/Expensify/App/assets/104348397/dd42e488-38b6-4c90-bdf0-7c97c77d9f53

cc- @parasharrajat

saranshbalyan-1234 commented 10 months ago

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 https://github.com/Expensify/App/blob/e87d35d8af177df33c4fdfbf221ead732efec310/src/components/PDFView/index.js#L326-L328

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 commented 10 months ago

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:

https://github.com/Expensify/App/blob/398590a6dc649f36ac91c23caca75d57ed844d1e/src/components/PDFView/index.js#L328-L330

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.

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:

https://github.com/Expensify/App/blob/ff18fdde13690fe81ff5d29802814e2817b71fd6/src/components/Attachments/AttachmentView/index.js#L84

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:

https://github.com/Expensify/App/blob/ff18fdde13690fe81ff5d29802814e2817b71fd6/src/components/Attachments/AttachmentView/index.js#L124

Remove the error text here to avoid flickering.
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 commented 10 months ago

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[bot] commented 10 months ago

📣 @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:

Make sure you've read and understood the contributing guidelines.
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.
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.
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 commented 10 months ago

Contributor details Your Expensify account email: anup2230@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~0125eea53463402da3

melvin-bot[bot] commented 10 months ago

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

strepanier03 commented 10 months ago

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

akinwale commented 10 months ago

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[bot] commented 10 months ago

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

ishpaul777 commented 10 months ago

@chiragsalian I do have proposal https://github.com/Expensify/App/issues/32109#issuecomment-1831562662 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 commented 10 months ago

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

chiragsalian commented 10 months ago

@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 https://github.com/Expensify/App/issues/32109 since it looks like the proposal there should solve this issue too right, @akinwale @parasharrajat, or am i missing something here?

parasharrajat commented 10 months ago

I will let you after sometime. AFK ATM.

parasharrajat commented 10 months ago

@chiragsalian IMO both are different issues. We can definitely merge them if you think so.

melvin-bot[bot] commented 10 months ago

@akinwale, @strepanier03, @chiragsalian Eep! 4 days overdue now. Issues have feelings too...

chiragsalian commented 10 months ago

@parasharrajat sure i dont mind keep the separate but can you explain why you think they are separate. Doesn't @ishpaul777 proposal solve the expected solution mentioned here,

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

melvin-bot[bot] commented 10 months ago

@akinwale @strepanier03 @chiragsalian this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] commented 10 months ago

@akinwale, @strepanier03, @chiragsalian Eep! 4 days overdue now. Issues have feelings too...

parasharrajat commented 10 months ago

IMO issues are different. https://github.com/Expensify/App/issues/32204 is talking about adding a preview for errors. https://github.com/Expensify/App/issues/32109 has an irregular height issue where the existing PDFView enlarges to the screen height when the PDF fails to load.

But they can be fixed together as the root cause lies with error UI. So we can handle it in https://github.com/Expensify/App/issues/32109

melvin-bot[bot] commented 10 months ago

@akinwale, @strepanier03, @chiragsalian Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 10 months ago

@akinwale @strepanier03 @chiragsalian this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

melvin-bot[bot] commented 10 months ago

@akinwale, @strepanier03, @chiragsalian 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

melvin-bot[bot] commented 10 months ago

@akinwale, @strepanier03, @chiragsalian Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

parasharrajat commented 10 months ago

@strepanier03 I think we should put this on hold if we are thinking to merge this with other issue.

melvin-bot[bot] commented 10 months ago

@akinwale, @strepanier03, @chiragsalian 12 days overdue now... This issue's end is nigh!

melvin-bot[bot] commented 10 months ago

@akinwale @strepanier03 @chiragsalian this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Decide whether any proposals currently meet our guidelines and can be approved as-is today
If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

melvin-bot[bot] commented 10 months ago

Current assignee @akinwale is eligible for the Internal assigner, not assigning anyone new.

melvin-bot[bot] commented 9 months ago

This issue has not been updated in over 14 days. @akinwale, @strepanier03, @chiragsalian eroding to Weekly issue.

strepanier03 commented 9 months ago

Updated title to reflect the Hold state.

strepanier03 commented 9 months ago

No update yet.

strepanier03 commented 9 months ago

Hmmm, all good here Melvin, on hold still.

strepanier03 commented 8 months ago

Hold GH is open, all good here Melvin.

strepanier03 commented 8 months ago

Still on Hold. All good for now.

strepanier03 commented 8 months ago

Still on hold.

strepanier03 commented 7 months ago

Hold GH is still open.

strepanier03 commented 7 months ago

HOLD GH still open.

strepanier03 commented 7 months ago

HOLD Gh still open.

strepanier03 commented 6 months ago

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

strepanier03 commented 6 months ago

HOLD GH is still open, pausing here.

strepanier03 commented 5 months ago

still on hold

strepanier03 commented 5 months ago

HOLD GH is still open.

chiragsalian commented 3 months ago

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 commented 3 months ago

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

Expensify / App

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

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

Bug0 Triage Checklist (Main S/O)

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)

Why should we merge this with https://github.com/Expensify/App/issues/32109 ?

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)

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)