[$1000] mWeb safari - attachments from Concierge don't download

[trjExpensify]

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. Navigate to the Concierge DM
2. Request sample attachments for testing. Ask for multiple images in a single message, another message with a single image, and a third message with multiple attachments (i.e PDF)
3. Once received, download the attachments

Expected Result:

All attachments should be downloadable. Note that due to CORS, locally these might open in a new window.

Actual Result:

Attachments don't download

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: v1.3.21-2
Reproducible in staging?: Y
Reproducible in production?:
If this was caught during regression testing, add the test name, ID and link from TestRail:
Found when executing the QA of this PR but might not be related.
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:
https://github.com/Expensify/App/assets/16232057/79dd7048-d035-40db-91cb-8e48b5cddcae

Expensify/Expensify Issue URL:
Issue reported by: @trjExpensify
Slack conversation:

CC: @kidroca @hayata-suenaga

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01471efa1f7f540ed7
• Upwork Job ID: 1665881749151117312
• Last Price Increase: 2023-06-27

[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

[strepanier03]

Started the testing for this one too.

[kidroca]

Firstly, let's confirm whether the issue occurs exclusively with Concierge attachments. Based on my understanding, this seems to be the case.

The existing fileDownload function (particularly in its web version) appears to be incompatible with attachments that don't originate from an Expensify domain, such as Amazon S3. The root of the issue lies in how the fetch API is used.

App/src/libs/fileDownload/index.js

Lines 10 to 12 in 56379d1

	export default function fileDownload(url, fileName) {
	return new Promise((resolve) => {
	fetch(url)

When the download button is pressed, a fetch request is initiated, but it is blocked from downloading files from non-Expensify domains. This triggers an alternative handling process in which the file URL is opened in a new window.

App/src/libs/fileDownload/index.js

Lines 40 to 42 in 56379d1

	.catch(() => {
	// file could not be downloaded, open sourceURL in new tab
	Linking.openURL(url);

However, this approach encounters limitations with certain browsers that may block attempts to open files in a new window, especially when the URL's origin differs from that of the initiating window.

To overcome these limitations, I propose the development of a dedicated "Download" component that would function similarly to an Anchor tag on the web.

By rendering an anchor tag in the document and triggering it to download the file, we establish a more robust strategy. This approach allows the browser to "see" the actual path (external origin) from which the user is initiating the file download.

Finally, we may need to consider the CORS configuration of the S3 bucket. Depending on the current configuration, it might block the site from downloading the file. In such a scenario, adjusting the CORS configuration on the S3 bucket to allow requests from the Expensify App's domain could resolve the issue.

[melvin-bot]

@strepanier03 Whoops! This issue is 2 days overdue. Let's get this updated quick!

[strepanier03]

I was having Concierge issues but got them sorted out and have some testing notes.

---

Multiple images in a single message

(Images added via drag and drop)

When I tried to download the images they opened in another tab but there was no option to download and they didn't appear in my downloads folder. I had to right-click and save or drag them to my desktop.

---

A single image in a single message

(Image added via drag and drop)

As with the previous test, when I tried to download the images they opened in another tab but there was no option to download and they didn't appear in my downloads folder. I had to right-click and save or drag them to my desktop.

---

Multiple attachments in a single message

(image added as attachment using paperclip icon in Concierge)

The same behavior happened with these as well. Clicking the link didn't download the image, just opened it in another tab.

[strepanier03]

@trjExpensify - I'm able to recreate this so I'm sending external, which I believe is the right move. If you have any other thoughts or feedback let me know.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01471efa1f7f540ed7

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[trjExpensify]

@trjExpensify - I'm able to recreate this so I'm sending external, which I believe is the right move. If you have any other thoughts or feedback let me know.

Yep, I think that's right and let's start with a review of @kidroca's proposal here.

As we're talking about the config of the S3 bucket, going to CC you @justinpersaud for vis incase there's any overlap here with the server migration project.

[strepanier03]

@0xmiroslav - Proposal ready for your review here.

[strepanier03]

@0xmiroslav any thoughts on the proposal here??

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[0xmiros]

I got help from Concierge so now have s3 attachment links. Reviewing proposal shortly.

[melvin-bot]

@strepanier03 @jasperhuangg @0xmiroslav 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]

@kidroca, @kevinksullivan, @jasperhuangg, @aimane-chnaif Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot]

@kidroca, @kevinksullivan, @jasperhuangg, @aimane-chnaif Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[kidroca]

Update:

1. Progress: The PR is nearly complete. I am in the process of finalizing the test steps.

2. Test Videos: I am currently working on creating test videos specifically for Android.

3. Bug Discovery:

   • Encountered an issue with downloading attachments from Concierge on both iOS and Android.
   • Details of the bug are available here.
   • This bug seems reminiscent of the current issue only affecting mWeb, which was related to CORS.
   • Upon further investigation, I traced the origin of this new bug. It appears to be a regression from this ticket.

To ensure the PR is complete and testable across all platforms, I'll need to address and fix this newly identified bug.

[jasperhuangg]

@kidroca Thanks for the update! I commented in the other issue. We'll have the contributor who introduced the regression fix it.

[kidroca]

@jasperhuangg
Thank you for addressing the regression. Given the situation, I have a few concerns:

1. Progress on Current Ticket: I'm keen to ensure there are no repercussions on my side for the delay caused by this regression. What's our course of action in such instances?

2. Ticket Status: Considering the regression, should we:

   • Put the current ticket on hold until the regression is addressed?
   • Or proceed with my PR, but with the understanding that testing on iOS/Android might not be feasible due to the regression?

3. Regarding the Regression Source:

   • I've taken the time to trace the bug back to a prior ticket. Interestingly, that ticket appeared to have been marked as completed after its designated regression period.
   • Given this, I'm curious as to how we can expect the original contributor to address the fix.
   • I went the extra mile to ensure we have a clear understanding of the bug's origin. My aim was to provide as much clarity as possible for the team. Moving forward, it would be helpful to know the team's stance on recognizing such additional efforts, as it could guide how we approach similar situations in the future (e.g. I was hoping to at least get a reporting bonus).

Awaiting your guidance on the way forward.

[jasperhuangg]

Or proceed with my PR, but with the understanding that testing on iOS/Android might not be feasible due to the regression?

I think this makes sense. We can make a note in the other PR that they'll need ensure that they test the flows introduced in this issue as well.

Moving forward, it would be helpful to know the team's stance on recognizing such additional efforts, as it could guide how we approach similar situations in the future (e.g. I was hoping to at least get a reporting bonus).

I definitely think you're eligible for a reporting bonus here. cc @kevinksullivan for visibility!

[kidroca]

@jasperhuangg

Sorry for delving into this perhaps more than necessary, but it seems less than ideal in this case to ask the original contributor to address the regression in a separate PR. I've already brought up the regression on the original PR, and the contributor suggested the same changes I already applied in this commit: 5b28515ca30c74bcbd0033fa2d16bdf1591df9a5

Do we really want me to revert my fix and let the original contributor apply the same fix in a separate PR? It seems like this might create unnecessary complexity and redundancy.

I'm open to following the agreed-upon process, but I wanted to bring this to your attention in case there's a more efficient way to handle this situation

[aimane-chnaif]

@kidroca there's another regression from that PR - #25491 (comment)
I think better to put your PR on hold until it's fixed and deployed.

[kidroca]

Hey guys, it seems the work here has stalled due to the regressions found. There doesn't seem to be anything that I can do at this moment. My PR was published a few days ago, and it appears we'll have to wait for the Android/iOS fixes before it's reviewed.

[aimane-chnaif]

The holding PR was deployed to production.
Now we can continue this issue

[kidroca]

@aimane-chnaif

The holding PR was deployed to production. Now we can continue this issue.

Thank you, @aimane-chnaif. However, the PR you mentioned only addresses one of the existing regressions.

@kidroca there's another regression from that PR - #25491 (comment)
I think better to put your PR on hold until it's fixed and deployed.

To clarify, the "another" regression you're referring to—concerning file names with illegal characters—is the one resolved just now.
The remaining regression is when a fileName is missing which happens for non-user made attachments like some Concierge attachments that are embedded via drag and drop

@jasperhuangg

@kidroca Thanks for the update! I commented in the other issue. We'll have the contributor who introduced the regression fix it.

It seems that the regression issue remains unaddressed and inactive. A separate ticket has been created for it: Expensify/App#26328.

I'd like to highlight that my existing PR #25556 offers a ready-to-deploy fix for this issue. Although the initial plan was for the original author to submit a new PR, there has been no progress on that front. My PR contains a one-line fix that could expedite the resolution of this issue.

[melvin-bot]

This issue has not been updated in over 15 days. @kidroca, @kevinksullivan, @jasperhuangg, @aimane-chnaif eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[kidroca]

Hey team,

I've been away since September 15th and just noticed that this issue is still pending. For context, you can refer to my earlier comment.

Could we get an update on the following:

Has the original contributor managed to fix the regression?
If not, should I proceed to fix the regression to expedite resolution?
Looking forward to your input.

Thank you!

[aimane-chnaif]

I am not sure but if that regression still happens, I think we can fix together.
Either way, I think we can proceed PR.
@kidroca please merge main, and I will review today

[kidroca]

Thanks @aimane-chnaif, I synced with main

[aimane-chnaif]

@kevinksullivan this is ready for payment.

[kevinksullivan]

@aimane-chnaif @kidroca the previous offer expired, so can you let me know when you accept the new offer?

https://www.upwork.com/jobs/~01e842e00764cb1614

[kidroca]

@aimane-chnaif @kidroca the previous offer expired, so can you let me know when you accept the new offer?

https://www.upwork.com/jobs/~01e842e00764cb1614

I'm part of the program for payments through App, so I'll open a request there

[JmillsExpensify]

Reviewed @kidroca's invoice as part of this issue. I approve payment.

[kevinksullivan]

Ah ok, sorry about that. I'll close this out if we're all set then.

[aimane-chnaif]

@kevinksullivan my payment is remaining