feat: LibraryCollectionKey and LibraryCollectionLocator created [FC-0062] #335
Conversation
|
Thanks for the pull request, @ChrisChV! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:
When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
openedx-webhooks
added
the
open-source-contribution
ChrisChV
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #335 +/- ##
==========================================
+ Coverage 93.96% 93.98% +0.01%
==========================================
Files 29 30 +1
Lines 2850 2924 +74
Branches 640 649 +9
==========================================
+ Hits 2678 2748 +70
- Misses 145 148 +3
- Partials 27 28 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
opaque_keys/edx/locator.py
Outdated
| @@ -1620,3 +1621,55 @@ def html_id(self) -> str: | |||
| # HTML5 allows ID values to contain any characters at all other than spaces. | |||
| # These key types don't allow spaces either, so no transform is needed. | |||
| return str(self) | |||
|
|
|||
|
|
|||
| class CollectionLocator(CheckFieldMixin, CollectionKey): | |||
There was a problem hiding this comment.
If this is specific to libraries, it should be called LibCollectionLocator
opaque_keys/edx/keys.py
Outdated
| @@ -89,6 +89,36 @@ def make_asset_key(self, asset_type: str, path: str) -> AssetKey: # pragma: no | |||
| raise NotImplementedError() | |||
|
|
|||
|
|
|||
| class CollectionKey(LearningContextKey): | |||
There was a problem hiding this comment.
If this is specific to libraries, it should be called LibCollectionKey
|
@ormsbee I remember you had some concerns about introducing a new opaque key. Can you let us know your thoughts? The tagging API currently assumes that anything we apply tags to has an opaque key, so in order to allow tagging of Collections, we need to either implement this key or remove that assumption. |
@bradenmacdonald: This is the Slack thread where I asked the question about whether we really needed them (you, @kdmccormick, and @yusuf-musleh commented). It boils down to the fact that OpaqueKey machinery is weird and complex for what it does (even explaining to someone why there's a Also, it's plausible that we'd want to change a library's key at some point, which is more feasible to do now since we're not storing as many references to it. Keys in
I was under the impression that any sort of identifier would do, and you mention in the thread that UUIDs would work as well. Is it important that they be humanly readable? If it's just backend machinery at the moment, I'd prefer to use UUIDs for now. Or even the primary key of the thing being tagged if it's easy–since that would be the most compact representation. I know some other tagging libraries actually make a model-per-thing-being-tagged. In the longer term, I think that tagging functionality will be simpler and easier to adopt into other systems if it does not force a requirement for OpaqueKeys (e.g. forums posts/comments, catalog information, students, etc.). I would expect that All that being said, I don't know how much work any of this is. If making it more general threatens the Sumac timeline, I'm okay with rolling forward with a FYI @kdmccormick |
|
@ormsbee We can of course use UUIDs for the things that are being tagged. I was thinking that if we use primary keys, or perhaps even if we want to use UUIDs, we'd want some kind of namespacing so we have an idea of what sort of thing is being tagged and/or can avoid PK collisions. And if we did that ( However, I guess if we're strictly using UUIDs, then we don't technically need namespacing at all. It may be hard to debug what thing is actually tagged though, since we'll just see a random UUID and have no idea what type of thing it is. Thoughts? |
|
Oh right, I'm forgetting that there's a single global tagging table and that we need to namespace. Would it be terrible if we stored UUIDs and had a 1:1 join table that matched those rows with |
|
Here is the current ID field; it's just a string ID. Tagging is not part of the publishing cycle so doesn't distinguish between published and draft things. We want to associate that |
|
@bradenmacdonald: Please pardon the silly questions stemming from my ignorance of the data model, and my brain being stuck on components and not collections. If the same tag from the same taxonomy applies to three totally different types of things (e.g. a component, a collection, and a forum comment), does the data model have any way to differentiate them at the database layer? Or does it rely purely on the Edit: My sense is "no, there isn't", but while I don't think it's good to roll our own opaque-keys-lite and embed that knowledge into the |
That.
We haven't had any use case for basic filtering by type, although it could currently be done as a string prefix search on We could add another column that's a foreign key to the |
…tor to LibCollectionLocator
ChrisChV
force-pushed
the
chris/FAL-3785-collections-key
branch
from
a16a3ee to
98ceab5
Compare
|
Like I said before, I don't want to jeopardize the timeline over this, and I'm not going to block the merging of new key types. As a general rule, I would like more of our data model to be explicitly structured at the database layer, and not rely on Python code to introspect things (or having to know that key type X always starts with one of these two strings). I realize that to a certain extent we have to do that, because so much of our content is still in the ModuleStore and serialized to MongoDB, but I don't want it to be our long term direction. |
ChrisChV
changed the title
Also, having some kind of hierarchy (which the OpaqueKey has) could be handy. If we nuke a Library with all the Components and Collections, we can use the |
There was a problem hiding this comment.
LGTM 👍
Thank you for your work, @ChrisChV!
I only have a little to add here as I don't have much context with the OpaqueKeys.
- I tested this using the instructions from Tag in Collections [FC-0062] edx-platform#35383
- I read through the code
-
I checked for accessibility issues - Includes documentation
opaque_keys/edx/locator.py
Outdated
| CHECKED_INIT = False | ||
|
|
||
| # Allow usage IDs to contian unicode characters | ||
| USAGE_ID_REGEXP = re.compile(r'^[\w\-.]+$', flags=re.UNICODE) |
There was a problem hiding this comment.
This is the collection id (a number), but I'm unsure if we want to make it stricter here.
Maybe for clarity? What do you think?
There was a problem hiding this comment.
Wait, it's the ID? As in the primary key? We can't use that as part of the key if there's ever an intention to make it importable into a different instance.
There was a problem hiding this comment.
At the moment we don't have any other identifier for collections; we'll need to add either a UUID or a slug (or both). If a slug, it only needs to be unique within a library. I prefer slugs for readability. For now the slug can be auto-generated by slugifying the collection name and appending a number until it's unique.
There was a problem hiding this comment.
Yeah, I think a slug is fine, as long as we write it to the database–say, a key field on Collection with a unique constraint on the combo of that and the LearningPackage (like how PublishableEntities work today).
There was a problem hiding this comment.
Cool, let's do that ^ @rpenido . Sorry for not realizing the need for a slug sooner. CC @pomegranited
opaque_keys/edx/keys.py
Outdated
| @@ -89,6 +89,36 @@ def make_asset_key(self, asset_type: str, path: str) -> AssetKey: # pragma: no | |||
| raise NotImplementedError() | |||
|
|
|||
|
|
|||
| class LibCollectionKey(LearningContextKey): | |||
There was a problem hiding this comment.
| class LibCollectionKey(LearningContextKey): | |
| class LibraryCollectionKey(OpaqueKey): |
- My sense is that a collection is not a learning context. More concretely, I can't imagine us wanting to store learner state against a collection, the way that we store learner state against a CourseKey or (for previewing purposes) a LibraryKey. Would it cause any issue to simply subclass OpaqueKey directly?
- For the sake of the API, let's be consistent between "Lib" and "Library". We have
lib:in the serialized keys, so I like that you to serialize withlib-collection:. But, in the code, we spell outLibraryLocatorV2, so I'd prefer if you spelled outLibraryCollectionKeyandLibraryCollectionLocatoras well.
There was a problem hiding this comment.
Agreed that a collection is not a learning context and it should not subclass LearningContextKey. However, it can have a library_key field if that makes sense, since it does related to a library.
There was a problem hiding this comment.
@bradenmacdonald Sure, having a library_key field makes sense.
@ChrisChV Is there any reason we need both a field LibraryCollectionLocator.lib_key and an identical property LibraryCollectionLocator.library_key ? Could there just be a field named LibraryCollectionLocator.library_key ? Or does that play poorly with inheritance?
There was a problem hiding this comment.
I combined both fields here: ce74081
opaque_keys/edx/keys.py
Outdated
| @property | ||
| def context_key(self) -> LearningContextKey: # pragma: no cover | ||
| """ | ||
| Get the learning context key (LearningContextKey) for this XBlock usage. | ||
| """ | ||
| raise NotImplementedError() |
There was a problem hiding this comment.
It's not clear to me that a library is the "collection" of the context--I think that's mixing metaphors. Since we're making the assumption that a collection always belongs to a library, I would just be concrete and use the lib_key field that's already defined on your locator below. And, going along with my other request, I would spell it out as library_key.
| @property | |
| def context_key(self) -> LearningContextKey: # pragma: no cover | |
| """ | |
| Get the learning context key (LearningContextKey) for this XBlock usage. | |
| """ | |
| raise NotImplementedError() | |
| @property | |
| def library_key(self) -> LibraryLocatorV2: # pragma: no cover | |
| """ | |
| Get the key of the library that this collection belongs to. | |
| """ | |
| raise NotImplementedError() |
…CollectionKey to OpaqueKey
ChrisChV
changed the title
|
@kdmccormick Thanks for the review! I updated the PR with your suggestions |
There was a problem hiding this comment.
@ChrisChV Could you do a version bump and add a changelog entry?
Otherwise this looks great 👍
- I tested this while testing Tag in Collections [FC-0062] edx-platform#35383
- I read through the code
-
I checked for accessibility issuesN/A - Includes documentation
-
User-facing strings are extracted for translationN/A
ChrisChV
force-pushed
the
chris/FAL-3785-collections-key
branch
from
168c373 to
f2a904f
Compare
|
I think we just need to address @kdmccormick 's question about combining Please also include a version bump so it'll create a new package version. |
@bradenmacdonald Done! |
|
@bradenmacdonald It's ready for merge |
|
@ChrisChV 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
|
@bradenmacdonald I see the tag for this version, but the pip package is not updated |
|
@ChrisChV It seems like I needed to publish a "Release". I've done that now so you should see it on PyPI soon. |
|
Hi @kdmccormick Sorry for pinging you directly, but we're having trouble bumping this requirement in edx-platform, cf openedx/edx-platform#35383. It doesn't seem to be actually constrained in anyone else's What's the procedure for updating these fundamental packages? |
Description
Creates
LibCollectionKeyandLibCollectionLocatorSupporting information
Testing instructions