feat: Video editor supports transcripts [FC-0076] #36058
Conversation
* Add error handler on save video to avoid create sjson * Support transcripts without edx_video_id in definition_to_xml
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @ChrisChV! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are 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. 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. |
ChrisChV
changed the title
This is used to be retroactive in copy-paste videos from Library to Course and Course to Library
There was a problem hiding this comment.
Hi @ChrisChV , this is working well for the most part, good job dealing with the old transcript code!
But I found a bug with the upstream/downstream syncing, and left a few nits/change requests too.
| @@ -81,13 +84,17 @@ def link_video_to_component(video_component, user): | |||
| edx_video_id = clean_video_id(video_component.edx_video_id) | |||
| if not edx_video_id: | |||
| edx_video_id = create_external_video(display_name='external video') | |||
|
|
|||
| if isinstance(video_component.usage_key, UsageKeyV2): | |||
| return edx_video_id | |||
There was a problem hiding this comment.
I don't understand why we're returning early here.. Could you add a comment to clarify?
There was a problem hiding this comment.
Makes sense. But I wonder, should we still be calling create_external_video and returning an edx_video_id at all, if it's not going to be saved into the video block? Doesn't that create some stranded video data in VAL?
There was a problem hiding this comment.
| @@ -10,6 +10,7 @@ | |||
| import re | |||
There was a problem hiding this comment.
I'm seeing a bug when I sync a LibraryBlock video with transcripts from an upstream video.
Steps to reproduce:
- Create a library video with transcripts (here, I imported them from the example youtube video).
- Publish the library video.
- Copy it to the clipboard.
- Paste into a course.
Note that the transcripts are displaying fine here. - Re-edit the library video, and replace a transcript. (Here, I replaced the English one, I don't know if replacing others causes the same issue).
- Return to the course LibraryBlock, and refresh to see the "updates available" button. Click it.
Note that the upstream video preview shows its transcripts fine, but the downstream (course) video preview doesn't show its transcripts anymore. - Accept changes.
Note that the course video no longer shows its transcripts, but if you edit it, you can see they're still there.
Syncing.upstream.video.breaks.transcripts.mp4
There was a problem hiding this comment.
I think this is related to openedx/modular-learning#246
There was a problem hiding this comment.
@ChrisChV That could very well be.. however I don't think it's resolved by @DanielVZ96 's #36173, but it's also possible that I didn't merge conflicts accurately. cf my merged branch.
There was a problem hiding this comment.
@pomegranited To be safe, I will wait until #36173 is ready to fix this bug.
There was a problem hiding this comment.
No worries @ChrisChV , thank you for keeping an eye on this issue.
There was a problem hiding this comment.
@bradenmacdonald @pomegranited I fixed this on 0745f09. I need to add tests, but you can test the fix
There was a problem hiding this comment.
Hi @ChrisChV -- your fix is an improvement, but now I'm seeing a slightly different bug in the "updates available" preview.
Steps to reproduce:
- Create a library video with one or more transcripts (here, I used your mp4 video link, but it happens with the imported transcripts from the example youtube video too).
- Publish the library video.
- Copy it to the clipboard.
- Paste into a course.
Note that the transcripts are displaying fine here (despite the message about needing to upload transcript files??) - Re-edit the library video, and replace a transcript. Wait for it to process fully before hitting Save.
- Publish the library video again.
- Return to the course LibraryBlock, and refresh to see the "updates available" button. Click it.
Bug: the downstream (course) video preview shows the updated English transcript from upstream, not the actual transcript that's on the downstream block. - Accept changes.
Sync works fine (sorry, screencapture below doesn't show this step, but it does work :) )
Transcripts.sync.bug.mp4
There was a problem hiding this comment.
Thanks @pomegranited! Do you think we can move that issue to another ticket? I've already spent a lot of budget on this task, and it's getting much bigger every time 😃
There was a problem hiding this comment.
@ChrisChV Yes I think so -- the syncing works, it's just the preview that's oddly borked. And AFAIK, syncing was out of scope here anyway :)
There was a problem hiding this comment.
Thanks! I created the issue: openedx/frontend-app-authoring#1682
…void raise AttributeError
There was a problem hiding this comment.
👍 Thank you for making those changes @ChrisChV ! Code looks and works great.
- I tested this using the testing instructions from feat: Enable transcripts for video library [FC-0076] frontend-app-authoring#1596.
I also tested "duplicating" video blocks with transcripts in courses, and they worked too. - I read through the code
- I checked for accessibility issues by using my keyboard to navigate
- Includes documentation -- good code comments
-
User-facing strings are extracted for translationN/A
| @@ -10,6 +10,7 @@ | |||
| import re | |||
There was a problem hiding this comment.
@ChrisChV That could very well be.. however I don't think it's resolved by @DanielVZ96 's #36173, but it's also possible that I didn't merge conflicts accurately. cf my merged branch.
| @@ -81,13 +84,17 @@ def link_video_to_component(video_component, user): | |||
| edx_video_id = clean_video_id(video_component.edx_video_id) | |||
| if not edx_video_id: | |||
| edx_video_id = create_external_video(display_name='external video') | |||
|
|
|||
| if isinstance(video_component.usage_key, UsageKeyV2): | |||
| return edx_video_id | |||
There was a problem hiding this comment.
Makes sense. But I wonder, should we still be calling create_external_video and returning an edx_video_id at all, if it's not going to be saved into the video block? Doesn't that create some stranded video data in VAL?
There was a problem hiding this comment.
@ChrisChV Nice work on a very complex and ugly part of the code 👏🏻. I have a few small changes to request but I think this is just about good to go.
| output_format=Transcript.SRT | ||
| ).encode() | ||
|
|
||
| filename = f"static/{edx_video_id}-{language_code}.srt" |
There was a problem hiding this comment.
Do we need to put the edx_video_id in the filename? Because transcript-{language_code}.srt would be a much nicer name.
save_video_transcript_in_learning_core created in transcript_ajax to avoid create an edx_video_id for library videos
* _save_transcript_field created to save the `transcripts` Field * _studio_transcript_upload updated to avoid create edx_video_id for libraries * _studio_transcript_upload updated to avoid add transcripts to VAL for libraries
There was a problem hiding this comment.
Code looks good to me - thanks for those updates!
@pomegranited can you please test this carefully again before merging? Or let me know if you want help.
There was a problem hiding this comment.
@ChrisChV This is working great, except for one weird issue still in the upstream/downstream preview. So I'm approving, in case this can be fixed in a follow-up PR?
- I tested this using the test instructions from feat: Enable transcripts for video library [FC-0076] frontend-app-authoring#1596, and also by syncing transcript changes from library blocks to downstream course blocks.
- I read through the code
- I checked for accessibility issues by using my keyboard to navigate
- Includes documentation
- User-facing strings are extracted for translation
| @@ -10,6 +10,7 @@ | |||
| import re | |||
There was a problem hiding this comment.
Hi @ChrisChV -- your fix is an improvement, but now I'm seeing a slightly different bug in the "updates available" preview.
Steps to reproduce:
- Create a library video with one or more transcripts (here, I used your mp4 video link, but it happens with the imported transcripts from the example youtube video too).
- Publish the library video.
- Copy it to the clipboard.
- Paste into a course.
Note that the transcripts are displaying fine here (despite the message about needing to upload transcript files??) - Re-edit the library video, and replace a transcript. Wait for it to process fully before hitting Save.
- Publish the library video again.
- Return to the course LibraryBlock, and refresh to see the "updates available" button. Click it.
Bug: the downstream (course) video preview shows the updated English transcript from upstream, not the actual transcript that's on the downstream block. - Accept changes.
Sync works fine (sorry, screencapture below doesn't show this step, but it does work :) )
Transcripts.sync.bug.mp4
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
Description
edx_video_idindefinition_to_xmledx_video_idSupporting information
Testing instructions
Follow the testing instructions at: openedx/frontend-app-authoring#1596
Deadline
No rush
Other information
library componentin a course. This will be fixed in Copy static assets when using a library component in a course (via Problem Bank or Library Content) modular-learning#246