Addressed notes from reviewers on Library Pagination.

cms/djangoapps/contentstore/views/item.py

@@ -206,6 +206,7 @@ def xblock_view_handler(request, usage_key_string, view_name):
         store = modulestore()
         xblock = store.get_item(usage_key)
         container_views = ['container_preview', 'reorderable_container_child_preview']
+        library = isinstance(usage_key, LibraryUsageLocator)
 
         # wrap the generated fragment in the xmodule_editor div so that the javascript
         # can bind to it correctly
@@ -234,7 +235,7 @@ def xblock_view_handler(request, usage_key_string, view_name):
             # are being shown in a reorderable container, so the xblock is automatically
             # added to the list.
             reorderable_items = set()
-            if view_name == 'reorderable_container_child_preview':
+            if not library and view_name == 'reorderable_container_child_preview':
                 reorderable_items.add(xblock.location)
 
             paging = None
@@ -258,7 +259,7 @@ def xblock_view_handler(request, usage_key_string, view_name):
                 'is_unit_page': is_unit(xblock),
                 'root_xblock': xblock if (view_name == 'container_preview') else None,
                 'reorderable_items': reorderable_items,
-                'paging': paging
+                'paging': paging,
             }
 
             fragment = get_preview_fragment(request, xblock, context)

cms/static/js/spec/views/pages/container_spec.js

@@ -273,7 +273,7 @@ define(["jquery", "underscore", "underscore.string", "js/common_helpers/ajax_hel
                 });
 
                 describe("xblock operations", function () {
-                    var getGroupElement,
+                    var getGroupElement, paginated,
                         NUM_COMPONENTS_PER_GROUP = 3, GROUP_TO_TEST = "A",
                         allComponentsInGroup = _.map(
                             _.range(NUM_COMPONENTS_PER_GROUP),
@@ -282,6 +282,11 @@ define(["jquery", "underscore", "underscore.string", "js/common_helpers/ajax_hel
                             }
                         );
 
+                    paginated = function () {
+                        return containerPage.enable_paging;
+                    };
+
+
                     getGroupElement = function () {
                         return containerPage.$("[data-locator='locator-group-" + GROUP_TO_TEST + "']");
                     };
@@ -294,6 +299,7 @@ define(["jquery", "underscore", "underscore.string", "js/common_helpers/ajax_hel
                             promptSpy = EditHelpers.createPromptSpy();
                         });
 
+
                         clickDelete = function (componentIndex, clickNo) {
 
                             // find all delete buttons for the given group
@@ -307,21 +313,25 @@ define(["jquery", "underscore", "underscore.string", "js/common_helpers/ajax_hel
                             EditHelpers.confirmPrompt(promptSpy, clickNo);
                         };
 
-                        deleteComponent = function (componentIndex) {
+                        deleteComponent = function (componentIndex, requestOffset) {
                             clickDelete(componentIndex);
                             AjaxHelpers.respondWithJson(requests, {});
 
                             // second to last request contains given component's id (to delete the component)
                             AjaxHelpers.expectJsonRequest(requests, 'DELETE',
                                     '/xblock/locator-component-' + GROUP_TO_TEST + (componentIndex + 1),
-                                null, requests.length - 2);
+                                null, requests.length - requestOffset);
 
                             // final request to refresh the xblock info
                             AjaxHelpers.expectJsonRequest(requests, 'GET', '/xblock/locator-container');
                         };
 
                         deleteComponentWithSuccess = function (componentIndex) {
-                            deleteComponent(componentIndex);
+                            var deleteOffset;
+
+                            deleteOffset = paginated() ? 3 : 2;
+
+                            deleteComponent(componentIndex, deleteOffset);
 
                             // verify the new list of components within the group
                             expectComponents(
@@ -350,9 +360,16 @@ define(["jquery", "underscore", "underscore.string", "js/common_helpers/ajax_hel
                             containerPage.$('.delete-button').first().click();
                             EditHelpers.confirmPrompt(promptSpy);
                             AjaxHelpers.respondWithJson(requests, {});
+                            var deleteOffset;
+
+                            if (paginated()) {
+                                deleteOffset = 3;
+                            } else {
+                                deleteOffset = 2;
+                            }
                             // expect the second to last request to be a delete of the xblock
                             AjaxHelpers.expectJsonRequest(requests, 'DELETE', '/xblock/locator-broken-javascript',
-                                null, requests.length - 2);
+                                null, requests.length - deleteOffset);
                             // expect the last request to be a fetch of the xblock info for the parent container
                             AjaxHelpers.expectJsonRequest(requests, 'GET', '/xblock/locator-container');
                         });
@@ -511,7 +528,7 @@ define(["jquery", "underscore", "underscore.string", "js/common_helpers/ajax_hel
                         });
 
                         describe('Template Picker', function () {
-                            var showTemplatePicker, verifyCreateHtmlComponent;
+                            var showTemplatePicker, verifyCreateHtmlComponent, call_count;
 
                             showTemplatePicker = function () {
                                 containerPage.$('.new-component .new-component-type a.multiple-templates')[0].click();
@@ -519,6 +536,7 @@ define(["jquery", "underscore", "underscore.string", "js/common_helpers/ajax_hel
 
                             verifyCreateHtmlComponent = function (test, templateIndex, expectedRequest) {
                                 var xblockCount;
+                                // call_count = paginated() ? 18: 10;
                                 renderContainerPage(test, mockContainerXBlockHtml);
                                 showTemplatePicker();
                                 xblockCount = containerPage.$('.studio-xblock-wrapper').length;
@@ -557,6 +575,6 @@ define(["jquery", "underscore", "underscore.string", "js/common_helpers/ajax_hel
             { enable_paging: true, page_size: 42 },
             {
                 initial: 'mock/mock-container-paged-xblock.underscore',
-                add_response: 'mock/mock-container-paged-after-add-xblock.underscore'
+                add_response: 'mock/mock-xblock-paged.underscore'
             });
     });

cms/static/js/views/library_container.js

-define(["jquery", "underscore", "js/views/xblock", "js/utils/module", "gettext", "js/views/feedback_notification",
+define(["jquery", "underscore", "js/views/container", "js/utils/module", "gettext", "js/views/feedback_notification",
         "js/views/paging_header", "js/views/paging_footer"],
-    function ($, _, XBlockView, ModuleUtils, gettext, NotificationView, PagingHeader, PagingFooter) {
-        var LibraryContainerView = XBlockView.extend({
+    function ($, _, ContainerView, ModuleUtils, gettext, NotificationView, PagingHeader, PagingFooter) {
+        var LibraryContainerView = ContainerView.extend({
             // Store the request token of the first xblock on the page (which we know was rendered by Studio when
             // the page was generated). Use that request token to filter out user-defined HTML in any
             // child xblocks within the page.
-            requestToken: "",
 
             initialize: function(options){
                 var self = this;
-                XBlockView.prototype.initialize.call(this);
+                ContainerView.prototype.initialize.call(this);
                 this.page_size = this.options.page_size || 10;
-                if (options) {
-                    this.page_reload_callback = options.page_reload_callback;
-                }
+                this.page_reload_callback = options.page_reload_callback || function () {};
                 // emulating Backbone.paginator interface
                 this.collection = {
                     currentPage: 0,
@@ -30,9 +27,6 @@ define(["jquery", "underscore", "js/views/xblock", "js/utils/module", "gettext",
 
             render: function(options) {
                 var eff_options = options || {};
-                if (eff_options.block_added) {
-                    this.collection.currentPage = this.getPageCount(this.collection.totalCount+1) - 1;
-                }
                 eff_options.page_number = typeof eff_options.page_number !== "undefined"
                     ? eff_options.page_number
                     : this.collection.currentPage;
@@ -53,9 +47,8 @@ define(["jquery", "underscore", "js/views/xblock", "js/utils/module", "gettext",
                     success: function(fragment) {
                         self.handleXBlockFragment(fragment, options);
                         self.processPaging({ requested_page: options.page_number });
-                        if (options.paging && self.page_reload_callback){
-                            self.page_reload_callback(self.$el);
-                        }
+                        // This is expected to render the add xblock components menu.
+                        self.page_reload_callback(self.$el)
                     }
                 });
             },
@@ -69,12 +62,12 @@ define(["jquery", "underscore", "js/views/xblock", "js/utils/module", "gettext",
             },
 
             getPageCount: function(total_count){
-                if (total_count==0) return 1;
+                if (total_count===0) return 1;
                 return Math.ceil(total_count / this.page_size);
             },
 
             setPage: function(page_number) {
-                this.render({ page_number: page_number, paging: true });
+                this.render({ page_number: page_number});
             },
 
             nextPage: function() {
@@ -129,32 +122,54 @@ define(["jquery", "underscore", "js/views/xblock", "js/utils/module", "gettext",
             },
 
             xblockReady: function () {
-                XBlockView.prototype.xblockReady.call(this);
+                ContainerView.prototype.xblockReady.call(this);
 
                 this.requestToken = this.$('div.xblock').first().data('request-token');
             },
 
-            refresh: function() { },
+            refresh: function(block_added) {
+                if (block_added) {
+                    this.collection.totalCount += 1;
+                    this.collection._size +=1;
+                    if (this.collection.totalCount == 1) {
+                        this.render();
+                        return
+                    }
+                    this.collection.totalPages = this.getPageCount(this.collection.totalCount);
+                    var new_page = this.collection.totalPages - 1;
+                    // If we're on a new page due to overflow, or this is the first item, set the page.
+                    if (((this.collection.currentPage) != new_page) || this.collection.totalCount == 1) {
+                        this.setPage(new_page);
+                    } else {
+                        this.pagingHeader.render();
+                        this.pagingFooter.render();
+                    }
+                }
+            },
 
             acknowledgeXBlockDeletion: function (locator){
                 this.notifyRuntime('deleted-child', locator);
                 this.collection._size -= 1;
                 this.collection.totalCount -= 1;
-                // pages are counted from 0 - thus currentPage == 1 if we're on second page
-                if (this.collection._size == 0 && this.collection.currentPage >= 1) {
-                    this.setPage(this.collection.currentPage - 1);
-                    this.collection.totalPages -= 1;
-                }
-                else {
+                var current_page = this.collection.currentPage;
+                var total_pages = this.getPageCount(this.collection.totalCount);
+                this.collection.totalPages = total_pages;
+                // Starts counting from 0
+                if ((current_page + 1) > total_pages) {
+                    // The number of total pages has changed. Move down.
+                    // Also, be mindful of the off-by-one.
+                    this.setPage(total_pages - 1)
+                } else if ((current_page + 1) != total_pages) {
+                    // Refresh page to get any blocks shifted from the next page.
+                    this.setPage(current_page)
+                } else {
+                    // We're on the last page, just need to update the numbers in the
+                    // pagination interface.
                     this.pagingHeader.render();
                     this.pagingFooter.render();
                 }
             },
 
-            makeRequestSpecificSelector: function(selector) {
-                return 'div.xblock[data-request-token="' + this.requestToken + '"] > ' + selector;
-            },
-
             sortDisplayName: function() {
                 return "Date added";  // TODO add support for sorting
             }

cms/static/js/views/pages/container.js

@@ -119,8 +119,11 @@ define(["jquery", "underscore", "gettext", "js/views/pages/base_page", "js/views
                         // Notify the runtime that the page has been successfully shown
                         xblockView.notifyRuntime('page-shown', self);
 
-                        // Render the add buttons
-                        self.renderAddXBlockComponents();
+                        // Render the add buttons. Paged containers should do this on their own.
+                        if (!self.enable_paging) {
+                            // Render the add buttons
+                            self.renderAddXBlockComponents();
+                        }
 
                         // Refresh the views now that the xblock is visible
                         self.onXBlockRefresh(xblockView);
@@ -141,8 +144,8 @@ define(["jquery", "underscore", "gettext", "js/views/pages/base_page", "js/views
                 return this.xblockView.model.urlRoot;
             },
 
-            onXBlockRefresh: function(xblockView) {
-                this.xblockView.refresh();
+            onXBlockRefresh: function(xblockView, block_added) {
+                this.xblockView.refresh(block_added);
                 // Update publish and last modified information from the server.
                 this.model.fetch();
             },
@@ -274,10 +277,10 @@ define(["jquery", "underscore", "gettext", "js/views/pages/base_page", "js/views
                     rootLocator = this.xblockView.model.id;
                 if (xblockElement.length === 0 || xblockElement.data('locator') === rootLocator) {
                     this.render({refresh: true, block_added: block_added});
-                } else if (parentElement.hasClass('reorderable-container')) {
-                    this.refreshChildXBlock(xblockElement);
+                } else if (parentElement.hasClass('reorderable-container') || this.enable_paging) {
+                    this.refreshChildXBlock(xblockElement, block_added);
                 } else {
-                    this.refreshXBlock(this.findXBlockElement(parentElement), block_added);
+                    this.refreshXBlock(this.findXBlockElement(parentElement));
                 }
             },
 
@@ -285,9 +288,11 @@ define(["jquery", "underscore", "gettext", "js/views/pages/base_page", "js/views
              * Refresh an xblock element inline on the page, using the specified xblockInfo.
              * Note that the element is removed and replaced with the newly rendered xblock.
              * @param xblockElement The xblock element to be refreshed.
+             * @param block_added Specifies if a block has been added, rather than just needs
+             * refreshing.
              * @returns {jQuery promise} A promise representing the complete operation.
              */
-            refreshChildXBlock: function(xblockElement) {
+            refreshChildXBlock: function(xblockElement, block_added) {
                 var self = this,
                     xblockInfo,
                     TemporaryXBlockView,
@@ -313,7 +318,7 @@ define(["jquery", "underscore", "gettext", "js/views/pages/base_page", "js/views
                 });
                 return temporaryView.render({
                     success: function() {
-                        self.onXBlockRefresh(temporaryView);
+                        self.onXBlockRefresh(temporaryView, block_added);
                         temporaryView.unbind();  // Remove the temporary view
                     }
                 });

cms/static/sass/elements/_pagination.scss

@@ -2,7 +2,7 @@
 // ==========================
 
 %pagination {
-  @include clearfix;
+  @include clearfix();
   display: inline-block;
   width: flex-grid(3, 12);
 
@@ -48,7 +48,7 @@
   }
 
   .nav-label {
-    @extend .sr;
+    @extend %cont-text-sr;
   }
 
   .pagination-form,
@@ -89,7 +89,7 @@
 
     .page-number-label,
     .submit-pagination-form {
-      @extend .sr;
+      @extend %cont-text-sr;
     }
 
     .page-number-input {
@@ -116,4 +116,4 @@
       }
     }
   }
-}
+}
\ No newline at end of file

cms/static/sass/elements/_xblocks.scss

@@ -105,7 +105,7 @@
 
   .container-paging-header {
     .meta-wrap {
-      margin: $baseline $baseline/2;
+      margin: $baseline ($baseline/2);
     }
     .meta {
       @extend %t-copy-sub2;

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

+<div class="studio-xblock-wrapper">
+    <header class="xblock-header">
+        <div class="header-details">
+            <span class="xblock-display-name">Mock XBlock</span>
+        </div>
+        <div class="header-actions">
+            <ul class="actions-list">
+            </ul>
+            <a href="#" class="button action-button notification-action-button" data-notification-action="add-missing-groups">
+                <span class="action-button-text">Add Missing Groups</span>
+            </a>
+        </div>
+    </header>
+    <article class="xblock-render">
+        <div class="xblock xblock-student_view xmodule_display xmodule_VerticalModule"
+             data-runtime-class="PreviewRuntime" data-init="XBlockToXModuleShim" data-runtime-version="1"
+             data-type="None">
+            <p>Mock XBlock</p>
+        </div>
+    </article>
+</div>

common/lib/xmodule/xmodule/library_root_xblock.py

@@ -76,7 +76,7 @@ class LibraryRoot(XBlock):
             fragment.add_frag_resources(rendered_child)
 
             contents.append({
-                'id': child.location.to_deprecated_string(),
+                'id': unicode(child.location),
                 'content': rendered_child.content
             })
 
@@ -85,7 +85,6 @@ class LibraryRoot(XBlock):
                 'items': contents,
                 'xblock_context': context,
                 'can_add': can_add,
-                'can_reorder': False,
                 'first_displayed': item_start,
                 'total_children': children_count,
                 'displayed_children': len(children_to_show)

common/lib/xmodule/xmodule/video_module/video_handlers.py

@@ -155,6 +155,7 @@ class VideoStudentViewHandlers(object):
 
         if transcript_name:
             # Get the asset path for course
+            asset_path = None
             course = self.descriptor.runtime.modulestore.get_course(self.course_id)
             if course.static_asset_path:
                 asset_path = course.static_asset_path