Hotfix/2013 12 12

cms/djangoapps/contentstore/views/course.py

@@ -116,7 +116,8 @@ def course_listing(request):
     """
     List all courses available to the logged in user
     """
-    courses = modulestore('direct').get_items(['i4x', None, None, 'course', None])
+    # there's an index on category which will be used if none of its antecedents are set
+    courses = modulestore('direct').get_items([None, None, None, 'course', None])
 
     # filter out courses that we don't have access too
     def course_filter(course):

common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py

@@ -4,10 +4,10 @@
 from random import randint
 import re
 import pymongo
+import bson.son
 
 from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError, DuplicateItemError
 from xmodule.modulestore.locator import BlockUsageLocator
-from xmodule.modulestore.mongo import draft
 from xmodule.modulestore import Location
 import urllib
 
@@ -91,9 +91,10 @@ def create_map_entry(self, course_location, course_id=None, draft_branch='draft'
             else:
                 course_id = "{0.org}.{0.course}".format(course_location)
         # very like _interpret_location_id but w/o the _id
-        location_id = {'org': course_location.org, 'course': course_location.course}
-        if course_location.category == 'course':
-            location_id['name'] = course_location.name
+        location_id = self._construct_location_son(
+            course_location.org, course_location.course, 
+            course_location.name if course_location.category == 'course' else None
+        )
 
         self.location_map.insert({
             '_id': location_id,
@@ -128,20 +129,25 @@ def translate_location(self, old_style_course_id, location, published=True, add_
         """
         location_id = self._interpret_location_course_id(old_style_course_id, location)
 
-        maps = self.location_map.find(location_id).sort('_id.name', pymongo.ASCENDING)
-        if maps.count() == 0:
+        maps = self.location_map.find(location_id)
+        maps = list(maps)
+        if len(maps) == 0:
             if add_entry_if_missing:
                 # create a new map
-                course_location = location.replace(category='course', name=location_id['_id.name'])
+                course_location = location.replace(category='course', name=location_id['_id']['name'])
                 self.create_map_entry(course_location)
                 entry = self.location_map.find_one(location_id)
             else:
                 raise ItemNotFoundError()
-        elif maps.count() > 1:
-            # if more than one, prefer the one w/o a name if that exists. Otherwise, choose the first (alphabetically)
+        elif len(maps) == 1:
             entry = maps[0]
         else:
+            # find entry w/o name, if any; otherwise, pick arbitrary
             entry = maps[0]
+            for item in maps:
+                if 'name' not in item['_id']:
+                    entry = item
+                    break
 
         if published:
             branch = entry['prod_branch']
@@ -242,11 +248,11 @@ def add_block_location_translator(self, location, old_course_id=None, usage_id=N
         location_id = self._interpret_location_course_id(old_course_id, location)
 
         maps = self.location_map.find(location_id)
-        if maps.count() == 0:
-            raise ItemNotFoundError()
-
         # turn maps from cursor to list
         map_list = list(maps)
+        if len(map_list) == 0:
+            raise ItemNotFoundError()
+
         encoded_location_name = self._encode_for_mongo(location.name)
         # check whether there's already a usage_id for this location (and it agrees w/ any passed in or found)
         for map_entry in map_list:
@@ -279,7 +285,10 @@ def add_block_location_translator(self, location, old_course_id=None, usage_id=N
                         )
 
                 map_entry['block_map'].setdefault(encoded_location_name, {})[location.category] = computed_usage_id
-                self.location_map.update({'_id': map_entry['_id']}, {'$set': {'block_map': map_entry['block_map']}})
+                self.location_map.update(
+                    {'_id': self._construct_location_son(**map_entry['_id'])}, 
+                    {'$set': {'block_map': map_entry['block_map']}}
+                )
 
         return computed_usage_id
 
@@ -317,7 +326,10 @@ def update_block_location_translator(self, location, usage_id, old_course_id=Non
 
             if location.category in map_entry['block_map'].setdefault(encoded_location_name, {}):
                 map_entry['block_map'][encoded_location_name][location.category] = usage_id
-                self.location_map.update({'_id': map_entry['_id']}, {'$set': {'block_map': map_entry['block_map']}})
+                self.location_map.update(
+                    {'_id': self._construct_location_son(**map_entry['_id'])}, 
+                    {'$set': {'block_map': map_entry['block_map']}}
+                )
 
         return usage_id
 
@@ -338,7 +350,10 @@ def delete_block_location_translator(self, location, old_course_id=None):
                     del map_entry['block_map'][encoded_location_name]
                 else:
                     del map_entry['block_map'][encoded_location_name][location.category]
-                self.location_map.update({'_id': map_entry['_id']}, {'$set': {'block_map': map_entry['block_map']}})
+                self.location_map.update(
+                    {'_id': self._construct_location_son(**map_entry['_id'])},
+                    {'$set': {'block_map': map_entry['block_map']}}
+                )
 
     def _add_to_block_map(self, location, location_id, block_map):
         '''add the given location to the block_map and persist it'''
@@ -357,7 +372,7 @@ def _add_to_block_map(self, location, location_id, block_map):
 
     def _interpret_location_course_id(self, course_id, location):
         """
-        Take the old style course id (org/course/run) and return a dict for querying the mapping table.
+        Take the old style course id (org/course/run) and return a dict w/ a SON for querying the mapping table.
         If the course_id is empty, it uses location, but this may result in an inadequate id.
 
         :param course_id: old style 'org/course/run' id from Location.course_id where Location.category = 'course'
@@ -367,12 +382,21 @@ def _interpret_location_course_id(self, course_id, location):
         if course_id:
             # re doesn't allow ?P<_id.org> and ilk
             matched = re.match(r'([^/]+)/([^/]+)/([^/]+)', course_id)
-            return dict(zip(['_id.org', '_id.course', '_id.name'], matched.groups()))
+            return {'_id': self._construct_location_son(*matched.groups())}
 
-        location_id = {'_id.org': location.org, '_id.course': location.course}
         if location.category == 'course':
-            location_id['_id.name'] = location.name
-        return location_id
+            return {'_id': self._construct_location_son(location.org, location.course, location.name)}
+        else:
+            return bson.son.SON([('_id.org', location.org), ('_id.course', location.course)])
+
+    def _construct_location_son(self, org, course, name=None):
+        """
+        Construct the SON needed to repr the location for either a query or an insertion
+        """
+        if name:
+            return bson.son.SON([('org', org), ('course', course), ('name', name)])
+        else:
+            return bson.son.SON([('org', org), ('course', course)])
 
     def _block_id_is_guid(self, name):
         """

common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py

@@ -36,8 +36,9 @@ def test_create_map(self):
         org = 'foo_org'
         course = 'bar_course'
         loc_mapper().create_map_entry(Location('i4x', org, course, 'course', 'baz_run'))
+        # pylint: disable=protected-access
         entry = loc_mapper().location_map.find_one({
-            '_id': {'org': org, 'course': course, 'name': 'baz_run'}
+            '_id': loc_mapper()._construct_location_son(org, course, 'baz_run')
         })
         self.assertIsNotNone(entry, "Didn't find entry")
         self.assertEqual(entry['course_id'], '{}.{}.baz_run'.format(org, course))
@@ -48,8 +49,9 @@ def test_create_map(self):
         # ensure create_entry does the right thing when not given a course (creates org/course
         # rather than org/course/run course_id)
         loc_mapper().create_map_entry(Location('i4x', org, course, 'vertical', 'baz_vert'))
+        # find the one which has no name
         entry = loc_mapper().location_map.find_one({
-            '_id': {'org': org, 'course': course}
+            '_id' : loc_mapper()._construct_location_son(org, course, None)
         })
         self.assertIsNotNone(entry, "Didn't find entry")
         self.assertEqual(entry['course_id'], '{}.{}'.format(org, course))
@@ -63,9 +65,7 @@ def test_create_map(self):
             'wip',
             'live',
             block_map)
-        entry = loc_mapper().location_map.find_one({
-            '_id': {'org': org, 'course': course}
-        })
+        entry = loc_mapper().location_map.find_one({'_id.org': org, '_id.course': course})
         self.assertIsNotNone(entry, "Didn't find entry")
         self.assertEqual(entry['course_id'], 'foo_org.geek_dept.quux_course.baz_run')
         self.assertEqual(entry['draft_branch'], 'wip')

lms/djangoapps/courseware/roles.py

@@ -8,6 +8,9 @@
 from django.contrib.auth.models import User, Group
 
 from xmodule.modulestore import Location
+from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError
+from xmodule.modulestore.django import loc_mapper
+from xmodule.modulestore.locator import CourseLocator
 
 
 class CourseContextRequired(Exception):
@@ -134,20 +137,45 @@ class CourseRole(GroupBasedRole):
     A named role in a particular course
     """
     def __init__(self, role, location, course_context=None):
-        # pylint: disable=no-member
-        loc = Location(location)
-        legacy_group_name = '{0}_{1}'.format(role, loc.course)
-
-        if loc.category.lower() == 'course':
-            course_id = loc.course_id
-        else:
+        """
+        Location may be either a Location, a string, dict, or tuple which Location will accept
+        in its constructor, or a CourseLocator. Handle all these giving some preference to
+        the preferred naming.
+        """
+        # TODO: figure out how to make the group name generation lazy so it doesn't force the
+        # loc mapping?
+        if not hasattr(location, 'course_id'):
+            location = Location(location)
+        # direct copy from auth.authz.get_all_course_role_groupnames will refactor to one impl asap
+        groupnames = []
+        try:
+            groupnames.append('{0}_{1}'.format(role, location.course_id))
+        except InvalidLocationError:  # will occur on old locations where location is not of category course
             if course_context is None:
                 raise CourseContextRequired()
-            course_id = course_context
+            else:
+                groupnames.append('{0}_{1}'.format(role, course_context))
 
-        group_name = '{0}_{1}'.format(role, course_id)
-
-        super(CourseRole, self).__init__([group_name, legacy_group_name])
+        # pylint: disable=no-member
+        if isinstance(location, Location):
+            try:
+                locator = loc_mapper().translate_location(location.course_id, location, False, False)
+                groupnames.append('{0}_{1}'.format(role, locator.course_id))
+            except (InvalidLocationError, ItemNotFoundError):
+                # if it's never been mapped, the auth won't be via the Locator syntax
+                pass
+            # least preferred legacy role_course format
+            groupnames.append('{0}_{1}'.format(role, location.course))
+        elif isinstance(location, CourseLocator):
+            # handle old Location syntax
+            old_location = loc_mapper().translate_locator_to_location(location, get_course=True)
+            if old_location:
+                # the slashified version of the course_id (myu/mycourse/myrun)
+                groupnames.append('{0}_{1}'.format(role, old_location.course_id))
+                # add the least desirable but sometimes occurring format.
+                groupnames.append('{0}_{1}'.format(role, old_location.course))
+
+        super(CourseRole, self).__init__(groupnames)
 
 
 class OrgRole(GroupBasedRole):

mongo_indexes.md

@@ -0,0 +1,21 @@
+These are the indexes each mongo db should have in order to perform well.
+Each section states the collection name and then the indexes. To create an index,
+you'll typically either use the mongohq type web interface or a standard terminal console.
+If a terminal, this assumes you've logged in and gotten to the mongo prompt 
+```
+mongo mydatabasename
+```
+
+If using the terminal, to add an index to a collection, you'll need to prefix ```ensureIndex``` with
+```
+db.collection_name
+```
+as in ```db.location_map.ensureIndex({'course_id': 1}{background: true})```
+
+location_map:
+=============
+
+```
+ensureIndex({​'_id.org': 1, '_id.course': 1})
+ensureIndex({​'course_id': 1})
+```