From 45a45d8e95079d3d751680827546cb510cbe1113 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 8 May 2026 11:27:42 -0700 Subject: [PATCH 1/6] fix: don't emit COLLECTION_CHANGED when entities restored into deleted collections --- .../applets/collections/tasks.py | 1 + .../applets/collections/test_signals.py | 28 +++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/src/openedx_content/applets/collections/tasks.py b/src/openedx_content/applets/collections/tasks.py index 6b5a43d9e..b0f822d95 100644 --- a/src/openedx_content/applets/collections/tasks.py +++ b/src/openedx_content/applets/collections/tasks.py @@ -36,6 +36,7 @@ def emit_collections_changed_for_entity_changes_task( 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") ) diff --git a/tests/openedx_content/applets/collections/test_signals.py b/tests/openedx_content/applets/collections/test_signals.py index 7c5c5fab4..c7bed1446 100644 --- a/tests/openedx_content/applets/collections/test_signals.py +++ b/tests/openedx_content/applets/collections/test_signals.py @@ -534,6 +534,34 @@ def test_entity_draft_restored_multiple_collections(lp1: LearningPackage) -> Non ) +def test_entity_draft_restored_skips_soft_deleted_collections(lp1: LearningPackage) -> None: + """ + Test that COLLECTION_CHANGED is NOT emitted for soft-deleted collections + when an entity's draft is restored, even if the entity is still a member + of those collections. + """ + col1 = api.create_collection(lp1.id, "col1", title="Collection 1", created_by=None) + api.create_collection(lp1.id, "col2-deleted", title="Collection 2", created_by=None) + entity = _create_entity_with_version(lp1.id, "entity1") + api.add_to_collection(lp1.id, "col1", PublishableEntity.objects.filter(id=entity.id)) + api.add_to_collection(lp1.id, "col2-deleted", PublishableEntity.objects.filter(id=entity.id)) + api.soft_delete_draft(entity.id) + api.delete_collection(lp1.id, "col2-deleted") # soft-delete col2 + + with capture_events(signals=[COLLECTION_CHANGED], expected_count=1) as captured: + api.create_publishable_entity_version( + entity.id, version_num=2, title="entity1 v2", created=now_time, created_by=None + ) + + # Only the still-enabled collection (col1) should receive an event. + event = captured[0] + assert event.kwargs["change"] == CollectionChangeData( + collection_id=col1.id, + collection_code="col1", + entities_added=[entity.id], + ) + + def test_entity_draft_restored_aborted(lp1: LearningPackage) -> None: """ Test that no COLLECTION_CHANGED is emitted when the From 825dc4f8d2090ea248085532949bf7f3562182c7 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 8 May 2026 14:32:02 -0700 Subject: [PATCH 2/6] test: add two tests for problematic collections events --- .../applets/collections/test_signals.py | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/tests/openedx_content/applets/collections/test_signals.py b/tests/openedx_content/applets/collections/test_signals.py index c7bed1446..8b5780bda 100644 --- a/tests/openedx_content/applets/collections/test_signals.py +++ b/tests/openedx_content/applets/collections/test_signals.py @@ -4,6 +4,7 @@ from datetime import datetime, timezone +from django.db import transaction import pytest from openedx_content import api @@ -588,3 +589,55 @@ def test_entity_created_no_collection_event(lp1: LearningPackage) -> None: """ with capture_events(signals=[COLLECTION_CHANGED], expected_count=0): _create_entity_with_version(lp1.id, "entity1") + + +# COLLECTION_CHANGED — combined events + + +def test_entity_created_and_assigned_in_bulk_context(lp1: LearningPackage) -> None: + """ + Test that no redundant events are emitted when an entity is created and + assigned to a collection in a bulk draft change context: a brand-new entity + can't have been in any prior collection, so the ENTITIES_DRAFT_CHANGED + handler must not emit an extra COLLECTION_CHANGED that just duplicates the + one ``add_to_collection`` already queues. + """ + with capture_events(signals=[COLLECTION_CHANGED], expected_count=2) as captured: + with api.bulk_draft_changes_for(lp1.id, changed_by=None, changed_at=now_time): + col1 = api.create_collection(lp1.id, "col1", title="Collection 1", created_by=None) + entity = _create_entity_with_version(lp1.id, "entity1") + api.add_to_collection(lp1.id, "col1", PublishableEntity.objects.filter(id=entity.id)) + + assert captured[0].kwargs["change"] == CollectionChangeData( + collection_id=col1.id, + collection_code="col1", + created=True, + ) + assert captured[1].kwargs["change"] == CollectionChangeData( + collection_id=col1.id, + collection_code="col1", + entities_added=[entity.id], + ) + + +def test_entity_created_and_assigned_in_transaction(lp1: LearningPackage) -> None: + """ + Test that no redundant events are emitted when an entity is created and + assigned to a collection in a single database transaction. + """ + with capture_events(signals=[COLLECTION_CHANGED], expected_count=2) as captured: + with transaction.atomic(): + col1 = api.create_collection(lp1.id, "col1", title="Collection 1", created_by=None) + entity = _create_entity_with_version(lp1.id, "entity1") + api.add_to_collection(lp1.id, "col1", PublishableEntity.objects.filter(id=entity.id)) + + assert captured[0].kwargs["change"] == CollectionChangeData( + collection_id=col1.id, + collection_code="col1", + created=True, + ) + assert captured[1].kwargs["change"] == CollectionChangeData( + collection_id=col1.id, + collection_code="col1", + entities_added=[entity.id], + ) From 7903aec91d369e825988b7b53f6196eeb2754179 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 8 May 2026 14:53:50 -0700 Subject: [PATCH 3/6] feat: distinguish between 'restored' and 'created' entities in entities changed event Fixes a bug with duplicate events being emitted for collection changes. Co-Authored-By: Claude --- .../applets/collections/signal_handlers.py | 15 +++++-------- src/openedx_content/applets/publishing/api.py | 22 ++++++++++++++++++- .../applets/publishing/signals.py | 9 ++++++++ 3 files changed, 35 insertions(+), 11 deletions(-) diff --git a/src/openedx_content/applets/collections/signal_handlers.py b/src/openedx_content/applets/collections/signal_handlers.py index 951840d4c..762710168 100644 --- a/src/openedx_content/applets/collections/signal_handlers.py +++ b/src/openedx_content/applets/collections/signal_handlers.py @@ -22,16 +22,11 @@ def on_entities_changed( collections that contain the changed entities. """ removed_entity_ids = [record.entity_id for record in change_log.changes if record.new_version_id is None] - # old_version_id=None covers both brand-new entities and restored soft-deletes; we can't distinguish - # them here without a DB query. The task is a no-op for new entities (not yet in any collection). - # TODO: if ChangeLogRecordData gains a 'restored' flag, filter to only restored entities here. - # (Newly-created entities cannot be part of collections yet, so we only care about entities that - # were previously in collections, then deleted and then restored.) - added_entity_ids = [ - record.entity_id - for record in change_log.changes - if record.old_version_id is None and record.new_version_id is not None - ] + # We only care about *restored* entities (un-soft-deleted) here. Brand-new entities also have + # old_version_id=None, but they cannot be in any collection at the moment they're created, so + # they would be a no-op for the task — and emitting on them produces a redundant event when + # the same transaction also explicitly adds the new entity to a collection. + added_entity_ids = [record.entity_id for record in change_log.changes if record.restored] if not removed_entity_ids and not added_entity_ids: return diff --git a/src/openedx_content/applets/publishing/api.py b/src/openedx_content/applets/publishing/api.py index 7b886a3d3..67c4e61f1 100644 --- a/src/openedx_content/applets/publishing/api.py +++ b/src/openedx_content/applets/publishing/api.py @@ -1268,6 +1268,25 @@ def _emit_event_for_change_log( learning_package_id = change_log.learning_package.id learning_package_title = change_log.learning_package.title + records = 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 +1295,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 diff --git a/src/openedx_content/applets/publishing/signals.py b/src/openedx_content/applets/publishing/signals.py index 148c8c17d..885741f65 100644 --- a/src/openedx_content/applets/publishing/signals.py +++ b/src/openedx_content/applets/publishing/signals.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: From 86b419397a8c7f15b591198e8239fbca635e830e Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 8 May 2026 15:00:22 -0700 Subject: [PATCH 4/6] fix: don't allow adding entities to a deleted collection (likely mistake) Co-Authored-By: Claude --- .../applets/collections/api.py | 5 +++ .../applets/collections/test_api.py | 31 ++++++++++++++----- 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/src/openedx_content/applets/collections/api.py b/src/openedx_content/applets/collections/api.py index b0b4a43f1..020fd9b08 100644 --- a/src/openedx_content/applets/collections/api.py +++ b/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}) diff --git a/tests/openedx_content/applets/collections/test_api.py b/tests/openedx_content/applets/collections/test_api.py index 76d234dc2..736de2f95 100644 --- a/tests/openedx_content/applets/collections/test_api.py +++ b/tests/openedx_content/applets/collections/test_api.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. From c953e28187734e9a5cc2f47271d757df5e29e365 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 8 May 2026 15:37:48 -0700 Subject: [PATCH 5/6] test: add an even more difficult edge case --- .../applets/collections/test_signals.py | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/tests/openedx_content/applets/collections/test_signals.py b/tests/openedx_content/applets/collections/test_signals.py index 8b5780bda..73ad10ce8 100644 --- a/tests/openedx_content/applets/collections/test_signals.py +++ b/tests/openedx_content/applets/collections/test_signals.py @@ -641,3 +641,30 @@ def test_entity_created_and_assigned_in_transaction(lp1: LearningPackage) -> Non collection_code="col1", entities_added=[entity.id], ) + + +def test_entity_restored_and_assigned_in_bulk_context(lp1: LearningPackage) -> None: + """ + Test that no redundant events are emitted when an entity is undeleted and + assigned to a new collection in a bulk draft change context. + """ + entity = _create_entity_with_version(lp1.id, "entity1") + v1 = api.get_draft_version(entity) + api.soft_delete_draft(entity.id, deleted_by=None) + + with capture_events(signals=[COLLECTION_CHANGED], expected_count=2) as captured: + with api.bulk_draft_changes_for(lp1.id, changed_by=None, changed_at=now_time): + api.set_draft_version(entity.id, v1.id) + col1 = api.create_collection(lp1.id, "col1", title="Collection 1", created_by=None) + api.add_to_collection(lp1.id, "col1", PublishableEntity.objects.filter(id=entity.id)) + + assert captured[0].kwargs["change"] == CollectionChangeData( + collection_id=col1.id, + collection_code="col1", + created=True, + ) + assert captured[1].kwargs["change"] == CollectionChangeData( + collection_id=col1.id, + collection_code="col1", + entities_added=[entity.id], + ) From e8274ffc8a3fc941df859ba9e9f9a3dfbb135a88 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 8 May 2026 16:27:28 -0700 Subject: [PATCH 6/6] feat!: COLLECTION_CHANGED ignores changes to entity draft state, like soft deletes Co-Authored-By: Claude --- .../applets/collections/signal_handlers.py | 41 ---- .../applets/collections/signals.py | 7 + .../applets/collections/tasks.py | 84 ------- src/openedx_content/apps.py | 1 - .../applets/collections/test_signals.py | 222 +++--------------- 5 files changed, 40 insertions(+), 315 deletions(-) delete mode 100644 src/openedx_content/applets/collections/signal_handlers.py delete mode 100644 src/openedx_content/applets/collections/tasks.py diff --git a/src/openedx_content/applets/collections/signal_handlers.py b/src/openedx_content/applets/collections/signal_handlers.py deleted file mode 100644 index 762710168..000000000 --- a/src/openedx_content/applets/collections/signal_handlers.py +++ /dev/null @@ -1,41 +0,0 @@ -"""Signal handlers for collections-related updates.""" - -from functools import partial - -from django.db import transaction -from django.dispatch import receiver - -from ..publishing.signals import ENTITIES_DRAFT_CHANGED, DraftChangeLogEventData, UserAttributionEventData -from .tasks import emit_collections_changed_for_entity_changes_task - - -@receiver(ENTITIES_DRAFT_CHANGED) -def on_entities_changed( - change_log: DraftChangeLogEventData, - changed_by: UserAttributionEventData, - **kwargs, -): - """ - When entity drafts are deleted or restored, notify affected collections. - - Dispatches a task to emit COLLECTION_CHANGED for any - collections that contain the changed entities. - """ - removed_entity_ids = [record.entity_id for record in change_log.changes if record.new_version_id is None] - # We only care about *restored* entities (un-soft-deleted) here. Brand-new entities also have - # old_version_id=None, but they cannot be in any collection at the moment they're created, so - # they would be a no-op for the task — and emitting on them produces a redundant event when - # the same transaction also explicitly adds the new entity to a collection. - added_entity_ids = [record.entity_id for record in change_log.changes if record.restored] - - if not removed_entity_ids and not added_entity_ids: - return - - transaction.on_commit( - partial( - emit_collections_changed_for_entity_changes_task.delay, - removed_entity_ids=removed_entity_ids, - added_entity_ids=added_entity_ids, - user_id=changed_by.user_id, - ) - ) diff --git a/src/openedx_content/applets/collections/signals.py b/src/openedx_content/applets/collections/signals.py index 701910239..dc8beae59 100644 --- a/src/openedx_content/applets/collections/signals.py +++ b/src/openedx_content/applets/collections/signals.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 diff --git a/src/openedx_content/applets/collections/tasks.py b/src/openedx_content/applets/collections/tasks.py deleted file mode 100644 index b0f822d95..000000000 --- a/src/openedx_content/applets/collections/tasks.py +++ /dev/null @@ -1,84 +0,0 @@ -"""Celery tasks for the collections applet.""" - -import logging -from collections import defaultdict - -from celery import shared_task # type: ignore[import] - -from ..publishing.models import PublishableEntity -from .models import Collection, CollectionPublishableEntity -from .signals import COLLECTION_CHANGED, CollectionChangeData, LearningPackageEventData, UserAttributionEventData - -logger = logging.getLogger(__name__) - - -@shared_task -def emit_collections_changed_for_entity_changes_task( - removed_entity_ids: list[int], - added_entity_ids: list[int], - user_id: int | None, -) -> int: - """ - Emit COLLECTION_CHANGED for each collection affected by entity draft - deletions or restorations. - - For each collection that contains any of the given entities, emits one event - with entities_removed (for deletions) and/or entities_added (for - restorations). A single event covers both if the same collection has - entities in both lists. - - Triggered by ENTITIES_DRAFT_CHANGED. New entities (old_version_id=None, - new_version_id is not None) that aren't in any collection result in a no-op. - """ - all_entity_ids = list(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") - ) - - collection_map: dict[int, Collection] = {} - removed_map: dict[int, list[PublishableEntity.ID]] = defaultdict(list) - added_map: dict[int, list[PublishableEntity.ID]] = defaultdict(list) - removed_set = set(removed_entity_ids) - added_set = set(added_entity_ids) - - for cpe in affected_cpes: - collection_map[cpe.collection_id] = cpe.collection - if cpe.entity_id in removed_set: - removed_map[cpe.collection_id].append(cpe.entity_id) - if cpe.entity_id in added_set: - added_map[cpe.collection_id].append(cpe.entity_id) - - emitted_events = 0 - for collection_id, collection in collection_map.items(): - # .. event_implemented_name: COLLECTION_CHANGED - # .. event_type: org.openedx.content.collections.collection_changed.v1 - COLLECTION_CHANGED.send_event( - time=collection.modified, - learning_package=LearningPackageEventData( - id=collection.learning_package.id, - title=collection.learning_package.title, - ), - changed_by=UserAttributionEventData(user_id=user_id), - change=CollectionChangeData( - collection_id=collection.id, - collection_code=collection.collection_code, - entities_removed=removed_map[collection_id], - entities_added=added_map[collection_id], - ), - ) - emitted_events += 1 - - if emitted_events: - logger.info( - "Entity draft changes (removed=%s, added=%s): emitted COLLECTION_CHANGED for %s collections.", - removed_entity_ids, - added_entity_ids, - emitted_events, - ) - return emitted_events diff --git a/src/openedx_content/apps.py b/src/openedx_content/apps.py index 9f2e74f2d..4923d1d83 100644 --- a/src/openedx_content/apps.py +++ b/src/openedx_content/apps.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 diff --git a/tests/openedx_content/applets/collections/test_signals.py b/tests/openedx_content/applets/collections/test_signals.py index 73ad10ce8..e18e7290f 100644 --- a/tests/openedx_content/applets/collections/test_signals.py +++ b/tests/openedx_content/applets/collections/test_signals.py @@ -386,7 +386,7 @@ def test_set_collections_aborted(lp1: LearningPackage) -> None: api.set_collections(entity, Collection.objects.filter(id=col1.id)) -# COLLECTION_CHANGED — on entity draft deletion +# COLLECTION_CHANGED — entity draft state changes do NOT emit def _create_entity_with_version(learning_package_id: LearningPackage.ID, entity_ref: str) -> PublishableEntity: @@ -398,197 +398,38 @@ def _create_entity_with_version(learning_package_id: LearningPackage.ID, entity_ return entity -def test_entity_draft_deleted_in_collection(lp1: LearningPackage, admin_user) -> None: +def test_entity_draft_state_changes_do_not_emit_collection_event(lp1: LearningPackage) -> None: """ - Test that COLLECTION_CHANGED is emitted with entities_removed - when an entity's draft is deleted and that entity is in a collection. - """ - collection = api.create_collection(lp1.id, "col1", title="Collection 1", created_by=None) - entity = _create_entity_with_version(lp1.id, "entity1") - api.add_to_collection(lp1.id, "col1", PublishableEntity.objects.filter(id=entity.id)) - - with capture_events(signals=[COLLECTION_CHANGED], expected_count=1) as captured: - api.soft_delete_draft(entity.id, deleted_by=admin_user.id) - - event = captured[0] - assert event.signal is COLLECTION_CHANGED - assert event.kwargs["learning_package"].id == lp1.id - assert event.kwargs["changed_by"].user_id == admin_user.id - assert event.kwargs["change"] == CollectionChangeData( - collection_id=collection.id, - collection_code="col1", - entities_removed=[entity.id], - ) - + COLLECTION_CHANGED reflects explicit mutations of a Collection (its metadata + or its membership rows), not changes to the draft state of entities that + happen to be members. Soft-deleting, restoring, or creating an entity does + NOT emit COLLECTION_CHANGED — even if that entity is in one or more + collections — because the ``CollectionPublishableEntity`` rows themselves + are unchanged. -def test_entity_draft_deleted_multiple_collections(lp1: LearningPackage) -> None: + Consumers that need to react to draft-state changes of entities-in-collections + should subscribe to ``ENTITIES_DRAFT_CHANGED`` directly. """ - Test that COLLECTION_CHANGED is emitted once per collection - when a deleted entity belongs to multiple collections. - """ - col1 = api.create_collection(lp1.id, "col1", title="Collection 1", created_by=None) - col2 = api.create_collection(lp1.id, "col2", title="Collection 2", created_by=None) + api.create_collection(lp1.id, "col1", title="Collection 1", created_by=None) + api.create_collection(lp1.id, "col2", title="Collection 2", created_by=None) entity = _create_entity_with_version(lp1.id, "entity1") + v1 = api.get_draft_version(entity) api.add_to_collection(lp1.id, "col1", PublishableEntity.objects.filter(id=entity.id)) api.add_to_collection(lp1.id, "col2", PublishableEntity.objects.filter(id=entity.id)) - with capture_events(signals=[COLLECTION_CHANGED], expected_count=2) as captured: - api.soft_delete_draft(entity.id) - - events_by_collection = {e.kwargs["change"].collection_id: e for e in captured} - assert set(events_by_collection.keys()) == {col1.id, col2.id} - assert events_by_collection[col1.id].kwargs["change"] == CollectionChangeData( - collection_id=col1.id, - collection_code="col1", - entities_removed=[entity.id], - ) - assert events_by_collection[col2.id].kwargs["change"] == CollectionChangeData( - collection_id=col2.id, - collection_code="col2", - entities_removed=[entity.id], - ) - - -def test_entity_draft_deleted_not_in_collection(lp1: LearningPackage) -> None: - """ - Test that no COLLECTION_CHANGED is emitted when the deleted - entity is not in any collection. - """ - entity = _create_entity_with_version(lp1.id, "entity1") - with capture_events(signals=[COLLECTION_CHANGED], expected_count=0): + # Soft-delete: no event, even though `entity` is in two collections. + api.soft_delete_draft(entity.id) + # Restore via reverting to the previous version: no event. + assert v1 is not None + api.set_draft_version(entity.id, v1.id) + # Soft-delete and restore via a new version: no event. api.soft_delete_draft(entity.id) - - -def test_entity_draft_deleted_aborted(lp1: LearningPackage) -> None: - """ - Test that no COLLECTION_CHANGED is emitted when the - entity-delete transaction is rolled back. - """ - api.create_collection(lp1.id, "col1", title="Collection 1", created_by=None) - entity = _create_entity_with_version(lp1.id, "entity1") - api.add_to_collection(lp1.id, "col1", PublishableEntity.objects.filter(id=entity.id)) - - with capture_events(signals=[COLLECTION_CHANGED], expected_count=0): - with abort_transaction(): - api.soft_delete_draft(entity.id) - - -# COLLECTION_CHANGED — on entity draft restore (deletion reverted) - - -def test_entity_draft_restored_in_collection(lp1: LearningPackage) -> None: - """ - Test that COLLECTION_CHANGED is emitted with entities_added - when a soft-deleted entity's draft is restored while it is in a collection. - """ - collection = api.create_collection(lp1.id, "col1", title="Collection 1", created_by=None) - entity = _create_entity_with_version(lp1.id, "entity1") - api.add_to_collection(lp1.id, "col1", PublishableEntity.objects.filter(id=entity.id)) - api.soft_delete_draft(entity.id) - - with capture_events(signals=[COLLECTION_CHANGED], expected_count=1) as captured: - api.create_publishable_entity_version( - entity.id, version_num=2, title="entity1 v2", created=now_time, created_by=None - ) - - event = captured[0] - assert event.signal is COLLECTION_CHANGED - assert event.kwargs["learning_package"].id == lp1.id - assert event.kwargs["change"] == CollectionChangeData( - collection_id=collection.id, - collection_code="col1", - entities_added=[entity.id], - ) - - -def test_entity_draft_restored_multiple_collections(lp1: LearningPackage) -> None: - """ - Test that COLLECTION_CHANGED is emitted once per collection - when a restored entity belongs to multiple collections. - """ - col1 = api.create_collection(lp1.id, "col1", title="Collection 1", created_by=None) - col2 = api.create_collection(lp1.id, "col2", title="Collection 2", created_by=None) - entity = _create_entity_with_version(lp1.id, "entity1") - api.add_to_collection(lp1.id, "col1", PublishableEntity.objects.filter(id=entity.id)) - api.add_to_collection(lp1.id, "col2", PublishableEntity.objects.filter(id=entity.id)) - original_version = api.get_draft_version(entity) - assert original_version is not None - - api.soft_delete_draft(entity.id) - - with capture_events(signals=[COLLECTION_CHANGED], expected_count=2) as captured: - # Restore the deleted draft to its previous version: - api.set_draft_version(entity.id, original_version.id) - - events_by_collection = {e.kwargs["change"].collection_id: e for e in captured} - assert set(events_by_collection.keys()) == {col1.id, col2.id} - assert events_by_collection[col1.id].kwargs["change"] == CollectionChangeData( - collection_id=col1.id, - collection_code="col1", - entities_added=[entity.id], - ) - assert events_by_collection[col2.id].kwargs["change"] == CollectionChangeData( - collection_id=col2.id, - collection_code="col2", - entities_added=[entity.id], - ) - - -def test_entity_draft_restored_skips_soft_deleted_collections(lp1: LearningPackage) -> None: - """ - Test that COLLECTION_CHANGED is NOT emitted for soft-deleted collections - when an entity's draft is restored, even if the entity is still a member - of those collections. - """ - col1 = api.create_collection(lp1.id, "col1", title="Collection 1", created_by=None) - api.create_collection(lp1.id, "col2-deleted", title="Collection 2", created_by=None) - entity = _create_entity_with_version(lp1.id, "entity1") - api.add_to_collection(lp1.id, "col1", PublishableEntity.objects.filter(id=entity.id)) - api.add_to_collection(lp1.id, "col2-deleted", PublishableEntity.objects.filter(id=entity.id)) - api.soft_delete_draft(entity.id) - api.delete_collection(lp1.id, "col2-deleted") # soft-delete col2 - - with capture_events(signals=[COLLECTION_CHANGED], expected_count=1) as captured: api.create_publishable_entity_version( entity.id, version_num=2, title="entity1 v2", created=now_time, created_by=None ) - - # Only the still-enabled collection (col1) should receive an event. - event = captured[0] - assert event.kwargs["change"] == CollectionChangeData( - collection_id=col1.id, - collection_code="col1", - entities_added=[entity.id], - ) - - -def test_entity_draft_restored_aborted(lp1: LearningPackage) -> None: - """ - Test that no COLLECTION_CHANGED is emitted when the - restore transaction is rolled back. - """ - api.create_collection(lp1.id, "col1", title="Collection 1", created_by=None) - entity = _create_entity_with_version(lp1.id, "entity1") - api.add_to_collection(lp1.id, "col1", PublishableEntity.objects.filter(id=entity.id)) - api.soft_delete_draft(entity.id) - - with capture_events(signals=[COLLECTION_CHANGED], expected_count=0): - with abort_transaction(): - api.create_publishable_entity_version( - entity.id, version_num=2, title="entity1 v2", created=now_time, created_by=None - ) - - -def test_entity_created_no_collection_event(lp1: LearningPackage) -> None: - """ - Test that no COLLECTION_CHANGED is emitted when a brand-new - entity gets its first version — even though the change log also has old_version=None. - - A freshly created entity is never in any collections yet, so the task is a no-op. - """ - with capture_events(signals=[COLLECTION_CHANGED], expected_count=0): - _create_entity_with_version(lp1.id, "entity1") + # Creating a brand-new entity is unrelated to any collection: no event. + _create_entity_with_version(lp1.id, "entity2") # COLLECTION_CHANGED — combined events @@ -596,11 +437,11 @@ def test_entity_created_no_collection_event(lp1: LearningPackage) -> None: def test_entity_created_and_assigned_in_bulk_context(lp1: LearningPackage) -> None: """ - Test that no redundant events are emitted when an entity is created and - assigned to a collection in a bulk draft change context: a brand-new entity - can't have been in any prior collection, so the ENTITIES_DRAFT_CHANGED - handler must not emit an extra COLLECTION_CHANGED that just duplicates the - one ``add_to_collection`` already queues. + Test that the expected events fire when an entity is created and assigned + to a collection inside a bulk draft change context: one ``created=True`` + from ``create_collection`` and one ``entities_added=[entity]`` from + ``add_to_collection``. The entity-creation itself does not emit an extra + COLLECTION_CHANGED. """ with capture_events(signals=[COLLECTION_CHANGED], expected_count=2) as captured: with api.bulk_draft_changes_for(lp1.id, changed_by=None, changed_at=now_time): @@ -622,8 +463,8 @@ def test_entity_created_and_assigned_in_bulk_context(lp1: LearningPackage) -> No def test_entity_created_and_assigned_in_transaction(lp1: LearningPackage) -> None: """ - Test that no redundant events are emitted when an entity is created and - assigned to a collection in a single database transaction. + Same as above, but in a plain ``transaction.atomic()`` context rather than + a ``bulk_draft_changes_for`` context. """ with capture_events(signals=[COLLECTION_CHANGED], expected_count=2) as captured: with transaction.atomic(): @@ -645,8 +486,11 @@ def test_entity_created_and_assigned_in_transaction(lp1: LearningPackage) -> Non def test_entity_restored_and_assigned_in_bulk_context(lp1: LearningPackage) -> None: """ - Test that no redundant events are emitted when an entity is undeleted and - assigned to a new collection in a bulk draft change context. + Test that an entity being restored and added to a new collection inside a + bulk draft change context produces exactly the expected explicit-mutation + events: ``created=True`` for the new collection and + ``entities_added=[entity]`` for the add. The restoration itself does not + emit a COLLECTION_CHANGED. """ entity = _create_entity_with_version(lp1.id, "entity1") v1 = api.get_draft_version(entity)