Commit e9e59e85 by Nimisha Asthagiri

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.
parent a6130507
...@@ -304,14 +304,7 @@ class VideoDescriptor(VideoFields, VideoTranscriptsMixin, VideoStudioViewHandler ...@@ -304,14 +304,7 @@ class VideoDescriptor(VideoFields, VideoTranscriptsMixin, VideoStudioViewHandler
self._field_data.set_many(self, field_data) self._field_data.set_many(self, field_data)
del self.data del self.data
editable_fields = super(VideoDescriptor, self).editable_metadata_fields
self.source_visible = False 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 self.source:
# If `source` field value exist in the `html5_sources` field values, # If `source` field value exist in the `html5_sources` field values,
# then delete `source` field value and use value from `html5_sources` field. # then delete `source` field value and use value from `html5_sources` field.
...@@ -320,14 +313,17 @@ class VideoDescriptor(VideoFields, VideoTranscriptsMixin, VideoStudioViewHandler ...@@ -320,14 +313,17 @@ class VideoDescriptor(VideoFields, VideoTranscriptsMixin, VideoStudioViewHandler
self.download_video = True self.download_video = True
else: # Otherwise, `source` field value will be used. else: # Otherwise, `source` field value will be used.
self.source_visible = True self.source_visible = True
if not download_video['explicitly_set']: if not self.fields['download_video'].is_set_on(self):
self.download_video = True 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. # for backward compatibility.
# If course was existed and was not re-imported by the moment of adding `download_track` field, # 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: # we should enable `download_track` if following is true:
download_track = editable_fields['download_track'] if not self.fields['download_track'].is_set_on(self) and self.track:
if not download_track['explicitly_set'] and self.track:
self.download_track = True self.download_track = True
def editor_saved(self, user, old_metadata, old_content): def editor_saved(self, user, old_metadata, old_content):
......
# -*- coding: utf-8 -*- # -*- coding: utf-8 -*-
"""Video xmodule tests in mongo.""" """Video xmodule tests in mongo."""
import json import json
import unittest
from collections import OrderedDict from collections import OrderedDict
from mock import patch, PropertyMock, MagicMock from mock import patch, MagicMock
from django.conf import settings from django.conf import settings
from xblock.fields import ScopeIds from xmodule.video_module import create_youtube_string
from xblock.field_data import DictFieldData
from xmodule.video_module import create_youtube_string, VideoDescriptor
from xmodule.x_module import STUDENT_VIEW from xmodule.x_module import STUDENT_VIEW
from xmodule.tests import get_test_descriptor_system
from xmodule.tests.test_video import VideoDescriptorTestBase from xmodule.tests.test_video import VideoDescriptorTestBase
from edxval.api import ( from edxval.api import create_profile, create_video
ValVideoNotFoundError,
get_video_info,
create_profile,
create_video
)
import mock
from . import BaseTestXmodule from . import BaseTestXmodule
from .test_video_xml import SOURCE_XML from .test_video_xml import SOURCE_XML
...@@ -417,7 +406,7 @@ class TestGetHtmlMethod(BaseTestXmodule): ...@@ -417,7 +406,7 @@ class TestGetHtmlMethod(BaseTestXmodule):
# it'll just fall back to the values in the VideoDescriptor. # it'll just fall back to the values in the VideoDescriptor.
self.assertIn("example_source.mp4", self.item_descriptor.render(STUDENT_VIEW).content) 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): def test_get_html_with_mocked_edx_video_id(self, mock_get_video_info):
mock_get_video_info.return_value = { mock_get_video_info.return_value = {
'url': '/edxval/video/example', 'url': '/edxval/video/example',
...@@ -798,77 +787,18 @@ class TestVideoDescriptorInitialization(BaseTestXmodule): ...@@ -798,77 +787,18 @@ class TestVideoDescriptorInitialization(BaseTestXmodule):
self.assertFalse(self.item_descriptor.source_visible) self.assertFalse(self.item_descriptor.source_visible)
def test_download_video_is_explicitly_set(self): 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 = { metadata = {
'track': u'http://some_track.srt', 'track': u'http://some_track.srt',
'source': 'http://example.org/video.mp4', 'source': 'http://example.org/video.mp4',
'html5_sources': ['http://youtu.be/3_yD_cEKoCk.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 fields = self.item_descriptor.editable_metadata_fields
self.assertIn('source', fields) self.assertIn('source', fields)
self.assertIn('download_video', fields)
self.assertFalse(self.item_descriptor.download_video) self.assertFalse(self.item_descriptor.download_video)
self.assertTrue(self.item_descriptor.source_visible) self.assertTrue(self.item_descriptor.source_visible)
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment