From 534e8d9aea395a602e269bdf01df212522118c02 Mon Sep 17 00:00:00 2001
From: Jillian Vogel <jill@opencraft.com>
Date: Wed, 21 Aug 2024 15:44:55 +0930
Subject: [PATCH 1/9] docs: adds guidelines for the Collection models

copied and modified slightly from @ormsbee's https://github.com/openedx/openedx-learning/pull/131
---
 .../apps/authoring/collections/models.py      | 63 +++++++++++++++++++
 .../apps/authoring/collections/readme.rst     |  9 +++
 2 files changed, 72 insertions(+)

diff --git a/openedx_learning/apps/authoring/collections/models.py b/openedx_learning/apps/authoring/collections/models.py
index d52d1109..06f4e44c 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
 
diff --git a/openedx_learning/apps/authoring/collections/readme.rst b/openedx_learning/apps/authoring/collections/readme.rst
index 5207bb51..962b42d1 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 PublisableEntity records, **not** PublishableEntityVersion records.

From 1fda3e5dd85c5c785a7eaa32ff114ef8b9af4239 Mon Sep 17 00:00:00 2001
From: Jillian Vogel <jill@opencraft.com>
Date: Wed, 21 Aug 2024 17:06:19 +0930
Subject: [PATCH 2/9] feat: adds CollectionObject model and APIs

to associate PublishableEntities with Collections.

Collection.modified timestamp is updated whenever its contents are changed.
---
 .../apps/authoring/collections/api.py         | 104 ++++++++-
 .../migrations/0003_collection_contents.py    |  32 +++
 .../apps/authoring/collections/models.py      |  36 +++-
 .../apps/authoring/collections/test_api.py    | 197 +++++++++++++++++-
 4 files changed, 357 insertions(+), 12 deletions(-)
 create mode 100644 openedx_learning/apps/authoring/collections/migrations/0003_collection_contents.py

diff --git a/openedx_learning/apps/authoring/collections/api.py b/openedx_learning/apps/authoring/collections/api.py
index 942903b7..cd9cb3dd 100644
--- a/openedx_learning/apps/authoring/collections/api.py
+++ b/openedx_learning/apps/authoring/collections/api.py
@@ -4,8 +4,11 @@
 from __future__ import annotations
 
 from django.db.models import QuerySet
+from django.db.transaction import atomic
+from django.utils import timezone
 
-from .models import Collection
+from ..publishing.models import PublishableEntity
+from .models import Collection, CollectionObject
 
 # The public API that will be re-exported by openedx_learning.apps.authoring.api
 # is listed in the __all__ entries below. Internal helper functions that are
@@ -13,10 +16,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_collections",
     "create_collection",
     "get_collection",
     "get_collections",
     "get_learning_package_collections",
+    "remove_from_collections",
     "update_collection",
 ]
 
@@ -26,16 +31,27 @@ def create_collection(
     title: str,
     created_by: int | None,
     description: str = "",
+    contents_qset: QuerySet[PublishableEntity] = PublishableEntity.objects.none(),  # default to empty set,
 ) -> Collection:
     """
     Create a new Collection
     """
-    collection = Collection.objects.create(
-        learning_package_id=learning_package_id,
-        title=title,
-        created_by_id=created_by,
-        description=description,
-    )
+
+    with atomic():
+        collection = Collection.objects.create(
+            learning_package_id=learning_package_id,
+            title=title,
+            created_by_id=created_by,
+            description=description,
+        )
+
+        added = add_to_collections(
+            Collection.objects.filter(id=collection.id),
+            contents_qset,
+        )
+        if added:
+            collection.refresh_from_db()  # fetch updated modified date
+
     return collection
 
 
@@ -70,6 +86,80 @@ def update_collection(
     return collection
 
 
+def add_to_collections(
+    collections_qset: QuerySet[Collection],
+    contents_qset: QuerySet[PublishableEntity],
+) -> int:
+    """
+    Adds a QuerySet of PublishableEntities to a QuerySet of Collections.
+
+    Records are created in bulk, and so integrity errors are deliberately ignored: they indicate that the content(s)
+    have already been added to the collection(s).
+
+    Returns the number of entities added (including any that already exist).
+    """
+    collection_objects = []
+    object_ids = contents_qset.values_list("pk", flat=True)
+    collection_ids = collections_qset.values_list("pk", flat=True)
+
+    for collection_id in collection_ids:
+        for object_id in object_ids:
+            collection_objects.append(
+                CollectionObject(
+                    collection_id=collection_id,
+                    object_id=object_id,
+                )
+            )
+
+    created = []
+    if collection_objects:
+        created = CollectionObject.objects.bulk_create(
+            collection_objects,
+            ignore_conflicts=True,
+        )
+
+        # Update the modified date for all the provided Collections.
+        # (Ignoring conflicts means we don't know which ones were modified.)
+        collections_qset.update(modified=timezone.now())
+
+    return len(created)
+
+
+def remove_from_collections(
+    collections_qset: QuerySet,
+    contents_qset: QuerySet,
+) -> int:
+    """
+    Removes a QuerySet of PublishableEntities from a QuerySet of Collections.
+
+    PublishableEntities are deleted from each Collection, in bulk.
+
+    Collections which had objects removed are marked with modified=now().
+
+    Returns the total number of entities deleted.
+    """
+    total_deleted = 0
+    object_ids = contents_qset.values_list("pk", flat=True)
+    collection_ids = collections_qset.values_list("pk", flat=True)
+    modified_collection_ids = []
+
+    for collection_id in collection_ids:
+        num_deleted, _ = CollectionObject.objects.filter(
+            collection_id=collection_id,
+            object_id__in=object_ids,
+        ).delete()
+
+        if num_deleted:
+            modified_collection_ids.append(collection_id)
+
+        total_deleted += num_deleted
+
+    # Update the modified date for the affected Collections
+    Collection.objects.filter(id__in=modified_collection_ids).update(modified=timezone.now())
+
+    return total_deleted
+
+
 def get_learning_package_collections(learning_package_id: int) -> QuerySet[Collection]:
     """
     Get all collections for a given learning package
diff --git a/openedx_learning/apps/authoring/collections/migrations/0003_collection_contents.py b/openedx_learning/apps/authoring/collections/migrations/0003_collection_contents.py
new file mode 100644
index 00000000..b88a87d5
--- /dev/null
+++ b/openedx_learning/apps/authoring/collections/migrations/0003_collection_contents.py
@@ -0,0 +1,32 @@
+# Generated by Django 4.2.14 on 2024-08-21 07:15
+
+import django.db.models.deletion
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('oel_publishing', '0002_alter_learningpackage_key_and_more'),
+        ('oel_collections', '0002_remove_collection_name_collection_created_by_and_more'),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name='CollectionObject',
+            fields=[
+                ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
+                ('collection', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='oel_collections.collection')),
+                ('object', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='oel_publishing.publishableentity')),
+            ],
+        ),
+        migrations.AddField(
+            model_name='collection',
+            name='contents',
+            field=models.ManyToManyField(related_name='collections', through='oel_collections.CollectionObject', to='oel_publishing.publishableentity'),
+        ),
+        migrations.AddConstraint(
+            model_name='collectionobject',
+            constraint=models.UniqueConstraint(fields=('collection', 'object'), name='oel_collections_cpe_uniq_col_obj'),
+        ),
+    ]
diff --git a/openedx_learning/apps/authoring/collections/models.py b/openedx_learning/apps/authoring/collections/models.py
index 06f4e44c..1f902498 100644
--- a/openedx_learning/apps/authoring/collections/models.py
+++ b/openedx_learning/apps/authoring/collections/models.py
@@ -72,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",
+    "CollectionObject",
 ]
 
 
@@ -142,6 +143,12 @@ class Collection(models.Model):
         ],
     )
 
+    contents: models.ManyToManyField[PublishableEntity, "CollectionObject"] = models.ManyToManyField(
+        PublishableEntity,
+        through="CollectionObject",
+        related_name="collections",
+    )
+
     class Meta:
         verbose_name_plural = "Collections"
         indexes = [
@@ -159,3 +166,30 @@ def __str__(self) -> str:
         User-facing string representation of a Collection.
         """
         return f"<{self.__class__.__name__}> ({self.id}:{self.title})"
+
+
+class CollectionObject(models.Model):
+    """
+    Collection -> PublishableEntity association.
+    """
+    collection = models.ForeignKey(
+        Collection,
+        on_delete=models.CASCADE,
+    )
+    object = models.ForeignKey(
+        PublishableEntity,
+        on_delete=models.CASCADE,
+    )
+
+    class Meta:
+        constraints = [
+            # Prevent race conditions from making multiple rows associating the
+            # same Collection to the same Entity.
+            models.UniqueConstraint(
+                fields=[
+                    "collection",
+                    "object",
+                ],
+                name="oel_collections_cpe_uniq_col_obj",
+            )
+        ]
diff --git a/tests/openedx_learning/apps/authoring/collections/test_api.py b/tests/openedx_learning/apps/authoring/collections/test_api.py
index 9f4eef70..2d1df7ed 100644
--- a/tests/openedx_learning/apps/authoring/collections/test_api.py
+++ b/tests/openedx_learning/apps/authoring/collections/test_api.py
@@ -10,7 +10,11 @@
 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
+from openedx_learning.apps.authoring.publishing.models import (
+    LearningPackage,
+    PublishableEntity,
+    PublishableEntityVersion,
+)
 from openedx_learning.lib.test_utils import TestCase
 
 User = get_user_model()
@@ -184,6 +188,190 @@ def test_create_collection_without_description(self):
         assert collection.enabled
 
 
+class CollectionContentsTestCase(CollectionTestCase):
+    """
+    Test collections that contain publishable entitites.
+    """
+    published_entity: PublishableEntity
+    pe_version: PublishableEntityVersion
+    draft_entity: PublishableEntity
+    de_version: PublishableEntityVersion
+    collection0: Collection
+    collection1: Collection
+    collection2: Collection
+
+    @classmethod
+    def setUpTestData(cls) -> None:
+        """
+        Initialize our content data (all our tests are read only).
+        """
+        super().setUpTestData()
+
+        # Make and Publish one PublishableEntity
+        cls.published_entity = publishing_api.create_publishable_entity(
+            cls.learning_package.id,
+            key="my_entity_published_example",
+            created=cls.now,
+            created_by=None,
+        )
+        cls.pe_version = publishing_api.create_publishable_entity_version(
+            cls.published_entity.id,
+            version_num=1,
+            title="An Entity that we'll Publish 🌴",
+            created=cls.now,
+            created_by=None,
+        )
+        publishing_api.publish_all_drafts(
+            cls.learning_package.id,
+            message="Publish from CollectionTestCase.setUpTestData",
+            published_at=cls.now,
+        )
+
+        # Leave another PublishableEntity in Draft.
+        cls.draft_entity = publishing_api.create_publishable_entity(
+            cls.learning_package.id,
+            key="my_entity_draft_example",
+            created=cls.now,
+            created_by=None,
+        )
+        cls.de_version = publishing_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=None,
+        )
+
+        # Create collections with some shared contents
+        cls.collection0 = collection_api.create_collection(
+            cls.learning_package.id,
+            title="Collection Empty",
+            created_by=None,
+            description="This collection contains 0 objects",
+        )
+        cls.collection1 = collection_api.create_collection(
+            cls.learning_package.id,
+            title="Collection One",
+            created_by=None,
+            description="This collection contains 1 object",
+            contents_qset=PublishableEntity.objects.filter(id__in=[
+                cls.published_entity.id,
+            ]),
+        )
+        cls.collection2 = collection_api.create_collection(
+            cls.learning_package.id,
+            title="Collection Two",
+            created_by=None,
+            description="This collection contains 2 objects",
+            contents_qset=PublishableEntity.objects.filter(id__in=[
+                cls.published_entity.id,
+                cls.draft_entity.id,
+            ]),
+        )
+
+    def test_create_collection_contents(self):
+        """
+        Ensure the collections were pre-populated with the expected publishable entities.
+        """
+        assert not list(self.collection0.contents.all())
+        assert list(self.collection1.contents.all()) == [
+            self.published_entity,
+        ]
+        assert list(self.collection2.contents.all()) == [
+            self.published_entity,
+            self.draft_entity,
+        ]
+
+    def test_add_to_collections(self):
+        """
+        Test adding objects to collections.
+        """
+        modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc)
+        with freeze_time(modified_time):
+            count = collection_api.add_to_collections(
+                Collection.objects.filter(id__in=[
+                    self.collection1.id,
+                ]),
+                PublishableEntity.objects.filter(id__in=[
+                    self.draft_entity.id,
+                ]),
+            )
+        assert count == 1
+        assert list(self.collection1.contents.all()) == [
+            self.published_entity,
+            self.draft_entity,
+        ]
+        self.collection1.refresh_from_db()
+        assert self.collection1.modified == modified_time
+
+    def test_add_to_collections_again(self):
+        """
+        Test that re-adding objects to collections doesn't throw an error.
+        """
+        modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc)
+        with freeze_time(modified_time):
+            count = collection_api.add_to_collections(
+                Collection.objects.filter(id__in=[
+                    self.collection1.id,
+                    self.collection2.id,
+                ]),
+                PublishableEntity.objects.filter(id__in=[
+                    self.published_entity.id,
+                ]),
+            )
+
+        assert count == 2
+        assert list(self.collection1.contents.all()) == [
+            self.published_entity,
+        ]
+        assert list(self.collection2.contents.all()) == [
+            self.published_entity,
+            self.draft_entity,
+        ]
+
+        # Modified dates will have have changed, even though we didn't really add anything
+        self.collection1.refresh_from_db()
+        assert self.collection1.modified == modified_time
+        self.collection2.refresh_from_db()
+        assert self.collection2.modified == modified_time
+
+    def test_remove_from_collections(self):
+        """
+        Test removing objects from collections.
+        """
+        # Expect no changes to be made to collection0
+        coll0_modified = self.collection0.modified
+
+        modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc)
+        with freeze_time(modified_time):
+            count = collection_api.remove_from_collections(
+                Collection.objects.filter(id__in=[
+                    self.collection0.id,
+                    self.collection1.id,
+                    self.collection2.id,
+                ]),
+                PublishableEntity.objects.filter(id__in=[
+                    self.published_entity.id,
+                ]),
+            )
+
+        assert count == 2
+
+        assert not list(self.collection0.contents.all())
+        assert not list(self.collection1.contents.all())
+        assert list(self.collection2.contents.all()) == [
+            self.draft_entity,
+        ]
+
+        # Check that the modified collections were updated
+        self.collection0.refresh_from_db()
+        assert self.collection0.modified == coll0_modified
+        self.collection1.refresh_from_db()
+        assert self.collection1.modified == modified_time
+        self.collection2.refresh_from_db()
+        assert self.collection2.modified == modified_time
+
+
 class UpdateCollectionTestCase(CollectionTestCase):
     """
     Test updating a collection.
@@ -250,9 +438,10 @@ def test_update_collection_empty(self):
                 self.collection.pk,
             )
 
-        assert collection.title == self.collection.title  # unchanged
-        assert collection.description == self.collection.description  # unchanged
-        assert collection.modified == self.collection.modified  # unchanged
+        # all fields unchanged
+        assert collection.title == self.collection.title
+        assert collection.description == self.collection.description
+        assert collection.modified == self.collection.modified
 
     def test_update_collection_not_found(self):
         """

From a2cd234f45fda1c54c679776b3f893b9e4cd70fc Mon Sep 17 00:00:00 2001
From: Jillian Vogel <jill@opencraft.com>
Date: Fri, 23 Aug 2024 17:20:34 +0930
Subject: [PATCH 3/9] feat: adds get_object_collections API and tests

---
 .../apps/authoring/collections/api.py         | 11 +++++++++
 .../apps/authoring/collections/test_api.py    | 24 +++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/openedx_learning/apps/authoring/collections/api.py b/openedx_learning/apps/authoring/collections/api.py
index cd9cb3dd..464fd467 100644
--- a/openedx_learning/apps/authoring/collections/api.py
+++ b/openedx_learning/apps/authoring/collections/api.py
@@ -21,6 +21,7 @@
     "get_collection",
     "get_collections",
     "get_learning_package_collections",
+    "get_object_collections",
     "remove_from_collections",
     "update_collection",
 ]
@@ -160,6 +161,16 @@ def remove_from_collections(
     return total_deleted
 
 
+def get_object_collections(object_id: int) -> QuerySet[Collection]:
+    """
+    Get all collections associated with a given PublishableEntity.
+
+    Only enabled collections are returned.
+    """
+    entity = PublishableEntity.objects.get(pk=object_id)
+    return entity.collections.filter(enabled=True).order_by("pk")
+
+
 def get_learning_package_collections(learning_package_id: int) -> QuerySet[Collection]:
     """
     Get all collections for a given learning package
diff --git a/tests/openedx_learning/apps/authoring/collections/test_api.py b/tests/openedx_learning/apps/authoring/collections/test_api.py
index 2d1df7ed..16b8bc33 100644
--- a/tests/openedx_learning/apps/authoring/collections/test_api.py
+++ b/tests/openedx_learning/apps/authoring/collections/test_api.py
@@ -199,6 +199,7 @@ class CollectionContentsTestCase(CollectionTestCase):
     collection0: Collection
     collection1: Collection
     collection2: Collection
+    disabled_collection: Collection
 
     @classmethod
     def setUpTestData(cls) -> None:
@@ -268,6 +269,17 @@ def setUpTestData(cls) -> None:
                 cls.draft_entity.id,
             ]),
         )
+        cls.disabled_collection = collection_api.create_collection(
+            cls.learning_package.id,
+            title="Disabled Collection",
+            created_by=None,
+            description="This disabled collection contains 1 object",
+            contents_qset=PublishableEntity.objects.filter(id__in=[
+                cls.published_entity.id,
+            ]),
+        )
+        cls.disabled_collection.enabled = False
+        cls.disabled_collection.save()
 
     def test_create_collection_contents(self):
         """
@@ -371,6 +383,18 @@ def test_remove_from_collections(self):
         self.collection2.refresh_from_db()
         assert self.collection2.modified == modified_time
 
+    def test_get_object_collections(self):
+        """
+        Tests fetching the enabled collections which contain a given object.
+        """
+        collections = collection_api.get_object_collections(
+            self.published_entity.id,
+        )
+        assert list(collections) == [
+            self.collection1,
+            self.collection2,
+        ]
+
 
 class UpdateCollectionTestCase(CollectionTestCase):
     """

From 7ec9ba56edd9c049b0fc839a0d7cfc581fa28f88 Mon Sep 17 00:00:00 2001
From: Jillian Vogel <jill@opencraft.com>
Date: Tue, 27 Aug 2024 15:28:44 +0930
Subject: [PATCH 4/9] chore: bumps version to 0.11.3

---
 openedx_learning/__init__.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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"

From 0e15dd0d6445464b73eb8572c8bc83f8b0fdee16 Mon Sep 17 00:00:00 2001
From: Jillian Vogel <jill@opencraft.com>
Date: Wed, 28 Aug 2024 19:29:30 +0930
Subject: [PATCH 5/9] fix: use PublishableEntity.key instead of IDs

---
 openedx_learning/apps/authoring/collections/api.py          | 6 +++---
 .../openedx_learning/apps/authoring/collections/test_api.py | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/openedx_learning/apps/authoring/collections/api.py b/openedx_learning/apps/authoring/collections/api.py
index 464fd467..4442d486 100644
--- a/openedx_learning/apps/authoring/collections/api.py
+++ b/openedx_learning/apps/authoring/collections/api.py
@@ -161,13 +161,13 @@ def remove_from_collections(
     return total_deleted
 
 
-def get_object_collections(object_id: int) -> QuerySet[Collection]:
+def get_object_collections(object_key: str) -> QuerySet[Collection]:
     """
-    Get all collections associated with a given PublishableEntity.
+    Get all collections associated with a given PublishableEntity.key.
 
     Only enabled collections are returned.
     """
-    entity = PublishableEntity.objects.get(pk=object_id)
+    entity = PublishableEntity.objects.get(key=object_key)
     return entity.collections.filter(enabled=True).order_by("pk")
 
 
diff --git a/tests/openedx_learning/apps/authoring/collections/test_api.py b/tests/openedx_learning/apps/authoring/collections/test_api.py
index 16b8bc33..d439e777 100644
--- a/tests/openedx_learning/apps/authoring/collections/test_api.py
+++ b/tests/openedx_learning/apps/authoring/collections/test_api.py
@@ -388,7 +388,7 @@ def test_get_object_collections(self):
         Tests fetching the enabled collections which contain a given object.
         """
         collections = collection_api.get_object_collections(
-            self.published_entity.id,
+            self.published_entity.key,
         )
         assert list(collections) == [
             self.collection1,

From 00111bceb3e2907920b3fc1de7da4fff0d438ddb Mon Sep 17 00:00:00 2001
From: Jillian Vogel <jill@opencraft.com>
Date: Thu, 29 Aug 2024 10:13:23 +0930
Subject: [PATCH 6/9] refactor: use "entities" instead of "contents" or
 "objects"

Addresses PR review.

* CollectionObject -> CollectionPublishableEntity,
  with added created_by and created fields
* contents -> entities, contents_qset -> entities_qset
* get_object_collections -> get_entity_collections,
  with added learning_package_id argument
---
 .../apps/authoring/collections/api.py         | 55 +++++++-----
 ...ontents.py => 0003_collection_entities.py} | 20 +++--
 .../apps/authoring/collections/models.py      | 28 ++++--
 .../apps/authoring/collections/readme.rst     |  2 +-
 .../apps/authoring/collections/test_api.py    | 87 ++++++++++---------
 5 files changed, 113 insertions(+), 79 deletions(-)
 rename openedx_learning/apps/authoring/collections/migrations/{0003_collection_contents.py => 0003_collection_entities.py} (50%)

diff --git a/openedx_learning/apps/authoring/collections/api.py b/openedx_learning/apps/authoring/collections/api.py
index 4442d486..171cd88e 100644
--- a/openedx_learning/apps/authoring/collections/api.py
+++ b/openedx_learning/apps/authoring/collections/api.py
@@ -7,8 +7,9 @@
 from django.db.transaction import atomic
 from django.utils import timezone
 
+from ..publishing import api as publishing_api
 from ..publishing.models import PublishableEntity
-from .models import Collection, CollectionObject
+from .models import Collection, CollectionPublishableEntity
 
 # The public API that will be re-exported by openedx_learning.apps.authoring.api
 # is listed in the __all__ entries below. Internal helper functions that are
@@ -21,7 +22,7 @@
     "get_collection",
     "get_collections",
     "get_learning_package_collections",
-    "get_object_collections",
+    "get_entity_collections",
     "remove_from_collections",
     "update_collection",
 ]
@@ -32,7 +33,7 @@ def create_collection(
     title: str,
     created_by: int | None,
     description: str = "",
-    contents_qset: QuerySet[PublishableEntity] = PublishableEntity.objects.none(),  # default to empty set,
+    entities_qset: QuerySet[PublishableEntity] = PublishableEntity.objects.none(),  # default to empty set,
 ) -> Collection:
     """
     Create a new Collection
@@ -48,7 +49,8 @@ def create_collection(
 
         added = add_to_collections(
             Collection.objects.filter(id=collection.id),
-            contents_qset,
+            entities_qset,
+            created_by,
         )
         if added:
             collection.refresh_from_db()  # fetch updated modified date
@@ -89,33 +91,35 @@ def update_collection(
 
 def add_to_collections(
     collections_qset: QuerySet[Collection],
-    contents_qset: QuerySet[PublishableEntity],
+    entities_qset: QuerySet[PublishableEntity],
+    created_by: int | None = None,
 ) -> int:
     """
     Adds a QuerySet of PublishableEntities to a QuerySet of Collections.
 
-    Records are created in bulk, and so integrity errors are deliberately ignored: they indicate that the content(s)
+    Records are created in bulk, and so integrity errors are deliberately ignored: they indicate that the entity(s)
     have already been added to the collection(s).
 
     Returns the number of entities added (including any that already exist).
     """
-    collection_objects = []
-    object_ids = contents_qset.values_list("pk", flat=True)
+    collection_entities = []
+    entity_ids = entities_qset.values_list("pk", flat=True)
     collection_ids = collections_qset.values_list("pk", flat=True)
 
     for collection_id in collection_ids:
-        for object_id in object_ids:
-            collection_objects.append(
-                CollectionObject(
+        for entity_id in entity_ids:
+            collection_entities.append(
+                CollectionPublishableEntity(
                     collection_id=collection_id,
-                    object_id=object_id,
+                    entity_id=entity_id,
+                    created_by_id=created_by,
                 )
             )
 
     created = []
-    if collection_objects:
-        created = CollectionObject.objects.bulk_create(
-            collection_objects,
+    if collection_entities:
+        created = CollectionPublishableEntity.objects.bulk_create(
+            collection_entities,
             ignore_conflicts=True,
         )
 
@@ -127,27 +131,27 @@ def add_to_collections(
 
 
 def remove_from_collections(
-    collections_qset: QuerySet,
-    contents_qset: QuerySet,
+    collections_qset: QuerySet[Collection],
+    entities_qset: QuerySet[PublishableEntity],
 ) -> int:
     """
     Removes a QuerySet of PublishableEntities from a QuerySet of Collections.
 
     PublishableEntities are deleted from each Collection, in bulk.
 
-    Collections which had objects removed are marked with modified=now().
+    Collections which had entities removed are marked with modified=now().
 
     Returns the total number of entities deleted.
     """
     total_deleted = 0
-    object_ids = contents_qset.values_list("pk", flat=True)
+    entity_ids = entities_qset.values_list("pk", flat=True)
     collection_ids = collections_qset.values_list("pk", flat=True)
     modified_collection_ids = []
 
     for collection_id in collection_ids:
-        num_deleted, _ = CollectionObject.objects.filter(
+        num_deleted, _ = CollectionPublishableEntity.objects.filter(
             collection_id=collection_id,
-            object_id__in=object_ids,
+            entity__in=entity_ids,
         ).delete()
 
         if num_deleted:
@@ -161,13 +165,16 @@ def remove_from_collections(
     return total_deleted
 
 
-def get_object_collections(object_key: str) -> QuerySet[Collection]:
+def get_entity_collections(learning_package_id: int, entity_key: str) -> QuerySet[Collection]:
     """
-    Get all collections associated with a given PublishableEntity.key.
+    Get all collections in the given learning package which contain this entity.
 
     Only enabled collections are returned.
     """
-    entity = PublishableEntity.objects.get(key=object_key)
+    entity = publishing_api.get_publishable_entity_by_key(
+        learning_package_id,
+        key=entity_key,
+    )
     return entity.collections.filter(enabled=True).order_by("pk")
 
 
diff --git a/openedx_learning/apps/authoring/collections/migrations/0003_collection_contents.py b/openedx_learning/apps/authoring/collections/migrations/0003_collection_entities.py
similarity index 50%
rename from openedx_learning/apps/authoring/collections/migrations/0003_collection_contents.py
rename to openedx_learning/apps/authoring/collections/migrations/0003_collection_entities.py
index b88a87d5..1c5e8405 100644
--- a/openedx_learning/apps/authoring/collections/migrations/0003_collection_contents.py
+++ b/openedx_learning/apps/authoring/collections/migrations/0003_collection_entities.py
@@ -1,32 +1,38 @@
-# Generated by Django 4.2.14 on 2024-08-21 07:15
+# 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='CollectionObject',
+            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')),
-                ('object', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='oel_publishing.publishableentity')),
+                ('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='contents',
-            field=models.ManyToManyField(related_name='collections', through='oel_collections.CollectionObject', to='oel_publishing.publishableentity'),
+            name='entities',
+            field=models.ManyToManyField(related_name='collections', through='oel_collections.CollectionPublishableEntity', to='oel_publishing.publishableentity'),
         ),
         migrations.AddConstraint(
-            model_name='collectionobject',
-            constraint=models.UniqueConstraint(fields=('collection', 'object'), name='oel_collections_cpe_uniq_col_obj'),
+            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 1f902498..c67e6c26 100644
--- a/openedx_learning/apps/authoring/collections/models.py
+++ b/openedx_learning/apps/authoring/collections/models.py
@@ -76,7 +76,7 @@
 
 __all__ = [
     "Collection",
-    "CollectionObject",
+    "CollectionPublishableEntity",
 ]
 
 
@@ -143,9 +143,9 @@ class Collection(models.Model):
         ],
     )
 
-    contents: models.ManyToManyField[PublishableEntity, "CollectionObject"] = models.ManyToManyField(
+    entities: models.ManyToManyField[PublishableEntity, "CollectionPublishableEntity"] = models.ManyToManyField(
         PublishableEntity,
-        through="CollectionObject",
+        through="CollectionPublishableEntity",
         related_name="collections",
     )
 
@@ -168,7 +168,7 @@ def __str__(self) -> str:
         return f"<{self.__class__.__name__}> ({self.id}:{self.title})"
 
 
-class CollectionObject(models.Model):
+class CollectionPublishableEntity(models.Model):
     """
     Collection -> PublishableEntity association.
     """
@@ -176,9 +176,21 @@ class CollectionObject(models.Model):
         Collection,
         on_delete=models.CASCADE,
     )
-    object = models.ForeignKey(
+    entity = models.ForeignKey(
         PublishableEntity,
-        on_delete=models.CASCADE,
+        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:
@@ -188,8 +200,8 @@ class Meta:
             models.UniqueConstraint(
                 fields=[
                     "collection",
-                    "object",
+                    "entity",
                 ],
-                name="oel_collections_cpe_uniq_col_obj",
+                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 962b42d1..e6d55e34 100644
--- a/openedx_learning/apps/authoring/collections/readme.rst
+++ b/openedx_learning/apps/authoring/collections/readme.rst
@@ -20,4 +20,4 @@ Things to remember:
 
 * Collections may grow very large.
 * Collections are not publishable in and of themselves.
-* Collections link to PublisableEntity records, **not** PublishableEntityVersion records.
+* 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 d439e777..d75fad42 100644
--- a/tests/openedx_learning/apps/authoring/collections/test_api.py
+++ b/tests/openedx_learning/apps/authoring/collections/test_api.py
@@ -8,7 +8,7 @@
 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.collections.models import Collection, CollectionPublishableEntity
 from openedx_learning.apps.authoring.publishing import api as publishing_api
 from openedx_learning.apps.authoring.publishing.models import (
     LearningPackage,
@@ -188,7 +188,7 @@ def test_create_collection_without_description(self):
         assert collection.enabled
 
 
-class CollectionContentsTestCase(CollectionTestCase):
+class CollectionEntitiesTestCase(CollectionTestCase):
     """
     Test collections that contain publishable entitites.
     """
@@ -200,27 +200,33 @@ class CollectionContentsTestCase(CollectionTestCase):
     collection1: Collection
     collection2: Collection
     disabled_collection: Collection
+    user: User  # type: ignore [valid-type]
 
     @classmethod
     def setUpTestData(cls) -> None:
         """
-        Initialize our content data (all our tests are read only).
+        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 = publishing_api.create_publishable_entity(
             cls.learning_package.id,
             key="my_entity_published_example",
             created=cls.now,
-            created_by=None,
+            created_by=cls.user.id,
         )
         cls.pe_version = publishing_api.create_publishable_entity_version(
             cls.published_entity.id,
             version_num=1,
             title="An Entity that we'll Publish 🌴",
             created=cls.now,
-            created_by=None,
+            created_by=cls.user.id,
         )
         publishing_api.publish_all_drafts(
             cls.learning_package.id,
@@ -233,38 +239,38 @@ def setUpTestData(cls) -> None:
             cls.learning_package.id,
             key="my_entity_draft_example",
             created=cls.now,
-            created_by=None,
+            created_by=cls.user.id,
         )
         cls.de_version = publishing_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=None,
+            created_by=cls.user.id,
         )
 
-        # Create collections with some shared contents
+        # Create collections with some shared entities
         cls.collection0 = collection_api.create_collection(
             cls.learning_package.id,
             title="Collection Empty",
-            created_by=None,
-            description="This collection contains 0 objects",
+            created_by=cls.user.id,
+            description="This collection contains 0 entities",
         )
         cls.collection1 = collection_api.create_collection(
             cls.learning_package.id,
             title="Collection One",
-            created_by=None,
-            description="This collection contains 1 object",
-            contents_qset=PublishableEntity.objects.filter(id__in=[
+            created_by=cls.user.id,
+            description="This collection contains 1 entity",
+            entities_qset=PublishableEntity.objects.filter(id__in=[
                 cls.published_entity.id,
             ]),
         )
         cls.collection2 = collection_api.create_collection(
             cls.learning_package.id,
             title="Collection Two",
-            created_by=None,
-            description="This collection contains 2 objects",
-            contents_qset=PublishableEntity.objects.filter(id__in=[
+            created_by=cls.user.id,
+            description="This collection contains 2 entities",
+            entities_qset=PublishableEntity.objects.filter(id__in=[
                 cls.published_entity.id,
                 cls.draft_entity.id,
             ]),
@@ -272,31 +278,31 @@ def setUpTestData(cls) -> None:
         cls.disabled_collection = collection_api.create_collection(
             cls.learning_package.id,
             title="Disabled Collection",
-            created_by=None,
-            description="This disabled collection contains 1 object",
-            contents_qset=PublishableEntity.objects.filter(id__in=[
+            created_by=cls.user.id,
+            description="This disabled collection contains 1 entity",
+            entities_qset=PublishableEntity.objects.filter(id__in=[
                 cls.published_entity.id,
             ]),
         )
         cls.disabled_collection.enabled = False
         cls.disabled_collection.save()
 
-    def test_create_collection_contents(self):
+    def test_create_collection_entities(self):
         """
         Ensure the collections were pre-populated with the expected publishable entities.
         """
-        assert not list(self.collection0.contents.all())
-        assert list(self.collection1.contents.all()) == [
+        assert not list(self.collection0.entities.all())
+        assert list(self.collection1.entities.all()) == [
             self.published_entity,
         ]
-        assert list(self.collection2.contents.all()) == [
+        assert list(self.collection2.entities.all()) == [
             self.published_entity,
             self.draft_entity,
         ]
 
     def test_add_to_collections(self):
         """
-        Test adding objects to collections.
+        Test adding entities to collections.
         """
         modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc)
         with freeze_time(modified_time):
@@ -307,18 +313,21 @@ def test_add_to_collections(self):
                 PublishableEntity.objects.filter(id__in=[
                     self.draft_entity.id,
                 ]),
+                created_by=self.user.id,
             )
         assert count == 1
-        assert list(self.collection1.contents.all()) == [
+        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
         self.collection1.refresh_from_db()
         assert self.collection1.modified == modified_time
 
     def test_add_to_collections_again(self):
         """
-        Test that re-adding objects to collections doesn't throw an error.
+        Test that re-adding entities to collections doesn't throw an error.
         """
         modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc)
         with freeze_time(modified_time):
@@ -333,10 +342,10 @@ def test_add_to_collections_again(self):
             )
 
         assert count == 2
-        assert list(self.collection1.contents.all()) == [
+        assert list(self.collection1.entities.all()) == [
             self.published_entity,
         ]
-        assert list(self.collection2.contents.all()) == [
+        assert list(self.collection2.entities.all()) == [
             self.published_entity,
             self.draft_entity,
         ]
@@ -349,7 +358,7 @@ def test_add_to_collections_again(self):
 
     def test_remove_from_collections(self):
         """
-        Test removing objects from collections.
+        Test removing entities from collections.
         """
         # Expect no changes to be made to collection0
         coll0_modified = self.collection0.modified
@@ -369,9 +378,9 @@ def test_remove_from_collections(self):
 
         assert count == 2
 
-        assert not list(self.collection0.contents.all())
-        assert not list(self.collection1.contents.all())
-        assert list(self.collection2.contents.all()) == [
+        assert not list(self.collection0.entities.all())
+        assert not list(self.collection1.entities.all())
+        assert list(self.collection2.entities.all()) == [
             self.draft_entity,
         ]
 
@@ -383,11 +392,12 @@ def test_remove_from_collections(self):
         self.collection2.refresh_from_db()
         assert self.collection2.modified == modified_time
 
-    def test_get_object_collections(self):
+    def test_get_entity_collections(self):
         """
-        Tests fetching the enabled collections which contain a given object.
+        Tests fetching the enabled collections which contain a given entity.
         """
-        collections = collection_api.get_object_collections(
+        collections = collection_api.get_entity_collections(
+            self.learning_package.id,
             self.published_entity.key,
         )
         assert list(collections) == [
@@ -462,10 +472,9 @@ def test_update_collection_empty(self):
                 self.collection.pk,
             )
 
-        # all fields unchanged
-        assert collection.title == self.collection.title
-        assert collection.description == self.collection.description
-        assert collection.modified == self.collection.modified
+        assert collection.title == self.collection.title  # unchanged
+        assert collection.description == self.collection.description  # unchanged
+        assert collection.modified == self.collection.modified  # unchanged
 
     def test_update_collection_not_found(self):
         """

From b723d7906e05b6a78aa618cba99ff330abd14bba Mon Sep 17 00:00:00 2001
From: Jillian Vogel <jill@opencraft.com>
Date: Thu, 29 Aug 2024 11:52:27 +0930
Subject: [PATCH 7/9] refactor: merge get_collections +
 get_learning_package_collections

We ended up not needing a "get all collections" function; we always
fetch them for a given learning package.

So we merged them into a single api.get_collections method that takes a
`learning_package_id` + optional `enabled` (defaults to True).
---
 .../apps/authoring/collections/api.py             | 15 ++-------------
 .../apps/authoring/collections/test_api.py        | 14 ++++++--------
 2 files changed, 8 insertions(+), 21 deletions(-)

diff --git a/openedx_learning/apps/authoring/collections/api.py b/openedx_learning/apps/authoring/collections/api.py
index 171cd88e..86c906e1 100644
--- a/openedx_learning/apps/authoring/collections/api.py
+++ b/openedx_learning/apps/authoring/collections/api.py
@@ -21,7 +21,6 @@
     "create_collection",
     "get_collection",
     "get_collections",
-    "get_learning_package_collections",
     "get_entity_collections",
     "remove_from_collections",
     "update_collection",
@@ -178,23 +177,13 @@ def get_entity_collections(learning_package_id: int, entity_key: str) -> QuerySe
     return entity.collections.filter(enabled=True).order_by("pk")
 
 
-def get_learning_package_collections(learning_package_id: int) -> QuerySet[Collection]:
+def get_collections(learning_package_id: int, enabled: bool | None = True) -> QuerySet[Collection]:
     """
     Get all collections for a given learning package
 
     Only enabled collections are returned
     """
-    return Collection.objects \
-                     .filter(learning_package_id=learning_package_id, enabled=True) \
-                     .select_related("learning_package") \
-                     .order_by('pk')
-
-
-def get_collections(enabled: bool | None = None) -> QuerySet[Collection]:
-    """
-    Get all collections, optionally caller can filter by enabled flag
-    """
-    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/tests/openedx_learning/apps/authoring/collections/test_api.py b/tests/openedx_learning/apps/authoring/collections/test_api.py
index d75fad42..41f13fdc 100644
--- a/tests/openedx_learning/apps/authoring/collections/test_api.py
+++ b/tests/openedx_learning/apps/authoring/collections/test_api.py
@@ -97,11 +97,11 @@ def test_get_collection_not_found(self):
         with self.assertRaises(ObjectDoesNotExist):
             collection_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 = collection_api.get_collections(self.learning_package.id)
         assert list(collections) == [
             self.collection1,
             self.collection2,
@@ -111,18 +111,17 @@ def test_get_invalid_learning_package_collections(self):
         """
         Test getting collections for an invalid learning package should return an empty queryset.
         """
-        collections = collection_api.get_learning_package_collections(12345)
+        collections = collection_api.get_collections(12345)
         assert not list(collections)
 
     def test_get_all_collections(self):
         """
         Test getting all collections.
         """
-        collections = collection_api.get_collections()
+        collections = collection_api.get_collections(self.learning_package.id, enabled=None)
         self.assertQuerySetEqual(collections, [
             self.collection1,
             self.collection2,
-            self.collection3,
             self.disabled_collection,
         ], ordered=True)
 
@@ -130,18 +129,17 @@ def test_get_all_enabled_collections(self):
         """
         Test getting all ENABLED collections.
         """
-        collections = collection_api.get_collections(enabled=True)
+        collections = collection_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 = collection_api.get_collections(self.learning_package.id, enabled=False)
         assert list(collections) == [self.disabled_collection]
 
 

From 2fe2e708fa208a0cfee33805afaa3089f7d70881 Mon Sep 17 00:00:00 2001
From: Jillian Vogel <jill@opencraft.com>
Date: Fri, 30 Aug 2024 11:12:47 +0930
Subject: [PATCH 8/9] refactor: address PR review

API:
* create_collection -- removed entities_qset param, added enabled param
* add/remove entities from a single collection, not a qset
* raise ValidationError if adding entities to a collection with
  mismatching learning packages
* add/remove returns updated collection object (not count)

Tests:
* use authoring API
* simplified data used by test classes
---
 .../apps/authoring/collections/api.py         | 132 +++++-------
 .../apps/authoring/collections/test_api.py    | 190 +++++++-----------
 2 files changed, 131 insertions(+), 191 deletions(-)

diff --git a/openedx_learning/apps/authoring/collections/api.py b/openedx_learning/apps/authoring/collections/api.py
index 86c906e1..46028ca2 100644
--- a/openedx_learning/apps/authoring/collections/api.py
+++ b/openedx_learning/apps/authoring/collections/api.py
@@ -3,13 +3,14 @@
 """
 from __future__ import annotations
 
+from datetime import datetime, timezone
+
+from django.core.exceptions import ValidationError
 from django.db.models import QuerySet
-from django.db.transaction import atomic
-from django.utils import timezone
 
 from ..publishing import api as publishing_api
 from ..publishing.models import PublishableEntity
-from .models import Collection, CollectionPublishableEntity
+from .models import Collection
 
 # The public API that will be re-exported by openedx_learning.apps.authoring.api
 # is listed in the __all__ entries below. Internal helper functions that are
@@ -17,12 +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_collections",
+    "add_to_collection",
     "create_collection",
     "get_collection",
     "get_collections",
     "get_entity_collections",
-    "remove_from_collections",
+    "remove_from_collection",
     "update_collection",
 ]
 
@@ -32,28 +33,18 @@ def create_collection(
     title: str,
     created_by: int | None,
     description: str = "",
-    entities_qset: QuerySet[PublishableEntity] = PublishableEntity.objects.none(),  # default to empty set,
+    enabled: bool = True,
 ) -> Collection:
     """
     Create a new Collection
     """
-
-    with atomic():
-        collection = Collection.objects.create(
-            learning_package_id=learning_package_id,
-            title=title,
-            created_by_id=created_by,
-            description=description,
-        )
-
-        added = add_to_collections(
-            Collection.objects.filter(id=collection.id),
-            entities_qset,
-            created_by,
-        )
-        if added:
-            collection.refresh_from_db()  # fetch updated modified date
-
+    collection = Collection.objects.create(
+        learning_package_id=learning_package_id,
+        title=title,
+        created_by_id=created_by,
+        description=description,
+        enabled=enabled,
+    )
     return collection
 
 
@@ -88,80 +79,63 @@ def update_collection(
     return collection
 
 
-def add_to_collections(
-    collections_qset: QuerySet[Collection],
+def add_to_collection(
+    collection_id: int,
     entities_qset: QuerySet[PublishableEntity],
     created_by: int | None = None,
-) -> int:
+) -> Collection:
     """
-    Adds a QuerySet of PublishableEntities to a QuerySet of Collections.
+    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.
 
-    Records are created in bulk, and so integrity errors are deliberately ignored: they indicate that the entity(s)
-    have already been added to the collection(s).
+    The Collection object's modified date is updated.
 
-    Returns the number of entities added (including any that already exist).
+    Returns the updated Collection object.
     """
-    collection_entities = []
-    entity_ids = entities_qset.values_list("pk", flat=True)
-    collection_ids = collections_qset.values_list("pk", flat=True)
-
-    for collection_id in collection_ids:
-        for entity_id in entity_ids:
-            collection_entities.append(
-                CollectionPublishableEntity(
-                    collection_id=collection_id,
-                    entity_id=entity_id,
-                    created_by_id=created_by,
-                )
+    collection = get_collection(collection_id)
+    learning_package_id = collection.learning_package_id
+
+    # Disallow adding entities outside the collection's learning package
+    for entity in entities_qset.all():
+        if entity.learning_package_id != learning_package_id:
+            raise ValidationError(
+                f"Cannot add entity {entity.pk} in learning package {entity.learning_package_id} to "
+                f"collection {collection_id} in learning package {learning_package_id}."
             )
 
-    created = []
-    if collection_entities:
-        created = CollectionPublishableEntity.objects.bulk_create(
-            collection_entities,
-            ignore_conflicts=True,
-        )
-
-        # Update the modified date for all the provided Collections.
-        # (Ignoring conflicts means we don't know which ones were modified.)
-        collections_qset.update(modified=timezone.now())
+    collection.entities.add(
+        *entities_qset.all(),
+        through_defaults={"created_by_id": created_by},
+    )
+    collection.modified = datetime.now(tz=timezone.utc)
+    collection.save()
 
-    return len(created)
+    return collection
 
 
-def remove_from_collections(
-    collections_qset: QuerySet[Collection],
+def remove_from_collection(
+    collection_id: int,
     entities_qset: QuerySet[PublishableEntity],
-) -> int:
+) -> Collection:
     """
-    Removes a QuerySet of PublishableEntities from a QuerySet of Collections.
+    Removes a QuerySet of PublishableEntities from a Collection.
 
-    PublishableEntities are deleted from each Collection, in bulk.
+    PublishableEntities are deleted (in bulk).
 
-    Collections which had entities removed are marked with modified=now().
+    The Collection's modified date is updated (even if nothing was removed).
 
-    Returns the total number of entities deleted.
+    Returns the updated Collection.
     """
-    total_deleted = 0
-    entity_ids = entities_qset.values_list("pk", flat=True)
-    collection_ids = collections_qset.values_list("pk", flat=True)
-    modified_collection_ids = []
-
-    for collection_id in collection_ids:
-        num_deleted, _ = CollectionPublishableEntity.objects.filter(
-            collection_id=collection_id,
-            entity__in=entity_ids,
-        ).delete()
+    collection = get_collection(collection_id)
 
-        if num_deleted:
-            modified_collection_ids.append(collection_id)
-
-        total_deleted += num_deleted
-
-    # Update the modified date for the affected Collections
-    Collection.objects.filter(id__in=modified_collection_ids).update(modified=timezone.now())
+    collection.entities.remove(*entities_qset.all())
+    collection.modified = datetime.now(tz=timezone.utc)
+    collection.save()
 
-    return total_deleted
+    return collection
 
 
 def get_entity_collections(learning_package_id: int, entity_key: str) -> QuerySet[Collection]:
@@ -181,7 +155,7 @@ def get_collections(learning_package_id: int, enabled: bool | None = True) -> Qu
     """
     Get all collections for a given learning package
 
-    Only enabled collections are returned
+    Enabled collections are returned by default.
     """
     qs = Collection.objects.filter(learning_package_id=learning_package_id)
     if enabled is not None:
diff --git a/tests/openedx_learning/apps/authoring/collections/test_api.py b/tests/openedx_learning/apps/authoring/collections/test_api.py
index 41f13fdc..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,14 @@
 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, CollectionPublishableEntity
-from openedx_learning.apps.authoring.publishing import api as publishing_api
-from openedx_learning.apps.authoring.publishing.models import (
+# 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,
@@ -30,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
@@ -56,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):
@@ -95,30 +100,30 @@ 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_collections(self):
         """
         Test getting all ENABLED collections for a learning package.
         """
-        collections = collection_api.get_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_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(self.learning_package.id, enabled=None)
+        collections = api.get_collections(self.learning_package.id, enabled=None)
         self.assertQuerySetEqual(collections, [
             self.collection1,
             self.collection2,
@@ -129,7 +134,7 @@ def test_get_all_enabled_collections(self):
         """
         Test getting all ENABLED collections.
         """
-        collections = collection_api.get_collections(self.learning_package.id, enabled=True)
+        collections = api.get_collections(self.learning_package.id, enabled=True)
         self.assertQuerySetEqual(collections, [
             self.collection1,
             self.collection2,
@@ -139,7 +144,7 @@ def test_get_all_disabled_collections(self):
         """
         Test getting all DISABLED collections.
         """
-        collections = collection_api.get_collections(self.learning_package.id, enabled=False)
+        collections = api.get_collections(self.learning_package.id, enabled=False)
         assert list(collections) == [self.disabled_collection]
 
 
@@ -158,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,
@@ -176,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",
@@ -186,7 +191,7 @@ def test_create_collection_without_description(self):
         assert collection.enabled
 
 
-class CollectionEntitiesTestCase(CollectionTestCase):
+class CollectionEntitiesTestCase(CollectionsTestCase):
     """
     Test collections that contain publishable entitites.
     """
@@ -194,10 +199,6 @@ class CollectionEntitiesTestCase(CollectionTestCase):
     pe_version: PublishableEntityVersion
     draft_entity: PublishableEntity
     de_version: PublishableEntityVersion
-    collection0: Collection
-    collection1: Collection
-    collection2: Collection
-    disabled_collection: Collection
     user: User  # type: ignore [valid-type]
 
     @classmethod
@@ -213,33 +214,33 @@ def setUpTestData(cls) -> None:
         )
 
         # Make and Publish one PublishableEntity
-        cls.published_entity = publishing_api.create_publishable_entity(
+        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 = publishing_api.create_publishable_entity_version(
+        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,
         )
-        publishing_api.publish_all_drafts(
+        api.publish_all_drafts(
             cls.learning_package.id,
             message="Publish from CollectionTestCase.setUpTestData",
             published_at=cls.now,
         )
 
-        # Leave another PublishableEntity in Draft.
-        cls.draft_entity = publishing_api.create_publishable_entity(
+        # 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 = publishing_api.create_publishable_entity_version(
+        cls.de_version = api.create_publishable_entity_version(
             cls.draft_entity.id,
             version_num=1,
             title="An Entity that we'll keep in Draft 🌴",
@@ -247,49 +248,32 @@ def setUpTestData(cls) -> None:
             created_by=cls.user.id,
         )
 
-        # Create collections with some shared entities
-        cls.collection0 = collection_api.create_collection(
-            cls.learning_package.id,
-            title="Collection Empty",
-            created_by=cls.user.id,
-            description="This collection contains 0 entities",
-        )
-        cls.collection1 = collection_api.create_collection(
-            cls.learning_package.id,
-            title="Collection One",
-            created_by=cls.user.id,
-            description="This collection contains 1 entity",
+        # 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,
             ]),
-        )
-        cls.collection2 = collection_api.create_collection(
-            cls.learning_package.id,
-            title="Collection Two",
             created_by=cls.user.id,
-            description="This collection contains 2 entities",
+        )
+        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 = collection_api.create_collection(
-            cls.learning_package.id,
-            title="Disabled Collection",
-            created_by=cls.user.id,
-            description="This disabled collection contains 1 entity",
+        cls.disabled_collection = api.add_to_collection(
+            cls.disabled_collection.id,
             entities_qset=PublishableEntity.objects.filter(id__in=[
                 cls.published_entity.id,
             ]),
         )
-        cls.disabled_collection.enabled = False
-        cls.disabled_collection.save()
 
     def test_create_collection_entities(self):
         """
         Ensure the collections were pre-populated with the expected publishable entities.
         """
-        assert not list(self.collection0.entities.all())
         assert list(self.collection1.entities.all()) == [
             self.published_entity,
         ]
@@ -297,104 +281,86 @@ def test_create_collection_entities(self):
             self.published_entity,
             self.draft_entity,
         ]
+        assert not list(self.collection3.entities.all())
 
-    def test_add_to_collections(self):
+    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):
-            count = collection_api.add_to_collections(
-                Collection.objects.filter(id__in=[
-                    self.collection1.id,
-                ]),
+            self.collection1 = api.add_to_collection(
+                self.collection1.id,
                 PublishableEntity.objects.filter(id__in=[
                     self.draft_entity.id,
                 ]),
                 created_by=self.user.id,
             )
-        assert count == 1
+
         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
-        self.collection1.refresh_from_db()
         assert self.collection1.modified == modified_time
 
-    def test_add_to_collections_again(self):
+    def test_add_to_collection_again(self):
         """
-        Test that re-adding entities to collections doesn't throw an error.
+        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):
-            count = collection_api.add_to_collections(
-                Collection.objects.filter(id__in=[
-                    self.collection1.id,
-                    self.collection2.id,
-                ]),
+            self.collection2 = api.add_to_collection(
+                self.collection2.id,
                 PublishableEntity.objects.filter(id__in=[
                     self.published_entity.id,
                 ]),
             )
 
-        assert count == 2
-        assert list(self.collection1.entities.all()) == [
-            self.published_entity,
-        ]
         assert list(self.collection2.entities.all()) == [
             self.published_entity,
             self.draft_entity,
         ]
-
-        # Modified dates will have have changed, even though we didn't really add anything
-        self.collection1.refresh_from_db()
-        assert self.collection1.modified == modified_time
-        self.collection2.refresh_from_db()
         assert self.collection2.modified == modified_time
 
-    def test_remove_from_collections(self):
+    def test_add_to_collection_wrong_learning_package(self):
         """
-        Test removing entities from collections.
+        We cannot add entities to a collection from a different learning package.
         """
-        # Expect no changes to be made to collection0
-        coll0_modified = self.collection0.modified
+        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):
-            count = collection_api.remove_from_collections(
-                Collection.objects.filter(id__in=[
-                    self.collection0.id,
-                    self.collection1.id,
-                    self.collection2.id,
-                ]),
+            self.collection2 = api.remove_from_collection(
+                self.collection2.id,
                 PublishableEntity.objects.filter(id__in=[
                     self.published_entity.id,
                 ]),
             )
 
-        assert count == 2
-
-        assert not list(self.collection0.entities.all())
-        assert not list(self.collection1.entities.all())
         assert list(self.collection2.entities.all()) == [
             self.draft_entity,
         ]
-
-        # Check that the modified collections were updated
-        self.collection0.refresh_from_db()
-        assert self.collection0.modified == coll0_modified
-        self.collection1.refresh_from_db()
-        assert self.collection1.modified == modified_time
-        self.collection2.refresh_from_db()
         assert self.collection2.modified == modified_time
 
     def test_get_entity_collections(self):
         """
         Tests fetching the enabled collections which contain a given entity.
         """
-        collections = collection_api.get_entity_collections(
+        collections = api.get_entity_collections(
             self.learning_package.id,
             self.published_entity.key,
         )
@@ -415,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,
@@ -428,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="",
@@ -443,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",
         )
@@ -452,7 +418,7 @@ def test_update_collection_partial(self):
         assert collection.description == self.collection.description  # unchanged
         assert f"{collection}" == f"<Collection> ({self.collection.pk}:New Title)"
 
-        collection = collection_api.update_collection(
+        collection = api.update_collection(
             self.collection.pk,
             description="New description",
         )
@@ -466,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,
             )
 
@@ -479,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")

From e03daaa36b07afd48197ed9b3940686a6e05d206 Mon Sep 17 00:00:00 2001
From: Jillian Vogel <jill@opencraft.com>
Date: Tue, 3 Sep 2024 09:38:19 +0930
Subject: [PATCH 9/9] fix: use fewer queries to discover invalid entities

being added to a collection
---
 openedx_learning/apps/authoring/collections/api.py | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/openedx_learning/apps/authoring/collections/api.py b/openedx_learning/apps/authoring/collections/api.py
index 46028ca2..03988922 100644
--- a/openedx_learning/apps/authoring/collections/api.py
+++ b/openedx_learning/apps/authoring/collections/api.py
@@ -99,12 +99,12 @@ def add_to_collection(
     learning_package_id = collection.learning_package_id
 
     # Disallow adding entities outside the collection's learning package
-    for entity in entities_qset.all():
-        if entity.learning_package_id != learning_package_id:
-            raise ValidationError(
-                f"Cannot add entity {entity.pk} in learning package {entity.learning_package_id} to "
-                f"collection {collection_id} in learning package {learning_package_id}."
-            )
+    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(),