From f9c45586a4a9361f3e5e8a3d469479f3a6ba52fc Mon Sep 17 00:00:00 2001 From: Andy Armstrong Date: Thu, 5 Dec 2013 11:48:34 -0500 Subject: [PATCH 1/2] Add pagination to Studio's Files and Uploads page These changes implement STUD-813. The commit consists of the following logical changes: - a REST API has been implemented for a course's assets - the page itself now fetches the assets client-side - the Backbone.Paginator library is used to support pagination - the AssetCollection has been refactored to extend Backbone.Paginator.requestPager so that it can be paged - an abstract PagingView class has been added to generalize the communication with a paging REST API - the AssetsView has been reimplemented to extend PagingView - two new child views have been added: - PagingHeader: the paging controls above the list of assets - PagingFooter: the paging controls below the assets --- CHANGELOG.rst | 2 + .../contentstore/tests/test_assets.py | 98 +--- .../contentstore/tests/test_contentstore.py | 12 +- .../tests/test_import_nostatic.py | 6 +- cms/djangoapps/contentstore/views/assets.py | 61 ++- cms/static/coffee/spec/main.coffee | 16 +- cms/static/coffee/spec/main_squire.coffee | 10 +- .../coffee/spec/views/assets_spec.coffee | 98 ++-- cms/static/js/collections/asset.js | 39 +- cms/static/js/spec/create_sinon.js | 12 +- cms/static/js/spec/views/paging_spec.js | 486 ++++++++++++++++++ cms/static/js/views/assets.js | 63 ++- cms/static/js/views/paging.js | 48 ++ cms/static/js/views/paging_footer.js | 57 ++ cms/static/js/views/paging_header.js | 64 +++ cms/static/js_test.yml | 2 + cms/static/js_test_squire.yml | 2 + cms/static/sass/views/_assets.scss | 163 ++++++ cms/templates/asset_index.html | 46 +- cms/templates/base.html | 9 + cms/templates/js/asset-library.underscore | 32 ++ cms/templates/js/paging-footer.underscore | 16 + cms/templates/js/paging-header.underscore | 11 + .../xmodule/xmodule/contentstore/content.py | 5 +- .../lib/xmodule/xmodule/contentstore/mongo.py | 7 +- .../lib/xmodule/xmodule/contentstore/utils.py | 2 +- .../xmodule/modulestore/store_utilities.py | 4 +- .../xmodule/modulestore/tests/test_mongo.py | 2 +- common/static/js/test/i18n.js | 8 + common/static/js/vendor/URI.min.js | 47 ++ .../js/vendor/backbone.paginator.min.js | 4 + requirements/edx/github.txt | 2 +- 32 files changed, 1214 insertions(+), 220 deletions(-) create mode 100644 cms/static/js/spec/views/paging_spec.js create mode 100644 cms/static/js/views/paging.js create mode 100644 cms/static/js/views/paging_footer.js create mode 100644 cms/static/js/views/paging_header.js create mode 100644 cms/templates/js/asset-library.underscore create mode 100644 cms/templates/js/paging-footer.underscore create mode 100644 cms/templates/js/paging-header.underscore create mode 100644 common/static/js/vendor/URI.min.js create mode 100644 common/static/js/vendor/backbone.paginator.min.js diff --git a/CHANGELOG.rst b/CHANGELOG.rst index a66cefdce9..d60e25d427 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,8 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. +Studio: Added pagination to the Files & Uploads page. + Blades: Video player improvements: - Disable edX controls on iPhone/iPod (native controls are used). - Disable unsupported controls (volume, playback rate) on iPad/Android. diff --git a/cms/djangoapps/contentstore/tests/test_assets.py b/cms/djangoapps/contentstore/tests/test_assets.py index 9ed9650b45..59c1cb068b 100644 --- a/cms/djangoapps/contentstore/tests/test_assets.py +++ b/cms/djangoapps/contentstore/tests/test_assets.py @@ -41,7 +41,7 @@ class AssetsTestCase(CourseTestCase): class AssetsToyCourseTestCase(CourseTestCase): """ - Tests the assets returned from assets_handler (full page content) for the toy test course. + Tests the assets returned from assets_handler for the toy test course. """ def test_toy_assets(self): module_store = modulestore('direct') @@ -56,10 +56,17 @@ class AssetsToyCourseTestCase(CourseTestCase): location = loc_mapper().translate_location(course.location.course_id, course.location, False, True) url = location.url_reverse('assets/', '') - resp = self.client.get(url, HTTP_ACCEPT='text/html') - # Test a small portion of the asset data passed to the client. - self.assertContains(resp, "new AssetCollection([{") - self.assertContains(resp, "/c4x/edX/toy/asset/handouts_sample_handout.txt") + self.assert_correct_asset_response(url, 0, 3, 3) + self.assert_correct_asset_response(url + "?page_size=2", 0, 2, 3) + self.assert_correct_asset_response(url + "?page_size=2&page=1", 2, 1, 3) + + def assert_correct_asset_response(self, url, expected_start, expected_length, expected_total): + resp = self.client.get(url, HTTP_ACCEPT='application/json') + json_response = json.loads(resp.content) + assets = json_response['assets'] + self.assertEquals(json_response['start'], expected_start) + self.assertEquals(len(assets), expected_length) + self.assertEquals(json_response['totalCount'], expected_total) class UploadTestCase(CourseTestCase): @@ -82,10 +89,6 @@ class UploadTestCase(CourseTestCase): resp = self.client.post(self.url, {"name": "file.txt"}, "application/json") self.assertEquals(resp.status_code, 400) - def test_get(self): - with self.assertRaises(NotImplementedError): - self.client.get(self.url) - class AssetToJsonTestCase(TestCase): """ @@ -163,80 +166,3 @@ class LockAssetTestCase(CourseTestCase): resp_asset = post_asset_update(False) self.assertFalse(resp_asset['locked']) verify_asset_locked_state(False) - - -class TestAssetIndex(CourseTestCase): - """ - Test getting asset lists via http (Note, the assets don't actually exist) - """ - def setUp(self): - """ - Create fake asset entries for the other tests to use - """ - super(TestAssetIndex, self).setUp() - self.entry_filter = self.create_asset_entries(contentstore(), 100) - location = loc_mapper().translate_location(self.course.location.course_id, self.course.location, False, True) - self.url = location.url_reverse('assets/', '') - - def tearDown(self): - """ - Get rid of the entries - """ - contentstore().fs_files.remove(self.entry_filter) - - def create_asset_entries(self, cstore, number): - """ - Create the fake entries - """ - course_filter = Location( - XASSET_LOCATION_TAG, category='asset', course=self.course.location.course, org=self.course.location.org - ) - # purge existing entries (a bit brutal but hopefully tests are independent enuf to not trip on this) - cstore.fs_files.remove(location_to_query(course_filter)) - base_entry = { - 'displayname': 'foo.jpg', - 'chunkSize': 262144, - 'length': 0, - 'uploadDate': datetime(2012, 1, 2, 0, 0), - 'contentType': 'image/jpeg', - } - for i in range(number): - base_entry['displayname'] = '{:03x}.jpeg'.format(i) - base_entry['uploadDate'] += timedelta(hours=i) - base_entry['_id'] = course_filter.replace(name=base_entry['displayname']).dict() - cstore.fs_files.insert(base_entry) - - return course_filter.dict() - - ASSET_LIST_RE = re.compile(r'AssetCollection\((.*)\);$', re.MULTILINE) - - def check_page_content(self, resp_content, entry_count, last_date=None): - """ - :param entry_count: - :param last_date: - """ - match = self.ASSET_LIST_RE.search(resp_content) - asset_list = json.loads(match.group(1)) - self.assertEqual(len(asset_list), entry_count) - for row in asset_list: - datetext = row['date_added'] - parsed_date = datetime.strptime(datetext, "%b %d, %Y at %H:%M UTC") - if last_date is None: - last_date = parsed_date - else: - self.assertGreaterEqual(last_date, parsed_date) - return last_date - - def test_query_assets(self): - """ - The actual test - """ - # get all - resp = self.client.get(self.url, HTTP_ACCEPT='text/html') - self.check_page_content(resp.content, 100) - # get first page of 10 - resp = self.client.get(self.url + "?max=10", HTTP_ACCEPT='text/html') - last_date = self.check_page_content(resp.content, 10) - # get next of 20 - resp = self.client.get(self.url + "?start=10&max=20", HTTP_ACCEPT='text/html') - self.check_page_content(resp.content, 20, last_date) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index a13742d404..e7e4d508fb 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -176,7 +176,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): Lock an arbitrary asset in the course :param course_location: """ - course_assets = content_store.get_all_content_for_course(course_location) + course_assets,__ = content_store.get_all_content_for_course(course_location) self.assertGreater(len(course_assets), 0, "No assets to lock") content_store.set_attr(course_assets[0]['_id'], 'locked', True) return course_assets[0]['_id'] @@ -585,7 +585,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): self.assertIsNotNone(course) # make sure we have some assets in our contentstore - all_assets = content_store.get_all_content_for_course(course_location) + all_assets,__ = content_store.get_all_content_for_course(course_location) self.assertGreater(len(all_assets), 0) # make sure we have some thumbnails in our contentstore @@ -698,7 +698,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): # make sure there's something in the trashcan course_location = CourseDescriptor.id_to_location('edX/toy/6.002_Spring_2012') - all_assets = trash_store.get_all_content_for_course(course_location) + all_assets,__ = trash_store.get_all_content_for_course(course_location) self.assertGreater(len(all_assets), 0) # make sure we have some thumbnails in our trashcan @@ -713,8 +713,9 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): empty_asset_trashcan([course_location]) # make sure trashcan is empty - all_assets = trash_store.get_all_content_for_course(course_location) + all_assets,count = trash_store.get_all_content_for_course(course_location) self.assertEqual(len(all_assets), 0) + self.assertEqual(count, 0) all_thumbnails = trash_store.get_all_content_thumbnails_for_course(course_location) self.assertEqual(len(all_thumbnails), 0) @@ -923,8 +924,9 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): self.assertEqual(len(items), 0) # assert that all content in the asset library is also deleted - assets = content_store.get_all_content_for_course(location) + assets,count = content_store.get_all_content_for_course(location) self.assertEqual(len(assets), 0) + self.assertEqual(count, 0) def verify_content_existence(self, store, root_dir, location, dirname, category_name, filename_suffix=''): filesystem = OSFS(root_dir / 'test_export') diff --git a/cms/djangoapps/contentstore/tests/test_import_nostatic.py b/cms/djangoapps/contentstore/tests/test_import_nostatic.py index 510f0ca6f7..4b81466009 100644 --- a/cms/djangoapps/contentstore/tests/test_import_nostatic.py +++ b/cms/djangoapps/contentstore/tests/test_import_nostatic.py @@ -84,9 +84,10 @@ class ContentStoreImportNoStaticTest(ModuleStoreTestCase): _, content_store, course, course_location = self.load_test_import_course() # make sure we have ONE asset in our contentstore ("should_be_imported.html") - all_assets = content_store.get_all_content_for_course(course_location) + all_assets,count = content_store.get_all_content_for_course(course_location) print "len(all_assets)=%d" % len(all_assets) self.assertEqual(len(all_assets), 1) + self.assertEqual(count, 1) content = None try: @@ -114,9 +115,10 @@ class ContentStoreImportNoStaticTest(ModuleStoreTestCase): module_store.get_item(course_location) # make sure we have NO assets in our contentstore - all_assets = content_store.get_all_content_for_course(course_location) + all_assets,count = content_store.get_all_content_for_course(course_location) print "len(all_assets)=%d" % len(all_assets) self.assertEqual(len(all_assets), 0) + self.assertEqual(count, 0) def test_no_static_link_rewrites_on_import(self): module_store = modulestore('direct') diff --git a/cms/djangoapps/contentstore/views/assets.py b/cms/djangoapps/contentstore/views/assets.py index decdd85d30..556465c9d2 100644 --- a/cms/djangoapps/contentstore/views/assets.py +++ b/cms/djangoapps/contentstore/views/assets.py @@ -41,9 +41,10 @@ def assets_handler(request, tag=None, package_id=None, branch=None, version_guid deleting assets, and changing the "locked" state of an asset. GET - html: return html page of all course assets (note though that a range of assets can be requested using start - and max query parameters) - json: not currently supported + html: return html page which will show all course assets. Note that only the asset container + is returned and that the actual assets are filled in with a client-side request. + json: returns a page of assets. A page parameter specifies the desired page, and the + optional page_size parameter indicates the number of items per page (defaults to 50). POST json: create (or update?) an asset. The only updating that can be done is changing the lock state. PUT @@ -55,9 +56,10 @@ def assets_handler(request, tag=None, package_id=None, branch=None, version_guid if not has_access(request.user, location): raise PermissionDenied() - if 'application/json' in request.META.get('HTTP_ACCEPT', 'application/json'): + response_format = request.REQUEST.get('format', 'html') + if response_format == 'json' or 'application/json' in request.META.get('HTTP_ACCEPT', 'application/json'): if request.method == 'GET': - raise NotImplementedError('coming soon') + return _assets_json(request, location) else: return _update_asset(request, location, asset_id) elif request.method == 'GET': # assume html @@ -73,22 +75,32 @@ def _asset_index(request, location): Supports start (0-based index into the list of assets) and max query parameters. """ old_location = loc_mapper().translate_locator_to_location(location) - course_module = modulestore().get_item(old_location) - maxresults = request.REQUEST.get('max', None) - start = request.REQUEST.get('start', None) + + return render_to_response('asset_index.html', { + 'context_course': course_module, + 'asset_callback_url': location.url_reverse('assets/', '') + }) + + +def _assets_json(request, location): + """ + Display an editable asset library. + + Supports start (0-based index into the list of assets) and max query parameters. + """ + requested_page = int(request.REQUEST.get('page', 0)) + requested_page_size = int(request.REQUEST.get('page_size', 50)) + current_page = max(requested_page, 0) + start = current_page * requested_page_size + + old_location = loc_mapper().translate_locator_to_location(location) + course_reference = StaticContent.compute_location(old_location.org, old_location.course, old_location.name) - if maxresults is not None: - maxresults = int(maxresults) - start = int(start) if start else 0 - assets = contentstore().get_all_content_for_course( - course_reference, start=start, maxresults=maxresults, - sort=[('uploadDate', DESCENDING)] - ) - else: - assets = contentstore().get_all_content_for_course( - course_reference, sort=[('uploadDate', DESCENDING)] - ) + assets, total_count = contentstore().get_all_content_for_course( + course_reference, start=start, maxresults=requested_page_size, sort=[('uploadDate', DESCENDING)] + ) + end = start + len(assets) asset_json = [] for asset in assets: @@ -101,10 +113,13 @@ def _asset_index(request, location): asset_locked = asset.get('locked', False) asset_json.append(_get_asset_json(asset['displayname'], asset['uploadDate'], asset_location, thumbnail_location, asset_locked)) - return render_to_response('asset_index.html', { - 'context_course': course_module, - 'asset_list': json.dumps(asset_json), - 'asset_callback_url': location.url_reverse('assets/', '') + return JsonResponse({ + 'start': start, + 'end': end, + 'page': current_page, + 'pageSize': requested_page_size, + 'totalCount': total_count, + 'assets': asset_json }) diff --git a/cms/static/coffee/spec/main.coffee b/cms/static/coffee/spec/main.coffee index 7c5a6f93f3..4e3cf838c1 100644 --- a/cms/static/coffee/spec/main.coffee +++ b/cms/static/coffee/spec/main.coffee @@ -24,6 +24,7 @@ requirejs.config({ "underscore.string": "xmodule_js/common_static/js/vendor/underscore.string.min", "backbone": "xmodule_js/common_static/js/vendor/backbone-min", "backbone.associations": "xmodule_js/common_static/js/vendor/backbone-associations-min", + "backbone.paginator": "xmodule_js/common_static/js/vendor/backbone.paginator.min", "tinymce": "xmodule_js/common_static/js/vendor/tiny_mce/tiny_mce", "jquery.tinymce": "xmodule_js/common_static/js/vendor/tiny_mce/jquery.tinymce", "xmodule": "xmodule_js/src/xmodule", @@ -38,6 +39,7 @@ requirejs.config({ "jasmine.async": "xmodule_js/common_static/js/vendor/jasmine.async", "draggabilly": "xmodule_js/common_static/js/vendor/draggabilly.pkgd", "domReady": "xmodule_js/common_static/js/vendor/domReady", + "URI": "xmodule_js/common_static/js/vendor/URI.min", "mathjax": "//edx-static.s3.amazonaws.com/mathjax-MathJax-727332c/MathJax.js?config=TeX-MML-AM_HTMLorMML-full&delayStartupUntil=configured", "youtube": "//www.youtube.com/player_api?noext", @@ -115,6 +117,10 @@ requirejs.config({ deps: ["backbone"], exports: "Backbone.Associations" }, + "backbone.paginator": { + deps: ["backbone"], + exports: "Backbone.Paginator" + }, "youtube": { exports: "YT" }, @@ -139,6 +145,9 @@ requirejs.config({ ] MathJax.Hub.Configured() }, + "URI": { + exports: "URI" + }, "xmodule": { exports: "XModule" }, @@ -197,10 +206,13 @@ define([ "js/spec/transcripts/videolist_spec", "js/spec/transcripts/message_manager_spec", "js/spec/transcripts/file_uploader_spec", - "js/spec/utils/module_spec", "js/spec/models/explicit_url_spec" - "js/spec/views/baseview_spec", + "js/spec/utils/handle_iframe_binding_spec", + "js/spec/utils/module_spec", + + "js/spec/views/baseview_spec", + "js/spec/views/paging_spec", # these tests are run separate in the cms-squire suite, due to process # isolation issues with Squire.js diff --git a/cms/static/coffee/spec/main_squire.coffee b/cms/static/coffee/spec/main_squire.coffee index 377ddf2db3..a8e052fd2e 100644 --- a/cms/static/coffee/spec/main_squire.coffee +++ b/cms/static/coffee/spec/main_squire.coffee @@ -23,6 +23,7 @@ requirejs.config({ "underscore.string": "xmodule_js/common_static/js/vendor/underscore.string.min", "backbone": "xmodule_js/common_static/js/vendor/backbone-min", "backbone.associations": "xmodule_js/common_static/js/vendor/backbone-associations-min", + "backbone.paginator": "xmodule_js/common_static/js/vendor/backbone.paginator.min", "tinymce": "xmodule_js/common_static/js/vendor/tiny_mce/tiny_mce", "jquery.tinymce": "xmodule_js/common_static/js/vendor/tiny_mce/jquery.tinymce", "xmodule": "xmodule_js/src/xmodule", @@ -34,6 +35,7 @@ requirejs.config({ "jasmine.async": "xmodule_js/common_static/js/vendor/jasmine.async", "draggabilly": "xmodule_js/common_static/js/vendor/draggabilly.pkgd", "domReady": "xmodule_js/common_static/js/vendor/domReady", + "URI": "xmodule_js/common_static/js/vendor/URI.min", "mathjax": "//edx-static.s3.amazonaws.com/mathjax-MathJax-727332c/MathJax.js?config=TeX-MML-AM_HTMLorMML-full&delayStartupUntil=configured", "youtube": "//www.youtube.com/player_api?noext", @@ -106,6 +108,10 @@ requirejs.config({ deps: ["backbone"], exports: "Backbone.Associations" }, + "backbone.paginator": { + deps: ["backbone"], + exports: "Backbone.Paginator" + }, "youtube": { exports: "YT" }, @@ -130,6 +136,9 @@ requirejs.config({ ] MathJax.Hub.Configured(); }, + "URI": { + exports: "URI" + }, "xmodule": { exports: "XModule" }, @@ -166,4 +175,3 @@ jasmine.getFixtures().fixturesPath += 'coffee/fixtures' define([ "coffee/spec/views/assets_spec" ]) - diff --git a/cms/static/coffee/spec/views/assets_spec.coffee b/cms/static/coffee/spec/views/assets_spec.coffee index 36ebef42bb..e00e30c266 100644 --- a/cms/static/coffee/spec/views/assets_spec.coffee +++ b/cms/static/coffee/spec/views/assets_spec.coffee @@ -2,7 +2,10 @@ define ["jasmine", "js/spec/create_sinon", "squire"], (jasmine, create_sinon, Squire) -> feedbackTpl = readFixtures('system-feedback.underscore') + assetLibraryTpl = readFixtures('asset-library.underscore') assetTpl = readFixtures('asset.underscore') + pagingHeaderTpl = readFixtures('paging-header.underscore') + pagingFooterTpl = readFixtures('paging-footer.underscore') describe "Asset view", -> beforeEach -> @@ -44,7 +47,7 @@ define ["jasmine", "js/spec/create_sinon", "squire"], spyOn(@model, "save").andCallThrough() @collection = new AssetCollection([@model]) - @collection.url = "update-asset-url" + @collection.url = "assets-url" @view = new AssetView({model: @model}) waitsFor (=> @view), "AssetView was not created", 1000 @@ -131,7 +134,10 @@ define ["jasmine", "js/spec/create_sinon", "squire"], describe "Assets view", -> beforeEach -> - setFixtures($(" +% for template_name in ["asset-library", "asset", "paging-header", "paging-footer"]: + +% endfor <%block name="jsextra">