[HOLD for payment 2024-06-28] [$250] mWeb - Video - Download option is not working #43117

lanitochka17 · 2024-06-05T12:25:43Z

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: 1.4.79-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: https://expensify.testrail.io/index.php?/tests/view/4594355
Email or phone of affected tester (no customers): applausetester+vd_mweb060424@applause.expensifail.com
Issue reported by: Applause - Internal Team

Action Performed:

Pre-requisite: the user must be logged in

Go to any chat
Send a video
Tap on the video preview
Tap on the three dots menu and select "Download"

Expected Result:

The video should start downloading on the device

Actual Result:

Nothing happens after tapping on "Download" option

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

Bug6502136_1717538746108.Vnsc8116_1_.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~013079356c488afb74
Upwork Job ID: 1798664874044810128
Last Price Increase: 2024-06-06
Automatic offers:

ShridharGoel | Contributor | 102736117
bernhardoj | Contributor | 102779959

Issue Owner

Current Issue Owner: @twisterdotcom

melvin-bot · 2024-06-05T12:25:54Z

Triggered auto assignment to @twisterdotcom (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 · 2024-06-05T12:26:12Z

@twisterdotcom 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 · 2024-06-05T12:26:26Z

We think that this bug might be related to #vip-vsp

bernhardoj · 2024-06-05T14:13:49Z

Proposal

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

Download video option doesn't work.

What is the root cause of that problem?

When we press the download option, it will take the video player ref source uri props and append an encryptedAuthToken to it.

App/src/components/VideoPlayerContexts/VideoPopoverMenuContext.tsx

Lines 32 to 42 in 66b4072

    
           const downloadAttachment = useCallback(() => { 
        
               if (videoPopoverMenuPlayerRef.current === null) { 
        
                   return; 
        
               } 
        
               const {source} = videoPopoverMenuPlayerRef.current?.props ?? {}; 
        
               if (typeof source === 'number' || !source) { 
        
                   return; 
        
               } 
        
               const sourceURI = addEncryptedAuthTokenToURL(source.uri); 
        
               fileDownload(sourceURI); 
        
           }, [videoPopoverMenuPlayerRef]);

However, the video player ref source uri already has encryptedAuthToken appended to it. (if not a local file)

App/src/components/VideoPlayer/BaseVideoPlayer.tsx

Line 73 in 66b4072

    
           const [sourceURL] = useState(VideoUtils.addSkipTimeTagToURL(url.includes('blob:') || url.includes('file:///') ? url : addEncryptedAuthTokenToURL(url), 0.001));

App/src/components/VideoPlayer/BaseVideoPlayer.tsx

Lines 347 to 351 in 66b4072

    
           source={{ 
        
               // if video is loading and is offline, we want to change uri to "" to 
        
               // reset the video player after connection is back 
        
               uri: !isLoading || (isLoading && !isOffline) ? sourceURL : '', 
        
           }}

So, encryptedAuthToken is appended twice resulting in error.

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

Remove addEncryptedAuthTokenToURL from here.

App/src/components/VideoPlayerContexts/VideoPopoverMenuContext.tsx

Lines 32 to 42 in 66b4072

    
           const downloadAttachment = useCallback(() => { 
        
               if (videoPopoverMenuPlayerRef.current === null) { 
        
                   return; 
        
               } 
        
               const {source} = videoPopoverMenuPlayerRef.current?.props ?? {}; 
        
               if (typeof source === 'number' || !source) { 
        
                   return; 
        
               } 
        
               const sourceURI = addEncryptedAuthTokenToURL(source.uri); 
        
               fileDownload(sourceURI); 
        
           }, [videoPopoverMenuPlayerRef]);

ShridharGoel · 2024-06-06T08:46:46Z

Proposal

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

mWeb - Video - Download option is not working.

What is the root cause of that problem?

In below code, we are adding encryptedAuthToken in the URL, but there's a possibility that it's already added:

App/src/components/VideoPlayerContexts/VideoPopoverMenuContext.tsx

Line 40 in 90dee7a

const sourceURI = addEncryptedAuthTokenToURL(source.uri);

This is already being added from here as well:

App/src/components/VideoPlayer/BaseVideoPlayer.tsx

Line 73 in 90dee7a

    
           const [sourceURL] = useState(VideoUtils.addSkipTimeTagToURL(url.includes('blob:') || url.includes('file:///') ? url : addEncryptedAuthTokenToURL(url), 0.001));

This leads to duplicate query params.

There are multiple other places as well where addEncryptedAuthTokenToURL is being called (all these are needed, but again there can be places where it gets added twice. Future logic changes can lead to same situation again.)

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

Line 471 in 90dee7a

const sourceURLWithAuth = addEncryptedAuthTokenToURL(sourceURL ?? '');

App/src/components/PDFThumbnail/index.tsx

Line 23 in 90dee7a

    
           file={isAuthTokenRequired ? addEncryptedAuthTokenToURL(previewSourceURL) : previewSourceURL}

App/src/components/PDFThumbnail/index.native.tsx

Line 23 in 90dee7a

    
           source={{uri: isAuthTokenRequired ? addEncryptedAuthTokenToURL(previewSourceURL) : previewSourceURL}}

App/src/components/Attachments/AttachmentView/index.tsx

Line 156 in 90dee7a

    
           const encryptedSourceUrl = isAuthTokenRequired ? addEncryptedAuthTokenToURL(source as string) : (source as string);

Why does this issue happen only on mWeb?

Because on normal web, duplicate params are being handled gracefully (extra one is not there in the request). But on mWeb, the duplicate ones don't get removed which leads to an exception.

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

We should update the logic of addEncryptedAuthTokenToURL to only add the token if it doesn't exist:

export default function (url: string) {
    if (url.includes('encryptedAuthToken')) {
        return url
    }
    return `${url}?encryptedAuthToken=${encodeURIComponent(encryptedAuthToken)}`;
}

We can also change the name to addEncryptedAuthTokenToURLIfNeeded.ts or something similar.

This will also make it future-proof, even if the auth token method gets called multiple times, it would only be added once.

melvin-bot · 2024-06-06T10:35:00Z

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

melvin-bot · 2024-06-06T10:35:05Z

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

jayeshmangwani · 2024-06-07T09:37:35Z

We can remove the addEncryptedAuthTokenToURL from downloadAttachment function, tested with few Download scenarios and it looks that we are already passing the auth token to every source files, @ShridharGoel do you have any scenario where you have found if we are not passing the addEncryptedAuthTokenToURL to the source ? If yes then I would agree with the adding the check of the encryptedAuthToken in the function.

But It looks that we already passing the EncryptedAuthToken already then @bernhardoj 's Proposal of removing the addEncryptedAuthTokenToURL from downloadAttachment function Looks good to me

🎀 👀 🎀 C+ reviewed

melvin-bot · 2024-06-07T09:37:47Z

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

melvin-bot · 2024-06-10T18:41:49Z

@twisterdotcom, @jayeshmangwani, @grgia Whoops! This issue is 2 days overdue. Let's get this updated quick!

jayeshmangwani · 2024-06-10T19:42:45Z

Not Overdue!
@grgia Whats your thought on the above Proposal ?

melvin-bot · 2024-06-14T14:56:38Z

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

bernhardoj · 2024-06-14T15:14:54Z

@grgia I think @jayeshmangwani meant to approve my proposal

melvin-bot · 2024-06-17T18:26:02Z

@twisterdotcom, @jayeshmangwani, @ShridharGoel, @grgia Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

grgia · 2024-06-18T11:19:10Z

@jayeshmangwani could you highlight the chosen proposal, I was under the impression you had said @ShridharGoel's proposal was fine. When you choose a proposal, could you include only the chosen proposal with the C+ reviewed comment?

jayeshmangwani · 2024-06-18T11:34:44Z

When you choose a proposal, could you include only the chosen proposal with the C+ reviewed comment?

I will keep this in mind for future reference. I should have split the comment. 🤦

grgia · 2024-06-18T11:37:09Z

@jayeshmangwani could you address #43117 (comment) ?

Or would you suggest we move forward with @bernhardoj's proposal.

Thanks for your understanding everyone

jayeshmangwani · 2024-06-18T11:44:35Z

When I was testing the downloadAttachment function with a few scenarios, we were passing the addEncryptedAuthTokenToURL to the source files every time. That's why I was considering going with the @bernhardoj 's proposal. I asked the same question to @ShridharGoel two weeks ago to see if they could find any scenario where we are not passing the encryptedAuthToken.

Or would you suggest we move forward with @bernhardoj's proposal.

@grgia If there is no need to add the addEncryptedAuthTokenToURL for any file, then we can safely remove it as per @bernhardoj's proposal.

grgia · 2024-06-18T11:59:47Z

@jayeshmangwani #3597
It seems like we should be using addEncryptedAuthTokenToURL for all downloaded attachments no?

jayeshmangwani · 2024-06-18T13:00:51Z

It seems like we should be using addEncryptedAuthTokenToURL for all downloaded attachments no?

Yes If isAuthTokenRequired is true then we need to append the encryptedAuthToken to the source url, but the thing here in this issue is we are appending the encryptedAuthToken twice, we already added EncryptedAuthTokenToURL to the sourceURL here:

App/src/components/VideoPlayer/BaseVideoPlayer.tsx

Line 73 in 8dea185

    
           const [sourceURL] = useState(VideoUtils.addSkipTimeTagToURL(url.includes('blob:') || url.includes('file:///') ? url : addEncryptedAuthTokenToURL(url), 0.001));

Then, we are passing the sourceURL to Video source.uri

App/src/components/VideoPlayer/BaseVideoPlayer.tsx

Lines 347 to 351 in 8dea185

    
           source={{ 
        
               // if video is loading and is offline, we want to change uri to "" to 
        
               // reset the video player after connection is back 
        
               uri: !isLoading || (isLoading && !isOffline) ? sourceURL : '', 
        
           }}

And here we are again appending the encryptedAuthToken to the source.uri

App/src/components/VideoPlayerContexts/VideoPopoverMenuContext.tsx

Line 40 in 8dea185

const sourceURI = addEncryptedAuthTokenToURL(source.uri);

grgia · 2024-06-18T14:06:37Z

Gotcha, that makes much more sense thank you @jayeshmangwani

melvin-bot · 2024-06-18T14:06:49Z

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

grgia · 2024-06-18T14:07:28Z

Going to go with @bernhardoj's proposal. @ShridharGoel let me know if you need help finding an issue to work on!

bernhardoj · 2024-06-19T04:31:39Z

PR is ready

cc: @jayeshmangwani

melvin-bot · 2024-06-21T21:17:21Z

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot · 2024-06-21T21:17:26Z

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

Fix can't download video #43974

If no regressions arise, payment will be issued on 2024-06-28. 🎊

For reference, here are some details about the assignees on this issue:

@jayeshmangwani requires payment through NewDot Manual Requests
@bernhardoj requires payment automatic offer (Contributor)

melvin-bot · 2024-06-21T21:17:27Z

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:

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

jayeshmangwani · 2024-06-27T13:51:53Z

[@jayeshmangwani] The PR that introduced the bug has been identified. Link to the PR: Incorrect video downloaded #40647 #41054
[@jayeshmangwani] 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: Incorrect video downloaded #40647 #41054 (comment)
[@jayeshmangwani] 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: Issue can be found when testing; no need to create a discussion for this.
[@jayeshmangwani] Determine if we should create a regression test for this bug. Yes
[@jayeshmangwani] 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 Test Proposal

Send a video to any chat
Open the video preview
Press the three-dot button
Press the download option
Verify the video is downloaded

Do we agree 👍 or 👎

twisterdotcom · 2024-06-28T09:54:36Z

Payment Summary:

@jayeshmangwani paid $250 through NewDot Manual Requests (C+)
@bernhardoj paid $250 here (Contributor)

jayeshmangwani · 2024-07-02T05:36:20Z

Requested $250 on NewDot.

JmillsExpensify · 2024-07-02T13:18:16Z

$250 approved for @jayeshmangwani

lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jun 5, 2024

melvin-bot bot assigned twisterdotcom Jun 5, 2024

twisterdotcom added the External Added to denote the issue can be worked on by a contributor label Jun 6, 2024

melvin-bot bot changed the title ~~mWeb - Video - Download option is not working~~ [$250] mWeb - Video - Download option is not working Jun 6, 2024

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 6, 2024

melvin-bot bot assigned jayeshmangwani Jun 6, 2024

melvin-bot bot assigned grgia Jun 7, 2024

melvin-bot bot added the Overdue label Jun 10, 2024

melvin-bot bot added Overdue and removed Overdue labels Jun 10, 2024

grgia assigned ShridharGoel Jun 14, 2024

melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 14, 2024

melvin-bot bot removed the Overdue label Jun 14, 2024

melvin-bot bot added the Overdue label Jun 17, 2024

melvin-bot bot removed the Overdue label Jun 18, 2024

grgia assigned bernhardoj and unassigned ShridharGoel Jun 18, 2024

bernhardoj mentioned this issue Jun 19, 2024

Fix can't download video #43974

Merged

50 tasks

melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Jun 19, 2024

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jun 21, 2024

melvin-bot bot changed the title ~~[$250] mWeb - Video - Download option is not working~~ [HOLD for payment 2024-06-28] [$250] mWeb - Video - Download option is not working Jun 21, 2024

melvin-bot bot removed the Reviewing Has a PR in review label Jun 21, 2024

jayeshmangwani mentioned this issue Jun 27, 2024

Incorrect video downloaded #40647 #41054

Merged

50 tasks

twisterdotcom closed this as completed Jun 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HOLD for payment 2024-06-28] [$250] mWeb - Video - Download option is not working #43117

[HOLD for payment 2024-06-28] [$250] mWeb - Video - Download option is not working #43117

lanitochka17 commented Jun 5, 2024 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Jun 5, 2024

lanitochka17 commented Jun 5, 2024

lanitochka17 commented Jun 5, 2024

bernhardoj commented Jun 5, 2024

ShridharGoel commented Jun 6, 2024 •

edited

Loading

melvin-bot bot commented Jun 6, 2024

melvin-bot bot commented Jun 6, 2024

jayeshmangwani commented Jun 7, 2024

melvin-bot bot commented Jun 7, 2024

melvin-bot bot commented Jun 10, 2024

jayeshmangwani commented Jun 10, 2024

melvin-bot bot commented Jun 14, 2024

bernhardoj commented Jun 14, 2024

melvin-bot bot commented Jun 17, 2024

grgia commented Jun 18, 2024 •

edited

Loading

jayeshmangwani commented Jun 18, 2024

grgia commented Jun 18, 2024

jayeshmangwani commented Jun 18, 2024

grgia commented Jun 18, 2024

jayeshmangwani commented Jun 18, 2024

grgia commented Jun 18, 2024

melvin-bot bot commented Jun 18, 2024

grgia commented Jun 18, 2024

bernhardoj commented Jun 19, 2024

melvin-bot bot commented Jun 21, 2024

melvin-bot bot commented Jun 21, 2024

melvin-bot bot commented Jun 21, 2024

jayeshmangwani commented Jun 27, 2024

twisterdotcom commented Jun 28, 2024

jayeshmangwani commented Jul 2, 2024

JmillsExpensify commented Jul 2, 2024

[HOLD for payment 2024-06-28] [$250] mWeb - Video - Download option is not working #43117

[HOLD for payment 2024-06-28] [$250] mWeb - Video - Download option is not working #43117

Comments

lanitochka17 commented Jun 5, 2024 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Jun 5, 2024

lanitochka17 commented Jun 5, 2024

lanitochka17 commented Jun 5, 2024

bernhardoj commented Jun 5, 2024

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?

ShridharGoel commented Jun 6, 2024 • edited Loading

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?

melvin-bot bot commented Jun 6, 2024

melvin-bot bot commented Jun 6, 2024

jayeshmangwani commented Jun 7, 2024

melvin-bot bot commented Jun 7, 2024

melvin-bot bot commented Jun 10, 2024

jayeshmangwani commented Jun 10, 2024

melvin-bot bot commented Jun 14, 2024

bernhardoj commented Jun 14, 2024

melvin-bot bot commented Jun 17, 2024

grgia commented Jun 18, 2024 • edited Loading

jayeshmangwani commented Jun 18, 2024

grgia commented Jun 18, 2024

jayeshmangwani commented Jun 18, 2024

grgia commented Jun 18, 2024

jayeshmangwani commented Jun 18, 2024

grgia commented Jun 18, 2024

melvin-bot bot commented Jun 18, 2024

grgia commented Jun 18, 2024

bernhardoj commented Jun 19, 2024

melvin-bot bot commented Jun 21, 2024

melvin-bot bot commented Jun 21, 2024

melvin-bot bot commented Jun 21, 2024

jayeshmangwani commented Jun 27, 2024

Regression Test Proposal

twisterdotcom commented Jun 28, 2024

jayeshmangwani commented Jul 2, 2024

JmillsExpensify commented Jul 2, 2024

lanitochka17 commented Jun 5, 2024 •

edited by melvin-bot bot

Loading

ShridharGoel commented Jun 6, 2024 •

edited

Loading

grgia commented Jun 18, 2024 •

edited

Loading