feat: improve/correct handling of events and collections (#593)

* fix: don't emit COLLECTION_CHANGED when entities restored into deleted collections
* test: add two tests for problematic collections events
* feat: distinguish between 'restored' and 'created' entities in entities changed event
   (Fixes a bug with duplicate events being emitted for collection changes.)
* fix: don't allow adding entities to a deleted collection (likely mistake)
* test: add an even more difficult edge case
* feat!: COLLECTION_CHANGED ignores changes to entity draft state, like soft deletes
* build: version bump to 1.1.0

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: bradenmacdonald

src/openedx_content/applets/collections/api.py

@@ -202,6 +202,11 @@ def add_to_collection(
         )
 
     collection = get_collection(learning_package_id, collection_code)
+    if not collection.enabled:
+        raise ValidationError(
+            "Cannot add entities to a disabled (soft deleted) collection "
+            f"(collection {collection_code} in learning package {learning_package_id})."
+        )
     existing_ids = set(collection.entities.values_list("id", flat=True))
     ids_to_add = entities_qset.values_list("id", flat=True)
     collection.entities.add(*ids_to_add, through_defaults={"created_by_id": created_by})

src/openedx_content/applets/collections/signal_handlers.py

@@ -52,6 +52,13 @@ class CollectionChangeData:
 information available. It does not distinguish between Containers, Components,
 or other entity types.
 
+⚠️ Collections do NOT participate in draft-publish nor versioning. If an entity
+is added to a collection and then its draft is soft deleted, no
+``COLLECTION_CHANGED`` event will fire, as the entity is still associated with
+the collection regardless of whether its draft or published versions exist. If
+your app cares about this case, you'll also need to subscribe to the
+``ENTITIES_DRAFT_CHANGED`` event.
+
 💾 This event is only emitted after any transaction has been committed.
 
 ⏳ This **batch** event is emitted **synchronously**. Handlers that do anything

src/openedx_content/applets/collections/signals.py

@@ -31,14 +31,14 @@ def emit_collections_changed_for_entity_changes_task(
     new_version_id is not None) that aren't in any collection result in a no-op.
     """
     all_entity_ids: list[PublishableEntity.ID] = [
-        PublishableEntity.PublishableEntityID(x)
-        for x in set(removed_entity_ids) | set(added_entity_ids)
+        PublishableEntity.PublishableEntityID(x) for x in set(removed_entity_ids) | set(added_entity_ids)
     ]
     if not all_entity_ids:
         return 0
 
     affected_cpes = (
         CollectionPublishableEntity.objects.filter(entity_id__in=all_entity_ids)
+        .filter(collection__enabled=True)  # Don't send events for soft-deleted collections
         .select_related("collection__learning_package")
         .order_by("collection_id", "entity_id")
     )

src/openedx_content/applets/collections/tasks.py

@@ -1268,6 +1268,27 @@ def _emit_event_for_change_log(
 
     learning_package_id = change_log.learning_package.id
     learning_package_title = change_log.learning_package.title
+    records: list[DraftChangeLogRecord | PublishLogRecord] = list(
+        change_log.records.order_by("id").select_related("old_version", "new_version").all()
+    )
+
+    # For draft change logs, distinguish "restored" entities (un-soft-delete) from brand-new entities.
+    # Both have old_version_id=None, but a brand-new entity has no DraftChangeLogRecord in any prior
+    # change log, while a restored entity does (its creation, soft-delete, and possibly more).
+    restored_entity_ids: set[int] = set()
+    if isinstance(change_log, DraftChangeLog):
+        candidate_entity_ids = [
+            r.entity_id for r in records if r.old_version_id is None and r.new_version_id is not None
+        ]
+        if candidate_entity_ids:
+            restored_entity_ids = set(
+                DraftChangeLogRecord.objects
+                .filter(entity_id__in=candidate_entity_ids)
+                .exclude(draft_change_log_id=change_log.id)
+                .values_list("entity_id", flat=True)
+                .distinct()
+            )
+
     changes = [
         signals.ChangeLogRecordData(
             entity_id=record.entity_id,
@@ -1276,8 +1297,9 @@ def _emit_event_for_change_log(
             new_version=record.new_version.version_num if record.new_version else None,
             new_version_id=record.new_version_id,
             direct=record.direct if isinstance(record, PublishLogRecord) else None,
+            restored=record.entity_id in restored_entity_ids,
         )
-        for record in change_log.records.order_by("id").select_related("old_version", "new_version").all()
+        for record in records
     ]
 
     change_log_data: signals.DraftChangeLogEventData | signals.PublishLogEventData

src/openedx_content/applets/publishing/api.py

@@ -68,6 +68,15 @@ class ChangeLogRecordData:
     (if applicable/known)
     """
 
+    restored: bool = False
+    """
+    True iff this is a draft change that goes from ``old_version=None`` to a non-None ``new_version`` for an
+    entity that already existed in some prior state (i.e. an un-soft-delete). False for brand-new entities
+    that have never had a Draft before, and for any change where ``old_version`` is not None.
+
+    Only populated for ``ENTITIES_DRAFT_CHANGED``; always False for ``ENTITIES_PUBLISHED``.
+    """
+
 
 @define
 class DraftChangeLogEventData:

src/openedx_content/applets/publishing/signals.py

@@ -49,5 +49,4 @@ def ready(self):
         """
         self.register_publishable_models()
         # Import signal handlers so Django registers all @receiver callbacks.
-        from .applets.collections import signal_handlers  # pylint: disable=unused-import
         from .applets.publishing import signal_handlers as _publishing_signal_handlers

src/openedx_content/apps.py

@@ -6,8 +6,4 @@
 """
 
 # The version for the entire repository
-# DEVELOPERS PLEASE NOTE:
-# - 1.0.x is reserved for `verawood-backports`
-# - Use 1.1.0 (if non-breaking) or 2.0.0 (if breaking) as the next `main` release,
-#   and then delete this comment.
-__version__ = "1.0.1"
+__version__ = "1.1.0"

src/openedx_core/__init__.py

@@ -320,13 +320,6 @@ def setUpTestData(cls) -> None:
                 cls.draft_unit.id,
             ]),
         )
-        cls.disabled_collection = api.add_to_collection(
-            cls.learning_package.id,
-            collection_code=cls.disabled_collection.collection_code,
-            entities_qset=PublishableEntity.objects.filter(id__in=[
-                cls.published_component.id,
-            ]),
-        )
 
 
 class CollectionAddRemoveEntitiesTestCase(CollectionEntitiesTestCase):
@@ -410,6 +403,30 @@ def test_add_to_collection_wrong_learning_package(self):
 
         assert not list(self.another_library_collection.entities.all())
 
+    def test_add_to_soft_deleted_collection(self):
+        """
+        We cannot add entities to a soft-deleted (disabled) collection.
+        """
+        api.delete_collection(
+            self.learning_package.id,
+            collection_code=self.collection1.collection_code,
+        )
+
+        with self.assertRaises(ValidationError):
+            api.add_to_collection(
+                self.learning_package.id,
+                self.collection1.collection_code,
+                PublishableEntity.objects.filter(id__in=[
+                    self.draft_component.id,
+                ]),
+                created_by=self.user.id,
+            )
+
+        # The collection's entities are unchanged.
+        assert list(self.collection1.entities.all()) == [
+            self.published_component.publishable_entity,
+        ]
+
     def test_remove_from_collection(self):
         """
         Test removing entities from a collection.