Changes based upon code review comments

cms/djangoapps/contentstore/features/component_settings_editor_helpers.py

@@ -105,8 +105,7 @@ def click_component_from_menu(category, component_type, is_advanced):
 
 @world.absorb
 def edit_component_and_select_settings():
-    world.wait_for(lambda _driver: world.css_visible('a.edit-button'))
-    world.css_click('a.edit-button')
+    world.edit_component()
     world.css_click('.settings-button')
 
 
@@ -127,7 +126,6 @@ def select_editor_tab(tab_name):
     world.wait_for_ajax_complete()
 
 
-
 def enter_xml_in_advanced_problem(step, text):
     """
     Edits an advanced problem (assumes only on page),

cms/djangoapps/contentstore/features/pages.py

@@ -50,8 +50,7 @@ def change_name(step, new_name):
     world.css_fill(input_css, new_name)
     if world.is_firefox():
         world.trigger_event(input_css)
-    save_button = 'a.action-save'
-    world.css_click(save_button)
+    world.save_component()
 
 
 @step(u'I drag the first static page to the last$')

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

@@ -286,5 +286,5 @@ def set_weight(weight):
 
 
 def open_high_level_source():
-    world.css_click('a.edit-button')
+    world.edit_component()
     world.css_click('.launch-latex-compiler > a')

cms/djangoapps/contentstore/features/transcripts.py

@@ -218,9 +218,7 @@ def check_transcripts_field(_step, values, field_name):
 
 @step('I save changes$')
 def save_changes(_step):
-    save_css = 'a.action-save'
-    world.css_click(save_css)
-    world.wait_for_ajax_complete()
+    world.save_component()
 
 
 @step('I open tab "([^"]*)"$')

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

@@ -112,11 +112,10 @@ def set_show_captions(step, setting):
     # Prevent cookies from overriding course settings
     world.browser.cookies.delete('hide_captions')
 
-    world.css_click('a.edit-button')
-    world.wait_for(lambda _driver: world.css_visible('a.action-save'))
+    world.edit_component()
     world.select_editor_tab('Advanced')
     world.browser.select('Transcript Display', setting)
-    world.css_click('a.action-save')
+    world.save_component()
 
 
 @step('when I view the video it (.*) show the captions$')
@@ -161,7 +160,7 @@ def correct_video_settings(_step):
 
 @step('my video display name change is persisted on save$')
 def video_name_persisted(step):
-    world.css_click('a.action-save')
+    world.save_component()
     reload_the_page(step)
     world.wait_for_xmodule()
     world.edit_component()

cms/djangoapps/contentstore/views/component.py

@@ -273,7 +273,7 @@ def unit_handler(request, tag=None, package_id=None, branch=None, version_guid=N
             'context_course': course,
             'unit': item,
             'unit_locator': locator,
-            'xblocks': [xblock for xblock in xblocks],
+            'xblocks': xblocks,
             'locators': locators,
             'component_templates': component_templates,
             'draft_preview_link': preview_lms_link,
@@ -320,8 +320,8 @@ def container_handler(request, tag=None, package_id=None, branch=None, version_g
             parent = get_parent_xblock(parent)
         ancestor_xblocks.reverse()
 
-        unit = None if not ancestor_xblocks else ancestor_xblocks[0]
-        unit_publish_state = None if not unit else compute_publish_state(unit)
+        unit = ancestor_xblocks[0] if ancestor_xblocks else None
+        unit_publish_state = compute_publish_state(unit) if unit else None
 
         return render_to_response('container.html', {
             'context_course': course,

cms/static/js/spec/views/modals/base_modal_spec.js

@@ -2,7 +2,7 @@ define(["jquery", "underscore", "js/views/modals/base_modal"],
     function ($, _, BaseModal) {
 
         describe("BaseModal", function() {
-            var baseViewPrototype, MockModal;
+            var baseViewPrototype, MockModal, modal;
 
             MockModal = BaseModal.extend({
                 initialize: function() {
@@ -14,8 +14,14 @@ define(["jquery", "underscore", "js/views/modals/base_modal"],
                 }
             });
 
+            afterEach(function() {
+                if (modal) {
+                    modal.hide();
+                }
+            });
+
             it('is visible after show is called', function () {
-                var modal = new MockModal();
+                modal = new MockModal();
                 modal.render();
                 modal.show();
                 expect($('body')).toHaveClass('modal-window-is-shown');
@@ -24,7 +30,7 @@ define(["jquery", "underscore", "js/views/modals/base_modal"],
             });
 
             it('is invisible after hide is called', function () {
-                var modal = new MockModal();
+                modal = new MockModal();
                 modal.render();
                 modal.show();
                 modal.hide();

cms/static/js/spec/views/xblock_container_spec.js

@@ -40,6 +40,9 @@ define(["jquery", "js/spec_helpers/create_sinon", "js/spec_helpers/edit_helpers"
 
                 afterEach(function() {
                     window.MockXBlock = null;
+                    if (edit_helpers.isShowingModal()) {
+                        edit_helpers.cancelModal();
+                    }
                 });
 
                 mockContainerXBlockHtml = readFixtures('mock/mock-container-xblock.underscore');
@@ -66,14 +69,18 @@ define(["jquery", "js/spec_helpers/create_sinon", "js/spec_helpers/edit_helpers"
                         "resources": []
                     });
                     editButtons = containerView.$('.edit-button');
+                    // The container renders four mock xblocks, so there should be four edit buttons
                     expect(editButtons.length).toBe(4);
                     editButtons.first().click();
+                    // Make sure that the correct xblock is requested to be edited
+                    expect(requests[requests.length - 1].url).toBe(
+                        '/xblock/testCourse/branch/draft/block/html447/studio_view'
+                    );
                     create_sinon.respondWithJson(requests, {
                         html: mockXBlockEditorHtml,
                         "resources": []
                     });
                     expect($('.wrapper-modal-window')).toHaveClass('is-shown');
-                    edit_helpers.cancelModal();
                 });
             });
         });

cms/static/js/spec_helpers/edit_helpers.js

@@ -13,6 +13,7 @@ define(["jquery", "js/spec_helpers/create_sinon", "js/views/modals/edit_xblock",
             editorModeButtonTemplate = readFixtures('editor-mode-button.underscore'),
             installEditTemplates,
             showEditModal,
+            isShowingModal,
             cancelModal;
 
         installEditTemplates = function() {
@@ -39,6 +40,10 @@ define(["jquery", "js/spec_helpers/create_sinon", "js/views/modals/edit_xblock",
             return modal;
         };
 
+        isShowingModal = function() {
+            return $('.wrapper-modal-window').length > 0;
+        };
+
         cancelModal = function(modal) {
             var modalElement, cancelButton;
             if (modal) {
@@ -51,10 +56,10 @@ define(["jquery", "js/spec_helpers/create_sinon", "js/views/modals/edit_xblock",
             cancelButton.click();
         };
 
-
         return {
-            "installEditTemplates": installEditTemplates,
-            "showEditModal": showEditModal,
-            "cancelModal": cancelModal
+            'installEditTemplates': installEditTemplates,
+            'showEditModal': showEditModal,
+            'isShowingModal': isShowingModal,
+            'cancelModal': cancelModal
         };
     });

cms/static/js/views/baseview.js

@@ -43,6 +43,8 @@ define(["jquery", "underscore", "backbone", "js/utils/handle_iframe_binding"],
 
             toggleExpandCollapse: function(event) {
                 var target = $(event.target);
+                // Don't propagate the event as it is possible that two views will both contain
+                // this element, e.g. clicking on the element of a child view container in a parent.
                 event.stopPropagation();
                 event.preventDefault();
                 target.closest('.expand-collapse').toggleClass('expand').toggleClass('collapse');

cms/static/js/views/modals/base_modal.js

 /**
  * This is a base modal implementation that provides common utilities.
  */
-define(["jquery", "underscore", "underscore.string", "gettext", "js/views/baseview"],
-    function($, _, str, gettext, BaseView) {
+define(["jquery", "js/views/baseview"],
+    function($, BaseView) {
         var BaseModal = BaseView.extend({
             options: $.extend({}, BaseView.prototype.options, {
                 type: "prompt",

cms/static/js/views/modals/edit_xblock.js

@@ -20,9 +20,9 @@ define(["jquery", "underscore", "gettext", "js/views/modals/base_modal",
 
             /**
              * Show an edit modal for the specified xblock
-             * @param xblockElement The
-             * @param rootXBlockInfo
-             * @param options
+             * @param xblockElement The element that contains the xblock to be edited.
+             * @param rootXBlockInfo An XBlockInfo model that describes the root xblock on the page.
+             * @param options A standard options object.
              */
             edit: function(xblockElement, rootXBlockInfo, options) {
                 this.xblockElement = xblockElement;
@@ -36,20 +36,17 @@ define(["jquery", "underscore", "gettext", "js/views/modals/base_modal",
             },
 
             render: function() {
-                var xblockInfo = this.xblockInfo;
                 this.$el.html(this.template({
-                    xblockInfo: xblockInfo
+                    xblockInfo: this.xblockInfo
                 }));
             },
 
             displayXBlock: function() {
-                var xblockInfo = this.xblockInfo,
-                    editorView = new XBlockEditorView({
+                this.editorView = new XBlockEditorView({
                     el: this.$('.xblock-editor'),
-                        model: xblockInfo
+                    model: this.xblockInfo
                 });
-                this.editorView = editorView;
-                editorView.render({
+                this.editorView.render({
                     success: _.bind(this.onDisplayXBlock, this)
                 });
             },
@@ -69,13 +66,22 @@ define(["jquery", "underscore", "gettext", "js/views/modals/base_modal",
                 } else {
                     this.$('.modal-window-title').text(title);
                     if (editorView.getMetadataEditor()) {
-                        this.addModeButton('editor', gettext("Editor"));
-                        this.addModeButton('settings', gettext("Settings"));
+                        this.addDefaultModes();
                         this.selectMode(editorView.mode);
                     }
                 }
             },
 
+            addDefaultModes: function() {
+                var defaultModes = this.editorView.getDefaultModes(),
+                    i,
+                    mode;
+                for (i = 0; i < defaultModes.length; i++) {
+                    mode = defaultModes[i];
+                    this.addModeButton(mode.id, mode.name);
+                }
+            },
+
             changeMode: function(event) {
                 var parent = $(event.target.parentElement),
                     mode = parent.data('mode');
@@ -107,7 +113,6 @@ define(["jquery", "underscore", "gettext", "js/views/modals/base_modal",
                 this.editorView.save({
                     success: function() {
                         self.hide();
-                        self.$el.html("");
                         if (refresh) {
                             refresh(xblockInfo);
                         }
@@ -124,24 +129,11 @@ define(["jquery", "underscore", "gettext", "js/views/modals/base_modal",
             },
 
             findXBlockInfo: function(xblockElement, defaultXBlockInfo) {
-                var xblockInfo = defaultXBlockInfo,
-                    locator,
-                    displayName,
-                    category;
+                var xblockInfo = defaultXBlockInfo;
                 if (xblockElement.length > 0) {
-                    locator = xblockElement.data('locator');
-                    displayName = xblockElement.data('display-name');
-                    category = xblockElement.data('category');
-                    if (!displayName) {
-                        displayName = category;
-                        if (!category) {
-                            displayName = gettext('Empty');
-                        }
-                    }
                     xblockInfo = new XBlockInfo({
-                        id: locator,
-                        display_name: displayName,
-                        category: category
+                        id: xblockElement.data('locator'),
+                        category: xblockElement.data('category')
                     });
                 }
                 return xblockInfo;

cms/static/js/views/xblock_container.js

@@ -31,7 +31,7 @@ define(["jquery", "underscore", "js/views/baseview", "js/views/xblock", "js/view
                 noContentElement.addClass('is-hidden');
                 xblockView.$el.addClass('is-hidden');
 
-                // Add actions to any root buttons
+                // Add actions to any top level buttons, e.g. "Edit" of the container itself
                 self.addButtonActions(this.$el);
 
                 // Render the xblock

cms/static/js/views/xblock_editor.js

@@ -38,6 +38,13 @@ define(["jquery", "underscore", "gettext", "js/views/feedback_notification", "js
                 }
             },
 
+            getDefaultModes: function() {
+                return [
+                    { id: 'editor', name: gettext("Editor")},
+                    { id: 'settings', name: gettext("Settings")}
+                ];
+            },
+
             hasCustomTabs: function() {
                 return this.$('.editor-with-tabs').length > 0;
             },

cms/templates/js/mock/mock-container-view.underscore

@@ -7,7 +7,7 @@
         <h1 class="page-header">
             <small class="navigation navigation-parents">
 
-                <a href="/unit/AndyA.ABT101.2014/branch/draft/block/vertical8eb" class="navigation-link navigation-parent">Unit 1</a>
+                <a href="/unit/TestCourse/branch/draft/block/vertical8eb" class="navigation-link navigation-parent">Unit 1</a>
                 <a href="#" class="navigation-link navigation-current">Nested Vertical Test</a>
             </small>
         </h1>
@@ -25,7 +25,7 @@
 <section class="content-area">
 
 <article class="content-primary window">
-<section class="wrapper-xblock level-page" data-locator="AndyA.ABT101.2014/branch/draft/block/vertical131">
+<section class="wrapper-xblock level-page" data-locator="TestCourse/branch/draft/block/vertical131">
 </section>
 <div class="no-container-content is-hidden">
     <p>This page has no content yet.</p>
@@ -39,8 +39,8 @@
         <h3 class="title-3">Publishing Status</h3>
         <p class="copy">
 
-            This content is published with unit <a href="/unit/AndyA.ABT101.2014/branch/draft/block/vertical8eb">Unit 1</a>.
-            Say something useful about <a href="/unit/AndyA.ABT101.2014/branch/draft/block/vertical8eb">Unit 1</a> being in draft or private mode.
+            This content is published with unit <a href="/unit/TestCourse/branch/draft/block/vertical8eb">Unit 1</a>.
+            Say something useful about <a href="/unit/TestCourse/branch/draft/block/vertical8eb">Unit 1</a> being in draft or private mode.
         </p>
     </div>
     <div class="bit">

cms/templates/js/mock/mock-container-xblock.underscore

-<section class="wrapper-xblock level-page" data-locator="AndyA.ABT101.2014/branch/draft/block/vertical131">
+<section class="wrapper-xblock level-page" data-locator="testCourse/branch/draft/block/vertical131">
 <header class="xblock-header">
     <div class="header-details">
         <a href="#" data-tooltip="Expand or Collapse" class="action expand-collapse collapse">
@@ -18,7 +18,7 @@
 <div class="vert-mod">
 <div class="vert vert-0" data-id="i4x://AndyA/ABT101/vertical/2758bbc495dd40d59050da15b40bd9a5">
 
-    <section class="wrapper-xblock level-nesting is-collapsible" data-locator="AndyA.ABT101.2014/branch/draft/block/vertical275">
+    <section class="wrapper-xblock level-nesting is-collapsible" data-locator="testCourse/branch/draft/block/vertical275">
         <header class="xblock-header">
             <div class="header-details">
                 <a href="#" data-tooltip="Expand or Collapse" class="action expand-collapse collapse">
@@ -38,7 +38,7 @@
                 <div class="vert-mod">
                     <div class="vert vert-0" data-id="i4x://AndyA/ABT101/html/4471618afafb45bfb86cbe511973e225">
 
-                        <section class="wrapper-xblock level-element" data-locator="AndyA.ABT101.2014/branch/draft/block/html447" data-display-name="" data-category="html">
+                        <section class="wrapper-xblock level-element" data-locator="testCourse/branch/draft/block/html447" data-display-name="" data-category="html">
                             <header class="xblock-header">
                                 <div class="header-details">
 
@@ -65,7 +65,7 @@
                     </div>
                     <div class="vert vert-1" data-id="i4x://AndyA/ABT101/video/fbd800d0bdbd4cb69ac70c47c9f699e1">
 
-                        <section class="wrapper-xblock level-element" data-locator="AndyA.ABT101.2014/branch/draft/block/videofbd" data-display-name="Video" data-category="video">
+                        <section class="wrapper-xblock level-element" data-locator="testCourse/branch/draft/block/videofbd" data-display-name="Video" data-category="video">
                             <header class="xblock-header">
                                 <div class="header-details">
                                     Video
@@ -163,7 +163,7 @@
 </div>
 <div class="vert vert-1" data-id="i4x://AndyA/ABT101/vertical/c5c8b27c2c5546e784432f3b2b6cf2ea">
 
-    <section class="wrapper-xblock level-nesting is-collapsible" data-locator="AndyA.ABT101.2014/branch/draft/block/verticalc5c">
+    <section class="wrapper-xblock level-nesting is-collapsible" data-locator="testCourse/branch/draft/block/verticalc5c">
         <header class="xblock-header">
             <div class="header-details">
                 <a href="#" data-tooltip="Expand or Collapse" class="action expand-collapse collapse">
@@ -183,7 +183,7 @@
                 <div class="vert-mod">
                     <div class="vert vert-0" data-id="i4x://AndyA/ABT101/html/dd6ef295fda74a639842e1a49c66b2c7">
 
-                        <section class="wrapper-xblock level-element" data-locator="AndyA.ABT101.2014/branch/draft/block/htmldd6" data-display-name="Text" data-category="html">
+                        <section class="wrapper-xblock level-element" data-locator="testCourse/branch/draft/block/htmldd6" data-display-name="Text" data-category="html">
                             <header class="xblock-header">
                                 <div class="header-details">
                                     Text
@@ -210,7 +210,7 @@
                     </div>
                     <div class="vert vert-1" data-id="i4x://AndyA/ABT101/problem/b40ecbe4ed1b4280ae93e2a158edae6f">
 
-                        <section class="wrapper-xblock level-element" data-locator="AndyA.ABT101.2014/branch/draft/block/problemb40" data-display-name="Checkboxes" data-category="problem">
+                        <section class="wrapper-xblock level-element" data-locator="testCourse/branch/draft/block/problemb40" data-display-name="Checkboxes" data-category="problem">
                             <header class="xblock-header">
                                 <div class="header-details">
                                     Checkboxes