From 70f77ce4bf039c509f0557273df8c4b3b85a8df9 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Thu, 10 Oct 2013 17:26:23 -0400 Subject: [PATCH 1/4] Add pagination to asset retrieval and push sorting down to the query --- cms/djangoapps/contentstore/views/assets.py | 21 ++++++++++++++----- cms/urls.py | 2 +- .../xmodule/xmodule/contentstore/content.py | 2 +- .../lib/xmodule/xmodule/contentstore/mongo.py | 16 ++++++++++---- 4 files changed, 30 insertions(+), 11 deletions(-) diff --git a/cms/djangoapps/contentstore/views/assets.py b/cms/djangoapps/contentstore/views/assets.py index 4f9db0bf4d..f55ccac37e 100644 --- a/cms/djangoapps/contentstore/views/assets.py +++ b/cms/djangoapps/contentstore/views/assets.py @@ -23,6 +23,7 @@ from .access import get_location_and_verify_access from util.json_request import JsonResponse import json from django.utils.translation import ugettext as _ +from pymongo import DESCENDING __all__ = ['asset_index', 'upload_asset'] @@ -30,11 +31,14 @@ __all__ = ['asset_index', 'upload_asset'] @login_required @ensure_csrf_cookie -def asset_index(request, org, course, name): +def asset_index(request, org, course, name, start=None, maxresults=None): """ Display an editable asset library org, course, name: Attributes of the Location for the item to edit + + :param start: which index of the result list to start w/, used for paging results + :param maxresults: maximum results """ location = get_location_and_verify_access(request, org, course, name) @@ -47,10 +51,17 @@ def asset_index(request, org, course, name): course_module = modulestore().get_item(location) course_reference = StaticContent.compute_location(org, course, name) - assets = contentstore().get_all_content_for_course(course_reference) - - # sort in reverse upload date order - assets = sorted(assets, key=lambda asset: asset['uploadDate'], reverse=True) + 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)] + ) asset_json = [] for asset in assets: diff --git a/cms/urls.py b/cms/urls.py index b465b07f32..e9bf93d5e5 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -73,7 +73,7 @@ urlpatterns = ('', # nopep8 url(r'^edit_tabs/(?P[^/]+)/(?P[^/]+)/course/(?P[^/]+)$', 'contentstore.views.edit_tabs', name='edit_tabs'), - url(r'^(?P[^/]+)/(?P[^/]+)/assets/(?P[^/]+)$', + url(r'^(?P[^/]+)/(?P[^/]+)/assets/(?P[^/]+)(/start/(?P\d+))?(/max/(?P\d+))?$', 'contentstore.views.asset_index', name='asset_index'), url(r'^(?P[^/]+)/(?P[^/]+)/assets/(?P[^/]+)/(?P.+)?.*$', 'contentstore.views.assets.update_asset', name='update_asset'), diff --git a/common/lib/xmodule/xmodule/contentstore/content.py b/common/lib/xmodule/xmodule/contentstore/content.py index 495b24426f..44a03341ed 100644 --- a/common/lib/xmodule/xmodule/contentstore/content.py +++ b/common/lib/xmodule/xmodule/contentstore/content.py @@ -168,7 +168,7 @@ class ContentStore(object): def find(self, filename): raise NotImplementedError - def get_all_content_for_course(self, location): + def get_all_content_for_course(self, location, start=0, maxresults=-1, sort=None): ''' Returns a list of all static assets for a course. The return format is a list of dictionary elements. Example: diff --git a/common/lib/xmodule/xmodule/contentstore/mongo.py b/common/lib/xmodule/xmodule/contentstore/mongo.py index 1e788c0a47..98756eccba 100644 --- a/common/lib/xmodule/xmodule/contentstore/mongo.py +++ b/common/lib/xmodule/xmodule/contentstore/mongo.py @@ -130,10 +130,12 @@ class MongoContentStore(ContentStore): def get_all_content_thumbnails_for_course(self, location): return self._get_all_content_for_course(location, get_thumbnails=True) - def get_all_content_for_course(self, location): - return self._get_all_content_for_course(location, get_thumbnails=False) + def get_all_content_for_course(self, location, start=0, maxresults=-1, sort=None): + return self._get_all_content_for_course( + location, start=start, maxresults=maxresults, get_thumbnails=False, sort=sort + ) - def _get_all_content_for_course(self, location, get_thumbnails=False): + def _get_all_content_for_course(self, location, get_thumbnails=False, start=0, maxresults=-1, sort=None): ''' Returns a list of all static assets for a course. The return format is a list of dictionary elements. Example: @@ -156,7 +158,13 @@ class MongoContentStore(ContentStore): course_filter = Location(XASSET_LOCATION_TAG, category="asset" if not get_thumbnails else "thumbnail", course=location.course, org=location.org) # 'borrow' the function 'location_to_query' from the Mongo modulestore implementation - items = self.fs_files.find(location_to_query(course_filter)) + if maxresults > 0: + items = self.fs_files.find( + location_to_query(course_filter), + skip=start, limit=maxresults, sort=sort + ) + else: + items = self.fs_files.find(location_to_query(course_filter), sort=sort) return list(items) def set_attr(self, location, attr, value=True): From 8cf4f80d509a10501b189a40060691fd715616a6 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Fri, 11 Oct 2013 12:58:10 -0400 Subject: [PATCH 2/4] Test 100 assets w/ pagination --- .../contentstore/tests/test_assets.py | 86 ++++++++++++++++++- 1 file changed, 83 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_assets.py b/cms/djangoapps/contentstore/tests/test_assets.py index 2e90955220..386043ef0a 100644 --- a/cms/djangoapps/contentstore/tests/test_assets.py +++ b/cms/djangoapps/contentstore/tests/test_assets.py @@ -6,19 +6,20 @@ Unit tests for the asset upload endpoint. #pylint: disable=W0621 #pylint: disable=W0212 -from datetime import datetime +from datetime import datetime, timedelta from io import BytesIO from pytz import UTC +import json +import re from unittest import TestCase, skip from .utils import CourseTestCase from django.core.urlresolvers import reverse from contentstore.views import assets -from xmodule.contentstore.content import StaticContent +from xmodule.contentstore.content import StaticContent, XASSET_LOCATION_TAG from xmodule.modulestore import Location from xmodule.contentstore.django import contentstore from xmodule.modulestore.django import modulestore from xmodule.modulestore.xml_importer import import_from_xml -import json class AssetsTestCase(CourseTestCase): @@ -147,3 +148,82 @@ 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) + + 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 + ) + 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 + asset_url = reverse( + 'asset_index', + kwargs={ + 'org': self.course.location.org, + 'course': self.course.location.course, + 'name': self.course.location.name + } + ) + resp = self.client.get(asset_url) + self.check_page_content(resp.content, 100) + # get first page of 10 + resp = self.client.get(asset_url + "/start/0/max/10") + last_date = self.check_page_content(resp.content, 10) + # get next of 20 + resp = self.client.get(asset_url + "/start/10/max/20") + last_date = self.check_page_content(resp.content, 20, last_date) \ No newline at end of file From ee2a3750ac02b1370e194403d3cf21f32bd4d75c Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Thu, 17 Oct 2013 09:51:56 -0400 Subject: [PATCH 3/4] Test leaving /start out of url. --- cms/djangoapps/contentstore/tests/test_assets.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_assets.py b/cms/djangoapps/contentstore/tests/test_assets.py index 386043ef0a..8803f85789 100644 --- a/cms/djangoapps/contentstore/tests/test_assets.py +++ b/cms/djangoapps/contentstore/tests/test_assets.py @@ -149,6 +149,7 @@ class LockAssetTestCase(CourseTestCase): 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) @@ -189,6 +190,7 @@ class TestAssetIndex(CourseTestCase): 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: @@ -222,8 +224,8 @@ class TestAssetIndex(CourseTestCase): resp = self.client.get(asset_url) self.check_page_content(resp.content, 100) # get first page of 10 - resp = self.client.get(asset_url + "/start/0/max/10") + resp = self.client.get(asset_url + "/max/10") last_date = self.check_page_content(resp.content, 10) # get next of 20 resp = self.client.get(asset_url + "/start/10/max/20") - last_date = self.check_page_content(resp.content, 20, last_date) \ No newline at end of file + last_date = self.check_page_content(resp.content, 20, last_date) From 214a3bd251ac8f052269f3cdfadffb8ef3cd9e78 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Thu, 17 Oct 2013 09:52:13 -0400 Subject: [PATCH 4/4] pylint/pep8 cleanups --- lms/djangoapps/courseware/courses.py | 26 +++++++++---------- .../courseware/tests/test_courses.py | 13 ++++++---- 2 files changed, 20 insertions(+), 19 deletions(-) diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index 625453aad0..8634ba895d 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -51,6 +51,7 @@ def get_course_by_id(course_id, depth=0): except InvalidLocationError: raise Http404("Invalid location") + def get_course_with_access(user, course_id, action, depth=0): """ Given a course_id, look up the corresponding course descriptor, @@ -85,24 +86,24 @@ def course_image_url(course): if course.static_asset_path or modulestore().get_modulestore_type(course.location.course_id) == XML_MODULESTORE_TYPE: return '/static/' + (course.static_asset_path or getattr(course, 'data_dir', '')) + "/images/course_image.jpg" else: - loc = course.location._replace(tag='c4x', category='asset', name=course.course_image) + loc = course.location.replace(tag='c4x', category='asset', name=course.course_image) _path = StaticContent.get_url_path_from_location(loc) return _path -def find_file(fs, dirs, filename): +def find_file(filesystem, dirs, filename): """ Looks for a filename in a list of dirs on a filesystem, in the specified order. - fs: an OSFS filesystem + filesystem: an OSFS filesystem dirs: a list of path objects filename: a string Returns d / filename if found in dir d, else raises ResourceNotFoundError. """ - for d in dirs: - filepath = path(d) / filename - if fs.exists(filepath): + for directory in dirs: + filepath = path(directory) / filename + if filesystem.exists(filepath): return filepath raise ResourceNotFoundError("Could not find {0}".format(filename)) @@ -146,7 +147,7 @@ def get_course_about_section(course, section_key): request = get_request_for_thread() - loc = course.location._replace(category='about', name=section_key) + loc = course.location.replace(category='about', name=section_key) # Use an empty cache field_data_cache = FieldDataCache([], course.id, request.user) @@ -182,7 +183,6 @@ def get_course_about_section(course, section_key): raise KeyError("Invalid about key " + str(section_key)) - def get_course_info_section(request, course, section_key): """ This returns the snippet of html to be rendered on the course info page, @@ -194,8 +194,6 @@ def get_course_info_section(request, course, section_key): - updates - guest_updates """ - - loc = Location(course.location.tag, course.location.org, course.location.course, 'course_info', section_key) # Use an empty cache @@ -237,13 +235,13 @@ def get_course_syllabus_section(course, section_key): if section_key in ['syllabus', 'guest_syllabus']: try: - fs = course.system.resources_fs + filesys = course.system.resources_fs # first look for a run-specific version dirs = [path("syllabus") / course.url_name, path("syllabus")] - filepath = find_file(fs, dirs, section_key + ".html") - with fs.open(filepath) as htmlFile: + filepath = find_file(filesys, dirs, section_key + ".html") + with filesys.open(filepath) as html_file: return replace_static_urls( - htmlFile.read().decode('utf-8'), + html_file.read().decode('utf-8'), getattr(course, 'data_dir', None), course_id=course.location.course_id, static_asset_path=course.static_asset_path, diff --git a/lms/djangoapps/courseware/tests/test_courses.py b/lms/djangoapps/courseware/tests/test_courses.py index ee05a483a5..71c314a48b 100644 --- a/lms/djangoapps/courseware/tests/test_courses.py +++ b/lms/djangoapps/courseware/tests/test_courses.py @@ -1,4 +1,7 @@ # -*- coding: utf-8 -*- +""" +Tests for course access +""" import mock from django.test import TestCase @@ -9,11 +12,12 @@ from xmodule.modulestore.django import get_default_store_name_for_current_reques CMS_BASE_TEST = 'testcms' + class CoursesTest(TestCase): def test_get_course_by_id_invalid_chars(self): """ Test that `get_course_by_id` throws a 404, rather than - an exception, when faced with unexpected characters + an exception, when faced with unexpected characters (such as unicode characters, and symbols such as = and ' ') """ with self.assertRaises(Http404): @@ -30,13 +34,12 @@ class CoursesTest(TestCase): self.assertEqual("//{}/".format(CMS_BASE_TEST), get_cms_course_link_by_id("too/too/many/slashes")) self.assertEqual("//{}/org/num/course/name".format(CMS_BASE_TEST), get_cms_course_link_by_id('org/num/name')) - @mock.patch('xmodule.modulestore.django.get_current_request_hostname', mock.Mock(return_value='preview.localhost')) - @override_settings(HOSTNAME_MODULESTORE_DEFAULT_MAPPINGS={'preview\.': 'draft'}) - def test_default_modulestore_preview_mapping(self): + @override_settings(HOSTNAME_MODULESTORE_DEFAULT_MAPPINGS={r'preview\.': 'draft'}) + def test_default_modulestore_preview_mapping(self): self.assertEqual(get_default_store_name_for_current_request(), 'draft') @mock.patch('xmodule.modulestore.django.get_current_request_hostname', mock.Mock(return_value='localhost')) - @override_settings(HOSTNAME_MODULESTORE_DEFAULT_MAPPINGS={'preview\.': 'draft'}) + @override_settings(HOSTNAME_MODULESTORE_DEFAULT_MAPPINGS={r'preview\.': 'draft'}) def test_default_modulestore_published_mapping(self): self.assertEqual(get_default_store_name_for_current_request(), 'default')