Convert Video Alpha to metadata-only.

CHANGELOG.rst

@@ -42,6 +42,9 @@ Common: Add a manage.py that knows about edx-platform specific settings and proj
 
 Common: Added *experimental* support for jsinput type.
 
+Studio: Remove XML from HTML5 video component editor. All settings are
+moved to be edited as metadata.
+
 Common: Added setting to specify Celery Broker vhost
 
 Common: Utilize new XBlock bulk save API in LMS and CMS.

cms/djangoapps/contentstore/features/common.py

@@ -228,6 +228,26 @@ def i_created_a_video_component(step):
     )
 
 
+@step('I have created a Video Alpha component$')
+def i_created_video_alpha(step):
+    step.given('I have enabled the videoalpha advanced module')
+    world.css_click('a.course-link')
+    step.given('I have added a new subsection')
+    step.given('I expand the first section')
+    world.css_click('a.new-unit-item')
+    world.css_click('.large-advanced-icon')
+    world.click_component_from_menu('videoalpha', None, '.xmodule_VideoAlphaModule')
+
+
+@step('I have enabled the (.*) advanced module$')
+def i_enabled_the_advanced_module(step, module):
+    step.given('I have opened a new course section in Studio')
+    world.css_click('.nav-course-settings')
+    world.css_click('.nav-course-settings-advanced')
+    type_in_codemirror(0, '["%s"]' % module)
+    press_the_notification_button(step, 'Save')
+
+
 @step('I have clicked the new unit button')
 def open_new_unit(step):
     step.given('I have opened a new course section in Studio')

cms/djangoapps/contentstore/features/videoalpha-editor.feature

+Feature: Video Alpha Component Editor
+  As a course author, I want to be able to create videoalpha components.
+
+  Scenario: User can view metadata
+    Given I have created a Video Alpha component
+    And I edit and select Settings
+    Then I see the correct videoalpha settings and default values
+
+  Scenario: User can modify display name
+    Given I have created a Video Alpha component
+    And I edit and select Settings
+    Then I can modify the display name
+    And my display name change is persisted on save
+
+  @skip
+  Scenario: Captions are hidden when "show captions" is false
+    Given I have created a Video component
+    And I have set "show captions" to False
+    Then when I view the video it does not show the captions
+
+  @skip
+  Scenario: Captions are shown when "show captions" is true
+    Given I have created a Video component
+    And I have set "show captions" to True
+    Then when I view the video it does show the captions

cms/djangoapps/contentstore/features/videoalpha-editor.py

+# disable missing docstring
+# pylint: disable=C0111
+
+from lettuce import world, step
+
+
+@step('I see the correct videoalpha settings and default values$')
+def correct_videoalpha_settings(_step):
+    world.verify_all_setting_entries([['Default Speed', '', False],
+                                      ['Display Name', 'Video Alpha', False],
+                                      ['Download Track', '', False],
+                                      ['Download Video', '', False],
+                                      ['HTML5 Subtitles', '', False],
+                                      ['Show Captions', 'True', False],
+                                      ['Speed: .75x', '', False],
+                                      ['Speed: 1.25x', '', False],
+                                      ['Speed: 1.5x', '', False],
+                                      ['Video Sources', '', False]])

cms/djangoapps/contentstore/features/videoalpha.feature

+Feature: Video Alpha Component
+  As a course author, I want to be able to view my created videos in Studio.
+
+  Scenario: Autoplay is disabled in Studio
+    Given I have created a Video Alpha component
+    Then when I view the video alpha it does not have autoplay enabled

cms/djangoapps/contentstore/features/videoalpha.py

+# disable missing docstring
+# pylint: disable=C0111
+
+from lettuce import world, step
+
+
+@step('when I view the video alpha it does not have autoplay enabled')
+def does_not_autoplay(_step):
+    assert world.css_find('.videoalpha')[0]['data-autoplay'] == 'False'
+    assert world.css_has_class('.video_control', 'play')

cms/templates/widgets/tabs-aggregator.html

@@ -9,7 +9,7 @@
         % endfor
         </ul>
       </div>
-      <div class="${'tabs-wrapper' if (len(tabs) != 1) else 'editor-single-tab' }">
+      <div class="tabs-wrapper">
        % for tab in tabs:
         <div class="component-tab ${'is-inactive' if not tab.get('current', False) else ''}" id="tab-${html_id}-${loop.index}" >
             <%include file="${tab['template']}" args="tabName=tab['name']"/>

cms/templates/widgets/tabs/metadata-edit-tab.html

@@ -20,5 +20,9 @@
     <%static:include path="js/metadata-option-entry.underscore" />
 </script>
 
+<script id="metadata-list-entry" type="text/template">
+    <%static:include path="js/metadata-list-entry.underscore" />
+</script>
+
 <div class="wrapper-comp-settings metadata_edit" id="settings-tab" data-metadata='${json.dumps(editable_metadata_fields) | h}'/>
 

common/lib/xmodule/xmodule/tests/test_videoalpha.py

 # -*- coding: utf-8 -*-
+#pylint: disable=W0212
 """Test for Video Alpha Xmodule functional logic.
 These tests data readed from  xml or from mongo.
 
@@ -13,11 +14,15 @@ the course, section, subsection, unit, etc.
 """
 
 import unittest
-from xmodule.videoalpha_module import VideoAlphaDescriptor
 from . import LogicTest
-from lxml import etree
-from pkg_resources import resource_string
 from .import get_test_system
+from xmodule.modulestore import Location
+from xmodule.videoalpha_module import VideoAlphaDescriptor, _create_youtube_string
+from xmodule.video_module import VideoDescriptor
+from .test_import import DummySystem
+
+from textwrap import dedent
+
 
 class VideoAlphaModuleTest(LogicTest):
     """Logic tests for VideoAlpha Xmodule."""
@@ -27,30 +32,62 @@ class VideoAlphaModuleTest(LogicTest):
         'data': '<videoalpha />'
     }
 
-    def test_get_timeframe_no_parameters(self):
-        "Make sure that timeframe() works correctly w/o parameters"
-        xmltree = etree.fromstring('<videoalpha>test</videoalpha>')
-        output = self.xmodule.get_timeframe(xmltree)
-        self.assertEqual(output, ('', ''))
+    def test_parse_time_empty(self):
+        """Ensure parse_time returns correctly with None or empty string."""
+        expected = ''
+        self.assertEqual(VideoAlphaDescriptor._parse_time(None), expected)
+        self.assertEqual(VideoAlphaDescriptor._parse_time(''), expected)
+
+    def test_parse_time(self):
+        """Ensure that times are parsed correctly into seconds."""
+        expected = 247
+        output = VideoAlphaDescriptor._parse_time('00:04:07')
+        self.assertEqual(output, expected)
+
+    def test_parse_youtube(self):
+        """Test parsing old-style Youtube ID strings into a dict."""
+        youtube_str = '0.75:jNCf2gIqpeE,1.00:ZwkTiUPN0mg,1.25:rsq9auxASqI,1.50:kMyNdzVHHgg'
+        output = VideoAlphaDescriptor._parse_youtube(youtube_str)
+        self.assertEqual(output, {'0.75': 'jNCf2gIqpeE',
+                                  '1.00': 'ZwkTiUPN0mg',
+                                  '1.25': 'rsq9auxASqI',
+                                  '1.50': 'kMyNdzVHHgg'})
 
-    def test_get_timeframe_with_one_parameter(self):
-        "Make sure that timeframe() works correctly with one parameter"
-        xmltree = etree.fromstring(
-            '<videoalpha start_time="00:04:07">test</videoalpha>'
+    def test_parse_youtube_one_video(self):
+        """
+        Ensure that all keys are present and missing speeds map to the
+        empty string.
+        """
+        youtube_str = '0.75:jNCf2gIqpeE'
+        output = VideoAlphaDescriptor._parse_youtube(youtube_str)
+        self.assertEqual(output, {'0.75': 'jNCf2gIqpeE',
+                                  '1.00': '',
+                                  '1.25': '',
+                                  '1.50': ''})
+
+    def test_parse_youtube_key_format(self):
+        """
+        Make sure that inconsistent speed keys are parsed correctly.
+        """
+        youtube_str = '1.00:p2Q6BrNhdh8'
+        youtube_str_hack = '1.0:p2Q6BrNhdh8'
+        self.assertEqual(
+            VideoAlphaDescriptor._parse_youtube(youtube_str),
+            VideoAlphaDescriptor._parse_youtube(youtube_str_hack)
         )
-        output = self.xmodule.get_timeframe(xmltree)
-        self.assertEqual(output, (247, ''))
-
-    def test_get_timeframe_with_two_parameters(self):
-        "Make sure that timeframe() works correctly with two parameters"
-        xmltree = etree.fromstring(
-            '''<videoalpha
-                    start_time="00:04:07"
-                    end_time="13:04:39"
-                >test</videoalpha>'''
+
+    def test_parse_youtube_empty(self):
+        """
+        Some courses have empty youtube attributes, so we should handle
+        that well.
+        """
+        self.assertEqual(
+            VideoAlphaDescriptor._parse_youtube(''),
+            {'0.75': '',
+             '1.00': '',
+             '1.25': '',
+             '1.50': ''}
         )
-        output = self.xmodule.get_timeframe(xmltree)
-        self.assertEqual(output, (247, 47079))
 
 
 class VideoAlphaDescriptorTest(unittest.TestCase):
@@ -66,16 +103,247 @@ class VideoAlphaDescriptorTest(unittest.TestCase):
         """"test get_context"""
         correct_tabs = [
             {
-                'name': "XML",
-                'template': "videoalpha/codemirror-edit.html",
-                'css': {'scss': [resource_string(__name__,
-                        '../css/tabs/codemirror.scss')]},
-                'current': True,
-            },
-            {
                 'name': "Settings",
-                'template': "tabs/metadata-edit-tab.html"
+                'template': "tabs/metadata-edit-tab.html",
+                'current': True
             }
         ]
         rendered_context = self.descriptor.get_context()
         self.assertListEqual(rendered_context['tabs'], correct_tabs)
+
+    def test_create_youtube_string(self):
+        """
+        Test that Youtube ID strings are correctly created when writing
+        back out to XML.
+        """
+        system = DummySystem(load_error_modules=True)
+        location = Location(["i4x", "edX", "videoalpha", "default", "SampleProblem1"])
+        model_data = {'location': location}
+        descriptor = VideoAlphaDescriptor(system, model_data)
+        descriptor.youtube_id_0_75 = 'izygArpw-Qo'
+        descriptor.youtube_id_1_0 = 'p2Q6BrNhdh8'
+        descriptor.youtube_id_1_25 = '1EeWXzPdhSA'
+        descriptor.youtube_id_1_5 = 'rABDYkeK0x8'
+        expected = "0.75:izygArpw-Qo,1.00:p2Q6BrNhdh8,1.25:1EeWXzPdhSA,1.50:rABDYkeK0x8"
+        self.assertEqual(_create_youtube_string(descriptor), expected)
+
+    def test_create_youtube_string_missing(self):
+        """
+        Test that Youtube IDs which aren't explicitly set aren't included
+        in the output string.
+        """
+        system = DummySystem(load_error_modules=True)
+        location = Location(["i4x", "edX", "videoalpha", "default", "SampleProblem1"])
+        model_data = {'location': location}
+        descriptor = VideoAlphaDescriptor(system, model_data)
+        descriptor.youtube_id_0_75 = 'izygArpw-Qo'
+        descriptor.youtube_id_1_0 = 'p2Q6BrNhdh8'
+        descriptor.youtube_id_1_25 = '1EeWXzPdhSA'
+        expected = "0.75:izygArpw-Qo,1.00:p2Q6BrNhdh8,1.25:1EeWXzPdhSA"
+        self.assertEqual(_create_youtube_string(descriptor), expected)
+
+
+class VideoAlphaDescriptorImportTestCase(unittest.TestCase):
+    """
+    Make sure that VideoAlphaDescriptor can import an old XML-based video correctly.
+    """
+
+    def test_constructor(self):
+        sample_xml = '''
+            <videoalpha display_name="Test Video"
+                   youtube="1.0:p2Q6BrNhdh8,0.75:izygArpw-Qo,1.25:1EeWXzPdhSA,1.5:rABDYkeK0x8"
+                   show_captions="false"
+                   start_time="00:00:01"
+                   end_time="00:01:00">
+              <source src="http://www.example.com/source.mp4"/>
+              <source src="http://www.example.com/source.ogg"/>
+              <track src="http://www.example.com/track"/>
+            </videoalpha>
+        '''
+        location = Location(["i4x", "edX", "videoalpha", "default",
+                             "SampleProblem1"])
+        model_data = {'data': sample_xml,
+                      'location': location}
+        system = DummySystem(load_error_modules=True)
+        descriptor = VideoAlphaDescriptor(system, model_data)
+        self.assertEquals(descriptor.youtube_id_0_75, 'izygArpw-Qo')
+        self.assertEquals(descriptor.youtube_id_1_0, 'p2Q6BrNhdh8')
+        self.assertEquals(descriptor.youtube_id_1_25, '1EeWXzPdhSA')
+        self.assertEquals(descriptor.youtube_id_1_5, 'rABDYkeK0x8')
+        self.assertEquals(descriptor.show_captions, False)
+        self.assertEquals(descriptor.start_time, 1.0)
+        self.assertEquals(descriptor.end_time, 60)
+        self.assertEquals(descriptor.track, 'http://www.example.com/track')
+        self.assertEquals(descriptor.source, 'http://www.example.com/source.mp4')
+        self.assertEquals(descriptor.html5_sources, ['http://www.example.com/source.mp4', 'http://www.example.com/source.ogg'])
+        self.assertEquals(descriptor.data, '')
+
+    def test_from_xml(self):
+        module_system = DummySystem(load_error_modules=True)
+        xml_data = '''
+            <videoalpha display_name="Test Video"
+                   youtube="1.0:p2Q6BrNhdh8,0.75:izygArpw-Qo,1.25:1EeWXzPdhSA,1.5:rABDYkeK0x8"
+                   show_captions="false"
+                   start_time="00:00:01"
+                   end_time="00:01:00">
+              <source src="http://www.example.com/source.mp4"/>
+              <track src="http://www.example.com/track"/>
+            </videoalpha>
+        '''
+        output = VideoAlphaDescriptor.from_xml(xml_data, module_system)
+        self.assertEquals(output.youtube_id_0_75, 'izygArpw-Qo')
+        self.assertEquals(output.youtube_id_1_0, 'p2Q6BrNhdh8')
+        self.assertEquals(output.youtube_id_1_25, '1EeWXzPdhSA')
+        self.assertEquals(output.youtube_id_1_5, 'rABDYkeK0x8')
+        self.assertEquals(output.show_captions, False)
+        self.assertEquals(output.start_time, 1.0)
+        self.assertEquals(output.end_time, 60)
+        self.assertEquals(output.track, 'http://www.example.com/track')
+        self.assertEquals(output.source, 'http://www.example.com/source.mp4')
+        self.assertEquals(output.html5_sources, ['http://www.example.com/source.mp4'])
+        self.assertEquals(output.data, '')
+
+    def test_from_xml_missing_attributes(self):
+        """
+        Ensure that attributes have the right values if they aren't
+        explicitly set in XML.
+        """
+        module_system = DummySystem(load_error_modules=True)
+        xml_data = '''
+            <videoalpha display_name="Test Video"
+                   youtube="1.0:p2Q6BrNhdh8,1.25:1EeWXzPdhSA"
+                   show_captions="true">
+              <source src="http://www.example.com/source.mp4"/>
+              <track src="http://www.example.com/track"/>
+            </videoalpha>
+        '''
+        output = VideoAlphaDescriptor.from_xml(xml_data, module_system)
+        self.assertEquals(output.youtube_id_0_75, '')
+        self.assertEquals(output.youtube_id_1_0, 'p2Q6BrNhdh8')
+        self.assertEquals(output.youtube_id_1_25, '1EeWXzPdhSA')
+        self.assertEquals(output.youtube_id_1_5, '')
+        self.assertEquals(output.show_captions, True)
+        self.assertEquals(output.start_time, 0.0)
+        self.assertEquals(output.end_time, 0.0)
+        self.assertEquals(output.track, 'http://www.example.com/track')
+        self.assertEquals(output.source, 'http://www.example.com/source.mp4')
+        self.assertEquals(output.html5_sources, ['http://www.example.com/source.mp4'])
+        self.assertEquals(output.data, '')
+
+    def test_from_xml_no_attributes(self):
+        """
+        Make sure settings are correct if none are explicitly set in XML.
+        """
+        module_system = DummySystem(load_error_modules=True)
+        xml_data = '<videoalpha></videoalpha>'
+        output = VideoAlphaDescriptor.from_xml(xml_data, module_system)
+        self.assertEquals(output.youtube_id_0_75, '')
+        self.assertEquals(output.youtube_id_1_0, '')
+        self.assertEquals(output.youtube_id_1_25, '')
+        self.assertEquals(output.youtube_id_1_5, '')
+        self.assertEquals(output.show_captions, True)
+        self.assertEquals(output.start_time, 0.0)
+        self.assertEquals(output.end_time, 0.0)
+        self.assertEquals(output.track, '')
+        self.assertEquals(output.source, '')
+        self.assertEquals(output.html5_sources, [])
+        self.assertEquals(output.data, '')
+
+    def test_old_video_format(self):
+        """
+        Test backwards compatibility with VideoModule's XML format.
+        """
+        module_system = DummySystem(load_error_modules=True)
+        xml_data = """
+            <videoalpha display_name="Test Video"
+                   youtube="1.0:p2Q6BrNhdh8,0.75:izygArpw-Qo,1.25:1EeWXzPdhSA,1.5:rABDYkeK0x8"
+                   show_captions="false"
+                   from="00:00:01"
+                   to="00:01:00">
+              <source src="http://www.example.com/source.mp4"/>
+              <track src="http://www.example.com/track"/>
+            </videoalpha>
+        """
+        output = VideoAlphaDescriptor.from_xml(xml_data, module_system)
+        self.assertEquals(output.youtube_id_0_75, 'izygArpw-Qo')
+        self.assertEquals(output.youtube_id_1_0, 'p2Q6BrNhdh8')
+        self.assertEquals(output.youtube_id_1_25, '1EeWXzPdhSA')
+        self.assertEquals(output.youtube_id_1_5, 'rABDYkeK0x8')
+        self.assertEquals(output.show_captions, False)
+        self.assertEquals(output.start_time, 1.0)
+        self.assertEquals(output.end_time, 60)
+        self.assertEquals(output.track, 'http://www.example.com/track')
+        self.assertEquals(output.source, 'http://www.example.com/source.mp4')
+        self.assertEquals(output.html5_sources, ['http://www.example.com/source.mp4'])
+        self.assertEquals(output.data, '')
+
+    def test_old_video_data(self):
+        module_system = DummySystem(load_error_modules=True)
+        xml_data = """
+            <video display_name="Test Video"
+                   youtube="1.0:p2Q6BrNhdh8,0.75:izygArpw-Qo,1.25:1EeWXzPdhSA,1.5:rABDYkeK0x8"
+                   show_captions="false"
+                   from="00:00:01"
+                   to="00:01:00">
+              <source src="http://www.example.com/source.mp4"/>
+              <track src="http://www.example.com/track"/>
+            </video>
+        """
+        video = VideoDescriptor.from_xml(xml_data, module_system)
+        video_alpha = VideoAlphaDescriptor(module_system, video._model_data)
+        self.assertEquals(video_alpha.youtube_id_0_75, 'izygArpw-Qo')
+        self.assertEquals(video_alpha.youtube_id_1_0, 'p2Q6BrNhdh8')
+        self.assertEquals(video_alpha.youtube_id_1_25, '1EeWXzPdhSA')
+        self.assertEquals(video_alpha.youtube_id_1_5, 'rABDYkeK0x8')
+        self.assertEquals(video_alpha.show_captions, False)
+        self.assertEquals(video_alpha.start_time, 1.0)
+        self.assertEquals(video_alpha.end_time, 60)
+        self.assertEquals(video_alpha.track, 'http://www.example.com/track')
+        self.assertEquals(video_alpha.source, 'http://www.example.com/source.mp4')
+        self.assertEquals(video_alpha.html5_sources, ['http://www.example.com/source.mp4'])
+        self.assertEquals(video_alpha.data, '')
+
+
+class VideoAlphaExportTestCase(unittest.TestCase):
+    """
+    Make sure that VideoAlphaDescriptor can export itself to XML
+    correctly.
+    """
+
+    def test_export_to_xml(self):
+        """Test that we write the correct XML on export."""
+        module_system = DummySystem(load_error_modules=True)
+        location = Location(["i4x", "edX", "videoalpha", "default", "SampleProblem1"])
+        desc = VideoAlphaDescriptor(module_system, {'location': location})
+
+        desc.youtube_id_0_75 = 'izygArpw-Qo'
+        desc.youtube_id_1_0 = 'p2Q6BrNhdh8'
+        desc.youtube_id_1_25 = '1EeWXzPdhSA'
+        desc.youtube_id_1_5 = 'rABDYkeK0x8'
+        desc.show_captions = False
+        desc.start_time = 1.0
+        desc.end_time = 60
+        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
+        expected = dedent('''\
+         <videoalpha display_name="Video Alpha" 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"/>
+           <source src="http://www.example.com/source.ogg"/>
+           <track src="http://www.example.com/track"/>
+         </videoalpha>
+        ''')
+
+        self.assertEquals(expected, xml)
+
+    def test_export_to_xml_empty_parameters(self):
+        """Test XML export with defaults."""
+        module_system = DummySystem(load_error_modules=True)
+        location = Location(["i4x", "edX", "videoalpha", "default", "SampleProblem1"])
+        desc = VideoAlphaDescriptor(module_system, {'location': location})
+
+        xml = desc.export_to_xml(None)
+        expected = '<videoalpha display_name="Video Alpha" show_captions="true"/>\n'
+
+        self.assertEquals(expected, xml)

common/lib/xmodule/xmodule/videoalpha_module.py

@@ -14,7 +14,7 @@ import json
 import logging
 
 from lxml import etree
-from pkg_resources import resource_string, resource_listdir
+from pkg_resources import resource_string
 
 from django.http import Http404
 from django.conf import settings
@@ -25,31 +25,93 @@ from xmodule.raw_module import RawDescriptor
 from xmodule.modulestore.mongo import MongoModuleStore
 from xmodule.modulestore.django import modulestore
 from xmodule.contentstore.content import StaticContent
-from xblock.core import Integer, Scope, String
+from xblock.core import Scope, String, Boolean, Float, List, Integer
 
 import datetime
 import time
-import textwrap
 
 log = logging.getLogger(__name__)
 
 
 class VideoAlphaFields(object):
     """Fields for `VideoAlphaModule` and `VideoAlphaDescriptor`."""
-    data = String(help="XML data for the problem",
-                  default=textwrap.dedent('''\
-                       <videoalpha show_captions="true" sub="name_of_file" youtube="0.75:JMD_ifUUfsU,1.0:OEoXaMPEzfM,1.25:AKqURZnYqpk,1.50:DYpADpL7jAY" >
-                           <source src="https://s3.amazonaws.com/edx-course-videos/edx-intro/edX-FA12-cware-1_100.mp4"/>
-                           <source src="https://s3.amazonaws.com/edx-course-videos/edx-intro/edX-FA12-cware-1_100.webm"/>
-                           <source src="https://s3.amazonaws.com/edx-course-videos/edx-intro/edX-FA12-cware-1_100.ogv"/>
-                       </videoalpha>'''),
-                  scope=Scope.content)
-    position = Integer(help="Current position in the video", scope=Scope.user_state, default=0)
     display_name = String(
         display_name="Display Name", help="Display name for this module",
         default="Video Alpha",
         scope=Scope.settings
     )
+    position = Integer(
+        help="Current position in the video",
+        scope=Scope.user_state,
+        default=0
+    )
+    show_captions = Boolean(
+        help="This controls whether or not captions are shown by default.",
+        display_name="Show Captions",
+        scope=Scope.settings,
+        default=True
+    )
+    #  TODO (pfogg): Do we want to show these to the user if HTML5 sources are preferred?
+    youtube_id_1_0 = String(
+        help="This is the Youtube ID reference for the normal speed video.",
+        display_name="Default Speed",
+        scope=Scope.settings,
+        default=""
+    )
+    youtube_id_0_75 = String(
+        help="The Youtube ID for the .75x speed video.",
+        display_name="Speed: .75x",
+        scope=Scope.settings,
+        default=""
+    )
+    youtube_id_1_25 = String(
+        help="The Youtube ID for the 1.25x speed video.",
+        display_name="Speed: 1.25x",
+        scope=Scope.settings,
+        default=""
+    )
+    youtube_id_1_5 = String(
+        help="The Youtube ID for the 1.5x speed video.",
+        display_name="Speed: 1.5x",
+        scope=Scope.settings,
+        default=""
+    )
+    start_time = Float(
+        help="Time the video starts",
+        display_name="Start Time",
+        scope=Scope.settings,
+        default=0.0
+    )
+    end_time = Float(
+        help="Time the video ends",
+        display_name="End Time",
+        scope=Scope.settings,
+        default=0.0
+    )
+    source = String(
+        help="The external URL to download the video. This appears as a link beneath the video.",
+        display_name="Download Video",
+        scope=Scope.settings,
+        default=""
+    )
+    html5_sources = List(
+        help="A comma-separated list of filenames to be used with HTML5 video.",
+        display_name="Video Sources",
+        scope=Scope.settings,
+        default=[]
+    )
+    track = String(
+        help="The external URL to download the subtitle track. This appears as a link beneath the video.",
+        display_name="Download Track",
+        scope=Scope.settings,
+        default=""
+    )
+    sub = String(
+        help="The name of the subtitle track (for non-Youtube videos).",
+        display_name="HTML5 Subtitles",
+        scope=Scope.settings,
+        default=""
+    )
 
 
 class VideoAlphaModule(VideoAlphaFields, XModule):
@@ -85,72 +147,6 @@ class VideoAlphaModule(VideoAlphaFields, XModule):
     css = {'scss': [resource_string(__name__, 'css/videoalpha/display.scss')]}
     js_module_name = "VideoAlpha"
 
-    def __init__(self, *args, **kwargs):
-        XModule.__init__(self, *args, **kwargs)
-        xmltree = etree.fromstring(self.data)
-
-        # Front-end expects an empty string, or a properly formatted string with YouTube IDs.
-        self.youtube_streams = xmltree.get('youtube', '')
-
-        self.sub = xmltree.get('sub')
-
-        self.autoplay = xmltree.get('autoplay') or ''
-        if self.autoplay.lower() not in ['true', 'false']:
-            self.autoplay = 'true'
-
-        self.position = 0
-        self.show_captions = xmltree.get('show_captions', 'true')
-        self.sources = {
-            'main': self._get_source(xmltree),
-            'mp4': self._get_source(xmltree, ['mp4']),
-            'webm': self._get_source(xmltree, ['webm']),
-            'ogv': self._get_source(xmltree, ['ogv']),
-        }
-        self.track = self._get_track(xmltree)
-        self.start_time, self.end_time = self.get_timeframe(xmltree)
-
-    def _get_source(self, xmltree, exts=None):
-        """Find the first valid source, which ends with one of `exts`."""
-        exts = ['mp4', 'ogv', 'avi', 'webm'] if exts is None else exts
-        condition = lambda src: any([src.endswith(ext) for ext in exts])
-        return self._get_first_external(xmltree, 'source', condition)
-
-    def _get_track(self, xmltree):
-        """Find the first valid track."""
-        return self._get_first_external(xmltree, 'track')
-
-    def _get_first_external(self, xmltree, tag, condition=bool):
-        """Will return the first 'valid' element of the given tag.
-        'valid' means that `condition('src' attribute) == True`
-        """
-        result = None
-
-        for element in xmltree.findall(tag):
-            src = element.get('src')
-            if condition(src):
-                result = src
-                break
-        return result
-
-    def get_timeframe(self, xmltree):
-        """ Converts 'start_time' and 'end_time' parameters in video tag to seconds.
-        If there are no parameters, returns empty string. """
-
-        def parse_time(str_time):
-            """Converts s in '12:34:45' format to seconds. If s is
-            None, returns empty string"""
-            if str_time is None:
-                return ''
-            else:
-                obj_time = time.strptime(str_time, '%H:%M:%S')
-                return datetime.timedelta(
-                    hours=obj_time.tm_hour,
-                    minutes=obj_time.tm_min,
-                    seconds=obj_time.tm_sec
-                ).total_seconds()
-
-        return parse_time(xmltree.get('start_time')), parse_time(xmltree.get('end_time'))
-
     def handle_ajax(self, dispatch, data):
         """This is not being called right now and we raise 404 error."""
         log.debug(u"GET {0}".format(data))
@@ -169,12 +165,15 @@ class VideoAlphaModule(VideoAlphaFields, XModule):
             # cdodge: filesystem static content support.
             caption_asset_path = "/static/subs/"
 
+        get_ext = lambda filename: filename.rpartition('.')[-1]
+        sources = {get_ext(src): src for src in self.html5_sources}
+        sources['main'] = self.source
+
         return self.system.render_template('videoalpha.html', {
-            'youtube_streams': self.youtube_streams,
+            'youtube_streams': _create_youtube_string(self),
             'id': self.location.html_id(),
             'sub': self.sub,
-            'autoplay': self.autoplay,
-            'sources': self.sources,
+            'sources': sources,
             'track': self.track,
             'display_name': self.display_name_with_default,
             # This won't work when we move to data that
@@ -193,18 +192,188 @@ class VideoAlphaDescriptor(VideoAlphaFields, TabsEditingDescriptor, RawDescripto
     module_class = VideoAlphaModule
 
     tabs = [
-        {
-            'name': "XML",
-            'template': "videoalpha/codemirror-edit.html",
-            'css': {'scss': [resource_string(__name__, 'css/tabs/codemirror.scss')]},
-            'current': True,
-        },
         # {
         #     'name': "Subtitles",
         #     'template': "videoalpha/subtitles.html",
         # },
         {
             'name': "Settings",
-            'template': "tabs/metadata-edit-tab.html"
+            'template': "tabs/metadata-edit-tab.html",
+            'current': True
+        }
+    ]
+
+    def __init__(self, *args, **kwargs):
+        super(VideoAlphaDescriptor, self).__init__(*args, **kwargs)
+        if self.data:
+            model_data = VideoAlphaDescriptor._parse_video_xml(self.data)
+            self._model_data.update(model_data)
+            del self.data
+
+    @property
+    def non_editable_metadata_fields(self):
+        non_editable_fields = super(TabsEditingDescriptor, self).non_editable_metadata_fields
+        non_editable_fields.extend([VideoAlphaFields.start_time,
+                                    VideoAlphaFields.end_time])
+        return non_editable_fields
+
+    @classmethod
+    def from_xml(cls, xml_data, system, org=None, course=None):
+        """
+        Creates an instance of this descriptor from the supplied xml_data.
+        This may be overridden by subclasses
+
+        xml_data: A string of xml that will be translated into data and children for
+            this module
+        system: A DescriptorSystem for interacting with external resources
+        org and course are optional strings that will be used in the generated modules
+            url identifiers
+        """
+        model_data = VideoAlphaDescriptor._parse_video_xml(xml_data)
+        video = cls(system, model_data)
+        return video
+
+    def export_to_xml(self, resource_fs):
+        """
+        Returns an xml string representing this module, and all modules
+        underneath it.  May also write required resources out to resource_fs
+
+        Assumes that modules have single parentage (that no module appears twice
+        in the same course), and that it is thus safe to nest modules as xml
+        children as appropriate.
+
+        The returned XML should be able to be parsed back into an identical
+        XModuleDescriptor using the from_xml method with the same system, org,
+        and course
+        """
+        xml = etree.Element('videoalpha')
+        attrs = {
+            'display_name': self.display_name,
+            'show_captions': json.dumps(self.show_captions),
+            'youtube': _create_youtube_string(self),
+            'start_time': datetime.timedelta(seconds=self.start_time),
+            'end_time': datetime.timedelta(seconds=self.end_time),
+            'sub': self.sub
         }
+        for key, value in attrs.items():
+            if value:
+                xml.set(key, str(value))
+
+        for source in self.html5_sources:
+            ele = etree.Element('source')
+            ele.set('src', source)
+            xml.append(ele)
+
+        if self.track:
+            ele = etree.Element('track')
+            ele.set('src', self.track)
+            xml.append(ele)
+
+        return etree.tostring(xml, pretty_print=True)
+
+    @staticmethod
+    def _parse_youtube(data):
+        """
+        Parses a string of Youtube IDs such as "1.0:AXdE34_U,1.5:VO3SxfeD"
+        into a dictionary. Necessary for backwards compatibility with
+        XML-based courses.
+        """
+        ret = {'0.75': '', '1.00': '', '1.25': '', '1.50': ''}
+        if data == '':
+            return ret
+        videos = data.split(',')
+        for video in videos:
+            pieces = video.split(':')
+            # HACK
+            # To elaborate somewhat: in many LMS tests, the keys for
+            # Youtube IDs are inconsistent. Sometimes a particular
+            # speed isn't present, and formatting is also inconsistent
+            # ('1.0' versus '1.00'). So it's necessary to either do
+            # something like this or update all the tests to work
+            # properly.
+            ret['%.2f' % float(pieces[0])] = pieces[1]
+        return ret
+
+    @staticmethod
+    def _parse_video_xml(xml_data):
+        """
+        Parse video fields out of xml_data. The fields are set if they are
+        present in the XML.
+        """
+        xml = etree.fromstring(xml_data)
+        model_data = {}
+
+        conversions = {
+            'show_captions': json.loads,
+            'start_time': VideoAlphaDescriptor._parse_time,
+            'end_time': VideoAlphaDescriptor._parse_time
+        }
+
+        # VideoModule and VideoAlphaModule use different names for
+        # these attributes -- need to convert between them
+        video_compat = {
+            'from': 'start_time',
+            'to': 'end_time'
+        }
+
+        for attr, value in xml.items():
+            if attr in video_compat:
+                attr = video_compat[attr]
+            if attr == 'youtube':
+                speeds = VideoAlphaDescriptor._parse_youtube(value)
+                for speed, youtube_id in speeds.items():
+                    # should have made these youtube_id_1_00 for
+                    # cleanliness, but hindsight doesn't need glasses
+                    normalized_speed = speed[:-1] if speed.endswith('0') else speed
+                    if youtube_id != '':
+                        model_data['youtube_id_{0}'.format(normalized_speed.replace('.', '_'))] = youtube_id
+            else:
+                #  Convert XML attrs into Python values.
+                if attr in conversions:
+                    value = conversions[attr](value)
+                model_data[attr] = value
+
+        sources = xml.findall('source')
+        if sources:
+            model_data['html5_sources'] = [ele.get('src') for ele in sources]
+            model_data['source'] = model_data['html5_sources'][0]
+
+        track = xml.find('track')
+        if track is not None:
+            model_data['track'] = track.get('src')
+
+        return model_data
+
+    @staticmethod
+    def _parse_time(str_time):
+        """Converts s in '12:34:45' format to seconds. If s is
+        None, returns empty string"""
+        if str_time is None or str_time == '':
+            return ''
+        else:
+            obj_time = time.strptime(str_time, '%H:%M:%S')
+            return datetime.timedelta(
+                hours=obj_time.tm_hour,
+                minutes=obj_time.tm_min,
+                seconds=obj_time.tm_sec
+            ).total_seconds()
+
+
+def _create_youtube_string(module):
+    """
+    Create a string of Youtube IDs from `module`'s metadata
+    attributes. Only writes a speed if an ID is present in the
+    module.  Necessary for backwards compatibility with XML-based
+    courses.
+    """
+    youtube_ids = [
+        module.youtube_id_0_75,
+        module.youtube_id_1_0,
+        module.youtube_id_1_25,
+        module.youtube_id_1_5
     ]
+    youtube_speeds = ['0.75', '1.00', '1.25', '1.50']
+    return ','.join([':'.join(pair)
+                     for pair
+                     in zip(youtube_speeds, youtube_ids)
+                     if pair[1]])

lms/djangoapps/courseware/tests/__init__.py

@@ -81,12 +81,16 @@ class BaseTestXmodule(ModuleStoreTestCase):
         # Allow us to assert that the template was called in the same way from
         # different code paths while maintaining the type returned by render_template
         self.runtime.render_template = lambda template, context: u'{!r}, {!r}'.format(template, sorted(context.items()))
+
         model_data = {'location': self.item_descriptor.location}
         model_data.update(self.MODEL_DATA)
 
         self.item_module = self.item_descriptor.module_class(
-            self.runtime, self.item_descriptor, model_data
+            self.runtime,
+            self.item_descriptor,
+            model_data
         )
+
         self.item_url = Location(self.item_module.location).url()
 
         # login all users for acces to Xmodule

lms/djangoapps/courseware/tests/test_videoalpha_mongo.py

@@ -4,6 +4,7 @@
 from . import BaseTestXmodule
 from .test_videoalpha_xml import SOURCE_XML
 from django.conf import settings
+from xmodule.videoalpha_module import _create_youtube_string
 
 
 class TestVideo(BaseTestXmodule):
@@ -15,6 +16,14 @@ class TestVideo(BaseTestXmodule):
         'data': DATA
     }
 
+    def setUp(self):
+        # Since the VideoAlphaDescriptor changes `self._model_data`,
+        # we need to instantiate `self.item_module` through
+        # `self.item_descriptor` rather than directly constructing it
+        super(TestVideo, self).setUp()
+        self.item_module = self.item_descriptor.xmodule(self.runtime)
+        self.item_module.runtime.render_template = lambda template, context: context
+
     def test_handle_ajax_dispatch(self):
         responses = {
             user.username: self.clients[user.username].post(
@@ -34,22 +43,31 @@ class TestVideo(BaseTestXmodule):
     def test_videoalpha_constructor(self):
         """Make sure that all parameters extracted correclty from xml"""
 
-        fragment = self.runtime.render(self.item_module, None, 'student_view')
+        context = self.item_module.get_html()
+
+        sources = {
+            'main': '.../mit-3091x/M-3091X-FA12-L21-3_100.mp4',
+            'mp4': '.../mit-3091x/M-3091X-FA12-L21-3_100.mp4',
+            'webm': '.../mit-3091x/M-3091X-FA12-L21-3_100.webm',
+            'ogv': '.../mit-3091x/M-3091X-FA12-L21-3_100.ogv'
+        }
+
         expected_context = {
             'data_dir': getattr(self, 'data_dir', None),
             'caption_asset_path': '/c4x/MITx/999/asset/subs_',
-            'show_captions': self.item_module.show_captions,
-            'display_name': self.item_module.display_name_with_default,
-            'end': self.item_module.end_time,
+            'show_captions': True,
+            'display_name': 'A Name',
+            'end': 3610.0,
             'id': self.item_module.location.html_id(),
-            'sources': self.item_module.sources,
-            'start': self.item_module.start_time,
-            'sub': self.item_module.sub,
-            'track': self.item_module.track,
-            'youtube_streams': self.item_module.youtube_streams,
+            'sources': sources,
+            'start': 3603.0,
+            'sub': 'a_sub_file.srt.sjson',
+            'track': '',
+            'youtube_streams': _create_youtube_string(self.item_module),
             'autoplay': settings.MITX_FEATURES.get('AUTOPLAY_VIDEOS', True)
         }
-        self.assertEqual(fragment.content, self.runtime.render_template('videoalpha.html', expected_context))
+
+        self.assertEqual(context, expected_context)
 
 
 class TestVideoNonYouTube(TestVideo):
@@ -57,9 +75,8 @@ class TestVideoNonYouTube(TestVideo):
 
     DATA = """
         <videoalpha show_captions="true"
-        data_dir=""
-        caption_asset_path=""
-        autoplay="true"
+        display_name="A Name"
+        sub="a_sub_file.srt.sjson"
         start_time="01:00:03" end_time="01:00:10"
         >
             <source src=".../mit-3091x/M-3091X-FA12-L21-3_100.mp4"/>
@@ -75,20 +92,28 @@ class TestVideoNonYouTube(TestVideo):
         """Make sure that if the 'youtube' attribute is omitted in XML, then
             the template generates an empty string for the YouTube streams.
         """
+        sources = {
+            u'main': u'.../mit-3091x/M-3091X-FA12-L21-3_100.mp4',
+            u'mp4': u'.../mit-3091x/M-3091X-FA12-L21-3_100.mp4',
+            u'webm': u'.../mit-3091x/M-3091X-FA12-L21-3_100.webm',
+            u'ogv': u'.../mit-3091x/M-3091X-FA12-L21-3_100.ogv'
+        }
+
+        context = self.item_module.get_html()
 
-        fragment = self.runtime.render(self.item_module, None, 'student_view')
         expected_context = {
             'data_dir': getattr(self, 'data_dir', None),
             'caption_asset_path': '/c4x/MITx/999/asset/subs_',
-            'show_captions': self.item_module.show_captions,
-            'display_name': self.item_module.display_name_with_default,
-            'end': self.item_module.end_time,
+            'show_captions': True,
+            'display_name': 'A Name',
+            'end': 3610.0,
             'id': self.item_module.location.html_id(),
-            'sources': self.item_module.sources,
-            'start': self.item_module.start_time,
-            'sub': self.item_module.sub,
-            'track': self.item_module.track,
+            'sources': sources,
+            'start': 3603.0,
+            'sub': 'a_sub_file.srt.sjson',
+            'track': '',
             'youtube_streams': '',
             'autoplay': settings.MITX_FEATURES.get('AUTOPLAY_VIDEOS', True)
         }
-        self.assertEqual(fragment.content, self.runtime.render_template('videoalpha.html', expected_context))
+
+        self.assertEqual(context, expected_context)

lms/djangoapps/courseware/tests/test_videoalpha_xml.py

@@ -15,23 +15,19 @@ course, section, subsection, unit, etc.
 
 import json
 import unittest
-from mock import Mock
-from lxml import etree
 
 from django.conf import settings
 
-from xmodule.videoalpha_module import VideoAlphaDescriptor, VideoAlphaModule
+from xmodule.videoalpha_module import VideoAlphaDescriptor, _create_youtube_string
 from xmodule.modulestore import Location
 from xmodule.tests import get_test_system
-from xmodule.tests import LogicTest
 
 
 SOURCE_XML = """
     <videoalpha show_captions="true"
+    display_name="A Name"
     youtube="0.75:jNCf2gIqpeE,1.0:ZwkTiUPN0mg,1.25:rsq9auxASqI,1.50:kMyNdzVHHgg"
-    data_dir=""
-    caption_asset_path=""
-    autoplay="true"
+    sub="a_sub_file.srt.sjson"
     start_time="01:00:03" end_time="01:00:10"
     >
         <source src=".../mit-3091x/M-3091X-FA12-L21-3_100.mp4"/>
@@ -54,74 +50,53 @@ class VideoAlphaFactory(object):
         """Method return VideoAlpha Xmodule instance."""
         location = Location(["i4x", "edX", "videoalpha", "default",
                              "SampleProblem1"])
-        model_data = {'data': VideoAlphaFactory.sample_problem_xml_youtube}
-
-        descriptor = Mock(weight="1")
+        model_data = {'data': VideoAlphaFactory.sample_problem_xml_youtube,
+                      'location': location}
 
         system = get_test_system()
         system.render_template = lambda template, context: context
-        VideoAlphaModule.location = location
-        module = VideoAlphaModule(system, descriptor, model_data)
-
-        return module
-
-
-class VideoAlphaModuleTest(LogicTest):
-    """Tests for logic of VideoAlpha Xmodule."""
-
-    descriptor_class = VideoAlphaDescriptor
-
-    raw_model_data = {
-        'data': '<videoalpha />'
-    }
 
-    def test_get_timeframe_no_parameters(self):
-        xmltree = etree.fromstring('<videoalpha>test</videoalpha>')
-        output = self.xmodule.get_timeframe(xmltree)
-        self.assertEqual(output, ('', ''))
+        descriptor = VideoAlphaDescriptor(system, model_data)
 
-    def test_get_timeframe_with_one_parameter(self):
-        xmltree = etree.fromstring(
-            '<videoalpha start_time="00:04:07">test</videoalpha>'
-        )
-        output = self.xmodule.get_timeframe(xmltree)
-        self.assertEqual(output, (247, ''))
+        module = descriptor.xmodule(system)
 
-    def test_get_timeframe_with_two_parameters(self):
-        xmltree = etree.fromstring(
-            '''<videoalpha
-                    start_time="00:04:07"
-                    end_time="13:04:39"
-                >test</videoalpha>'''
-        )
-        output = self.xmodule.get_timeframe(xmltree)
-        self.assertEqual(output, (247, 47079))
+        return module
 
 
 class VideoAlphaModuleUnitTest(unittest.TestCase):
     """Unit tests for VideoAlpha Xmodule."""
 
-    def test_videoalpha_constructor(self):
+    def test_videoalpha_get_html(self):
         """Make sure that all parameters extracted correclty from xml"""
         module = VideoAlphaFactory.create()
-        module.runtime.render_template = lambda template, context: u'{!r}, {!r}'.format(template, sorted(context.items()))
+        module.runtime.render_template = lambda template, context: context
+
+        sources = {
+            'main': '.../mit-3091x/M-3091X-FA12-L21-3_100.mp4',
+            'mp4': '.../mit-3091x/M-3091X-FA12-L21-3_100.mp4',
+            'ogv': '.../mit-3091x/M-3091X-FA12-L21-3_100.ogv',
+            'webm': '.../mit-3091x/M-3091X-FA12-L21-3_100.webm',
+        }
 
-        fragment = module.runtime.render(module, None, 'student_view')
         expected_context = {
             'caption_asset_path': '/static/subs/',
-            'sub': module.sub,
+            'sub': 'a_sub_file.srt.sjson',
             'data_dir': getattr(self, 'data_dir', None),
-            'display_name': module.display_name_with_default,
-            'end': module.end_time,
-            'start': module.start_time,
+            'display_name': 'A Name',
+            'end': 3610.0,
+            'start': 3603.0,
             'id': module.location.html_id(),
-            'show_captions': module.show_captions,
-            'sources': module.sources,
-            'youtube_streams': module.youtube_streams,
-            'track': module.track,
+            'show_captions': True,
+            'sources': sources,
+            'youtube_streams': _create_youtube_string(module),
+            'track': '',
             'autoplay': settings.MITX_FEATURES.get('AUTOPLAY_VIDEOS', True)
         }
-        self.assertEqual(fragment.content, module.runtime.render_template('videoalpha.html', expected_context))
+
+        self.assertEqual(module.get_html(), expected_context)
+
+    def test_videoalpha_instance_state(self):
+        module = VideoAlphaFactory.create()
 
         self.assertDictEqual(
             json.loads(module.get_instance_state()),