Skip to content

Hotfix/multiple videos bug - #835

Closed
valera-rozuvan wants to merge 1 commit into
masterfrom
hotfix/multiple_videos_bug
Closed

Hotfix/multiple videos bug#835
valera-rozuvan wants to merge 1 commit into
masterfrom
hotfix/multiple_videos_bug

Conversation

@valera-rozuvan

@valera-rozuvan valera-rozuvan commented Aug 30, 2013

Copy link
Copy Markdown
Contributor

This PR addresses the issue BLD-292: "If multiple video players are put on the same page, only one of them works".

@polesye Please update this description, adding any necessary information on what is being done. Also, assign someone for review.

@singingwolfboy Please review!

@ghost ghost assigned polesye Aug 30, 2013
auraz
auraz reviewed Aug 30, 2013
View reviewed changes
Comment thread common/lib/xmodule/xmodule/js/src/video/01_initialize.js Outdated

@auraz auraz Aug 30, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please do not use ?: in multiline if

@polesye

polesye commented Aug 30, 2013

Copy link
Copy Markdown
Contributor

@wedaly Please review.

@wedaly

wedaly commented Aug 30, 2013

Copy link
Copy Markdown
Contributor

  1. I'm seeing three test failures in Jenkins related to the videos: http://jenkins.edx.org:8080/job/edx-feature-branch-tests/11210/testReport/?

  2. Please squash these commits.

@Lyla-Fischer

Lyla-Fischer commented Sep 2, 2013

Copy link
Copy Markdown

@nedbat can you also review this fix and confirm (or at least give a probability over 95%) that this patch will fix our intermittent YouTube error (BLD-313)?

@valera-rozuvan

valera-rozuvan commented Sep 3, 2013

Copy link
Copy Markdown
Contributor Author

@nedbat Should be ready for you r review. Putting up my comment from https://edx-wiki.atlassian.net/browse/BLD-313 :

The following has been done to address the current issue (Video player doesn't play for some students):

if "1.0" YouTube ID has not been specified, and no non-YouTube
sources were specified
or
if "1.0" YouTube ID has not been specified, and non-YouTube sources
were specified, but browser does not support their type
or
if YouTube AJAX request exceeds the time-out, and no non-YouTube
sources were specified

then

video is not rendered, and a message instead of the video is displayed.

@valera-rozuvan

valera-rozuvan commented Sep 3, 2013

Copy link
Copy Markdown
Contributor Author

Update:

if YouTube AJAX request exceeds the time-out, and no non-YouTube
sources were specified

we continue loading YouTube video in-case that the timeout was to short, and that YouTube is accessible. In the case of China, or some other place where YouTube is unavailable, instead of the video, some page will be shown where the YouTube request is set to redirect (by external routers).

@ghost ghost assigned singingwolfboy Sep 3, 2013
@adampalay

adampalay commented Sep 3, 2013

Copy link
Copy Markdown
Contributor

When this is approved, can you make another branch off of release so that we may hotfix it?

nedbat
nedbat reviewed Sep 3, 2013
View reviewed changes
Comment thread common/lib/xmodule/xmodule/video_module.py Outdated

@nedbat nedbat Sep 3, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

getattr(settings, 'VIDEO_PORT', None) is better: it's very rare to call dunder methods directly.

@auraz auraz Sep 3, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fixed, thank you.

@adampalay

adampalay commented Sep 3, 2013

Copy link
Copy Markdown
Contributor

In fact, if this is going to be hotfixed, this branch needs to be rebased off of release for testing purposes

@chrisndodge

chrisndodge commented Sep 3, 2013

Copy link
Copy Markdown
Contributor

Did a quick spot check on localdev and the video which had been failing to load on master is working on this branch

@nedbat

nedbat commented Sep 3, 2013

Copy link
Copy Markdown
Contributor

👍

@polesye @auraz
Fix multiple video bug.
0e76bcf 
Adding yt_test_timeout option on the backend.
Updating state.config to read yt test timeout from the attributes of
video div.
Add additional fallbacks.
Add acceptance tests for supporting Youtube failover.
Add yt-test-url to fixtures.
Add Youtube failover parameters to tests.
Wrapped HTML error message in i18n.
@valera-rozuvan

valera-rozuvan commented Sep 4, 2013

Copy link
Copy Markdown
Contributor Author

Carried over to https://github.com/edx/edx-platform/pull/850 and https://github.com/edx/edx-platform/pull/851 . Merged to master.

@valera-rozuvan
valera-rozuvan deleted the hotfix/multiple_videos_bug branch September 4, 2013 07:01
ny0m referenced this pull request in open-craft/openedx-platform Aug 14, 2017
@e-kolpakov
[MCKIN-5253] Problem builder version bump (#835)
f56cd64 
[MCKIN-5253] Version bump to problem-builder v2.7.0
andrey-canon pushed a commit to eduNEXT/edx-platform that referenced this pull request Jul 25, 2018
Nico van Niekerk
Merge pull request openedx#835 from proversity-org/proversity/ENH-agr…
ecfe56c 
…eement-switch

ENH: setting to remove platform name from registration page in honor link
@baby636 baby636 mentioned this pull request Nov 5, 2022
Open
@baby636 baby636 mentioned this pull request Dec 26, 2022
Open
DanielVZ96 referenced this pull request in open-craft/openedx-platform Jan 31, 2024
@edx-requirements-bot
chore: Updating Python Requirements (#835)
7eb6bff
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@singingwolfboy singingwolfboy

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@valera-rozuvan @polesye @wedaly @Lyla-Fischer @adampalay @chrisndodge @nedbat @auraz @singingwolfboy