From e9e59e85fc28cba14fa46842c83ee7cabf6c4ff4 Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Wed, 4 Feb 2015 20:41:29 -0500 Subject: [PATCH] Optimize video_module's __init__ method. The overall behavior of the __init__ method has remained the same. What's changed is how it determines whether a field is explicitly set. Instead of using the slower performing editable_metadata_fields, it calls _field_data.has directly to check for explicitly set fields. --- .../xmodule/video_module/video_module.py | 16 +-- .../courseware/tests/test_video_mongo.py | 104 +++--------------- 2 files changed, 23 insertions(+), 97 deletions(-) diff --git a/common/lib/xmodule/xmodule/video_module/video_module.py b/common/lib/xmodule/xmodule/video_module/video_module.py index 1d3111191c..d6007513b7 100644 --- a/common/lib/xmodule/xmodule/video_module/video_module.py +++ b/common/lib/xmodule/xmodule/video_module/video_module.py @@ -304,14 +304,7 @@ class VideoDescriptor(VideoFields, VideoTranscriptsMixin, VideoStudioViewHandler self._field_data.set_many(self, field_data) del self.data - editable_fields = super(VideoDescriptor, self).editable_metadata_fields - self.source_visible = False - # Set download_video field to default value if its not explicitly set for backward compatibility. - download_video = editable_fields['download_video'] - if not download_video['explicitly_set']: - self.download_video = self.download_video - if self.source: # If `source` field value exist in the `html5_sources` field values, # then delete `source` field value and use value from `html5_sources` field. @@ -320,14 +313,17 @@ class VideoDescriptor(VideoFields, VideoTranscriptsMixin, VideoStudioViewHandler self.download_video = True else: # Otherwise, `source` field value will be used. self.source_visible = True - if not download_video['explicitly_set']: + if not self.fields['download_video'].is_set_on(self): self.download_video = True + # Set download_video field to default value if its not explicitly set for backward compatibility. + if not self.fields['download_video'].is_set_on(self): + self.download_video = self.download_video + # for backward compatibility. # If course was existed and was not re-imported by the moment of adding `download_track` field, # we should enable `download_track` if following is true: - download_track = editable_fields['download_track'] - if not download_track['explicitly_set'] and self.track: + if not self.fields['download_track'].is_set_on(self) and self.track: self.download_track = True def editor_saved(self, user, old_metadata, old_content): diff --git a/lms/djangoapps/courseware/tests/test_video_mongo.py b/lms/djangoapps/courseware/tests/test_video_mongo.py index 4fdfc76a9c..789cc388d4 100644 --- a/lms/djangoapps/courseware/tests/test_video_mongo.py +++ b/lms/djangoapps/courseware/tests/test_video_mongo.py @@ -1,27 +1,16 @@ # -*- coding: utf-8 -*- """Video xmodule tests in mongo.""" import json -import unittest from collections import OrderedDict -from mock import patch, PropertyMock, MagicMock +from mock import patch, MagicMock from django.conf import settings -from xblock.fields import ScopeIds -from xblock.field_data import DictFieldData - -from xmodule.video_module import create_youtube_string, VideoDescriptor +from xmodule.video_module import create_youtube_string from xmodule.x_module import STUDENT_VIEW -from xmodule.tests import get_test_descriptor_system from xmodule.tests.test_video import VideoDescriptorTestBase -from edxval.api import ( - ValVideoNotFoundError, - get_video_info, - create_profile, - create_video -) -import mock +from edxval.api import create_profile, create_video from . import BaseTestXmodule from .test_video_xml import SOURCE_XML @@ -417,7 +406,7 @@ class TestGetHtmlMethod(BaseTestXmodule): # it'll just fall back to the values in the VideoDescriptor. self.assertIn("example_source.mp4", self.item_descriptor.render(STUDENT_VIEW).content) - @mock.patch('edxval.api.get_video_info') + @patch('edxval.api.get_video_info') def test_get_html_with_mocked_edx_video_id(self, mock_get_video_info): mock_get_video_info.return_value = { 'url': '/edxval/video/example', @@ -798,81 +787,22 @@ class TestVideoDescriptorInitialization(BaseTestXmodule): self.assertFalse(self.item_descriptor.source_visible) def test_download_video_is_explicitly_set(self): - with patch( - 'xmodule.editing_module.TabsEditingDescriptor.editable_metadata_fields', - new_callable=PropertyMock, - return_value={ - 'download_video': { - 'default_value': False, - 'explicitly_set': True, - 'display_name': 'Video Download Allowed', - 'help': 'Show a link beneath the video to allow students to download the video.', - 'type': 'Boolean', - 'value': False, - 'field_name': 'download_video', - 'options': [ - {'display_name': "True", "value": True}, - {'display_name': "False", "value": False} - ], - }, - 'html5_sources': { - 'default_value': [], - 'explicitly_set': False, - 'display_name': 'Video Sources', - 'help': 'A list of filenames to be used with HTML5 video.', - 'type': 'List', - 'value': [u'http://youtu.be/3_yD_cEKoCk.mp4'], - 'field_name': 'html5_sources', - 'options': [], - }, - 'source': { - 'default_value': '', - 'explicitly_set': False, - 'display_name': 'Download Video', - 'help': 'The external URL to download the video.', - 'type': 'Generic', - 'value': u'http://example.org/video.mp4', - 'field_name': 'source', - 'options': [], - }, - 'track': { - 'default_value': '', - 'explicitly_set': False, - 'display_name': 'Download Transcript', - 'help': 'The external URL to download the timed transcript track.', - 'type': 'Generic', - 'value': u'http://some_track.srt', - 'field_name': 'track', - 'options': [], - }, - 'download_track': { - 'default_value': False, - 'explicitly_set': False, - 'display_name': 'Transcript Download Allowed', - 'help': 'Show a link beneath the video to allow students to download the transcript. Note: You must add a link to the HTML5 Transcript field above.', - 'type': 'Generic', - 'value': False, - 'field_name': 'download_track', - 'options': [], - }, - 'transcripts': {}, - 'handout': {}, - } - ): - metadata = { - 'track': u'http://some_track.srt', - 'source': 'http://example.org/video.mp4', - 'html5_sources': ['http://youtu.be/3_yD_cEKoCk.mp4'], - } + metadata = { + 'track': u'http://some_track.srt', + 'source': 'http://example.org/video.mp4', + 'html5_sources': ['http://youtu.be/3_yD_cEKoCk.mp4'], + 'download_video': False, + } - self.initialize_module(metadata=metadata) + self.initialize_module(metadata=metadata) - fields = self.item_descriptor.editable_metadata_fields - self.assertIn('source', fields) + fields = self.item_descriptor.editable_metadata_fields + self.assertIn('source', fields) + self.assertIn('download_video', fields) - self.assertFalse(self.item_descriptor.download_video) - self.assertTrue(self.item_descriptor.source_visible) - self.assertTrue(self.item_descriptor.download_track) + self.assertFalse(self.item_descriptor.download_video) + self.assertTrue(self.item_descriptor.source_visible) + self.assertTrue(self.item_descriptor.download_track) def test_source_is_empty(self): metadata = {