From f72a1db84814de4dd99ca78371de1c571ff34de4 Mon Sep 17 00:00:00 2001 From: zubair-arbi Date: Fri, 3 Mar 2017 15:45:09 +0500 Subject: [PATCH 01/10] add aggregate course key --- opaque_keys/edx/keys.py | 16 +++++++++ opaque_keys/edx/locator.py | 68 +++++++++++++++++++++++++++++++++++++- setup.py | 5 ++- 3 files changed, 87 insertions(+), 2 deletions(-) diff --git a/opaque_keys/edx/keys.py b/opaque_keys/edx/keys.py index 72e29684..5df08200 100644 --- a/opaque_keys/edx/keys.py +++ b/opaque_keys/edx/keys.py @@ -77,6 +77,22 @@ def block_type(self): # pragma: no cover raise NotImplementedError() +class AggregateCourseKey(OpaqueKey): + """ + An :class:`opaque_keys.OpaqueKey` identifying a + serialized Course Key object. + """ + KEY_TYPE = 'aggregate_course_key' + __slots__ = () + + @abstractproperty + def serialize(self): # pragma: no cover + """ + The serialized course key without run information. + """ + raise NotImplementedError() + + class CourseObjectMixin(object): """ An abstract :class:`opaque_keys.OpaqueKey` mixin diff --git a/opaque_keys/edx/locator.py b/opaque_keys/edx/locator.py index 0dcdaac1..2282ebef 100644 --- a/opaque_keys/edx/locator.py +++ b/opaque_keys/edx/locator.py @@ -16,7 +16,7 @@ from six import string_types, text_type from opaque_keys import OpaqueKey, InvalidKeyError -from opaque_keys.edx.keys import CourseKey, UsageKey, DefinitionKey, AssetKey +from opaque_keys.edx.keys import CourseKey, UsageKey, DefinitionKey, AssetKey, AggregateCourseKey log = logging.getLogger(__name__) @@ -1330,3 +1330,69 @@ def to_deprecated_list_repr(self): # Register AssetLocator as the deprecated fallback for AssetKey AssetKey.set_deprecated_fallback(AssetLocator) + + +class AggregateCourseLocator(BlockUsageLocator, AggregateCourseKey): # pylint: disable=abstract-method + """ + An AggregateCourseKey implementation class. + """ + CANONICAL_NAMESPACE = 'aggregate-course' + KEY_FIELDS = ('org', 'course') + __slots__ = KEY_FIELDS + + URL_RE_SOURCE = r""" + ((?P{ALLOWED_ID_CHARS}+)\+(?P{ALLOWED_ID_CHARS}+))? + """.format( + ALLOWED_ID_CHARS=Locator.ALLOWED_ID_CHARS + ) + + URL_RE = re.compile('^' + URL_RE_SOURCE) + + def __init__(self, org=None, course=None, **kwargs): + """ + Construct a AggregateCourseLocator. + + Arguments: + org (string): Organization identifier for the course + course (string): Course name + + """ + super(AggregateCourseLocator, self).__init__( + org=org, + course=course, + **kwargs + ) + + if self.org is None or self.course is None: + raise InvalidKeyError(self.__class__, 'Both org and course should be set.') + + @classmethod + def _from_string(cls, serialized): + """ + Return a AggregateCourseLocator parsing the given serialized string. + + Arguments: + serialized: string for matching + + """ + parse = cls.URL_RE.match(serialized) + if not parse: + raise InvalidKeyError(cls, serialized) + + parse = parse.groupdict() + return cls(**{key: parse.get(key) for key in cls.KEY_FIELDS}) + + def _to_string(self): + """ + Return a string representing this location. + """ + return u"{org}+{course}".format( + org=self.org, + course=self.course + ) + + def serialize(self): + """ + The serialized course key without run information. + """ + return '{org}+{course}'.format(org=self.org, course=self.course) diff --git a/setup.py b/setup.py index 20d124bd..69b3e113 100644 --- a/setup.py +++ b/setup.py @@ -2,7 +2,7 @@ setup( name='edx-opaque-keys', - version='0.4', + version='0.5', author='edX', url='https://github.com/edx/opaque-keys', classifiers=[ @@ -31,6 +31,9 @@ # don't use slashes in any new code 'slashes = opaque_keys.edx.locator:CourseLocator', ], + 'aggregate_course_key': [ + 'aggregate-course = opaque_keys.edx.locator:AggregateCourseLocator', + ], 'usage_key': [ 'block-v1 = opaque_keys.edx.locator:BlockUsageLocator', 'lib-block-v1 = opaque_keys.edx.locator:LibraryUsageLocator', From 11498a50f03fdd0f71f517d96a9fe0f26327d786 Mon Sep 17 00:00:00 2001 From: zubair-arbi Date: Fri, 3 Mar 2017 16:23:07 +0500 Subject: [PATCH 02/10] remove BlockUsageLocator from inheritance + update regex for the aggregagte course key --- opaque_keys/edx/locator.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/opaque_keys/edx/locator.py b/opaque_keys/edx/locator.py index 2282ebef..f0da1957 100644 --- a/opaque_keys/edx/locator.py +++ b/opaque_keys/edx/locator.py @@ -1332,7 +1332,7 @@ def to_deprecated_list_repr(self): AssetKey.set_deprecated_fallback(AssetLocator) -class AggregateCourseLocator(BlockUsageLocator, AggregateCourseKey): # pylint: disable=abstract-method +class AggregateCourseLocator(AggregateCourseKey): # pylint: disable=abstract-method """ An AggregateCourseKey implementation class. """ @@ -1340,13 +1340,11 @@ class AggregateCourseLocator(BlockUsageLocator, AggregateCourseKey): # pylint KEY_FIELDS = ('org', 'course') __slots__ = KEY_FIELDS - URL_RE_SOURCE = r""" - ((?P{ALLOWED_ID_CHARS}+)\+(?P{ALLOWED_ID_CHARS}+))? - """.format( + URL_RE_SOURCE = r'^(?P{ALLOWED_ID_CHARS}+)\+(?P{ALLOWED_ID_CHARS}+)$'.format( ALLOWED_ID_CHARS=Locator.ALLOWED_ID_CHARS ) - URL_RE = re.compile('^' + URL_RE_SOURCE) + URL_RE = re.compile(URL_RE_SOURCE) def __init__(self, org=None, course=None, **kwargs): """ @@ -1386,7 +1384,7 @@ def _to_string(self): """ Return a string representing this location. """ - return u"{org}+{course}".format( + return u'{org}+{course}'.format( org=self.org, course=self.course ) From b82477937afb80bab61d168f6a60c34367730e5f Mon Sep 17 00:00:00 2001 From: zubair-arbi Date: Mon, 6 Mar 2017 15:33:37 +0500 Subject: [PATCH 03/10] add tests for aggregate course locator --- opaque_keys/edx/tests/test_locators.py | 34 ++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/opaque_keys/edx/tests/test_locators.py b/opaque_keys/edx/tests/test_locators.py index 036778ef..807e8dd3 100644 --- a/opaque_keys/edx/tests/test_locators.py +++ b/opaque_keys/edx/tests/test_locators.py @@ -5,11 +5,13 @@ import random +import ddt from six import text_type from bson.objectid import ObjectId -from opaque_keys.edx.locator import Locator, CourseLocator, DefinitionLocator, VersionTree -from opaque_keys.edx.keys import DefinitionKey +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import DefinitionKey, AggregateCourseKey +from opaque_keys.edx.locator import Locator, CourseLocator, DefinitionLocator, VersionTree, AggregateCourseLocator class LocatorTests(TestCase): @@ -59,3 +61,31 @@ def test_version_tree(self): test_id = ObjectId(test_id_loc) valid_locator = CourseLocator(version_guid=test_id) self.assertEqual(VersionTree(valid_locator).children, []) + + +@ddt.ddt +class AggregateCourseLocatorTests(TestCase): + """ + Tests for :class:`.AggregateCourseLocator` + """ + + def test_aggregate_course_locator(self): + aggregate_course_key = 'aggregate-course:org+course' + aggregate_course_locator = AggregateCourseLocator(org='org', course='course') + self.assertEqual(aggregate_course_locator, AggregateCourseKey.from_string(aggregate_course_key)) + + def test_aggregate_course_locator_serialize(self): + aggregate_course_key = 'aggregate-course:org+course' + aggregate_course_locator = AggregateCourseKey.from_string(aggregate_course_key) + serialized_aggregate_course_key = 'org+course' + self.assertEqual(serialized_aggregate_course_key, aggregate_course_locator.serialize()) + + @ddt.data( + "org/course/run", + "org+course+run", + "org+course+run+foo", + "aggregate-course:org+course+run", + ) + def test_invalid_format_course_key(self, course_key): + with self.assertRaises(InvalidKeyError): + AggregateCourseKey.from_string(course_key) From 1f24d585070751efe4e6476bda554da32d639f64 Mon Sep 17 00:00:00 2001 From: zubair-arbi Date: Tue, 7 Mar 2017 12:19:06 +0500 Subject: [PATCH 04/10] remove serialize method in favor of _to_string + add from_course_key method to create aggregate course key from a course key object + add/update tests --- opaque_keys/edx/keys.py | 8 ++++++-- opaque_keys/edx/locator.py | 17 +++++++++++------ opaque_keys/edx/tests/test_locators.py | 25 +++++++++++++++++++------ 3 files changed, 36 insertions(+), 14 deletions(-) diff --git a/opaque_keys/edx/keys.py b/opaque_keys/edx/keys.py index 5df08200..6ca6c9b5 100644 --- a/opaque_keys/edx/keys.py +++ b/opaque_keys/edx/keys.py @@ -86,9 +86,13 @@ class AggregateCourseKey(OpaqueKey): __slots__ = () @abstractproperty - def serialize(self): # pragma: no cover + def from_course_key(self, course_key): # pragma: no cover """ - The serialized course key without run information. + Get aggregate course key from the course run key. + + Arguments: + course_key (:class:`CourseKey`): The course identifier. + """ raise NotImplementedError() diff --git a/opaque_keys/edx/locator.py b/opaque_keys/edx/locator.py index f0da1957..05b697dc 100644 --- a/opaque_keys/edx/locator.py +++ b/opaque_keys/edx/locator.py @@ -1364,6 +1364,17 @@ def __init__(self, org=None, course=None, **kwargs): if self.org is None or self.course is None: raise InvalidKeyError(self.__class__, 'Both org and course should be set.') + @classmethod + def from_course_key(cls, course_key): + """ + Get aggregate course key from the course run key object. + + Arguments: + course_key (:class:`CourseKey`): The course identifier. + + """ + return cls(**{key: getattr(course_key, key) for key in cls.KEY_FIELDS}) + @classmethod def _from_string(cls, serialized): """ @@ -1388,9 +1399,3 @@ def _to_string(self): org=self.org, course=self.course ) - - def serialize(self): - """ - The serialized course key without run information. - """ - return '{org}+{course}'.format(org=self.org, course=self.course) diff --git a/opaque_keys/edx/tests/test_locators.py b/opaque_keys/edx/tests/test_locators.py index 807e8dd3..7b5a0ff2 100644 --- a/opaque_keys/edx/tests/test_locators.py +++ b/opaque_keys/edx/tests/test_locators.py @@ -10,7 +10,7 @@ from bson.objectid import ObjectId from opaque_keys import InvalidKeyError -from opaque_keys.edx.keys import DefinitionKey, AggregateCourseKey +from opaque_keys.edx.keys import DefinitionKey, CourseKey, AggregateCourseKey from opaque_keys.edx.locator import Locator, CourseLocator, DefinitionLocator, VersionTree, AggregateCourseLocator @@ -74,17 +74,30 @@ def test_aggregate_course_locator(self): aggregate_course_locator = AggregateCourseLocator(org='org', course='course') self.assertEqual(aggregate_course_locator, AggregateCourseKey.from_string(aggregate_course_key)) + @ddt.data( + 'org/course/run', + 'course-v1:org+course+run', + ) + def test_aggregate_course_locator_from_course_key(self, course_id): + course = CourseKey.from_string(course_id) + aggregate_course_locator = AggregateCourseLocator.from_course_key(course) + + aggregate_course_key = 'aggregate-course:{}+{}'.format(course.org, course.course) + self.assertEqual(aggregate_course_locator, AggregateCourseKey.from_string(aggregate_course_key)) + def test_aggregate_course_locator_serialize(self): aggregate_course_key = 'aggregate-course:org+course' aggregate_course_locator = AggregateCourseKey.from_string(aggregate_course_key) serialized_aggregate_course_key = 'org+course' - self.assertEqual(serialized_aggregate_course_key, aggregate_course_locator.serialize()) + # Allow access to _to_string + # pylint: disable=protected-access + self.assertEqual(serialized_aggregate_course_key, aggregate_course_locator._to_string()) @ddt.data( - "org/course/run", - "org+course+run", - "org+course+run+foo", - "aggregate-course:org+course+run", + 'org/course/run', + 'org+course+run', + 'org+course+run+foo', + 'aggregate-course:org+course+run', ) def test_invalid_format_course_key(self, course_key): with self.assertRaises(InvalidKeyError): From 12086b51663c04f6f05d1e5844849de381f63b88 Mon Sep 17 00:00:00 2001 From: zubair-arbi Date: Wed, 8 Mar 2017 13:04:26 +0500 Subject: [PATCH 05/10] address feedback from clinton, part 1 --- opaque_keys/edx/locator.py | 26 +++++++-------- opaque_keys/edx/tests/test_locators.py | 45 +++++++++++++++++--------- setup.py | 4 +-- 3 files changed, 43 insertions(+), 32 deletions(-) diff --git a/opaque_keys/edx/locator.py b/opaque_keys/edx/locator.py index 05b697dc..28442332 100644 --- a/opaque_keys/edx/locator.py +++ b/opaque_keys/edx/locator.py @@ -1336,33 +1336,29 @@ class AggregateCourseLocator(AggregateCourseKey): # pylint: disable=abstract- """ An AggregateCourseKey implementation class. """ - CANONICAL_NAMESPACE = 'aggregate-course' + CANONICAL_NAMESPACE = 'course-v2' KEY_FIELDS = ('org', 'course') __slots__ = KEY_FIELDS - URL_RE_SOURCE = r'^(?P{ALLOWED_ID_CHARS}+)\+(?P{ALLOWED_ID_CHARS}+)$'.format( - ALLOWED_ID_CHARS=Locator.ALLOWED_ID_CHARS + KEY_REGEX = re.compile( + r'^(?P{ALLOWED_ID_CHARS}+)\+(?P{ALLOWED_ID_CHARS}+)$'.format( + ALLOWED_ID_CHARS=Locator.ALLOWED_ID_CHARS + ) ) - URL_RE = re.compile(URL_RE_SOURCE) - def __init__(self, org=None, course=None, **kwargs): """ Construct a AggregateCourseLocator. Arguments: org (string): Organization identifier for the course - course (string): Course name + course (string): Course number """ - super(AggregateCourseLocator, self).__init__( - org=org, - course=course, - **kwargs - ) + super(AggregateCourseLocator, self).__init__(org=org, course=course, **kwargs) - if self.org is None or self.course is None: - raise InvalidKeyError(self.__class__, 'Both org and course should be set.') + if not (self.org and self.course): + raise InvalidKeyError(self.__class__, 'Both org and course must be set.') @classmethod def from_course_key(cls, course_key): @@ -1384,7 +1380,7 @@ def _from_string(cls, serialized): serialized: string for matching """ - parse = cls.URL_RE.match(serialized) + parse = cls.KEY_REGEX.match(serialized) if not parse: raise InvalidKeyError(cls, serialized) @@ -1395,7 +1391,7 @@ def _to_string(self): """ Return a string representing this location. """ - return u'{org}+{course}'.format( + return '{org}+{course}'.format( org=self.org, course=self.course ) diff --git a/opaque_keys/edx/tests/test_locators.py b/opaque_keys/edx/tests/test_locators.py index 7b5a0ff2..78a9201a 100644 --- a/opaque_keys/edx/tests/test_locators.py +++ b/opaque_keys/edx/tests/test_locators.py @@ -70,35 +70,50 @@ class AggregateCourseLocatorTests(TestCase): """ def test_aggregate_course_locator(self): - aggregate_course_key = 'aggregate-course:org+course' - aggregate_course_locator = AggregateCourseLocator(org='org', course='course') - self.assertEqual(aggregate_course_locator, AggregateCourseKey.from_string(aggregate_course_key)) + """ + Verify that the method "from_string" of class "AggregateCourseKey" + returns an object of "AggregateCourseLocator" for a valid course key. + """ + aggregate_course_key = 'course-v2:org+course' + aggregate_course_locator = AggregateCourseKey.from_string(aggregate_course_key) + expected_course_locator = AggregateCourseLocator(org='org', course='course') + self.assertEqual(expected_course_locator, aggregate_course_locator) @ddt.data( 'org/course/run', 'course-v1:org+course+run', ) def test_aggregate_course_locator_from_course_key(self, course_id): - course = CourseKey.from_string(course_id) - aggregate_course_locator = AggregateCourseLocator.from_course_key(course) - - aggregate_course_key = 'aggregate-course:{}+{}'.format(course.org, course.course) - self.assertEqual(aggregate_course_locator, AggregateCourseKey.from_string(aggregate_course_key)) + """ + Verify that the method "from_course_key" of class "AggregateCourseLocator" + coverts a valid course run key to an aggregate course key. + """ + course_key = CourseKey.from_string(course_id) + expected_course_key = AggregateCourseLocator(org=course_key.org, course=course_key.course) + actual_course_key = AggregateCourseLocator.from_course_key(course_key) + self.assertEqual(expected_course_key, actual_course_key) - def test_aggregate_course_locator_serialize(self): - aggregate_course_key = 'aggregate-course:org+course' - aggregate_course_locator = AggregateCourseKey.from_string(aggregate_course_key) - serialized_aggregate_course_key = 'org+course' - # Allow access to _to_string + def test_serialize_to_string(self): + """ + Verify that the method "from_course_key" of class "AggregateCourseLocator" + coverts a valid course run key to an aggregate course key. + """ + course_key = CourseKey.from_string('course-v1:org+course+run') + aggregate_course_locator = AggregateCourseLocator(org=course_key.org, course=course_key.course) + expected_serialized_key = '{org}+{course}'.format(org=course_key.org, course=course_key.course) # pylint: disable=protected-access - self.assertEqual(serialized_aggregate_course_key, aggregate_course_locator._to_string()) + self.assertEqual(expected_serialized_key, aggregate_course_locator._to_string()) @ddt.data( 'org/course/run', 'org+course+run', 'org+course+run+foo', - 'aggregate-course:org+course+run', + 'course-v2:org+course+run', ) def test_invalid_format_course_key(self, course_key): + """ + Verify that the method "from_string" of class "AggregateCourseKey" + raises exception "InvalidKeyError" for unsupported key formats. + """ with self.assertRaises(InvalidKeyError): AggregateCourseKey.from_string(course_key) diff --git a/setup.py b/setup.py index 69b3e113..af0447bf 100644 --- a/setup.py +++ b/setup.py @@ -2,7 +2,7 @@ setup( name='edx-opaque-keys', - version='0.5', + version='0.5.0', author='edX', url='https://github.com/edx/opaque-keys', classifiers=[ @@ -32,7 +32,7 @@ 'slashes = opaque_keys.edx.locator:CourseLocator', ], 'aggregate_course_key': [ - 'aggregate-course = opaque_keys.edx.locator:AggregateCourseLocator', + 'course-v2 = opaque_keys.edx.locator:AggregateCourseLocator', ], 'usage_key': [ 'block-v1 = opaque_keys.edx.locator:BlockUsageLocator', From b1b4a1b2fed44572e35a1b90b8bf0597bdb5375a Mon Sep 17 00:00:00 2001 From: zubair-arbi Date: Thu, 9 Mar 2017 16:11:16 +0500 Subject: [PATCH 06/10] change naming formate from aggregate course key to course key v2 --- opaque_keys/edx/keys.py | 6 ++-- opaque_keys/edx/locator.py | 14 ++++---- opaque_keys/edx/tests/test_locators.py | 44 +++++++++++++------------- setup.py | 4 +-- 4 files changed, 34 insertions(+), 34 deletions(-) diff --git a/opaque_keys/edx/keys.py b/opaque_keys/edx/keys.py index 6ca6c9b5..f64db257 100644 --- a/opaque_keys/edx/keys.py +++ b/opaque_keys/edx/keys.py @@ -77,18 +77,18 @@ def block_type(self): # pragma: no cover raise NotImplementedError() -class AggregateCourseKey(OpaqueKey): +class CourseKeyV2(OpaqueKey): """ An :class:`opaque_keys.OpaqueKey` identifying a serialized Course Key object. """ - KEY_TYPE = 'aggregate_course_key' + KEY_TYPE = 'course_key_v2' __slots__ = () @abstractproperty def from_course_key(self, course_key): # pragma: no cover """ - Get aggregate course key from the course run key. + Get course key v2 from the course run key. Arguments: course_key (:class:`CourseKey`): The course identifier. diff --git a/opaque_keys/edx/locator.py b/opaque_keys/edx/locator.py index 28442332..43b185da 100644 --- a/opaque_keys/edx/locator.py +++ b/opaque_keys/edx/locator.py @@ -16,7 +16,7 @@ from six import string_types, text_type from opaque_keys import OpaqueKey, InvalidKeyError -from opaque_keys.edx.keys import CourseKey, UsageKey, DefinitionKey, AssetKey, AggregateCourseKey +from opaque_keys.edx.keys import CourseKey, UsageKey, DefinitionKey, AssetKey, CourseKeyV2 log = logging.getLogger(__name__) @@ -1332,9 +1332,9 @@ def to_deprecated_list_repr(self): AssetKey.set_deprecated_fallback(AssetLocator) -class AggregateCourseLocator(AggregateCourseKey): # pylint: disable=abstract-method +class CourseLocatorV2(CourseKeyV2): # pylint: disable=abstract-method """ - An AggregateCourseKey implementation class. + An CourseKeyV2 implementation class. """ CANONICAL_NAMESPACE = 'course-v2' KEY_FIELDS = ('org', 'course') @@ -1348,14 +1348,14 @@ class AggregateCourseLocator(AggregateCourseKey): # pylint: disable=abstract- def __init__(self, org=None, course=None, **kwargs): """ - Construct a AggregateCourseLocator. + Construct a CourseLocatorV2. Arguments: org (string): Organization identifier for the course course (string): Course number """ - super(AggregateCourseLocator, self).__init__(org=org, course=course, **kwargs) + super(CourseLocatorV2, self).__init__(org=org, course=course, **kwargs) if not (self.org and self.course): raise InvalidKeyError(self.__class__, 'Both org and course must be set.') @@ -1363,7 +1363,7 @@ def __init__(self, org=None, course=None, **kwargs): @classmethod def from_course_key(cls, course_key): """ - Get aggregate course key from the course run key object. + Get course key v2 from the course run key object. Arguments: course_key (:class:`CourseKey`): The course identifier. @@ -1374,7 +1374,7 @@ def from_course_key(cls, course_key): @classmethod def _from_string(cls, serialized): """ - Return a AggregateCourseLocator parsing the given serialized string. + Return a CourseLocatorV2 parsing the given serialized string. Arguments: serialized: string for matching diff --git a/opaque_keys/edx/tests/test_locators.py b/opaque_keys/edx/tests/test_locators.py index 78a9201a..2fa840eb 100644 --- a/opaque_keys/edx/tests/test_locators.py +++ b/opaque_keys/edx/tests/test_locators.py @@ -10,8 +10,8 @@ from bson.objectid import ObjectId from opaque_keys import InvalidKeyError -from opaque_keys.edx.keys import DefinitionKey, CourseKey, AggregateCourseKey -from opaque_keys.edx.locator import Locator, CourseLocator, DefinitionLocator, VersionTree, AggregateCourseLocator +from opaque_keys.edx.keys import DefinitionKey, CourseKey, CourseKeyV2 +from opaque_keys.edx.locator import Locator, CourseLocator, DefinitionLocator, VersionTree, CourseLocatorV2 class LocatorTests(TestCase): @@ -64,45 +64,45 @@ def test_version_tree(self): @ddt.ddt -class AggregateCourseLocatorTests(TestCase): +class CourseLocatorV2Tests(TestCase): """ - Tests for :class:`.AggregateCourseLocator` + Tests for :class:`.CourseLocatorV2` """ - def test_aggregate_course_locator(self): + def test_course_locator_v2(self): """ - Verify that the method "from_string" of class "AggregateCourseKey" - returns an object of "AggregateCourseLocator" for a valid course key. + Verify that the method "from_string" of class "CourseKeyV2" + returns an object of "CourseLocatorV2" for a valid course key. """ - aggregate_course_key = 'course-v2:org+course' - aggregate_course_locator = AggregateCourseKey.from_string(aggregate_course_key) - expected_course_locator = AggregateCourseLocator(org='org', course='course') - self.assertEqual(expected_course_locator, aggregate_course_locator) + course_key_v2 = 'course-v2:org+course' + course_locator_v2 = CourseKeyV2.from_string(course_key_v2) + expected_course_locator = CourseLocatorV2(org='org', course='course') + self.assertEqual(expected_course_locator, course_locator_v2) @ddt.data( 'org/course/run', 'course-v1:org+course+run', ) - def test_aggregate_course_locator_from_course_key(self, course_id): + def test_course_locator_v2_from_course_key(self, course_id): """ - Verify that the method "from_course_key" of class "AggregateCourseLocator" - coverts a valid course run key to an aggregate course key. + Verify that the method "from_course_key" of class "CourseLocatorV2" + coverts a valid course run key to a course key v2. """ course_key = CourseKey.from_string(course_id) - expected_course_key = AggregateCourseLocator(org=course_key.org, course=course_key.course) - actual_course_key = AggregateCourseLocator.from_course_key(course_key) + expected_course_key = CourseLocatorV2(org=course_key.org, course=course_key.course) + actual_course_key = CourseLocatorV2.from_course_key(course_key) self.assertEqual(expected_course_key, actual_course_key) def test_serialize_to_string(self): """ - Verify that the method "from_course_key" of class "AggregateCourseLocator" - coverts a valid course run key to an aggregate course key. + Verify that the method "_to_string" of class "CourseLocatorV2" + serializes a course key v2 to a string with expected format. """ course_key = CourseKey.from_string('course-v1:org+course+run') - aggregate_course_locator = AggregateCourseLocator(org=course_key.org, course=course_key.course) + course_locator_v2 = CourseLocatorV2(org=course_key.org, course=course_key.course) expected_serialized_key = '{org}+{course}'.format(org=course_key.org, course=course_key.course) # pylint: disable=protected-access - self.assertEqual(expected_serialized_key, aggregate_course_locator._to_string()) + self.assertEqual(expected_serialized_key, course_locator_v2._to_string()) @ddt.data( 'org/course/run', @@ -112,8 +112,8 @@ def test_serialize_to_string(self): ) def test_invalid_format_course_key(self, course_key): """ - Verify that the method "from_string" of class "AggregateCourseKey" + Verify that the method "from_string" of class "CourseKeyV2" raises exception "InvalidKeyError" for unsupported key formats. """ with self.assertRaises(InvalidKeyError): - AggregateCourseKey.from_string(course_key) + CourseKeyV2.from_string(course_key) diff --git a/setup.py b/setup.py index af0447bf..12ad34a5 100644 --- a/setup.py +++ b/setup.py @@ -31,8 +31,8 @@ # don't use slashes in any new code 'slashes = opaque_keys.edx.locator:CourseLocator', ], - 'aggregate_course_key': [ - 'course-v2 = opaque_keys.edx.locator:AggregateCourseLocator', + 'course_key_v2': [ + 'course-v2 = opaque_keys.edx.locator:CourseLocatorV2', ], 'usage_key': [ 'block-v1 = opaque_keys.edx.locator:BlockUsageLocator', From 7a7595ab8dc500a52239ed09ce38ebcf03a93a1c Mon Sep 17 00:00:00 2001 From: zubair-arbi Date: Fri, 10 Mar 2017 13:02:42 +0500 Subject: [PATCH 07/10] address feedback from clinton, part 2 --- opaque_keys/edx/keys.py | 2 +- opaque_keys/edx/locator.py | 2 +- opaque_keys/edx/tests/test_locators.py | 30 +++++++++++++------------- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/opaque_keys/edx/keys.py b/opaque_keys/edx/keys.py index f64db257..58a92f7f 100644 --- a/opaque_keys/edx/keys.py +++ b/opaque_keys/edx/keys.py @@ -86,7 +86,7 @@ class CourseKeyV2(OpaqueKey): __slots__ = () @abstractproperty - def from_course_key(self, course_key): # pragma: no cover + def from_course_run_key(self, course_key): # pragma: no cover """ Get course key v2 from the course run key. diff --git a/opaque_keys/edx/locator.py b/opaque_keys/edx/locator.py index 43b185da..dd0e6e06 100644 --- a/opaque_keys/edx/locator.py +++ b/opaque_keys/edx/locator.py @@ -1361,7 +1361,7 @@ def __init__(self, org=None, course=None, **kwargs): raise InvalidKeyError(self.__class__, 'Both org and course must be set.') @classmethod - def from_course_key(cls, course_key): + def from_course_run_key(cls, course_key): """ Get course key v2 from the course run key object. diff --git a/opaque_keys/edx/tests/test_locators.py b/opaque_keys/edx/tests/test_locators.py index 2fa840eb..43a2bc1e 100644 --- a/opaque_keys/edx/tests/test_locators.py +++ b/opaque_keys/edx/tests/test_locators.py @@ -69,10 +69,10 @@ class CourseLocatorV2Tests(TestCase): Tests for :class:`.CourseLocatorV2` """ - def test_course_locator_v2(self): + def test_from_string(self): """ - Verify that the method "from_string" of class "CourseKeyV2" - returns an object of "CourseLocatorV2" for a valid course key. + Verify that the method `from_string` of class `CourseKeyV2` + returns an object of `CourseLocatorV2` for a valid course key. """ course_key_v2 = 'course-v2:org+course' course_locator_v2 = CourseKeyV2.from_string(course_key_v2) @@ -83,26 +83,26 @@ def test_course_locator_v2(self): 'org/course/run', 'course-v1:org+course+run', ) - def test_course_locator_v2_from_course_key(self, course_id): + def test_from_course_run_key(self, course_id): """ - Verify that the method "from_course_key" of class "CourseLocatorV2" + Verify that the method `from_course_run_key` of class `CourseLocatorV2` coverts a valid course run key to a course key v2. """ course_key = CourseKey.from_string(course_id) expected_course_key = CourseLocatorV2(org=course_key.org, course=course_key.course) - actual_course_key = CourseLocatorV2.from_course_key(course_key) + actual_course_key = CourseLocatorV2.from_course_run_key(course_key) self.assertEqual(expected_course_key, actual_course_key) def test_serialize_to_string(self): """ - Verify that the method "_to_string" of class "CourseLocatorV2" + Verify that the method `_to_string` of class `CourseLocatorV2` serializes a course key v2 to a string with expected format. """ - course_key = CourseKey.from_string('course-v1:org+course+run') - course_locator_v2 = CourseLocatorV2(org=course_key.org, course=course_key.course) - expected_serialized_key = '{org}+{course}'.format(org=course_key.org, course=course_key.course) - # pylint: disable=protected-access - self.assertEqual(expected_serialized_key, course_locator_v2._to_string()) + organization = 'org' + course_number = 'course' + course_locator_v2 = CourseLocatorV2(org=organization, course=course_number) + expected_serialized_key = 'course-v2:{org}+{course}'.format(org=organization, course=course_number) + self.assertEqual(expected_serialized_key, str(course_locator_v2)) @ddt.data( 'org/course/run', @@ -110,10 +110,10 @@ def test_serialize_to_string(self): 'org+course+run+foo', 'course-v2:org+course+run', ) - def test_invalid_format_course_key(self, course_key): + def test_from_string_with_invalid_input(self, course_key): """ - Verify that the method "from_string" of class "CourseKeyV2" - raises exception "InvalidKeyError" for unsupported key formats. + Verify that the method `from_string` of class `CourseKeyV2` + raises exception `InvalidKeyError` for unsupported key formats. """ with self.assertRaises(InvalidKeyError): CourseKeyV2.from_string(course_key) From b6f4b81428b1968a14a6ec4dd3a266b6001b10af Mon Sep 17 00:00:00 2001 From: zubair-arbi Date: Fri, 10 Mar 2017 16:14:30 +0500 Subject: [PATCH 08/10] add inter conversion methods in course key and course key v2 --- opaque_keys/edx/keys.py | 18 ++++++++++++++++++ opaque_keys/edx/locator.py | 16 ++++++++++++++++ opaque_keys/edx/tests/test_course_locators.py | 14 +++++++++++++- opaque_keys/edx/tests/test_locators.py | 12 ++++++++++++ 4 files changed, 59 insertions(+), 1 deletion(-) diff --git a/opaque_keys/edx/keys.py b/opaque_keys/edx/keys.py index 58a92f7f..11739935 100644 --- a/opaque_keys/edx/keys.py +++ b/opaque_keys/edx/keys.py @@ -61,6 +61,13 @@ def make_asset_key(self, asset_type, path): # pragma: no cover """ raise NotImplementedError() + @abstractmethod + def make_course_key_v2(self): # pragma: no cover + """ + Returns a course key v2 object without run information. + """ + raise NotImplementedError() + class DefinitionKey(OpaqueKey): """ @@ -96,6 +103,17 @@ def from_course_run_key(self, course_key): # pragma: no cover """ raise NotImplementedError() + @abstractproperty + def make_course_run_key(self, course_run): # pragma: no cover + """ + Get course run key (course_key) from the course key v2. + + Arguments: + course_run (str): The course run for course run identifier. + + """ + raise NotImplementedError() + class CourseObjectMixin(object): """ diff --git a/opaque_keys/edx/locator.py b/opaque_keys/edx/locator.py index dd0e6e06..3a6afbd5 100644 --- a/opaque_keys/edx/locator.py +++ b/opaque_keys/edx/locator.py @@ -380,6 +380,12 @@ def _from_deprecated_string(cls, serialized): return cls(*serialized.split('/'), deprecated=True) + def make_course_key_v2(self): + """ + Returns a course key v2 object without run information. + """ + return CourseLocatorV2(org=self.org, course=self.course) + CourseKey.set_deprecated_fallback(CourseLocator) @@ -1371,6 +1377,16 @@ def from_course_run_key(cls, course_key): """ return cls(**{key: getattr(course_key, key) for key in cls.KEY_FIELDS}) + def make_course_run_key(self, course_run): + """ + Get course run key (course_key) from the course key v2. + + Arguments: + course_run (str): The course run for course run identifier. + + """ + return CourseLocator(org=self.org, course=self.course, run=course_run) + @classmethod def _from_string(cls, serialized): """ diff --git a/opaque_keys/edx/tests/test_course_locators.py b/opaque_keys/edx/tests/test_course_locators.py index fbe17b1c..4d952f0b 100644 --- a/opaque_keys/edx/tests/test_course_locators.py +++ b/opaque_keys/edx/tests/test_course_locators.py @@ -11,7 +11,7 @@ from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey -from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator +from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator, CourseLocatorV2 from opaque_keys.edx.tests import LocatorBaseTest, TestDeprecated @@ -279,3 +279,15 @@ def test_empty_run(self): 'org/course/', text_type(CourseLocator('org', 'course', '', deprecated=True)) ) + + def test_make_course_key_v2(self): + """ + Verify that the method `make_course_key_v2` of class `CourseLocator` + returns a course key v2 object without course run information. + """ + organization = 'org' + course_number = 'course' + course_run = 'run' + course_run_key = CourseLocator(org=organization, course=course_number, run=course_run) + expected_course_key_v2 = CourseLocatorV2(org=organization, course=course_number) + self.assertEqual(expected_course_key_v2, course_run_key.make_course_key_v2()) diff --git a/opaque_keys/edx/tests/test_locators.py b/opaque_keys/edx/tests/test_locators.py index 43a2bc1e..5817a2bf 100644 --- a/opaque_keys/edx/tests/test_locators.py +++ b/opaque_keys/edx/tests/test_locators.py @@ -93,6 +93,18 @@ def test_from_course_run_key(self, course_id): actual_course_key = CourseLocatorV2.from_course_run_key(course_key) self.assertEqual(expected_course_key, actual_course_key) + def test_make_course_run_key(self): + """ + Verify that the method `make_course_run_key` of class `CourseLocatorV2` + returns a course run key with provided run. + """ + organization = 'org' + course_number = 'course' + course_run = 'run' + course_key_v2 = CourseLocatorV2(org=organization, course=course_number) + expected_course_run_key = CourseLocator(org=organization, course=course_number, run=course_run) + self.assertEqual(expected_course_run_key, course_key_v2.make_course_run_key(course_run)) + def test_serialize_to_string(self): """ Verify that the method `_to_string` of class `CourseLocatorV2` From 39e568fa0b661511eefa06f2f82e84274ab7901d Mon Sep 17 00:00:00 2001 From: zubair-arbi Date: Fri, 10 Mar 2017 16:17:31 +0500 Subject: [PATCH 09/10] remove ddt from test_from_course_run_key --- opaque_keys/edx/tests/test_locators.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/opaque_keys/edx/tests/test_locators.py b/opaque_keys/edx/tests/test_locators.py index 5817a2bf..39cc9a56 100644 --- a/opaque_keys/edx/tests/test_locators.py +++ b/opaque_keys/edx/tests/test_locators.py @@ -79,16 +79,12 @@ def test_from_string(self): expected_course_locator = CourseLocatorV2(org='org', course='course') self.assertEqual(expected_course_locator, course_locator_v2) - @ddt.data( - 'org/course/run', - 'course-v1:org+course+run', - ) - def test_from_course_run_key(self, course_id): + def test_from_course_run_key(self): """ Verify that the method `from_course_run_key` of class `CourseLocatorV2` coverts a valid course run key to a course key v2. """ - course_key = CourseKey.from_string(course_id) + course_key = CourseKey.from_string('course-v1:org+course+run') expected_course_key = CourseLocatorV2(org=course_key.org, course=course_key.course) actual_course_key = CourseLocatorV2.from_course_run_key(course_key) self.assertEqual(expected_course_key, actual_course_key) From 0fd82da4bf4d81524421b416ca8049bd54a5f4b5 Mon Sep 17 00:00:00 2001 From: zubair-arbi Date: Fri, 10 Mar 2017 17:05:46 +0500 Subject: [PATCH 10/10] add method "make_course_key_v2" in LibraryLocator which raises NotImplementedError as library locators are course agnostic --- opaque_keys/edx/locator.py | 9 +++++---- opaque_keys/edx/tests/test_library_locators.py | 9 +++++++++ 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/opaque_keys/edx/locator.py b/opaque_keys/edx/locator.py index 3a6afbd5..58f1d638 100644 --- a/opaque_keys/edx/locator.py +++ b/opaque_keys/edx/locator.py @@ -580,10 +580,11 @@ def _to_deprecated_string(self): """ LibraryLocators are never deprecated. """ raise NotImplementedError - @classmethod - def _from_deprecated_string(cls, serialized): - """ LibraryLocators are never deprecated. """ - raise NotImplementedError + def make_course_key_v2(self): # pragma: no cover + """ + Returns a course key v2 object without run information. + """ + raise NotImplementedError() class BlockUsageLocator(BlockLocatorBase, UsageKey): diff --git a/opaque_keys/edx/tests/test_library_locators.py b/opaque_keys/edx/tests/test_library_locators.py index 43caf731..6bf94c78 100644 --- a/opaque_keys/edx/tests/test_library_locators.py +++ b/opaque_keys/edx/tests/test_library_locators.py @@ -237,3 +237,12 @@ def test_course_agnostic(self): self.assertEqual(lib_key2, lib_key3) self.assertEqual(lib_key3.org, None) self.assertEqual(lib_key3.library, None) + + def test_make_course_key_v2(self): + """ + Verify that the method `make_course_key_v2` of class `LibraryLocator` + raises exception `NotImplementedError`. + """ + lib_key = CourseKey.from_string('library-v1:TestX+lib1') + with self.assertRaises(NotImplementedError): + lib_key.make_course_key_v2() # pylint: disable=protected-access