feat: save youtube thumbnail info (WIP) #35263
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @rpenido! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Update the status of your PRYour PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:
When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
rpenido
force-pushed
the
rpenido/add-youtube-thumbnails
branch
from
a9246f2 to
6c948d6
Compare
| @@ -645,6 +645,9 @@ def editor_saved(self, user, old_metadata, old_content): # lint-amnesty, pylint | |||
| if val_youtube_id and self.youtube_id_1_0 != val_youtube_id: | |||
| self.youtube_id_1_0 = val_youtube_id | |||
|
|
|||
| if not self.edx_video_id and self.youtube_id_1_0: | |||
| self.thumbnail = f"https://img.youtube.com/vi/{self.youtube_id_1_0}/sddefault.jpg" | |||
There was a problem hiding this comment.
The only probably I can see with this is with the edge case in which:
- The user explicitly sets the thumbnail to a value in OLX; and...
- The user changes some video metadata, but not the actual YouTube video location.
In that case, we would not want to override the explicit thumbnail they set with the YouTube default.
Whatever you end up writing though, please add thorough comments explaining why this is okay to do, since this logic would not be obvious to someone who isn't familiar with some of the weirdness of VideoBlock.
There was a problem hiding this comment.
Then we can also add and not self.thumbnail to the check?
There was a problem hiding this comment.
Then we can also add
and not self.thumbnailto the check?
This will prevent the thumbnail from being updated if the YouTube URL changes. Also, we are not cleaning the thumbnail if we change the URL (i.e. from a YouTube video for a stream)
There was a problem hiding this comment.
Ah, right. Then I think it's better to never save this thumbnail value into the thumbnail field - besides that means that existing videos without thumbnails won't show up unless they are edited and saved. Instead, every time index_dictionary() is called, return self.thumbnail or (autodetect youtube URL if applicable) or None but don't save it into the self.thumbnail field.
There was a problem hiding this comment.
The advantage of saving it is that it can be used in other places, like the block editor (which works out of the box).
Another option would be to create a new thumbnail_youtube field to store this data and add a getter to the thumbnail property using the thumbnail field as a fallback.
def get_thumbnail(self):
return self.thumbnail or self.thumbnail_youtubeThere was a problem hiding this comment.
If we store it and the user change the YouTube URL, will we be able to automatically update the thumbnail_youtube field?
There was a problem hiding this comment.
Yes. We already have the logic to handle it with the youtube_id_1_0 field.
|
Closing this as this is part of a Discovery |
Description
This is a proof of concept PR to discuss the best way to store the YouTube thumbnails for the Video XBlock