diff --git a/cms/djangoapps/contentstore/tests/test_utils.py b/cms/djangoapps/contentstore/tests/test_utils.py index 26c49843b5..30e9e29451 100644 --- a/cms/djangoapps/contentstore/tests/test_utils.py +++ b/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. """ diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index a2e927ef46..d956a903b6 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/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' diff --git a/cms/djangoapps/contentstore/views/user.py b/cms/djangoapps/contentstore/views/user.py index 8b92107e88..db2c1eb058 100644 --- a/cms/djangoapps/contentstore/views/user.py +++ b/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, diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 0b061f5a94..2fe165077a 100644 --- a/common/djangoapps/student/views.py +++ b/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() diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index eb721dfc99..a2297a7d26 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/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): ''' diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index 066197f4b2..4fdbc4aef0 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index c8ed57d027..497bd7f792 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py index d71223f59b..f86a8dcaef 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/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, {'category': 'chapter'}) + matches = modulestore().get_items(locator, qualifiers={}) + self.assertEqual(len(matches), 6) + 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):