Merge branch 'release'

Conflicts: common/lib/xmodule/xmodule/video_module.py
2013-08-16 14:25:03 -04:00
parent f3e282e3ed 9c67e56d00
commit e91205aff3
6 changed files with 137 additions and 20 deletions
--- a/cms/djangoapps/contentstore/views/assets.py
+++ b/cms/djangoapps/contentstore/views/assets.py
@@ -359,6 +359,8 @@ def generate_export_course(request, org, course, name):
    try:
        export_to_xml(modulestore('direct'), contentstore(), loc, root_dir, name, modulestore())
    except SerializationError, e:
+        logging.exception('There was an error exporting course {0}. {1}'.format(course_module.location, unicode(e)))
+
        unit = None
        failed_item = None
        parent = None
@@ -391,6 +393,7 @@ def generate_export_course(request, org, course, name):
            })
        })
    except Exception, e:
+        logging.exception('There was an error exporting course {0}. {1}'.format(course_module.location, unicode(e)))
        return render_to_response('export.html', {
            'context_course': course_module,
            'successful_import_redirect_url': '',
--- a/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js
+++ b/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js
@@ -312,15 +312,34 @@ function () {
        var newIndex;

        if (this.videoCaption.loaded) {
-            time = Math.round(Time.convert(time, this.speed, '1.0') * 1000 + 250);
+            // Current mode === 'flash' can only be for YouTube videos. So, we
+            // don't have to also check for videoType === 'youtube'.
+            if (this.currentPlayerMode === 'flash') {
+                // Total play time changes with speed change. Also there is
+                // a 250 ms delay we have to take into account.
+                time = Math.round(
+                    Time.convert(time, this.speed, '1.0') * 1000 + 250
+                );
+            } else {
+                // Total play time remains constant when speed changes.
+                time = Math.round(parseInt(time, 10) * 1000);
+            }
+
            newIndex = this.videoCaption.search(time);

-            if (newIndex !== void 0 && this.videoCaption.currentIndex !== newIndex) {
+            if (
+                newIndex !== void 0 &&
+                this.videoCaption.currentIndex !== newIndex
+            ) {
                if (this.videoCaption.currentIndex) {
-                    this.videoCaption.subtitlesEl.find('li.current').removeClass('current');
+                    this.videoCaption.subtitlesEl
+                        .find('li.current')
+                        .removeClass('current');
                }

-                this.videoCaption.subtitlesEl.find("li[data-index='" + newIndex + "']").addClass('current');
+                this.videoCaption.subtitlesEl
+                    .find("li[data-index='" + newIndex + "']")
+                    .addClass('current');

                this.videoCaption.currentIndex = newIndex;

@@ -333,9 +352,29 @@ function () {
        var time;

        event.preventDefault();
-        time = Math.round(Time.convert($(event.target).data('start'), '1.0', this.speed) / 1000);

-        this.trigger('videoPlayer.onCaptionSeek', {'type': 'onCaptionSeek', 'time': time});
+        // Current mode === 'flash' can only be for YouTube videos. So, we
+        // don't have to also check for videoType === 'youtube'.
+        if (this.currentPlayerMode === 'flash') {
+            // Total play time changes with speed change. Also there is
+            // a 250 ms delay we have to take into account.
+            time = Math.round(
+                Time.convert(
+                    $(event.target).data('start'), '1.0', this.speed
+                ) / 1000
+            );
+        } else {
+            // Total play time remains constant when speed changes.
+            time = parseInt($(event.target).data('start'), 10)/1000;
+        }
+
+        this.trigger(
+            'videoPlayer.onCaptionSeek',
+            {
+                'type': 'onCaptionSeek',
+                'time': time
+            }
+        );
    }

    function calculateOffset(element) {
--- a/common/lib/xmodule/xmodule/tests/test_video.py
+++ b/common/lib/xmodule/xmodule/tests/test_video.py
@@ -15,6 +15,7 @@ the course, section, subsection, unit, etc.

 import unittest
 from . import LogicTest
+from lxml import etree
 from .import get_test_system
 from xmodule.modulestore import Location
 from xmodule.video_module import VideoDescriptor, _create_youtube_string
@@ -289,6 +290,62 @@ class VideoDescriptorImportTestCase(unittest.TestCase):
            'data': ''
        })

+    def test_from_xml_double_quotes(self):
+        """
+        Make sure we can handle the double-quoted string format (which was used for exporting for
+        a few weeks).
+        """
+        module_system = DummySystem(load_error_modules=True)
+        xml_data ='''
+            <video display_name="&quot;display_name&quot;"
+                html5_sources="[&quot;source_1&quot;, &quot;source_2&quot;]"
+                show_captions="false"
+                source="&quot;http://download_video&quot;"
+                sub="&quot;html5_subtitles&quot;"
+                track="&quot;http://download_track&quot;"
+                youtube_id_0_75="&quot;OEoXaMPEzf65&quot;"
+                youtube_id_1_25="&quot;OEoXaMPEzf125&quot;"
+                youtube_id_1_5="&quot;OEoXaMPEzf15&quot;"
+                youtube_id_1_0="&quot;OEoXaMPEzf10&quot;"
+                />
+        '''
+        output = VideoDescriptor.from_xml(xml_data, module_system)
+        self.assert_attributes_equal(output, {
+            'youtube_id_0_75': 'OEoXaMPEzf65',
+            'youtube_id_1_0': 'OEoXaMPEzf10',
+            'youtube_id_1_25': 'OEoXaMPEzf125',
+            'youtube_id_1_5': 'OEoXaMPEzf15',
+            'show_captions': False,
+            'start_time': 0.0,
+            'end_time': 0.0,
+            'track': 'http://download_track',
+            'source': 'http://download_video',
+            'html5_sources': ["source_1", "source_2"],
+            'data': ''
+        })
+
+    def test_from_xml_double_quote_concatenated_youtube(self):
+        module_system = DummySystem(load_error_modules=True)
+        xml_data = '''
+            <video display_name="Test Video"
+                   youtube="1.0:&quot;p2Q6BrNhdh8&quot;,1.25:&quot;1EeWXzPdhSA&quot;">
+            </video>
+        '''
+        output = VideoDescriptor.from_xml(xml_data, module_system)
+        self.assert_attributes_equal(output, {
+            'youtube_id_0_75': '',
+            'youtube_id_1_0': 'p2Q6BrNhdh8',
+            'youtube_id_1_25': '1EeWXzPdhSA',
+            'youtube_id_1_5': '',
+            'show_captions': True,
+            'start_time': 0.0,
+            'end_time': 0.0,
+            'track': '',
+            'source': '',
+            'html5_sources': [],
+            'data': ''
+        })
+
    def test_old_video_format(self):
        """
        Test backwards compatibility with VideoModule's XML format.
@@ -370,7 +427,7 @@ class VideoExportTestCase(unittest.TestCase):
        desc.track = 'http://www.example.com/track'
        desc.html5_sources = ['http://www.example.com/source.mp4', 'http://www.example.com/source.ogg']

-        xml = desc.export_to_xml(None)  # We don't use the `resource_fs` parameter
+        xml = desc.definition_to_xml(None)  # We don't use the `resource_fs` parameter
        expected = dedent('''\
         <video url_name="SampleProblem1" start_time="0:00:01" youtube="0.75:izygArpw-Qo,1.00:p2Q6BrNhdh8,1.25:1EeWXzPdhSA,1.50:rABDYkeK0x8" show_captions="false" end_time="0:01:00">
           <source src="http://www.example.com/source.mp4"/>
@@ -379,7 +436,7 @@ class VideoExportTestCase(unittest.TestCase):
         </video>
        ''')

-        self.assertEquals(expected, xml)
+        self.assertEquals(expected, etree.tostring(xml, pretty_print=True))

    def test_export_to_xml_empty_parameters(self):
        """Test XML export with defaults."""
@@ -387,7 +444,7 @@ class VideoExportTestCase(unittest.TestCase):
        location = Location(["i4x", "edX", "video", "default", "SampleProblem1"])
        desc = VideoDescriptor(module_system, {'location': location})

-        xml = desc.export_to_xml(None)
+        xml = desc.definition_to_xml(None)
        expected = '<video url_name="SampleProblem1"/>\n'

-        self.assertEquals(expected, xml)
+        self.assertEquals(expected, etree.tostring(xml, pretty_print=True))
--- a/common/lib/xmodule/xmodule/video_module.py
+++ b/common/lib/xmodule/xmodule/video_module.py
@@ -240,7 +240,7 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor
        video = cls(system, model_data)
        return video

-    def export_to_xml(self, resource_fs):
+    def definition_to_xml(self, resource_fs):
        """
        Returns an xml string representing this module.
        """
@@ -266,7 +266,7 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor
            if key in fields and fields[key].default == getattr(self, key):
                continue
            if value:
-                xml.set(key, str(value))
+                xml.set(key, unicode(value))

        for source in self.html5_sources:
            ele = etree.Element('source')
@@ -277,7 +277,7 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor
            ele = etree.Element('track')
            ele.set('src', self.track)
            xml.append(ele)
-        return etree.tostring(xml, pretty_print=True)
+        return xml

    @staticmethod
    def _parse_youtube(data):
@@ -293,11 +293,14 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor
            pieces = video.split(':')
            try:
                speed = '%.2f' % float(pieces[0])  # normalize speed
-                youtube_id = pieces[1]
+
+                # Handle the fact that youtube IDs got double-quoted for a period of time.
+                # Note: we pass in "VideoFields.youtube_id_1_0" so we deserialize as a String--
+                # it doesn't matter what the actual speed is for the purposes of deserializing.
+                youtube_id = VideoDescriptor._deserialize(VideoFields.youtube_id_1_0.name, pieces[1])
                ret[speed] = youtube_id
            except (ValueError, IndexError):
                log.warning('Invalid YouTube ID: %s' % video)
-
        return ret

    @staticmethod
@@ -310,7 +313,6 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor
        model_data = {}

        conversions = {
-            'show_captions': json.loads,
            'start_time': VideoDescriptor._parse_time,
            'end_time': VideoDescriptor._parse_time
        }
@@ -349,10 +351,21 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor
                #  Convert XML attrs into Python values.
                if attr in conversions:
                    value = conversions[attr](value)
+                else:
+                # We export values with json.dumps (well, except for Strings, but
+                # for about a month we did it for Strings also).
+                    value = VideoDescriptor._deserialize(attr, value)
                model_data[attr] = value

        return model_data

+    @classmethod
+    def _deserialize(cls, attr, value):
+        """
+        Handles deserializing values that may have been encoded with json.dumps.
+        """
+        return cls.get_map_for_field(attr).from_xml(value)
+
    @staticmethod
    def _parse_time(str_time):
        """Converts s in '12:34:45' format to seconds. If s is
--- a/common/lib/xmodule/xmodule/x_module.py
+++ b/common/lib/xmodule/xmodule/x_module.py
@@ -173,11 +173,11 @@ class XModule(XModuleFields, HTMLSnippet, XBlock):
            # don't need to set category as it will automatically get from descriptor
        elif isinstance(self.location, Location):
            self.url_name = self.location.name
-            if not hasattr(self, 'category'):
+            if getattr(self, 'category', None) is None:
                self.category = self.location.category
        elif isinstance(self.location, BlockUsageLocator):
            self.url_name = self.location.usage_id
-            if not hasattr(self, 'category'):
+            if getattr(self, 'category', None) is None:
                raise InsufficientSpecificationError()
        else:
            raise InsufficientSpecificationError()
@@ -467,11 +467,11 @@ class XModuleDescriptor(XModuleFields, HTMLSnippet, ResourceTemplates, XBlock):
        self.system = self.runtime
        if isinstance(self.location, Location):
            self.url_name = self.location.name
-            if not hasattr(self, 'category'):
+            if getattr(self, 'category', None) is None:
                self.category = self.location.category
        elif isinstance(self.location, BlockUsageLocator):
            self.url_name = self.location.usage_id
-            if not hasattr(self, 'category'):
+            if getattr(self, 'category', None) is None:
                raise InsufficientSpecificationError()
        else:
            raise InsufficientSpecificationError()
--- a/common/lib/xmodule/xmodule/xml_module.py
+++ b/common/lib/xmodule/xmodule/xml_module.py
@@ -167,6 +167,11 @@ class XmlDescriptor(XModuleDescriptor):

    @classmethod
    def get_map_for_field(cls, attr):
+        """
+        Returns a serialize/deserialize AttrMap for the given field of a class.
+
+        Searches through fields defined by cls to find one named attr.
+        """
        for field in set(cls.fields + cls.lms.fields):
            if field.name == attr:
                from_xml = lambda val: deserialize_field(field, val)