[HOLD for payment 2024-07-24][$250] mWeb/Safari - Attachments - New tab doesn't open when clicking the download button from the overflow menu on a .doc file

[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.84-2
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/4636547
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

1. Navigate to a conversation
2. Upload a .doc attachment to the chat
3. Long press the .doc attachment
4. Click download

Expected Result:

Download starts on a new tab

Actual Result:

Download starts on the same tab

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

Bug6516195_1718645800178.IMG_9378.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~018590692829acb48f
• Upwork Job ID: 1802929010080656004
• Last Price Increase: 2024-06-25
• Automatic offers:
• dukenv0307 | Contributor | 102939688
• Krishna2323 | Contributor | 102961763

Issue Owner

Current Issue Owner: @dukenv0307

[melvin-bot]

Triggered auto assignment to @jliexpensify (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]

@jliexpensify 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

[jliexpensify]

ios <> Android swapsies

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~018590692829acb48f

[melvin-bot]

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

[Krishna2323]

Proposal

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

Opening a new tab is expected behaviour but it should be the same for context menu download button.

Inconsistency bug

safari_file_download_inconsistency.mp4

What is the root cause of that problem?

We are not passing the shouldOpenExternalLink parameter when downloading the attachment from context menu.

App/src/pages/home/report/ContextMenu/ContextMenuActions.tsx

Line 507 in b335423

fileDownload(sourceURLWithAuth, originalFileName ?? '').then(() => Download.setDownload(sourceID, false));

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

We should check if the attachment html is link and pass the shouldOpenExternalLink parameter accordingly.

to:

const isLinkAttachment = html.startsWith('<a href');
fileDownload(sourceURLWithAuth, originalFileName ?? '', undefined, isLinkAttachment && Browser.isMobileSafari()).then(() => Download.setDownload(sourceID, false));

We should also check on other places where fileDownload is used.

What alternative solutions did you explore? (Optional)

[nkdengineer]

Proposal

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

A new tab opens and the download starts

What is the root cause of that problem?

We pass shouldOpenExternalLink as true if the browser is safari. It's added to fix this issue #37131.

App/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.tsx

Line 52 in b335423

fileDownload(sourceURLWithAuth, displayName, '', Browser.isMobileSafari()).then(() => Download.setDownload(sourceID, false));

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

I think we don't need to open a new tab when downloading the file in mWeb safari.

fileDownload(sourceURLWithAuth, displayName, '').then(() => Download.setDownload(sourceID, false)); 

App/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.tsx

Line 52 in b335423

fileDownload(sourceURLWithAuth, displayName, '', Browser.isMobileSafari()).then(() => Download.setDownload(sourceID, false));

Instead, we will do nothing when clicking on the download button if a file is downloading in mWeb safari. To do that we can get all download collection and check we're downloading a file or not. Or we can create another Onyx key to know whether we download a file.

What alternative solutions did you explore? (Optional)

[DylanDylann]

@nkdengineer Did you test your proposal with the previous problem? Please provide a test branch then I can ensure that there are no risks if we revert to the previous solution

[DylanDylann]

@Krishna2323 Please keep your proposal consistent.

Please re-state the problem that we are trying to solve in this issue.
you said: "new tab open" is the problem that we are trying to solve in this issue

What changes do you think we should make in order to solve the problem?
you suggest: new tab should open when clicking download button on context menu"

Two these sections are conflict

[DylanDylann]

@Krishna2323 One more thing

We should also check on other places where fileDownload is used.

Which places you want to mention here?

[Krishna2323]

@DylanDylann, I updated my proposal code snippet part. We also need to check if the attachment is a link or not before passing Browser.isMobileSafari().

Two these sections are conflict

In RCA section I explained the reason for the behaviour and inconsistency.

This is expected behaviour for mWeb safari, the behaviour was added in this #37131 to fix multiple issue in mWeb safari and desktop app, however it seems like we missed adding the same behaviour when downloading files from context menu.

I think we only need to make change for the context menu.

[DylanDylann]

mWeb/Safari - Attachments - New tab opens when clicking the download button on a .doc file

This is not the problem you are trying to solve

[DylanDylann]

@Krishna2323 In this section

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

Please re-state the problem you see and you are trying to resolve in your proposal. I don't see any value in this section when you copy the issue title. On the contrary, it confuses me

[Krishna2323]

@DylanDylann, opening a new tab is expected behaviour and we should do the same when downloading from the attachment from context menu.

Please re-state the problem you see and you are trying to resolve in your proposal. I don't see any value in this section when you copy the issue title. On the contrary, it confuses me

Sorry for that, updated the proposal.

[nkdengineer]

@DylanDylann The test branch here.

[melvin-bot]

@kadiealexander, @DylanDylann Eep! 4 days overdue now. Issues have feelings too...

[DylanDylann]

@dukenv0307 will take this issue

[kadiealexander]

@dukenv0307 sorry for the delay! Please take a look at the proposals.

[dukenv0307]

Thanks for all your proposals @nkdengineer @Krishna2323. I want to re-confirm the expectations.

1. Opening the new tab seems to be the expected behavior according to this.
2. But If users long press on the message, then click download, it doesn't open the new tab. Is it the expectation?

IMO, we should fix 2 to match 1 like @Krishna2323's proposal. (We can use REGEX_LINK_IN_ANCHOR to detect the anchor attachment)

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[dukenv0307]

Pls let me know what your thought ^ @srikarparsi

[melvin-bot]

@srikarparsi @kadiealexander @dukenv0307 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!

[srikarparsi]

Yes, this sounds good. I updated the issue's title and description.

[melvin-bot]

📣 @Krishna2323 🎉 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 📖

[dukenv0307]

Waiting for your PR @Krishna2323

[Krishna2323]

Will raise PR by end of tomorrow.

[Krishna2323]

@dukenv0307, PR ready for review ^

[kadiealexander]

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: N/A
• [@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: N/A
• [@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: N/A
• [@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.
• [@kadiealexander] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/417767

[kadiealexander]

Payouts due:

• Contributor: $250 @Krishna2323 (here)
• Contributor+: $250 @dukenv0307 (here)

Upwork job is here.

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

1. Navigate to a conversation
2. Upload a .doc attachment to the chat
3. Long press the .doc attachment
4. Click download
5. Verify download option is opened in a new tab on mweb safari, for other platforms, the file is downloaded in this tab/screen

Do we 👍 or 👎