-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: LibraryCollectionKey and LibraryCollectionLocator created [FC-0062] #335
Merged
bradenmacdonald
merged 9 commits into
openedx:master
from
open-craft:chris/FAL-3785-collections-key
Sep 4, 2024
Merged
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
6e23233
feat: CollectionKey and CollectionLocator created
ChrisChV 98ceab5
refactor: Rename CollectionKey to LibCollectionKey and CollectionLoca…
ChrisChV bc3b56b
refactor: Use a LibraryLocatorV2 to build a LibCollectionKey
ChrisChV 81de217
fix: Tests and coverage
ChrisChV e3ff649
refactor: Rename LibCollection* to LibraryCollection*. Change Library…
ChrisChV f2a904f
refactor: LibraryCollectionLocator to obtain org and lib slug from li…
ChrisChV ce74081
refactor: Update library_key property of LibraryCollectionKey
ChrisChV 97d46a5
chore: Update CHANGELOG.res and bump version
ChrisChV 780e4b2
refactor: Rename USAGE_ID_REGEXP to COLLECTION_ID_REGEXP in LibraryCo…
ChrisChV File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
""" | ||
Tests of LibraryCollectionLocator | ||
""" | ||
import ddt | ||
from opaque_keys import InvalidKeyError | ||
from opaque_keys.edx.tests import LocatorBaseTest | ||
from opaque_keys.edx.locator import LibraryCollectionLocator, LibraryLocatorV2 | ||
|
||
|
||
@ddt.ddt | ||
class TestLibraryCollectionLocator(LocatorBaseTest): | ||
""" | ||
Tests of :class:`.LibraryCollectionLocator` | ||
""" | ||
@ddt.data( | ||
"org/lib/id/foo", | ||
"org/lib/id", | ||
"org+lib+id", | ||
"org+lib+", | ||
"org+lib++id@library", | ||
"org+ne@t", | ||
"per%ent+sign", | ||
) | ||
def test_coll_key_from_invalid_string(self, coll_id_str): | ||
with self.assertRaises(InvalidKeyError): | ||
LibraryCollectionLocator.from_string(coll_id_str) | ||
|
||
def test_coll_key_constructor(self): | ||
org = 'TestX' | ||
lib = 'LibraryX' | ||
code = 'test-problem-bank' | ||
library_key = LibraryLocatorV2(org=org, slug=lib) | ||
coll_key = LibraryCollectionLocator(library_key=library_key, collection_id=code) | ||
library_key = coll_key.library_key | ||
self.assertEqual(str(coll_key), "lib-collection:TestX:LibraryX:test-problem-bank") | ||
self.assertEqual(coll_key.org, org) | ||
self.assertEqual(coll_key.collection_id, code) | ||
self.assertEqual(library_key.org, org) | ||
self.assertEqual(library_key.slug, lib) | ||
|
||
def test_coll_key_constructor_bad_ids(self): | ||
library_key = LibraryLocatorV2(org="TestX", slug="lib1") | ||
|
||
with self.assertRaises(ValueError): | ||
LibraryCollectionLocator(library_key=library_key, collection_id='usage-!@#{$%^&*}') | ||
with self.assertRaises(TypeError): | ||
LibraryCollectionLocator(library_key=None, collection_id='usage') | ||
|
||
def test_coll_key_from_string(self): | ||
org = 'TestX' | ||
lib = 'LibraryX' | ||
code = 'test-problem-bank' | ||
str_key = f"lib-collection:{org}:{lib}:{code}" | ||
coll_key = LibraryCollectionLocator.from_string(str_key) | ||
library_key = coll_key.library_key | ||
self.assertEqual(str(coll_key), str_key) | ||
self.assertEqual(coll_key.org, org) | ||
self.assertEqual(coll_key.collection_id, code) | ||
self.assertEqual(library_key.org, org) | ||
self.assertEqual(library_key.slug, lib) | ||
|
||
def test_coll_key_invalid_from_string(self): | ||
with self.assertRaises(InvalidKeyError): | ||
LibraryCollectionLocator.from_string("this-is-a-great-test") |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think a slug is fine, as long as we write it to the database–say, a
key
field onCollection
with a unique constraint on the combo of that and the LearningPackage (like how PublishableEntities work today).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, let's do that ^ @rpenido . Sorry for not realizing the need for a slug sooner. CC @pomegranited