From 2f63a9b4032521c6e1180398be1af5904911109c Mon Sep 17 00:00:00 2001 From: kimth Date: Sun, 19 Aug 2012 20:12:01 -0400 Subject: [PATCH 01/32] Limit number of files that can be uploaded at once --- lms/djangoapps/courseware/module_render.py | 6 ++++++ lms/envs/common.py | 1 + 2 files changed, 7 insertions(+) diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 53c7e453dd..7a23927504 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -379,6 +379,12 @@ def modx_dispatch(request, dispatch=None, id=None, course_id=None): if request.FILES: for fileinput_id in request.FILES.keys(): inputfiles = request.FILES.getlist(fileinput_id) + + if len(inputfiles) > settings.MAX_FILEUPLOADS_PER_INPUT: + too_many_files_msg = 'Submission aborted! Maximum %d files may be submitted at once' %\ + settings.MAX_FILEUPLOADS_PER_INPUT + return HttpResponse(json.dumps({'success': too_many_files_msg})) + for inputfile in inputfiles: if inputfile.size > settings.STUDENT_FILEUPLOAD_MAX_SIZE: # Bytes file_too_big_msg = 'Submission aborted! Your file "%s" is too large (max size: %d MB)' %\ diff --git a/lms/envs/common.py b/lms/envs/common.py index c412a3c8cd..14a9627b40 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -141,6 +141,7 @@ TEMPLATE_CONTEXT_PROCESSORS = ( ) STUDENT_FILEUPLOAD_MAX_SIZE = 4*1000*1000 # 4 MB +MAX_FILEUPLOADS_PER_INPUT = 10 # FIXME: # We should have separate S3 staged URLs in case we need to make changes to From 891797a3bb7e58df27c27dda1ceb82420711a57a Mon Sep 17 00:00:00 2001 From: kimth Date: Sun, 19 Aug 2012 20:38:10 -0400 Subject: [PATCH 02/32] Submission of files through requests.POST depletes the file pointer. Need to rewind for replay --- common/lib/capa/capa/xqueue_interface.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/common/lib/capa/capa/xqueue_interface.py b/common/lib/capa/capa/xqueue_interface.py index 10f36636b8..2930eb682d 100644 --- a/common/lib/capa/capa/xqueue_interface.py +++ b/common/lib/capa/capa/xqueue_interface.py @@ -83,6 +83,9 @@ class XQueueInterface(object): if error and (msg == 'login_required'): # Log in, then try again self._login() + if files_to_upload is not None: + for f in files_to_upload: # Need to rewind file pointers + f.seek(0) (error, msg) = self._send_to_queue(header, body, files_to_upload) return (error, msg) From c6c95c63ac2eedcdb9b906231ef25ffb58325b5b Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Sun, 19 Aug 2012 23:48:37 -0400 Subject: [PATCH 03/32] Keep each course's descriptors separate * apply policy per-course, even if multiple courses share course and org fields * keep descriptors separate in xml store, so that if two such courses change the same module in different ways, it works. Such edits will need to merged on CMS import... * add get_instance(course_id, location) method to replace get_item(location). Update all the call sites * tests, including a 2nd toy course with same course and org. --- common/djangoapps/student/views.py | 2 +- common/lib/xmodule/xmodule/course_module.py | 4 ++ .../xmodule/xmodule/modulestore/__init__.py | 7 +++ .../lib/xmodule/xmodule/modulestore/mongo.py | 7 +++ common/lib/xmodule/xmodule/modulestore/xml.py | 59 +++++++++++++++---- .../xmodule/modulestore/xml_importer.py | 31 +++++----- common/lib/xmodule/xmodule/tests/__init__.py | 1 + .../lib/xmodule/xmodule/tests/test_export.py | 12 ++-- .../lib/xmodule/xmodule/tests/test_import.py | 45 ++++++++++++++ common/test/data/two_toys/README | 1 + .../test/data/two_toys/chapter/Overview.xml | 4 ++ common/test/data/two_toys/course.xml | 1 + .../data/two_toys/course/TT_2012_Fall.xml | 3 + .../data/two_toys/policies/TT_2012_Fall.json | 23 ++++++++ .../data/two_toys/video/Video_Resources.xml | 1 + common/test/data/two_toys/video/Welcome.xml | 1 + .../two_toys/videosequence/Toy_Videos.xml | 3 + lms/djangoapps/courseware/courses.py | 2 +- lms/djangoapps/courseware/grades.py | 5 +- .../management/commands/check_course.py | 5 +- .../management/commands/clean_xml.py | 19 ++++-- lms/djangoapps/courseware/module_render.py | 34 +++++------ lms/djangoapps/courseware/views.py | 10 ++-- 23 files changed, 215 insertions(+), 65 deletions(-) create mode 100644 common/test/data/two_toys/README create mode 100644 common/test/data/two_toys/chapter/Overview.xml create mode 100644 common/test/data/two_toys/course.xml create mode 100644 common/test/data/two_toys/course/TT_2012_Fall.xml create mode 100644 common/test/data/two_toys/policies/TT_2012_Fall.json create mode 100644 common/test/data/two_toys/video/Video_Resources.xml create mode 100644 common/test/data/two_toys/video/Welcome.xml create mode 100644 common/test/data/two_toys/videosequence/Toy_Videos.xml diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index a99b46fd13..cab57e6819 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -102,7 +102,7 @@ def main_index(request, extra_context={}, user=None): def course_from_id(course_id): """Return the CourseDescriptor corresponding to this course_id""" course_loc = CourseDescriptor.id_to_location(course_id) - return modulestore().get_item(course_loc) + return modulestore().get_instance(course_id, course_loc) def press(request): diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index 4729de905d..1f12debfa8 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -134,6 +134,10 @@ class CourseDescriptor(SequenceDescriptor): 'all_descriptors' : all_descriptors,} + @staticmethod + def make_id(org, course, url_name): + return '/'.join([org, course, url_name]) + @staticmethod def id_to_location(course_id): '''Convert the given course_id (org/course/name) to a location object. diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index 6c77127d7e..8783fa7ad7 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -223,6 +223,13 @@ class ModuleStore(object): """ raise NotImplementedError + def get_instance(self, course_id, location): + """ + Get an instance of this location, with policy for course_id applied. + TODO (vshnayder): this may want to live outside the modulestore eventually + """ + raise NotImplementedError + def get_item_errors(self, location): """ Return a list of (msg, exception-or-None) errors that the modulestore diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index caada39f3e..7aa05e474f 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -217,6 +217,13 @@ class MongoModuleStore(ModuleStoreBase): item = self._find_one(location) return self._load_items([item], depth)[0] + def get_instance(self, course_id, location): + """ + TODO (vshnayder): implement policy tracking in mongo. + For now, just delegate to get_item and ignore policy. + """ + return self.get_item(location) + def get_items(self, location, depth=0): items = self.collection.find( location_to_query(location), diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index a77266113f..e0fecb243d 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -3,6 +3,7 @@ import logging import os import re +from collections import defaultdict from fs.osfs import OSFS from importlib import import_module from lxml import etree @@ -33,7 +34,7 @@ def clean_out_mako_templating(xml_string): return xml_string class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): - def __init__(self, xmlstore, org, course, course_dir, + def __init__(self, xmlstore, course_id, course_dir, policy, error_tracker, **kwargs): """ A class that handles loading from xml. Does some munging to ensure that @@ -43,6 +44,7 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): """ self.unnamed_modules = 0 self.used_slugs = set() + self.org, self.course, self.url_name = course_id.split('/') def process_xml(xml): """Takes an xml string, and returns a XModuleDescriptor created from @@ -80,21 +82,24 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): xml_data.set('url_name', slug) descriptor = XModuleDescriptor.load_from_xml( - etree.tostring(xml_data), self, org, - course, xmlstore.default_class) + etree.tostring(xml_data), self, self.org, + self.course, xmlstore.default_class) #log.debug('==> importing descriptor location %s' % # repr(descriptor.location)) descriptor.metadata['data_dir'] = course_dir - xmlstore.modules[descriptor.location] = descriptor + xmlstore.modules[course_id][descriptor.location] = descriptor if xmlstore.eager: descriptor.get_children() return descriptor render_template = lambda: '' - load_item = xmlstore.get_item + # TODO (vshnayder): we are somewhat architecturally confused in the loading code: + # load_item should actually be get_instance, because it expects the course-specific + # policy to be loaded. For now, just add the course_id here... + load_item = lambda location: xmlstore.get_instance(course_id, location) resources_fs = OSFS(xmlstore.data_dir / course_dir) MakoDescriptorSystem.__init__(self, load_item, resources_fs, @@ -127,7 +132,7 @@ class XMLModuleStore(ModuleStoreBase): self.eager = eager self.data_dir = path(data_dir) - self.modules = {} # location -> XModuleDescriptor + self.modules = defaultdict(dict) # course_id -> dict(location -> XModuleDescriptor) self.courses = {} # course_dir -> XModuleDescriptor for the course if default_class is None: @@ -236,14 +241,24 @@ class XMLModuleStore(ModuleStoreBase): tracker(msg) course = course_dir - url_name = course_data.get('url_name') + url_name = course_data.get('url_name', course_data.get('slug')) if url_name: policy_path = self.data_dir / course_dir / 'policies' / '{0}.json'.format(url_name) policy = self.load_policy(policy_path, tracker) else: policy = {} + # VS[compat] : 'name' is deprecated, but support it for now... + if course_data.get('name'): + url_name = Location.clean(course_data.get('name')) + tracker("'name' is deprecated for module xml. Please use " + "display_name and url_name.") + else: + raise ValueError("Can't load a course without a 'url_name' " + "(or 'name') set. Set url_name.") - system = ImportSystem(self, org, course, course_dir, policy, tracker) + + course_id = CourseDescriptor.make_id(org, course, url_name) + system = ImportSystem(self, course_id, course_dir, policy, tracker) course_descriptor = system.process_xml(etree.tostring(course_data)) @@ -257,6 +272,27 @@ class XMLModuleStore(ModuleStoreBase): return course_descriptor + def get_instance(self, course_id, location, depth=0): + """ + Returns an XModuleDescriptor instance for the item at + location, with the policy for course_id. (In case two xml + dirs have different content at the same location, return the + one for this course_id.) + + If any segment of the location is None except revision, raises + xmodule.modulestore.exceptions.InsufficientSpecificationError + + If no object is found at that location, raises + xmodule.modulestore.exceptions.ItemNotFoundError + + location: Something that can be passed to Location + """ + location = Location(location) + try: + return self.modules[course_id][location] + except KeyError: + raise ItemNotFoundError(location) + def get_item(self, location, depth=0): """ Returns an XModuleDescriptor instance for the item at location. @@ -271,11 +307,8 @@ class XMLModuleStore(ModuleStoreBase): location: Something that can be passed to Location """ - location = Location(location) - try: - return self.modules[location] - except KeyError: - raise ItemNotFoundError(location) + raise NotImplementedError("XMLModuleStores can't guarantee that definitions" + " are unique. Use get_instance.") def get_courses(self, depth=0): diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index 891db7e994..89f94d8cdb 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -22,21 +22,22 @@ def import_from_xml(store, data_dir, course_dirs=None, eager=True, eager=eager, course_dirs=course_dirs ) - for module in module_store.modules.itervalues(): + for course_id in module_store.modules.keys(): + for module in module_store.modules[course_id].itervalues(): - # TODO (cpennington): This forces import to overrite the same items. - # This should in the future create new revisions of the items on import - try: - store.create_item(module.location) - except DuplicateItemError: - log.exception('Item already exists at %s' % module.location.url()) - pass - if 'data' in module.definition: - store.update_item(module.location, module.definition['data']) - if 'children' in module.definition: - store.update_children(module.location, module.definition['children']) - # NOTE: It's important to use own_metadata here to avoid writing - # inherited metadata everywhere. - store.update_metadata(module.location, dict(module.own_metadata)) + # TODO (cpennington): This forces import to overrite the same items. + # This should in the future create new revisions of the items on import + try: + store.create_item(module.location) + except DuplicateItemError: + log.exception('Item already exists at %s' % module.location.url()) + pass + if 'data' in module.definition: + store.update_item(module.location, module.definition['data']) + if 'children' in module.definition: + store.update_children(module.location, module.definition['children']) + # NOTE: It's important to use own_metadata here to avoid writing + # inherited metadata everywhere. + store.update_metadata(module.location, dict(module.own_metadata)) return module_store diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index a3eac0258e..2a380bb8be 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -8,6 +8,7 @@ import unittest import os import fs +import fs.osfs import json import json diff --git a/common/lib/xmodule/xmodule/tests/test_export.py b/common/lib/xmodule/xmodule/tests/test_export.py index 268c3d1062..2520d95937 100644 --- a/common/lib/xmodule/xmodule/tests/test_export.py +++ b/common/lib/xmodule/xmodule/tests/test_export.py @@ -84,20 +84,22 @@ class RoundTripTestCase(unittest.TestCase): strip_filenames(exported_course) self.assertEquals(initial_course, exported_course) + self.assertEquals(initial_course.id, exported_course.id) + course_id = initial_course.id print "Checking key equality" - self.assertEquals(sorted(initial_import.modules.keys()), - sorted(second_import.modules.keys())) + self.assertEquals(sorted(initial_import.modules[course_id].keys()), + sorted(second_import.modules[course_id].keys())) print "Checking module equality" - for location in initial_import.modules.keys(): + for location in initial_import.modules[course_id].keys(): print "Checking", location if location.category == 'html': print ("Skipping html modules--they can't import in" " final form without writing files...") continue - self.assertEquals(initial_import.modules[location], - second_import.modules[location]) + self.assertEquals(initial_import.modules[course_id][location], + second_import.modules[course_id][location]) def setUp(self): diff --git a/common/lib/xmodule/xmodule/tests/test_import.py b/common/lib/xmodule/xmodule/tests/test_import.py index 2061b63fb6..34e4767a62 100644 --- a/common/lib/xmodule/xmodule/tests/test_import.py +++ b/common/lib/xmodule/xmodule/tests/test_import.py @@ -207,3 +207,48 @@ class ImportTestCase(unittest.TestCase): check_for_key(key, c) check_for_key('graceperiod', course) + + + def test_policy_loading(self): + """Make sure that when two courses share content with the same + org and course names, policy applies to the right one.""" + + def get_course(name): + print "Importing {0}".format(name) + + modulestore = XMLModuleStore(DATA_DIR, eager=True, course_dirs=[name]) + courses = modulestore.get_courses() + self.assertEquals(len(courses), 1) + return courses[0] + + toy = get_course('toy') + two_toys = get_course('two_toys') + + self.assertEqual(toy.url_name, "2012_Fall") + self.assertEqual(two_toys.url_name, "TT_2012_Fall") + + toy_ch = toy.get_children()[0] + two_toys_ch = two_toys.get_children()[0] + + self.assertEqual(toy_ch.display_name, "Overview") + self.assertEqual(two_toys_ch.display_name, "Two Toy Overview") + + + def test_definition_loading(self): + """When two courses share the same org and course name and + both have a module with the same url_name, the definitions shouldn't clash. + + TODO (vshnayder): once we have a CMS, this shouldn't + happen--locations should uniquely name definitions. But in + our imperfect XML world, it can (and likely will) happen.""" + + modulestore = XMLModuleStore(DATA_DIR, eager=True, course_dirs=['toy', 'two_toys']) + + toy_id = "edX/toy/2012_Fall" + two_toy_id = "edX/toy/TT_2012_Fall" + + location = Location(["i4x", "edX", "toy", "video", "Welcome"]) + toy_video = modulestore.get_instance(toy_id, location) + two_toy_video = modulestore.get_instance(two_toy_id, location) + self.assertEqual(toy_video.metadata['youtube'], "1.0:p2Q6BrNhdh8") + self.assertEqual(two_toy_video.metadata['youtube'], "1.0:p2Q6BrNhdh9") diff --git a/common/test/data/two_toys/README b/common/test/data/two_toys/README new file mode 100644 index 0000000000..ef1a501ee8 --- /dev/null +++ b/common/test/data/two_toys/README @@ -0,0 +1 @@ +A copy of the toy course, with different metadata. Used to test policy loading for course with identical org course fields and shared content. diff --git a/common/test/data/two_toys/chapter/Overview.xml b/common/test/data/two_toys/chapter/Overview.xml new file mode 100644 index 0000000000..a738a9eec3 --- /dev/null +++ b/common/test/data/two_toys/chapter/Overview.xml @@ -0,0 +1,4 @@ + + + diff --git a/common/test/data/two_toys/course.xml b/common/test/data/two_toys/course.xml new file mode 100644 index 0000000000..655859ab8b --- /dev/null +++ b/common/test/data/two_toys/course.xml @@ -0,0 +1 @@ + diff --git a/common/test/data/two_toys/course/TT_2012_Fall.xml b/common/test/data/two_toys/course/TT_2012_Fall.xml new file mode 100644 index 0000000000..b6dbd48b4c --- /dev/null +++ b/common/test/data/two_toys/course/TT_2012_Fall.xml @@ -0,0 +1,3 @@ + + + diff --git a/common/test/data/two_toys/policies/TT_2012_Fall.json b/common/test/data/two_toys/policies/TT_2012_Fall.json new file mode 100644 index 0000000000..2831767b32 --- /dev/null +++ b/common/test/data/two_toys/policies/TT_2012_Fall.json @@ -0,0 +1,23 @@ +{ + "course/TT_2012_Fall": { + "graceperiod": "2 days 5 hours 59 minutes 59 seconds", + "start": "2015-07-17T12:00", + "display_name": "Two Toys Course" + }, + "chapter/Overview": { + "display_name": "Two Toy Overview" + }, + "videosequence/Toy_Videos": { + "display_name": "Toy Videos", + "format": "Lecture Sequence" + }, + "html/toylab": { + "display_name": "Toy lab" + }, + "video/Video_Resources": { + "display_name": "Video Resources" + }, + "video/Welcome": { + "display_name": "Welcome" + } +} diff --git a/common/test/data/two_toys/video/Video_Resources.xml b/common/test/data/two_toys/video/Video_Resources.xml new file mode 100644 index 0000000000..88657ed79f --- /dev/null +++ b/common/test/data/two_toys/video/Video_Resources.xml @@ -0,0 +1 @@ +