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..d6dd85deea 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 @@ -331,7 +338,8 @@ class ModuleStoreBase(ModuleStore): and datastores. """ # check that item is present and raise the promised exceptions if needed - self.get_item(location) + # TODO (vshnayder): post-launch, make errors properties of items + #self.get_item(location) errorlog = self._get_errorlog(location) return errorlog.errors 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 @@ +