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

@@ -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):

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.

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

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),

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):

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

common/lib/xmodule/xmodule/tests/__init__.py

@@ -8,6 +8,7 @@
 import unittest
 import os
 import fs
+import fs.osfs
 import json
 
 import json

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):

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")

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.

common/test/data/two_toys/chapter/Overview.xml

@@ -0,0 +1,4 @@
+<chapter>
+  <videosequence url_name="Toy_Videos"/>
+  <video url_name="Welcome"/>
+</chapter>

common/test/data/two_toys/course.xml

@@ -0,0 +1 @@
+<course url_name="TT_2012_Fall" org="edX" course="toy"/>

common/test/data/two_toys/course/TT_2012_Fall.xml

@@ -0,0 +1,3 @@
+<course display_name="Toy Course" graceperiod="2 days 5 hours 59 minutes 59 seconds" start="2015-07-17T12:00">
+  <chapter url_name="Overview"/>
+</course>

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"
+    }
+}

common/test/data/two_toys/video/Video_Resources.xml

@@ -0,0 +1 @@
+<video youtube="1.0:1bK-WdDi6Qw" display_name="Video Resources"/>

common/test/data/two_toys/video/Welcome.xml

@@ -0,0 +1 @@
+<video youtube="1.0:p2Q6BrNhdh9" display_name="Welcome"/>

common/test/data/two_toys/videosequence/Toy_Videos.xml

@@ -0,0 +1,3 @@
+<videosequence display_name="Toy Videos" format="Lecture Sequence">
+  <video url_name="Video_Resources"/>
+</videosequence>

lms/djangoapps/courseware/courses.py

@@ -25,7 +25,7 @@ def get_course_by_id(course_id):
     """
     try:
         course_loc = CourseDescriptor.id_to_location(course_id)
-        return modulestore().get_item(course_loc)
+        return modulestore().get_instance(course_id, course_loc)
     except (KeyError, ItemNotFoundError):
         raise Http404("Course not found.")
 

lms/djangoapps/courseware/grades.py

@@ -9,6 +9,7 @@ from django.conf import settings
 from models import StudentModuleCache
 from module_render import get_module, get_instance_module
 from xmodule import graders
+from xmodule.course_module import CourseDescriptor
 from xmodule.graders import Score
 from models import StudentModule
 
@@ -63,8 +64,10 @@ def grade(student, request, course, student_module_cache=None):
                 scores = []
                 # TODO: We need the request to pass into here. If we could forgo that, our arguments
                 # would be simpler
+                course_id = CourseDescriptor.location_to_id(course.location)
                 section_module = get_module(student, request,
-                                            section_descriptor.location, student_module_cache)
+                                            section_descriptor.location, student_module_cache,
+                                            course_id)
                 if section_module is None:
                     # student doesn't have access to this module, or something else
                     # went wrong.

lms/djangoapps/courseware/management/commands/check_course.py

@@ -1,5 +1,7 @@
 import os.path
 
+# THIS COMMAND IS OUT OF DATE
+
 from lxml import etree
 
 from django.core.management.base import BaseCommand
@@ -78,7 +80,8 @@ class Command(BaseCommand):
 
         # TODO (cpennington): Get coursename in a legitimate way
         course_location = 'i4x://edx/6002xs12/course/6.002_Spring_2012'
-        student_module_cache = StudentModuleCache.cache_for_descriptor_descendents(sample_user, modulestore().get_item(course_location))
+        student_module_cache = StudentModuleCache.cache_for_descriptor_descendents(
+            sample_user, modulestore().get_item(course_location))
         course = get_module(sample_user, None, course_location, student_module_cache)
 
         to_run = [

lms/djangoapps/courseware/management/commands/clean_xml.py

@@ -124,9 +124,11 @@ def check_roundtrip(course_dir):
     print "======== ideally there is no diff above this ======="
 
 
-def clean_xml(course_dir, export_dir):
+def clean_xml(course_dir, export_dir, force):
     (ok, course) = import_with_checks(course_dir)
-    if ok:
+    if ok or force:
+        if not ok:
+            print "WARNING: Exporting despite errors"
         export(course, export_dir)
         check_roundtrip(export_dir)
     else:
@@ -138,11 +140,18 @@ class Command(BaseCommand):
     help = """Imports specified course.xml, validate it, then exports in
     a canonical format.
 
-Usage: clean_xml PATH-TO-COURSE-DIR PATH-TO-OUTPUT-DIR
+Usage: clean_xml PATH-TO-COURSE-DIR PATH-TO-OUTPUT-DIR [force]
+
+If 'force' is specified as the last argument, exports even if there
+were import errors.
 """
     def handle(self, *args, **options):
-        if len(args) != 2:
+        n = len(args)
+        if n < 2 or n > 3:
             print Command.help
             return
 
-        clean_xml(args[0], args[1])
+        force = False
+        if n == 3 and args[2] == 'force':
+            force = True
+        clean_xml(args[0], args[1], force)

lms/djangoapps/courseware/module_render.py

@@ -71,13 +71,13 @@ def toc_for_course(user, request, course, active_chapter, active_section, course
     '''
 
     student_module_cache = StudentModuleCache.cache_for_descriptor_descendents(user, course, depth=2)
-    course = get_module(user, request, course.location, student_module_cache, course_id=course_id)
+    course = get_module(user, request, course.location, student_module_cache, course_id)
 
     chapters = list()
     for chapter in course.get_display_items():
         hide_from_toc = chapter.metadata.get('hide_from_toc','false').lower() == 'true'
         if hide_from_toc:
-            continue 
+            continue
 
         sections = list()
         for section in chapter.get_display_items():
@@ -131,7 +131,7 @@ def get_section(course_module, chapter, section):
     return section_module
 
 
-def get_module(user, request, location, student_module_cache, position=None, course_id=None):
+def get_module(user, request, location, student_module_cache, course_id, position=None):
     ''' Get an instance of the xmodule class identified by location,
     setting the state based on an existing StudentModule, or creating one if none
     exists.
@@ -141,21 +141,14 @@ def get_module(user, request, location, student_module_cache, position=None, cou
       - request               : current django HTTPrequest
       - location              : A Location-like object identifying the module to load
       - student_module_cache  : a StudentModuleCache
+      - course_id             : the course_id in the context of which to load module
       - position              : extra information from URL for user-specified
                                 position within module
 
     Returns: xmodule instance
 
     '''
-    descriptor = modulestore().get_item(location)
-    
-    # NOTE:
-    #   A 'course_id' is understood to be the triplet (org, course, run), for example
-    #       (MITx, 6.002x, 2012_Spring).
-    #   At the moment generic XModule does not contain enough information to replicate
-    #       the triplet (it is missing 'run'), so we must pass down course_id
-    if course_id is None:
-        course_id = descriptor.location.course_id # Will NOT produce (org, course, run) for non-CourseModule's
+    descriptor = modulestore().get_instance(course_id, location)
 
     # Short circuit--if the user shouldn't have access, bail without doing any work
     if not has_access(user, descriptor, 'load'):
@@ -207,7 +200,7 @@ def get_module(user, request, location, student_module_cache, position=None, cou
         Delegate to get_module.  It does an access check, so may return None
         """
         return get_module(user, request, location,
-                                       student_module_cache, position, course_id=course_id)
+                                       student_module_cache, course_id, position)
 
     # TODO (cpennington): When modules are shared between courses, the static
     # prefix is going to have to be specific to the module, not the directory
@@ -325,7 +318,7 @@ def xqueue_callback(request, course_id, userid, id, dispatch):
 
     student_module_cache = StudentModuleCache.cache_for_descriptor_descendents(
         user, modulestore().get_item(id), depth=0, select_for_update=True)
-    instance = get_module(user, request, id, student_module_cache)
+    instance = get_module(user, request, id, student_module_cache, course_id)
     if instance is None:
         log.debug("No module {0} for user {1}--access denied?".format(id, user))
         raise Http404
@@ -361,7 +354,7 @@ def xqueue_callback(request, course_id, userid, id, dispatch):
     return HttpResponse("")
 
 
-def modx_dispatch(request, dispatch=None, id=None, course_id=None):
+def modx_dispatch(request, dispatch, location, course_id):
     ''' Generic view for extensions. This is where AJAX calls go.
 
     Arguments:
@@ -370,7 +363,8 @@ def modx_dispatch(request, dispatch=None, id=None, course_id=None):
       - dispatch -- the command string to pass through to the module's handle_ajax call
            (e.g. 'problem_reset').  If this string contains '?', only pass
            through the part before the first '?'.
-      - id -- the module id. Used to look up the XModule instance
+      - location -- the module location. Used to look up the XModule instance
+      - course_id -- defines the course context for this request.
     '''
     # ''' (fix emacs broken parsing)
 
@@ -392,12 +386,14 @@ def modx_dispatch(request, dispatch=None, id=None, course_id=None):
                     return HttpResponse(json.dumps({'success': file_too_big_msg}))
             p[fileinput_id] = inputfiles
 
-    student_module_cache = StudentModuleCache.cache_for_descriptor_descendents(request.user, modulestore().get_item(id))
-    instance = get_module(request.user, request, id, student_module_cache, course_id=course_id)
+    student_module_cache = StudentModuleCache.cache_for_descriptor_descendents(
+        request.user, modulestore().get_item(course_id, location))
+
+    instance = get_module(request.user, request, location, student_module_cache, course_id)
     if instance is None:
         # Either permissions just changed, or someone is trying to be clever
         # and load something they shouldn't have access to.
-        log.debug("No module {0} for user {1}--access denied?".format(id, user))
+        log.debug("No module {0} for user {1}--access denied?".format(location, user))
         raise Http404
 
     instance_module = get_instance_module(request.user, instance, student_module_cache)

lms/djangoapps/courseware/views.py

@@ -64,7 +64,7 @@ def courses(request):
     '''
     Render "find courses" page.  The course selection work is done in courseware.courses.
     '''
-    universities = get_courses_by_university(request.user, 
+    universities = get_courses_by_university(request.user,
                                              domain=request.META.get('HTTP_HOST'))
     return render_to_response("courses.html", {'universities': universities})
 
@@ -139,7 +139,7 @@ def index(request, course_id, chapter=None, section=None,
                                                           section_descriptor)
                 module = get_module(request.user, request,
                                     section_descriptor.location,
-                                    student_module_cache, course_id=course_id)
+                                    student_module_cache, course_id)
                 if module is None:
                     # User is probably being clever and trying to access something
                     # they don't have access to.
@@ -279,9 +279,11 @@ def profile(request, course_id, student_id=None):
     user_info = UserProfile.objects.get(user=student)
 
     student_module_cache = StudentModuleCache.cache_for_descriptor_descendents(request.user, course)
-    course_module = get_module(request.user, request, course.location, student_module_cache, course_id=course_id)
+    course_module = get_module(request.user, request, course.location,
+                               student_module_cache, course_id)
 
-    courseware_summary = grades.progress_summary(student, course_module, course.grader, student_module_cache)
+    courseware_summary = grades.progress_summary(student, course_module,
+                                                 course.grader, student_module_cache)
     grade_summary = grades.grade(request.user, request, course, student_module_cache)
 
     context = {'name': user_info.name,