Skip to content
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 Unavailable problem at YouTube preview #26980

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Sn-Kinos
Copy link

@Sn-Kinos Sn-Kinos commented May 9, 2024

Summary

Fixed the problem that YouTube preview shows Video Unavailable instead of the video.

  • Added the properties on the iframe for YouTube preview at youtube_video.tsx

Reproduce

Create a post that occurs Video Unavailable problem. I used https://youtu.be/edsx_MOhVnk on the below screenshots.

Ticket Link

No Tickets. (less than 20 lines of code change)

Screenshots

I haven't change the UI. But I can show about it w/ screenshots.

before after
image image

Release Note

Fixed the problem that YouTube preview shows Video Unavailable instead of the video.

@mm-cloud-bot mm-cloud-bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 9, 2024
@mattermost-build
Copy link
Contributor

Hello @Sn-Kinos,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@pvev
Copy link
Contributor

pvev commented May 9, 2024

hey @Sn-Kinos , thank you for your contribution. Could you please describe the exact steps on how to reproduce the issue you are trying to solve so we can create a ticket and associate it with this fix? Thanks!

@Sn-Kinos
Copy link
Author

hey @Sn-Kinos , thank you for your contribution. Could you please describe the exact steps on how to reproduce the issue you are trying to solve so we can create a ticket and associate it with this fix? Thanks!

Thanks @pvev! I added the reproduce section on the text :D

@Sn-Kinos Sn-Kinos marked this pull request as draft May 14, 2024 04:05
@Sn-Kinos Sn-Kinos marked this pull request as ready for review May 14, 2024 04:05
@Sn-Kinos Sn-Kinos marked this pull request as draft May 22, 2024 04:59
@Sn-Kinos Sn-Kinos marked this pull request as ready for review May 22, 2024 04:59
@Sn-Kinos
Copy link
Author

@pvev I wonder if there's anything else I need to do 🏃‍♂️

@pvev pvev requested review from pvev and hmhealey May 27, 2024 07:27
@pvev pvev added the 2: Dev Review Requires review by a developer label May 27, 2024
@pvev
Copy link
Contributor

pvev commented May 27, 2024

@pvev I wonder if there's anything else I need to do 🏃‍♂️

Thanks @Sn-Kinos we will start the dev review and security review and QE process now. We will keep you posted.

@pvev pvev requested review from larkox and removed request for hmhealey May 27, 2024 07:30
Copy link
Contributor

@larkox larkox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codewise, LGTM. Waiting for security review.

@larkox larkox added 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review 3: Security Review Review requested from Security Team labels May 27, 2024
@unified-ci-app
Copy link
Contributor

E2E test triggered successfully for PR #26980. The corresponding commit's status check will be available shortly.

Copy link

E2E test run is starting for commit 88e5376dec80ff16ddcdb4cee8650aa44e87bb6a.
You can check its progress by either:

Copy link
Member

@hmhealey hmhealey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, @Sn-Kinos! I always assumed that the fact that YouTube videos didn't play for me in MM was something on my end, not that we were missing some things on the iframe. Great find!

@@ -75,6 +76,11 @@ export default class YoutubeVideo extends React.PureComponent<Props, State> {
height='360px'
frameBorder='0'
allowFullScreen={true}
allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture; web-share"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to allow all these things? picture-in-picture and encrypted-media are the only ones that seem like they would be applicable to a video in MM. autoplay is technically applicable, but I don't think we want autoplaying video in MM

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's necessary and the feature has been on src as autoplay=1.

Without autoplay With autoplay
Screen.Recording.2024-05-29.at.11.02.51.mov
Screen.Recording.2024-05-29.at.11.04.53.mov

Copy link
Member

@hmhealey hmhealey May 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right. I forgot we went from showing a static image to the actual video. That makes sense.

I guess web-share/clipboard-write are needed for a share button and accelerometer/gyroscope are needed for rotating the video then (Edit: and 360 degree videos or whatever those are called)

@hmhealey hmhealey dismissed their stale review May 29, 2024 13:44

No changes needed about what I commented on

@pvev
Copy link
Contributor

pvev commented Jun 1, 2024

@Sn-Kinos sorry that this review is taking longer that normal, but there are some security concerns about this change that are being discussed internally. As soon as we define an action, we will let you know. Thanks.

@Sn-Kinos
Copy link
Author

Sn-Kinos commented Jun 3, 2024

@Sn-Kinos sorry that this review is taking longer that normal, but there are some security concerns about this change that are being discussed internally. As soon as we define an action, we will let you know. Thanks.

I understand. I referred this iframe code from another commercial messenger app. I hope it can help you.

@pvev
Copy link
Contributor

pvev commented Jun 7, 2024

@Sn-Kinos, thank you for your patience. The decision to use the referrer policy strict-origin-when-cross-origin can expose some sensitive information, which may be considered more or less critical depending on the use case. Therefore, this decision should be made by the workspace administrator.

Given this, the idea is to make this value configurable through a parameter in the admin console. I wanted to ask if you would like to add this new configuration parameter and set the referrer policy based on that parameter (I would help and guide you through this process). Alternatively, I can merge your PR as it is and then take care of adding this configuration parameter in a subsequent PR.

@Sn-Kinos
Copy link
Author

Sn-Kinos commented Jun 7, 2024

@pvev I prefer a subsequents PR. I thought there would be no problem because I brought the parameters used in Slack as it is, but I guess it wasn't?

Copy link
Contributor

@pvev pvev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution @Sn-Kinos 🚀

@pvev
Copy link
Contributor

pvev commented Jun 9, 2024

/update-branch

@pvev pvev removed the 2: Dev Review Requires review by a developer label Jun 9, 2024
@Sn-Kinos
Copy link
Author

Sn-Kinos commented Jun 9, 2024

@pvev I fixed lint and type error on CI. How can I re-run actions?

@Sn-Kinos
Copy link
Author

@pvev I fixed code to pass all tests. Is there anything to do for me to merge this?

@yasserfaraazkhan yasserfaraazkhan added the Setup Cloud Test Server Setup an on-prem test server label Jun 11, 2024
@mm-cloud-bot
Copy link

Creating a new SpinWick test server using Mattermost Cloud.

@mm-cloud-bot
Copy link

Mattermost test server created! 🎉

Access here: https://mattermost-pr-26980.test.mattermost.cloud

Account Type Username Password
Admin sysadmin Sys@dmin123
User user-1 User-1@123

Your Spinwick's installation ID is: 1s8uod98hpfejyix9udpihqxqh
To access the logs, please click here

Copy link
Contributor

@yasserfaraazkhan yasserfaraazkhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

before
Screenshot 2024-06-11 234834

PR changes are working as expected.

Screenshot 2024-06-11 234851

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review 3: Security Review Review requested from Security Team Contributor release-note Denotes a PR that will be considered when it comes time to generate release notes. Setup Cloud Test Server Setup an on-prem test server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants