[$250] Concierge- Concierge video download button open the video in another tab rather than download

[IuliiaHerets]

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: v9.0.70-9 Reproducible in staging?: Yes Reproducible in production?: Yes If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Email or phone of affected tester (no customers): [email protected] Issue reported by: Applause Internal Team

Action Performed:

1. Navigate to https://staging.new.expensify.com
2. Create a new account - Select the purpose "Chat and split expenses with friends" and complete onboarding process
3. Navigate to Concierge video and expand the video to see download button
4. Click download button

Expected Result:

The download button on the video will directly download the video (other attachment videos behavior)

Actual Result:

Concierge video download button open the video in another tab rather than downloading the video

Workaround:

Unknown

Platforms:

• [ ] Android: Standalone
• [ ] Android: HybridApp
• [x] Android: mWeb Chrome
• [ ] iOS: Standalone
• [ ] iOS: HybridApp
• [ ] iOS: mWeb Safari
• [x] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/4b8d27ad-bfa4-4aca-8f27-aaeb6db2a2db

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021864480510463449920
• Upwork Job ID: 1864480510463449920
• Last Price Increase: 2024-12-05

Issue Owner

Current Issue Owner: @sobitneupane

[melvin-bot[bot]]

Triggered auto assignment to @VictoriaExpensify (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.

[VictoriaExpensify]

Able to reproduce, agree we should fix it.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[ikevin127]

This is done on purpose by the following logic:

https://github.com/Expensify/App/blob/0e28aa4d7ab0e5ba4d2b050c47dc78b35a45992f/src/libs/fileDownload/DownloadUtils.ts#L44-L47

The download button on the video will directly download the video (other attachment videos behavior)

Probably (other attachment videos behavior) refers to blobs or local files which are indeed handled differently, but since these videos are hosted on CDN they are categorized as external which, as stated by the comment in the code block above, might pose a CORS issue during direct downloads.

[code4161]

Proposal

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

Concierge video download button open the video in another tab( since the video URL is an external URL ), rather than downloading the video

What is the root cause of that problem?

The fetchFileDownload function in the DownloadUtils file handles the download and it has the following condition to evaluate whether the file should be downloaded or opened in new tab:

if (
        shouldOpenExternalLink ||
        (!isApiUrl && !isAttachmentUrl && !isSageUrl)
    )
   {
        // Different origin URLs might pose a CORS issue during direct downloads.
        // Opening in a new tab avoids this limitation, letting the browser handle the download.
        Link.openExternalLink(url);
        return Promise.resolve();
    }

We have the function called through fileDownload from the AttachmentModal component with shouldOpenExternalLink parameter as default value(false).

const fileDownload: FileDownload = (url, fileName, successMessage = '', shouldOpenExternalLink = false, formData = undefined, requestType = 'get', onDownloadFailed?: () => void) =>
    fetchFileDownload(url, fileName, successMessage, shouldOpenExternalLink, formData, requestType, onDownloadFailed);

if (typeof sourceURL === 'string') {
            const fileName = type === CONST.ATTACHMENT_TYPE.SEARCH ? FileUtils.getFileName(`${sourceURL}`) : file?.name;
            fileDownload(sourceURL, fileName ?? '');
 }

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

Changes in fetchFileDownload function:

• Modify the fetchFileDownload function by adding a new optional parameter - forceDownload with default value as false.
• Modify the logic in fetchFileDownload function to check for this flag before opening links in new tab.
• In case of CORS issue, there is an existing logic to open the attachment in new tab. Will also check it that is working.

Changes in fileDownload function:

• Modify the fileDownload function by adding a new optional parameter - forceDownload with default value as false.
• Include the parameter in the fetchFileDownload function call.

Changes in AttachmentModal component:

• Check if the content is from Concierge workspace by comparing the account id in params with the ACCOUNT_ID.CONCIERGE constant data in CONST file.
• If it is found to be from Concierge workspace, call the fileDownload function with the forceDownload parameter or else go with the current flow.

Test: Add required test cases.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

N/A

What alternative solutions did you explore? (Optional):

N/A

Thanks!

[melvin-bot[bot]]

📣 @code4161! 📣 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>

[code4161]

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

[melvin-bot[bot]]

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

[saifelance]

@MelvinBot Do you also have my details?

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

[sobitneupane]

Thanks @ikevin127

@VictoriaExpensify, As mentioned by @ikevin127 here, it is done on purpose to avoid CORS issue.

[code4161]

Hi @sobitneupane , there is a fallback present in the code to open the video/image in another tab in case of CORS issue.

[muttmuure]

Do we need to fix this?

[muttmuure]

It doesn't seem that important

[muttmuure]

Anyway, this is not a quality issue, so removing from the project

[sobitneupane]

@code4161

there is a fallback present in the code to open the video/image in another tab in case of CORS issue.

Can you please add the permalink for the code you suggested above?

If we already have fallback present, is it safe to remove following block of code?

if (
        shouldOpenExternalLink ||
        (!isApiUrl && !isAttachmentUrl && !isSageUrl)
    )
   {
        // Different origin URLs might pose a CORS issue during direct downloads.
        // Opening in a new tab avoids this limitation, letting the browser handle the download.
        Link.openExternalLink(url);
        return Promise.resolve();
    }

[code4161]

@sobitneupane

Here is the permalink to the fallback code.

 if (onDownloadFailed) {
          onDownloadFailed();
 } else {
   // file could not be downloaded, open sourceURL in new tab
   Link.openExternalLink(url);
}

It is currently handled in two ways:

• If there is a custom function for handling failed downloads, that function will get called and as of now, it is used to do things, like open a model.
• If there is no custom function, then the content will be opened in a new tab.

Further analysis:

These custom onDownloadFailed functions are only used in cases where we are trying an exporting data to CSV file.

Regarding the question: If we already have fallback present, is it safe to remove following block of code?:

Yes, it should be good to remove the block of code, but if the current flow is good except the download for Concierge video, we can consider if this is a necessary action.

Thanks!

[melvin-bot[bot]]

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

[melvin-bot[bot]]

@sobitneupane, @VictoriaExpensify Huh... This is 4 days overdue. Who can take care of this?

[sobitneupane]

Thanks @code4161,

It looks like we are preemptively avoiding fetch calls for requests where we can anticipate a CORS issue. Removing the suggested block of code will still not achieve the desired outcome, as the fetch request will proceed, fail, and the catch will ultimately open the link in a new tab. This would result in unnecessary fetch attempts and potential errors in the console.

Let me know if you have further thoughts!

[melvin-bot[bot]]

@sobitneupane @VictoriaExpensify 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]]

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

[sobitneupane]

@VictoriaExpensify We can close this issue. The video is intentionally opened in new tab to avoid CORS issue.

[code4161]

@sobitneupane , I second your thought, there is no need to relay on the fallback if you expected CORS issue on all downloads. If you want allow this download feature for almost all files or videos, adding a CDN maybe a good option.

[melvin-bot[bot]]

@sobitneupane, @VictoriaExpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[sobitneupane]

https://github.com/Expensify/App/issues/53537#issuecomment-2556435027

[melvin-bot[bot]]

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

[melvin-bot[bot]]

@sobitneupane, @VictoriaExpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot[bot]]

@sobitneupane, @VictoriaExpensify Still overdue 6 days?! Let's take care of this!

[sobitneupane]

@VictoriaExpensify We can close the issue.