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

fix(AU-2128): Let 'en' be selected as transcript by default when youtube video #35116

Merged
merged 3 commits into from
Jul 15, 2024

Conversation

Rodra
Copy link
Contributor

@Rodra Rodra commented Jul 12, 2024

AU-2128

Description

Let english transcript be selected by default in video player despite transcript not available in transcripts["transcripts"] but available in transcripts["sub"].

Current Behavior
https://www.loom.com/share/a4e31efd83d8418f9577f3e43c9a5721?sid=34060663-e8dc-456a-a49d-ea163b650c4d

New Behavior
https://www.loom.com/share/c7cf07c1440942c893e7287496b83b7b?sid=bec454c3-7685-4dca-9970-daf341d64c43

Testing instructions

Deadline

Other information

Copy link
Contributor

@nsprenkle nsprenkle left a comment

Choose a reason for hiding this comment

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

@Rodra , could you add some comments here to explain the logic, I'm finding it hard to parse through.

@Rodra
Copy link
Contributor Author

Rodra commented Jul 15, 2024

@Rodra , could you add some comments here to explain the logic, I'm finding it hard to parse through.

Sure, this new condition when selecting the default transcript is the one that does the trick https://github.com/openedx/edx-platform/pull/35116/files#diff-a7e3c5a7f8c78b1241f3617e180bc1976ac83b45e804055e4a677eed0bca87e4R872.

Since at this point of the code https://github.com/openedx/edx-platform/pull/35116/files#diff-a7e3c5a7f8c78b1241f3617e180bc1976ac83b45e804055e4a677eed0bca87e4L867 other_langs is a list that doesn't have english (because is not part of that list if it's a youtube video), we were skipping the first condition of the if (https://github.com/openedx/edx-platform/pull/35116/files#diff-a7e3c5a7f8c78b1241f3617e180bc1976ac83b45e804055e4a677eed0bca87e4L867) and the transcript language selected ends up being the one that was set prior to triggering this action (https://github.com/openedx/edx-platform/pull/35116/files#diff-a7e3c5a7f8c78b1241f3617e180bc1976ac83b45e804055e4a677eed0bca87e4L871).

But I noticed that english IS being added to the transcript options of the video player, but a little bit later in here https://github.com/openedx/edx-platform/blob/master/xmodule/video_block/video_block.py#L191. So I just reused this condition to select transcript as default even if english is not inside of the transcript options list yet. Because I'm sure it will be added later.

Copy link
Contributor

@rayzhou-bit rayzhou-bit left a comment

Choose a reason for hiding this comment

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

lgtm!

@rayzhou-bit rayzhou-bit merged commit 69b4f69 into master Jul 15, 2024
49 checks passed
@rayzhou-bit rayzhou-bit deleted the fix/AU-2128 branch July 15, 2024 17:00
@edx-pipeline-bot
Copy link
Contributor

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

@edx-pipeline-bot
Copy link
Contributor

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

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

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

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been rolled back from the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been rolled back from the edX production environment.

@edx-pipeline-bot
Copy link
Contributor

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

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

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

mudassir-hafeez pushed a commit to mudassir-hafeez/edx-platform that referenced this pull request Jul 24, 2024
mudassir-hafeez pushed a commit to mudassir-hafeez/edx-platform that referenced this pull request Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants