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.

Sign up for GitHub

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

Jump to bottom

Hotfix/multiple videos bug #835

Closed
wants to merge 1 commit into from
Closed

Hotfix/multiple videos bug #835

wants to merge 1 commit into from

Conversation

valera-rozuvan
Copy link
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
common/lib/xmodule/xmodule/js/src/video/01_initialize.js
(isFinite(parseInt(this.el.data('yt-test-timeout'), 10))) ?
parseInt(this.el.data('yt-test-timeout'), 10) :
1500
),
Copy link
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
Copy link
Contributor

polesye commented Aug 30, 2013

@wedaly Please review.

@wedaly
Copy link
Contributor

wedaly commented Aug 30, 2013

  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
Copy link

@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
Copy link
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
Copy link
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
Copy link
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
common/lib/xmodule/xmodule/video_module.py
@@ -164,6 +164,12 @@ def get_html(self):
sources = {get_ext(src): src for src in self.html5_sources}
sources['main'] = self.source

# for testing Youtube timeout in acceptance tests
if settings.__getattr__('VIDEO_PORT', None):
Copy link
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.

Copy link
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
Copy link
Contributor

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

@chrisndodge
Copy link
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
Copy link
Contributor

nedbat commented Sep 3, 2013

👍

@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
Copy link
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 pushed a commit to open-craft/edx-platform that referenced this pull request Aug 14, 2017
@e-kolpakov
[MCKIN-5253] Problem builder version bump (openedx#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
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
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