Merge pull request #455 from edx/peter-fogg/remove-videoalpha-xml

Peter fogg/remove videoalpha xml

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')
@@ -236,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$')

cms/djangoapps/contentstore/features/component_settings_editor_helpers.py

@@ -60,6 +60,12 @@ def edit_component_and_select_settings():
 
 
 @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)
     assert_equal(value, setting.find_by_css('.setting-input')[0].value)

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

@@ -22,3 +22,19 @@ def set_show_captions(step, setting):
     world.wait_for(lambda _driver: world.css_visible('a.save-button'))
     world.browser.select('Show Captions', setting)
     world.css_click('a.save-button')
+
+
+@step('I see the correct videoalpha settings and default values$')
+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],
+                                      ['Youtube ID for 1.25x speed', '', False],
+                                      ['Youtube ID for 1.5x speed', '', False]])

cms/djangoapps/contentstore/features/video.feature

@@ -22,3 +22,28 @@ Feature: Video Component
     Given I have created a Video component
     And I have toggled captions
     Then when I view the video it does show the captions
+
+  Scenario: Autoplay is disabled in Studio for Video Alpha
+    Given I have created a Video Alpha component
+    Then when I view the videoalpha it does not have autoplay enabled
+
+  Scenario: User can view Video Alpha metadata
+    Given I have created a Video Alpha component
+    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 the component
+    Then I can modify the display name
+    And my videoalpha display name change is persisted on save
+
+  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 videoalpha it does not show the captions
+
+  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 videoalpha it does show the captions

cms/djangoapps/contentstore/features/video.py

 #pylint: disable=C0111
 
 from lettuce import world, step
+from terrain.steps import reload_the_page
 
 ############### ACTIONS ####################
 
 
-@step('when I view the video it does not have autoplay enabled')
-def does_not_autoplay(_step):
-    assert world.css_find('.video')[0]['data-autoplay'] == 'False'
+@step('when I view the (.*) it does not have autoplay enabled')
+def does_not_autoplay(_step, video_type):
+    assert world.css_find('.%s' % video_type)[0]['data-autoplay'] == 'False'
     assert world.css_has_class('.video_control', 'play')
 
 
@@ -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)

cms/static/coffee/fixtures/metadata-list-entry.underscore

+../../../templates/js/metadata-list-entry.underscore
\ No newline at end of file

cms/static/coffee/spec/views/metadata_edit_spec.coffee

@@ -3,12 +3,14 @@ describe "Test Metadata Editor", ->
     numberEntryTemplate = readFixtures('metadata-number-entry.underscore')
     stringEntryTemplate = readFixtures('metadata-string-entry.underscore')
     optionEntryTemplate = readFixtures('metadata-option-entry.underscore')
+    listEntryTemplate = readFixtures('metadata-list-entry.underscore')
 
     beforeEach ->
         setFixtures($("<script>", {id: "metadata-editor-tpl", type: "text/template"}).text(editorTemplate))
         appendSetFixtures($("<script>", {id: "metadata-number-entry", type: "text/template"}).text(numberEntryTemplate))
         appendSetFixtures($("<script>", {id: "metadata-string-entry", type: "text/template"}).text(stringEntryTemplate))
         appendSetFixtures($("<script>", {id: "metadata-option-entry", type: "text/template"}).text(optionEntryTemplate))
+        appendSetFixtures($("<script>", {id: "metadata-list-entry", type: "text/template"}).text(listEntryTemplate))
 
     genericEntry = {
         default_value: 'default value',
@@ -62,6 +64,18 @@ describe "Test Metadata Editor", ->
         value: 10.2
     }
 
+    listEntry = {
+        default_value: ["a thing", "another thing"],
+        display_name: "List",
+        explicitly_set: false,
+        field_name: "list",
+        help: "A list of things.",
+        inheritable: false,
+        options: [],
+        type: CMS.Models.Metadata.LIST_TYPE,
+        value: ["the first display value", "the second"]
+    }
+
     # Test for the editor that creates the individual views.
     describe "CMS.Views.Metadata.Editor creates editors for each field", ->
         beforeEach ->
@@ -84,16 +98,18 @@ describe "Test Metadata Editor", ->
                             {"display_name": "Never", "value": "never"}],
                         type: "unknown type",
                         value: null
-                    }
+                    },
+                    listEntry
                 ]
             )
 
         it "creates child views on initialize, and sorts them alphabetically", ->
             view = new CMS.Views.Metadata.Editor({collection: @model})
             childModels = view.collection.models
-            expect(childModels.length).toBe(5)
-            childViews = view.$el.find('.setting-input')
-            expect(childViews.length).toBe(5)
+            expect(childModels.length).toBe(6)
+            # 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) ->
                 expect(childModels[index].get('display_name')).toBe(display_name)
@@ -101,9 +117,10 @@ describe "Test Metadata Editor", ->
 
             verifyEntry(0, 'Display Name', 'text')
             verifyEntry(1, 'Inputs', 'number')
-            verifyEntry(2, 'Show Answer', 'select-one')
-            verifyEntry(3, 'Unknown', 'text')
-            verifyEntry(4, 'Weight', 'number')
+            verifyEntry(2, 'List', '')
+            verifyEntry(3, 'Show Answer', 'select-one')
+            verifyEntry(4, 'Unknown', 'text')
+            verifyEntry(5, 'Weight', 'number')
 
         it "returns its display name", ->
             view = new CMS.Views.Metadata.Editor({collection: @model})
@@ -146,27 +163,27 @@ describe "Test Metadata Editor", ->
     # Tests for individual views.
     assertInputType = (view, expectedType) ->
         input = view.$el.find('.setting-input')
-        expect(input.length).toBe(1)
-        expect(input[0].type).toBe(expectedType)
+        expect(input.length).toEqual(1)
+        expect(input[0].type).toEqual(expectedType)
 
     assertValueInView = (view, expectedValue) ->
-        expect(view.getValueFromEditor()).toBe(expectedValue)
+        expect(view.getValueFromEditor()).toEqual(expectedValue)
 
     assertCanUpdateView = (view, newValue) ->
         view.setValueInEditor(newValue)
-        expect(view.getValueFromEditor()).toBe(newValue)
+        expect(view.getValueFromEditor()).toEqual(newValue)
 
     assertClear = (view, modelValue, editorValue=modelValue) ->
         view.clear()
         expect(view.model.getValue()).toBe(null)
-        expect(view.model.getDisplayValue()).toBe(modelValue)
-        expect(view.getValueFromEditor()).toBe(editorValue)
+        expect(view.model.getDisplayValue()).toEqual(modelValue)
+        expect(view.getValueFromEditor()).toEqual(editorValue)
 
     assertUpdateModel = (view, originalValue, newValue) ->
         view.setValueInEditor(newValue)
-        expect(view.model.getValue()).toBe(originalValue)
+        expect(view.model.getValue()).toEqual(originalValue)
         view.updateModel()
-        expect(view.model.getValue()).toBe(newValue)
+        expect(view.model.getValue()).toEqual(newValue)
 
     describe "CMS.Views.Metadata.String is a basic string input with clear functionality", ->
         beforeEach ->
@@ -298,3 +315,45 @@ describe "Test Metadata Editor", ->
 
             verifyDisallowedChars(@integerView)
             verifyDisallowedChars(@floatView)
+
+    describe "CMS.Views.Metadata.List allows the user to enter an ordered list of strings", ->
+      beforeEach ->
+        listModel = new CMS.Models.Metadata(listEntry)
+        @listView = new CMS.Views.Metadata.List({model: listModel})
+        @el = @listView.$el
+
+      it "returns the initial value upon initialization", ->
+        assertValueInView(@listView, ['the first display value', 'the second'])
+
+      it "updates its value correctly", ->
+        assertCanUpdateView(@listView, ['a new item', 'another new item', 'a third'])
+
+      it "has a clear method to revert to the model default", ->
+        assertClear(@listView, ['a thing', 'another thing'])
+
+      it "has an update model method", ->
+        assertUpdateModel(@listView, null, ['a new value'])
+
+      it "can add an entry", ->
+        expect(@listView.model.get('value').length).toEqual(2)
+        @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)
+        @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')

cms/static/js/models/metadata_model.js

@@ -111,3 +111,4 @@ CMS.Models.Metadata.SELECT_TYPE = "Select";
 CMS.Models.Metadata.INTEGER_TYPE = "Integer";
 CMS.Models.Metadata.FLOAT_TYPE = "Float";
 CMS.Models.Metadata.GENERIC_TYPE = "Generic";
+CMS.Models.Metadata.LIST_TYPE = "List";

cms/static/js/views/metadata_editor_view.js

@@ -27,6 +27,9 @@ CMS.Views.Metadata.Editor = Backbone.View.extend({
                     model.getType() === CMS.Models.Metadata.FLOAT_TYPE) {
                     new CMS.Views.Metadata.Number(data);
                 }
+                else if(model.getType() === CMS.Models.Metadata.LIST_TYPE) {
+                    new CMS.Views.Metadata.List(data);
+                }
                 else {
                     // Everything else is treated as GENERIC_TYPE, which uses String editor.
                     new CMS.Views.Metadata.String(data);
@@ -310,3 +313,59 @@ CMS.Views.Metadata.Option = CMS.Views.Metadata.AbstractEditor.extend({
         }).prop('selected', true);
     }
 });
+
+CMS.Views.Metadata.List = CMS.Views.Metadata.AbstractEditor.extend({
+
+    events : {
+        "click .setting-clear" : "clear",
+        "keypress .setting-input" : "showClearButton",
+        "change input" : "updateModel",
+        "input input" : "enableAdd",
+        "click .create-setting" : "addEntry",
+        "click .remove-setting" : "removeEntry"
+    },
+
+    templateName: "metadata-list-entry",
+
+    getValueFromEditor: function () {
+        return _.map(
+            this.$el.find('li input'),
+            function (ele) { return ele.value.trim(); }
+        ).filter(_.identity);
+    },
+
+    setValueInEditor: function (value) {
+        var list = this.$el.find('ol');
+        list.empty();
+        _.each(value, function(ele, index) {
+            var template = _.template(
+                '<li class="list-settings-item">' +
+                    '<input type="text" class="input" value="<%= ele %>">' +
+                    '<a href="#" class="remove-action remove-setting" data-index="<%= index %>"><i class="icon-remove-sign"></i><span class="sr">Remove</span></a>' +
+                '</li>'
+            );
+            list.append($(template({'ele': ele, 'index': index})));
+        });
+    },
+
+    addEntry: function(event) {
+        event.preventDefault();
+        // We don't call updateModel here since it's bound to the
+        // change event
+        var list = this.model.get('value') || [];
+        this.setValueInEditor(list.concat(['']))
+        this.$el.find('.create-setting').addClass('is-disabled');
+    },
+
+    removeEntry: function(event) {
+        event.preventDefault();
+        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');
+    }
+});

cms/static/sass/views/_textbooks.scss

@@ -148,6 +148,14 @@ body.course.textbooks {
         padding: ($baseline*0.75) $baseline;
         background: $gray-l6;
 
+        .action {
+          margin-right: ($baseline/4);
+
+          &:last-child {
+            margin-right: 0;
+          }
+        }
+
         // add a chapter is below with chapters styling
 
         .action-primary {

cms/static/sass/views/_unit.scss

@@ -449,12 +449,39 @@ body.course.unit {
 
       // Module Actions, also used for Static Pages
       .module-actions {
-        box-shadow: inset 0 1px 1px $shadow;
-        padding: 0 0 $baseline $baseline;
-        background-color: $gray-l6;
+        box-shadow: inset 0 1px 2px $shadow;
+        border-top: 1px solid $gray-l1;
+        padding: ($baseline*0.75) $baseline;
+        background: $gray-l6;
 
-        .save-button {
-        margin: ($baseline/2) 8px 0 0;
+        .action {
+          display: inline-block;
+          vertical-align: middle;
+          margin-right: ($baseline/4);
+
+          &:last-child {
+            margin-right: 0;
+          }
+        }
+
+        .action-primary {
+          @include blue-button;
+          @extend .t-action2;
+          @include transition(all .15s);
+          display: inline-block;
+          padding: ($baseline/5) $baseline;
+          font-weight: 600;
+          text-transform: uppercase;
+        }
+
+        .action-secondary {
+          @include grey-button;
+          @extend .t-action2;
+          @include transition(all .15s);
+          display: inline-block;
+          padding: ($baseline/5) $baseline;
+          font-weight: 600;
+          text-transform: uppercase;
         }
       }
     }
@@ -599,26 +626,27 @@ body.course.unit {
         }
       }
 
-      .wrapper-comp-setting{
+      .wrapper-comp-setting {
         display: inline-block;
         min-width: 300px;
-        width: 45%;
+        width: 55%;
         top: 0;
         vertical-align: top;
         margin-bottom:5px;
         position: relative;
       }
 
-      label.setting-label {
+      .setting-label {
         @extend .t-copy-sub1;
         @include transition(color $tmg-f2 ease-in-out 0s);
-        font-weight: 400;
         vertical-align: middle;
         display: inline-block;
         position: relative;
         left: 0;
+        width: 33%;
         min-width: 100px;
-        width: 35%;
+        margin-right: ($baseline/2);
+        font-weight: 600;
 
         &.is-focused {
           color: $blue;
@@ -708,13 +736,97 @@ body.course.unit {
         }
       }
 
-      .tip.setting-help {
+      .setting-help {
         @include font-size(12);
         display: inline-block;
         font-color: $gray-l6;
-        min-width: 260px;
-        width: 50%;
+        min-width: ($baseline*10);
+        width: 35%;
+        vertical-align: top;
+      }
+
+
+
+      // TYPE: enumerated lists of metadata sets
+      .metadata-list-enum {
+
+        * {
+          @include box-sizing(border-box);
+        }
+
+        // label
+        .setting-label {
+          vertical-align: top;
+          margin-top: ($baseline/2);
+        }
+
+        // inputs and labels
+        .wrapper-list-settings {
+          @include size(45%,100%);
+          display: inline-block;
+          min-width: ($baseline*5);
+
+          // enumerated fields
+          .list-settings {
+            margin: 0;
+
+            .list-settings-item {
+              margin-bottom: ($baseline/2);
+            }
+
+            // inputs
+            .input {
+              width: 80%;
+              margin-right: ($baseline/2);
+              vertical-align: middle;
+            }
+          }
+        }
+
+        // actions
+        .create-action, .remove-action, .setting-clear {
+
+        }
+
+        .setting-clear {
           vertical-align: top;
+          margin-top: ($baseline/4);
+        }
+
+        .create-setting {
+          @extend .ui-btn-flat-outline;
+          @extend .t-action3;
+          display: block;
+          width: 100%;
+          padding: ($baseline/2);
+          font-weight: 600;
+
+          *[class^="icon-"] {
+            margin-right: ($baseline/4);
+          }
+
+          // STATE: disabled
+          &.is-disabled {
+
+          }
+        }
+
+        .remove-setting {
+          @include transition(color 0.25s ease-in-out);
+          @include font-size(20);
+          display: inline-block;
+          background: transparent;
+          color: $blue-l3;
+
+          &:hover {
+            color: $blue;
+          }
+
+          // STATE: disabled
+          &.is-disabled {
+
+          }
+        }
       }
     }
   }

cms/templates/component.html

@@ -26,8 +26,8 @@
 	        </div>
 		</div>
 		<div class="row module-actions">
-	  		<a href="#" class="save-button">${_("Save")}</a>
-	  		<a href="#" class="cancel-button">${_("Cancel")}</a>
+	  		<a href="#" class="save-button action-primary action">${_("Save")}</a>
+	  		<a href="#" class="cancel-button action-secondary action">${_("Cancel")}</a>
 		</div> <!-- Module Actions-->
   </div>
 </div>

cms/templates/js/metadata-editor.underscore

 <ul class="list-input settings-list">
     <% _.each(_.range(numEntries), function() { %>
-    <li class="field comp-setting-entry metadata_entry" id="settings-listing">
+    <li class="field comp-setting-entry metadata_entry">
     </li>
     <% }) %>
 </ul>

cms/templates/js/metadata-list-entry.underscore

+<div class="wrapper-comp-setting metadata-list-enum">
+  <label class="label setting-label" for="<%= uniqueId %>"><%= model.get('display_name')%></label>
+  <div id="<%= uniqueId %>" class="wrapper-list-settings">
+    <ol class="list-settings">
+
+    </ol>
+
+    <a href="#" class="create-action create-setting">
+      <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") %>">
+    <i class="icon-undo"></i>
+    <span class="sr">"<%= gettext("Clear Value") %>"</span>
+  </button>
+</div>
+<span class="tip setting-help"><%= model.get('help') %></span>

cms/templates/widgets/metadata-edit.html

@@ -25,6 +25,10 @@
     <%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>
+
 <% showHighLevelSource='source_code' in editable_metadata_fields and editable_metadata_fields['source_code']['explicitly_set'] %>
 <% metadata_field_copy = copy.copy(editable_metadata_fields) %>
 ## Delete 'source_code' field (if it exists) so metadata editor view does not attempt to render it.

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/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

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);
-        }
 
         // 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

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.
+These test data read from xml, not from mongo.
 
-we have a ModuleStoreTestCase class defined in
+We have a ModuleStoreTestCase class defined in
 common/lib/xmodule/xmodule/modulestore/tests/django_utils.py. You can
 search for usages of this in the cms and lms tests for examples. You use
 this so that it will do things like point the modulestore setting to 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,267 @@ 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 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"
+                   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.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)
+        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.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):
+        """
+        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.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):
+        """
+        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.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):
+        """
+        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.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"
+                   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.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):
+    """
+    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" youtube="1.00:OEoXaMPEzfM" show_captions="true"/>\n'
+
+        self.assertEquals(expected, xml)

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()

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,94 @@ 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",
+        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: 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",
+        scope=Scope.settings,
+        default="OEoXaMPEzfM"
+    )
+    youtube_id_0_75 = String(
+        help="The Youtube ID for the .75x speed video.",
+        display_name="Youtube ID for .75x speed",
+        scope=Scope.settings,
+        default=""
+    )
+    youtube_id_1_25 = String(
+        help="The Youtube ID for the 1.25x speed video.",
+        display_name="Youtube ID for 1.25x speed",
+        scope=Scope.settings,
+        default=""
+    )
+    youtube_id_1_5 = String(
+        help="The Youtube ID for the 1.5x speed video.",
+        display_name="Youtube ID for 1.5x speed",
+        scope=Scope.settings,
+        default=""
+    )
+    start_time = Float(
+        help="Start time for the video.",
+        display_name="Start Time",
+        scope=Scope.settings,
+        default=0.0
+    )
+    end_time = Float(
+        help="End time for the video.",
+        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 list of filenames to be used with HTML5 video. The first supported filetype will be displayed.",
+        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 +148,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,19 +166,22 @@ 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
             # 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)
@@ -193,18 +193,174 @@ 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)
+        # For backwards compatibility -- if we've got XML data, parse
+        # it out and set the metadata fields
+        if self.data:
+            model_data = VideoAlphaDescriptor._parse_video_xml(self.data)
+            self._model_data.update(model_data)
+            del self.data
+
+    @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.
+        """
+        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 not 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]])

common/lib/xmodule/xmodule/x_module.py

@@ -10,7 +10,7 @@ from pkg_resources import resource_listdir, resource_string, resource_isdir
 from xmodule.modulestore import inheritance, Location
 from xmodule.modulestore.exceptions import ItemNotFoundError, InsufficientSpecificationError, InvalidLocationError
 
-from xblock.core import XBlock, Scope, String, Integer, Float, ModelType
+from xblock.core import XBlock, Scope, String, Integer, Float, List, ModelType
 from xblock.fragment import Fragment
 from xblock.runtime import Runtime
 from xmodule.modulestore.locator import BlockUsageLocator
@@ -766,7 +766,7 @@ class XModuleDescriptor(XModuleFields, HTMLSnippet, ResourceTemplates, XBlock):
             # 2. Number editors for integers and floats.
             # 3. A generic string editor for anything else (editing JSON representation of the value).
             editor_type = "Generic"
-            values = [] if field.values is None else copy.deepcopy(field.values)
+            values = copy.deepcopy(field.values)
             if isinstance(values, tuple):
                 values = list(values)
             if isinstance(values, list):
@@ -783,11 +783,13 @@ class XModuleDescriptor(XModuleFields, HTMLSnippet, ResourceTemplates, XBlock):
                 editor_type = "Integer"
             elif isinstance(field, Float):
                 editor_type = "Float"
+            elif isinstance(field, List):
+                editor_type = "List"
             metadata_fields[field.name] = {'field_name': field.name,
                                            'type': editor_type,
                                            'display_name': field.display_name,
                                            'value': field.to_json(value),
-                                           'options': values,
+                                           'options': [] if values is None else values,
                                            'default_value': field.to_json(default_value),
                                            'inheritable': inheritable,
                                            'explicitly_set': explicitly_set,

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': 'example.mp4',
+            'mp4': 'example.mp4',
+            'webm': 'example.webm',
+            'ogv': 'example.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,14 +75,13 @@ 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"/>
-            <source src=".../mit-3091x/M-3091X-FA12-L21-3_100.webm"/>
-            <source src=".../mit-3091x/M-3091X-FA12-L21-3_100.ogv"/>
+            <source src="example.mp4"/>
+            <source src="example.webm"/>
+            <source src="example.ogv"/>
         </videoalpha>
     """
     MODEL_DATA = {
@@ -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'example.mp4',
+            u'mp4': u'example.mp4',
+            u'webm': u'example.webm',
+            u'ogv': u'example.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,
-            'youtube_streams': '',
+            'sources': sources,
+            'start': 3603.0,
+            'sub': 'a_sub_file.srt.sjson',
+            'track': '',
+            'youtube_streams': '1.00:OEoXaMPEzfM',
             '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

 # -*- coding: utf-8 -*-
 """Test for VideoAlpha Xmodule functional logic.
-These tests data readed from xml, not from mongo.
+These test data read from xml, not from mongo.
 
 We have a ModuleStoreTestCase class defined in
 common/lib/xmodule/xmodule/modulestore/tests/django_utils.py.
@@ -15,28 +15,24 @@ 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"/>
-        <source src=".../mit-3091x/M-3091X-FA12-L21-3_100.webm"/>
-        <source src=".../mit-3091x/M-3091X-FA12-L21-3_100.ogv"/>
+        <source src="example.mp4"/>
+        <source src="example.webm"/>
+        <source src="example.ogv"/>
     </videoalpha>
 """
 
@@ -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
 
+        descriptor = VideoAlphaDescriptor(system, model_data)
 
-class VideoAlphaModuleTest(LogicTest):
-    """Tests for logic of VideoAlpha Xmodule."""
-
-    descriptor_class = VideoAlphaDescriptor
-
-    raw_model_data = {
-        'data': '<videoalpha />'
-    }
+        module = descriptor.xmodule(system)
 
-    def test_get_timeframe_no_parameters(self):
-        xmltree = etree.fromstring('<videoalpha>test</videoalpha>')
-        output = self.xmodule.get_timeframe(xmltree)
-        self.assertEqual(output, ('', ''))
-
-    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, ''))
-
-    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': 'example.mp4',
+            'mp4': 'example.mp4',
+            'webm': 'example.webm',
+            'ogv': 'example.ogv'
+        }
 
-        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()),

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>