CMS changes for opaque keys [partial commit] - #3537
Conversation
There was a problem hiding this comment.
@dmitchell Who should look at this comment:
"DO NOT MERGE: need to figure out how to handle the metadata inheritance refresh"
There was a problem hiding this comment.
It was redundant w/ a call in the modulestore; so, I removed it.
|
I'm not able to get the studio up and running with this on devstack. I keep getting: I don't see that file existing in this branch. Is there something I'm missing? |
|
@carsongee are you using this branch or the consolidated opaque-keys branch? This branch is purely for review. Use the opaque-keys branch for testing |
|
Thanks @dmitchell. I have noticed that while not in this diff, there is a side affect not addressed. When attempting to import a course over an existing legacy one it is getting skipped as a duplicate because of code I had added in https://github.com/edx/edx-platform/pull/3145 as a workaround to some of the issues that this branch will hopefully fix. I'm guess that can either be removed here or needs some adapting. |
There was a problem hiding this comment.
@dmitchell Is this call to modulestore() correct (getting the default from the django settings)? Or should it call get_modulestore?
There was a problem hiding this comment.
It depends on what "usage_key" can refer to. However, I don't think there is ever any harm to calling get_modulestore to make sure you get the correct one.
There was a problem hiding this comment.
Changed. I wonder how the old worked.
|
@carsongee https://edx-wiki.atlassian.net/browse/STUD-1535 is my main long-term goal, but it looks like it will be august before it lands. All of this refactoring is in preparation for that (removing all of the logic which knows how to interpret keys and such). I don't recall whether we pointed out https://github.com/edx/edx-platform/wiki/Opaque-Keys and the developer notes it references to you. |
|
Yeah, @sarina sent me the wiki page, and it is going to be quite the undertaking. Since this code is specifically designed to remove the need for my code in that PR, I would suggest removing it from this branch. |
There was a problem hiding this comment.
@dmitchell @cpennington I don't know (currently) how mongo indexes work. Someone else should review this file in the CMS branch.
There was a problem hiding this comment.
Well, I wrote the file; so, my review is 👍 :-)
This commit adds base classes for CourseKeys and UsageKeys, and Location and
SlashSeparatedCourseKey implementations of both.
These keys are now objects with a limited interface, and the particular
internal representation is managed by the data storage layer (the
modulestore).
For the LMS, there should be no outward-facing changes to the system. The keys
are, for now, a change to internal representation only. For Studio, the new
serialized form of the keys is used in urls, to allow for further migration in
the future.
Co-Author: Andy Armstrong andya@edx.org Co-Author: Christina Roberts
christina@edx.org Co-Author: David Baumgold db@edx.org Co-Author: Diana
Huang dkh@edx.org Co-Author: Don Mitchell dmitchell@edx.org Co-Author:
Julia Hansbrough julia@edx.org Co-Author: Nimisha Asthagiri
nasthagiri@edx.org Co-Author: Sarina Canelake sarina@edx.org
[LMS-2370]