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/src/openedx_content/applets/collections/signal_handlers.py b/src/openedx_content/applets/collections/signal_handlers.py deleted file mode 100644 index 951840d4c..000000000 --- a/src/openedx_content/applets/collections/signal_handlers.py +++ /dev/null @@ -1,46 +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] - # 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 - ] - - 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 6b5a43d9e..000000000 --- a/src/openedx_content/applets/collections/tasks.py +++ /dev/null @@ -1,83 +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) - .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/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: 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_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. diff --git a/tests/openedx_content/applets/collections/test_signals.py b/tests/openedx_content/applets/collections/test_signals.py index 7c5c5fab4..e18e7290f 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 @@ -385,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: @@ -397,166 +398,117 @@ 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)) + 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. - 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], - ) - - -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) + api.create_publishable_entity_version( + entity.id, version_num=2, title="entity1 v2", created=now_time, created_by=None + ) + # Creating a brand-new entity is unrelated to any collection: no event. + _create_entity_with_version(lp1.id, "entity2") -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) +# COLLECTION_CHANGED — combined events -def test_entity_draft_restored_in_collection(lp1: LearningPackage) -> None: +def test_entity_created_and_assigned_in_bulk_context(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. + 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. """ - 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 - ) + 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)) - 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, + 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_draft_restored_multiple_collections(lp1: LearningPackage) -> None: +def test_entity_created_and_assigned_in_transaction(lp1: LearningPackage) -> None: """ - Test that COLLECTION_CHANGED is emitted once per collection - when a restored entity belongs to multiple collections. + Same as above, but in a plain ``transaction.atomic()`` context rather than + a ``bulk_draft_changes_for`` context. """ - 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) + 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)) - 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( + assert captured[0].kwargs["change"] == CollectionChangeData( collection_id=col1.id, collection_code="col1", - entities_added=[entity.id], + created=True, ) - assert events_by_collection[col2.id].kwargs["change"] == CollectionChangeData( - collection_id=col2.id, - collection_code="col2", + assert captured[1].kwargs["change"] == CollectionChangeData( + collection_id=col1.id, + collection_code="col1", entities_added=[entity.id], ) -def test_entity_draft_restored_aborted(lp1: LearningPackage) -> None: +def test_entity_restored_and_assigned_in_bulk_context(lp1: LearningPackage) -> None: """ - Test that no COLLECTION_CHANGED is emitted when the - restore transaction is rolled back. + 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. """ - 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 - ) - + v1 = api.get_draft_version(entity) + api.soft_delete_draft(entity.id, deleted_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. + 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)) - 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") + 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], + )