feat: Video editor supports transcripts [FC-0076] by ChrisChV · Pull Request #36058 · openedx/openedx-platform

ChrisChV · 2024-12-25T21:16:28Z

Description

Add error handler on save video to avoid creating sjson
Support transcripts without edx_video_id in definition_to_xml
When copying a video from a library to a course: Create a new edx_video_id
Save transcripts as static assets in a video in a library when adding a new transcript.
Delete transcripts as static assets in a video in a library when deleting transcripts.
Support download transcript in a video in a library.
Support replace transcript in a video in a library.
Support updating transcripts in video in a library.
Refactor the code of downloading YouTube transcripts to enable this feature in libraries.
Support copy from a library to a course and a course to a library.
Which edX user roles will this change impact? "Course Author"

Supporting information

Github link: Video editor supports transcripts (minimal version) frontend-app-authoring#1352
Used in: feat: Enable transcripts for video library [FC-0076] frontend-app-authoring#1596
Internal ticket: FAL-3989

Testing instructions

Follow the testing instructions at: openedx/frontend-app-authoring#1596

Deadline

No rush

Other information

TODO: The transcripts are not copied when you add a library component in 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

* Add error handler on save video to avoid create sjson * Support transcripts without edx_video_id in definition_to_xml

openedx-webhooks · 2024-12-25T21:16:33Z

Thanks for the pull request, @ChrisChV!

This repository is currently maintained by @openedx/wg-maintenance-edx-platform.

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 approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To 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:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

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:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

…ary components

…y videos

This is used to be retroactive in copy-paste videos from Library to Course and Course to Library

pomegranited

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.

pomegranited · 2025-01-29T07:41:47Z

        edx_video_id = create_external_video(display_name='external video')
+
+        if isinstance(video_component.usage_key, UsageKeyV2):
+            return edx_video_id


I don't understand why we're returning early here.. Could you add a comment to clarify?

Updated e4f7c72

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?

Updated https://github.com/openedx/edx-platform/pull/36058/files/34e5cbc493bf116a18d3e22ed139b3866e230161..46aa8ca7afe16896f37858c9290ea117adae8cdc

pomegranited · 2025-01-29T09:36:04Z

@@ -10,6 +10,7 @@
 import re


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

I think this is related to openedx/modular-learning#246

@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.

@pomegranited To be safe, I will wait until #36173 is ready to fix this bug.

No worries @ChrisChV , thank you for keeping an eye on this issue.

@bradenmacdonald @pomegranited I fixed this on 0745f09. I need to add tests, but you can test the fix

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

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 😃

@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 :)

Thanks! I created the issue: openedx/frontend-app-authoring#1682

…void raise AttributeError

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

…libraries

…n of load_block

bradenmacdonald

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.

pomegranited

@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 openedx/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

pomegranited · 2025-02-20T02:26:46Z

@@ -10,6 +10,7 @@
 import re


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

edx-pipeline-bot · 2025-02-21T21:03:03Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

* Add error handler on save video to avoid creating sjson * Support transcripts without edx_video_id in definition_to_xml * When copying a video from a library to a course: Create a new edx_video_id * Save transcripts as static assets in a video in a library when adding a new transcript. * Delete transcripts as static assets in a video in a library when deleting transcripts. * Support download transcript in a video in a library. * Support replace transcript in a video in a library. * Support updating transcripts in video in a library. * Refactor the code of downloading YouTube transcripts to enable this feature in libraries. * Support copy from a library to a course and a course to a library.

kdmccormick · 2025-02-24T16:53:20Z

+    isVideoBlock = downstream.usage_key.block_type == "video"
    for field_name in syncable_fields - customizable_fields:
+        if isVideoBlock and field_name == 'edx_video_id':
+            # Avoid overwriting edx_video_id between blocks
+            continue


@ChrisChV @pomegranited With all due appreciation for the VideoBlock nonsense you've had to grapple with to support transcripts, it truly pains me to see such block-type-specific code in this highly generalized function 😓 Upstream syncing is complex but nature, but adding special cases like this move it from being "complex" to being "complicated".

Is there any other way we can ensure that edx_video_id isn't synced? Would it not work to put it in the dict returned by get_customizable_fields, as @pomegranited did for graceperiod, grading_method, etc.?

+1, good point

Though the two root problems here are (1) that there is no standard way to associate a block with external data that's not in an XBlock field (e.g. edx_video_id points to VAL or ORA2 has tons of Django models holding its data) and (2) that there is no proper XBlock API for dealing with static files (e.g. transcripts)

edx-pipeline-bot · 2025-02-24T17:11:06Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2025-02-24T17:11:08Z

2U Release Notice: This PR has been deployed to the edX production environment.

* Add error handler on save video to avoid creating sjson * Support transcripts without edx_video_id in definition_to_xml * When copying a video from a library to a course: Create a new edx_video_id * Save transcripts as static assets in a video in a library when adding a new transcript. * Delete transcripts as static assets in a video in a library when deleting transcripts. * Support download transcript in a video in a library. * Support replace transcript in a video in a library. * Support updating transcripts in video in a library. * Refactor the code of downloading YouTube transcripts to enable this feature in libraries. * Support copy from a library to a course and a course to a library.

feat: Updates to support save video with transcript in library home

9cdf45f

* 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 PR author is not from Axim or 2U label Dec 25, 2024

ChrisChV marked this pull request as draft December 25, 2024 21:16

ChrisChV changed the title ~~feat: Video editor supports transcripts~~ feat: Video editor supports transcripts [FC-0076] Dec 25, 2024

style: Fix lint

3885dd1

mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Dec 27, 2024

ChrisChV added 15 commits January 15, 2025 14:50

feat: Updated code to get english transcript from filename

a723e96

style: Fix lint

1c2a575

feat: Upload transcript file as static asset in Learning Core in libr…

74f027f

…ary components

style: Fix lint

2adacee

feat: Updates to support download transcripts from youtube in vlibrar…

0393829

…y videos

style: Fix lint

e27221e

test: Add test for download youtube transcripts in library content

05067f6

style: Fix lint

2f53a75

feat: Support copy transcripts from a library

5c6819a

refactor: Allow use edx_video_id (edxval) in new runtime

95b51e8

This is used to be retroactive in copy-paste videos from Library to Course and Course to Library

refactor: Adds new edx_video_id when copy to course

c42e223

refactor: Remove unnevessary code

4bf3af3

feat: Support delete transcripts in library

1de1d4e

style: Fix lint

a59898d

Merge branch 'master' into chris/FAL-3989-video-transcripts

9173ed8

ChrisChV marked this pull request as ready for review January 29, 2025 01:37

ChrisChV mentioned this pull request Jan 29, 2025

feat: Enable transcripts for video library [FC-0076] openedx/frontend-app-authoring#1596

Merged

2 tasks

pomegranited requested changes Jan 29, 2025

View reviewed changes

ChrisChV added 5 commits January 29, 2025 19:30

style: Nits on the code

590a5cd

refactor: Update replace_transcript to verify transcript

0e54450

refactor: Verify LibraryLocatorV2 in manage_video_subtitles_save to a…

5685f16

…void raise AttributeError

style: Add comment in transcripts_ajax.py

e4f7c72

refactor: _get_item to avoid return isLibraryContent

b478ac5

ChrisChV added 12 commits February 12, 2025 18:55

refactor: _import_transcripts created in helpers.py

34e5cbc

refactor: save_video_transcript_in_learning_core created

c73f82c

save_video_transcript_in_learning_core created in transcript_ajax to avoid create an edx_video_id for library videos

refactor: _studio_transcript_upload in video_handlers

ddb6953

* _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

refactor: transcripts_ajax to avoid use edx_video_id in filenames in …

46aa8ca

…libraries

refactor: Update _get_item in transcripts_ajax to use check_permissio…

5bb426f

…n of load_block

refactor: Delete lib_permissions from content_libraries/api.py

5dda777

style: Fix typos in comments in video_handlers.py

53f42fa

style: Update comment of test_export_to_xml_without_video_id

9c7e4e8

fix: Fix tests and lint

8c6a3dd

fix: Sync transcripts in video blocks from library to course

0745f09

test: Tests for sync video block

eff9d04

style: Fix lint

4973037

bradenmacdonald reviewed Feb 19, 2025

View reviewed changes

Comment thread xmodule/video_block/video_handlers.py Outdated

bradenmacdonald reviewed Feb 19, 2025

View reviewed changes

Comment thread cms/djangoapps/contentstore/views/transcripts_ajax.py Outdated

bradenmacdonald reviewed Feb 19, 2025

View reviewed changes

Comment thread xmodule/video_block/video_handlers.py

bradenmacdonald approved these changes Feb 19, 2025

View reviewed changes

ChrisChV added 2 commits February 19, 2025 17:34

style: Nits and comments in the code

6903be2

style: Fix lint

e6ae3ad

pomegranited approved these changes Feb 20, 2025

View reviewed changes

Merge branch 'master' into chris/FAL-3989-video-transcripts

be65caa

ChrisChV merged commit b6489e7 into openedx:master Feb 21, 2025

ChrisChV deleted the chris/FAL-3989-video-transcripts branch February 21, 2025 18:33

kdmccormick reviewed Feb 24, 2025

View reviewed changes

Uh oh!

Conversation

ChrisChV commented Dec 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Supporting information

Testing instructions

Deadline

Other information

Uh oh!

openedx-webhooks commented Dec 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pomegranited left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bradenmacdonald left a comment

Choose a reason for hiding this comment

Uh oh!

pomegranited left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edx-pipeline-bot commented Feb 21, 2025

Uh oh!

kdmccormick Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edx-pipeline-bot commented Feb 24, 2025

Uh oh!

edx-pipeline-bot commented Feb 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

ChrisChV commented Dec 25, 2024 •

edited

Loading

openedx-webhooks commented Dec 25, 2024 •

edited

Loading

kdmccormick Feb 24, 2025 •

edited

Loading