From ae7916b0d0adad73f94ec73c48914a9ad30e230c Mon Sep 17 00:00:00 2001 From: Kyle D McCormick Date: Wed, 18 Mar 2026 14:54:03 -0400 Subject: [PATCH] feat!: Collection.key -> Collection.collection_code Also, standardize internal usage of collection_key to collection_code. This helps clarify that Collection.key is *not* an OpaqueKey, but is rather a local slug, which can be combined with other identifiers to form a fully- qualified LibraryCollectionKey instance. BREAKING CHANGE: Collection.key has been renamed to Collection.collection_code. The underlying database field is still named _key. BREAKING CHANGE: Collection.collection_code now validates that its contents is a valid 'Slug' [A-Za-z0-9_-]. This was already effectively true, because LibraryCollectionKey can only be built with slug-like parts, but we now enforce it at the db level. Part of: https://github.com/openedx/openedx-core/issues/322 --- .../applets/backup_restore/zipper.py | 2 +- .../applets/collections/api.py | 34 +++++++-------- .../applets/collections/models.py | 18 ++++---- src/openedx_content/applets/components/api.py | 4 +- src/openedx_content/applets/publishing/api.py | 4 +- src/openedx_django_lib/fields.py | 28 +++++++++++++ .../applets/backup_restore/test_backup.py | 2 +- .../applets/collections/test_api.py | 42 +++++++++---------- .../applets/components/test_api.py | 6 +-- 9 files changed, 86 insertions(+), 54 deletions(-) diff --git a/src/openedx_content/applets/backup_restore/zipper.py b/src/openedx_content/applets/backup_restore/zipper.py index 1bb8bbba6..d7241a186 100644 --- a/src/openedx_content/applets/backup_restore/zipper.py +++ b/src/openedx_content/applets/backup_restore/zipper.py @@ -778,7 +778,7 @@ def _save_collections(self, learning_package, collections): ) collection = collections_api.add_to_collection( learning_package_id=learning_package.id, - key=collection.key, + collection_code=collection.key, entities_qset=publishing_api.get_publishable_entities(learning_package.id).filter(key__in=entities) ) diff --git a/src/openedx_content/applets/collections/api.py b/src/openedx_content/applets/collections/api.py index f23b6fbc5..0037c08c6 100644 --- a/src/openedx_content/applets/collections/api.py +++ b/src/openedx_content/applets/collections/api.py @@ -33,7 +33,7 @@ def create_collection( learning_package_id: int, - key: str, + collection_code: str, *, title: str, created_by: int | None, @@ -45,7 +45,7 @@ def create_collection( """ collection = Collection.objects.create( learning_package_id=learning_package_id, - key=key, + collection_code=collection_code, title=title, created_by_id=created_by, description=description, @@ -54,24 +54,24 @@ def create_collection( return collection -def get_collection(learning_package_id: int, collection_key: str) -> Collection: +def get_collection(learning_package_id: int, collection_code: str) -> Collection: """ Get a Collection by ID """ - return Collection.objects.get_by_key(learning_package_id, collection_key) + return Collection.objects.get_by_code(learning_package_id, collection_code) def update_collection( learning_package_id: int, - key: str, + collection_code: str, *, title: str | None = None, description: str | None = None, ) -> Collection: """ - Update a Collection identified by the learning_package_id + key. + Update a Collection identified by the learning_package_id + collection_code. """ - collection = get_collection(learning_package_id, key) + collection = get_collection(learning_package_id, collection_code) # If no changes were requested, there's nothing to update, so just return # the Collection as-is @@ -89,17 +89,17 @@ def update_collection( def delete_collection( learning_package_id: int, - key: str, + collection_code: str, *, hard_delete=False, ) -> Collection: """ - Disables or deletes a collection identified by the given learning_package + key. + Disables or deletes a collection identified by the given learning_package + collection_code. By default (hard_delete=False), the collection is "soft deleted", i.e disabled. Soft-deleted collections can be re-enabled using restore_collection. """ - collection = get_collection(learning_package_id, key) + collection = get_collection(learning_package_id, collection_code) if hard_delete: collection.delete() @@ -111,12 +111,12 @@ def delete_collection( def restore_collection( learning_package_id: int, - key: str, + collection_code: str, ) -> Collection: """ Undo a "soft delete" by re-enabling a Collection. """ - collection = get_collection(learning_package_id, key) + collection = get_collection(learning_package_id, collection_code) collection.enabled = True collection.save() @@ -125,7 +125,7 @@ def restore_collection( def add_to_collection( learning_package_id: int, - key: str, + collection_code: str, entities_qset: QuerySet[PublishableEntity], created_by: int | None = None, ) -> Collection: @@ -145,10 +145,10 @@ def add_to_collection( if invalid_entity: raise ValidationError( f"Cannot add entity {invalid_entity.pk} in learning package {invalid_entity.learning_package_id} " - f"to collection {key} in learning package {learning_package_id}." + f"to collection {collection_code} in learning package {learning_package_id}." ) - collection = get_collection(learning_package_id, key) + collection = get_collection(learning_package_id, collection_code) collection.entities.add( *entities_qset.all(), through_defaults={"created_by_id": created_by}, @@ -161,7 +161,7 @@ def add_to_collection( def remove_from_collection( learning_package_id: int, - key: str, + collection_code: str, entities_qset: QuerySet[PublishableEntity], ) -> Collection: """ @@ -173,7 +173,7 @@ def remove_from_collection( Returns the updated Collection. """ - collection = get_collection(learning_package_id, key) + collection = get_collection(learning_package_id, collection_code) collection.entities.remove(*entities_qset.all()) collection.modified = datetime.now(tz=timezone.utc) diff --git a/src/openedx_content/applets/collections/models.py b/src/openedx_content/applets/collections/models.py index 2f315b247..b431d6755 100644 --- a/src/openedx_content/applets/collections/models.py +++ b/src/openedx_content/applets/collections/models.py @@ -70,7 +70,9 @@ from django.db import models from django.utils.translation import gettext_lazy as _ -from openedx_django_lib.fields import MultiCollationTextField, case_insensitive_char_field, key_field +from openedx_django_lib.fields import ( + MultiCollationTextField, case_insensitive_char_field, case_sensitive_slug_field +) from openedx_django_lib.validators import validate_utc_datetime from ..publishing.models import LearningPackage, PublishableEntity @@ -85,12 +87,12 @@ class CollectionManager(models.Manager): """ Custom manager for Collection class. """ - def get_by_key(self, learning_package_id: int, key: str): + def get_by_code(self, learning_package_id: int, collection_code: str): """ - Get the Collection for the given Learning Package + key. + Get the Collection for the given Learning Package + collection code. """ return self.select_related('learning_package') \ - .get(learning_package_id=learning_package_id, key=key) + .get(learning_package_id=learning_package_id, collection_code=collection_code) class Collection(models.Model): @@ -105,10 +107,12 @@ class Collection(models.Model): learning_package = models.ForeignKey(LearningPackage, on_delete=models.CASCADE) # Every collection is uniquely and permanently identified within its learning package - # by a 'key' that is set during creation. Both will appear in the + # by a 'code' that is set during creation. Both will appear in the # collection's opaque key: - # e.g. "lib-collection:lib:key" is the opaque key for a library collection. - key = key_field(db_column='_key') + # e.g. "lib-collection:{org_code}:{library_code}:{collection_code}" + # is the opaque key for a library collection. + # Formerly known as the "key", hence the column name. + code = case_sensitive_slug_field(db_column='_key') title = case_insensitive_char_field( null=False, diff --git a/src/openedx_content/applets/components/api.py b/src/openedx_content/applets/components/api.py index 46de5356e..e7c4bf8c8 100644 --- a/src/openedx_content/applets/components/api.py +++ b/src/openedx_content/applets/components/api.py @@ -436,7 +436,7 @@ def get_components( # pylint: disable=too-many-positional-arguments def get_collection_components( learning_package_id: int, - collection_key: str, + collection_code: str, ) -> QuerySet[Component]: """ Returns a QuerySet of Components relating to the PublishableEntities in a Collection. @@ -445,7 +445,7 @@ def get_collection_components( """ return Component.objects.filter( learning_package_id=learning_package_id, - publishable_entity__collections__key=collection_key, + publishable_entity__collections__collection_code=collection_code, ).order_by('pk') diff --git a/src/openedx_content/applets/publishing/api.py b/src/openedx_content/applets/publishing/api.py index 64b190a09..e0f10268c 100644 --- a/src/openedx_content/applets/publishing/api.py +++ b/src/openedx_content/applets/publishing/api.py @@ -1745,7 +1745,7 @@ def get_containers( def get_collection_containers( learning_package_id: int, - collection_key: str, + collection_code: str, ) -> QuerySet[Container]: """ Returns a QuerySet of Containers relating to the PublishableEntities in a Collection. @@ -1754,7 +1754,7 @@ def get_collection_containers( """ return Container.objects.filter( publishable_entity__learning_package_id=learning_package_id, - publishable_entity__collections__key=collection_key, + publishable_entity__collections__collection_code=collection_code, ).order_by('pk') diff --git a/src/openedx_django_lib/fields.py b/src/openedx_django_lib/fields.py index c41678993..7d401b7e9 100644 --- a/src/openedx_django_lib/fields.py +++ b/src/openedx_django_lib/fields.py @@ -92,6 +92,26 @@ def case_sensitive_char_field(**kwargs) -> MultiCollationCharField: return MultiCollationCharField(**final_kwargs) +def case_sensitive_slug_field(**kwargs) -> MultiCollationCharField: + """ + Return a case-sensitive ``MultiCollationSlugField``. + + See `case_sensitive_slug_field`. + """ + # Set our default arguments + final_kwargs = { + "null": False, + "db_collations": { + "sqlite": "BINARY", + "mysql": "utf8mb4_bin", + }, + } + # Override our defaults with whatever is passed in. + final_kwargs.update(kwargs) + + return MultiCollationSlugField(**final_kwargs) + + def immutable_uuid_field() -> models.UUIDField: """ Stable, randomly-generated UUIDs. @@ -198,6 +218,14 @@ class MultiCollationCharField(MultiCollationMixin, models.CharField): """ +class MultiCollationSlugField(MultiCollationMixin, models.SlugField): + """ + SlugField subclass with per-database-vendor collation settings. + + See `MultiCollationCharField`. + """ + + class MultiCollationTextField(MultiCollationMixin, models.TextField): """ TextField subclass with per-database-vendor collation settings. diff --git a/tests/openedx_content/applets/backup_restore/test_backup.py b/tests/openedx_content/applets/backup_restore/test_backup.py index 741753950..36753cc98 100644 --- a/tests/openedx_content/applets/backup_restore/test_backup.py +++ b/tests/openedx_content/applets/backup_restore/test_backup.py @@ -146,7 +146,7 @@ def setUpTestData(cls): cls.collection = api.create_collection( cls.learning_package.id, - key="COL1", + collection_code="COL1", created_by=cls.user.id, title="Collection 1", description="Description of Collection 1", diff --git a/tests/openedx_content/applets/collections/test_api.py b/tests/openedx_content/applets/collections/test_api.py index faf6352aa..bd34520b4 100644 --- a/tests/openedx_content/applets/collections/test_api.py +++ b/tests/openedx_content/applets/collections/test_api.py @@ -63,35 +63,35 @@ def setUpTestData(cls) -> None: super().setUpTestData() cls.collection1 = api.create_collection( cls.learning_package.id, - key="COL1", + collection_code="COL1", created_by=None, title="Collection 1", description="Description of Collection 1", ) cls.collection2 = api.create_collection( cls.learning_package.id, - key="COL2", + collection_code="COL2", created_by=None, title="Collection 2", description="Description of Collection 2", ) cls.collection3 = api.create_collection( cls.learning_package.id, - key="COL3", + collection_code="COL3", created_by=None, title="Collection 3", description="Description of Collection 3", ) cls.another_library_collection = api.create_collection( cls.learning_package_2.id, - key="another_library", + collection_code="another_library", created_by=None, title="Collection 4", description="Description of Collection 4", ) cls.disabled_collection = api.create_collection( cls.learning_package.id, - key="disabled_collection", + collection_code="disabled_collection", created_by=None, title="Disabled Collection", description="Description of Disabled Collection", @@ -190,7 +190,7 @@ def test_create_collection(self): with freeze_time(created_time): collection = api.create_collection( self.learning_package.id, - key='MYCOL', + collection_code='MYCOL', title="My Collection", created_by=user.id, description="This is my collection", @@ -210,7 +210,7 @@ def test_create_collection_without_description(self): """ collection = api.create_collection( self.learning_package.id, - key='MYCOL', + collection_code='MYCOL', created_by=None, title="My Collection", ) @@ -281,7 +281,7 @@ def setUpTestData(cls) -> None: # Add some shared components to the collections cls.collection1 = api.add_to_collection( cls.learning_package.id, - key=cls.collection1.key, + collection_code=cls.collection1.key, entities_qset=PublishableEntity.objects.filter(id__in=[ cls.published_component.pk, ]), @@ -289,7 +289,7 @@ def setUpTestData(cls) -> None: ) cls.collection2 = api.add_to_collection( cls.learning_package.id, - key=cls.collection2.key, + collection_code=cls.collection2.key, entities_qset=PublishableEntity.objects.filter(id__in=[ cls.published_component.pk, cls.draft_component.pk, @@ -298,7 +298,7 @@ def setUpTestData(cls) -> None: ) cls.disabled_collection = api.add_to_collection( cls.learning_package.id, - key=cls.disabled_collection.key, + collection_code=cls.disabled_collection.key, entities_qset=PublishableEntity.objects.filter(id__in=[ cls.published_component.pk, ]), @@ -470,7 +470,7 @@ def setUpTestData(cls) -> None: super().setUpTestData() cls.collection = api.create_collection( cls.learning_package.id, - key="MYCOL", + collection_code="MYCOL", title="Collection", created_by=None, description="Description of Collection", @@ -484,7 +484,7 @@ def test_update_collection(self): with freeze_time(modified_time): collection = api.update_collection( self.learning_package.id, - key=self.collection.key, + collection_code=self.collection.key, title="New Title", description="", ) @@ -500,7 +500,7 @@ def test_update_collection_partial(self): """ collection = api.update_collection( self.learning_package.id, - key=self.collection.key, + collection_code=self.collection.key, title="New Title", ) @@ -510,7 +510,7 @@ def test_update_collection_partial(self): collection = api.update_collection( self.learning_package.id, - key=self.collection.key, + collection_code=self.collection.key, description="New description", ) @@ -525,7 +525,7 @@ def test_update_collection_empty(self): with freeze_time(modified_time): collection = api.update_collection( self.learning_package.id, - key=self.collection.key, + collection_code=self.collection.key, ) assert collection.title == self.collection.title # unchanged @@ -539,7 +539,7 @@ def test_update_collection_not_found(self): with self.assertRaises(ObjectDoesNotExist): api.update_collection( self.learning_package.id, - key="12345", + collection_code="12345", title="New Title", ) @@ -557,7 +557,7 @@ def test_soft_delete(self): with freeze_time(modified_time): collection = api.delete_collection( self.learning_package.id, - key=self.collection2.key, + collection_code=self.collection2.key, ) # Collection was disabled and still exists in the database @@ -579,7 +579,7 @@ def test_delete(self): with freeze_time(modified_time): collection = api.delete_collection( self.learning_package.id, - key=self.collection2.key, + collection_code=self.collection2.key, hard_delete=True, ) @@ -604,14 +604,14 @@ def test_restore(self): """ collection = api.delete_collection( self.learning_package.id, - key=self.collection2.key, + collection_code=self.collection2.key, ) modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc) with freeze_time(modified_time): collection = api.restore_collection( self.learning_package.id, - key=self.collection2.key, + collection_code=self.collection2.key, ) # Collection was enabled and still exists in the database @@ -708,7 +708,7 @@ def test_set_collection_wrong_learning_package(self): ) collection = api.create_collection( learning_package_3.id, - key="MYCOL", + collection_code="MYCOL", title="My Collection", created_by=None, description="Description of Collection", diff --git a/tests/openedx_content/applets/components/test_api.py b/tests/openedx_content/applets/components/test_api.py index 759aa25dc..01bdcebf8 100644 --- a/tests/openedx_content/applets/components/test_api.py +++ b/tests/openedx_content/applets/components/test_api.py @@ -653,21 +653,21 @@ def setUpTestData(cls) -> None: ) cls.collection1 = collection_api.create_collection( cls.learning_package.id, - key="MYCOL1", + collection_code="MYCOL1", title="Collection1", created_by=None, description="Description of Collection 1", ) cls.collection2 = collection_api.create_collection( cls.learning_package.id, - key="MYCOL2", + collection_code="MYCOL2", title="Collection2", created_by=None, description="Description of Collection 2", ) cls.collection3 = collection_api.create_collection( cls.learning_package.id, - key="MYCOL3", + collection_code="MYCOL3", title="Collection3", created_by=None, description="Description of Collection 3",