From ca8a355c3958b86f8f103bdbe0b3f97855c54313 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=B4mulo=20Penido?= <romulo.penido@gmail.com>
Date: Tue, 8 Apr 2025 16:19:59 -0300
Subject: [PATCH 1/5] refactor: move set_collection function to make it generic

---
 .../apps/authoring/collections/api.py         |  55 +++++++-
 .../apps/authoring/components/api.py          |  56 +-------
 .../apps/authoring/collections/test_api.py    | 122 +++++++++++++++++-
 .../apps/authoring/components/test_api.py     |  78 +----------
 4 files changed, 174 insertions(+), 137 deletions(-)

diff --git a/openedx_learning/apps/authoring/collections/api.py b/openedx_learning/apps/authoring/collections/api.py
index 07b7a2eaf..e0bd0dc58 100644
--- a/openedx_learning/apps/authoring/collections/api.py
+++ b/openedx_learning/apps/authoring/collections/api.py
@@ -10,7 +10,7 @@
 
 from ..publishing import api as publishing_api
 from ..publishing.models import PublishableEntity
-from .models import Collection
+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
@@ -27,6 +27,7 @@
     "remove_from_collection",
     "restore_collection",
     "update_collection",
+    "set_collections",
 ]
 
 
@@ -204,3 +205,55 @@ def get_collections(learning_package_id: int, enabled: bool | None = True) -> Qu
     if enabled is not None:
         qs = qs.filter(enabled=enabled)
     return qs.select_related("learning_package").order_by('pk')
+
+
+def set_collections(
+    learning_package_id: int,
+    publishable_entity: PublishableEntity,
+    collection_qset: QuerySet[Collection],
+    created_by: int | None = None,
+) -> set[Collection]:
+    """
+    Set collections for a given publishable entity.
+
+    These Collections must belong to the same LearningPackage as the PublishableEntity,
+    or a ValidationError will be raised.
+
+    Modified date of all collections related to entity is updated.
+
+    Returns the updated collections.
+    """
+    # Disallow adding entities outside the collection's learning package
+    invalid_collection = collection_qset.exclude(learning_package_id=learning_package_id).first()
+    if invalid_collection:
+        raise ValidationError(
+            f"Cannot add collection {invalid_collection.pk} in learning package  "
+            f"{invalid_collection.learning_package_id} to entity {publishable_entity} in "
+            f"learning package {learning_package_id}."
+        )
+    current_relations = CollectionPublishableEntity.objects.filter(
+        entity=publishable_entity
+    ).select_related('collection')
+    # Clear other collections for given entity and add only new collections from collection_qset
+    removed_collections = set(
+        r.collection for r in current_relations.exclude(collection__in=collection_qset)
+    )
+    new_collections = set(collection_qset.exclude(
+        id__in=current_relations.values_list('collection', flat=True)
+    ))
+    # Use `remove` instead of `CollectionPublishableEntity.delete()` to trigger m2m_changed signal which will handle
+    # updating entity index.
+    publishable_entity.collections.remove(*removed_collections)
+    publishable_entity.collections.add(
+        *new_collections,
+        through_defaults={"created_by_id": created_by},
+    )
+    # Update modified date via update to avoid triggering post_save signal for collections
+    # The signal triggers index update for each collection synchronously which will be very slow in this case.
+    # Instead trigger the index update in the caller function asynchronously.
+    affected_collection = removed_collections | new_collections
+    Collection.objects.filter(
+        id__in=[collection.id for collection in affected_collection]
+    ).update(modified=datetime.now(tz=timezone.utc))
+
+    return affected_collection
diff --git a/openedx_learning/apps/authoring/components/api.py b/openedx_learning/apps/authoring/components/api.py
index 1316a1574..4af0e360e 100644
--- a/openedx_learning/apps/authoring/components/api.py
+++ b/openedx_learning/apps/authoring/components/api.py
@@ -13,18 +13,16 @@
 from __future__ import annotations
 
 import mimetypes
-from datetime import datetime, timezone
+from datetime import datetime
 from enum import StrEnum, auto
 from logging import getLogger
 from pathlib import Path
 from uuid import UUID
 
-from django.core.exceptions import ValidationError
 from django.db.models import Q, QuerySet
 from django.db.transaction import atomic
 from django.http.response import HttpResponse, HttpResponseNotFound
 
-from ..collections.models import Collection, CollectionPublishableEntity
 from ..contents import api as contents_api
 from ..publishing import api as publishing_api
 from .models import Component, ComponentType, ComponentVersion, ComponentVersionContent
@@ -51,7 +49,6 @@
     "look_up_component_version_content",
     "AssetError",
     "get_redirect_response_for_component_asset",
-    "set_collections",
 ]
 
 
@@ -605,54 +602,3 @@ def _error_header(error: AssetError) -> dict[str, str]:
     )
 
     return HttpResponse(headers={**info_headers, **redirect_headers})
-
-
-def set_collections(
-    learning_package_id: int,
-    component: Component,
-    collection_qset: QuerySet[Collection],
-    created_by: int | None = None,
-) -> set[Collection]:
-    """
-    Set collections for a given component.
-
-    These Collections must belong to the same LearningPackage as the Component, or a ValidationError will be raised.
-
-    Modified date of all collections related to component is updated.
-
-    Returns the updated collections.
-    """
-    # Disallow adding entities outside the collection's learning package
-    invalid_collection = collection_qset.exclude(learning_package_id=learning_package_id).first()
-    if invalid_collection:
-        raise ValidationError(
-            f"Cannot add collection {invalid_collection.pk} in learning package  "
-            f"{invalid_collection.learning_package_id} to component {component} in "
-            f"learning package {learning_package_id}."
-        )
-    current_relations = CollectionPublishableEntity.objects.filter(
-        entity=component.publishable_entity
-    ).select_related('collection')
-    # Clear other collections for given component and add only new collections from collection_qset
-    removed_collections = set(
-        r.collection for r in current_relations.exclude(collection__in=collection_qset)
-    )
-    new_collections = set(collection_qset.exclude(
-        id__in=current_relations.values_list('collection', flat=True)
-    ))
-    # Use `remove` instead of `CollectionPublishableEntity.delete()` to trigger m2m_changed signal which will handle
-    # updating component index.
-    component.publishable_entity.collections.remove(*removed_collections)
-    component.publishable_entity.collections.add(
-        *new_collections,
-        through_defaults={"created_by_id": created_by},
-    )
-    # Update modified date via update to avoid triggering post_save signal for collections
-    # The signal triggers index update for each collection synchronously which will be very slow in this case.
-    # Instead trigger the index update in the caller function asynchronously.
-    affected_collection = removed_collections | new_collections
-    Collection.objects.filter(
-        id__in=[collection.id for collection in affected_collection]
-    ).update(modified=datetime.now(tz=timezone.utc))
-
-    return affected_collection
diff --git a/tests/openedx_learning/apps/authoring/collections/test_api.py b/tests/openedx_learning/apps/authoring/collections/test_api.py
index a6eb4ffd0..66ba15b8f 100644
--- a/tests/openedx_learning/apps/authoring/collections/test_api.py
+++ b/tests/openedx_learning/apps/authoring/collections/test_api.py
@@ -51,6 +51,7 @@ class CollectionsTestCase(CollectionTestCase):
     collection1: Collection
     collection2: Collection
     collection3: Collection
+    another_library_collection: Collection
     disabled_collection: Collection
 
     @classmethod
@@ -74,15 +75,22 @@ def setUpTestData(cls) -> None:
             description="Description of Collection 2",
         )
         cls.collection3 = api.create_collection(
-            cls.learning_package_2.id,
+            cls.learning_package.id,
             key="COL3",
             created_by=None,
             title="Collection 3",
             description="Description of Collection 3",
         )
+        cls.another_library_collection = api.create_collection(
+            cls.learning_package_2.id,
+            key="another_library",
+            created_by=None,
+            title="Collection 4",
+            description="Description of Collection 4",
+        )
         cls.disabled_collection = api.create_collection(
             cls.learning_package.id,
-            key="COL4",
+            key="disabled_collection",
             created_by=None,
             title="Disabled Collection",
             description="Description of Disabled Collection",
@@ -113,7 +121,7 @@ def test_get_collection_wrong_learning_package(self):
         Test getting a collection that doesn't exist in the requested learning package.
         """
         with self.assertRaises(ObjectDoesNotExist):
-            api.get_collection(self.learning_package.pk, self.collection3.key)
+            api.get_collection(self.learning_package.pk, self.another_library_collection.key)
 
     def test_get_collections(self):
         """
@@ -123,6 +131,7 @@ def test_get_collections(self):
         assert list(collections) == [
             self.collection1,
             self.collection2,
+            self.collection3,
         ]
 
     def test_get_invalid_collections(self):
@@ -140,6 +149,7 @@ def test_get_all_collections(self):
         self.assertQuerySetEqual(collections, [
             self.collection1,
             self.collection2,
+            self.collection3,
             self.disabled_collection,
         ], ordered=True)
 
@@ -151,6 +161,7 @@ def test_get_all_enabled_collections(self):
         self.assertQuerySetEqual(collections, [
             self.collection1,
             self.collection2,
+            self.collection3,
         ], ordered=True)
 
     def test_get_all_disabled_collections(self):
@@ -301,6 +312,7 @@ def test_create_collection_entities(self):
             self.draft_component.publishable_entity,
         ]
         assert not list(self.collection3.entities.all())
+        assert not list(self.another_library_collection.entities.all())
 
     def test_add_to_collection(self):
         """
@@ -352,13 +364,13 @@ def test_add_to_collection_wrong_learning_package(self):
         with self.assertRaises(ValidationError):
             api.add_to_collection(
                 self.learning_package_2.id,
-                self.collection3.key,
+                self.another_library_collection.key,
                 PublishableEntity.objects.filter(id__in=[
                     self.published_component.pk,
                 ]),
             )
 
-        assert not list(self.collection3.entities.all())
+        assert not list(self.another_library_collection.entities.all())
 
     def test_remove_from_collection(self):
         """
@@ -405,6 +417,10 @@ def test_get_collection_components(self):
             self.learning_package.id,
             self.collection3.key,
         ))
+        assert not list(api.get_collection_components(
+            self.learning_package.id,
+            self.another_library_collection.key,
+        ))
 
 
 class UpdateCollectionTestCase(CollectionTestCase):
@@ -573,3 +589,99 @@ def test_restore(self):
             self.published_component.publishable_entity,
             self.draft_component.publishable_entity,
         ]
+
+
+class SetCollectionsTestCase(CollectionEntitiesTestCase):
+    """
+    Test setting multiple collections in a component.
+    """
+    def test_set_collections(self):
+        """
+        Test setting collections in a component
+        """
+        modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc)
+        with freeze_time(modified_time):
+            api.set_collections(
+                self.learning_package.id,
+                self.draft_component.publishable_entity,
+                collection_qset=Collection.objects.filter(id__in=[
+                    self.collection1.pk,
+                    self.collection2.pk,
+                ]),
+                created_by=self.user.id,
+            )
+        assert list(self.collection1.entities.all()) == [
+            self.published_component.publishable_entity,
+            self.draft_component.publishable_entity,
+        ]
+        assert list(self.collection2.entities.all()) == [
+            self.published_component.publishable_entity,
+            self.draft_component.publishable_entity,
+        ]
+
+        for collection_entity in CollectionPublishableEntity.objects.filter(
+            entity=self.draft_component.publishable_entity
+        ):
+            if collection_entity.collection == self.collection1:
+                # The collection1 was newly associated, so it has a created_by
+                assert collection_entity.created_by == self.user
+            else:
+                # The collection2 was already associated, with no created_by
+                assert collection_entity.created_by is None
+
+        # The collection1 was newly associated, so the modified time is set
+        assert Collection.objects.get(id=self.collection1.pk).modified == modified_time
+        # The collection2 was already associated, so the modified time is unchanged
+        assert Collection.objects.get(id=self.collection2.pk).modified != modified_time
+
+        # Set collections again, but this time remove collection1 and add collection3
+        # Expected result: collection2 & collection3 associated to component and collection1 is excluded.
+        new_modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc)
+        with freeze_time(new_modified_time):
+            api.set_collections(
+                self.learning_package.id,
+                self.draft_component.publishable_entity,
+                collection_qset=Collection.objects.filter(id__in=[
+                    self.collection3.pk,
+                    self.collection2.pk,
+                ]),
+                created_by=self.user.id,
+            )
+        assert list(self.collection1.entities.all()) == [
+            self.published_component.publishable_entity,
+        ]
+        assert list(self.collection2.entities.all()) == [
+            self.published_component.publishable_entity,
+            self.draft_component.publishable_entity,
+        ]
+        assert list(self.collection3.entities.all()) == [
+            self.draft_component.publishable_entity,
+        ]
+        # update modified time of all three collections as they were all updated
+        assert Collection.objects.get(id=self.collection1.pk).modified == new_modified_time
+        # collection2 was unchanged, so it should have the same modified time as before
+        assert Collection.objects.get(id=self.collection2.pk).modified != new_modified_time
+        assert Collection.objects.get(id=self.collection3.pk).modified == new_modified_time
+
+    def test_set_collection_wrong_learning_package(self):
+        """
+        We cannot set collections with a different learning package than the component.
+        """
+        learning_package_3 = api.create_learning_package(
+            key="ComponentTestCase-test-key-3",
+            title="Components Test Case Learning Package-3",
+        )
+
+        with self.assertRaises(ValidationError):
+            api.set_collections(
+                learning_package_3.id,
+                self.draft_component.publishable_entity,
+                collection_qset=Collection.objects.filter(id__in=[
+                    self.collection1.pk,
+                ]),
+                created_by=self.user.id,
+            )
+
+        assert list(self.collection1.entities.all()) == [
+            self.published_component.publishable_entity,
+        ]
diff --git a/tests/openedx_learning/apps/authoring/components/test_api.py b/tests/openedx_learning/apps/authoring/components/test_api.py
index ac3902457..6f1656ec9 100644
--- a/tests/openedx_learning/apps/authoring/components/test_api.py
+++ b/tests/openedx_learning/apps/authoring/components/test_api.py
@@ -5,11 +5,10 @@
 
 from django.contrib.auth import get_user_model
 from django.contrib.auth.models import User as UserType  # pylint: disable=imported-auth-user
-from django.core.exceptions import ObjectDoesNotExist, ValidationError
-from freezegun import freeze_time
+from django.core.exceptions import ObjectDoesNotExist
 
 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.collections.models import Collection
 from openedx_learning.apps.authoring.components import api as components_api
 from openedx_learning.apps.authoring.components.models import Component, ComponentType
 from openedx_learning.apps.authoring.contents import api as contents_api
@@ -606,76 +605,3 @@ def setUpTestData(cls) -> None:
             username="user",
             email="user@example.com",
         )
-
-    def test_set_collections(self):
-        """
-        Test setting collections in a component
-        """
-        modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc)
-        with freeze_time(modified_time):
-            components_api.set_collections(
-                self.learning_package.id,
-                self.published_problem,
-                collection_qset=Collection.objects.filter(id__in=[
-                    self.collection1.pk,
-                    self.collection2.pk,
-                ]),
-                created_by=self.user.id,
-            )
-        assert list(self.collection1.entities.all()) == [
-            self.published_problem.publishable_entity,
-        ]
-        assert list(self.collection2.entities.all()) == [
-            self.published_problem.publishable_entity,
-        ]
-        for collection_entity in CollectionPublishableEntity.objects.filter(
-            entity=self.published_problem.publishable_entity
-        ):
-            assert collection_entity.created_by == self.user
-        assert Collection.objects.get(id=self.collection1.pk).modified == modified_time
-        assert Collection.objects.get(id=self.collection2.pk).modified == modified_time
-
-        # Set collections again, but this time remove collection1 and add collection3
-        # Expected result: collection2 & collection3 associated to component and collection1 is excluded.
-        new_modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc)
-        with freeze_time(new_modified_time):
-            components_api.set_collections(
-                self.learning_package.id,
-                self.published_problem,
-                collection_qset=Collection.objects.filter(id__in=[
-                    self.collection3.pk,
-                    self.collection2.pk,
-                ]),
-                created_by=self.user.id,
-            )
-        assert not list(self.collection1.entities.all())
-        assert list(self.collection2.entities.all()) == [
-            self.published_problem.publishable_entity,
-        ]
-        assert list(self.collection3.entities.all()) == [
-            self.published_problem.publishable_entity,
-        ]
-        # update modified time of all three collections as they were all updated
-        assert Collection.objects.get(id=self.collection1.pk).modified == new_modified_time
-        assert Collection.objects.get(id=self.collection2.pk).modified == new_modified_time
-        assert Collection.objects.get(id=self.collection3.pk).modified == new_modified_time
-
-    def test_set_collection_wrong_learning_package(self):
-        """
-        We cannot set collections with a different learning package than the component.
-        """
-        learning_package_2 = publishing_api.create_learning_package(
-            key="ComponentTestCase-test-key-2",
-            title="Components Test Case Learning Package-2",
-        )
-        with self.assertRaises(ValidationError):
-            components_api.set_collections(
-                learning_package_2.id,
-                self.published_problem,
-                collection_qset=Collection.objects.filter(id__in=[
-                    self.collection1.pk,
-                ]),
-                created_by=self.user.id,
-            )
-
-        assert not list(self.collection1.entities.all())

From 66d0d1763ce373a22b4432fa1228d1e0c90e8832 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=B4mulo=20Penido?= <romulo.penido@gmail.com>
Date: Thu, 10 Apr 2025 12:55:15 -0300
Subject: [PATCH 2/5] feat: add collections support for containers

---
 .../apps/authoring/publishing/api.py          | 16 ++++++++
 .../apps/authoring/collections/test_api.py    | 39 ++++++++++++++++++-
 2 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py
index e9641dc14..df4e266bb 100644
--- a/openedx_learning/apps/authoring/publishing/api.py
+++ b/openedx_learning/apps/authoring/publishing/api.py
@@ -74,6 +74,7 @@
     "get_container",
     "get_container_by_key",
     "get_containers",
+    "get_collection_containers",
     "ChildrenEntitiesAction",
     "ContainerEntityListEntry",
     "get_entities_in_container",
@@ -955,6 +956,21 @@ def get_containers(
     return container_cls.objects.filter(publishable_entity__learning_package=learning_package_id)
 
 
+def get_collection_containers(
+    learning_package_id: int,
+    collection_key: str,
+) -> QuerySet[Container]:
+    """
+    Returns a QuerySet of Containers relating to the PublishableEntities in a Collection.
+
+    Containers have a one-to-one relationship with PublishableEntity, but the reverse may not always be true.
+    """
+    return Container.objects.filter(
+        publishable_entity__learning_package_id=learning_package_id,
+        publishable_entity__collections__key=collection_key,
+    ).order_by('pk')
+
+
 @dataclass(frozen=True)
 class ContainerEntityListEntry:
     """
diff --git a/tests/openedx_learning/apps/authoring/collections/test_api.py b/tests/openedx_learning/apps/authoring/collections/test_api.py
index 66ba15b8f..c92c5111c 100644
--- a/tests/openedx_learning/apps/authoring/collections/test_api.py
+++ b/tests/openedx_learning/apps/authoring/collections/test_api.py
@@ -18,6 +18,7 @@
     LearningPackage,
     PublishableEntity,
 )
+from openedx_learning.apps.authoring.units.models import Unit
 from openedx_learning.lib.test_utils import TestCase
 
 User = get_user_model()
@@ -221,10 +222,11 @@ def test_create_collection_without_description(self):
 
 class CollectionEntitiesTestCase(CollectionsTestCase):
     """
-    Base class with collections that contain components.
+    Base class with collections that contain entities.
     """
     published_component: Component
     draft_component: Component
+    draft_unit: Unit
     user: UserType
     html_type: ComponentType
     problem_type: ComponentType
@@ -243,6 +245,13 @@ def setUpTestData(cls) -> None:
 
         cls.html_type = api.get_or_create_component_type("xblock.v1", "html")
         cls.problem_type = api.get_or_create_component_type("xblock.v1", "problem")
+        created_time = datetime(2025, 4, 1, tzinfo=timezone.utc)
+        cls.draft_unit = api.create_unit(
+            learning_package_id=cls.learning_package.id,
+            key="unit-1",
+            created=created_time,
+            created_by=cls.user.id,
+        )
 
         # Make and publish one Component
         cls.published_component, _ = api.create_component_and_version(
@@ -284,6 +293,7 @@ def setUpTestData(cls) -> None:
             entities_qset=PublishableEntity.objects.filter(id__in=[
                 cls.published_component.pk,
                 cls.draft_component.pk,
+                cls.draft_unit.pk,
             ]),
         )
         cls.disabled_collection = api.add_to_collection(
@@ -308,6 +318,7 @@ def test_create_collection_entities(self):
             self.published_component.publishable_entity,
         ]
         assert list(self.collection2.entities.all()) == [
+            self.draft_unit.publishable_entity,
             self.published_component.publishable_entity,
             self.draft_component.publishable_entity,
         ]
@@ -325,11 +336,13 @@ def test_add_to_collection(self):
                 self.collection1.key,
                 PublishableEntity.objects.filter(id__in=[
                     self.draft_component.pk,
+                    self.draft_unit.pk,
                 ]),
                 created_by=self.user.id,
             )
 
         assert list(self.collection1.entities.all()) == [
+            self.draft_unit.publishable_entity,
             self.published_component.publishable_entity,
             self.draft_component.publishable_entity,
         ]
@@ -352,6 +365,7 @@ def test_add_to_collection_again(self):
             )
 
         assert list(self.collection2.entities.all()) == [
+            self.draft_unit.publishable_entity,
             self.published_component.publishable_entity,
             self.draft_component.publishable_entity,
         ]
@@ -383,6 +397,7 @@ def test_remove_from_collection(self):
                 self.collection2.key,
                 PublishableEntity.objects.filter(id__in=[
                     self.published_component.pk,
+                    self.draft_unit.pk,
                 ]),
             )
 
@@ -422,6 +437,24 @@ def test_get_collection_components(self):
             self.another_library_collection.key,
         ))
 
+    def test_get_collection_containers(self):
+        assert not list(api.get_collection_containers(
+            self.learning_package.id,
+            self.collection1.key,
+        ))
+        assert list(api.get_collection_containers(
+            self.learning_package.id,
+            self.collection2.key,
+        )) == [self.draft_unit.container]
+        assert not list(api.get_collection_containers(
+            self.learning_package.id,
+            self.collection3.key,
+        ))
+        assert not list(api.get_collection_containers(
+            self.learning_package.id,
+            self.another_library_collection.key,
+        ))
+
 
 class UpdateCollectionTestCase(CollectionTestCase):
     """
@@ -533,6 +566,7 @@ def test_soft_delete(self):
         assert collection == api.get_collection(self.learning_package.id, collection.key)
         # ...and the collection's entities remain intact.
         assert list(collection.entities.all()) == [
+            self.draft_unit.publishable_entity,
             self.published_component.publishable_entity,
             self.draft_component.publishable_entity,
         ]
@@ -586,6 +620,7 @@ def test_restore(self):
         assert collection == api.get_collection(self.learning_package.id, collection.key)
         # ...and the collection's entities remain intact.
         assert list(collection.entities.all()) == [
+            self.draft_unit.publishable_entity,
             self.published_component.publishable_entity,
             self.draft_component.publishable_entity,
         ]
@@ -615,6 +650,7 @@ def test_set_collections(self):
             self.draft_component.publishable_entity,
         ]
         assert list(self.collection2.entities.all()) == [
+            self.draft_unit.publishable_entity,
             self.published_component.publishable_entity,
             self.draft_component.publishable_entity,
         ]
@@ -651,6 +687,7 @@ def test_set_collections(self):
             self.published_component.publishable_entity,
         ]
         assert list(self.collection2.entities.all()) == [
+            self.draft_unit.publishable_entity,
             self.published_component.publishable_entity,
             self.draft_component.publishable_entity,
         ]

From 50c6a275c4cb8aa1b21cc3ebe9bc431eeac50705 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=B4mulo=20Penido?= <romulo@opencraft.com>
Date: Fri, 11 Apr 2025 20:24:30 -0300
Subject: [PATCH 3/5] fix: apply suggestions from code review

Co-authored-by: Jillian <jill@opencraft.com>
---
 .../apps/authoring/collections/api.py         | 23 ++++++++-----------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/openedx_learning/apps/authoring/collections/api.py b/openedx_learning/apps/authoring/collections/api.py
index e0bd0dc58..595b2cb81 100644
--- a/openedx_learning/apps/authoring/collections/api.py
+++ b/openedx_learning/apps/authoring/collections/api.py
@@ -208,7 +208,6 @@ def get_collections(learning_package_id: int, enabled: bool | None = True) -> Qu
 
 
 def set_collections(
-    learning_package_id: int,
     publishable_entity: PublishableEntity,
     collection_qset: QuerySet[Collection],
     created_by: int | None = None,
@@ -224,12 +223,9 @@ def set_collections(
     Returns the updated collections.
     """
     # Disallow adding entities outside the collection's learning package
-    invalid_collection = collection_qset.exclude(learning_package_id=learning_package_id).first()
-    if invalid_collection:
+    if collection_qset.exclude(learning_package_id=publishable_entity.learning_package_id).count():
         raise ValidationError(
-            f"Cannot add collection {invalid_collection.pk} in learning package  "
-            f"{invalid_collection.learning_package_id} to entity {publishable_entity} in "
-            f"learning package {learning_package_id}."
+            "Collection entities must be from the same learning package as the collection.",
         )
     current_relations = CollectionPublishableEntity.objects.filter(
         entity=publishable_entity
@@ -238,19 +234,18 @@ def set_collections(
     removed_collections = set(
         r.collection for r in current_relations.exclude(collection__in=collection_qset)
     )
+    removed_collections = set(
+        r.collection for r in current_relations.exclude(collection__in=collection_qset)
+    )
     new_collections = set(collection_qset.exclude(
         id__in=current_relations.values_list('collection', flat=True)
     ))
-    # Use `remove` instead of `CollectionPublishableEntity.delete()` to trigger m2m_changed signal which will handle
-    # updating entity index.
-    publishable_entity.collections.remove(*removed_collections)
-    publishable_entity.collections.add(
-        *new_collections,
+    # Triggers a m2m_changed signal
+    publishable_entity.collections.set(
+        objs=new_collections,
         through_defaults={"created_by_id": created_by},
     )
-    # Update modified date via update to avoid triggering post_save signal for collections
-    # The signal triggers index update for each collection synchronously which will be very slow in this case.
-    # Instead trigger the index update in the caller function asynchronously.
+    # Update modified date via update to avoid triggering post_save signal for all collections, which can be very slow.
     affected_collection = removed_collections | new_collections
     Collection.objects.filter(
         id__in=[collection.id for collection in affected_collection]

From 595a03214d4843af1c507ea65672d4b726383ff1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=B4mulo=20Penido?= <romulo.penido@gmail.com>
Date: Fri, 11 Apr 2025 20:51:29 -0300
Subject: [PATCH 4/5] fix: updating tests and fixing code

---
 openedx_learning/apps/authoring/collections/api.py   |  5 +----
 .../apps/authoring/collections/test_api.py           | 12 ++++++++----
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/openedx_learning/apps/authoring/collections/api.py b/openedx_learning/apps/authoring/collections/api.py
index 595b2cb81..e7e2acec1 100644
--- a/openedx_learning/apps/authoring/collections/api.py
+++ b/openedx_learning/apps/authoring/collections/api.py
@@ -234,15 +234,12 @@ def set_collections(
     removed_collections = set(
         r.collection for r in current_relations.exclude(collection__in=collection_qset)
     )
-    removed_collections = set(
-        r.collection for r in current_relations.exclude(collection__in=collection_qset)
-    )
     new_collections = set(collection_qset.exclude(
         id__in=current_relations.values_list('collection', flat=True)
     ))
     # Triggers a m2m_changed signal
     publishable_entity.collections.set(
-        objs=new_collections,
+        objs=collection_qset,
         through_defaults={"created_by_id": created_by},
     )
     # Update modified date via update to avoid triggering post_save signal for all collections, which can be very slow.
diff --git a/tests/openedx_learning/apps/authoring/collections/test_api.py b/tests/openedx_learning/apps/authoring/collections/test_api.py
index c92c5111c..b70a8abd3 100644
--- a/tests/openedx_learning/apps/authoring/collections/test_api.py
+++ b/tests/openedx_learning/apps/authoring/collections/test_api.py
@@ -637,7 +637,6 @@ def test_set_collections(self):
         modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc)
         with freeze_time(modified_time):
             api.set_collections(
-                self.learning_package.id,
                 self.draft_component.publishable_entity,
                 collection_qset=Collection.objects.filter(id__in=[
                     self.collection1.pk,
@@ -675,7 +674,6 @@ def test_set_collections(self):
         new_modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc)
         with freeze_time(new_modified_time):
             api.set_collections(
-                self.learning_package.id,
                 self.draft_component.publishable_entity,
                 collection_qset=Collection.objects.filter(id__in=[
                     self.collection3.pk,
@@ -708,13 +706,19 @@ def test_set_collection_wrong_learning_package(self):
             key="ComponentTestCase-test-key-3",
             title="Components Test Case Learning Package-3",
         )
+        collection = api.create_collection(
+            learning_package_3.id,
+            key="MYCOL",
+            title="My Collection",
+            created_by=None,
+            description="Description of Collection",
+        )
 
         with self.assertRaises(ValidationError):
             api.set_collections(
-                learning_package_3.id,
                 self.draft_component.publishable_entity,
                 collection_qset=Collection.objects.filter(id__in=[
-                    self.collection1.pk,
+                    collection.pk,
                 ]),
                 created_by=self.user.id,
             )

From 2b9f0192d34a57eebbca6aa49629f5f8f16f116d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=B4mulo=20Penido?= <romulo.penido@gmail.com>
Date: Sat, 12 Apr 2025 01:52:31 -0300
Subject: [PATCH 5/5] chore: bump version

---
 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 ad6efd51d..c15af56f2 100644
--- a/openedx_learning/__init__.py
+++ b/openedx_learning/__init__.py
@@ -2,4 +2,4 @@
 Open edX Learning ("Learning Core").
 """
 
-__version__ = "0.21.0"
+__version__ = "0.22.0"