-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Video - Invalid file is downloaded when downloading video. #37163
Conversation
Signed-off-by: Krishna Gupta <belivethatkg@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apply suggestion and revert the rest
Signed-off-by: Krishna Gupta <belivethatkg@gmail.com>
Please revert the other changes |
This reverts commit b82fb45.
Signed-off-by: Krishna Gupta <belivethatkg@gmail.com>
@s77rt, do you have any clue about this error? Why does it happen when downloading from popover menu? When we download from message context menu the request is sent to bug_url.mp4 |
I'm not sure I can reproduce that bug. Can you please look into the native bug as well? On native it seems that
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
@s77rt, yes, the bug also exists on native devices as well as the desktop app. After some digging, I found that in the code below, we don't use App/src/components/VideoPlayerContexts/VideoPopoverMenuContext.js Lines 28 to 33 in d0d77c1
|
@Krishna2323 On web the download should start on the same tab, using |
@s77rt no it won't, we use App/src/libs/fileDownload/getAttachmentDetails.ts Lines 23 to 31 in d0d77c1
Same for video/image attachment modal download option: App/src/components/Attachments/AttachmentCarousel/extractAttachmentsFromReport.js Lines 21 to 25 in d0d77c1
|
@Krishna2323 Please test on all platforms, if it works push the changes |
Signed-off-by: Krishna Gupta <belivethatkg@gmail.com>
@s77rt, you were right it opens a new tab in web, first when I tried passing |
The source should be kept relative then. Can you check why |
@s77rt, the source we pass to |
I tested using the source directly (setting state in |
Can you try |
@s77rt, that won't work on native devices because in native devices we need full path instead of just the path. |
Why would that make a difference? On web we still need to use the path only |
@s77rt, because we already use From here we get for attachment download button and we also pass it to video player context menu: App/src/components/Attachments/AttachmentCarousel/extractAttachmentsFromReport.js Line 25 in 56bda00
And from
|
I think we can use Edit: No bug here. |
const downloadAttachment = useCallback(() => { | ||
currentVideoPlayerRef.current.getStatusAsync().then((status) => { | ||
const sourceURI = `/${Url.getPathFromURL(status.uri)}`; | ||
const sourceURI = tryResolveUrlFromApiRoot(status.uri); | ||
fileDownload(sourceURI); | ||
}); | ||
}, [currentVideoPlayerRef]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const downloadAttachment = useCallback(() => fileDownload(addEncryptedAuthTokenToURL(currentlyPlayingURL)), [currentlyPlayingURL]);
This should do it. Please test on all platforms
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the auth token only if the url does not start with blob:
or file:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const downloadAttachment = useCallback(() => {
const sourceURI = currentlyPlayingURL.startsWith('blob:') || currentlyPlayingURL.startsWith('file:') ? currentlyPlayingURL : addEncryptedAuthTokenToURL(currentlyPlayingURL);
fileDownload(sourceURI);
}, [currentlyPlayingURL]);
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid.movAndroid: mWeb Chromemweb-chrome.moviOS: NativeI can't download videos on iOS but I think it's related to my simulator or settings as I can't download photos either iOS: mWeb Safarimweb-safari.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
@Krishna2323 Once you complete the tests and adding the screenshots / videos, please tag me |
Signed-off-by: Krishna Gupta <belivethatkg@gmail.com>
@s77rt, I tested on all platforms, works well. Screenshots are also added. |
@Krishna2323 Please remove step 1 and step 5 from the testing steps, and change step 7 to:
|
Done. |
@Krishna2323 You removed the wrong step, please double check |
Also please make the requested change on the last step |
Sorry for that, I updated wrong step, Updated. |
Thank you! |
fix: Video - Invalid file is downloaded when downloading video. (cherry picked from commit 2509631)
…ng-37163-1 🍒 Cherry pick PR #37163 to staging 🍒
🚀 Deployed to staging by https://github.com/deetergp in version: 1.4.43-19 🚀
|
This is been CPed to staging and works 👍 Screen.Recording.2024-02-26.at.11.03.38.AM.mov |
🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.43-20 🚀
|
Details
Fixed Issues
$ #37092
PROPOSAL: #37092 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
android_native_1.mp4
Android: mWeb Chrome
android_chrome.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.mp4
MacOS: Desktop
desktop_app_2.mp4