-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Define custom completion for VideoModule #16569
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
Changes from all commits
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 |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| (function() { | ||
| 'use strict'; | ||
| describe('VideoPlayer completion', function() { | ||
| var state, oldOTBD; | ||
|
|
||
| beforeEach(function() { | ||
| oldOTBD = window.onTouchBasedDevice; | ||
| window.onTouchBasedDevice = jasmine | ||
| .createSpy('onTouchBasedDevice') | ||
| .and.returnValue(null); | ||
|
|
||
| state = jasmine.initializePlayer({ | ||
| recordedYoutubeIsAvailable: true, | ||
| completionEnabled: true, | ||
| publishCompletionUrl: 'https://example.com/publish_completion_url' | ||
|
|
||
| }); | ||
| state.completionHandler.completeAfterTime = 20; | ||
| }); | ||
|
|
||
| afterEach(function() { | ||
| $('source').remove(); | ||
| window.onTouchBasedDevice = oldOTBD; | ||
| state.storage.clear(); | ||
| if (state.videoPlayer) { | ||
| state.videoPlayer.destroy(); | ||
| } | ||
| }); | ||
|
|
||
| it('calls the completion api when marking an object complete', function() { | ||
| state.completionHandler.markCompletion(Date.now()); | ||
| expect($.ajax).toHaveBeenCalledWith({ | ||
| url: state.config.publishCompletionUrl, | ||
| type: 'POST', | ||
| contentType: 'application/json', | ||
| dataType: 'json', | ||
| data: JSON.stringify({completion: 1.0}), | ||
| success: jasmine.any(Function), | ||
| error: jasmine.any(Function) | ||
| }); | ||
| expect(state.completionHandler.isComplete).toEqual(true); | ||
| }); | ||
|
|
||
| it('calls the completion api on the LMS when the time updates', function() { | ||
| spyOn(state.completionHandler, 'markCompletion').and.callThrough(); | ||
| state.el.trigger('timeupdate', 24.0); | ||
| expect(state.completionHandler.markCompletion).toHaveBeenCalled(); | ||
| state.completionHandler.markCompletion.calls.reset(); | ||
| // But the handler is not called again after the block is completed. | ||
| state.el.trigger('timeupdate', 30.0); | ||
| expect(state.completionHandler.markCompletion).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('calls the completion api on the LMS when the video ends', function() { | ||
| spyOn(state.completionHandler, 'markCompletion').and.callThrough(); | ||
| state.el.trigger('ended'); | ||
| expect(state.completionHandler.markCompletion).toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
| }).call(this); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,178 @@ | ||
| (function(define) { | ||
|
Contributor
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. Seems like there should be tests added to:
Contributor
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. Does thumbs-up mean this is coming?
Contributor
Author
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. Yes. I worked on tests on Friday, but wasn't able to get them all in place. I'll ping you again when they are ready or by end of day at the latest. |
||
| 'use strict'; | ||
| /** | ||
| * Completion handler | ||
| * @exports video/09_completion.js | ||
| * @constructor | ||
| * @param {Object} state The object containing the state of the video | ||
| * @return {jquery Promise} | ||
| */ | ||
| define('video/09_completion.js', [], function() { | ||
| var VideoCompletionHandler = function(state) { | ||
| if (!(this instanceof VideoCompletionHandler)) { | ||
| return new VideoCompletionHandler(state); | ||
| } | ||
| this.state = state; | ||
| this.state.completionHandler = this; | ||
| this.initialize(); | ||
| return $.Deferred().resolve().promise(); | ||
| }; | ||
|
|
||
| VideoCompletionHandler.prototype = { | ||
|
|
||
| /** Tears down the VideoCompletionHandler. | ||
| * | ||
| * * Removes backreferences from this.state to this. | ||
| * * Turns off signal handlers. | ||
| */ | ||
| destroy: function() { | ||
| this.el.remove(); | ||
| this.el.off('timeupdate.completion'); | ||
| this.el.off('ended.completion'); | ||
| delete this.state.completionHandler; | ||
| }, | ||
|
|
||
| /** Initializes the VideoCompletionHandler. | ||
| * | ||
| * This sets all the instance variables needed to perform | ||
| * completion calculations. | ||
| */ | ||
| initialize: function() { | ||
| // Attributes with "Time" in the name refer to the number of seconds since | ||
| // the beginning of the video, except for lastSentTime, which refers to a | ||
| // timestamp in seconds since the Unix epoch. | ||
| this.lastSentTime = undefined; | ||
| this.isComplete = false; | ||
| this.completionPercentage = this.state.config.completionPercentage; | ||
| this.startTime = this.state.config.startTime; | ||
| this.endTime = this.state.config.endTime; | ||
| this.isEnabled = this.state.config.completionEnabled; | ||
| if (this.endTime) { | ||
| this.completeAfterTime = this.calculateCompleteAfterTime(this.startTime, this.endTime); | ||
| } | ||
| if (this.isEnabled) { | ||
| this.bindHandlers(); | ||
| } | ||
| }, | ||
|
|
||
| /** Bind event handler callbacks. | ||
| * | ||
| * When ended is triggered, mark the video complete | ||
| * unconditionally. | ||
| * | ||
| * When timeupdate is triggered, check to see if the user has | ||
| * passed the completeAfterTime in the video, and if so, mark the | ||
| * video complete. | ||
| * | ||
| * When destroy is triggered, clean up outstanding resources. | ||
| */ | ||
| bindHandlers: function() { | ||
| var self = this; | ||
|
|
||
| /** Event handler to check if the video is complete, and submit | ||
| * a completion if it is. | ||
| * | ||
| * If the timeupdate handler doesn't fire after the required | ||
| * percentage, this will catch any fully complete videos. | ||
| */ | ||
| this.state.el.on('ended.completion', function() { | ||
| self.handleEnded(); | ||
| }); | ||
|
|
||
| /** Event handler to check video progress, and mark complete if | ||
| * greater than completionPercentage | ||
| */ | ||
| this.state.el.on('timeupdate.completion', function(ev, currentTime) { | ||
| self.handleTimeUpdate(currentTime); | ||
| }); | ||
|
|
||
| /** Event handler to clean up resources when the video player | ||
| * is destroyed. | ||
| */ | ||
| this.state.el.off('destroy', this.destroy); | ||
| }, | ||
|
|
||
| /** Handler to call when the ended event is triggered */ | ||
| handleEnded: function() { | ||
| if (this.isComplete) { | ||
| return; | ||
| } | ||
| this.markCompletion(); | ||
| }, | ||
|
|
||
| /** Handler to call when a timeupdate event is triggered */ | ||
| handleTimeUpdate: function(currentTime) { | ||
| var duration; | ||
| if (this.isComplete) { | ||
| return; | ||
| } | ||
| if (this.lastSentTime !== undefined && currentTime - this.lastSentTime < this.repostDelaySeconds()) { | ||
| // Throttle attempts to submit in case of network issues | ||
| return; | ||
| } | ||
| if (this.completeAfterTime === undefined) { | ||
| // Duration is not available at initialization time | ||
| duration = this.state.videoPlayer.duration(); | ||
| if (!duration) { | ||
| // duration is not yet set. Wait for another event, | ||
| // or fall back to 'ended' handler. | ||
| return; | ||
| } | ||
| this.completeAfterTime = this.calculateCompleteAfterTime(this.startTime, duration); | ||
| } | ||
|
|
||
| if (currentTime > this.completeAfterTime) { | ||
| this.markCompletion(currentTime); | ||
| } | ||
| }, | ||
|
|
||
| /** Submit completion to the LMS */ | ||
| markCompletion: function(currentTime) { | ||
| var self = this; | ||
| var errmsg; | ||
| this.isComplete = true; | ||
| this.lastSentTime = currentTime; | ||
| if (this.state.config.publishCompletionUrl) { | ||
| $.ajax({ | ||
| type: 'POST', | ||
| url: this.state.config.publishCompletionUrl, | ||
| contentType: 'application/json', | ||
| dataType: 'json', | ||
| data: JSON.stringify({completion: 1.0}), | ||
|
Contributor
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. @jcdyer I messed around with a couple of the jquery.ajax options for posting data, like Also found this stackoverflow post which suggests stringifying the data like you did. So no worries here :) |
||
| success: function() { | ||
| self.state.el.off('timeupdate.completion'); | ||
| self.state.el.off('ended.completion'); | ||
| }, | ||
| error: function(xhr) { | ||
| /* eslint-disable no-console */ | ||
| self.complete = false; | ||
| errmsg = 'Failed to submit completion'; | ||
| if (xhr.responseJSON !== undefined) { | ||
| errmsg += ': ' + xhr.responseJSON.error; | ||
| } | ||
| console.warn(errmsg); | ||
| /* eslint-enable no-console */ | ||
| } | ||
| }); | ||
| } else { | ||
| /* eslint-disable no-console */ | ||
| console.warn('publishCompletionUrl not defined'); | ||
| /* eslint-enable no-console */ | ||
| } | ||
| }, | ||
|
|
||
| /** Determine what point in the video (in seconds from the | ||
| * beginning) counts as complete. | ||
| */ | ||
| calculateCompleteAfterTime: function(startTime, endTime) { | ||
| return startTime + (endTime - startTime) * this.completionPercentage; | ||
| }, | ||
|
|
||
| /** How many seconds to wait after a POST fails to try again. */ | ||
| repostDelaySeconds: function() { | ||
| return 3.0; | ||
| } | ||
| }; | ||
| return VideoCompletionHandler; | ||
| }); | ||
| }(RequireJS.define)); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,14 @@ | ||
| """ | ||
| Utils for video bumper | ||
| """ | ||
| from collections import OrderedDict | ||
| import copy | ||
| import json | ||
| import pytz | ||
| import logging | ||
| from collections import OrderedDict | ||
|
|
||
| from datetime import datetime, timedelta | ||
| from django.conf import settings | ||
| import pytz | ||
|
|
||
| from .video_utils import set_query_parameter | ||
|
|
||
|
|
@@ -137,6 +137,9 @@ def bumper_metadata(video, sources): | |
| 'transcriptAvailableTranslationsUrl': set_query_parameter( | ||
| video.runtime.handler_url(video, 'transcript', 'available_translations').rstrip('/?'), 'is_bumper', 1 | ||
| ), | ||
| 'publishCompletionUrl': set_query_parameter( | ||
| video.runtime.handler_url(video, 'publish_completion', '').rstrip('?'), 'is_bumper', 1 | ||
|
Contributor
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. Not sure if the difference is intentional and needed, but just noting the difference between
Contributor
Author
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. Yeah. I tested both ways, and noticed that the trailing / wasn't actually changing the behavior of the call, so I figured why worry about it? |
||
| ), | ||
| }) | ||
|
|
||
| return metadata | ||
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.
This test seems to be flaky. I saw it fail once, and couldn't bear to let the typo survive. I'm not sure what the process is for external contributors marking tests flaky, so I haven't created any tickets about it.
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.
If you know it to be flaky, I don't see any issue with you following the updated flaky test process.