Make split mongo read-only API consistent with other modulestores.

cms/djangoapps/auth/tests/test_authz.py

@@ -181,7 +181,7 @@ def test_get_staff(self):
         create_all_course_groups(self.creator, self.location)
         add_user_to_course_group(self.creator, self.staff, self.location, STAFF_ROLE_NAME)
 
-        location2 = 'i4x', 'mitX', '103', 'course2', 'test2'
+        location2 = 'i4x', 'mitX', '103', 'course', 'test2'
         staff2 = User.objects.create_user('teststaff2', 'teststaff2+courses@edx.org', 'foo')
         create_all_course_groups(self.creator, location2)
         add_user_to_course_group(self.creator, staff2, location2, STAFF_ROLE_NAME)
@@ -193,7 +193,7 @@ def test_get_instructor(self):
         create_all_course_groups(self.creator, self.location)
         add_user_to_course_group(self.creator, self.staff, self.location, STAFF_ROLE_NAME)
 
-        location2 = 'i4x', 'mitX', '103', 'course2', 'test2'
+        location2 = 'i4x', 'mitX', '103', 'course', 'test2'
         creator2 = User.objects.create_user('testcreator2', 'testcreator2+courses@edx.org', 'foo')
         staff2 = User.objects.create_user('teststaff2', 'teststaff2+courses@edx.org', 'foo')
         create_all_course_groups(creator2, location2)

cms/djangoapps/contentstore/tests/test_utils.py

@@ -5,8 +5,6 @@
 import copy
 from django.test import TestCase
 from django.test.utils import override_settings
-from xmodule.modulestore.tests.factories import CourseFactory
-from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 
 
 class LMSLinksTestCase(TestCase):
@@ -56,21 +54,28 @@ def about_page_no_lms_base_test(self):
     def get_about_page_link(self):
         """ create mock course and return the about page link """
         location = 'i4x', 'mitX', '101', 'course', 'test'
-        utils.get_course_id = mock.Mock(return_value="mitX/101/test")
         return utils.get_lms_link_for_about_page(location)
 
     def lms_link_test(self):
         """ Tests get_lms_link_for_item. """
         location = 'i4x', 'mitX', '101', 'vertical', 'contacting_us'
-        utils.get_course_id = mock.Mock(return_value="mitX/101/test")
-        link = utils.get_lms_link_for_item(location, False)
+        link = utils.get_lms_link_for_item(location, False, "mitX/101/test")
         self.assertEquals(link, "//localhost:8000/courses/mitX/101/test/jump_to/i4x://mitX/101/vertical/contacting_us")
-        link = utils.get_lms_link_for_item(location, True)
+        link = utils.get_lms_link_for_item(location, True, "mitX/101/test")
         self.assertEquals(
             link,
             "//preview/courses/mitX/101/test/jump_to/i4x://mitX/101/vertical/contacting_us"
         )
 
+        # If no course_id is passed in, it is obtained from the location. This is the case for
+        # Studio dashboard.
+        location = 'i4x', 'mitX', '101', 'course', 'test'
+        link = utils.get_lms_link_for_item(location)
+        self.assertEquals(
+            link,
+            "//localhost:8000/courses/mitX/101/test/jump_to/i4x://mitX/101/course/test"
+        )
+
 
 class ExtraPanelTabTestCase(TestCase):
     """ Tests adding and removing extra course tabs. """
@@ -145,4 +150,3 @@ def test_remove_extra_panel_tab(self):
                 changed, actual_tabs = utils.remove_extra_panel_tab(tab_type, course)
                 self.assertFalse(changed)
                 self.assertEqual(actual_tabs, expected_tabs)
-

cms/djangoapps/contentstore/utils.py

@@ -89,8 +89,17 @@ def get_course_for_item(location):
 
 
 def get_lms_link_for_item(location, preview=False, course_id=None):
+    """
+    Returns an LMS link to the course with a jump_to to the provided location.
+
+    :param location: the location to jump to
+    :param preview: True if the preview version of LMS should be returned. Default value is false.
+    :param course_id: the course_id within which the location lives. If not specified, the course_id is obtained
+           by calling Location(location).course_id; note that this only works for locations representing courses
+           instead of elements within courses.
+    """
     if course_id is None:
-        course_id = get_course_id(location)
+        course_id = Location(location).course_id
 
     if settings.LMS_BASE is not None:
         if preview:
@@ -136,22 +145,14 @@ def get_lms_link_for_about_page(location):
     if about_base is not None:
         lms_link = "//{about_base_url}/courses/{course_id}/about".format(
             about_base_url=about_base,
-            course_id=get_course_id(location)
+            course_id=Location(location).course_id
         )
     else:
         lms_link = None
 
     return lms_link
 
 
-def get_course_id(location):
-    """
-    Returns the course_id from a given the location tuple.
-    """
-    # TODO: These will need to be changed to point to the particular instance of this problem in the particular course
-    return modulestore().get_containing_courses(Location(location))[0].id
-
-
 class UnitState(object):
     draft = 'draft'
     private = 'private'

cms/djangoapps/contentstore/views/user.py

@@ -54,8 +54,7 @@ def format_course_for_view(course):
                 'name': course.location.name,
             }),
             get_lms_link_for_item(
-                course.location,
-                course_id=course.location.course_id,
+                course.location
             ),
             course.display_org_with_default,
             course.display_number_with_default,

common/lib/xmodule/xmodule/modulestore/__init__.py

@@ -235,8 +235,15 @@ def __repr__(self):
 
     @property
     def course_id(self):
-        """Return the ID of the Course that this item belongs to by looking
-        at the location URL hierachy"""
+        """
+        Return the ID of the Course that this item belongs to by looking
+        at the location URL hierachy.
+
+        Throws an InvalidLocationError is this location does not represent a course.
+        """
+        if self.category != 'course':
+            raise InvalidLocationError('Cannot call course_id for {0} because it is not of category course'.format(self))
+
         return "/".join([self.org, self.course, self.name])
 
     def replace(self, **kwargs):
@@ -370,20 +377,12 @@ def get_parent_locations(self, location, course_id):
         '''
         raise NotImplementedError
 
-    def get_containing_courses(self, location):
-        '''
-        Returns the list of courses that contains the specified location
-
-        TODO (cpennington): This should really take a module instance id,
-        rather than a location
-        '''
-        courses = [
-            course
-            for course in self.get_courses()
-            if course.location.org == location.org and course.location.course == location.course
-        ]
-
-        return courses
+    def get_errored_courses(self):
+        """
+        Return a dictionary of course_dir -> [(msg, exception_str)], for each
+        course_dir where course loading failed.
+        """
+        raise NotImplementedError
 
 
 class ModuleStoreBase(ModuleStore):
@@ -424,6 +423,15 @@ def get_item_errors(self, location):
         errorlog = self._get_errorlog(location)
         return errorlog.errors
 
+    def get_errored_courses(self):
+        """
+        Returns an empty dict.
+
+        It is up to subclasses to extend this method if the concept
+        of errored courses makes sense for their implementation.
+        """
+        return {}
+
     def get_course(self, course_id):
         """Default impl--linear search through course list"""
         for c in self.get_courses():

common/lib/xmodule/xmodule/modulestore/mongo/base.py

@@ -681,7 +681,7 @@ def create_and_save_xmodule(self, location, definition_data=None, metadata=None,
         # we should remove this once we can break this reference from the course to static tabs
         # TODO move this special casing to app tier (similar to attaching new element to parent)
         if location.category == 'static_tab':
-            course = self.get_course_for_item(location)
+            course = self._get_course_for_item(location)
             existing_tabs = course.tabs or []
             existing_tabs.append({
                 'type': 'static_tab',
@@ -701,7 +701,7 @@ def fire_updated_modulestore_signal(self, course_id, location):
             self.modulestore_update_signal.send(self, modulestore=self, course_id=course_id,
                                                 location=location)
 
-    def get_course_for_item(self, location, depth=0):
+    def _get_course_for_item(self, location, depth=0):
         '''
         VS[compat]
         cdodge: for a given Xmodule, return the course that it belongs to
@@ -790,7 +790,7 @@ def update_metadata(self, location, metadata):
         # we should remove this once we can break this reference from the course to static tabs
         loc = Location(location)
         if loc.category == 'static_tab':
-            course = self.get_course_for_item(loc)
+            course = self._get_course_for_item(loc)
             existing_tabs = course.tabs or []
             for tab in existing_tabs:
                 if tab.get('url_slug') == loc.name:
@@ -818,7 +818,7 @@ def delete_item(self, location, delete_all_versions=False):
         # we should remove this once we can break this reference from the course to static tabs
         if location.category == 'static_tab':
             item = self.get_item(location)
-            course = self.get_course_for_item(item.location)
+            course = self._get_course_for_item(item.location)
             existing_tabs = course.tabs or []
             course.tabs = [tab for tab in existing_tabs if tab.get('url_slug') != location.name]
             # Save the updates to the course to the MongoKeyValueStore
@@ -841,13 +841,6 @@ def get_parent_locations(self, location, course_id):
                                      {'_id': True})
         return [i['_id'] for i in items]
 
-    def get_errored_courses(self):
-        """
-        This function doesn't make sense for the mongo modulestore, as courses
-        are loaded on demand, rather than up front
-        """
-        return {}
-
     def _create_new_model_data(self, category, location, definition_data, metadata):
         """
         To instantiate a new xmodule which will be saved latter, set up the dbModel and kvs

common/lib/xmodule/xmodule/modulestore/split_mongo/split.py

@@ -226,7 +226,7 @@ def _lookup_course(self, course_locator):
             entry['branch'] = course_locator.branch
         return entry
 
-    def get_courses(self, branch, qualifiers=None):
+    def get_courses(self, branch='published', qualifiers=None):
         '''
         Returns a list of course descriptors matching any given qualifiers.
 
@@ -235,6 +235,9 @@ def get_courses(self, branch, qualifiers=None):
 
         Note, this is to find the current head of the named branch type
         (e.g., 'draft'). To get specific versions via guid use get_course.
+
+        :param branch: the branch for which to return courses. Default value is 'published'.
+        :param qualifiers: a optional dict restricting which elements should match
         '''
         if qualifiers is None:
             qualifiers = {}
@@ -272,13 +275,6 @@ def get_course(self, course_locator):
         result = self._load_items(course_entry, [root], 0, lazy=True)
         return result[0]
 
-    def get_course_for_item(self, location):
-        '''
-        Provided for backward compatibility. Is equivalent to calling get_course
-        :param location:
-        '''
-        return self.get_course(location)
-
     def has_item(self, block_location):
         """
         Returns True if location exists in its course. Returns false if
@@ -313,9 +309,8 @@ def get_item(self, location, depth=0):
             raise ItemNotFoundError(location)
         return items[0]
 
-    # TODO refactor this and get_courses to use a constructed query
-    def get_items(self, locator, qualifiers):
-        '''
+    def get_items(self, locator, course_id=None, depth=0, qualifiers=None):
+        """
         Get all of the modules in the given course matching the qualifiers. The
         qualifiers should only be fields in the structures collection (sorry).
         There will be a separate search method for searching through
@@ -331,9 +326,14 @@ def get_items(self, locator, qualifiers):
         try arbitrary queries.
 
         :param locator: CourseLocator or BlockUsageLocator restricting search scope
+        :param course_id: ignored. Only included for API compatibility.
+        :param depth: ignored. Only included for API compatibility.
         :param qualifiers: a dict restricting which elements should match
-        '''
+
+        """
         # TODO extend to only search a subdag of the course?
+        if qualifiers is None:
+            qualifiers = {}
         course = self._lookup_course(locator)
         items = []
         for usage_id, value in course['blocks'].iteritems():
@@ -345,23 +345,35 @@ def get_items(self, locator, qualifiers):
         else:
             return []
 
-    # What's the use case for usage_id being separate?
+    def get_instance(self, course_id, location, depth=0):
+        """
+        Get an instance of this location.
+
+        For now, just delegate to get_item and ignore course policy.
+
+        depth (int): An argument that some module stores may use to prefetch
+            descendants of the queried modules for more efficient results later
+            in the request. The depth is counted in the number of
+            calls to get_children() to cache. None indicates to cache all descendants.
+        """
+        return self.get_item(location, depth=depth)
+
     def get_parent_locations(self, locator, usage_id=None):
         '''
         Return the locations (Locators w/ usage_ids) for the parents of this location in this
         course. Could use get_items(location, {'children': usage_id}) but this is slightly faster.
-        NOTE: does not actually ensure usage_id exists
-        If usage_id is None, then the locator must specify the usage_id
+        NOTE: the locator must contain the usage_id, and this code does not actually ensure usage_id exists
+
+        :param locator: BlockUsageLocator restricting search scope
+        :param usage_id: ignored. Only included for API compatibility. Specify the usage_id within the locator.
         '''
-        if usage_id is None:
-            usage_id = locator.usage_id
+
         course = self._lookup_course(locator)
         items = []
         for parent_id, value in course['blocks'].iteritems():
             for child_id in value['children']:
-                if usage_id == child_id:
-                    locator = locator.as_course_locator()
-                    items.append(BlockUsageLocator(url=locator, usage_id=parent_id))
+                if locator.usage_id == child_id:
+                    items.append(BlockUsageLocator(url=locator.as_course_locator(), usage_id=parent_id))
         return items
 
     def get_course_index_info(self, course_locator):
@@ -1050,14 +1062,6 @@ def delete_course(self, course_id):
         # this is the only real delete in the system. should it do something else?
         self.course_index.remove(index['_id'])
 
-    # TODO remove all callers and then this
-    def get_errored_courses(self):
-        """
-        This function doesn't make sense for the mongo modulestore, as structures
-        are loaded on demand, rather than up front
-        """
-        return {}
-
     def inherit_metadata(self, block_map, block, inheriting_metadata=None):
         """
         Updates block with any value

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

@@ -159,3 +159,12 @@ def test_clean_for_html():
 def test_html_id():
     loc = Location("tag://org/course/cat/name:more_name@rev")
     assert_equals(loc.html_id(), "tag-org-course-cat-name_more_name-rev")
+
+
+def test_course_id():
+    loc = Location('i4x', 'mitX', '103', 'course', 'test2')
+    assert_equals('mitX/103/test2', loc.course_id)
+
+    loc = Location('i4x', 'mitX', '103', '_not_a_course', 'test2')
+    with assert_raises(InvalidLocationError):
+        loc.course_id