diff --git a/cms/djangoapps/contentstore/tests/test_crud.py b/cms/djangoapps/contentstore/tests/test_crud.py index 6fde065be1..2e02970d2e 100644 --- a/cms/djangoapps/contentstore/tests/test_crud.py +++ b/cms/djangoapps/contentstore/tests/test_crud.py @@ -12,7 +12,6 @@ from xmodule.html_module import HtmlDescriptor from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase -@unittest.skip("Not fixing split until we land opaque-keys 0.9") class TemplateTests(unittest.TestCase): """ Test finding and using the templates (boilerplates) for xblocks. @@ -55,25 +54,25 @@ class TemplateTests(unittest.TestCase): def test_factories(self): test_course = persistent_factories.PersistentCourseFactory.create( - course_id='testx.tempcourse', org='testx', + offering='tempcourse', org='testx', display_name='fun test course', user_id='testbot' ) self.assertIsInstance(test_course, CourseDescriptor) self.assertEqual(test_course.display_name, 'fun test course') - index_info = modulestore('split').get_course_index_info(test_course.location) + index_info = modulestore('split').get_course_index_info(test_course.id) self.assertEqual(index_info['org'], 'testx') - self.assertEqual(index_info['_id'], 'testx.tempcourse') + self.assertEqual(index_info['offering'], 'tempcourse') test_chapter = persistent_factories.ItemFactory.create(display_name='chapter 1', parent_location=test_course.location) self.assertIsInstance(test_chapter, SequenceDescriptor) # refetch parent which should now point to child - test_course = modulestore('split').get_course(test_course.id) + test_course = modulestore('split').get_course(test_course.id.version_agnostic()) self.assertIn(test_chapter.location.block_id, test_course.children) with self.assertRaises(DuplicateCourseError): persistent_factories.PersistentCourseFactory.create( - course_id='testx.tempcourse', org='testx', + offering='tempcourse', org='testx', display_name='fun test course', user_id='testbot' ) @@ -82,7 +81,7 @@ class TemplateTests(unittest.TestCase): Test create_xblock to create non persisted xblocks """ test_course = persistent_factories.PersistentCourseFactory.create( - course_id='testx.tempcourse', org='testx', + offering='tempcourse', org='testx', display_name='fun test course', user_id='testbot' ) @@ -109,7 +108,7 @@ class TemplateTests(unittest.TestCase): try saving temporary xblocks """ test_course = persistent_factories.PersistentCourseFactory.create( - course_id='testx.tempcourse', org='testx', + offering='tempcourse', org='testx', display_name='fun test course', user_id='testbot' ) test_chapter = modulestore('split').create_xblock( @@ -148,7 +147,7 @@ class TemplateTests(unittest.TestCase): def test_delete_course(self): test_course = persistent_factories.PersistentCourseFactory.create( - course_id='edu.harvard.history.doomed', org='testx', + offering='history.doomed', org='edu.harvard', display_name='doomed test course', user_id='testbot') persistent_factories.ItemFactory.create(display_name='chapter 1', @@ -171,7 +170,7 @@ class TemplateTests(unittest.TestCase): Test get_block_generations """ test_course = persistent_factories.PersistentCourseFactory.create( - course_id='edu.harvard.history.hist101', org='testx', + offering='history.hist101', org='edu.harvard', display_name='history test course', user_id='testbot' ) @@ -193,7 +192,9 @@ class TemplateTests(unittest.TestCase): second_problem = persistent_factories.ItemFactory.create( display_name='problem 2', - parent_location=BlockUsageLocator.make_relative(updated_loc, block_id=sub.location.block_id), + parent_location=BlockUsageLocator.make_relative( + updated_loc, block_type='problem', block_id=sub.location.block_id + ), user_id='testbot', category='problem', data="" ) diff --git a/cms/djangoapps/contentstore/views/tests/test_course_index.py b/cms/djangoapps/contentstore/views/tests/test_course_index.py index fc007db468..861c2a8862 100644 --- a/cms/djangoapps/contentstore/views/tests/test_course_index.py +++ b/cms/djangoapps/contentstore/views/tests/test_course_index.py @@ -7,7 +7,7 @@ import lxml from contentstore.tests.utils import CourseTestCase from contentstore.utils import reverse_course_url from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory -from xmodule.modulestore import parsers +from xmodule.modulestore.locator import Locator class TestCourseIndex(CourseTestCase): @@ -38,7 +38,7 @@ class TestCourseIndex(CourseTestCase): for link in course_link_eles: self.assertRegexpMatches( link.get("href"), - 'course/slashes:{0}'.format(parsers.ALLOWED_ID_CHARS) + 'course/slashes:{0}'.format(Locator.ALLOWED_ID_CHARS) ) # now test that url outline_response = authed_client.get(link.get("href"), {}, HTTP_ACCEPT='text/html') diff --git a/common/djangoapps/cache_toolbox/core.py b/common/djangoapps/cache_toolbox/core.py index 3321f83d5e..a0a512ca68 100644 --- a/common/djangoapps/cache_toolbox/core.py +++ b/common/djangoapps/cache_toolbox/core.py @@ -118,4 +118,9 @@ def get_cached_content(location): def del_cached_content(location): - cache.delete(unicode(location).encode("utf-8")) + # delete content for the given location, as well as for content with run=None. + # it's possible that the content could have been cached without knowing the + # course_key - and so without having the run. + cache.delete_many( + [unicode(loc).encode("utf-8") for loc in [location, location.replace(run=None)]] + ) diff --git a/common/djangoapps/student/migrations/0035_access_roles.py b/common/djangoapps/student/migrations/0035_access_roles.py index 799665250f..4c96d58f0c 100644 --- a/common/djangoapps/student/migrations/0035_access_roles.py +++ b/common/djangoapps/student/migrations/0035_access_roles.py @@ -9,6 +9,7 @@ from opaque_keys import InvalidKeyError import bson.son import logging from django.db.models.query_utils import Q +from django.db.utils import IntegrityError log = logging.getLogger(__name__) @@ -28,69 +29,72 @@ class Migration(DataMigration): loc_map_collection = loc_mapper().location_map # b/c the Groups table had several entries for each course, we need to ensure we process each unique # course only once. The below datastructures help ensure that. - hold = {} - done = set() - orgs = {} - query = Q(name__startswith='course_creator_group') + hold = {} # key of course_id_strings with array of group objects. Should only be org scoped entries + # or deleted courses + orgs = {} # downcased org to last recorded normal case of the org + query = Q(name='course_creator_group') for role in ['staff', 'instructor', 'beta_testers', ]: query = query | Q(name__startswith=role) for group in orm['auth.Group'].objects.filter(query).all(): - def _migrate_users(correct_course_key, lower_org): + def _migrate_users(correct_course_key, role, lower_org): """ Get all the users from the old group and migrate to this course key in the new table """ for user in orm['auth.user'].objects.filter(groups=group).all(): entry = orm['student.courseaccessrole']( - role=parsed_entry.group('role_id'), user=user, - org=correct_course_key.org, course_id=correct_course_key.to_deprecated_string() + role=role, user=user, + org=correct_course_key.org, course_id=correct_course_key ) - entry.save() + try: + entry.save() + except IntegrityError: + # already stored + pass orgs[lower_org] = correct_course_key.org - done.add(correct_course_key) - # should this actually loop through all groups and log any which are not compliant? That is, - # remove the above filter parsed_entry = self.GROUP_ENTRY_RE.match(group.name) - if parsed_entry.group('role_id') == 'course_creator_group': + role = parsed_entry.group('role_id') + if role == 'course_creator_group': for user in orm['auth.user'].objects.filter(groups=group).all(): - entry = orm['student.courseaccessrole'](role=parsed_entry.group('role_id'), user=user) + entry = orm['student.courseaccessrole'](role=role, user=user) entry.save() else: course_id_string = parsed_entry.group('course_id_string') try: course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id_string) - if course_key not in done: - # is the downcased version, get the normal cased one. loc_mapper() has no - # methods taking downcased SSCK; so, need to do it manually here - correct_course_key = self._map_downcased_ssck(course_key, loc_map_collection, done) - _migrate_users(correct_course_key, course_key.org) - done.add(course_key) + # course_key is the downcased version, get the normal cased one. loc_mapper() has no + # methods taking downcased SSCK; so, need to do it manually here + correct_course_key = self._map_downcased_ssck(course_key, loc_map_collection) + if correct_course_key is not None: + _migrate_users(correct_course_key, role, course_key.org) except InvalidKeyError: entry = loc_map_collection.find_one({ 'course_id': re.compile(r'^{}$'.format(course_id_string), re.IGNORECASE) }) if entry is None: - # not a course_id as far as we can tell - if course_id_string not in done: - hold[course_id_string] = group + hold.setdefault(course_id_string, []).append(group) else: - correct_course_key = self._cache_done_return_ssck(entry, done) - _migrate_users(correct_course_key, entry['lower_id']['org']) + correct_course_key = SlashSeparatedCourseKey(*entry['_id'].values()) + _migrate_users(correct_course_key, role, entry['lower_id']['org']) - # see if any in hold ere missed above - for not_ssck, group in hold.iteritems(): - if not_ssck not in done: - if not_ssck in orgs: + # see if any in hold were missed above + for held_auth_scope, groups in hold.iteritems(): + # orgs indexed by downcased org + held_auth_scope = held_auth_scope.lower() + if held_auth_scope in orgs: + for group in groups: + role = self.GROUP_ENTRY_RE.match(group.name).group('role_id') # they have org permission for user in orm['auth.user'].objects.filter(groups=group).all(): entry = orm['student.courseaccessrole']( - role=parsed_entry.group('role_id'), user=user, - org=orgs[not_ssck], + role=role, + user=user, + org=orgs[held_auth_scope], ) entry.save() - else: - # should this just log or really make an effort to do the conversion? - log.warn("Didn't convert role %s", group.name) + else: + # don't silently skip unexpected roles + log.warn("Didn't convert roles %s", [group.name for group in groups]) def backwards(self, orm): "Write your backwards methods here." @@ -98,10 +102,9 @@ class Migration(DataMigration): # the semantic of backwards should be other than perhaps clearing the table. orm['student.courseaccessrole'].objects.all().delete() - def _map_downcased_ssck(self, downcased_ssck, loc_map_collection, done): + def _map_downcased_ssck(self, downcased_ssck, loc_map_collection): """ - Get the normal cased version of this downcased slash sep course key and add - the lowercased locator form to done map + Get the normal cased version of this downcased slash sep course key """ # given the regex, the son may be an overkill course_son = bson.son.SON([ @@ -111,25 +114,11 @@ class Migration(DataMigration): ]) entry = loc_map_collection.find_one(course_son) if entry: - return self._cache_done_return_ssck(entry, done) + idpart = entry['_id'] + return SlashSeparatedCourseKey(idpart['org'], idpart['course'], idpart['name']) else: return None - def _cache_done_return_ssck(self, entry, done): - """ - Add all the various formats which auth may use to the done set and return the ssck for the entry - """ - # cache that the dotted form is done too - if 'lower_course_id' in entry: - done.add(entry['lower_course_id']) - elif 'course_id' in entry: - done.add(entry['course_id'].lower()) - elif 'lower_org' in entry: - done.add('{}.{}'.format(entry['lower_org'], entry['lower_offering'])) - else: - done.add('{}.{}'.format(entry['org'].lower(), entry['offering'].lower())) - return SlashSeparatedCourseKey(*entry['_id'].values()) - models = { 'auth.group': { diff --git a/common/lib/xmodule/setup.py b/common/lib/xmodule/setup.py index 55d12c74f1..2e2e5ba340 100644 --- a/common/lib/xmodule/setup.py +++ b/common/lib/xmodule/setup.py @@ -79,5 +79,8 @@ setup( 'asset-location = xmodule.modulestore.locations:AssetLocation', 'edx = xmodule.modulestore.locator:BlockUsageLocator', ], + 'definition_key': [ + 'defx = xmodule.modulestore.locator:DefinitionLocator', + ], }, ) diff --git a/common/lib/xmodule/xmodule/modulestore/keys.py b/common/lib/xmodule/xmodule/modulestore/keys.py index b1fe2277d2..3f4a04872b 100644 --- a/common/lib/xmodule/xmodule/modulestore/keys.py +++ b/common/lib/xmodule/xmodule/modulestore/keys.py @@ -58,7 +58,7 @@ class DefinitionKey(OpaqueKey): KEY_TYPE = 'definition_key' __slots__ = () - @abstractmethod + @abstractproperty def block_type(self): """ The XBlock type of this definition. @@ -125,6 +125,10 @@ class UsageKey(CourseObjectMixin, OpaqueKey): """ raise NotImplementedError() + @property + def block_type(self): + return self.category + class OpaqueKeyReader(IdReader): """ diff --git a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py b/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py index 929e0e9bb0..03cf5c8456 100644 --- a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py +++ b/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py @@ -150,6 +150,7 @@ class LocMapperStore(object): entry = self._migrate_if_necessary([entry])[0] block_id = entry['block_map'].get(self.encode_key_for_mongo(location.name)) + category = location.category if block_id is None: if add_entry_if_missing: block_id = self._add_to_block_map( @@ -159,14 +160,15 @@ class LocMapperStore(object): raise ItemNotFoundError(location) else: # jump_to_id uses a None category. - if location.category is None: + if category is None: if len(block_id) == 1: # unique match (most common case) + category = block_id.keys()[0] block_id = block_id.values()[0] else: raise InvalidLocationError() - elif location.category in block_id: - block_id = block_id[location.category] + elif category in block_id: + block_id = block_id[category] elif add_entry_if_missing: block_id = self._add_to_block_map(location, course_son, entry['block_map']) else: @@ -179,10 +181,12 @@ class LocMapperStore(object): ) published_usage = BlockUsageLocator( prod_course_locator, + block_type=category, block_id=block_id ) draft_usage = BlockUsageLocator( prod_course_locator.for_branch(entry['draft_branch']), + block_type=category, block_id=block_id ) if published: @@ -285,6 +289,7 @@ class LocMapperStore(object): org=entry[entry_org], offering=entry[entry_offering], branch=entry['prod_branch'] ), + block_type=category, block_id=block_id ) draft_locator = BlockUsageLocator( @@ -292,6 +297,7 @@ class LocMapperStore(object): org=entry[entry_org], offering=entry[entry_offering], branch=entry['draft_branch'] ), + block_type=category, block_id=block_id ) self._cache_location_map_entry(location, published_locator, draft_locator) diff --git a/common/lib/xmodule/xmodule/modulestore/locator.py b/common/lib/xmodule/xmodule/modulestore/locator.py index deb1105729..e164216552 100644 --- a/common/lib/xmodule/xmodule/modulestore/locator.py +++ b/common/lib/xmodule/xmodule/modulestore/locator.py @@ -13,15 +13,7 @@ from bson.errors import InvalidId from opaque_keys import OpaqueKey, InvalidKeyError -from xmodule.modulestore.keys import CourseKey, UsageKey - -from xmodule.modulestore.parsers import ( - parse_url, - parse_block_ref, - BRANCH_PREFIX, - BLOCK_PREFIX, - VERSION_PREFIX, - ALLOWED_ID_RE) +from xmodule.modulestore.keys import CourseKey, UsageKey, DefinitionKey log = logging.getLogger(__name__) @@ -45,14 +37,10 @@ class Locator(OpaqueKey): Locator is an abstract base class: do not instantiate """ - @abstractmethod - def url(self): - """ - Return a string containing the URL for this location. Raises - InvalidKeyError if the instance doesn't have a - complete enough specification to generate a url - """ - raise NotImplementedError() + BLOCK_TYPE_PREFIX = r"type" + # Prefix for the version portion of a locator URL, when it is preceded by a course ID + VERSION_PREFIX = r"version" + ALLOWED_ID_CHARS = r'[\w\-~.:]' def __str__(self): ''' @@ -86,35 +74,42 @@ class BlockLocatorBase(Locator): # Token separating org from offering ORG_SEPARATOR = '+' - def version(self): - """ - Returns the ObjectId referencing this specific location. - """ - return self.version_guid + # Prefix for the branch portion of a locator URL + BRANCH_PREFIX = r"branch" + # Prefix for the block portion of a locator URL + BLOCK_PREFIX = r"block" + + ALLOWED_ID_RE = re.compile(r'^' + Locator.ALLOWED_ID_CHARS + '+$', re.UNICODE) + + URL_RE_SOURCE = r""" + ((?P{ALLOWED_ID_CHARS}+)\+(?P{ALLOWED_ID_CHARS}+)\+?)?? + ({BRANCH_PREFIX}\+(?P{ALLOWED_ID_CHARS}+)\+?)? + ({VERSION_PREFIX}\+(?P[A-F0-9]+)\+?)? + ({BLOCK_TYPE_PREFIX}\+(?P{ALLOWED_ID_CHARS}+)\+?)? + ({BLOCK_PREFIX}\+(?P{ALLOWED_ID_CHARS}+))? + """.format( + ALLOWED_ID_CHARS=Locator.ALLOWED_ID_CHARS, BRANCH_PREFIX=BRANCH_PREFIX, + VERSION_PREFIX=Locator.VERSION_PREFIX, BLOCK_TYPE_PREFIX=Locator.BLOCK_TYPE_PREFIX, BLOCK_PREFIX=BLOCK_PREFIX + ) + + URL_RE = re.compile('^' + URL_RE_SOURCE + '$', re.IGNORECASE | re.VERBOSE | re.UNICODE) - def url(self): - """ - Return a string containing the URL for this location. - """ - return self.NAMESPACE_SEPARATOR.join([self.CANONICAL_NAMESPACE, self._to_string()]) @classmethod - def _parse_url(cls, url): + def parse_url(cls, string): """ - url must be a string beginning with 'edx:' and containing - either a valid version_guid or org & offering (with optional branch), or both. + Raises InvalidKeyError if string cannot be parsed. + + If it can be parsed as a version_guid with no preceding org + offering, returns a dict + with key 'version_guid' and the value, + + If it can be parsed as a org + offering, returns a dict + with key 'id' and optional keys 'branch' and 'version_guid'. """ - if not isinstance(url, basestring): - raise TypeError('%s is not an instance of basestring' % url) - - parse = parse_url(url) - if not parse: - raise InvalidKeyError(cls, url) - - if parse['version_guid']: - parse['version_guid'] = cls.as_object_id(parse['version_guid']) - - return parse + match = cls.URL_RE.match(string) + if not match: + raise InvalidKeyError(cls, string) + return match.groupdict() @property def package_id(self): @@ -130,13 +125,10 @@ class CourseLocator(BlockLocatorBase, CourseKey): CourseLocator(version_guid=ObjectId('519665f6223ebd6980884f2b')) CourseLocator(org='mit.eecs', offering='6.002x') CourseLocator(org='mit.eecs', offering='6002x', branch = 'published') - CourseLocator.from_string('edx:version/519665f6223ebd6980884f2b') - CourseLocator.from_string('version/519665f6223ebd6980884f2b') - CourseLocator.from_string('edx:mit.eecs+6002x') - CourseLocator.from_string('mit.eecs+6002x') - CourseLocator.from_string('edx:mit.eecs+6002x/branch/published') - CourseLocator.from_string('edx:mit.eecs+6002x/branch/published/version/519665f6223ebd6980884f2b') - CourseLocator.from_string('mit.eecs+6002x/branch/published/version/519665f6223ebd6980884f2b') + CourseLocator.from_string('course-locator:version+519665f6223ebd6980884f2b') + CourseLocator.from_string('course-locator:mit.eecs+6002x') + CourseLocator.from_string('course-locator:mit.eecs+6002x+branch+published') + CourseLocator.from_string('course-locator:mit.eecs+6002x+branch+published+version+519665f6223ebd6980884f2b') Should have at least a specific org & offering (id for the course as if it were a project w/ versions) with optional 'branch', @@ -163,7 +155,7 @@ class CourseLocator(BlockLocatorBase, CourseKey): if version_guid: version_guid = self.as_object_id(version_guid) - if not all(field is None or ALLOWED_ID_RE.match(field) for field in [org, offering, branch]): + if not all(field is None or self.ALLOWED_ID_RE.match(field) for field in [org, offering, branch]): raise InvalidKeyError(self.__class__, [org, offering, branch]) super(CourseLocator, self).__init__( @@ -173,31 +165,27 @@ class CourseLocator(BlockLocatorBase, CourseKey): version_guid=version_guid ) - if self.version_guid is None and self.org is None and self.offering is None: + if self.version_guid is None and (self.org is None or self.offering is None): raise InvalidKeyError(self.__class__, "Either version_guid or org and offering should be set") + def version(self): + """ + Returns the ObjectId referencing this specific location. + """ + return self.version_guid + @classmethod def _from_string(cls, serialized): """ Return a CourseLocator parsing the given serialized string :param serialized: matches the string to a CourseLocator """ - kwargs = cls._parse_url(serialized) - try: - return cls(**{key: kwargs.get(key) for key in cls.KEY_FIELDS}) - except ValueError: - raise InvalidKeyError(cls, "Either version_guid or org and offering should be set: {}".format(serialized)) + parse = cls.parse_url(serialized) - def is_fully_specified(self): - """ - Returns True if either version_guid is specified, or org+offering+branch - are specified. - This should always return True, since this should be validated in the constructor. - """ - return ( - self.version_guid is not None or - (self.org is not None and self.offering is not None and self.branch is not None) - ) + if parse['version_guid']: + parse['version_guid'] = cls.as_object_id(parse['version_guid']) + + return cls(**{key: parse.get(key) for key in cls.KEY_FIELDS}) def html_id(self): """ @@ -212,6 +200,7 @@ class CourseLocator(BlockLocatorBase, CourseKey): def make_usage_key(self, block_type, block_id): return BlockUsageLocator( course_key=self, + block_type=block_type, block_id=block_id ) @@ -280,13 +269,13 @@ class CourseLocator(BlockLocatorBase, CourseKey): if self.offering: parts.append(unicode(self.package_id)) if self.branch: - parts.append(u"{prefix}{branch}".format(prefix=BRANCH_PREFIX, branch=self.branch)) + parts.append(u"{prefix}+{branch}".format(prefix=self.BRANCH_PREFIX, branch=self.branch)) if self.version_guid: - parts.append(u"{prefix}{guid}".format(prefix=VERSION_PREFIX, guid=self.version_guid)) - return u"/".join(parts) + parts.append(u"{prefix}+{guid}".format(prefix=self.VERSION_PREFIX, guid=self.version_guid)) + return u"+".join(parts) -class BlockUsageLocator(BlockLocatorBase, UsageKey): # TODO implement UsageKey methods +class BlockUsageLocator(BlockLocatorBase, UsageKey): """ Encodes a location. @@ -305,12 +294,13 @@ class BlockUsageLocator(BlockLocatorBase, UsageKey): # TODO implement UsageKey branch : string """ CANONICAL_NAMESPACE = 'edx' - KEY_FIELDS = ('course_key', 'block_id') + KEY_FIELDS = ('course_key', 'block_type', 'block_id') # fake out class instrospection as this is an attr in this class's instances course_key = None + block_type = None - def __init__(self, course_key, block_id): + def __init__(self, course_key, block_type, block_id): """ Construct a BlockUsageLocator """ @@ -318,7 +308,7 @@ class BlockUsageLocator(BlockLocatorBase, UsageKey): # TODO implement UsageKey if block_id is None: raise InvalidKeyError(self.__class__, "Missing block id") - super(BlockUsageLocator, self).__init__(course_key=course_key, block_id=block_id) + super(BlockUsageLocator, self).__init__(course_key=course_key, block_type=block_type, block_id=block_id) @classmethod def _from_string(cls, serialized): @@ -326,11 +316,11 @@ class BlockUsageLocator(BlockLocatorBase, UsageKey): # TODO implement UsageKey Requests CourseLocator to deserialize its part and then adds the local deserialization of block """ course_key = CourseLocator._from_string(serialized) - parsed_parts = parse_url(serialized) - block_id = parsed_parts.get('block_id') + parsed_parts = cls.parse_url(serialized) + block_id = parsed_parts.get('block_id', None) if block_id is None: raise InvalidKeyError(cls, serialized) - return cls(course_key, block_id) + return cls(course_key, parsed_parts.get('block_type'), block_id) def version_agnostic(self): """ @@ -342,7 +332,8 @@ class BlockUsageLocator(BlockLocatorBase, UsageKey): # TODO implement UsageKey """ return BlockUsageLocator( course_key=self.course_key.version_agnostic(), - block_id=self.block_id + block_type=self.block_type, + block_id=self.block_id, ) def course_agnostic(self): @@ -354,6 +345,7 @@ class BlockUsageLocator(BlockLocatorBase, UsageKey): # TODO implement UsageKey """ return BlockUsageLocator( course_key=self.course_key.course_agnostic(), + block_type=self.block_type, block_id=self.block_id ) @@ -363,6 +355,17 @@ class BlockUsageLocator(BlockLocatorBase, UsageKey): # TODO implement UsageKey """ return BlockUsageLocator( self.course_key.for_branch(branch), + block_type=self.block_type, + block_id=self.block_id + ) + + def for_version(self, version_guid): + """ + Return a UsageLocator for the same block in a different branch of the course. + """ + return BlockUsageLocator( + self.course_key.for_version(version_guid), + block_type=self.block_type, block_id=self.block_id ) @@ -370,11 +373,10 @@ class BlockUsageLocator(BlockLocatorBase, UsageKey): # TODO implement UsageKey def _parse_block_ref(cls, block_ref): if isinstance(block_ref, LocalId): return block_ref + elif len(block_ref) > 0 and cls.ALLOWED_ID_RE.match(block_ref): + return block_ref else: - parse = parse_block_ref(block_ref) - if not parse: - raise InvalidKeyError(cls, block_ref) - return parse.get('block_id') + raise InvalidKeyError(cls, block_ref) @property def definition_key(self): @@ -400,6 +402,9 @@ class BlockUsageLocator(BlockLocatorBase, UsageKey): # TODO implement UsageKey def version_guid(self): return self.course_key.version_guid + def version(self): + return self.course_key.version_guid + @property def name(self): """ @@ -411,7 +416,7 @@ class BlockUsageLocator(BlockLocatorBase, UsageKey): # TODO implement UsageKey return self.course_key.is_fully_specified() @classmethod - def make_relative(cls, course_locator, block_id): + def make_relative(cls, course_locator, block_type, block_id): """ Return a new instance which has the given block_id in the given course :param course_locator: may be a BlockUsageLocator in the same snapshot @@ -420,6 +425,7 @@ class BlockUsageLocator(BlockLocatorBase, UsageKey): # TODO implement UsageKey course_locator = course_locator.course_key return BlockUsageLocator( course_key=course_locator, + block_type=block_type, block_id=block_id ) @@ -428,34 +434,17 @@ class BlockUsageLocator(BlockLocatorBase, UsageKey): # TODO implement UsageKey Return a new instance which has the this block_id in the given course :param course_key: a CourseKey object representing the new course to map into """ - return BlockUsageLocator.make_relative(course_key, self.block_id) - - def url_reverse(self, prefix, postfix=''): - """ - Do what reverse is supposed to do but seems unable to do. Generate a url using prefix unicode(self) postfix - :param prefix: the beginning of the url (will be forced to begin and end with / if non-empty) - :param postfix: the part to append to the url (will be forced to begin w/ / if non-empty) - """ - if prefix: - if not prefix.endswith('/'): - prefix += '/' - if not prefix.startswith('/'): - prefix = '/' + prefix - else: - prefix = '/' - if postfix and not postfix.startswith('/'): - postfix = '/' + postfix - elif postfix is None: - postfix = '' - return prefix + unicode(self) + postfix + return BlockUsageLocator.make_relative(course_key, self.block_type, self.block_id) def _to_string(self): """ Return a string representing this location. """ - return u"{course_key}/{BLOCK_PREFIX}{block_id}".format( + return u"{course_key}+{BLOCK_TYPE_PREFIX}+{block_type}+{BLOCK_PREFIX}+{block_id}".format( course_key=self.course_key._to_string(), - BLOCK_PREFIX=BLOCK_PREFIX, + BLOCK_TYPE_PREFIX=self.BLOCK_TYPE_PREFIX, + block_type=self.block_type, + BLOCK_PREFIX=self.BLOCK_PREFIX, block_id=self.block_id ) @@ -467,43 +456,61 @@ class BlockUsageLocator(BlockLocatorBase, UsageKey): # TODO implement UsageKey place, but I have no way to override. We should clearly define the purpose and restrictions of this (e.g., I'm assuming periods are fine). """ - return re.sub('[^\w-]', '-', self._to_string()) + return unicode(self) -class DefinitionLocator(Locator): +class DefinitionLocator(Locator, DefinitionKey): """ Container for how to locate a description (the course-independent content). """ CANONICAL_NAMESPACE = 'defx' - KEY_FIELDS = ('definition_id',) + KEY_FIELDS = ('definition_id', 'block_type') - URL_RE = re.compile(r'^defx:' + VERSION_PREFIX + '([^/]+)$', re.IGNORECASE) + # override the abstractproperty + block_type = None + definition_id = None - def __init__(self, definition_id): + def __init__(self, block_type, definition_id): if isinstance(definition_id, LocalId): - super(DefinitionLocator, self).__init__(definition_id) + super(DefinitionLocator, self).__init__(definition_id=definition_id, block_type=block_type) elif isinstance(definition_id, basestring): - regex_match = self.URL_RE.match(definition_id) - if regex_match is not None: - super(DefinitionLocator, self).__init__(self.as_object_id(regex_match.group(1))) - else: - super(DefinitionLocator, self).__init__(self.as_object_id(definition_id)) - else: - super(DefinitionLocator, self).__init__(self.as_object_id(definition_id)) + try: + definition_id = self.as_object_id(definition_id) + except ValueError: + raise InvalidKeyError(self, definition_id) + super(DefinitionLocator, self).__init__(definition_id=definition_id, block_type=block_type) + elif isinstance(definition_id, ObjectId): + super(DefinitionLocator, self).__init__(definition_id=definition_id, block_type=block_type) def _to_string(self): ''' Return a string representing this location. - unicode(self) returns something like this: "version/519665f6223ebd6980884f2b" + unicode(self) returns something like this: "519665f6223ebd6980884f2b+type+problem" ''' - return VERSION_PREFIX + str(self.definition_id) + return u"{}+{}+{}".format(unicode(self.definition_id), self.BLOCK_TYPE_PREFIX, self.block_type) - def url(self): + URL_RE = re.compile( + r"^(?P[A-F0-9]+)\+{}\+(?P{ALLOWED_ID_CHARS}+)$".format( + Locator.BLOCK_TYPE_PREFIX, ALLOWED_ID_CHARS=Locator.ALLOWED_ID_CHARS + ), + re.IGNORECASE | re.VERBOSE | re.UNICODE + ) + + @classmethod + def _from_string(cls, serialized): """ - Return a string containing the URL for this location. - url(self) returns something like this: 'defx:version/519665f6223ebd6980884f2b' + Return a DefinitionLocator parsing the given serialized string + :param serialized: matches the string to """ - return u'defx:' + self._to_string() + parse = cls.URL_RE.match(serialized) + if not parse: + raise InvalidKeyError(cls, serialized) + + parse = parse.groupdict() + if parse['definition_id']: + parse['definition_id'] = cls.as_object_id(parse['definition_id']) + + return cls(**{key: parse.get(key) for key in cls.KEY_FIELDS}) def version(self): """ diff --git a/common/lib/xmodule/xmodule/modulestore/parsers.py b/common/lib/xmodule/xmodule/modulestore/parsers.py deleted file mode 100644 index 1bb2d12317..0000000000 --- a/common/lib/xmodule/xmodule/modulestore/parsers.py +++ /dev/null @@ -1,63 +0,0 @@ -import re - -# Prefix for the branch portion of a locator URL -BRANCH_PREFIX = r"branch/" -# Prefix for the block portion of a locator URL -BLOCK_PREFIX = r"block/" -# Prefix for the version portion of a locator URL, when it is preceded by a course ID -VERSION_PREFIX = r"version/" - -ALLOWED_ID_CHARS = r'[\w\-~.:+]' -ALLOWED_ID_RE = re.compile(r'^{}+$'.format(ALLOWED_ID_CHARS), re.UNICODE) - -# NOTE: if we need to support period in place of +, make it aggressive (take the first period in the string) -URL_RE_SOURCE = r""" - ((?P{ALLOWED_ID_CHARS}+)\+(?P{ALLOWED_ID_CHARS}+)/?)? - ({BRANCH_PREFIX}(?P{ALLOWED_ID_CHARS}+)/?)? - ({VERSION_PREFIX}(?P[A-F0-9]+)/?)? - ({BLOCK_PREFIX}(?P{ALLOWED_ID_CHARS}+))? - """.format( - ALLOWED_ID_CHARS=ALLOWED_ID_CHARS, BRANCH_PREFIX=BRANCH_PREFIX, - VERSION_PREFIX=VERSION_PREFIX, BLOCK_PREFIX=BLOCK_PREFIX - ) - -URL_RE = re.compile('^' + URL_RE_SOURCE + '$', re.IGNORECASE | re.VERBOSE | re.UNICODE) - - -def parse_url(string): - """ - followed by either a version_guid or a org + offering pair. If tag_optional, then - the url does not have to start with the tag and edx will be assumed. - - Examples: - 'edx:version/0123FFFF' - 'edx:mit.eecs.6002x' - 'edx:mit.eecs.6002x/branch/published' - 'edx:mit.eecs.6002x/branch/published/block/HW3' - 'edx:mit.eecs.6002x/branch/published/version/000eee12345/block/HW3' - - This returns None if string cannot be parsed. - - If it can be parsed as a version_guid with no preceding org + offering, returns a dict - with key 'version_guid' and the value, - - If it can be parsed as a org + offering, returns a dict - with key 'id' and optional keys 'branch' and 'version_guid'. - """ - match = URL_RE.match(string) - if not match: - return None - matched_dict = match.groupdict() - return matched_dict - - -def parse_block_ref(string): - r""" - A block_ref is a string of url safe characters (see ALLOWED_ID_CHARS) - - If string is a block_ref, returns a dict with key 'block_ref' and the value, - otherwise returns None. - """ - if ALLOWED_ID_RE.match(string): - return {'block_id': string} - return None diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py index bb50171d38..a81dec3108 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py @@ -108,6 +108,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem): offering=course_entry_override.get('offering'), branch=course_entry_override.get('branch'), ), + block_type=json_data.get('category'), block_id=block_id, ) @@ -131,6 +132,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem): self, BlockUsageLocator( CourseLocator(version_guid=course_entry_override['structure']['_id']), + block_type='error', block_id=block_id ), error_msg=exc_info_to_str(sys.exc_info()) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py index ded67104b4..261e63d3ac 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py @@ -8,14 +8,14 @@ class DefinitionLazyLoader(object): object doesn't force access during init but waits until client wants the definition. Only works if the modulestore is a split mongo store. """ - def __init__(self, modulestore, definition_id): + def __init__(self, modulestore, block_type, definition_id): """ Simple placeholder for yet-to-be-fetched data :param modulestore: the pymongo db connection with the definitions :param definition_locator: the id of the record in the above to fetch """ self.modulestore = modulestore - self.definition_locator = DefinitionLocator(definition_id) + self.definition_locator = DefinitionLocator(block_type, definition_id) def fetch(self): """ diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 91f94a8bcc..d1e4725ec0 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -152,7 +152,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): if lazy: for block in new_module_data.itervalues(): - block['definition'] = DefinitionLazyLoader(self, block['definition']) + block['definition'] = DefinitionLazyLoader(self, block['category'], block['definition']) else: # Load all descendants by id descendent_definitions = self.db_connection.find_matching_definitions({ @@ -242,11 +242,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): :param course_locator: any subclass of CourseLocator ''' - # NOTE: if and when this uses cache, the update if changed logic will break if the cache - # holds the same objects as the descriptors! - if not course_locator.is_fully_specified(): - raise InsufficientSpecificationError('Not fully specified: %s' % course_locator) - if course_locator.org and course_locator.offering and course_locator.branch: # use the course id index = self.db_connection.get_course_index(course_locator) @@ -258,6 +253,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): if course_locator.version_guid is not None and version_guid != course_locator.version_guid: # This may be a bit too touchy but it's hard to infer intent raise VersionConflictError(course_locator, version_guid) + elif course_locator.version_guid is None: + raise InsufficientSpecificationError(course_locator) else: # TODO should this raise an exception if branch was provided? version_guid = course_locator.version_guid @@ -322,9 +319,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): def get_course(self, course_id, depth=None): ''' Gets the course descriptor for the course identified by the locator - which may or may not be a blockLocator. - - raises InsufficientSpecificationError ''' assert(isinstance(course_id, CourseLocator)) course_entry = self._lookup_course(course_id) @@ -458,6 +452,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): return [ BlockUsageLocator.make_relative( locator, + block_type=course['structure']['blocks'][parent_id].get('category'), block_id=LocMapperStore.decode_key_from_mongo(parent_id), ) for parent_id in items @@ -471,12 +466,13 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): course = self._lookup_course(course_key) items = {LocMapperStore.decode_key_from_mongo(block_id) for block_id in course['structure']['blocks'].keys()} items.remove(course['structure']['root']) - for block_id, block_data in course['structure']['blocks'].iteritems(): + blocks = course['structure']['blocks'] + for block_id, block_data in blocks.iteritems(): items.difference_update(block_data.get('fields', {}).get('children', [])) if block_data['category'] in detached_categories: items.discard(LocMapperStore.decode_key_from_mongo(block_id)) return [ - BlockUsageLocator(course_key=course_key, block_id=block_id) + BlockUsageLocator(course_key=course_key, block_type=blocks[block_id]['category'], block_id=block_id) for block_id in items ] @@ -613,11 +609,11 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): # convert the results value sets to locators for k, versions in result.iteritems(): result[k] = [ - BlockUsageLocator(CourseLocator(version_guid=version), block_id=block_id) + block_locator.for_version(version) for version in versions ] return VersionTree( - BlockUsageLocator(CourseLocator(version_guid=possible_roots[0]), block_id=block_id), + block_locator.for_version(possible_roots[0]), result ) @@ -650,7 +646,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): 'schema_version': self.SCHEMA_VERSION, } self.db_connection.insert_definition(document) - definition_locator = DefinitionLocator(new_id) + definition_locator = DefinitionLocator(category, new_id) return definition_locator def update_definition_from_data(self, definition_locator, new_def_data, user_id): @@ -685,7 +681,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): old_definition['edit_info']['previous_version'] = definition_locator.definition_id old_definition['schema_version'] = self.SCHEMA_VERSION self.db_connection.insert_definition(old_definition) - return DefinitionLocator(old_definition['_id']), True + return DefinitionLocator(old_definition['category'], old_definition['_id']), True else: return definition_locator, False @@ -829,11 +825,13 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): self._update_head(index_entry, course_or_parent_locator.branch, new_id) item_loc = BlockUsageLocator( course_or_parent_locator.version_agnostic(), + block_type=category, block_id=new_block_id, ) else: item_loc = BlockUsageLocator( CourseLocator(version_guid=new_id), + block_type=category, block_id=new_block_id, ) @@ -1029,7 +1027,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): course_key = CourseLocator(version_guid=new_id) # fetch and return the new item--fetching is unnecessary but a good qc step - new_locator = BlockUsageLocator(course_key, descriptor.location.block_id) + new_locator = descriptor.location.map_into_course(course_key) return self.get_item(new_locator) else: # nothing changed, just return the one sent in @@ -1101,18 +1099,14 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): self._update_head(index_entry, xblock.location.branch, new_id) # fetch and return the new item--fetching is unnecessary but a good qc step - return self.get_item( - BlockUsageLocator( - xblock.location.course_key.for_version(new_id), - block_id=xblock.location.block_id, - ) - ) + return self.get_item(xblock.location.for_version(new_id)) else: return xblock def _persist_subdag(self, xblock, user_id, structure_blocks, new_id): # persist the definition if persisted != passed new_def_data = self._filter_special_fields(xblock.get_explicitly_set_fields_by_scope(Scope.content)) + is_updated = False if xblock.definition_locator is None or isinstance(xblock.definition_locator.definition_id, LocalId): xblock.definition_locator = self.create_definition_from_data( new_def_data, xblock.category, user_id) @@ -1134,9 +1128,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): else: is_new = False encoded_block_id = LocMapperStore.encode_key_for_mongo(xblock.location.block_id) - is_updated = is_updated or ( - xblock.has_children and structure_blocks[encoded_block_id]['fields']['children'] != xblock.children - ) children = [] if xblock.has_children: @@ -1147,6 +1138,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): children.append(child_block.location.block_id) else: children.append(child) + is_updated = is_updated or structure_blocks[encoded_block_id]['fields']['children'] != children block_fields = xblock.get_explicitly_set_fields_by_scope(Scope.settings) if not is_new and not is_updated: @@ -1419,9 +1411,9 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): if isinstance(definition, DefinitionLazyLoader): return definition.definition_locator elif '_id' not in definition: - return DefinitionLocator(LocalId()) + return DefinitionLocator(definition.get('category'), LocalId()) else: - return DefinitionLocator(definition['_id']) + return DefinitionLocator(definition['category'], definition['_id']) def get_modulestore_type(self, course_id): """ diff --git a/common/lib/xmodule/xmodule/modulestore/tests/persistent_factories.py b/common/lib/xmodule/xmodule/modulestore/tests/persistent_factories.py index 9f6675f033..3962e9b98e 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/persistent_factories.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/persistent_factories.py @@ -32,14 +32,14 @@ class PersistentCourseFactory(SplitFactory): # pylint: disable=W0613 @classmethod - def _create(cls, target_class, course_id='testX.999', org='testX', user_id='test_user', + def _create(cls, target_class, offering='999', org='testX', user_id='test_user', master_branch='draft', **kwargs): modulestore = kwargs.pop('modulestore') root_block_id = kwargs.pop('root_block_id', 'course') # Write the data to the mongo datastore new_course = modulestore.create_course( - course_id, org, user_id, fields=kwargs, + org, offering, user_id, fields=kwargs, master_branch=master_branch, root_block_id=root_block_id ) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py b/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py index 4dba9b11f7..274deb014d 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py @@ -267,6 +267,7 @@ class TestLocationMapper(LocMapperSetupSansDjango): ) prob_locator = BlockUsageLocator( prob_course_key, + block_type='problem', block_id='problem2', ) prob_location = loc_mapper().translate_locator_to_location(prob_locator) @@ -289,20 +290,21 @@ class TestLocationMapper(LocMapperSetupSansDjango): prob_location = loc_mapper().translate_locator_to_location(prob_locator, get_course=True) self.assertEqual(prob_location, SlashSeparatedCourseKey(org, course, run)) # explicit branch - prob_locator = BlockUsageLocator( - prob_course_key.for_branch('draft'), block_id=prob_locator.block_id - ) + prob_locator = prob_locator.for_branch('draft') prob_location = loc_mapper().translate_locator_to_location(prob_locator) # Even though the problem was set as draft, we always return revision=None to work # with old mongo/draft modulestores. self.assertEqual(prob_location, Location(org, course, run, 'problem', 'abc123', None)) - prob_locator = BlockUsageLocator(prob_course_key.for_branch('production'), block_id='problem2') + prob_locator = BlockUsageLocator( + prob_course_key.for_branch('production'), + block_type='problem', block_id='problem2' + ) prob_location = loc_mapper().translate_locator_to_location(prob_locator) self.assertEqual(prob_location, Location(org, course, run, 'problem', 'abc123', None)) # same for chapter except chapter cannot be draft in old system chap_locator = BlockUsageLocator( prob_course_key.for_branch('production'), - block_id='chapter48f', + block_type='chapter', block_id='chapter48f', ) chap_location = loc_mapper().translate_locator_to_location(chap_locator) self.assertEqual(chap_location, Location(org, course, run, 'chapter', '48f23a10395384929234')) @@ -311,7 +313,7 @@ class TestLocationMapper(LocMapperSetupSansDjango): chap_location = loc_mapper().translate_locator_to_location(chap_locator) self.assertEqual(chap_location, Location(org, course, run, 'chapter', '48f23a10395384929234')) chap_locator = BlockUsageLocator( - prob_course_key.for_branch('production'), block_id='chapter48f' + prob_course_key.for_branch('production'), block_type='chapter', block_id='chapter48f' ) chap_location = loc_mapper().translate_locator_to_location(chap_locator) self.assertEqual(chap_location, Location(org, course, run, 'chapter', '48f23a10395384929234')) @@ -319,7 +321,7 @@ class TestLocationMapper(LocMapperSetupSansDjango): # look for non-existent problem prob_locator2 = BlockUsageLocator( prob_course_key.for_branch('draft'), - block_id='problem3' + block_type='problem', block_id='problem3' ) prob_location = loc_mapper().translate_locator_to_location(prob_locator2) self.assertIsNone(prob_location, 'Found non-existent problem') diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py b/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py index 40ff22ddd9..eaea216e7c 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py @@ -7,8 +7,8 @@ import random from bson.objectid import ObjectId from opaque_keys import InvalidKeyError from xmodule.modulestore.locator import Locator, CourseLocator, BlockUsageLocator, DefinitionLocator -from xmodule.modulestore.parsers import BRANCH_PREFIX, BLOCK_PREFIX, VERSION_PREFIX from ddt import ddt, data +from xmodule.modulestore.keys import UsageKey, CourseKey, DefinitionKey @ddt @@ -40,7 +40,7 @@ class LocatorTest(TestCase): testobj_1 = CourseLocator(version_guid=test_id_1) self.check_course_locn_fields(testobj_1, version_guid=test_id_1) self.assertEqual(str(testobj_1.version_guid), test_id_1_loc) - self.assertEqual(testobj_1._to_string(), VERSION_PREFIX + test_id_1_loc) + self.assertEqual(testobj_1._to_string(), u'+'.join((testobj_1.VERSION_PREFIX, test_id_1_loc))) # Test using a given string test_id_2_loc = '519665f6223ebd6980884f2b' @@ -48,24 +48,24 @@ class LocatorTest(TestCase): testobj_2 = CourseLocator(version_guid=test_id_2) self.check_course_locn_fields(testobj_2, version_guid=test_id_2) self.assertEqual(str(testobj_2.version_guid), test_id_2_loc) - self.assertEqual(testobj_2._to_string(), VERSION_PREFIX + test_id_2_loc) + self.assertEqual(testobj_2._to_string(), u'+'.join((testobj_2.VERSION_PREFIX, test_id_2_loc))) @data( ' mit.eecs', 'mit.eecs ', - VERSION_PREFIX + 'mit.eecs', - BLOCK_PREFIX + 'black/mit.eecs', + CourseLocator.VERSION_PREFIX + '+mit.eecs', + BlockUsageLocator.BLOCK_PREFIX + '+black+mit.eecs', 'mit.ee cs', 'mit.ee,cs', - 'mit.ee/cs', + 'mit.ee+cs', 'mit.ee&cs', 'mit.ee()cs', - BRANCH_PREFIX + 'this', - 'mit.eecs/' + BRANCH_PREFIX, - 'mit.eecs/' + BRANCH_PREFIX + 'this/' + BRANCH_PREFIX + 'that', - 'mit.eecs/' + BRANCH_PREFIX + 'this/' + BRANCH_PREFIX, - 'mit.eecs/' + BRANCH_PREFIX + 'this ', - 'mit.eecs/' + BRANCH_PREFIX + 'th%is ', + CourseLocator.BRANCH_PREFIX + '+this', + 'mit.eecs+' + CourseLocator.BRANCH_PREFIX, + 'mit.eecs+' + CourseLocator.BRANCH_PREFIX + '+this+' + CourseLocator.BRANCH_PREFIX + '+that', + 'mit.eecs+' + CourseLocator.BRANCH_PREFIX + '+this+' + CourseLocator.BRANCH_PREFIX, + 'mit.eecs+' + CourseLocator.BRANCH_PREFIX + '+this ', + 'mit.eecs+' + CourseLocator.BRANCH_PREFIX + '+th%is ', ) def test_course_constructor_bad_package_id(self, bad_id): """ @@ -78,18 +78,20 @@ class LocatorTest(TestCase): CourseLocator(org='test', offering=bad_id) with self.assertRaises(InvalidKeyError): - CourseLocator.from_string('course-locator:' + bad_id) + CourseKey.from_string('course-locator:test+{}'.format(bad_id)) @data('course-locator:', 'course-locator:/mit.eecs', 'http:mit.eecs', 'course-locator//mit.eecs') def test_course_constructor_bad_url(self, bad_url): with self.assertRaises(InvalidKeyError): - CourseLocator.from_string(bad_url) + CourseKey.from_string(bad_url) def test_course_constructor_url(self): # Test parsing a url when it starts with a version ID and there is also a block ID. # This hits the parsers parse_guid method. test_id_loc = '519665f6223ebd6980884f2b' - testobj = CourseLocator.from_string("course-locator:{}{}/{}hw3".format(VERSION_PREFIX, test_id_loc, BLOCK_PREFIX)) + testobj = CourseKey.from_string("course-locator:{}+{}+{}+hw3".format( + CourseLocator.VERSION_PREFIX, test_id_loc, CourseLocator.BLOCK_PREFIX + )) self.check_course_locn_fields( testobj, version_guid=ObjectId(test_id_loc) @@ -97,7 +99,9 @@ class LocatorTest(TestCase): def test_course_constructor_url_package_id_and_version_guid(self): test_id_loc = '519665f6223ebd6980884f2b' - testobj = CourseLocator.from_string('course-locator:mit.eecs+honors.6002x/' + VERSION_PREFIX + test_id_loc) + testobj = CourseKey.from_string( + 'course-locator:mit.eecs+honors.6002x+{}+{}'.format(CourseLocator.VERSION_PREFIX, test_id_loc) + ) self.check_course_locn_fields( testobj, org='mit.eecs', @@ -109,8 +113,8 @@ class LocatorTest(TestCase): test_id_loc = '519665f6223ebd6980884f2b' org = 'mit.eecs' offering = '~6002x' - testobj = CourseLocator.from_string('course-locator:{}+{}/{}draft-1/{}{}'.format( - org, offering, BRANCH_PREFIX, VERSION_PREFIX, test_id_loc + testobj = CourseKey.from_string('course-locator:{}+{}+{}+draft-1+{}+{}'.format( + org, offering, CourseLocator.BRANCH_PREFIX, CourseLocator.VERSION_PREFIX, test_id_loc )) self.check_course_locn_fields( testobj, @@ -126,15 +130,13 @@ class LocatorTest(TestCase): testurn = '{}+{}'.format(org, offering) testobj = CourseLocator(org=org, offering=offering) self.check_course_locn_fields(testobj, org=org, offering=offering) - self.assertEqual(testobj.package_id, testurn) self.assertEqual(testobj._to_string(), testurn) def test_course_constructor_package_id_separate_branch(self): org = 'mit.eecs' offering = '6002x' - testurn = '{}+{}'.format(org, offering) test_branch = 'published' - expected_urn = '{}+{}/{}{}'.format(org, offering, BRANCH_PREFIX, test_branch) + expected_urn = '{}+{}+{}+{}'.format(org, offering, CourseLocator.BRANCH_PREFIX, test_branch) testobj = CourseLocator(org=org, offering=offering, branch=test_branch) self.check_course_locn_fields( testobj, @@ -142,7 +144,6 @@ class LocatorTest(TestCase): offering=offering, branch=test_branch, ) - self.assertEqual(testobj.package_id, testurn) self.assertEqual(testobj.branch, test_branch) self.assertEqual(testobj._to_string(), expected_urn) @@ -151,17 +152,21 @@ class LocatorTest(TestCase): expected_offering = '6002x' expected_branch = 'published' expected_block_ref = 'HW3' - testurn = 'edx:{}+{}/{}{}/{}{}'.format( - expected_org, expected_offering, BRANCH_PREFIX, expected_branch, BLOCK_PREFIX, 'HW3' + testurn = 'edx:{}+{}+{}+{}+{}+{}+{}+{}'.format( + expected_org, expected_offering, CourseLocator.BRANCH_PREFIX, expected_branch, + BlockUsageLocator.BLOCK_TYPE_PREFIX, 'problem', BlockUsageLocator.BLOCK_PREFIX, 'HW3' + ) + testobj = UsageKey.from_string(testurn) + self.check_block_locn_fields( + testobj, + org=expected_org, + offering=expected_offering, + branch=expected_branch, + block_type='problem', + block=expected_block_ref ) - testobj = BlockUsageLocator.from_string(testurn) - self.check_block_locn_fields(testobj, - org=expected_org, - offering=expected_offering, - branch=expected_branch, - block=expected_block_ref) self.assertEqual(unicode(testobj), testurn) - testobj = BlockUsageLocator(testobj.course_key.for_version(ObjectId()), testobj.block_id) + testobj = testobj.for_version(ObjectId()) agnostic = testobj.version_agnostic() self.assertIsNone(agnostic.version_guid) self.check_block_locn_fields(agnostic, @@ -172,13 +177,16 @@ class LocatorTest(TestCase): def test_block_constructor_url_version_prefix(self): test_id_loc = '519665f6223ebd6980884f2b' - testobj = BlockUsageLocator.from_string( - 'edx:mit.eecs+6002x/{}{}/{}lab2'.format(VERSION_PREFIX, test_id_loc, BLOCK_PREFIX) + testobj = UsageKey.from_string( + 'edx:mit.eecs+6002x+{}+{}+{}+problem+{}+lab2'.format( + CourseLocator.VERSION_PREFIX, test_id_loc, BlockUsageLocator.BLOCK_TYPE_PREFIX, BlockUsageLocator.BLOCK_PREFIX + ) ) self.check_block_locn_fields( testobj, org='mit.eecs', offering='6002x', + block_type='problem', block='lab2', version_guid=ObjectId(test_id_loc) ) @@ -195,9 +203,10 @@ class LocatorTest(TestCase): def test_block_constructor_url_kitchen_sink(self): test_id_loc = '519665f6223ebd6980884f2b' - testobj = BlockUsageLocator.from_string( - 'edx:mit.eecs+6002x/{}draft/{}{}/{}lab2'.format( - BRANCH_PREFIX, VERSION_PREFIX, test_id_loc, BLOCK_PREFIX + testobj = UsageKey.from_string( + 'edx:mit.eecs+6002x+{}+draft+{}+{}+{}+problem+{}+lab2'.format( + CourseLocator.BRANCH_PREFIX, CourseLocator.VERSION_PREFIX, test_id_loc, + BlockUsageLocator.BLOCK_TYPE_PREFIX, BlockUsageLocator.BLOCK_PREFIX ) ) self.check_block_locn_fields( @@ -219,6 +228,7 @@ class LocatorTest(TestCase): block_id = 'problem:with-colon~2' testobj = BlockUsageLocator( CourseLocator(org=org, offering=offering, branch=branch), + block_type='problem', block_id=block_id ) self.check_block_locn_fields( @@ -234,54 +244,32 @@ class LocatorTest(TestCase): branch = 'foo' baseobj = CourseLocator(org=org, offering=offering, branch=branch) block_id = 'problem:with-colon~2' - testobj = BlockUsageLocator.make_relative(baseobj, block_id) + testobj = BlockUsageLocator.make_relative(baseobj, 'problem', block_id) self.check_block_locn_fields( testobj, org=org, offering=offering, branch=branch, block=block_id ) block_id = 'completely_different' - testobj = BlockUsageLocator.make_relative(testobj, block_id) + testobj = BlockUsageLocator.make_relative(testobj, 'problem', block_id) self.check_block_locn_fields( testobj, org=org, offering=offering, branch=branch, block=block_id ) def test_repr(self): - testurn = 'edx:mit.eecs+6002x/' + BRANCH_PREFIX + 'published/' + BLOCK_PREFIX + 'HW3' - testobj = BlockUsageLocator.from_string(testurn) - self.assertEqual("BlockUsageLocator(CourseLocator(u'mit.eecs', u'6002x', u'published', None), u'HW3')", repr(testobj)) - - def test_url_reverse(self): - """ - Test the url_reverse method - """ - locator = BlockUsageLocator( - CourseLocator(org="a", offering="fancy_course-id", branch="branch_1.2-3"), - block_id='element' - ) - self.assertEqual( - '/expression/{}/format'.format(unicode(locator)), - locator.url_reverse('expression', 'format') - ) - self.assertEqual( - '/expression/{}/format'.format(unicode(locator)), - locator.url_reverse('/expression', '/format') - ) - self.assertEqual( - '/expression/{}'.format(unicode(locator)), - locator.url_reverse('expression/', None) - ) - self.assertEqual( - '/expression/{}'.format(unicode(locator)), - locator.url_reverse('/expression/', '') + testurn = u'edx:mit.eecs+6002x+{}+published+{}+problem+{}+HW3'.format( + CourseLocator.BRANCH_PREFIX, BlockUsageLocator.BLOCK_TYPE_PREFIX, BlockUsageLocator.BLOCK_PREFIX ) + testobj = UsageKey.from_string(testurn) + self.assertEqual("BlockUsageLocator(CourseLocator(u'mit.eecs', u'6002x', u'published', None), u'problem', u'HW3')", repr(testobj)) def test_description_locator_url(self): object_id = '{:024x}'.format(random.randrange(16 ** 24)) - definition_locator = DefinitionLocator(object_id) - self.assertEqual('defx:' + VERSION_PREFIX + object_id, unicode(definition_locator)) + definition_locator = DefinitionLocator('html', object_id) + self.assertEqual('defx:{}+{}+html'.format(object_id, DefinitionLocator.BLOCK_TYPE_PREFIX), unicode(definition_locator)) + self.assertEqual(definition_locator, DefinitionKey.from_string(unicode(definition_locator))) def test_description_locator_version(self): object_id = '{:024x}'.format(random.randrange(16 ** 24)) - definition_locator = DefinitionLocator(object_id) + definition_locator = DefinitionLocator('html', object_id) self.assertEqual(object_id, str(definition_locator.version())) # ------------------------------------------------------------------ @@ -298,10 +286,12 @@ class LocatorTest(TestCase): self.assertEqual(testobj.branch, branch) def check_block_locn_fields(self, testobj, version_guid=None, - org=None, offering=None, branch=None, block=None): + org=None, offering=None, branch=None, block_type=None, block=None): """ Does adds a block id check over and above the check_course_locn_fields tests """ self.check_course_locn_fields(testobj, version_guid, org, offering, branch) + if block_type is not None: + self.assertEqual(testobj.block_type, block_type) self.assertEqual(testobj.block_id, block) 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 84b346d94e..a1a528112e 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py @@ -442,7 +442,7 @@ class SplitModuleTest(unittest.TestCase): Sets up the initial data into the db ''' split_store = modulestore() - for course_id, course_spec in SplitModuleTest.COURSE_CONTENT.iteritems(): + for _course_id, course_spec in SplitModuleTest.COURSE_CONTENT.iteritems(): course = split_store.create_course( course_spec['org'], course_spec['offering'], course_spec['user_id'], fields=course_spec['fields'], @@ -454,7 +454,8 @@ class SplitModuleTest(unittest.TestCase): if course.location.block_id == block_id: block = course else: - block_usage = BlockUsageLocator.make_relative(course.location, block_id) + # not easy to figure out the category but get_item won't care + block_usage = BlockUsageLocator.make_relative(course.location, '', block_id) block = split_store.get_item(block_usage) for key, value in fields.iteritems(): setattr(block, key, value) @@ -466,7 +467,7 @@ class SplitModuleTest(unittest.TestCase): elif spec['parent'] == course.location.block_id: parent = course else: - block_usage = BlockUsageLocator.make_relative(course.location, spec['parent']) + block_usage = BlockUsageLocator.make_relative(course.location, '', spec['parent']) parent = split_store.get_item(block_usage) block_id = LocalId(spec['id']) child = split_store.create_xblock( @@ -477,6 +478,7 @@ class SplitModuleTest(unittest.TestCase): # publish "testx.wonderful" to_publish = BlockUsageLocator( CourseLocator(org="testx", offering="wonderful", branch="draft"), + block_type='course', block_id="head23456" ) destination = CourseLocator(org="testx", offering="wonderful", branch="published") @@ -676,12 +678,12 @@ class SplitModuleItemTests(SplitModuleTest): course = modulestore().get_course(course_locator) previous_version = course.previous_version # positive tests of various forms - locator = BlockUsageLocator(CourseLocator(version_guid=previous_version), block_id='head12345') + locator = course.location.map_into_course(CourseLocator(version_guid=previous_version)) self.assertTrue( modulestore().has_item(locator), "couldn't find in %s" % previous_version ) - locator = BlockUsageLocator(course_locator, block_id='head12345') + locator = course.location.version_agnostic() self.assertTrue( modulestore().has_item(locator), ) @@ -689,6 +691,7 @@ class SplitModuleItemTests(SplitModuleTest): modulestore().has_item( BlockUsageLocator( locator.course_key.for_branch('published'), + block_type=locator.block_type, block_id=locator.block_id ) ), @@ -696,7 +699,7 @@ class SplitModuleItemTests(SplitModuleTest): ) # not a course obj - locator = BlockUsageLocator(course_locator, block_id='chapter1') + locator = BlockUsageLocator(course_locator, block_type='chapter', block_id='chapter1') self.assertTrue( modulestore().has_item(locator), "couldn't find chapter1" @@ -705,26 +708,25 @@ class SplitModuleItemTests(SplitModuleTest): # in published course locator = BlockUsageLocator( CourseLocator(org="testx", offering="wonderful", branch='draft'), + block_type="course", block_id="head23456" ) self.assertTrue( - modulestore().has_item( - BlockUsageLocator(locator.course_key.for_branch("published"), block_id=locator.block_id) - ) + modulestore().has_item(locator.for_branch("published")) ) - locator = locator.for_branch('published') - self.assertTrue(modulestore().has_item(locator), "couldn't find in published") def test_negative_has_item(self): # negative tests--not found # no such course or block locator = BlockUsageLocator( CourseLocator(org="foo", offering="doesnotexist", branch='draft'), + block_type="course", block_id="head23456" ) self.assertFalse(modulestore().has_item(locator)) locator = BlockUsageLocator( CourseLocator(org="testx", offering="wonderful", branch='draft'), + block_type="vertical", block_id="doesnotexist" ) self.assertFalse(modulestore().has_item(locator)) @@ -738,7 +740,7 @@ class SplitModuleItemTests(SplitModuleTest): previous_version = course.previous_version # positive tests of various forms - locator = BlockUsageLocator(CourseLocator(version_guid=previous_version), block_id='head12345') + locator = course.location.map_into_course(CourseLocator(version_guid=previous_version)) block = modulestore().get_item(locator) self.assertIsInstance(block, CourseDescriptor) self.assertIsInstance(modulestore().get_item(locator), CourseDescriptor) @@ -759,36 +761,27 @@ class SplitModuleItemTests(SplitModuleTest): block.grade_cutoffs, {"Pass": 0.45}, ) - locator = BlockUsageLocator(hero_locator, block_id='head12345') - verify_greek_hero(modulestore().get_item(locator)) + verify_greek_hero(modulestore().get_item(course.location)) # try to look up other branches with self.assertRaises(ItemNotFoundError): - modulestore().get_item( - BlockUsageLocator( - hero_locator.for_branch("published"), - block_id=locator.block_id, - ) - ) - self.assertIsInstance( - modulestore().get_item(locator), - CourseDescriptor - ) + modulestore().get_item(course.location.for_branch("published")) def test_get_non_root(self): # not a course obj locator = BlockUsageLocator( - CourseLocator(org='testx', offering='GreekHero', branch='draft'), 'chapter1' + CourseLocator(org='testx', offering='GreekHero', branch='draft'), 'chapter', 'chapter1' ) block = modulestore().get_item(locator) - self.assertEqual(block.location.package_id, "testx+GreekHero") + self.assertEqual(block.location.org, "testx") + self.assertEqual(block.location.offering, "GreekHero") self.assertEqual(block.category, 'chapter') self.assertEqual(block.display_name, "Hercules") self.assertEqual(block.edited_by, "testassist@edx.org") # in published course locator = BlockUsageLocator( - CourseLocator(org='testx', offering='wonderful', branch='published'), 'head23456' + CourseLocator(org='testx', offering='wonderful', branch='published'), 'course', 'head23456' ) self.assertIsInstance( modulestore().get_item(locator), @@ -798,12 +791,12 @@ class SplitModuleItemTests(SplitModuleTest): # negative tests--not found # no such course or block locator = BlockUsageLocator( - CourseLocator(org='doesnotexist', offering='doesnotexist', branch='draft'), 'head23456' + CourseLocator(org='doesnotexist', offering='doesnotexist', branch='draft'), 'course', 'head23456' ) with self.assertRaises(ItemNotFoundError): modulestore().get_item(locator) locator = BlockUsageLocator( - CourseLocator(org='testx', offering='wonderful', branch='draft'), 'doesnotexist' + CourseLocator(org='testx', offering='wonderful', branch='draft'), 'html', 'doesnotexist' ) with self.assertRaises(ItemNotFoundError): modulestore().get_item(locator) @@ -864,7 +857,7 @@ class SplitModuleItemTests(SplitModuleTest): ''' locator = BlockUsageLocator( CourseLocator(org='testx', offering='GreekHero', branch='draft'), - block_id='chapter1' + 'chapter', block_id='chapter1' ) parents = modulestore().get_parent_locations(locator) self.assertEqual(len(parents), 1) @@ -884,7 +877,7 @@ class SplitModuleItemTests(SplitModuleTest): Test the existing get_children method on xdescriptors """ locator = BlockUsageLocator( - CourseLocator(org='testx', offering='GreekHero', branch='draft'), 'head12345' + CourseLocator(org='testx', offering='GreekHero', branch='draft'), 'course', 'head12345' ) block = modulestore().get_item(locator) children = block.get_children() @@ -952,11 +945,11 @@ class TestItemCrud(SplitModuleTest): self.assertIsNotNone(new_module.definition_locator) self.assertEqual(new_module.display_name, 'new sequential') # check that block does not exist in previous version - locator = BlockUsageLocator( - CourseLocator(version_guid=premod_course.location.version_guid), - block_id=new_module.location.block_id + locator = new_module.location.map_into_course( + CourseLocator(version_guid=premod_course.location.version_guid) ) - self.assertRaises(ItemNotFoundError, modulestore().get_item, locator) + with self.assertRaises(ItemNotFoundError): + modulestore().get_item(locator) def test_create_parented_item(self): """ @@ -964,12 +957,12 @@ class TestItemCrud(SplitModuleTest): """ locator = BlockUsageLocator( CourseLocator(org='testx', offering='GreekHero', branch='draft'), - block_id='chapter2' + 'chapter', block_id='chapter2' ) original = modulestore().get_item(locator) locator = BlockUsageLocator( - CourseLocator(org='testx', offering='wonderful', branch='draft'), 'head23456' + CourseLocator(org='testx', offering='wonderful', branch='draft'), 'course', 'head23456' ) premod_course = modulestore().get_course(locator.course_key) category = 'chapter' @@ -992,12 +985,12 @@ class TestItemCrud(SplitModuleTest): """ locator = BlockUsageLocator( CourseLocator(org='testx', offering='GreekHero', branch='draft'), - block_id='problem1' + 'problem', block_id='problem1' ) original = modulestore().get_item(locator) locator = BlockUsageLocator( - CourseLocator(org='guestx', offering='contender', branch='draft'), 'head345679' + CourseLocator(org='guestx', offering='contender', branch='draft'), 'course', 'head345679' ) category = 'problem' new_payload = "empty" @@ -1031,8 +1024,8 @@ class TestItemCrud(SplitModuleTest): Check that using odd characters in block id don't break ability to add and retrieve block. """ course_key = CourseLocator(org='guestx', offering='contender', branch='draft') - parent_locator = BlockUsageLocator(course_key, block_id="head345679") - chapter_locator = BlockUsageLocator(course_key, block_id="foo.bar_-~:0") + parent_locator = BlockUsageLocator(course_key, 'course', block_id="head345679") + chapter_locator = BlockUsageLocator(course_key, 'chapter', block_id="foo.bar_-~:0") modulestore().create_item( parent_locator, 'chapter', 'anotheruser', block_id=chapter_locator.block_id, @@ -1043,7 +1036,7 @@ class TestItemCrud(SplitModuleTest): self.assertEqual(new_module.location.block_id, "foo.bar_-~:0") # hardcode to ensure BUL init didn't change # now try making that a parent of something new_payload = "empty" - problem_locator = BlockUsageLocator(course_key, block_id="prob.bar_-~:99a") + problem_locator = BlockUsageLocator(course_key, 'problem', block_id="prob.bar_-~:99a") modulestore().create_item( chapter_locator, 'problem', 'anotheruser', block_id=problem_locator.block_id, @@ -1119,10 +1112,7 @@ class TestItemCrud(SplitModuleTest): ) # add new child to old parent in continued (leave off version_guid) - course_module_locator = BlockUsageLocator( - new_course.location.course_key.version_agnostic(), - block_id=new_course.location.block_id, - ) + course_module_locator = new_course.location.version_agnostic() new_ele = modulestore().create_item( course_module_locator, 'chapter', user, fields={'display_name': 'chapter 4'}, @@ -1143,7 +1133,7 @@ class TestItemCrud(SplitModuleTest): """ locator = BlockUsageLocator( CourseLocator(org="testx", offering="GreekHero", branch='draft'), - block_id="problem3_2" + 'problem', block_id="problem3_2" ) problem = modulestore().get_item(locator) pre_def_id = problem.definition_locator.definition_id @@ -1160,10 +1150,7 @@ class TestItemCrud(SplitModuleTest): self.assertNotEqual(updated_problem.location.version_guid, pre_version_guid) self.assertEqual(updated_problem.max_attempts, 4) # refetch to ensure original didn't change - original_location = BlockUsageLocator( - CourseLocator(version_guid=pre_version_guid), - block_id=problem.location.block_id - ) + original_location = problem.location.map_into_course(CourseLocator(version_guid=pre_version_guid)) problem = modulestore().get_item(original_location) self.assertNotEqual(problem.max_attempts, 4, "original changed") @@ -1179,7 +1166,7 @@ class TestItemCrud(SplitModuleTest): test updating an item's children ensuring the definition doesn't version but the course does if it should """ locator = BlockUsageLocator( - CourseLocator(org='testx', offering='GreekHero', branch='draft'), 'chapter3' + CourseLocator(org='testx', offering='GreekHero', branch='draft'), 'chapter', 'chapter3' ) block = modulestore().get_item(locator) pre_def_id = block.definition_locator.definition_id @@ -1206,7 +1193,7 @@ class TestItemCrud(SplitModuleTest): test updating an item's definition: ensure it gets versioned as well as the course getting versioned """ locator = BlockUsageLocator( - CourseLocator(org='testx', offering='GreekHero', branch='draft'), 'head12345' + CourseLocator(org='testx', offering='GreekHero', branch='draft'), 'course', 'head12345' ) block = modulestore().get_item(locator) pre_def_id = block.definition_locator.definition_id @@ -1226,13 +1213,13 @@ class TestItemCrud(SplitModuleTest): """ locator = BlockUsageLocator( CourseLocator('testx', 'GreekHero', branch='draft'), - block_id='problem1' + 'problem', block_id='problem1' ) original = modulestore().get_item(locator) # first add 2 children to the course for the update to manipulate locator = BlockUsageLocator( CourseLocator('guestx', 'contender', branch='draft'), - block_id="head345679" + 'course', block_id="head345679" ) category = 'problem' new_payload = "empty" @@ -1282,11 +1269,7 @@ class TestItemCrud(SplitModuleTest): with self.assertRaises(VersionConflictError): modulestore().has_item(locn_to_del) - locator = BlockUsageLocator( - CourseLocator(version_guid=locn_to_del.version_guid), - block_id=locn_to_del.block_id - ) - self.assertTrue(modulestore().has_item(locator)) + self.assertTrue(modulestore().has_item(locn_to_del.course_agnostic())) self.assertNotEqual(new_course_loc.version_guid, course.location.version_guid) # delete a subtree @@ -1301,22 +1284,9 @@ class TestItemCrud(SplitModuleTest): if node: node_loc = node.location self.assertFalse( - modulestore().has_item( - BlockUsageLocator( - CourseLocator( - org=node_loc.org, - offering=node_loc.offering, - branch=node_loc.branch, - ), - block_id=node_loc.block_id - ) - ) + modulestore().has_item(node_loc.version_agnostic()) ) - locator = BlockUsageLocator( - CourseLocator(version_guid=node.location.version_guid), - block_id=node.location.block_id - ) - self.assertTrue(modulestore().has_item(locator)) + self.assertTrue(modulestore().has_item(node_loc.course_agnostic())) if node.has_children: for sub in node.get_children(): check_subtree(sub) @@ -1327,10 +1297,7 @@ class TestItemCrud(SplitModuleTest): Create a course we can delete """ course = modulestore().create_course('nihilx', 'deletion', 'deleting_user') - root = BlockUsageLocator( - course.id.version_agnostic().for_branch('draft'), - block_id=course.location.block_id, - ) + root = course.location.version_agnostic().for_branch('draft') for _ in range(4): self.create_subtree_for_deletion(root, ['chapter', 'vertical', 'problem']) return modulestore().get_item(root) @@ -1342,7 +1309,7 @@ class TestItemCrud(SplitModuleTest): if not category_queue: return node = modulestore().create_item(parent.version_agnostic(), category_queue[0], 'deleting_user') - node_loc = BlockUsageLocator(parent.course_key, block_id=node.location.block_id) + node_loc = node.location.map_into_course(parent.course_key) for _ in range(4): self.create_subtree_for_deletion(node_loc, category_queue[1:]) @@ -1523,13 +1490,13 @@ class TestInheritance(SplitModuleTest): # Note, not testing value where defined (course) b/c there's no # defined accessor for it on CourseDescriptor. locator = BlockUsageLocator( - CourseLocator(org='testx', offering='GreekHero', branch='draft'), 'problem3_2' + CourseLocator(org='testx', offering='GreekHero', branch='draft'), 'problem', 'problem3_2' ) node = modulestore().get_item(locator) # inherited self.assertEqual(node.graceperiod, datetime.timedelta(hours=2)) locator = BlockUsageLocator( - CourseLocator(org='testx', offering='GreekHero', branch='draft'), 'problem1' + CourseLocator(org='testx', offering='GreekHero', branch='draft'), 'problem', 'problem1' ) node = modulestore().get_item(locator) # overridden @@ -1560,19 +1527,19 @@ class TestPublish(SplitModuleTest): ) # add a child under chapter1 new_module = modulestore().create_item( - BlockUsageLocator.make_relative(source_course, "chapter1"), "sequential", self.user, + BlockUsageLocator.make_relative(source_course, "chapter", "chapter1"), "sequential", self.user, fields={'display_name': 'new sequential'}, ) # remove chapter1 from expected b/c its pub'd version != the source anymore since source changed expected.remove("chapter1") # check that it's not in published course with self.assertRaises(ItemNotFoundError): - modulestore().get_item(BlockUsageLocator.make_relative(dest_course, new_module.location.block_id)) + modulestore().get_item(new_module.location.map_into_course(dest_course)) # publish it modulestore().xblock_publish(self.user, source_course, dest_course, [new_module.location.block_id], None) expected.append(new_module.location.block_id) # check that it is in the published course and that its parent is the chapter - pub_module = modulestore().get_item(BlockUsageLocator.make_relative(dest_course, new_module.location.block_id)) + pub_module = modulestore().get_item(new_module.location.map_into_course(dest_course)) self.assertEqual( modulestore().get_parent_locations(pub_module.location)[0].block_id, "chapter1" ) @@ -1584,7 +1551,7 @@ class TestPublish(SplitModuleTest): modulestore().xblock_publish(self.user, source_course, dest_course, [new_module.location.block_id], None) expected.append(new_module.location.block_id) # check that it is in the published course (no error means it worked) - pub_module = modulestore().get_item(BlockUsageLocator.make_relative(dest_course, new_module.location.block_id)) + pub_module = modulestore().get_item(new_module.location.map_into_course(dest_course)) self._check_course( source_course, dest_course, expected, ["chapter2", "chapter3", "problem1", "problem3_2"] ) @@ -1617,11 +1584,11 @@ class TestPublish(SplitModuleTest): expected = ["head12345", "chapter1", "chapter3", "problem1", "problem3_2"] self._check_course(source_course, dest_course, expected, ["chapter2"]) # now move problem1 and delete problem3_2 - chapter1 = modulestore().get_item(BlockUsageLocator.make_relative(source_course, "chapter1")) - chapter3 = modulestore().get_item(BlockUsageLocator.make_relative(source_course, "chapter3")) + chapter1 = modulestore().get_item(source_course.make_usage_key("chapter", "chapter1")) + chapter3 = modulestore().get_item(source_course.make_usage_key("chapter", "chapter3")) chapter1.children.append("problem1") chapter3.children.remove("problem1") - modulestore().delete_item(BlockUsageLocator.make_relative(source_course, "problem3_2"), self.user) + modulestore().delete_item(source_course.make_usage_key("problem", "problem3_2"), self.user) modulestore().xblock_publish(self.user, source_course, dest_course, ["head12345"], ["chapter2"]) expected = ["head12345", "chapter1", "chapter3", "problem1"] self._check_course(source_course, dest_course, expected, ["chapter2", "problem3_2"]) @@ -1633,8 +1600,9 @@ class TestPublish(SplitModuleTest): history_info = modulestore().get_course_history_info(dest_course_loc) self.assertEqual(history_info['edited_by'], self.user) for expected in expected_blocks: - source = modulestore().get_item(BlockUsageLocator.make_relative(source_course_loc, expected)) - pub_copy = modulestore().get_item(BlockUsageLocator.make_relative(dest_course_loc, expected)) + # since block_type has no impact on identity, we can just provide an empty string + source = modulestore().get_item(source_course_loc.make_usage_key("", expected)) + pub_copy = modulestore().get_item(dest_course_loc.make_usage_key("", expected)) # everything except previous_version & children should be the same self.assertEqual(source.category, pub_copy.category) self.assertEqual(source.update_version, pub_copy.update_version) @@ -1649,7 +1617,7 @@ class TestPublish(SplitModuleTest): self.assertEqual(field.read_from(source), field.read_from(pub_copy)) for unexp in unexpected_blocks: with self.assertRaises(ItemNotFoundError): - modulestore().get_item(BlockUsageLocator.make_relative(dest_course_loc, unexp)) + modulestore().get_item(dest_course_loc.make_usage_key("", unexp)) def _compare_children(self, source_children, dest_children, unexpected): """ diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_w_old_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_w_old_mongo.py index d4377b5bd8..796a38223b 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_w_old_mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_w_old_mongo.py @@ -105,6 +105,7 @@ class SplitWMongoCourseBoostrapper(unittest.TestCase): # create pointer for split course_or_parent_locator = BlockUsageLocator( course_key=self.split_course_key, + block_type=parent_category, block_id=parent_name ) else: diff --git a/common/lib/xmodule/xmodule/open_ended_grading_classes/controller_query_service.py b/common/lib/xmodule/xmodule/open_ended_grading_classes/controller_query_service.py index 0577a9034b..b3cb9d7496 100644 --- a/common/lib/xmodule/xmodule/open_ended_grading_classes/controller_query_service.py +++ b/common/lib/xmodule/xmodule/open_ended_grading_classes/controller_query_service.py @@ -37,7 +37,7 @@ class ControllerQueryService(GradingService): def check_combined_notifications(self, course_id, student_id, user_is_staff, last_time_viewed): params = { 'student_id': student_id, - 'course_id': course_id, + 'course_id': course_id.to_deprecated_string(), 'user_is_staff': user_is_staff, 'last_time_viewed': last_time_viewed, } diff --git a/common/lib/xmodule/xmodule/open_ended_grading_classes/peer_grading_service.py b/common/lib/xmodule/xmodule/open_ended_grading_classes/peer_grading_service.py index dd9a74293a..50d654a0bd 100644 --- a/common/lib/xmodule/xmodule/open_ended_grading_classes/peer_grading_service.py +++ b/common/lib/xmodule/xmodule/open_ended_grading_classes/peer_grading_service.py @@ -85,7 +85,7 @@ class PeerGradingService(GradingService): return result def get_problem_list(self, course_id, grader_id): - params = {'course_id': course_id, 'student_id': grader_id} + params = {'course_id': course_id.to_deprecated_string(), 'student_id': grader_id} result = self.get(self.get_problem_list_url, params) if 'problem_list' in result: @@ -100,7 +100,7 @@ class PeerGradingService(GradingService): return result def get_notifications(self, course_id, grader_id): - params = {'course_id': course_id, 'student_id': grader_id} + params = {'course_id': course_id.to_deprecated_string(), 'student_id': grader_id} result = self.get(self.get_notifications_url, params) self._record_result( 'get_notifications', diff --git a/lms/djangoapps/django_comment_client/base/views.py b/lms/djangoapps/django_comment_client/base/views.py index 98afcb9e8f..3b9d8f7dae 100644 --- a/lms/djangoapps/django_comment_client/base/views.py +++ b/lms/djangoapps/django_comment_client/base/views.py @@ -182,7 +182,7 @@ def _create_comment(request, course_key, thread_id=None, parent_id=None): 'anonymous': anonymous, 'anonymous_to_peers': anonymous_to_peers, 'user_id': request.user.id, - 'course_id': course_key, + 'course_id': course_key.to_deprecated_string(), 'thread_id': thread_id, 'parent_id': parent_id, }) diff --git a/lms/djangoapps/django_comment_client/forum/tests.py b/lms/djangoapps/django_comment_client/forum/tests.py index f66246fad4..65199b5836 100644 --- a/lms/djangoapps/django_comment_client/forum/tests.py +++ b/lms/djangoapps/django_comment_client/forum/tests.py @@ -285,7 +285,7 @@ class UserProfileTestCase(ModuleStoreTestCase): StringEndsWithMatcher('/users/{}/active_threads'.format(self.profiled_user.id)), data=None, params=PartialDictMatcher({ - "course_id": self.course.id, + "course_id": self.course.id.to_deprecated_string(), "page": params.get("page", 1), "per_page": views.THREADS_PER_PAGE }), diff --git a/lms/djangoapps/open_ended_grading/staff_grading_service.py b/lms/djangoapps/open_ended_grading/staff_grading_service.py index dce62af596..4fb8a11661 100644 --- a/lms/djangoapps/open_ended_grading/staff_grading_service.py +++ b/lms/djangoapps/open_ended_grading/staff_grading_service.py @@ -9,7 +9,6 @@ from django.conf import settings from django.http import HttpResponse, Http404 from django.utils.translation import ugettext as _ -from xmodule.course_module import CourseDescriptor from xmodule.modulestore.locations import SlashSeparatedCourseKey from xmodule.open_ended_grading_classes.grading_service_module import GradingService, GradingServiceError from xmodule.modulestore.django import ModuleI18nService @@ -116,7 +115,7 @@ class StaffGradingService(GradingService): Raises: GradingServiceError: something went wrong with the connection. """ - params = {'course_id': course_id, 'grader_id': grader_id} + params = {'course_id': course_id.to_deprecated_string(), 'grader_id': grader_id} result = self.get(self.get_problem_list_url, params) tags = [u'course_id:{}'.format(course_id)] self._record_result('get_problem_list', result, tags) @@ -148,7 +147,7 @@ class StaffGradingService(GradingService): self.get( self.get_next_url, params={ - 'location': location, + 'location': location.to_deprecated_string(), 'grader_id': grader_id } ) @@ -170,7 +169,7 @@ class StaffGradingService(GradingService): Raises: GradingServiceError if there's a problem connecting. """ - data = {'course_id': course_id, + data = {'course_id': course_id.to_deprecated_string(), 'submission_id': submission_id, 'score': score, 'feedback': feedback, @@ -186,7 +185,7 @@ class StaffGradingService(GradingService): return result def get_notifications(self, course_id): - params = {'course_id': course_id} + params = {'course_id': course_id.to_deprecated_string()} result = self.get(self.get_notifications_url, params) tags = [ u'course_id:{}'.format(course_id), @@ -274,7 +273,7 @@ def get_next(request, course_id): ', '.join(missing))) grader_id = unique_id_for_user(request.user) p = request.POST - location = p['location'] + location = course_key.make_usage_key_from_deprecated_string(p['location']) return HttpResponse(json.dumps(_get_next(course_key, grader_id, location)), mimetype="application/json") @@ -400,7 +399,7 @@ def save_grade(request, course_id): grader_id = unique_id_for_user(request.user) - location = p['location'] + location = course_key.make_usage_key_from_deprecated_string(p['location']) try: result = staff_grading_service().save_grade(course_key, diff --git a/lms/djangoapps/open_ended_grading/tests.py b/lms/djangoapps/open_ended_grading/tests.py index 79cc972b1b..43ba2aeab6 100644 --- a/lms/djangoapps/open_ended_grading/tests.py +++ b/lms/djangoapps/open_ended_grading/tests.py @@ -109,13 +109,13 @@ class TestStaffGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase): self.student = 'view@test.com' self.instructor = 'view2@test.com' self.password = 'foo' - self.location = 'TestLocation' self.create_account('u1', self.student, self.password) self.create_account('u2', self.instructor, self.password) self.activate_user(self.student) self.activate_user(self.instructor) self.course_id = SlashSeparatedCourseKey("edX", "toy", "2012_Fall") + self.location_string = self.course_id.make_usage_key('html', 'TestLocation').to_deprecated_string() self.toy = modulestore().get_course(self.course_id) make_instructor(self.toy, self.instructor) @@ -140,7 +140,7 @@ class TestStaffGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase): self.login(self.instructor, self.password) url = reverse('staff_grading_get_next', kwargs={'course_id': self.course_id.to_deprecated_string()}) - data = {'location': self.location} + data = {'location': self.location_string} response = check_for_post_code(self, 200, url, data) @@ -165,7 +165,7 @@ class TestStaffGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase): data = {'score': '12', 'feedback': 'great!', 'submission_id': '123', - 'location': self.location, + 'location': self.location_string, 'submission_flagged': "true", 'rubric_scores[]': ['1', '2']} if skip: @@ -227,7 +227,7 @@ class TestStaffGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase): 'score': '12', 'feedback': '', 'submission_id': '123', - 'location': self.location, + 'location': self.location_string, 'submission_flagged': "false", 'rubric_scores[]': ['1', '2'] } @@ -262,13 +262,13 @@ class TestPeerGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase): self.student = 'view@test.com' self.instructor = 'view2@test.com' self.password = 'foo' - self.location = 'TestLocation' self.create_account('u1', self.student, self.password) self.create_account('u2', self.instructor, self.password) self.activate_user(self.student) self.activate_user(self.instructor) self.course_id = SlashSeparatedCourseKey("edX", "toy", "2012_Fall") + self.location_string = self.course_id.make_usage_key('html', 'TestLocation').to_deprecated_string() self.toy = modulestore().get_course(self.course_id) location = "i4x://edX/toy/peergrading/init" field_data = DictFieldData({'data': "", 'location': location, 'category':'peergrading'}) @@ -292,7 +292,7 @@ class TestPeerGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase): self.logout() def test_get_next_submission_success(self): - data = {'location': self.location} + data = {'location': self.location_string} response = self.peer_module.get_next_submission(data) content = response @@ -312,7 +312,7 @@ class TestPeerGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase): def test_save_grade_success(self): data = { 'rubric_scores[]': [0, 0], - 'location': self.location, + 'location': self.location_string, 'submission_id': 1, 'submission_key': 'fake key', 'score': 2, @@ -342,7 +342,7 @@ class TestPeerGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase): self.assertTrue(d['error'].find('Missing required keys:') > -1) def test_is_calibrated_success(self): - data = {'location': self.location} + data = {'location': self.location_string} response = self.peer_module.is_student_calibrated(data) self.assertTrue(response['success']) @@ -355,7 +355,7 @@ class TestPeerGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase): self.assertFalse('calibrated' in response) def test_show_calibration_essay_success(self): - data = {'location': self.location} + data = {'location': self.location_string} response = self.peer_module.show_calibration_essay(data) @@ -376,7 +376,7 @@ class TestPeerGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase): def test_save_calibration_essay_success(self): data = { 'rubric_scores[]': [0, 0], - 'location': self.location, + 'location': self.location_string, 'submission_id': 1, 'submission_key': 'fake key', 'score': 2, @@ -410,7 +410,7 @@ class TestPeerGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase): """ data = { 'rubric_scores[]': [0, 0], - 'location': self.location, + 'location': self.location_string, 'submission_id': 1, 'submission_key': 'fake key', 'score': 2, diff --git a/lms/djangoapps/open_ended_grading/views.py b/lms/djangoapps/open_ended_grading/views.py index 5bc6932fae..508c07eac5 100644 --- a/lms/djangoapps/open_ended_grading/views.py +++ b/lms/djangoapps/open_ended_grading/views.py @@ -1,11 +1,9 @@ import logging -from django.conf import settings from django.views.decorators.cache import cache_control from edxmako.shortcuts import render_to_response from django.core.urlresolvers import reverse -from student.models import unique_id_for_user from courseware.courses import get_course_with_access from xmodule.open_ended_grading_classes.grading_service_module import GradingServiceError @@ -20,11 +18,11 @@ from xmodule.modulestore import SlashSeparatedCourseKey from xmodule.modulestore.exceptions import NoPathToItem from django.http import HttpResponse, Http404, HttpResponseRedirect -from edxmako.shortcuts import render_to_string from django.utils.translation import ugettext as _ -from open_ended_grading.utils import (STAFF_ERROR_MESSAGE, STUDENT_ERROR_MESSAGE, - StudentProblemList, generate_problem_url, create_controller_query_service) +from open_ended_grading.utils import ( + STAFF_ERROR_MESSAGE, StudentProblemList, generate_problem_url, create_controller_query_service +) log = logging.getLogger(__name__) @@ -68,9 +66,10 @@ def staff_grading(request, course_id): """ Show the instructor grading interface. """ - course = get_course_with_access(request.user, 'staff', course_id) + course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) + course = get_course_with_access(request.user, 'staff', course_key) - ajax_url = _reverse_with_slash('staff_grading', course_id) + ajax_url = _reverse_with_slash('staff_grading', course_key) return render_to_response('instructor/staff_grading.html', { 'course': course, @@ -118,9 +117,9 @@ def peer_grading(request, course_id): When a student clicks on the "peer grading" button in the open ended interface, link them to a peer grading xmodule in the course. ''' - + course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) #Get the current course - course = get_course_with_access(request.user, 'load', course_id) + course = get_course_with_access(request.user, 'load', course_key) found_module, problem_url = find_peer_grading_module(course) if not found_module: @@ -187,13 +186,11 @@ def flagged_problem_list(request, course_id): ''' course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) course = get_course_with_access(request.user, 'staff', course_key) - student_id = unique_id_for_user(request.user) # call problem list service success = False error_text = "" problem_list = [] - base_course_url = reverse('courses') # Make a service that can query edX ORA. controller_qs = create_controller_query_service() diff --git a/lms/lib/comment_client/user.py b/lms/lib/comment_client/user.py index 6d7f44f916..e2ddd69a7b 100644 --- a/lms/lib/comment_client/user.py +++ b/lms/lib/comment_client/user.py @@ -86,7 +86,7 @@ class User(models.Model): if not self.course_id: raise CommentClientRequestError("Must provide course_id when retrieving active threads for the user") url = _url_for_user_active_threads(self.id) - params = {'course_id': self.course_id} + params = {'course_id': self.course_id.to_deprecated_string()} params = merge_dict(params, query_params) response = perform_request( 'get', @@ -102,7 +102,7 @@ class User(models.Model): if not self.course_id: raise CommentClientRequestError("Must provide course_id when retrieving subscribed threads for the user") url = _url_for_user_subscribed_threads(self.id) - params = {'course_id': self.course_id} + params = {'course_id': self.course_id.to_deprecated_string()} params = merge_dict(params, query_params) response = perform_request( 'get', @@ -118,7 +118,7 @@ class User(models.Model): url = self.url(action='get', params=self.attributes) retrieve_params = self.default_retrieve_params if self.attributes.get('course_id'): - retrieve_params['course_id'] = self.course_id + retrieve_params['course_id'] = self.course_id.to_deprecated_string() try: response = perform_request( 'get',