From f9d85e650c6897905b8ca1e4c292e2426d706f02 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 27 Apr 2016 09:28:13 -0400 Subject: [PATCH] Remove unnecessary VideoModule save_states. Before this commit, roughly half the save_state AJAX calls for the VideoModule were done at load time, in order to send information about YouTube availability (the value for which was almost always true). This commit sends the value for what the LMS already thinks YouTube availability is for this user, so that the AJAX callback is only used in the case where the client side sees something different. [PERF-262] --- .../video/video_save_state_plugin_spec.js | 32 +++++++++++++++++-- .../js/src/video/09_save_state_plugin.js | 7 +++- .../xmodule/video_module/video_module.py | 7 +++- .../courseware/tests/test_video_mongo.py | 4 +++ 4 files changed, 45 insertions(+), 5 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/spec/video/video_save_state_plugin_spec.js b/common/lib/xmodule/xmodule/js/spec/video/video_save_state_plugin_spec.js index 85d174b215..42cdc1dcc8 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/video_save_state_plugin_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/video/video_save_state_plugin_spec.js @@ -9,12 +9,14 @@ .createSpy('onTouchBasedDevice') .and.returnValue(null); - state = jasmine.initializePlayer(); + state = jasmine.initializePlayer({ + recordedYoutubeIsAvailable: true + }); spyOn(state.storage, 'setItem'); }); afterEach(function () { - + $('source').remove(); window.onTouchBasedDevice = oldOTBD; state.storage.clear(); @@ -199,7 +201,20 @@ expect(state.storage.setItem).toHaveBeenCalledWith('language', 'ua'); }); - it('can save information about youtube availability', function () { + it('can save youtube availability', function () { + $.ajax.calls.reset(); + + // Test the cases where we shouldn't send anything at all -- client + // side code determines that YouTube availability is the same as + // what's already been recorded on the server side. + state.config.recordedYoutubeIsAvailable = true; + state.el.trigger('youtube_availability', [true]); + state.config.recordedYoutubeIsAvailable = false; + state.el.trigger('youtube_availability', [false]); + expect($.ajax).not.toHaveBeenCalled(); + + // Test that we can go from unavailable -> available + state.config.recordedYoutubeIsAvailable = false; state.el.trigger('youtube_availability', [true]); expect($.ajax).toHaveBeenCalledWith({ url: state.config.saveStateUrl, @@ -208,6 +223,17 @@ dataType: 'json', data: {youtube_is_available: true} }); + + // Test that we can go from available -> unavailable + state.config.recordedYoutubeIsAvailable = true; + state.el.trigger('youtube_availability', [false]); + expect($.ajax).toHaveBeenCalledWith({ + url: state.config.saveStateUrl, + type: 'POST', + async: true, + dataType: 'json', + data: {youtube_is_available: false} + }); }); it('can destroy itself', function () { diff --git a/common/lib/xmodule/xmodule/js/src/video/09_save_state_plugin.js b/common/lib/xmodule/xmodule/js/src/video/09_save_state_plugin.js index 11d2772803..df64d9c088 100644 --- a/common/lib/xmodule/xmodule/js/src/video/09_save_state_plugin.js +++ b/common/lib/xmodule/xmodule/js/src/video/09_save_state_plugin.js @@ -84,7 +84,12 @@ define('video/09_save_state_plugin.js', [], function() { }, onYoutubeAvailability: function (event, youtubeIsAvailable) { - this.saveState(true, {youtube_is_available: youtubeIsAvailable}); + // Compare what the client-side code has determined Youtube + // availability to be (true/false) vs. what the LMS recorded for + // this user. The LMS will assume YouTube is available by default. + if (youtubeIsAvailable !== this.state.config.recordedYoutubeIsAvailable) { + this.saveState(true, {youtube_is_available: youtubeIsAvailable}); + } }, saveState: function (async, data) { diff --git a/common/lib/xmodule/xmodule/video_module/video_module.py b/common/lib/xmodule/xmodule/video_module/video_module.py index e27e92ac55..f7e524eb17 100644 --- a/common/lib/xmodule/xmodule/video_module/video_module.py +++ b/common/lib/xmodule/xmodule/video_module/video_module.py @@ -332,7 +332,12 @@ class VideoModule(VideoFields, VideoTranscriptsMixin, VideoStudentViewHandlers, ## There is no option in the "Advanced Editor" to set this option. However, ## this option will have an effect if changed to "True". The code on ## front-end exists. - 'autohideHtml5': False + 'autohideHtml5': False, + + # This is the server's guess at whether youtube is available for + # this user, based on what was recorded the last time we saw the + # user, and defaulting to True. + 'recordedYoutubeIsAvailable': self.youtube_is_available, } bumperize(self) diff --git a/lms/djangoapps/courseware/tests/test_video_mongo.py b/lms/djangoapps/courseware/tests/test_video_mongo.py index 540317bbc4..9856814fa3 100644 --- a/lms/djangoapps/courseware/tests/test_video_mongo.py +++ b/lms/djangoapps/courseware/tests/test_video_mongo.py @@ -79,6 +79,7 @@ class TestVideoYouTube(TestVideo): self.item_descriptor, 'transcript', 'available_translations' ).rstrip('/?'), "autohideHtml5": False, + "recordedYoutubeIsAvailable": True, })), 'track': None, 'transcript_download_format': 'srt', @@ -157,6 +158,7 @@ class TestVideoNonYouTube(TestVideo): self.item_descriptor, 'transcript', 'available_translations' ).rstrip('/?'), "autohideHtml5": False, + "recordedYoutubeIsAvailable": True, })), 'track': None, 'transcript_download_format': 'srt', @@ -211,6 +213,7 @@ class TestGetHtmlMethod(BaseTestXmodule): self.item_descriptor, 'transcript', 'available_translations' ).rstrip('/?'), "autohideHtml5": False, + "recordedYoutubeIsAvailable": True, }) def test_get_html_track(self): @@ -1379,6 +1382,7 @@ class TestVideoWithBumper(TestVideo): self.item_descriptor, 'transcript', 'available_translations' ).rstrip('/?'), "autohideHtml5": False, + "recordedYoutubeIsAvailable": True, })), 'track': None, 'transcript_download_format': 'srt',