[HOLD for payment 2023-05-22] [$1000] Large pdf files are allowed to be dragged multiple times and the preview automatically opens up the second time unlike the smaller pdf files and images

[kavimuru]

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:

1. Open web chrome
2. Go to any chat
3. Drag the large pdf file (3-8 MB) onto the chat. (I tested it with documents that I've attached below for medium-large pdfs)
4. Make sure that the document preview is shown
5. Now drag the same large pdf file again on the previous pdf preview
6. Click on send
7. Notice that the pdf file is opened two times and is sent two times
8. But now repeat the same steps for small pdf files (in KB) or on images and notice that even when you drag the same small pdf file or image at the second time, it doesn't show the preview and the file is not uploaded (unlike the previous case where dragging the large pdf in the second time uploaded the file)

Expected Result:

Either both the large and small pdfs should be allowed to be dragged multiple times even when the preview for the previous pdf is open OR none of them should be allowed to be dragged and be uploaded on the second time

Actual Result:

Large pdf files are allowed to be dragged multiple times and the preview automatically opens up the second time unlike the smaller pdf files and images

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.8.8
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

Recording.175.mp4

pdf-2023-04-29_20.37.10.mp4

Cambridge_IELTS_17_with_Answers_Academic__www.luckyielts.com_ (2) (1).pdf

Expensify/Expensify Issue URL:
Issue reported by: @priya-zha
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1682780909172669

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017d5dc9ec54d81805
• Upwork Job ID: 1653229585044373504
• Last Price Increase: 2023-05-02

[MelvinBot]

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

[MelvinBot]

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

[MitchExpensify]

I am unable to reproduce this

[MitchExpensify]

Somewhat curiously I was unable to reproduce this when testing with the IELTS file but could reproduce this with the "Power of your subconscious" file

[MitchExpensify]

I think we should not allow the second "large" file to be uploaded/queued for preview and the behavior should copy the "small" file upload where you cannot drag and drop a second file on a file upload preview.. curious what the engineer and C+ assigned feels with regards to that!

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~017d5dc9ec54d81805

[MelvinBot]

Current assignee @MitchExpensify is eligible for the External assigner, not assigning anyone new.

[MelvinBot]

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

[MelvinBot]

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

[akinwale]

Proposal

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

Dragging a large PDF attachment to the report when a preview of the PDF attachment is already being displayed results in the PDF being uploaded twice.

What is the root cause of that problem?

There are no checks to prevent a file from being dropped in the ReportActionCompose component when an AttachmentModal is displayed. This allows an attachment to be dropped multiple times even when the preview is active.

App/src/pages/home/report/ReportActionCompose.js

Line 775 in ce3b938

<AttachmentModal

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

Add a state variable to the ReportActionCompose component to track if the attachment modal is currently being displayed, and check the value in order to prevent dropping new files.

Here is a quick summary of steps to achieve this. All changes will be made in ReportActionCompose.

1. Add a boolean attachmentPreviewActive state value.
2. Modify the onDrop event handler for the DragAndDrop component.

e.preventDefault();
if (this.state.attachmentPreviewActive) {
    this.setState({isDraggingOver: false});
    return;
}
// ... the existing drop handling code
this.setState({attachmentPreviewActive: true, isDraggingOver: false});

3. Add an attachmentPreviewClosed event handler and bind it in the constructor.

attachmentPreviewClosed() {
    this.setState({attachmentPreviewActive: false});
}

4. Set the onModalHide prop for AttachmentModal to this.attachmentPreviewClosed.

What alternative solutions did you explore? (Optional)

Alternatively, using the attachmentPreviewActive state check, set the disabled prop to this.props.disabled || this.state.attachmentPreviewActive but this results in navigation away from the currently open URL to view the dropped file, leading to a bad user experience.

The original approach is recommended because if a file is dropped while the attachment modal is open, the drop action is simply ignored and the user can proceed to upload or cancel the currently displayed attachment from the preview.

[parasharrajat]

@akinwale why can't we just use the disabled prop on DragNDrop component?

[akinwale]

@akinwale why can't we just use the disabled prop on DragNDrop component?

That can be used also. We can change the disabled prop to this.props.disabled || this.state.attachmentPreviewActive.

[parasharrajat]

What is the difference? Using disabled prop will navigate the page when the user drags an image over the app.

Which one do you suggest & why?

[akinwale]

What is the difference? Using disabled prop will navigate the page when the user drags an image over the app.

Which one do you suggest & why?

It would be better to go with my original proposal because it's terrible UX for a user to be navigated away from the page when they drop a file, expecting that the current page should be able to handle the drop behaviour seamlessly without losing their currently open chats / workspace.

To highlight the differences.
Original proposal
If a file is dropped while the attachment modal is open, the drop action is simply ignored and the user can proceed to upload or cancel the currently displayed attachment from the preview.

Using disabled prop
If a file is dropped while the attachment modal is open, the browser navigates to open the file, and the user loses their current flow, which can be a jarring experience.

[akinwale]

Proposal

Updated

@parasharrajat I have updated my proposal with additional details. Please have a look. Thanks.

[parasharrajat]

Ok, I think we can go with the original proposal. But we will have to make sure that the user is seeing a no-drop icon on the cursor while dropping for this issue.

@akinwale's proposal looks good to me.

cc: @thienlnam

🎀 👀 🎀 C+ reviewed

[akinwale]

But we will have to make sure that the user is seeing a no-drop icon on the cursor while dropping for this issue.

@parasharrajat This would require a lot of refactoring because the DragAndDrop component is below the AttachmentModal component after the file has finished loading. Note that the issue reported is happening for larger files because the drag and drop appears to be able to receive drop events while the file preview is still in the process of loading.

[parasharrajat]

Can you propose here what is needed to make the no-drip cursor so that I can review?

[akinwale]

Can you propose here what is needed to make the no-drip cursor so that I can review?

@parasharrajat I did some testing and based on my findings, the refactor may not be as much as initially thought. The following changes will need to be made to AttachmentModal.

1. Add an event listener to handle dragenter and dragover events in componentDidMount, bind in the constructor and remove in componentWillUnmount.
2. Set the event.dataTransfer.dropEffect to none in the event handler if this.state.isModalOpen is true. Reference for more details. Note that this may not actually display a no-drop icon, depending on the platform being tested on. It took me a while to figure this out.

Here's what the changes would look like.

//... constructor code
    this.dragEnterOverListener = this.dragEnterOverListener.bind(this);
}

componentDidMount() {
    document.addEventListener('dragenter', this.dragEnterOverListener);
    document.addEventListener('dragover', this.dragEnterOverListener)
}

componentWillUnmount() {
    document.removeEventListener('dragenter', this.dragEnterOverListener);
    document.removeEventListener('dragover', this.dragEnterOverListener);
}

dragEnterOverListener(event) {
     event.preventDefault();
     if (this.state.isModalOpen) {
         event.dataTransfer.dropEffect = 'none';
     }
}

[parasharrajat]

Ok, let's leave this part. I don't think it is very necessary to add a blocking cursor over the AttachmentModal.

[melvin-bot]

@akinwale, @parasharrajat, @thienlnam, @MitchExpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[MitchExpensify]

PR is merged, switching to weekly - Added a calendar reminder for payment 👍

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[mountiny]

Noting there was a regression from this PR so we should adjust the rewards accordingly

[melvin-bot]

@akinwale @parasharrajat @thienlnam @MitchExpensify 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]

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

• fix: prevent dropping files in the drop zone if the attachment modal is active #18624

If no regressions arise, payment will be issued on 2023-05-22. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

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

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

[MitchExpensify]

Everyone's paid! Thanks again

@parasharrajat if you could identify the PR that introduced the bug as a next step, that'd be great!

[parasharrajat]

This is a new find. We never prevented such behavior in the past so it is not a regression.

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

[@parasharrajat] 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: NA

[@parasharrajat] 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: NA

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

[@parasharrajat] 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.

#18210 (comment)

[akinwale]

Everyone's paid! Thanks again

Received $1500, thanks. This includes the 50% bonus, but there was a regression with my PR which caused a deploy blocker. Considering this falls within the regression period according to the contributor guide, I shouldn't have received this bonus. Is there an exception here or is there anything I need to do?

[parasharrajat]

Regression Test Steps

1. Open app on chrome browser.
2. Go to any chat
3. Drag the large pdf file (3-8 MB) onto the chat. (I tested it with documents that I've attached below for medium-large pdfs)
4. Make sure that the document preview is shown
5. Now drag the same large pdf file again on the previous pdf preview
6. Click on send
7. Notice that the pdf file is opened and sent once.

Do you agree 👍 or 👎 ?

[thienlnam]

cc @MitchExpensify

Seems that we've missed a regression deduction
#18210 (comment)
#18210 (comment)

[MitchExpensify]

Gahhh, my mistake on the overpayment. I believe there is a refund feature in Upwork. Mind checking that out for me @akinwale @parasharrajat ? Sorry about that, I appreciate the catch

[akinwale]

Gahhh, my mistake on the overpayment. I believe there is a refund feature in Upwork. Mind checking that out for me @akinwale @parasharrajat ? Sorry about that, I appreciate the catch

Refunded $500.

[parasharrajat]

Refunded $1000

[MitchExpensify]

Thank you both! Request the test be added here https://github.com/Expensify/Expensify/issues/286533