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

cms/djangoapps/contentstore/tests/test_utils.py

@@ -56,21 +56,27 @@ class LMSLinksTestCase(TestCase):
     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. """

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,7 +145,7 @@ 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
@@ -144,14 +153,6 @@ def get_lms_link_for_about_page(location):
     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 index(request):
                 '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/djangoapps/student/views.py

@@ -274,7 +274,7 @@ def dashboard(request):
     # Global staff can see what courses errored on their dashboard
     staff_access = False
     errored_courses = {}
-    if has_access(user, 'global', 'staff'):
+    if has_access(user, 'global', 'staff') and callable(getattr(modulestore(), 'get_errored_courses')):
         # Show any courses that errored on load
         staff_access = True
         errored_courses = modulestore().get_errored_courses()

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

@@ -370,21 +370,6 @@ class ModuleStore(object):
         '''
         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
-
 
 class ModuleStoreBase(ModuleStore):
     '''

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

@@ -841,13 +841,6 @@ class MongoModuleStore(ModuleStoreBase):
                                      {'_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 @@ class SplitMongoModuleStore(ModuleStoreBase):
             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 @@ class SplitMongoModuleStore(ModuleStoreBase):
 
         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 @@ class SplitMongoModuleStore(ModuleStoreBase):
         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 @@ class SplitMongoModuleStore(ModuleStoreBase):
             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 @@ class SplitMongoModuleStore(ModuleStoreBase):
         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,22 @@ class SplitMongoModuleStore(ModuleStoreBase):
         else:
             return []
 
-    # What's the use case for usage_id being separate?
     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 +1049,6 @@ class SplitMongoModuleStore(ModuleStoreBase):
         # 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_split_modulestore.py

@@ -107,7 +107,7 @@ class SplitModuleCourseTests(SplitModuleTest):
     '''
 
     def test_get_courses(self):
-        courses = modulestore().get_courses('draft')
+        courses = modulestore().get_courses(branch='draft')
         # should have gotten 3 draft courses
         self.assertEqual(len(courses), 3, "Wrong number of courses")
         # check metadata -- NOTE no promised order
@@ -138,35 +138,40 @@ class SplitModuleCourseTests(SplitModuleTest):
 
     def test_branch_requests(self):
         # query w/ branch qualifier (both draft and published)
-        courses_published = modulestore().get_courses('published')
-        self.assertEqual(len(courses_published), 1, len(courses_published))
-        course = self.findByIdInResult(courses_published, "head23456")
-        self.assertIsNotNone(course, "published courses")
-        self.assertEqual(course.location.course_id, "wonderful")
-        self.assertEqual(str(course.location.version_guid), self.GUID_P,
-                         course.location.version_guid)
-        self.assertEqual(course.category, 'course', 'wrong category')
-        self.assertEqual(len(course.tabs), 4, "wrong number of tabs")
-        self.assertEqual(course.display_name, "The most wonderful course",
-                         course.display_name)
-        self.assertIsNone(course.advertised_start)
-        self.assertEqual(len(course.children), 0,
-                         "children")
+        def _verify_published_course(courses_published):
+            """ Helper function for verifying published course. """
+            self.assertEqual(len(courses_published), 1, len(courses_published))
+            course = self.findByIdInResult(courses_published, "head23456")
+            self.assertIsNotNone(course, "published courses")
+            self.assertEqual(course.location.course_id, "wonderful")
+            self.assertEqual(str(course.location.version_guid), self.GUID_P,
+                             course.location.version_guid)
+            self.assertEqual(course.category, 'course', 'wrong category')
+            self.assertEqual(len(course.tabs), 4, "wrong number of tabs")
+            self.assertEqual(course.display_name, "The most wonderful course",
+                             course.display_name)
+            self.assertIsNone(course.advertised_start)
+            self.assertEqual(len(course.children), 0,
+                             "children")
+
+        _verify_published_course(modulestore().get_courses(branch='published'))
+        # default for branch is 'published'.
+        _verify_published_course(modulestore().get_courses())
 
     def test_search_qualifiers(self):
         # query w/ search criteria
-        courses = modulestore().get_courses('draft', qualifiers={'org': 'testx'})
+        courses = modulestore().get_courses(branch='draft', qualifiers={'org': 'testx'})
         self.assertEqual(len(courses), 2)
         self.assertIsNotNone(self.findByIdInResult(courses, "head12345"))
         self.assertIsNotNone(self.findByIdInResult(courses, "head23456"))
 
         courses = modulestore().get_courses(
-            'draft',
+            branch='draft',
             qualifiers={'edited_on': {"$lt": datetime.datetime(2013, 3, 28, 15)}})
         self.assertEqual(len(courses), 2)
 
         courses = modulestore().get_courses(
-            'draft',
+            branch='draft',
             qualifiers={'org': 'testx', "prettyid": "test_course"})
         self.assertEqual(len(courses), 1)
         self.assertIsNotNone(self.findByIdInResult(courses, "head12345"))
@@ -415,14 +420,17 @@ class SplitModuleItemTests(SplitModuleTest):
         '''
         locator = CourseLocator(version_guid=self.GUID_D0)
         # get all modules
-        matches = modulestore().get_items(locator, {})
+        matches = modulestore().get_items(locator)
+        self.assertEqual(len(matches), 6)
+        matches = modulestore().get_items(locator, qualifiers={})
         self.assertEqual(len(matches), 6)
-        matches = modulestore().get_items(locator, {'category': 'chapter'})
+        matches = modulestore().get_items(locator, qualifiers={'category': 'chapter'})
         self.assertEqual(len(matches), 3)
-        matches = modulestore().get_items(locator, {'category': 'garbage'})
+        matches = modulestore().get_items(locator, qualifiers={'category': 'garbage'})
         self.assertEqual(len(matches), 0)
         matches = modulestore().get_items(
             locator,
+            qualifiers=
             {
                 'category': 'chapter',
                 'metadata': {'display_name': {'$regex': 'Hera'}}
@@ -430,7 +438,7 @@ class SplitModuleItemTests(SplitModuleTest):
         )
         self.assertEqual(len(matches), 2)
 
-        matches = modulestore().get_items(locator, {'children': 'chapter2'})
+        matches = modulestore().get_items(locator, qualifiers={'children': 'chapter2'})
         self.assertEqual(len(matches), 1)
         self.assertEqual(matches[0].location.usage_id, 'head12345')
 
@@ -438,8 +446,8 @@ class SplitModuleItemTests(SplitModuleTest):
         '''
         get_parent_locations(locator, [usage_id], [branch]): [BlockUsageLocator]
         '''
-        locator = CourseLocator(course_id="GreekHero", branch='draft')
-        parents = modulestore().get_parent_locations(locator, usage_id='chapter1')
+        locator = BlockUsageLocator(course_id="GreekHero", branch='draft', usage_id='chapter1')
+        parents = modulestore().get_parent_locations(locator)
         self.assertEqual(len(parents), 1)
         self.assertEqual(parents[0].usage_id, 'head12345')
         self.assertEqual(parents[0].course_id, "GreekHero")
@@ -447,7 +455,8 @@ class SplitModuleItemTests(SplitModuleTest):
         parents = modulestore().get_parent_locations(locator)
         self.assertEqual(len(parents), 1)
         self.assertEqual(parents[0].usage_id, 'head12345')
-        parents = modulestore().get_parent_locations(locator, usage_id='nosuchblock')
+        locator.usage_id='nosuchblock'
+        parents = modulestore().get_parent_locations(locator)
         self.assertEqual(len(parents), 0)
 
     def test_get_children(self):