From 9c84440501f93e045723a41c29a7bce8b4f724a2 Mon Sep 17 00:00:00 2001
From: Peter Fogg <pfogg@edx.org>
Date: Mon, 29 Jul 2013 16:20:41 -0400
Subject: [PATCH] PR comment cleanup.

---
 .../contentstore/features/common.py           |   8 +-
 .../component_settings_editor_helpers.py      |   6 +
 .../contentstore/features/video-editor.py     |   2 +
 .../contentstore/features/video.feature       |  12 +-
 cms/djangoapps/contentstore/features/video.py |  13 ++
 .../spec/views/metadata_edit_spec.coffee      |  29 +++-
 cms/static/js/views/metadata_editor_view.js   |   7 +
 .../js/metadata-list-entry.underscore         |   2 +-
 .../js/src/videoalpha/01_initialize.js        |   2 +-
 .../xmodule/js/src/videoalpha/10_main.js      |   5 +-
 .../xmodule/xmodule/tests/test_videoalpha.py  | 152 ++++++++++--------
 .../xmodule/xmodule/tests/test_xml_module.py  |  11 +-
 .../lib/xmodule/xmodule/videoalpha_module.py  |  28 +---
 .../courseware/tests/test_videoalpha_mongo.py |   4 +-
 .../courseware/tests/test_videoalpha_xml.py   |   2 +-
 lms/templates/videoalpha.html                 |   6 +-
 16 files changed, 168 insertions(+), 121 deletions(-)

diff --git a/cms/djangoapps/contentstore/features/common.py b/cms/djangoapps/contentstore/features/common.py
index 30ee0518c1..e06f27bcea 100644
--- a/cms/djangoapps/contentstore/features/common.py
+++ b/cms/djangoapps/contentstore/features/common.py
@@ -256,14 +256,14 @@ def open_new_unit(step):
     world.css_click('a.new-unit-item')
 
 
-@step('when I view the video it (.*) show the captions')
-def shows_captions(step, show_captions):
+@step('when I view the (video.*) it (.*) show the captions')
+def shows_captions(_step, video_type, show_captions):
     # Prevent cookies from overriding course settings
     world.browser.cookies.delete('hide_captions')
     if show_captions == 'does not':
-        assert world.css_has_class('.video', 'closed')
+        assert world.css_has_class('.%s' % video_type, 'closed')
     else:
-        assert world.is_css_not_present('.video.closed')
+        assert world.is_css_not_present('.%s.closed' % video_type)
 
 
 @step('the save button is disabled$')
diff --git a/cms/djangoapps/contentstore/features/component_settings_editor_helpers.py b/cms/djangoapps/contentstore/features/component_settings_editor_helpers.py
index 513eb699e9..f240386f26 100644
--- a/cms/djangoapps/contentstore/features/component_settings_editor_helpers.py
+++ b/cms/djangoapps/contentstore/features/component_settings_editor_helpers.py
@@ -59,6 +59,12 @@ def edit_component_and_select_settings():
     world.css_click('#settings-mode')
 
 
+@world.absorb
+def edit_component():
+    world.wait_for(lambda _driver: world.css_visible('a.edit-button'))
+    world.css_click('a.edit-button')
+
+
 @world.absorb
 def verify_setting_entry(setting, display_name, value, explicitly_set):
     assert_equal(display_name, setting.find_by_css('.setting-label')[0].value)
diff --git a/cms/djangoapps/contentstore/features/video-editor.py b/cms/djangoapps/contentstore/features/video-editor.py
index b74f37bb85..ad3229ab53 100644
--- a/cms/djangoapps/contentstore/features/video-editor.py
+++ b/cms/djangoapps/contentstore/features/video-editor.py
@@ -29,8 +29,10 @@ def correct_videoalpha_settings(_step):
     world.verify_all_setting_entries([['Display Name', 'Video Alpha', False],
                                       ['Download Track', '', False],
                                       ['Download Video', '', False],
+                                      ['End Time', '0', False],
                                       ['HTML5 Subtitles', '', False],
                                       ['Show Captions', 'True', False],
+                                      ['Start Time', '0', False],
                                       ['Video Sources', '', False],
                                       ['Youtube ID', 'OEoXaMPEzfM', False],
                                       ['Youtube ID for .75x speed', '', False],
diff --git a/cms/djangoapps/contentstore/features/video.feature b/cms/djangoapps/contentstore/features/video.feature
index 86cd2c27c9..dc85bbb17f 100644
--- a/cms/djangoapps/contentstore/features/video.feature
+++ b/cms/djangoapps/contentstore/features/video.feature
@@ -29,23 +29,21 @@ Feature: Video Component
 
   Scenario: User can view Video Alpha metadata
     Given I have created a Video Alpha component
-    And I edit and select Settings
+    And I edit the component
     Then I see the correct videoalpha settings and default values
 
   Scenario: User can modify Video Alpha display name
     Given I have created a Video Alpha component
-    And I edit and select Settings
+    And I edit the component
     Then I can modify the display name
-    And my display name change is persisted on save
+    And my videoalpha display name change is persisted on save
 
-  @skip
   Scenario: Video Alpha captions are hidden when "show captions" is false
     Given I have created a Video Alpha component
     And I have set "show captions" to False
-    Then when I view the video it does not show the captions
+    Then when I view the videoalpha it does not show the captions
 
-  @skip
   Scenario: Video Alpha captions are shown when "show captions" is true
     Given I have created a Video Alpha component
     And I have set "show captions" to True
-    Then when I view the video it does show the captions
+    Then when I view the videoalpha it does show the captions
diff --git a/cms/djangoapps/contentstore/features/video.py b/cms/djangoapps/contentstore/features/video.py
index d22e580222..72a8762197 100644
--- a/cms/djangoapps/contentstore/features/video.py
+++ b/cms/djangoapps/contentstore/features/video.py
@@ -1,6 +1,7 @@
 #pylint: disable=C0111
 
 from lettuce import world, step
+from terrain.steps import reload_the_page
 
 ############### ACTIONS ####################
 
@@ -31,3 +32,15 @@ def hide_or_show_captions(step, shown):
         button = world.css_find(button_css)
         button.mouse_out()
         world.css_click(button_css)
+
+@step('I edit the component')
+def i_edit_the_component(_step):
+    world.edit_component()
+
+
+@step('my videoalpha display name change is persisted on save')
+def videoalpha_name_persisted(step):
+    world.css_click('a.save-button')
+    reload_the_page(step)
+    world.edit_component()
+    world.verify_setting_entry(world.get_setting_entry('Display Name'), 'Display Name', '3.4', True)
diff --git a/cms/static/coffee/spec/views/metadata_edit_spec.coffee b/cms/static/coffee/spec/views/metadata_edit_spec.coffee
index bcc3cb436b..926e5be315 100644
--- a/cms/static/coffee/spec/views/metadata_edit_spec.coffee
+++ b/cms/static/coffee/spec/views/metadata_edit_spec.coffee
@@ -107,7 +107,8 @@ describe "Test Metadata Editor", ->
             view = new CMS.Views.Metadata.Editor({collection: @model})
             childModels = view.collection.models
             expect(childModels.length).toBe(6)
-            childViews = view.$el.find('.setting-input')
+            # Be sure to check list view as well as other input types
+            childViews = view.$el.find('.setting-input, .list-settings')
             expect(childViews.length).toBe(6)
 
             verifyEntry = (index, display_name, type) ->
@@ -116,7 +117,7 @@ describe "Test Metadata Editor", ->
 
             verifyEntry(0, 'Display Name', 'text')
             verifyEntry(1, 'Inputs', 'number')
-            verifyEntry(2, 'List', 'text')
+            verifyEntry(2, 'List', '')
             verifyEntry(3, 'Show Answer', 'select-one')
             verifyEntry(4, 'Unknown', 'text')
             verifyEntry(5, 'Weight', 'number')
@@ -319,9 +320,7 @@ describe "Test Metadata Editor", ->
       beforeEach ->
         listModel = new CMS.Models.Metadata(listEntry)
         @listView = new CMS.Views.Metadata.List({model: listModel})
-
-      it "uses a text input type", ->
-        assertInputType(@listView, 'text')
+        @el = @listView.$el
 
       it "returns the initial value upon initialization", ->
         assertValueInView(@listView, ['the first display value', 'the second'])
@@ -337,10 +336,24 @@ describe "Test Metadata Editor", ->
 
       it "can add an entry", ->
         expect(@listView.model.get('value').length).toEqual(2)
-        @listView.$el.find('.setting-add').click()
-        expect(@listView.$el.find('input.input').length).toEqual(3)
+        @el.find('.create-setting').click()
+        expect(@el.find('input.input').length).toEqual(3)
 
       it "can remove an entry", ->
         expect(@listView.model.get('value').length).toEqual(2)
-        @listView.$el.find('.setting-remove').first().click()
+        @el.find('.remove-setting').first().click()
         expect(@listView.model.get('value').length).toEqual(1)
+
+      it "only allows one blank entry at a time", ->
+        expect(@el.find('input').length).toEqual(2)
+        @el.find('.create-setting').click()
+        @el.find('.create-setting').click()
+        expect(@el.find('input').length).toEqual(3)
+
+      it "re-enables the add setting button after entering a new value", ->
+        expect(@el.find('input').length).toEqual(2)
+        @el.find('.create-setting').click()
+        expect(@el.find('.create-setting')).toHaveClass('is-disabled')
+        @el.find('input').last().val('third setting')
+        @el.find('input').last().trigger('input')
+        expect(@el.find('.create-setting')).not.toHaveClass('is-disabled')
diff --git a/cms/static/js/views/metadata_editor_view.js b/cms/static/js/views/metadata_editor_view.js
index bf74c4212f..f8c093e62a 100644
--- a/cms/static/js/views/metadata_editor_view.js
+++ b/cms/static/js/views/metadata_editor_view.js
@@ -320,6 +320,7 @@ CMS.Views.Metadata.List = CMS.Views.Metadata.AbstractEditor.extend({
         "click .setting-clear" : "clear",
         "keypress .setting-input" : "showClearButton",
         "change input" : "updateModel",
+        "input input" : "enableAdd",
         "click .create-setting" : "addEntry",
         "click .remove-setting" : "removeEntry"
     },
@@ -353,6 +354,7 @@ CMS.Views.Metadata.List = CMS.Views.Metadata.AbstractEditor.extend({
         // change event
         var list = this.model.get('value') || [];
         this.setValueInEditor(list.concat(['']))
+        this.$el.find('.create-setting').addClass('is-disabled');
     },
 
     removeEntry: function(event) {
@@ -360,5 +362,10 @@ CMS.Views.Metadata.List = CMS.Views.Metadata.AbstractEditor.extend({
         var entry = $(event.currentTarget).siblings().val();
         this.setValueInEditor(_.without(this.model.get('value'), entry));
         this.updateModel();
+        this.$el.find('.create-setting').removeClass('is-disabled');
+    },
+
+    enableAdd: function() {
+        this.$el.find('.create-setting').removeClass('is-disabled');
     }
 });
diff --git a/cms/templates/js/metadata-list-entry.underscore b/cms/templates/js/metadata-list-entry.underscore
index d4f79e2e17..8b3d99c507 100644
--- a/cms/templates/js/metadata-list-entry.underscore
+++ b/cms/templates/js/metadata-list-entry.underscore
@@ -6,7 +6,7 @@
     </ol>
 
     <a href="#" class="create-action create-setting">
-      <i class="icon-plus"></i>Add <span class="sr"><%= model.get('display_name')%></span>
+      <i class="icon-plus"></i><%= gettext("Add") %> <span class="sr"><%= model.get('display_name')%></span>
     </a>
   </div>
   <button class="action setting-clear inactive" type="button" name="setting-clear" value="<%= gettext("Clear") %>" data-tooltip="<%= gettext("Clear") %>">
diff --git a/common/lib/xmodule/xmodule/js/src/videoalpha/01_initialize.js b/common/lib/xmodule/xmodule/js/src/videoalpha/01_initialize.js
index 800cb62f98..d5d7149ceb 100644
--- a/common/lib/xmodule/xmodule/js/src/videoalpha/01_initialize.js
+++ b/common/lib/xmodule/xmodule/js/src/videoalpha/01_initialize.js
@@ -148,7 +148,7 @@ function (VideoPlayer) {
     //     Option
     //         this.config.show_captions = true | false
     //
-    //     defines whether to turn off/on the captions altogether. User will not have the ability to turn them on/off.
+    //     Defines whether or not captions are shown on first viewing.
     //
     //     Option
     //          this.hide_captions = true | false
diff --git a/common/lib/xmodule/xmodule/js/src/videoalpha/10_main.js b/common/lib/xmodule/xmodule/js/src/videoalpha/10_main.js
index b586687697..b5f647f563 100644
--- a/common/lib/xmodule/xmodule/js/src/videoalpha/10_main.js
+++ b/common/lib/xmodule/xmodule/js/src/videoalpha/10_main.js
@@ -60,10 +60,7 @@ function (
         VideoProgressSlider(state);
         VideoVolumeControl(state);
         VideoSpeedControl(state);
-
-        if (state.config.show_captions) {
-            VideoCaption(state);
-        }
+        VideoCaption(state);
 
         // Because the 'state' object is only available inside this closure, we will also make
         // it available to the caller by returning it. This is necessary so that we can test
diff --git a/common/lib/xmodule/xmodule/tests/test_videoalpha.py b/common/lib/xmodule/xmodule/tests/test_videoalpha.py
index 838b509cda..76f86d6d4a 100644
--- a/common/lib/xmodule/xmodule/tests/test_videoalpha.py
+++ b/common/lib/xmodule/xmodule/tests/test_videoalpha.py
@@ -148,6 +148,14 @@ class VideoAlphaDescriptorImportTestCase(unittest.TestCase):
     Make sure that VideoAlphaDescriptor can import an old XML-based video correctly.
     """
 
+    def assert_attributes_equal(self, video, attrs):
+        """
+        Assert that `video` has the correct attributes. `attrs` is a map
+        of {metadata_field: value}.
+        """
+        for key, value in attrs.items():
+            self.assertEquals(getattr(video, key), value)
+
     def test_constructor(self):
         sample_xml = '''
             <videoalpha display_name="Test Video"
@@ -166,17 +174,18 @@ class VideoAlphaDescriptorImportTestCase(unittest.TestCase):
                       '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, '')
+        self.assert_attributes_equal(descriptor, {
+            'youtube_id_0_75': 'izygArpw-Qo',
+            'youtube_id_1_0': 'p2Q6BrNhdh8',
+            'youtube_id_1_25': '1EeWXzPdhSA',
+            'youtube_id_1_5': 'rABDYkeK0x8',
+            'show_captions': False,
+            'start_time': 1.0,
+            'end_time': 60,
+            'track': 'http://www.example.com/track',
+            'html5_sources': ['http://www.example.com/source.mp4', 'http://www.example.com/source.ogg'],
+            'data': ''
+        })
 
     def test_from_xml(self):
         module_system = DummySystem(load_error_modules=True)
@@ -191,17 +200,19 @@ class VideoAlphaDescriptorImportTestCase(unittest.TestCase):
             </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, '')
+        self.assert_attributes_equal(output, {
+            'youtube_id_0_75': 'izygArpw-Qo',
+            'youtube_id_1_0': 'p2Q6BrNhdh8',
+            'youtube_id_1_25': '1EeWXzPdhSA',
+            'youtube_id_1_5': 'rABDYkeK0x8',
+            'show_captions': False,
+            'start_time': 1.0,
+            'end_time': 60,
+            'track': 'http://www.example.com/track',
+            'source': 'http://www.example.com/source.mp4',
+            'html5_sources': ['http://www.example.com/source.mp4'],
+            'data': ''
+        })
 
     def test_from_xml_missing_attributes(self):
         """
@@ -218,17 +229,19 @@ class VideoAlphaDescriptorImportTestCase(unittest.TestCase):
             </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, '')
+        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': 'http://www.example.com/track',
+            'source': 'http://www.example.com/source.mp4',
+            'html5_sources': ['http://www.example.com/source.mp4'],
+            'data': ''
+        })
 
     def test_from_xml_no_attributes(self):
         """
@@ -237,17 +250,19 @@ class VideoAlphaDescriptorImportTestCase(unittest.TestCase):
         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, '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, '')
-        self.assertEquals(output.html5_sources, [])
-        self.assertEquals(output.data, '')
+        self.assert_attributes_equal(output, {
+            'youtube_id_0_75': '',
+            'youtube_id_1_0': 'OEoXaMPEzfM',
+            'youtube_id_1_25': '',
+            '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):
         """
@@ -265,19 +280,23 @@ class VideoAlphaDescriptorImportTestCase(unittest.TestCase):
             </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, '')
+        self.assert_attributes_equal(output, {
+            'youtube_id_0_75': 'izygArpw-Qo',
+            'youtube_id_1_0': 'p2Q6BrNhdh8',
+            'youtube_id_1_25': '1EeWXzPdhSA',
+            'youtube_id_1_5': 'rABDYkeK0x8',
+            'show_captions': False,
+            'start_time': 1.0,
+            'end_time': 60,
+            'track': 'http://www.example.com/track',
+            'html5_sources': ['http://www.example.com/source.mp4'],
+            'data': ''
+        })
 
     def test_old_video_data(self):
+        """
+        Ensure that Video Alpha is able to read VideoModule's model data.
+        """
         module_system = DummySystem(load_error_modules=True)
         xml_data = """
             <video display_name="Test Video"
@@ -291,17 +310,18 @@ class VideoAlphaDescriptorImportTestCase(unittest.TestCase):
         """
         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, '')
+        self.assert_attributes_equal(video_alpha, {
+            'youtube_id_0_75': 'izygArpw-Qo',
+            'youtube_id_1_0': 'p2Q6BrNhdh8',
+            'youtube_id_1_25': '1EeWXzPdhSA',
+            'youtube_id_1_5': 'rABDYkeK0x8',
+            'show_captions': False,
+            'start_time': 1.0,
+            'end_time': 60,
+            'track': 'http://www.example.com/track',
+            'html5_sources': ['http://www.example.com/source.mp4'],
+            'data': ''
+        })
 
 
 class VideoAlphaExportTestCase(unittest.TestCase):
diff --git a/common/lib/xmodule/xmodule/tests/test_xml_module.py b/common/lib/xmodule/xmodule/tests/test_xml_module.py
index a277ff2900..fdcff8ade2 100644
--- a/common/lib/xmodule/xmodule/tests/test_xml_module.py
+++ b/common/lib/xmodule/xmodule/tests/test_xml_module.py
@@ -39,7 +39,8 @@ class TestFields(object):
     float_non_select = Float(scope=Scope.settings, default=.999, values={'min': 0, 'step': .3})
     # Used for testing that Booleans get mapped to select type
     boolean_select = Boolean(scope=Scope.settings)
-
+    # Used for testing Lists
+    list_field = List(scope=Scope.settings, default=[])
 
 class EditableMetadataFieldsTest(unittest.TestCase):
     def test_display_name_field(self):
@@ -63,7 +64,7 @@ class EditableMetadataFieldsTest(unittest.TestCase):
     def test_integer_field(self):
         descriptor = self.get_descriptor({'max_attempts': '7'})
         editable_fields = descriptor.editable_metadata_fields
-        self.assertEqual(6, len(editable_fields))
+        self.assertEqual(7, len(editable_fields))
         self.assert_field_values(
             editable_fields, 'max_attempts', TestFields.max_attempts,
             explicitly_set=True, inheritable=False, value=7, default_value=1000, type='Integer',
@@ -137,6 +138,12 @@ class EditableMetadataFieldsTest(unittest.TestCase):
             type='Float', options={'min': 0, 'step': .3}
         )
 
+        self.assert_field_values(
+            editable_fields, 'list_field', TestFields.list_field,
+            explicitly_set=False, inheritable=False, value=[], default_value=[],
+            type='List'
+        )
+
     # Start of helper methods
     def get_xml_editable_fields(self, model_data):
         system = get_test_system()
diff --git a/common/lib/xmodule/xmodule/videoalpha_module.py b/common/lib/xmodule/xmodule/videoalpha_module.py
index 2c79660f36..287bc8cd97 100644
--- a/common/lib/xmodule/xmodule/videoalpha_module.py
+++ b/common/lib/xmodule/xmodule/videoalpha_module.py
@@ -51,6 +51,8 @@ class VideoAlphaFields(object):
         scope=Scope.settings,
         default=True
     )
+    # TODO: This should be moved to Scope.content, but this will
+    # require data migration to support the old video module.
     youtube_id_1_0 = String(
         help="This is the Youtube ID reference for the normal speed video.",
         display_name="Youtube ID",
@@ -76,13 +78,13 @@ class VideoAlphaFields(object):
         default=""
     )
     start_time = Float(
-        help="Time the video starts",
+        help="Start time for the video.",
         display_name="Start Time",
         scope=Scope.settings,
         default=0.0
     )
     end_time = Float(
-        help="Time the video ends",
+        help="End time for the video.",
         display_name="End Time",
         scope=Scope.settings,
         default=0.0
@@ -94,7 +96,7 @@ class VideoAlphaFields(object):
         default=""
     )
     html5_sources = List(
-        help="A list of filenames to be used with HTML5 video.",
+        help="A list of filenames to be used with HTML5 video. The first supported filetype will be displayed.",
         display_name="Video Sources",
         scope=Scope.settings,
         default=[]
@@ -179,7 +181,7 @@ class VideoAlphaModule(VideoAlphaFields, XModule):
             # isn't on the filesystem
             'data_dir': getattr(self, 'data_dir', None),
             'caption_asset_path': caption_asset_path,
-            'show_captions': self.show_captions,
+            'show_captions': json.dumps(self.show_captions),
             'start': self.start_time,
             'end': self.end_time,
             'autoplay': settings.MITX_FEATURES.get('AUTOPLAY_VIDEOS', True)
@@ -211,11 +213,6 @@ class VideoAlphaDescriptor(VideoAlphaFields, TabsEditingDescriptor, RawDescripto
             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
-        return non_editable_fields + [VideoAlphaFields.start_time, VideoAlphaFields.end_time]
-
     @classmethod
     def from_xml(cls, xml_data, system, org=None, course=None):
         """
@@ -234,16 +231,7 @@ class VideoAlphaDescriptor(VideoAlphaFields, TabsEditingDescriptor, RawDescripto
 
     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
+        Returns an xml string representing this module.
         """
         xml = etree.Element('videoalpha')
         attrs = {
@@ -347,7 +335,7 @@ class VideoAlphaDescriptor(VideoAlphaFields, TabsEditingDescriptor, RawDescripto
     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 == '':
+        if not str_time:
             return ''
         else:
             obj_time = time.strptime(str_time, '%H:%M:%S')
diff --git a/lms/djangoapps/courseware/tests/test_videoalpha_mongo.py b/lms/djangoapps/courseware/tests/test_videoalpha_mongo.py
index a0069e7d53..38b2b6fb8d 100644
--- a/lms/djangoapps/courseware/tests/test_videoalpha_mongo.py
+++ b/lms/djangoapps/courseware/tests/test_videoalpha_mongo.py
@@ -55,7 +55,7 @@ class TestVideo(BaseTestXmodule):
         expected_context = {
             'data_dir': getattr(self, 'data_dir', None),
             'caption_asset_path': '/c4x/MITx/999/asset/subs_',
-            'show_captions': True,
+            'show_captions': 'true',
             'display_name': 'A Name',
             'end': 3610.0,
             'id': self.item_module.location.html_id(),
@@ -104,7 +104,7 @@ class TestVideoNonYouTube(TestVideo):
         expected_context = {
             'data_dir': getattr(self, 'data_dir', None),
             'caption_asset_path': '/c4x/MITx/999/asset/subs_',
-            'show_captions': True,
+            'show_captions': 'true',
             'display_name': 'A Name',
             'end': 3610.0,
             'id': self.item_module.location.html_id(),
diff --git a/lms/djangoapps/courseware/tests/test_videoalpha_xml.py b/lms/djangoapps/courseware/tests/test_videoalpha_xml.py
index 4a4e378174..bd5010d16e 100644
--- a/lms/djangoapps/courseware/tests/test_videoalpha_xml.py
+++ b/lms/djangoapps/courseware/tests/test_videoalpha_xml.py
@@ -86,7 +86,7 @@ class VideoAlphaModuleUnitTest(unittest.TestCase):
             'end': 3610.0,
             'start': 3603.0,
             'id': module.location.html_id(),
-            'show_captions': True,
+            'show_captions': 'true',
             'sources': sources,
             'youtube_streams': _create_youtube_string(module),
             'track': '',
diff --git a/lms/templates/videoalpha.html b/lms/templates/videoalpha.html
index 416620a4b5..682c299e10 100644
--- a/lms/templates/videoalpha.html
+++ b/lms/templates/videoalpha.html
@@ -58,17 +58,13 @@
                     <a href="#" class="add-fullscreen" title="${_('Fill browser')}">${_('Fill browser')}</a>
                     <a href="#" class="quality_control" title="${_('HD')}">${_('HD')}</a>
 
-                    % if show_captions == 'true':
-                    <a href="#" class="hide-subtitles" title="${_('Turn off captions')}">${_('Captions')}</a>
-                    % endif
+                    <a href="#" class="hide-subtitles" title="${_('Turn off captions')}">Captions</a>
                 </div>
             </div>
         </section>
   </article>
 
-      % if show_captions == 'true':
       <ol class="subtitles"><li></li></ol>
-      % endif
 
 </div>
 </div>