diff --git a/openedx_learning/__init__.py b/openedx_learning/__init__.py index 79f39989..e8a025da 100644 --- a/openedx_learning/__init__.py +++ b/openedx_learning/__init__.py @@ -1,4 +1,4 @@ """ Open edX Learning ("Learning Core"). """ -__version__ = "0.11.3" +__version__ = "0.11.4" diff --git a/openedx_learning/apps/authoring/collections/api.py b/openedx_learning/apps/authoring/collections/api.py index 03988922..1b366b5a 100644 --- a/openedx_learning/apps/authoring/collections/api.py +++ b/openedx_learning/apps/authoring/collections/api.py @@ -30,6 +30,8 @@ def create_collection( learning_package_id: int, + key: str, + *, title: str, created_by: int | None, description: str = "", @@ -40,6 +42,7 @@ def create_collection( """ collection = Collection.objects.create( learning_package_id=learning_package_id, + key=key, title=title, created_by_id=created_by, description=description, @@ -48,22 +51,24 @@ def create_collection( return collection -def get_collection(collection_id: int) -> Collection: +def get_collection(learning_package_id: int, collection_key: str) -> Collection: """ Get a Collection by ID """ - return Collection.objects.get(id=collection_id) + return Collection.objects.get_by_key(learning_package_id, collection_key) def update_collection( - collection_id: int, + learning_package_id: int, + key: str, + *, title: str | None = None, description: str | None = None, ) -> Collection: """ - Update a Collection + Update a Collection identified by the learning_package_id + key. """ - collection = Collection.objects.get(id=collection_id) + collection = get_collection(learning_package_id, key) # If no changes were requested, there's nothing to update, so just return # the Collection as-is @@ -80,7 +85,8 @@ def update_collection( def add_to_collection( - collection_id: int, + learning_package_id: int, + key: str, entities_qset: QuerySet[PublishableEntity], created_by: int | None = None, ) -> Collection: @@ -95,17 +101,15 @@ def add_to_collection( Returns the updated Collection object. """ - collection = get_collection(collection_id) - learning_package_id = collection.learning_package_id - # Disallow adding entities outside the collection's learning package invalid_entity = entities_qset.exclude(learning_package_id=learning_package_id).first() if invalid_entity: raise ValidationError( f"Cannot add entity {invalid_entity.pk} in learning package {invalid_entity.learning_package_id} " - f"to collection {collection_id} in learning package {learning_package_id}." + f"to collection {key} in learning package {learning_package_id}." ) + collection = get_collection(learning_package_id, key) collection.entities.add( *entities_qset.all(), through_defaults={"created_by_id": created_by}, @@ -117,7 +121,8 @@ def add_to_collection( def remove_from_collection( - collection_id: int, + learning_package_id: int, + key: str, entities_qset: QuerySet[PublishableEntity], ) -> Collection: """ @@ -129,7 +134,7 @@ def remove_from_collection( Returns the updated Collection. """ - collection = get_collection(collection_id) + collection = get_collection(learning_package_id, key) collection.entities.remove(*entities_qset.all()) collection.modified = datetime.now(tz=timezone.utc) diff --git a/openedx_learning/apps/authoring/collections/migrations/0004_collection_key.py b/openedx_learning/apps/authoring/collections/migrations/0004_collection_key.py new file mode 100644 index 00000000..843419ee --- /dev/null +++ b/openedx_learning/apps/authoring/collections/migrations/0004_collection_key.py @@ -0,0 +1,57 @@ +# Generated by Django 4.2.15 on 2024-09-04 23:15 + +from django.db import migrations, models +from django.utils.crypto import get_random_string + +import openedx_learning.lib.fields + + +def generate_keys(apps, schema_editor): + """ + Generates a random strings to initialize the key field where NULL. + """ + length = 50 + Collection = apps.get_model("oel_collections", "Collection") + for collection in Collection.objects.filter(key=None): + # Keep generating keys until we get a unique one + key = get_random_string(length) + while Collection.objects.filter(key=key).exists(): + key = get_random_string(length) + + collection.key = key + collection.save() + + +class Migration(migrations.Migration): + + dependencies = [ + ('oel_collections', '0003_collection_entities'), + ] + + operations = [ + # 1. Temporarily add this field with null=True, blank=True + migrations.AddField( + model_name='collection', + name='key', + field=openedx_learning.lib.fields.MultiCollationCharField( + db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, + db_column='_key', max_length=500, null=True, blank=True), + preserve_default=False, + ), + # 2. Populate the null keys + migrations.RunPython(generate_keys), + # 3. Add null=False, blank=False to disallow NULL values + migrations.AlterField( + model_name='collection', + name='key', + field=openedx_learning.lib.fields.MultiCollationCharField( + db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, + db_column='_key', max_length=500, null=False, blank=False), + preserve_default=False, + ), + # 4. Enforce unique constraint + migrations.AddConstraint( + model_name='collection', + constraint=models.UniqueConstraint(fields=('learning_package', 'key'), name='oel_coll_uniq_lp_key'), + ), + ] diff --git a/openedx_learning/apps/authoring/collections/models.py b/openedx_learning/apps/authoring/collections/models.py index c67e6c26..24adce69 100644 --- a/openedx_learning/apps/authoring/collections/models.py +++ b/openedx_learning/apps/authoring/collections/models.py @@ -70,8 +70,9 @@ from django.db import models from django.utils.translation import gettext_lazy as _ -from ....lib.fields import MultiCollationTextField, case_insensitive_char_field -from ....lib.validators import validate_utc_datetime +from openedx_learning.lib.fields import MultiCollationTextField, case_insensitive_char_field, key_field +from openedx_learning.lib.validators import validate_utc_datetime + from ..publishing.models import LearningPackage, PublishableEntity __all__ = [ @@ -80,16 +81,35 @@ ] +class CollectionManager(models.Manager): + """ + Custom manager for Collection class. + """ + def get_by_key(self, learning_package_id: int, key: str): + """ + Get the Collection for the given Learning Package + key. + """ + return self.select_related('learning_package') \ + .get(learning_package_id=learning_package_id, key=key) + + class Collection(models.Model): """ Represents a collection of library components """ + objects: CollectionManager[Collection] = CollectionManager() id = models.AutoField(primary_key=True) # Each collection belongs to a learning package 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 + # collection's opaque key: + # e.g. "lib-collection:lib:key" is the opaque key for a library collection. + key = key_field(db_column='_key') + title = case_insensitive_char_field( null=False, blank=False, @@ -151,6 +171,16 @@ class Collection(models.Model): class Meta: verbose_name_plural = "Collections" + constraints = [ + # Keys are unique within a given LearningPackage. + models.UniqueConstraint( + fields=[ + "learning_package", + "key", + ], + name="oel_coll_uniq_lp_key", + ), + ] indexes = [ models.Index(fields=["learning_package", "title"]), ] @@ -165,7 +195,7 @@ def __str__(self) -> str: """ User-facing string representation of a Collection. """ - return f"<{self.__class__.__name__}> ({self.id}:{self.title})" + return f"<{self.__class__.__name__}> (lp:{self.learning_package_id} {self.key}:{self.title})" class CollectionPublishableEntity(models.Model): diff --git a/tests/openedx_learning/apps/authoring/collections/test_api.py b/tests/openedx_learning/apps/authoring/collections/test_api.py index 3d7f299e..8407537c 100644 --- a/tests/openedx_learning/apps/authoring/collections/test_api.py +++ b/tests/openedx_learning/apps/authoring/collections/test_api.py @@ -59,24 +59,28 @@ def setUpTestData(cls) -> None: super().setUpTestData() cls.collection1 = api.create_collection( cls.learning_package.id, + key="COL1", created_by=None, title="Collection 1", description="Description of Collection 1", ) cls.collection2 = api.create_collection( cls.learning_package.id, + key="COL2", created_by=None, title="Collection 2", description="Description of Collection 2", ) cls.collection3 = api.create_collection( cls.learning_package_2.id, + key="COL3", created_by=None, title="Collection 3", description="Description of Collection 3", ) cls.disabled_collection = api.create_collection( cls.learning_package.id, + key="COL4", created_by=None, title="Disabled Collection", description="Description of Disabled Collection", @@ -92,7 +96,7 @@ def test_get_collection(self): """ Test getting a single collection. """ - collection = api.get_collection(self.collection1.pk) + collection = api.get_collection(self.learning_package.pk, 'COL1') assert collection == self.collection1 def test_get_collection_not_found(self): @@ -100,7 +104,14 @@ def test_get_collection_not_found(self): Test getting a collection that doesn't exist. """ with self.assertRaises(ObjectDoesNotExist): - api.get_collection(12345) + api.get_collection(self.learning_package.pk, '12345') + + def test_get_collection_wrong_learning_package(self): + """ + Test getting a collection that doesn't exist in the requested learning package. + """ + with self.assertRaises(ObjectDoesNotExist): + api.get_collection(self.learning_package.pk, self.collection3.key) def test_get_collections(self): """ @@ -165,12 +176,14 @@ def test_create_collection(self): with freeze_time(created_time): collection = api.create_collection( self.learning_package.id, + key='MYCOL', title="My Collection", created_by=user.id, description="This is my collection", ) assert collection.title == "My Collection" + assert collection.key == "MYCOL" assert collection.description == "This is my collection" assert collection.enabled assert collection.created == created_time @@ -183,10 +196,12 @@ def test_create_collection_without_description(self): """ collection = api.create_collection( self.learning_package.id, + key='MYCOL', created_by=None, title="My Collection", ) assert collection.title == "My Collection" + assert collection.key == "MYCOL" assert collection.description == "" assert collection.enabled @@ -250,21 +265,24 @@ def setUpTestData(cls) -> None: # Add some shared entities to the collections cls.collection1 = api.add_to_collection( - cls.collection1.id, + cls.learning_package.id, + key=cls.collection1.key, entities_qset=PublishableEntity.objects.filter(id__in=[ cls.published_entity.id, ]), created_by=cls.user.id, ) cls.collection2 = api.add_to_collection( - cls.collection2.id, + cls.learning_package.id, + key=cls.collection2.key, entities_qset=PublishableEntity.objects.filter(id__in=[ cls.published_entity.id, cls.draft_entity.id, ]), ) cls.disabled_collection = api.add_to_collection( - cls.disabled_collection.id, + cls.learning_package.id, + key=cls.disabled_collection.key, entities_qset=PublishableEntity.objects.filter(id__in=[ cls.published_entity.id, ]), @@ -290,7 +308,8 @@ def test_add_to_collection(self): modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc) with freeze_time(modified_time): self.collection1 = api.add_to_collection( - self.collection1.id, + self.learning_package.id, + self.collection1.key, PublishableEntity.objects.filter(id__in=[ self.draft_entity.id, ]), @@ -312,7 +331,8 @@ def test_add_to_collection_again(self): modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc) with freeze_time(modified_time): self.collection2 = api.add_to_collection( - self.collection2.id, + self.learning_package.id, + self.collection2.key, PublishableEntity.objects.filter(id__in=[ self.published_entity.id, ]), @@ -330,7 +350,8 @@ def test_add_to_collection_wrong_learning_package(self): """ with self.assertRaises(ValidationError): api.add_to_collection( - self.collection3.id, + self.learning_package_2.id, + self.collection3.key, PublishableEntity.objects.filter(id__in=[ self.published_entity.id, ]), @@ -345,7 +366,8 @@ def test_remove_from_collection(self): modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc) with freeze_time(modified_time): self.collection2 = api.remove_from_collection( - self.collection2.id, + self.learning_package.id, + self.collection2.key, PublishableEntity.objects.filter(id__in=[ self.published_entity.id, ]), @@ -376,13 +398,15 @@ class UpdateCollectionTestCase(CollectionTestCase): """ collection: Collection - def setUp(self) -> None: + @classmethod + def setUpTestData(cls) -> None: """ Initialize our content data """ - super().setUp() - self.collection = api.create_collection( - self.learning_package.id, + super().setUpTestData() + cls.collection = api.create_collection( + cls.learning_package.id, + key="MYCOL", title="Collection", created_by=None, description="Description of Collection", @@ -395,7 +419,8 @@ def test_update_collection(self): modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc) with freeze_time(modified_time): collection = api.update_collection( - self.collection.pk, + self.learning_package.id, + key=self.collection.key, title="New Title", description="", ) @@ -410,16 +435,18 @@ def test_update_collection_partial(self): Test updating a collection's title. """ collection = api.update_collection( - self.collection.pk, + self.learning_package.id, + key=self.collection.key, title="New Title", ) assert collection.title == "New Title" assert collection.description == self.collection.description # unchanged - assert f"{collection}" == f" ({self.collection.pk}:New Title)" + assert f"{collection}" == f" (lp:{self.learning_package.id} {self.collection.key}:New Title)" collection = api.update_collection( - self.collection.pk, + self.learning_package.id, + key=self.collection.key, description="New description", ) @@ -433,7 +460,8 @@ def test_update_collection_empty(self): modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc) with freeze_time(modified_time): collection = api.update_collection( - self.collection.pk, + self.learning_package.id, + key=self.collection.key, ) assert collection.title == self.collection.title # unchanged @@ -445,4 +473,8 @@ def test_update_collection_not_found(self): Test updating a collection that doesn't exist. """ with self.assertRaises(ObjectDoesNotExist): - api.update_collection(12345, title="New Title") + api.update_collection( + self.learning_package.id, + key="12345", + title="New Title", + )