Merge pull request #3745 from edx/dhm/no-package-id

Dhm/no package

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

@@ -100,9 +100,7 @@ class LocMapperStore(object):
         self.location_map.insert({
             '_id': course_son,
             'org': org,
-            'lower_org': org.lower(),
             'offering': offering,
-            'lower_offering': offering.lower(),
             'draft_branch': draft_branch,
             'prod_branch': prod_branch,
             'block_map': block_map or {},
@@ -197,7 +195,7 @@ class LocMapperStore(object):
         self._cache_location_map_entry(location, published_usage, draft_usage)
         return result
 
-    def translate_locator_to_location(self, locator, get_course=False, lower_only=False):
+    def translate_locator_to_location(self, locator, get_course=False):
         """
         Returns an old style Location for the given Locator if there's an appropriate entry in the
         mapping collection. Note, it requires that the course was previously mapped (a side effect of
@@ -211,53 +209,31 @@ class LocMapperStore(object):
             locator: a BlockUsageLocator to translate
             get_course: rather than finding the map for this locator, returns the CourseKey
                 for the mapped course.
-            lower_only: (obsolete?) the locator's fields are lowercased and not the actual case
-                for the identifier (e.g., came from a sql db which lowercases all ids). Find the actual
-                case Location for the desired object
         """
         if get_course:
             cached_value = self._get_course_location_from_cache(
                 # if locator is already a course_key it won't have course_key attr
-                getattr(locator, 'course_key', locator),
-                lower_only
+                getattr(locator, 'course_key', locator)
             )
         else:
             cached_value = self._get_location_from_cache(locator)
         if cached_value:
             return cached_value
 
-        # This does not require that the course exist in any modulestore
-        # only that it has a mapping entry.
-        if lower_only:
-            # migrate any records which don't have the lower_org and lower_offering fields as
-            # this won't be able to find what it wants. (only needs to be run once ever per db,
-            # I'm not sure how to control that, but I'm putting some check here for once per launch)
-            if not getattr(self, 'lower_offering_migrated', False):
-                obsolete = self.location_map.find(
-                    {'lower_org': {"$exists": False}, "lower_offering": {"$exists": False}, }
-                )
-                self._migrate_if_necessary(obsolete)
-                setattr(self, 'lower_offering_migrated', True)
-
-            entry = self.location_map.find_one(bson.son.SON([
-                ('lower_org', locator.org.lower()),
-                ('lower_offering', locator.offering.lower()),
-            ]))
-        else:
-            # migrate any records which don't have the lower_org and lower_offering fields as
-            # this won't be able to find what it wants. (only needs to be run once ever per db,
-            # I'm not sure how to control that, but I'm putting some check here for once per launch)
-            if not getattr(self, 'offering_migrated', False):
-                obsolete = self.location_map.find(
-                    {'org': {"$exists": False}, "offering": {"$exists": False}, }
-                )
-                self._migrate_if_necessary(obsolete)
-                setattr(self, 'offering_migrated', True)
+        # migrate any records which don't have the org and offering fields as
+        # this won't be able to find what it wants. (only needs to be run once ever per db,
+        # I'm not sure how to control that, but I'm putting some check here for once per launch)
+        if not getattr(self, 'offering_migrated', False):
+            obsolete = self.location_map.find(
+                {'org': {"$exists": False}, "offering": {"$exists": False}, }
+            )
+            self._migrate_if_necessary(obsolete)
+            setattr(self, 'offering_migrated', True)
 
-            entry = self.location_map.find_one(bson.son.SON([
-                ('org', locator.org),
-                ('lower_offering', locator.offering),
-            ]))
+        entry = self.location_map.find_one(bson.son.SON([
+            ('org', locator.org),
+            ('offering', locator.offering),
+        ]))
 
         # look for one which maps to this block block_id
         if entry is None:
@@ -277,12 +253,8 @@ class LocMapperStore(object):
                     self.decode_key_from_mongo(old_name)
                 )
 
-                if lower_only:
-                    entry_org = "lower_org"
-                    entry_offering = "lower_offering"
-                else:
-                    entry_org = "org"
-                    entry_offering = "offering"
+                entry_org = "org"
+                entry_offering = "offering"
 
                 published_locator = BlockUsageLocator(
                     CourseLocator(
@@ -454,18 +426,20 @@ class LocMapperStore(object):
         """
         return self.cache.get(unicode(locator))
 
-    def _get_course_location_from_cache(self, locator_package_id, lower_only=False):
+    def _get_course_location_from_cache(self, course_key):
         """
-        See if the package_id is in the cache. If so, return the mapped location to the
+        See if the course_key is in the cache. If so, return the mapped location to the
         course root.
         """
-        if lower_only:
-            cache_key = u'courseIdLower+{}'.format(locator_package_id.lower())
-        else:
-            cache_key = u'courseId+{}'.format(locator_package_id)
-
+        cache_key = self._course_key_cache_string(course_key)
         return self.cache.get(cache_key)
 
+    def _course_key_cache_string(self, course_key):
+        """
+        Return the string used to cache the course key
+        """
+        return u'{0.org}+{0.offering}'.format(course_key)
+
     def _cache_course_locator(self, old_course_id, published_course_locator, draft_course_locator):
         """
         For quick lookup of courses
@@ -482,8 +456,7 @@ class LocMapperStore(object):
         """
         setmany = {}
         if location.category == 'course':
-            setmany[u'courseId+{}'.format(published_usage.package_id)] = location
-            setmany[u'courseIdLower+{}'.format(published_usage.package_id.lower())] = location
+            setmany[self._course_key_cache_string(published_usage)] = location.course_key
         setmany[unicode(published_usage)] = location
         setmany[unicode(draft_usage)] = location
         setmany[unicode(location)] = (published_usage, draft_usage)
@@ -504,8 +477,7 @@ class LocMapperStore(object):
             delete_keys = []
             published_locator = unicode(cached_key[0].course_key)
             course_location = self._course_location_from_cache(published_locator)
-            delete_keys.append(u'courseId+{}'.format(published_locator))
-            delete_keys.append(u'courseIdLower+{}'.format(unicode(cached_key[0].course_key).lower()))
+            delete_keys.append(self._course_key_cache_string(course_key))
             delete_keys.append(published_locator)
             delete_keys.append(unicode(cached_key[1].course_key))
             delete_keys.append(unicode(course_location))
@@ -535,8 +507,7 @@ class LocMapperStore(object):
         """
         delete_keys = []
         if location.category == 'course':
-            delete_keys.append(u'courseId+{}'.format(published_usage.package_id))
-            delete_keys.append(u'courseIdLower+{}'.format(published_usage.package_id.lower()))
+            delete_keys.append(self._course_key_cache_string(published_usage.course_key))
 
         delete_keys.append(unicode(published_usage))
         delete_keys.append(unicode(draft_usage))
@@ -560,7 +531,7 @@ class LocMapperStore(object):
         """
         If entry had an '_id' without a run, remove the whole record.
 
-        Add fields: schema, org, offering, lower_org, and lower_offering
+        Add fields: schema, org, offering
         Remove: course_id, lower_course_id
         :param entry:
         """
@@ -579,9 +550,7 @@ class LocMapperStore(object):
         entry.pop('lower_course_id', None)
         old_course_id = SlashSeparatedCourseKey(entry['_id']['org'], entry['_id']['course'], entry['_id']['name'])
         entry['org'] = old_course_id.org
-        entry['lower_org'] = old_course_id.org.lower()
         entry['offering'] = old_course_id.offering.replace('/', '+')
-        entry['lower_offering'] = entry['offering'].lower()
         return self._migrate_1(entry, True)
 
     # insert new migrations just before _migrate_top. _migrate_top sets the schema version and

common/lib/xmodule/xmodule/modulestore/locator.py

@@ -111,13 +111,6 @@ class BlockLocatorBase(Locator):
             raise InvalidKeyError(cls, string)
         return match.groupdict()
 
-    @property
-    def package_id(self):
-        if self.org and self.offering:
-            return u'{}{}{}'.format(self.org, self.ORG_SEPARATOR, self.offering)
-        else:
-            return None
-
 
 class CourseLocator(BlockLocatorBase, CourseKey):
     """
@@ -267,7 +260,7 @@ class CourseLocator(BlockLocatorBase, CourseKey):
         """
         parts = []
         if self.offering:
-            parts.append(unicode(self.package_id))
+            parts.extend([self.org, self.offering])
             if self.branch:
                 parts.append(u"{prefix}+{branch}".format(prefix=self.BRANCH_PREFIX, branch=self.branch))
         if self.version_guid:
@@ -391,10 +384,6 @@ class BlockUsageLocator(BlockLocatorBase, UsageKey):
         return self.course_key.offering
 
     @property
-    def package_id(self):
-        return self.course_key.package_id
-
-    @property
     def branch(self):
         return self.course_key.branch