ENT-211 add course key v2 #87

zubair-arbi · 2017-03-03T10:48:40Z

@asadiqbal08 @saleem-latif @mattdrayer
Add a new type CourseKeyV2 of OpaqueKey for handling course keys without run information.

Usage:

>>> from opaque_keys.edx.keys import CourseKeyV2
>>> from opaque_keys.edx.locator import CourseLocatorV2
>>> course_key_v2 = CourseKeyV2.from_string('course-v2:edX+DemoX')
>>> course_key_v2
CourseLocatorV2(org='edX', course='DemoX')
>>> course_key_v2._to_string()
'edX+DemoX'
>>> str(course_key_v2)
'course-v2:edX+DemoX'
>>> course_run_key = CourseKey.from_string('course-v1:edX+DemoX+Demo_Course')
>>> course_run_key.make_course_key_v2()
CourseLocatorV2(u'edX', u'DemoX')
>>> course_locator_v2 = CourseLocatorV2.from_course_run_key(course_run_key)
>>> course_locator_v2
CourseLocatorV2(u'edX', u'DemoX')
>>> course_locator_v2.make_course_run_key('Spring_2017')
CourseLocator(u'edX', u'DemoX', 'Spring_2017', None, None)

coveralls · 2017-03-03T10:59:16Z

Coverage decreased (-0.4%) to 96.081% when pulling f72a1db on zub/ENT-211-add-aggregate-course-key into ffb2ee1 on master.

…egagte course key

zubair-arbi · 2017-03-03T11:39:03Z

@cpennington Please let me know if this implementation is what you have suggested.

coveralls · 2017-03-03T11:40:14Z

Coverage decreased (-0.4%) to 96.081% when pulling 11498a5 on zub/ENT-211-add-aggregate-course-key into ffb2ee1 on master.

coveralls · 2017-03-06T10:44:26Z

Coverage decreased (-0.07%) to 96.43% when pulling 5f565ee on zub/ENT-211-add-aggregate-course-key into ffb2ee1 on master.

coveralls · 2017-03-06T10:55:45Z

Coverage decreased (-0.03%) to 96.476% when pulling b824779 on zub/ENT-211-add-aggregate-course-key into ffb2ee1 on master.

coveralls · 2017-03-06T10:56:58Z

Coverage decreased (-0.03%) to 96.476% when pulling b824779 on zub/ENT-211-add-aggregate-course-key into ffb2ee1 on master.

cpennington · 2017-03-06T15:53:08Z

opaque_keys/edx/locator.py

+        """
+        The serialized course key without run information.
+        """
+        return '{org}+{course}'.format(org=self.org, course=self.course)


If you've implement _to_string, you don't need serialize. If you override serialize like this, it's going to break the namespacing that's built into opaque keys, which will make it impossible to have new/alternate versions of AggregateCourseKeys .

Removed the method serialize

cpennington · 2017-03-06T16:45:13Z

opaque_keys/edx/locator.py

@@ -1330,3 +1330,67 @@ def to_deprecated_list_repr(self):

 # Register AssetLocator as the deprecated fallback for AssetKey
 AssetKey.set_deprecated_fallback(AssetLocator)
+
+
+class AggregateCourseLocator(AggregateCourseKey):    # pylint: disable=abstract-method


What formats of key does this need to parse? Can you add some tests to make sure it parses/serializes the way you expect?

Also, should you be able to get an AggregateCourseKey from a CourseKey?

@cpennington @clintonb The only format need is org+course. I have added/update tests in opaque_keys/edx/tests/test_locators.py for the AggregateCourseLocator.

I was unable to directly get a AggregateCourseKey locator from a CourseKey locator mainly due to changed KEY_TYPE and OpaqueKey Hierarchy. To handle the conversion from CourseKey to AggregateCourseKey I have added a new method from_course_key in AggregateCourseKey which takes a course key object and returns a locator AggregateCourseLocator from it. Please let me know your suggestion on this approach.

a) I'm pretty skeptical about the claim that the only format needed is org+course. We've been down this road before, and being able to version the key formats is quite useful over the long term. Please don't neglect that.

b) My suggestion would be to add a method to CourseKey called get_aggregate_course_key which would construct an appropriate AggregateCourseKey object. That's consistent with how we've managed this in the past. You might also add something to AggregateCourseKey that let's you construct a particular run CourseKey by supplying the run_id. (This is all very much in parallel to the relationship between CourseKey and UsageKey).

a) Sorry I forgot to mention that the complete format of course key has version, e.g. course-v2:edX+DemoX
b) I have added the method make_course_key_v2 in CourseKey which will return CourseKeyV2 locator. Also added the method make_course_run_key in CourseKeyV2 which will return a CourseKey locator CourseLocator from the provided course run.
@cpennington thanks for the suggestions. Please have a look on the changes and let me know if I have missed anything.

…method to create aggregate course key from a course key object + add/update tests

coveralls · 2017-03-07T07:32:41Z

Coverage increased (+0.03%) to 96.53% when pulling 1f24d58 on zub/ENT-211-add-aggregate-course-key into ffb2ee1 on master.

clintonb · 2017-03-07T12:52:34Z

opaque_keys/edx/keys.py

+    An :class:`opaque_keys.OpaqueKey` identifying a
+    serialized Course Key object.
+    """
+    KEY_TYPE = 'aggregate_course_key'


@cpennington any reason (aside from confusion) not to call this course-v2 and use course-run-v1 to replace course-v1?

No technical issues that I can think of.

Is this something we should do as part of this PR? We can certainly make the change but I don't want to introduce two new ideas at one time if we don't have to

I would call this course-v2 now. Add a ticket to your backlog to add course-run-v1.

@cpennington @clintonb I have to change the key type to course_key_v2 or just course_v2 because the CANONICAL_NAMESPACE of AggregateCourseLocator would be course-v2? Also should I change the name of aggregate course key and locator names too?

I have change the CANONICAL_NAMESPACE of AggregateCourseLocator to course-v2. Now the aggregate course key format would be course-v2:org+course

AggregateCourseKey.from_string('course-v2:edX+DemoX')

I thought we agreed to get rid of the word aggregate. I have no clue what it means in this context.

clintonb · 2017-03-07T12:54:09Z

opaque_keys/edx/locator.py

+
+        """
+        super(AggregateCourseLocator, self).__init__(
+            org=org,


Nit: one line

clintonb · 2017-03-07T12:55:00Z

opaque_keys/edx/locator.py

+        )
+
+        if self.org is None or self.course is None:
+            raise InvalidKeyError(self.__class__, 'Both org and course should be set.')


s/should/must

clintonb · 2017-03-07T12:56:02Z

opaque_keys/edx/locator.py

+    KEY_FIELDS = ('org', 'course')
+    __slots__ = KEY_FIELDS
+
+    URL_RE_SOURCE = r'^(?P<org>{ALLOWED_ID_CHARS}+)\+(?P<course>{ALLOWED_ID_CHARS}+)$'.format(


This doesn't need to be a separate variable. Combine it with URL_RE.

clintonb · 2017-03-07T12:57:32Z

opaque_keys/edx/locator.py

+        ALLOWED_ID_CHARS=Locator.ALLOWED_ID_CHARS
+    )
+
+    URL_RE = re.compile(URL_RE_SOURCE)


Give this a more appropriate name, KEY_REGEX.

clintonb · 2017-03-07T13:58:57Z

opaque_keys/edx/tests/test_locators.py

+        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))


The use of AggregateCourseKey.from_string is confusing and unnecessary.

course_key = CourseKey.from_string(course_id) expected = AggregateCourseKey(org=course_key.org, course=course_key.course) actual = AggregateCourseKey.from_course_key(course_key) assert expected == actual

clintonb · 2017-03-07T13:59:40Z

opaque_keys/edx/tests/test_locators.py

+        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):


test_serialize_to_string

clintonb · 2017-03-07T14:01:10Z

opaque_keys/edx/tests/test_locators.py

+
+    def test_aggregate_course_locator_serialize(self):
+        aggregate_course_key = 'aggregate-course:org+course'
+        aggregate_course_locator = AggregateCourseKey.from_string(aggregate_course_key)


You should not need to use AggregateCourseKey.from_string to instantiate a new instance of AggregateCourseKey. Fix this here, and in all other tests where it is used unnecessarily.

@clintonb @cpennington This seems to be the documented way if you try to initialize the AggregateCourseKey directly you get error:

>>> AggregateCourseKey(org=course_key.org, course=course_key.course) TypeError: Can't instantiate abstract class AggregateCourseKey with abstract methods _to_string, from_course_key

clintonb · 2017-03-07T14:02:35Z

opaque_keys/edx/locator.py

+            **kwargs
+        )
+
+        if self.org is None or self.course is None:


You need to account for empty string: if not (self.org and self.course)

clintonb · 2017-03-07T14:03:02Z

opaque_keys/edx/locator.py

+        """
+        Return a string representing this location.
+        """
+        return u'{org}+{course}'.format(


No need for the u.

coveralls · 2017-03-08T08:16:09Z

Coverage increased (+0.03%) to 96.53% when pulling 12086b5 on zub/ENT-211-add-aggregate-course-key into ffb2ee1 on master.

zubair-arbi · 2017-03-08T09:02:20Z

@cpennington @clintonb please give another review.

clintonb · 2017-03-09T03:51:20Z

opaque_keys/edx/tests/test_locators.py

+
+    def test_aggregate_course_locator(self):
+        """
+        Verify that the method "from_string" of class "AggregateCourseKey"


Use ` instead of " for Python code (classes, methods).

clintonb · 2017-03-09T03:52:49Z

opaque_keys/edx/keys.py

+    An :class:`opaque_keys.OpaqueKey` identifying a
+    serialized Course Key object.
+    """
+    KEY_TYPE = 'aggregate_course_key'


I thought we agreed to get rid of the word aggregate. I have no clue what it means in this context.

coveralls · 2017-03-09T11:22:32Z

Coverage increased (+0.07%) to 96.575% when pulling b1b4a1b on zub/ENT-211-add-aggregate-course-key into ffb2ee1 on master.

zubair-arbi · 2017-03-09T13:03:00Z

@clintonb @cpennington I have updated the naming. Please review.

clintonb · 2017-03-09T13:21:41Z

opaque_keys/edx/keys.py

+    __slots__ = ()
+
+    @abstractproperty
+    def from_course_key(self, course_key):  # pragma: no cover


Call this from_course_run_key. Eventually, course-key-v1 will be replaced with course-run-key-v1.

clintonb · 2017-03-09T13:22:35Z

opaque_keys/edx/tests/test_locators.py

+
+    def test_course_locator_v2(self):
+        """
+        Verify that the method "from_string" of class "CourseKeyV2"


Use ` instead of " to format Python entities. This should be fixed in all of your docstrings in this file.

clintonb · 2017-03-09T13:24:57Z

opaque_keys/edx/tests/test_locators.py

+        self.assertEqual(expected_course_locator, course_locator_v2)
+
+    @ddt.data(
+        'org/course/run',


There is no need for ddt in this instance. Whether we use the old or new format for CourseKey doesn't matter. The method simply expects an instance of CourseKey.

clintonb · 2017-03-09T13:29:27Z

opaque_keys/edx/tests/test_locators.py

+        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


No need to call the protected member. Use str(course_locator_v2).

clintonb · 2017-03-09T13:30:13Z

opaque_keys/edx/tests/test_locators.py

+        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')


This is unnecessary.

clintonb · 2017-03-09T13:30:56Z

opaque_keys/edx/tests/test_locators.py

+    Tests for :class:`.CourseLocatorV2`
+    """
+
+    def test_course_locator_v2(self):


test_from_string

clintonb · 2017-03-09T13:31:57Z

opaque_keys/edx/tests/test_locators.py

+        'org/course/run',
+        'course-v1:org+course+run',
+    )
+    def test_course_locator_v2_from_course_key(self, course_id):


You don't need to prefix your test methods with course_locator_v2. The fact that CourseLocatorV2 is given by the name of the test class, CourseLocatorV2Tests. This should simply be test_from_course_key.

clintonb · 2017-03-09T13:32:38Z

opaque_keys/edx/tests/test_locators.py

+        'org+course+run+foo',
+        'course-v2:org+course+run',
+    )
+    def test_invalid_format_course_key(self, course_key):


test_from_string_with_invalid_input

coveralls · 2017-03-10T08:14:23Z

Coverage increased (+0.03%) to 96.531% when pulling 7a7595a on zub/ENT-211-add-aggregate-course-key into ffb2ee1 on master.

…lementedError as library locators are course agnostic

coveralls · 2017-03-10T12:26:16Z

Coverage increased (+0.1%) to 96.611% when pulling 0fd82da on zub/ENT-211-add-aggregate-course-key into ffb2ee1 on master.

zubair-arbi · 2017-03-10T12:28:43Z

@clintonb Please have another look.
@cpennington I have added inter-conversion methods for course key and course key v2. I have updated the sample usage in the description of this PR. Since the LibraryLocator is course agnostic so the method make_course_key_v2 will just raise error NotImplementedError.

clintonb

I was ready to approve this if the issues highlighted yesterday had been resolved. Now the API has been expanded in a manner that is not necessarily in scope or desired.

It would be ideal to limit the co-mingling of key types to avoid deprecation/cleanup issues down the road. I advocate for removing these new methods. If we actually need them in the future, we can add them. Today, however, there is no use case for them that cannot be met by existing methods.

clintonb · 2017-03-10T14:32:45Z

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


Is this really necessary given that you already have CourseKeyV2.from_course_run_key()?

@cpennington @clintonb just to confirm, I have to remove this method only or do I need to remove the make_course_run_key in class CourseKeyV2 too?

@zubair-arbi: You should have some a method to go from CourseKeyV2 to CourseKey and back again, but only one of each. I would suggest using regular methods, rather than class methods, in general. So, probably CourseKeyV2.make_course_run_key, and CourseKey.make_course_key_v2.

clintonb · 2017-03-10T14:34:32Z

opaque_keys/edx/keys.py

+        raise NotImplementedError()
+
+    @abstractproperty
+    def make_course_run_key(self, course_run):  # pragma: no cover


Why is this needed? We already have existing methods of creating course run keys.

These were my suggestion. The existing keys typically are related by giving the more general key a way to create more specific keys by hydrating additional information.

That said, I agree that it doesn't make much sense to have both methods. We should pick one, and stick to it.

cpennington · 2017-03-13T18:48:16Z

I'm concerned about the names CourseKeyV2 and CourseKey. They are fundamentally different things, but that distinction isn't really captured in the names. I think it would be awesome to deprecate CourseKey and start encouraging the use of CourseRunKey and AggregateCourseKey (which are clearly different). "Course" is just too ambiguous right now, until a vast majority of our software switches to using CourseRun for course runs.

add aggregate course key

f72a1db

remove BlockUsageLocator from inheritance + update regex for the aggr…

11498a5

…egagte course key

add tests for aggregate course locator

b824779

zubair-arbi force-pushed the zub/ENT-211-add-aggregate-course-key branch from 5f565ee to b824779 Compare March 6, 2017 10:45

cpennington reviewed Mar 6, 2017

View reviewed changes

remove serialize method in favor of _to_string + add from_course_key …

1f24d58

…method to create aggregate course key from a course key object + add/update tests

zubair-arbi changed the title ~~WIP: ENT-211 add aggregate course key~~ ENT-211 add aggregate course key Mar 7, 2017

clintonb suggested changes Mar 7, 2017

View reviewed changes

address feedback from clinton, part 1

12086b5

zubair-arbi mentioned this pull request Mar 8, 2017

ENT-201: Add support for course run key in catalog contains endpoint openedx/course-discovery#721

Merged

clintonb reviewed Mar 9, 2017

View reviewed changes

change naming formate from aggregate course key to course key v2

b1b4a1b

zubair-arbi changed the title ~~ENT-211 add aggregate course key~~ ENT-211 add course key v2 Mar 9, 2017

clintonb suggested changes Mar 9, 2017

View reviewed changes

address feedback from clinton, part 2

7a7595a

zubair-arbi added 2 commits March 10, 2017 16:14

add inter conversion methods in course key and course key v2

b6f4b81

remove ddt from test_from_course_run_key

39e568f

add method "make_course_key_v2" in LibraryLocator which raises NotImp…

0fd82da

…lementedError as library locators are course agnostic

clintonb suggested changes Mar 10, 2017

View reviewed changes

ENT-211 add course key v2 #87

Are you sure you want to change the base?

ENT-211 add course key v2 #87

Conversation

zubair-arbi commented Mar 3, 2017 • edited Loading

coveralls commented Mar 3, 2017

zubair-arbi commented Mar 3, 2017

coveralls commented Mar 3, 2017

coveralls commented Mar 6, 2017

coveralls commented Mar 6, 2017

coveralls commented Mar 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 8, 2017

zubair-arbi commented Mar 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 9, 2017

zubair-arbi commented Mar 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 10, 2017

coveralls commented Mar 10, 2017

zubair-arbi commented Mar 10, 2017

clintonb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpennington commented Mar 13, 2017

zubair-arbi commented Mar 3, 2017 •

edited

Loading