diff --git a/cms/djangoapps/auth/tests/test_authz.py b/cms/djangoapps/auth/tests/test_authz.py index e04c108250..69050539cf 100644 --- a/cms/djangoapps/auth/tests/test_authz.py +++ b/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) diff --git a/cms/djangoapps/contentstore/tests/test_utils.py b/cms/djangoapps/contentstore/tests/test_utils.py index 26c49843b5..c3335aaaa0 100644 --- a/cms/djangoapps/contentstore/tests/test_utils.py +++ b/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) - 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 d98931f65e..9414bfb9e8 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/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index eb721dfc99..d616f21efa 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/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(): diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index 066197f4b2..21daff1875 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index c8ed57d027..74c7e7241a 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,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 diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_location.py b/common/lib/xmodule/xmodule/modulestore/tests/test_location.py index f0f0e8bf48..7e8ba1731b 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_location.py +++ b/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 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..9976a33a00 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")) @@ -322,21 +327,26 @@ 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) + + def verify_greek_hero(block): + self.assertEqual(block.location.course_id, "GreekHero") + 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") + self.assertEqual(len(block.children), 3) + self.assertEqual(block.definition_locator.definition_id, "head12345_12") + # check dates and graders--forces loading of descriptor + self.assertEqual(block.edited_by, "testassist@edx.org") + self.assertDictEqual( + block.grade_cutoffs, {"Pass": 0.45}, + ) locator = BlockUsageLocator(course_id='GreekHero', usage_id='head12345', branch='draft') - block = modulestore().get_item(locator) - 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") - self.assertEqual(len(block.children), 3) - self.assertEqual(block.definition_locator.definition_id, "head12345_12") - # check dates and graders--forces loading of descriptor - self.assertEqual(block.edited_by, "testassist@edx.org") - self.assertDictEqual( - block.grade_cutoffs, {"Pass": 0.45}, - ) + 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, @@ -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, {'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 +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):