refactor: move set_collection function to make it generic

Author: rpenido

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

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

tests/openedx_learning/apps/authoring/collections/test_api.py

@@ -74,15 +74,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="COL4",
+            created_by=None,
+            title="Collection 4",
+            description="Description of Collection 4",
+        )
         cls.disabled_collection = api.create_collection(
             cls.learning_package.id,
-            key="COL4",
+            key="another_library",
             created_by=None,
             title="Disabled Collection",
             description="Description of Disabled Collection",
@@ -113,7 +120,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 +130,7 @@ def test_get_collections(self):
         assert list(collections) == [
             self.collection1,
             self.collection2,
+            self.collection3,
         ]
 
     def test_get_invalid_collections(self):
@@ -140,6 +148,7 @@ def test_get_all_collections(self):
         self.assertQuerySetEqual(collections, [
             self.collection1,
             self.collection2,
+            self.collection3,
             self.disabled_collection,
         ], ordered=True)
 
@@ -151,6 +160,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 +311,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 +363,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 +416,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 +588,96 @@ def test_restore(self):
             self.published_component.publishable_entity,
             self.draft_component.publishable_entity,
         ]
+
+
+class SetCollectionsTestCase(CollectionEntitiesTestCase):
+    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 == 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,
+        ]