-
Notifications
You must be signed in to change notification settings - Fork 3.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
feat: Video editor supports transcripts [FC-0076] #36058
base: master
Are you sure you want to change the base?
Changes from 24 commits
9cdf45f
3885dd1
a723e96
1c2a575
74f027f
2adacee
0393829
e27221e
05067f6
2f53a75
5c6819a
95b51e8
c42e223
4bf3af3
1de1d4e
a59898d
9173ed8
590a5cd
0e54450
5685f16
e4f7c72
b478ac5
173a0af
4532a2a
bb10d18
0203dac
a254fe4
34e5cbc
c73f82c
ddb6953
46aa8ca
5bb426f
5dda777
53f42fa
9c7e4e8
8c6a3dd
0745f09
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ | |
import re | ||
|
||
from attrs import frozen, Factory | ||
from django.core.files.base import ContentFile | ||
from django.conf import settings | ||
from django.contrib.auth import get_user_model | ||
from django.utils.translation import gettext as _ | ||
|
@@ -23,6 +24,8 @@ | |
from xmodule.exceptions import NotFoundError | ||
from xmodule.modulestore.django import modulestore | ||
from xmodule.xml_block import XmlMixin | ||
from xmodule.video_block.transcripts_utils import Transcript, build_components_import_path | ||
from edxval.api import create_external_video, create_or_update_video_transcript | ||
|
||
from cms.djangoapps.models.settings.course_grading import CourseGradingModel | ||
from cms.lib.xblock.upstream_sync import UpstreamLink, UpstreamLinkException, fetch_customizable_fields | ||
|
@@ -299,13 +302,21 @@ def import_staged_content_from_user_clipboard(parent_key: UsageKey, request) -> | |
tags=user_clipboard.content.tags, | ||
) | ||
|
||
usage_key = new_xblock.scope_ids.usage_id | ||
ChrisChV marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if usage_key.block_type == 'video': | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since that PR merged first, does this one need to be updated? |
||
# The edx_video_id must always be new so as not | ||
# to interfere with the data of the copied block | ||
new_xblock.edx_video_id = create_external_video(display_name='external video') | ||
store.update_item(new_xblock, request.user.id) | ||
|
||
# Now handle static files that need to go into Files & Uploads. | ||
static_files = content_staging_api.get_staged_content_static_files(user_clipboard.content.id) | ||
notices, substitutions = _import_files_into_course( | ||
block=new_xblock, | ||
course_key=parent_key.context_key, | ||
staged_content_id=user_clipboard.content.id, | ||
static_files=static_files, | ||
usage_key=new_xblock.scope_ids.usage_id, | ||
usage_key=usage_key, | ||
) | ||
|
||
# Rewrite the OLX's static asset references to point to the new | ||
|
@@ -504,6 +515,7 @@ def _import_xml_node_to_parent( | |
|
||
|
||
def _import_files_into_course( | ||
block: XBlock, | ||
course_key: CourseKey, | ||
staged_content_id: int, | ||
static_files: list[content_staging_api.StagedContentFileData], | ||
|
@@ -540,6 +552,7 @@ def _import_files_into_course( | |
# At this point, we know this is a "Files & Uploads" asset that we may need to copy into the course: | ||
try: | ||
result, substitution_for_file = _import_file_into_course( | ||
block, | ||
course_key, | ||
staged_content_id, | ||
file_data_obj, | ||
|
@@ -566,6 +579,7 @@ def _import_files_into_course( | |
|
||
|
||
def _import_file_into_course( | ||
block: XBlock, | ||
course_key: CourseKey, | ||
staged_content_id: int, | ||
file_data_obj: content_staging_api.StagedContentFileData, | ||
|
@@ -583,8 +597,8 @@ def _import_file_into_course( | |
# we're not going to attempt to change. | ||
if clipboard_file_path.startswith('static/'): | ||
# If it's in this form, it came from a library and assumes component-local assets | ||
file_path = clipboard_file_path.lstrip('static/') | ||
import_path = f"components/{usage_key.block_type}/{usage_key.block_id}/{file_path}" | ||
file_path = clipboard_file_path.removeprefix('static/') | ||
import_path = build_components_import_path(usage_key, file_path) | ||
filename = pathlib.Path(file_path).name | ||
new_key = course_key.make_asset_key("asset", import_path.replace("/", "_")) | ||
else: | ||
|
@@ -616,6 +630,24 @@ def _import_file_into_course( | |
if thumbnail_content is not None: | ||
content.thumbnail_location = thumbnail_location | ||
contentstore().save(content) | ||
if usage_key.block_type == 'video': | ||
# Adding transcripts to VAL using the new edx_video_id | ||
language_code = next((k for k, v in block.transcripts.items() if v == filename), None) | ||
if language_code: | ||
sjson_subs = Transcript.convert( | ||
content=data, | ||
input_format=Transcript.SRT, | ||
output_format=Transcript.SJSON | ||
).encode() | ||
create_or_update_video_transcript( | ||
video_id=block.edx_video_id, | ||
language_code=language_code, | ||
metadata={ | ||
'file_format': Transcript.SJSON, | ||
'language_code': language_code | ||
}, | ||
file_data=ContentFile(sjson_subs), | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This new code does not seem to match the docstring of the function: "Import a single staged static asset file into the course, unless it already exists." It also doesn't use I think the code is fine but it should be moved out of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated 34e5cbc |
||
return True, {clipboard_file_path: f"static/{import_path}"} | ||
elif current_file.content_digest == file_data_obj.md5_hash: | ||
# The file already exists and matches exactly, so no action is needed | ||
|
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.
I'm seeing a bug when I sync a LibraryBlock video with transcripts from an upstream video.
Steps to reproduce:
Note that the transcripts are displaying fine here.
Note that the upstream video preview shows its transcripts fine, but the downstream (course) video preview doesn't show its transcripts anymore.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is related to openedx/modular-learning#246
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pomegranited To be safe, I will wait until #36173 is ready to fix this bug.
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.
No worries @ChrisChV , thank you for keeping an eye on this issue.
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.
Now that that other PR is merged, any update on this?
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.
@bradenmacdonald @pomegranited I fixed this on 0745f09. I need to add tests, but you can test the fix