Merge pull request #667 from edx/christina/read-only-api

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

cms/djangoapps/auth/tests/test_authz.py

@@ -181,7 +181,7 @@ class CourseGroupTest(TestCase):
         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 @@ class CourseGroupTest(TestCase):
         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 collections
 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 @@ 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. """
@@ -145,4 +150,3 @@ class ExtraPanelTabTestCase(TestCase):
                 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,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/lib/xmodule/xmodule/modulestore/__init__.py

@@ -235,8 +235,15 @@ class Location(_LocationBase):
 
     @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 @@ 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
+    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 @@ class ModuleStoreBase(ModuleStore):
         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 @@ class MongoModuleStore(ModuleStoreBase):
         # 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 @@ class MongoModuleStore(ModuleStoreBase):
             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 @@ class MongoModuleStore(ModuleStoreBase):
         # 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 @@ class MongoModuleStore(ModuleStoreBase):
         # 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 @@ 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,35 @@ class SplitMongoModuleStore(ModuleStoreBase):
         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 @@ 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_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

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,7 +138,8 @@ class SplitModuleCourseTests(SplitModuleTest):
 
     def test_branch_requests(self):
         # query w/ branch qualifier (both draft and published)
-        courses_published = modulestore().get_courses('published')
+        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")
@@ -153,20 +154,24 @@ class SplitModuleCourseTests(SplitModuleTest):
             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"))
@@ -322,11 +327,11 @@ class SplitModuleItemTests(SplitModuleTest):
         locator = BlockUsageLocator(version_guid=self.GUID_D1, usage_id='head12345')
         block = modulestore().get_item(locator)
         self.assertIsInstance(block, CourseDescriptor)
+        # get_instance just redirects to get_item, ignores course_id
+        self.assertIsInstance(modulestore().get_instance("course_id", locator), CourseDescriptor)
 
-        locator = BlockUsageLocator(course_id='GreekHero', usage_id='head12345', branch='draft')
-        block = modulestore().get_item(locator)
+        def verify_greek_hero(block):
             self.assertEqual(block.location.course_id, "GreekHero")
-        # look at this one in detail
             self.assertEqual(len(block.tabs), 6, "wrong number of tabs")
             self.assertEqual(block.display_name, "The Ancient Greek Hero")
             self.assertEqual(block.advertised_start, "Fall 2013")
@@ -338,6 +343,11 @@ class SplitModuleItemTests(SplitModuleTest):
                 block.grade_cutoffs, {"Pass": 0.45},
             )
 
+        locator = BlockUsageLocator(course_id='GreekHero', usage_id='head12345', branch='draft')
+        verify_greek_hero(modulestore().get_item(locator))
+        # get_instance just redirects to get_item, ignores course_id
+        verify_greek_hero(modulestore().get_instance("course_id", locator))
+
         # try to look up other branches
         self.assertRaises(ItemNotFoundError,
                           modulestore().get_item,
@@ -415,14 +425,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 +443,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 +451,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 +460,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):