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

Do not allow YouTube video URLs when using the URLPicker #5503

Merged
merged 1 commit into from
Jun 22, 2023

Conversation

acelaya
Copy link
Contributor

@acelaya acelaya commented Jun 21, 2023

Since we are currently using raw YouTube video URLs for video assignments, there's a possibility that someone configures it using the regular URLPicker instead of the YouTubePicker.

This is problematic, as the latter performs some extra validation that is not performed by the URLPicker (is a public existing video, it's not age-restricted, it's embeddable, etc).

Bypassing these validations could result in an assignment which fails for students at a later point in time.

This PRs adds a simple check that displays an error if a YouTube video URL is set in the URLPicker. The message is different if youtube is not allowed for the active course.

YouTube enabled:

image

YouTube not enabled:

image

Considerations / TODO

  • There are other possible approaches to address this problem. They were listed in Handle assignments created with a YouTube URL using the regular URLPicker #5499, but this seems like the simplest way to "get away with it" for now.
  • We need to define an error which is meaningful enough. I considered mentioning what to do next, e.g. "Go back and select YouTube instead", but considering YouTube is enabled/disabled by course, this might be the wrong assumption.
  • Related to the above, we might consider letting YouTube video URLs through, when YouTube is not enabled for the course, or dynamically display different error messages.

This PR closes #5499

@robertknight
Copy link
Member

We need to define an error which is meaningful enough. I considered mentioning what to do next, e.g. "Go back and select YouTube instead", but considering YouTube is enabled/disabled by course, this might be the wrong assumption.

I believe the plan is to enable YouTube for everyone once the feature is released, so this will no longer be a problem. What we could do in the interim is to show different messages depending on whether the flag is enabled or not.

If enabled:

"To annotate a video, go back and choose the YouTube option."

If disabled:

"Annotating YouTube videos is not yet supported. This feature is coming soon."

@acelaya
Copy link
Contributor Author

acelaya commented Jun 21, 2023

I believe the plan is to enable YouTube for everyone once the feature is released, so this will no longer be a problem. What we could do in the interim is to show different messages depending on whether the flag is enabled or not.

If enabled:

"To annotate a video, go back and choose the YouTube option."

If disabled:

"Annotating YouTube videos is not yet supported. This feature is coming soon."

I like it!

@acelaya acelaya marked this pull request as ready for review June 21, 2023 12:37
Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

LGTM

@acelaya acelaya merged commit 4726eb3 into main Jun 22, 2023
8 checks passed
@acelaya acelaya deleted the url-picker-yt-restriction branch June 22, 2023 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle assignments created with a YouTube URL using the regular URLPicker
2 participants