Addresses latest comments on pull request #132. In particular:

- Make 1.0x speed default Youtube ID Lyla's introduction video.
- Add acceptance tests for toggling captions on/off.
- Move captions settings acceptance tests into video-editor.feature.
- Rename acceptance test methods to reflect their function.
- Remove undefined variable from video Jasmine tests.
- Test VideoDescriptor.from_xml when no attributes are set.
- Convert Stringy* to non-Stringy equivalents.
- Test parsing of Youtube ID strings and times in more cases.
- Remove some commented-out code.

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

@@ -11,3 +11,13 @@ Feature: Video Component Editor
     And I edit and select Settings
     Then I can modify the display name
     And my display name change is persisted on save
+
+  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
+
+  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/video.feature

@@ -16,15 +16,9 @@ Feature: Video Component
 
   Scenario: Captions are shown correctly
     Given I have created a Video component
-    And I have shown captions
     Then when I view the video it does show the captions
 
-  Scenario: Captions are hidden when "show captions" is false
+  Scenario: Captions are toggled correctly
     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
-
-  Scenario: Captions are shown when "show captions" is true
-    Given I have created a Video component
-    And I have set "show captions" to True
+    And I have toggled captions
     Then when I view the video it does show the captions

cms/djangoapps/contentstore/features/video.py

@@ -18,31 +18,33 @@ def video_takes_a_single_click(step):
     assert(world.is_css_present('.xmodule_VideoModule'))
 
 
-@step('I have (hidden|shown) captions')
+@step('I have (hidden|toggled) captions')
 def hide_or_show_captions(step, shown):
+    button_css = 'a.hide-subtitles'
     if shown == 'hidden':
-        world.css_click('a.hide-subtitles')
+        world.css_click(button_css)
+    if shown == 'toggled':
+        world.css_click(button_css)
+        # When we click the first time, a tooltip shows up. We want to
+        # click the button rather than the tooltip, so move the mouse
+        # away to make it disappear.
+        button = world.css_find(button_css)
+        button.mouse_out()
+        world.css_click(button_css)
 
 
 @step('when I view the video it (.*) show the captions')
-def does_not_show_captions(step, show_captions):
+def shows_captions(step, show_captions):
     # Prevent cookies from overriding course settings
     world.browser.cookies.delete('hide_captions')
     if show_captions == 'does not':
         assert world.css_find('.video')[0].has_class('closed')
     else:
-        assert not world.css_find('.video')[0].has_class('closed')
-
-
-# @step('when I view the video it does show the captions')
-# def shows_captions(step):
-#     # Prevent cookies from overriding course settings
-#     world.browser.cookies.delete('hide_captions')
-#     assert not world.css_find('.video')[0].has_class('closed')
+        assert world.is_css_not_present('.video.closed')
 
 
 @step('I have set "show captions" to (.*)')
-def set_show_captions_false(step, setting):
+def set_show_captions(step, setting):
     world.css_click('a.edit-button')
     world.browser.select('Show Captions', setting)
     world.css_click('a.save-button')

common/lib/xmodule/xmodule/js/spec/video/display_spec.coffee

@@ -59,7 +59,7 @@ describe 'Video', ->
         @originalYT = window.YT
         window.YT = { Player: true }
         spyOn(window, 'VideoPlayer').andReturn(@stubVideoPlayer)
-        @video = new Video '#example', @videosDefinition
+        @video = new Video '#example'
 
       afterEach ->
         window.YT = @originalYT
@@ -72,7 +72,7 @@ describe 'Video', ->
       beforeEach ->
         @originalYT = window.YT
         window.YT = {}
-        @video = new Video '#example', @videosDefinition
+        @video = new Video '#example'
 
       afterEach ->
         window.YT = @originalYT
@@ -85,7 +85,7 @@ describe 'Video', ->
         @originalYT = window.YT
         window.YT = {}
         spyOn(window, 'VideoPlayer').andReturn(@stubVideoPlayer)
-        @video = new Video '#example', @videosDefinition
+        @video = new Video '#example'
         window.onYouTubePlayerAPIReady()
 
       afterEach ->
@@ -98,7 +98,7 @@ describe 'Video', ->
   describe 'youtubeId', ->
     beforeEach ->
       $.cookie.andReturn '1.0'
-      @video = new Video '#example', @videosDefinition
+      @video = new Video '#example'
 
     describe 'with speed', ->
       it 'return the video id for given speed', ->
@@ -111,7 +111,7 @@ describe 'Video', ->
 
   describe 'setSpeed', ->
     beforeEach ->
-      @video = new Video '#example', @videosDefinition
+      @video = new Video '#example'
 
     describe 'when new speed is available', ->
       beforeEach ->
@@ -132,14 +132,14 @@ describe 'Video', ->
 
   describe 'getDuration', ->
     beforeEach ->
-      @video = new Video '#example', @videosDefinition
+      @video = new Video '#example'
 
     it 'return duration for current video', ->
       expect(@video.getDuration()).toEqual 200
 
   describe 'log', ->
     beforeEach ->
-      @video = new Video '#example', @videosDefinition
+      @video = new Video '#example'
       @video.setSpeed '1.0'
       spyOn Logger, 'log'
       @video.player = { currentTime: 25 }

common/lib/xmodule/xmodule/tests/test_video_module.py

@@ -57,3 +57,20 @@ class VideoDescriptorImportTestCase(unittest.TestCase):
         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')
+
+    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 = '<video></video>'
+        output = VideoDescriptor.from_xml(xml_data, module_system)
+        self.assertEquals(output.youtube_id_0_75, '')
+        self.assertEquals(output.youtube_id_1_0, 'OEoXaMPEzfM')
+        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, '')

common/lib/xmodule/xmodule/video_module.py

@@ -14,8 +14,7 @@ from django.http import Http404
 from xmodule.x_module import XModule
 from xmodule.raw_module import RawDescriptor
 from xmodule.editing_module import MetadataOnlyEditingDescriptor
-from xblock.core import Integer, Scope, String
-from fields import StringyFloat, StringyBoolean
+from xblock.core import Integer, Scope, String, Float, Boolean
 
 log = logging.getLogger(__name__)
 
@@ -23,13 +22,13 @@ log = logging.getLogger(__name__)
 class VideoFields(object):
     """Fields for `VideoModule` and `VideoDescriptor`."""
     position = Integer(help="Current position in the video", scope=Scope.user_state, default=0)
-    show_captions = StringyBoolean(help="This controls whether or not captions are shown by default.", display_name="Show Captions", scope=Scope.settings, default=True)
-    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="")
+    show_captions = Boolean(help="This controls whether or not captions are shown by default.", display_name="Show Captions", scope=Scope.settings, default=True)
+    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="OEoXaMPEzfM")
     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 = StringyFloat(help="Time the video starts", display_name="Start Time", scope=Scope.settings, default=0.0)
-    end_time = StringyFloat(help="Time the video ends", display_name="End Time", scope=Scope.settings, default=0.0)
+    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="")
     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="")
 
@@ -125,7 +124,9 @@ class VideoDescriptor(VideoFields,
                 video.youtube_id_1_25 = speeds['1.25']
             if speeds['1.50']:
                 video.youtube_id_1_5 = speeds['1.50']
-        video.show_captions = True if xml.get('show_captions') == 'true' else False
+        show_captions = xml.get('show_captions')
+        if show_captions:
+            video.show_captions = json.loads(show_captions)
         source = _get_first_external(xml, 'source')
         if source:
             video.source = source
@@ -177,7 +178,7 @@ def _parse_youtube(data):
 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:
+    if str_time is None or str_time == '':
         return ''
     else:
         obj_time = time.strptime(str_time, '%H:%M:%S')

lms/djangoapps/courseware/tests/test_video_xml.py

@@ -68,13 +68,23 @@ class VideoModuleLogicTest(LogicTest):
     }
 
     def test_parse_time(self):
-        """Ensure that times are parse correctly into seconds."""
+        """Ensure that times are parsed correctly into seconds."""
         output = _parse_time('00:04:07')
         self.assertEqual(output, 247)
 
+    def test_parse_time_none(self):
+        """Check parsing of None."""
+        output = _parse_time(None)
+        self.assertEqual(output, '')
+
+    def test_parse_time_empty(self):
+        """Check parsing of the empty string."""
+        output = _parse_time('')
+        self.assertEqual(output, '')
+
     def test_parse_youtube(self):
         """Test parsing old-style Youtube ID strings into a dict."""
-        youtube_str = '0.75:jNCf2gIqpeE,1.0:ZwkTiUPN0mg,1.25:rsq9auxASqI,1.50:kMyNdzVHHgg'
+        youtube_str = '0.75:jNCf2gIqpeE,1.00:ZwkTiUPN0mg,1.25:rsq9auxASqI,1.50:kMyNdzVHHgg'
         output = _parse_youtube(youtube_str)
         self.assertEqual(output, {'0.75': 'jNCf2gIqpeE',
                                   '1.00': 'ZwkTiUPN0mg',
@@ -92,3 +102,11 @@ class VideoModuleLogicTest(LogicTest):
                                   '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(_parse_youtube(youtube_str), _parse_youtube(youtube_str_hack))