diff --git a/openedx_learning/__init__.py b/openedx_learning/__init__.py index cf87e3bf..79f39989 100644 --- a/openedx_learning/__init__.py +++ b/openedx_learning/__init__.py @@ -1,4 +1,4 @@ """ Open edX Learning ("Learning Core"). """ -__version__ = "0.11.2" +__version__ = "0.11.3" diff --git a/openedx_learning/apps/authoring/collections/api.py b/openedx_learning/apps/authoring/collections/api.py index 942903b7..03988922 100644 --- a/openedx_learning/apps/authoring/collections/api.py +++ b/openedx_learning/apps/authoring/collections/api.py @@ -3,8 +3,13 @@ """ from __future__ import annotations +from datetime import datetime, timezone + +from django.core.exceptions import ValidationError from django.db.models import QuerySet +from ..publishing import api as publishing_api +from ..publishing.models import PublishableEntity from .models import Collection # The public API that will be re-exported by openedx_learning.apps.authoring.api @@ -13,10 +18,12 @@ # start with an underscore AND it is not in __all__, that function is considered # to be callable only by other apps in the authoring package. __all__ = [ + "add_to_collection", "create_collection", "get_collection", "get_collections", - "get_learning_package_collections", + "get_entity_collections", + "remove_from_collection", "update_collection", ] @@ -26,6 +33,7 @@ def create_collection( title: str, created_by: int | None, description: str = "", + enabled: bool = True, ) -> Collection: """ Create a new Collection @@ -35,6 +43,7 @@ def create_collection( title=title, created_by_id=created_by, description=description, + enabled=enabled, ) return collection @@ -70,23 +79,85 @@ def update_collection( return collection -def get_learning_package_collections(learning_package_id: int) -> QuerySet[Collection]: +def add_to_collection( + collection_id: int, + entities_qset: QuerySet[PublishableEntity], + created_by: int | None = None, +) -> Collection: """ - Get all collections for a given learning package + Adds a QuerySet of PublishableEntities to a Collection. + + These Entities must belong to the same LearningPackage as the Collection, or a ValidationError will be raised. + + PublishableEntities already in the Collection are silently ignored. + + The Collection object's modified date is updated. + + 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}." + ) + + collection.entities.add( + *entities_qset.all(), + through_defaults={"created_by_id": created_by}, + ) + collection.modified = datetime.now(tz=timezone.utc) + collection.save() + + return collection + + +def remove_from_collection( + collection_id: int, + entities_qset: QuerySet[PublishableEntity], +) -> Collection: + """ + Removes a QuerySet of PublishableEntities from a Collection. + + PublishableEntities are deleted (in bulk). + + The Collection's modified date is updated (even if nothing was removed). + + Returns the updated Collection. + """ + collection = get_collection(collection_id) + + collection.entities.remove(*entities_qset.all()) + collection.modified = datetime.now(tz=timezone.utc) + collection.save() - Only enabled collections are returned + return collection + + +def get_entity_collections(learning_package_id: int, entity_key: str) -> QuerySet[Collection]: + """ + Get all collections in the given learning package which contain this entity. + + Only enabled collections are returned. """ - return Collection.objects \ - .filter(learning_package_id=learning_package_id, enabled=True) \ - .select_related("learning_package") \ - .order_by('pk') + entity = publishing_api.get_publishable_entity_by_key( + learning_package_id, + key=entity_key, + ) + return entity.collections.filter(enabled=True).order_by("pk") -def get_collections(enabled: bool | None = None) -> QuerySet[Collection]: +def get_collections(learning_package_id: int, enabled: bool | None = True) -> QuerySet[Collection]: """ - Get all collections, optionally caller can filter by enabled flag + Get all collections for a given learning package + + Enabled collections are returned by default. """ - qs = Collection.objects.all() + qs = Collection.objects.filter(learning_package_id=learning_package_id) if enabled is not None: qs = qs.filter(enabled=enabled) return qs.select_related("learning_package").order_by('pk') diff --git a/openedx_learning/apps/authoring/collections/migrations/0003_collection_entities.py b/openedx_learning/apps/authoring/collections/migrations/0003_collection_entities.py new file mode 100644 index 00000000..1c5e8405 --- /dev/null +++ b/openedx_learning/apps/authoring/collections/migrations/0003_collection_entities.py @@ -0,0 +1,38 @@ +# Generated by Django 4.2.15 on 2024-08-29 00:05 + +import django.db.models.deletion +from django.conf import settings +from django.db import migrations, models + +import openedx_learning.lib.validators + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('oel_publishing', '0002_alter_learningpackage_key_and_more'), + ('oel_collections', '0002_remove_collection_name_collection_created_by_and_more'), + ] + + operations = [ + migrations.CreateModel( + name='CollectionPublishableEntity', + fields=[ + ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('created', models.DateTimeField(auto_now_add=True, validators=[openedx_learning.lib.validators.validate_utc_datetime])), + ('collection', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='oel_collections.collection')), + ('created_by', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, to=settings.AUTH_USER_MODEL)), + ('entity', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, to='oel_publishing.publishableentity')), + ], + ), + migrations.AddField( + model_name='collection', + name='entities', + field=models.ManyToManyField(related_name='collections', through='oel_collections.CollectionPublishableEntity', to='oel_publishing.publishableentity'), + ), + migrations.AddConstraint( + model_name='collectionpublishableentity', + constraint=models.UniqueConstraint(fields=('collection', 'entity'), name='oel_collections_cpe_uniq_col_ent'), + ), + ] diff --git a/openedx_learning/apps/authoring/collections/models.py b/openedx_learning/apps/authoring/collections/models.py index d52d1109..c67e6c26 100644 --- a/openedx_learning/apps/authoring/collections/models.py +++ b/openedx_learning/apps/authoring/collections/models.py @@ -1,5 +1,68 @@ """ Core models for Collections + +TLDR Guidelines: +1. DO NOT modify these models to store full version snapshots. +2. DO NOT use these models to try to reconstruct historical versions of + Collections for fast querying. + +If you're trying to do either of these things, you probably want a new model or +app. For more details, read on. + +The goal of these models is to provide a lightweight method of organizing +PublishableEntities. The first use case for this is modeling the structure of a +v1 Content Library within a LearningPackage. This is what we'll use the +Collection model for. + +An important thing to note here is that Collections are *NOT* publishable +entities themselves. They have no "Draft" or "Published" versions. Collections +are never "published", though the things inside of them are. + +When a LibraryContentBlock makes use of a Content Library, it copies all of +the items it will use into the Course itself. It will also store a version +on the LibraryContentBlock -- this is a MongoDB ObjectID in v1 and an integer in +v2 Libraries. Later on, the LibraryContentBlock will want to check back to see +if any updates have been made, using its version as a key. If a new version +exists, the course team has the option of re-copying data from the Library. + +ModuleStore based v1 Libraries and Blockstore-based v2 libraries both version +the entire library in a series of snapshots. This makes it difficult to have +very large libraries, which is an explicit goal for Modular Learning. In +Learning Core, we've moved to tracking the versions of individual Components to +address this issue. But that means we no longer have a single version indicator +for "has anything here changed"? + +We *could* have put that version in the ``publishing`` app's PublishLog, but +that would make it too broad. We want the ability to eventually collapse many v1 +Libraries into a single Learning Core backed v2 Library. If we tracked the +versioning in only a central location, then we'd have many false positives where +the version was bumped because something else in the Learning Package changed. +So instead, we're creating a new Collection model inside the LearningPackage to +track that concept. + +A critical takeaway is that we don't have to store snapshots of every version of +a Collection, because that data has been copied over by the LibraryContentBlock. +We only need to store the current state of the Collection, and increment the +version numbers when changes happen. This will allow the LibraryContentBlock to +check in and re-copy over the latest version if the course team desires. + +That's why these models only store the current state of a Collection. Unlike the +``components`` app, ``collections`` does not store fully materialized snapshots +of past versions. This is done intentionally in order to save space and reduce +the cost of writes. Collections may grow to be very large, and we don't want to +be writing N rows with every version, where N is the number of +PublishableEntities in a Collection. + +MVP of these models does not store changesets, but we can add this when there's a +use case for it. The number of rows in these changesets would grow in proportion +to the number of things that are actually changing (instead of copying over +everything on every version). This is could be used to make it easier to figure out +what changed between two given versions of a Collection. A LibraryContentBlock +in a course would have stored the version number of the last time it copied data +from the Collection, and we can eventually surface this data to the user. Note that +while it may be possible to reconstruct past versions of Collections based off of +this changeset data, it's going to be a very slow process to do so, and it is +strongly discouraged. """ from __future__ import annotations @@ -9,10 +72,11 @@ from ....lib.fields import MultiCollationTextField, case_insensitive_char_field from ....lib.validators import validate_utc_datetime -from ..publishing.models import LearningPackage +from ..publishing.models import LearningPackage, PublishableEntity __all__ = [ "Collection", + "CollectionPublishableEntity", ] @@ -79,6 +143,12 @@ class Collection(models.Model): ], ) + entities: models.ManyToManyField[PublishableEntity, "CollectionPublishableEntity"] = models.ManyToManyField( + PublishableEntity, + through="CollectionPublishableEntity", + related_name="collections", + ) + class Meta: verbose_name_plural = "Collections" indexes = [ @@ -96,3 +166,42 @@ def __str__(self) -> str: User-facing string representation of a Collection. """ return f"<{self.__class__.__name__}> ({self.id}:{self.title})" + + +class CollectionPublishableEntity(models.Model): + """ + Collection -> PublishableEntity association. + """ + collection = models.ForeignKey( + Collection, + on_delete=models.CASCADE, + ) + entity = models.ForeignKey( + PublishableEntity, + on_delete=models.RESTRICT, + ) + created_by = models.ForeignKey( + settings.AUTH_USER_MODEL, + on_delete=models.SET_NULL, + null=True, + blank=True, + ) + created = models.DateTimeField( + auto_now_add=True, + validators=[ + validate_utc_datetime, + ], + ) + + class Meta: + constraints = [ + # Prevent race conditions from making multiple rows associating the + # same Collection to the same Entity. + models.UniqueConstraint( + fields=[ + "collection", + "entity", + ], + name="oel_collections_cpe_uniq_col_ent", + ) + ] diff --git a/openedx_learning/apps/authoring/collections/readme.rst b/openedx_learning/apps/authoring/collections/readme.rst index 5207bb51..e6d55e34 100644 --- a/openedx_learning/apps/authoring/collections/readme.rst +++ b/openedx_learning/apps/authoring/collections/readme.rst @@ -12,3 +12,12 @@ had to create many small libraries. For the Libraries Relaunch ("v2"), we want to encourage people to create large libraries with lots of content, and organize that content using tags and collections. + +Architecture Guidelines +----------------------- + +Things to remember: + +* Collections may grow very large. +* Collections are not publishable in and of themselves. +* Collections link to PublishableEntity records, **not** PublishableEntityVersion records. diff --git a/tests/openedx_learning/apps/authoring/collections/test_api.py b/tests/openedx_learning/apps/authoring/collections/test_api.py index 9f4eef70..3d7f299e 100644 --- a/tests/openedx_learning/apps/authoring/collections/test_api.py +++ b/tests/openedx_learning/apps/authoring/collections/test_api.py @@ -4,13 +4,18 @@ from datetime import datetime, timezone from django.contrib.auth import get_user_model -from django.core.exceptions import ObjectDoesNotExist +from django.core.exceptions import ObjectDoesNotExist, ValidationError from freezegun import freeze_time -from openedx_learning.apps.authoring.collections import api as collection_api -from openedx_learning.apps.authoring.collections.models import Collection -from openedx_learning.apps.authoring.publishing import api as publishing_api -from openedx_learning.apps.authoring.publishing.models import LearningPackage +# Ensure our APIs and models are all exported to the package API. +from openedx_learning.api import authoring as api +from openedx_learning.api.authoring_models import ( + Collection, + CollectionPublishableEntity, + LearningPackage, + PublishableEntity, + PublishableEntityVersion, +) from openedx_learning.lib.test_utils import TestCase User = get_user_model() @@ -26,20 +31,20 @@ class CollectionTestCase(TestCase): @classmethod def setUpTestData(cls) -> None: - cls.learning_package = publishing_api.create_learning_package( + cls.learning_package = api.create_learning_package( key="ComponentTestCase-test-key", title="Components Test Case Learning Package", ) - cls.learning_package_2 = publishing_api.create_learning_package( + cls.learning_package_2 = api.create_learning_package( key="ComponentTestCase-test-key-2", title="Components Test Case another Learning Package", ) cls.now = datetime(2024, 8, 5, tzinfo=timezone.utc) -class GetCollectionTestCase(CollectionTestCase): +class CollectionsTestCase(CollectionTestCase): """ - Test grabbing a queryset of Collections. + Base class with a bunch of collections pre-created. """ collection1: Collection collection2: Collection @@ -52,38 +57,42 @@ def setUpTestData(cls) -> None: Initialize our content data (all our tests are read only). """ super().setUpTestData() - cls.collection1 = collection_api.create_collection( + cls.collection1 = api.create_collection( cls.learning_package.id, created_by=None, title="Collection 1", description="Description of Collection 1", ) - cls.collection2 = collection_api.create_collection( + cls.collection2 = api.create_collection( cls.learning_package.id, created_by=None, title="Collection 2", description="Description of Collection 2", ) - cls.collection3 = collection_api.create_collection( + cls.collection3 = api.create_collection( cls.learning_package_2.id, created_by=None, title="Collection 3", description="Description of Collection 3", ) - cls.disabled_collection = collection_api.create_collection( + cls.disabled_collection = api.create_collection( cls.learning_package.id, created_by=None, title="Disabled Collection", description="Description of Disabled Collection", + enabled=False, ) - cls.disabled_collection.enabled = False - cls.disabled_collection.save() + +class GetCollectionTestCase(CollectionsTestCase): + """ + Test grabbing a queryset of Collections. + """ def test_get_collection(self): """ Test getting a single collection. """ - collection = collection_api.get_collection(self.collection1.pk) + collection = api.get_collection(self.collection1.pk) assert collection == self.collection1 def test_get_collection_not_found(self): @@ -91,34 +100,33 @@ def test_get_collection_not_found(self): Test getting a collection that doesn't exist. """ with self.assertRaises(ObjectDoesNotExist): - collection_api.get_collection(12345) + api.get_collection(12345) - def test_get_learning_package_collections(self): + def test_get_collections(self): """ Test getting all ENABLED collections for a learning package. """ - collections = collection_api.get_learning_package_collections(self.learning_package.id) + collections = api.get_collections(self.learning_package.id) assert list(collections) == [ self.collection1, self.collection2, ] - def test_get_invalid_learning_package_collections(self): + def test_get_invalid_collections(self): """ Test getting collections for an invalid learning package should return an empty queryset. """ - collections = collection_api.get_learning_package_collections(12345) + collections = api.get_collections(12345) assert not list(collections) def test_get_all_collections(self): """ Test getting all collections. """ - collections = collection_api.get_collections() + collections = api.get_collections(self.learning_package.id, enabled=None) self.assertQuerySetEqual(collections, [ self.collection1, self.collection2, - self.collection3, self.disabled_collection, ], ordered=True) @@ -126,18 +134,17 @@ def test_get_all_enabled_collections(self): """ Test getting all ENABLED collections. """ - collections = collection_api.get_collections(enabled=True) + collections = api.get_collections(self.learning_package.id, enabled=True) self.assertQuerySetEqual(collections, [ self.collection1, self.collection2, - self.collection3, ], ordered=True) def test_get_all_disabled_collections(self): """ Test getting all DISABLED collections. """ - collections = collection_api.get_collections(enabled=False) + collections = api.get_collections(self.learning_package.id, enabled=False) assert list(collections) == [self.disabled_collection] @@ -156,7 +163,7 @@ def test_create_collection(self): ) created_time = datetime(2024, 8, 8, tzinfo=timezone.utc) with freeze_time(created_time): - collection = collection_api.create_collection( + collection = api.create_collection( self.learning_package.id, title="My Collection", created_by=user.id, @@ -174,7 +181,7 @@ def test_create_collection_without_description(self): """ Test creating a collection without a description. """ - collection = collection_api.create_collection( + collection = api.create_collection( self.learning_package.id, created_by=None, title="My Collection", @@ -184,6 +191,185 @@ def test_create_collection_without_description(self): assert collection.enabled +class CollectionEntitiesTestCase(CollectionsTestCase): + """ + Test collections that contain publishable entitites. + """ + published_entity: PublishableEntity + pe_version: PublishableEntityVersion + draft_entity: PublishableEntity + de_version: PublishableEntityVersion + user: User # type: ignore [valid-type] + + @classmethod + def setUpTestData(cls) -> None: + """ + Initialize our content data + """ + super().setUpTestData() + + cls.user = User.objects.create( + username="user", + email="user@example.com", + ) + + # Make and Publish one PublishableEntity + cls.published_entity = api.create_publishable_entity( + cls.learning_package.id, + key="my_entity_published_example", + created=cls.now, + created_by=cls.user.id, + ) + cls.pe_version = api.create_publishable_entity_version( + cls.published_entity.id, + version_num=1, + title="An Entity that we'll Publish 🌴", + created=cls.now, + created_by=cls.user.id, + ) + api.publish_all_drafts( + cls.learning_package.id, + message="Publish from CollectionTestCase.setUpTestData", + published_at=cls.now, + ) + + # Create two Draft PublishableEntities, one in each learning package + cls.draft_entity = api.create_publishable_entity( + cls.learning_package.id, + key="my_entity_draft_example", + created=cls.now, + created_by=cls.user.id, + ) + cls.de_version = api.create_publishable_entity_version( + cls.draft_entity.id, + version_num=1, + title="An Entity that we'll keep in Draft 🌴", + created=cls.now, + created_by=cls.user.id, + ) + + # Add some shared entities to the collections + cls.collection1 = api.add_to_collection( + cls.collection1.id, + 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, + 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, + entities_qset=PublishableEntity.objects.filter(id__in=[ + cls.published_entity.id, + ]), + ) + + def test_create_collection_entities(self): + """ + Ensure the collections were pre-populated with the expected publishable entities. + """ + assert list(self.collection1.entities.all()) == [ + self.published_entity, + ] + assert list(self.collection2.entities.all()) == [ + self.published_entity, + self.draft_entity, + ] + assert not list(self.collection3.entities.all()) + + def test_add_to_collection(self): + """ + Test adding entities to collections. + """ + modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc) + with freeze_time(modified_time): + self.collection1 = api.add_to_collection( + self.collection1.id, + PublishableEntity.objects.filter(id__in=[ + self.draft_entity.id, + ]), + created_by=self.user.id, + ) + + assert list(self.collection1.entities.all()) == [ + self.published_entity, + self.draft_entity, + ] + for collection_entity in CollectionPublishableEntity.objects.filter(collection=self.collection1): + assert collection_entity.created_by == self.user + assert self.collection1.modified == modified_time + + def test_add_to_collection_again(self): + """ + Test that re-adding entities to a collection doesn't throw an error. + """ + modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc) + with freeze_time(modified_time): + self.collection2 = api.add_to_collection( + self.collection2.id, + PublishableEntity.objects.filter(id__in=[ + self.published_entity.id, + ]), + ) + + assert list(self.collection2.entities.all()) == [ + self.published_entity, + self.draft_entity, + ] + assert self.collection2.modified == modified_time + + def test_add_to_collection_wrong_learning_package(self): + """ + We cannot add entities to a collection from a different learning package. + """ + with self.assertRaises(ValidationError): + api.add_to_collection( + self.collection3.id, + PublishableEntity.objects.filter(id__in=[ + self.published_entity.id, + ]), + ) + + assert not list(self.collection3.entities.all()) + + def test_remove_from_collection(self): + """ + Test removing entities from a collection. + """ + modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc) + with freeze_time(modified_time): + self.collection2 = api.remove_from_collection( + self.collection2.id, + PublishableEntity.objects.filter(id__in=[ + self.published_entity.id, + ]), + ) + + assert list(self.collection2.entities.all()) == [ + self.draft_entity, + ] + assert self.collection2.modified == modified_time + + def test_get_entity_collections(self): + """ + Tests fetching the enabled collections which contain a given entity. + """ + collections = api.get_entity_collections( + self.learning_package.id, + self.published_entity.key, + ) + assert list(collections) == [ + self.collection1, + self.collection2, + ] + + class UpdateCollectionTestCase(CollectionTestCase): """ Test updating a collection. @@ -195,7 +381,7 @@ def setUp(self) -> None: Initialize our content data """ super().setUp() - self.collection = collection_api.create_collection( + self.collection = api.create_collection( self.learning_package.id, title="Collection", created_by=None, @@ -208,7 +394,7 @@ def test_update_collection(self): """ modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc) with freeze_time(modified_time): - collection = collection_api.update_collection( + collection = api.update_collection( self.collection.pk, title="New Title", description="", @@ -223,7 +409,7 @@ def test_update_collection_partial(self): """ Test updating a collection's title. """ - collection = collection_api.update_collection( + collection = api.update_collection( self.collection.pk, title="New Title", ) @@ -232,7 +418,7 @@ def test_update_collection_partial(self): assert collection.description == self.collection.description # unchanged assert f"{collection}" == f" ({self.collection.pk}:New Title)" - collection = collection_api.update_collection( + collection = api.update_collection( self.collection.pk, description="New description", ) @@ -246,7 +432,7 @@ def test_update_collection_empty(self): """ modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc) with freeze_time(modified_time): - collection = collection_api.update_collection( + collection = api.update_collection( self.collection.pk, ) @@ -259,4 +445,4 @@ def test_update_collection_not_found(self): Test updating a collection that doesn't exist. """ with self.assertRaises(ObjectDoesNotExist): - collection_api.update_collection(12345, title="New Title") + api.update_collection(12345, title="New Title")