From 3f936dc37888eeeaef44039a12ef8d43af4f9dd7 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 14 Apr 2026 17:28:02 -0700 Subject: [PATCH 01/30] docs: unrelated fix to outdated comment --- tests/openedx_content/applets/publishing/test_models.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/openedx_content/applets/publishing/test_models.py b/tests/openedx_content/applets/publishing/test_models.py index ccfbd9fec..edd3a0a4d 100644 --- a/tests/openedx_content/applets/publishing/test_models.py +++ b/tests/openedx_content/applets/publishing/test_models.py @@ -27,5 +27,5 @@ class FooEntityVersion(PublishableEntityVersionMixin): # Test typing of PublishableEntity identifiers. pe = PublishableEntity() - assert_type(pe.pk, PublishableEntity.ID) - assert_type(pe.id, PublishableEntity.ID) # `id` should show as deprecated + assert_type(pe.pk, PublishableEntity.ID) # `pk` should show as deprecated + assert_type(pe.id, PublishableEntity.ID) From f20a26b9f0a86e4d6b56919eb2858708479cc952 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 15 Apr 2026 17:06:36 -0700 Subject: [PATCH 02/30] chore: unrelated pylint fix --- src/openedx_tagging/models/base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/openedx_tagging/models/base.py b/src/openedx_tagging/models/base.py index 000d4d338..7957a45df 100644 --- a/src/openedx_tagging/models/base.py +++ b/src/openedx_tagging/models/base.py @@ -421,7 +421,7 @@ def copy(self, taxonomy: Taxonomy) -> Taxonomy: return self - def get_filtered_tags( # pylint: disable=too-many-positional-arguments + def get_filtered_tags( self, depth: int | None = None, parent_tag_value: str | None = None, From 0f39f27a669638f7320563b85b60b6332413fbe9 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 14 Apr 2026 17:28:18 -0700 Subject: [PATCH 03/30] feat: emit event signals when publishable entities are changed. Co-Authored-By: Claude Opus 4.6 --- src/openedx_content/applets/publishing/api.py | 12 +++ .../applets/publishing/signals.py | 45 +++++++++++ .../applets/publishing/test_signals.py | 40 ++++++++++ tests/utils.py | 79 +++++++++++++++++++ 4 files changed, 176 insertions(+) create mode 100644 src/openedx_content/applets/publishing/signals.py create mode 100644 tests/openedx_content/applets/publishing/test_signals.py create mode 100644 tests/utils.py diff --git a/src/openedx_content/applets/publishing/api.py b/src/openedx_content/applets/publishing/api.py index 3bbcec19c..58317c033 100644 --- a/src/openedx_content/applets/publishing/api.py +++ b/src/openedx_content/applets/publishing/api.py @@ -35,6 +35,7 @@ PublishSideEffect, ) from .models.publish_log import Published +from . import signals # The public API that will be re-exported by openedx_content.api # is listed in the __all__ entries below. Internal helper functions that are @@ -685,6 +686,17 @@ def set_draft_version( ) draft.save() _create_side_effects_for_change_log(change_log) + # Send out an event immediately, since this is an isolated change. + # TODO: use transaction.on_commit for this event. + signals.LEARNING_PACKAGE_ENTITIES_CHANGED.send_event( + time=set_at, + learning_package=signals.LearningPackageEventData( + id=learning_package_id, + title="TODO: set me", + ), + changed_by=signals.UserAttributionEventData(user_id=set_by), + change_log=signals.DraftChangeLogEventData(draft_change_log_id=change_log.id), + ) def _add_to_existing_draft_change_log( diff --git a/src/openedx_content/applets/publishing/signals.py b/src/openedx_content/applets/publishing/signals.py new file mode 100644 index 000000000..a210895f4 --- /dev/null +++ b/src/openedx_content/applets/publishing/signals.py @@ -0,0 +1,45 @@ +""" +Low-level events/signals emitted by openedx_content +""" + +from attrs import define +from openedx_events.tooling import OpenEdxPublicSignal # type: ignore[import-untyped] + +from .models.learning_package import LearningPackage + + +@define +class LearningPackageEventData: + """Identifies which learning package an event is associated with.""" + id: LearningPackage.ID + title: str # Since 'id' is not easily human-understandable, we include the title too + +@define +class UserAttributionEventData: + """Identifies which user triggered the event.""" + user_id: int | None + +@define +class DraftChangeLogEventData: + """Summary of a `DraftChangeLog`""" + draft_change_log_id: int + + +LEARNING_PACKAGE_ENTITIES_CHANGED = OpenEdxPublicSignal( + event_type="org.openedx.content.publishing.lp_entities_changed.v1", + data={ + "learning_package": LearningPackageEventData, + "changed_by": UserAttributionEventData, + "change_log": DraftChangeLogEventData, + }, +) +""" +The draft version of one or more entities in a `LearningPackage` has changed. + +This is a low-level batch event. It does not have any course or library context +information available. It does not distinguish between Containers, Components, +or other entity types. + +Collections and tags are not `PublishableEntity`-based, so do not participate in +this event. +""" diff --git a/tests/openedx_content/applets/publishing/test_signals.py b/tests/openedx_content/applets/publishing/test_signals.py new file mode 100644 index 000000000..336757c64 --- /dev/null +++ b/tests/openedx_content/applets/publishing/test_signals.py @@ -0,0 +1,40 @@ +""" +Tests related to the Catalog signal handlers +""" + +from datetime import datetime, timezone + +import pytest + +from openedx_content import api +from openedx_content.applets.publishing.signals import LEARNING_PACKAGE_ENTITIES_CHANGED + +from tests.utils import capture_events + +pytestmark = pytest.mark.django_db +now_time = datetime.now(tz=timezone.utc) + + +def test_unbatched_events() -> None: + """ + Test that LEARNING_PACKAGE_ENTITIES_CHANGED is emitted when we change a + publishable entity. + """ + learning_package = api.create_learning_package(key="lp1", title="Test LP") + + entity = api.create_publishable_entity(learning_package.id, key="entity1", created=now_time, created_by=None) + # create_publishable_entity_version also calls set_draft_version internally, so + with capture_events(expected_count=1) as captured: + v1 = api.create_publishable_entity_version( + entity.id, version_num=1, title="Entity 1 V1", created=now_time, created_by=None + ) + + entity.refresh_from_db() + assert api.get_draft_version(entity.id) == v1 + + event = captured[0] + assert event.signal is LEARNING_PACKAGE_ENTITIES_CHANGED + assert event.kwargs["learning_package"].id == learning_package.id + assert event.kwargs["changed_by"].user_id is None + assert event.kwargs["change_log"].draft_change_log_id > 0 + assert event.kwargs["metadata"].time == now_time diff --git a/tests/utils.py b/tests/utils.py new file mode 100644 index 000000000..f68fa3fb1 --- /dev/null +++ b/tests/utils.py @@ -0,0 +1,79 @@ +""" +Shared testing utilities for openedx-core tests. +""" + +from __future__ import annotations + +from contextlib import contextmanager +from dataclasses import dataclass +from typing import Generator + +from openedx_events.tooling import OpenEdxPublicSignal # type: ignore[import-untyped] + + +@dataclass +class CapturedEvent: + """A single captured event emission.""" + + signal: OpenEdxPublicSignal + kwargs: dict + + +@contextmanager +def capture_events( + signals: list[OpenEdxPublicSignal] | None = None, + expected_count: int | None = None, +) -> Generator[list[CapturedEvent], None, None]: + """ + Context manager that captures Open edX events emitted during the block. + + Args: + signals: Optional list of ``OpenEdxPublicSignal`` instances to monitor. + Defaults to all registered signals (OpenEdxPublicSignal.all_events()). + expected_count: How many events are expected (optional). If specified, + will assert that the resulting list has this length. + + Yields: + list[CapturedEvent]: A list that is populated as each event fires. + Each entry has a ``signal`` attribute and a ``kwargs`` + dict containing the event data (learning_package, + changed_by, etc.) plus ``metadata`` and + ``from_event_bus``. + + Example usage:: + + with capture_events(expected_count=1) as captured: + api.do_something(entity.id, ...) + + assert captured[0].signal is LEARNING_PACKAGE_ENTITIES_CHANGED + assert captured[0].kwargs['learning_package'].id == learning_package.id + """ + if signals is None: + signals = list(OpenEdxPublicSignal.all_events()) + + captured: list[CapturedEvent] = [] + receivers: dict[OpenEdxPublicSignal, object] = {} + + for signal in signals: + + def make_receiver(sig: OpenEdxPublicSignal): + def receiver(sender, **kwargs): + kwargs.pop("signal", None) + captured.append(CapturedEvent(signal=sig, kwargs=kwargs)) + + return receiver + + receiver = make_receiver(signal) + signal.connect(receiver) + receivers[signal] = receiver + + try: + yield captured + finally: + for signal, receiver in receivers.items(): + signal.disconnect(receiver) + + if expected_count is not None: + assert len(captured) == expected_count, ( + f"Expected {expected_count} event(s), got {len(captured)}: {[e.signal for e in captured]}" + ) From 296b74cf8b27355b4735324404466b7fe22265ce Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 15 Apr 2026 16:42:41 -0700 Subject: [PATCH 04/30] feat: emit change log details, don't emit if txn is rolled back --- src/openedx_content/api.py | 5 +- src/openedx_content/api_signals.py | 7 +++ src/openedx_content/applets/publishing/api.py | 55 ++++++++++++++----- .../applets/publishing/signals.py | 37 +++++++++++++ .../applets/containers/test_api.py | 4 +- .../applets/publishing/test_signals.py | 52 ++++++++++++++---- .../applets/sections/test_api.py | 2 +- .../applets/subsections/test_api.py | 2 +- .../openedx_content/applets/units/test_api.py | 2 +- tests/utils.py | 2 +- 10 files changed, 138 insertions(+), 30 deletions(-) create mode 100644 src/openedx_content/api_signals.py diff --git a/src/openedx_content/api.py b/src/openedx_content/api.py index d8c2f0c3b..34ad846dc 100644 --- a/src/openedx_content/api.py +++ b/src/openedx_content/api.py @@ -9,7 +9,10 @@ """ # These wildcard imports are okay because these api modules declare __all__. -# pylint: disable=wildcard-import +# pylint: disable=wildcard-import,unused-import + +# Signals are kept in a separate namespace: +from . import api_signals as signals from .applets.backup_restore.api import * from .applets.collections.api import * from .applets.components.api import * diff --git a/src/openedx_content/api_signals.py b/src/openedx_content/api_signals.py new file mode 100644 index 000000000..185fe51f6 --- /dev/null +++ b/src/openedx_content/api_signals.py @@ -0,0 +1,7 @@ +""" +Signals that are part of the public API of openedx_content +""" + +# These wildcard imports are okay because these api modules declare __all__. +# pylint: disable=wildcard-import +from .applets.publishing.signals import * diff --git a/src/openedx_content/applets/publishing/api.py b/src/openedx_content/applets/publishing/api.py index 58317c033..67775aa3d 100644 --- a/src/openedx_content/applets/publishing/api.py +++ b/src/openedx_content/applets/publishing/api.py @@ -13,10 +13,11 @@ from django.core.exceptions import ObjectDoesNotExist from django.db.models import F, Prefetch, Q, QuerySet -from django.db.transaction import atomic +from django.db.transaction import atomic, on_commit from openedx_django_lib.fields import create_hash_digest +from . import signals from .contextmanagers import DraftChangeLogContext from .models import ( Draft, @@ -35,7 +36,6 @@ PublishSideEffect, ) from .models.publish_log import Published -from . import signals # The public API that will be re-exported by openedx_content.api # is listed in the __all__ entries below. Internal helper functions that are @@ -686,17 +686,8 @@ def set_draft_version( ) draft.save() _create_side_effects_for_change_log(change_log) - # Send out an event immediately, since this is an isolated change. - # TODO: use transaction.on_commit for this event. - signals.LEARNING_PACKAGE_ENTITIES_CHANGED.send_event( - time=set_at, - learning_package=signals.LearningPackageEventData( - id=learning_package_id, - title="TODO: set me", - ), - changed_by=signals.UserAttributionEventData(user_id=set_by), - change_log=signals.DraftChangeLogEventData(draft_change_log_id=change_log.id), - ) + # Send out an event immediately after this database transaction commits, since this is an isolated change. + _emit_event_for_change_log(change_log, time_stamp=set_at, user_id=set_by) def _add_to_existing_draft_change_log( @@ -932,6 +923,44 @@ def _create_side_effects_for_change_log(change_log: DraftChangeLog | PublishLog) update_dependencies_hash_digests_for_log(change_log) +def _emit_event_for_change_log( + change_log: PublishLog | DraftChangeLog, time_stamp: datetime, user_id: int | None +) -> None: + """ + Construct and emit the _CHANGED event when a set of entities is changed or published. + + Works with either `DraftChangeLog` or `PublishLog`. + """ + + learning_package_id = change_log.learning_package.id + learning_package_title = change_log.learning_package.title + changes = [ + signals.ChangeLogRecordData( + entity_id=record.entity_id, + old_version=record.old_version.version_num if record.old_version else None, + new_version=record.new_version.version_num if record.new_version else None, + ) + for record in change_log.records.select_related("old_version", "new_version").all() + ] + + if isinstance(change_log, DraftChangeLog): + signal = signals.LEARNING_PACKAGE_ENTITIES_CHANGED + change_log_data = signals.DraftChangeLogEventData(draft_change_log_id=change_log.id, changes=changes) + else: + raise NotImplementedError + + # Send out an event immediately after this database transaction commits. + def send_change_event(): + signal.send_event( + time=time_stamp, + learning_package=signals.LearningPackageEventData(id=learning_package_id, title=learning_package_title), + changed_by=signals.UserAttributionEventData(user_id=user_id), + change_log=change_log_data, + ) + + on_commit(send_change_event) + + def update_dependencies_hash_digests_for_log( change_log: DraftChangeLog | PublishLog, backfill: bool = False, diff --git a/src/openedx_content/applets/publishing/signals.py b/src/openedx_content/applets/publishing/signals.py index a210895f4..28381b2d4 100644 --- a/src/openedx_content/applets/publishing/signals.py +++ b/src/openedx_content/applets/publishing/signals.py @@ -6,23 +6,60 @@ from openedx_events.tooling import OpenEdxPublicSignal # type: ignore[import-untyped] from .models.learning_package import LearningPackage +from .models.publishable_entity import PublishableEntity + +# Public API available via openedx_content.api +__all__ = [ + "LearningPackageEventData", + "UserAttributionEventData", + "ChangeLogRecordData", + "DraftChangeLogEventData", + "LEARNING_PACKAGE_ENTITIES_CHANGED", +] @define class LearningPackageEventData: """Identifies which learning package an event is associated with.""" + id: LearningPackage.ID title: str # Since 'id' is not easily human-understandable, we include the title too + @define class UserAttributionEventData: """Identifies which user triggered the event.""" + user_id: int | None + +@define +class ChangeLogRecordData: + """A single change that was made to a PublishableEntity""" + + entity_id: PublishableEntity.ID + + old_version: int | None + """ + The old version_num of this entity (not the PublishableEntityVersion ID). + This is None if the entity is newly created. + """ + + new_version: int | None + """ + The new version_num of this entity (not the PublishableEntityVersion ID). + This is None if the entity is now deleted. + """ + + # Future: direct? https://github.com/openedx/openedx-core/pull/539 + + @define class DraftChangeLogEventData: """Summary of a `DraftChangeLog`""" + draft_change_log_id: int + changes: list[ChangeLogRecordData] LEARNING_PACKAGE_ENTITIES_CHANGED = OpenEdxPublicSignal( diff --git a/tests/openedx_content/applets/containers/test_api.py b/tests/openedx_content/applets/containers/test_api.py index 9a5fdb431..d41ab2932 100644 --- a/tests/openedx_content/applets/containers/test_api.py +++ b/tests/openedx_content/applets/containers/test_api.py @@ -355,10 +355,10 @@ def test_create_container_queries(lp: LearningPackage, child_entity1: TestEntity "container_cls": TestContainer, } # The exact numbers here aren't too important - this is just to alert us if anything significant changes. - with django_assert_num_queries(31): + with django_assert_num_queries(33): containers_api.create_container_and_version(lp.id, key="c1", **base_args) # And try with a a container that has children: - with django_assert_num_queries(32): + with django_assert_num_queries(34): containers_api.create_container_and_version(lp.id, key="c2", **base_args, entities=[child_entity1]) diff --git a/tests/openedx_content/applets/publishing/test_signals.py b/tests/openedx_content/applets/publishing/test_signals.py index 336757c64..acf8dec94 100644 --- a/tests/openedx_content/applets/publishing/test_signals.py +++ b/tests/openedx_content/applets/publishing/test_signals.py @@ -5,36 +5,68 @@ from datetime import datetime, timezone import pytest +from django.db import transaction from openedx_content import api -from openedx_content.applets.publishing.signals import LEARNING_PACKAGE_ENTITIES_CHANGED - from tests.utils import capture_events -pytestmark = pytest.mark.django_db +pytestmark = pytest.mark.django_db(transaction=True) now_time = datetime.now(tz=timezone.utc) -def test_unbatched_events() -> None: +class DeliberateRollbackException(Exception): + """Exception used to deliberately cancel and roll back a DB transaction""" + + +def test_single_entity_changed() -> None: """ Test that LEARNING_PACKAGE_ENTITIES_CHANGED is emitted when we change a publishable entity. """ - learning_package = api.create_learning_package(key="lp1", title="Test LP") + learning_package = api.create_learning_package(key="lp1", title="Test LP 📦") entity = api.create_publishable_entity(learning_package.id, key="entity1", created=now_time, created_by=None) - # create_publishable_entity_version also calls set_draft_version internally, so + + NEW_VERSION_NUM = 3 # Just for fun let's use a version number other than 1 + with capture_events(expected_count=1) as captured: v1 = api.create_publishable_entity_version( - entity.id, version_num=1, title="Entity 1 V1", created=now_time, created_by=None + entity.id, version_num=NEW_VERSION_NUM, title="Entity 1 V3", created=now_time, created_by=None ) entity.refresh_from_db() assert api.get_draft_version(entity.id) == v1 - event = captured[0] - assert event.signal is LEARNING_PACKAGE_ENTITIES_CHANGED + # Because only one change (create_..._version) has affected this version, it's easy for us to get its DraftChangeLog + expected_draft_change_log_id = v1.draftchangelogrecord_set.get().draft_change_log_id + + event = captured[0] # capture_events(...) context manager already asserted there's only one event. + assert event.signal is api.signals.LEARNING_PACKAGE_ENTITIES_CHANGED assert event.kwargs["learning_package"].id == learning_package.id + assert event.kwargs["learning_package"].title == "Test LP 📦" assert event.kwargs["changed_by"].user_id is None - assert event.kwargs["change_log"].draft_change_log_id > 0 + assert event.kwargs["change_log"].draft_change_log_id == expected_draft_change_log_id + assert event.kwargs["change_log"].changes == [ + api.signals.ChangeLogRecordData(entity_id=entity.id, old_version=None, new_version=NEW_VERSION_NUM), + ] assert event.kwargs["metadata"].time == now_time + + +def test_single_entity_changed_abort() -> None: + """ + Test that no events are emitted when we roll back a transaction that would have + changed a publishable entity. + """ + learning_package = api.create_learning_package(key="lp1", title="Test LP 📦") + + entity = api.create_publishable_entity(learning_package.id, key="entity1", created=now_time, created_by=None) + + with capture_events(expected_count=0): + try: + with transaction.atomic(): + api.create_publishable_entity_version( + entity.id, version_num=1, title="Entity 1 V1", created=now_time, created_by=None + ) + raise DeliberateRollbackException() + except DeliberateRollbackException: + pass diff --git a/tests/openedx_content/applets/sections/test_api.py b/tests/openedx_content/applets/sections/test_api.py index d3464b6bb..3de23f97b 100644 --- a/tests/openedx_content/applets/sections/test_api.py +++ b/tests/openedx_content/applets/sections/test_api.py @@ -153,7 +153,7 @@ def test_section_queries(self) -> None: """ Test the number of queries needed for each part of the sections API """ - with self.assertNumQueries(37): + with self.assertNumQueries(39): section = self.create_section_with_subsections([self.subsection_1, self.subsection_2_v1]) with self.assertNumQueries(160): content_api.publish_from_drafts( diff --git a/tests/openedx_content/applets/subsections/test_api.py b/tests/openedx_content/applets/subsections/test_api.py index 344951304..f52b965a6 100644 --- a/tests/openedx_content/applets/subsections/test_api.py +++ b/tests/openedx_content/applets/subsections/test_api.py @@ -131,7 +131,7 @@ def test_subsection_queries(self) -> None: """ Test the number of queries needed for each part of the subsections API """ - with self.assertNumQueries(37): + with self.assertNumQueries(39): subsection = self.create_subsection_with_units([self.unit_1, self.unit_1_v1]) with self.assertNumQueries(102): # TODO: this seems high? content_api.publish_from_drafts( diff --git a/tests/openedx_content/applets/units/test_api.py b/tests/openedx_content/applets/units/test_api.py index 944c5d076..c89a0e398 100644 --- a/tests/openedx_content/applets/units/test_api.py +++ b/tests/openedx_content/applets/units/test_api.py @@ -132,7 +132,7 @@ def test_unit_queries(self) -> None: """ Test the number of queries needed for each part of the units API """ - with self.assertNumQueries(35): + with self.assertNumQueries(37): unit = self.create_unit_with_components([self.component_1, self.component_2_v1]) with self.assertNumQueries(48): # TODO: this seems high? content_api.publish_from_drafts( diff --git a/tests/utils.py b/tests/utils.py index f68fa3fb1..0926fea39 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -57,7 +57,7 @@ def capture_events( for signal in signals: def make_receiver(sig: OpenEdxPublicSignal): - def receiver(sender, **kwargs): + def receiver(sender, **kwargs): # pylint: disable=unused-argument kwargs.pop("signal", None) captured.append(CapturedEvent(signal=sig, kwargs=kwargs)) From 88f379126c6d9b797de5ab8cdba0828b6fb126f0 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 15 Apr 2026 17:59:53 -0700 Subject: [PATCH 05/30] feat: emit a bulk change event when multiple entities are edited at once --- src/openedx_content/applets/publishing/api.py | 4 ++ .../applets/publishing/test_signals.py | 57 ++++++++++++++++++- 2 files changed, 60 insertions(+), 1 deletion(-) diff --git a/src/openedx_content/applets/publishing/api.py b/src/openedx_content/applets/publishing/api.py index 67775aa3d..9234b0ebb 100644 --- a/src/openedx_content/applets/publishing/api.py +++ b/src/openedx_content/applets/publishing/api.py @@ -9,6 +9,7 @@ from contextlib import nullcontext from datetime import datetime, timezone +from functools import partial from typing import ContextManager, Optional, cast from django.core.exceptions import ObjectDoesNotExist @@ -1413,11 +1414,14 @@ def bulk_draft_changes_for( with bulk_draft_changes_for(component.learning_package.id): update_one_component(component.learning_package.id, component) """ + if not changed_at: + changed_at = datetime.now(tz=timezone.utc) return DraftChangeLogContext( learning_package_id, changed_at=changed_at, changed_by=changed_by, exit_callbacks=[ _create_side_effects_for_change_log, + partial(_emit_event_for_change_log, time_stamp=changed_at, user_id=changed_by), ] ) diff --git a/tests/openedx_content/applets/publishing/test_signals.py b/tests/openedx_content/applets/publishing/test_signals.py index acf8dec94..0563ec8fc 100644 --- a/tests/openedx_content/applets/publishing/test_signals.py +++ b/tests/openedx_content/applets/publishing/test_signals.py @@ -25,7 +25,9 @@ def test_single_entity_changed() -> None: """ learning_package = api.create_learning_package(key="lp1", title="Test LP 📦") - entity = api.create_publishable_entity(learning_package.id, key="entity1", created=now_time, created_by=None) + # Note: creating an entity does not emit any events until we create a version of that entity. + with capture_events(expected_count=0): + entity = api.create_publishable_entity(learning_package.id, key="entity1", created=now_time, created_by=None) NEW_VERSION_NUM = 3 # Just for fun let's use a version number other than 1 @@ -70,3 +72,56 @@ def test_single_entity_changed_abort() -> None: raise DeliberateRollbackException() except DeliberateRollbackException: pass + + +def test_multiple_entites_changed() -> None: + """ + Test that LEARNING_PACKAGE_ENTITIES_CHANGED is emitted when we change + several publishable entities in a single edit. + """ + learning_package = api.create_learning_package(key="lp1", title="Test LP 📦") + + # Entity 1 will have no initial version: + entity1 = api.create_publishable_entity(learning_package.id, key="entity1", created=now_time, created_by=None) + # Entity 2 will have an initial version: + entity2 = api.create_publishable_entity(learning_package.id, key="entity2", created=now_time, created_by=None) + api.create_publishable_entity_version( + entity2.id, version_num=1, title="Entity 2 V1", created=now_time, created_by=None + ) + # Entity 3 will have an initial version that later gets deleted: + entity3 = api.create_publishable_entity(learning_package.id, key="entity3", created=now_time, created_by=None) + api.create_publishable_entity_version( + entity3.id, version_num=1, title="Entity 3 V1", created=now_time, created_by=None + ) + + with capture_events(expected_count=1) as captured: + with api.bulk_draft_changes_for(learning_package.id, changed_by=None, changed_at=now_time) as draft_change_log: + # Create two versions of entity1: + api.create_publishable_entity_version( + entity1.id, version_num=1, title="Entity 1 V1", created=now_time, created_by=None + ) + api.create_publishable_entity_version( + entity1.id, version_num=2, title="Entity 1 V2", created=now_time, created_by=None + ) + # Create a version 2 of entity 2: + api.create_publishable_entity_version( + entity2.id, version_num=2, title="Entity 2 V2", created=now_time, created_by=None + ) + # Delete entity 3: + api.set_draft_version(entity3.id, None, set_at=now_time, set_by=None) + + event = captured[0] + assert event.signal is api.signals.LEARNING_PACKAGE_ENTITIES_CHANGED + assert event.kwargs["learning_package"].id == learning_package.id + assert event.kwargs["learning_package"].title == "Test LP 📦" + assert event.kwargs["changed_by"].user_id is None + assert event.kwargs["change_log"].draft_change_log_id == draft_change_log.id + assert event.kwargs["change_log"].changes == [ + # Entity 1 jumps from no version to version 2: + api.signals.ChangeLogRecordData(entity_id=entity1.id, old_version=None, new_version=2), + # Entity 2 jumps v1 -> v2: + api.signals.ChangeLogRecordData(entity_id=entity2.id, old_version=1, new_version=2), + # Entity 3 gets deleted: + api.signals.ChangeLogRecordData(entity_id=entity3.id, old_version=1, new_version=None), + ] + assert event.kwargs["metadata"].time == now_time From dc93d731a10a8b11b2c3deee8367031e92a802e8 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 16 Apr 2026 11:52:06 -0700 Subject: [PATCH 06/30] test: update bulk test to verify user_id --- .../applets/publishing/test_signals.py | 40 +++++++++---------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/tests/openedx_content/applets/publishing/test_signals.py b/tests/openedx_content/applets/publishing/test_signals.py index 0563ec8fc..c2b0da2d6 100644 --- a/tests/openedx_content/applets/publishing/test_signals.py +++ b/tests/openedx_content/applets/publishing/test_signals.py @@ -74,47 +74,43 @@ def test_single_entity_changed_abort() -> None: pass -def test_multiple_entites_changed() -> None: +def test_multiple_entites_changed(admin_user) -> None: """ Test that LEARNING_PACKAGE_ENTITIES_CHANGED is emitted when we change several publishable entities in a single edit. """ learning_package = api.create_learning_package(key="lp1", title="Test LP 📦") + created_args = {"created": now_time, "created_by": admin_user.id} # Entity 1 will have no initial version: - entity1 = api.create_publishable_entity(learning_package.id, key="entity1", created=now_time, created_by=None) + entity1 = api.create_publishable_entity(learning_package.id, key="entity1", **created_args) # Entity 2 will have an initial version: - entity2 = api.create_publishable_entity(learning_package.id, key="entity2", created=now_time, created_by=None) - api.create_publishable_entity_version( - entity2.id, version_num=1, title="Entity 2 V1", created=now_time, created_by=None - ) + entity2 = api.create_publishable_entity(learning_package.id, key="entity2", **created_args) + api.create_publishable_entity_version(entity2.id, version_num=1, title="Entity 2 V1", **created_args) # Entity 3 will have an initial version that later gets deleted: - entity3 = api.create_publishable_entity(learning_package.id, key="entity3", created=now_time, created_by=None) - api.create_publishable_entity_version( - entity3.id, version_num=1, title="Entity 3 V1", created=now_time, created_by=None - ) + entity3 = api.create_publishable_entity(learning_package.id, key="entity3", **created_args) + api.create_publishable_entity_version(entity3.id, version_num=1, title="Entity 3 V1", **created_args) with capture_events(expected_count=1) as captured: - with api.bulk_draft_changes_for(learning_package.id, changed_by=None, changed_at=now_time) as draft_change_log: + with api.bulk_draft_changes_for( + learning_package.id, + changed_by=admin_user.id, + changed_at=now_time, + ) as draft_change_log: + # Note: the 'created_args' values below get ignored because of the bulk context. # Create two versions of entity1: - api.create_publishable_entity_version( - entity1.id, version_num=1, title="Entity 1 V1", created=now_time, created_by=None - ) - api.create_publishable_entity_version( - entity1.id, version_num=2, title="Entity 1 V2", created=now_time, created_by=None - ) + api.create_publishable_entity_version(entity1.id, version_num=1, title="Entity 1 V1", **created_args) + api.create_publishable_entity_version(entity1.id, version_num=2, title="Entity 1 V2", **created_args) # Create a version 2 of entity 2: - api.create_publishable_entity_version( - entity2.id, version_num=2, title="Entity 2 V2", created=now_time, created_by=None - ) + api.create_publishable_entity_version(entity2.id, version_num=2, title="Entity 2 V2", **created_args) # Delete entity 3: - api.set_draft_version(entity3.id, None, set_at=now_time, set_by=None) + api.set_draft_version(entity3.id, None, set_at=now_time, set_by=admin_user.id) event = captured[0] assert event.signal is api.signals.LEARNING_PACKAGE_ENTITIES_CHANGED assert event.kwargs["learning_package"].id == learning_package.id assert event.kwargs["learning_package"].title == "Test LP 📦" - assert event.kwargs["changed_by"].user_id is None + assert event.kwargs["changed_by"].user_id is admin_user.id assert event.kwargs["change_log"].draft_change_log_id == draft_change_log.id assert event.kwargs["change_log"].changes == [ # Entity 1 jumps from no version to version 2: From 160a649b752922dc49f8a1552514a8aef820f6a3 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 16 Apr 2026 12:06:38 -0700 Subject: [PATCH 07/30] test: verify that bulk event is not emitted on transaction rollback --- .../applets/publishing/signals.py | 5 ++ .../applets/publishing/test_signals.py | 46 +++++++++++++++---- tests/utils.py | 25 ++++++++++ 3 files changed, 66 insertions(+), 10 deletions(-) diff --git a/src/openedx_content/applets/publishing/signals.py b/src/openedx_content/applets/publishing/signals.py index 28381b2d4..0d67bce5d 100644 --- a/src/openedx_content/applets/publishing/signals.py +++ b/src/openedx_content/applets/publishing/signals.py @@ -73,6 +73,11 @@ class DraftChangeLogEventData: """ The draft version of one or more entities in a `LearningPackage` has changed. +This is emitted for when the first version of an entity is created, when a new +version of an entity is created (i.e. the entity is modified), when an entity is +reverted to an old version, or when an entity is deleted. (All referring to the +draft version of the entity.) + This is a low-level batch event. It does not have any course or library context information available. It does not distinguish between Containers, Components, or other entity types. diff --git a/tests/openedx_content/applets/publishing/test_signals.py b/tests/openedx_content/applets/publishing/test_signals.py index c2b0da2d6..39cb5c18f 100644 --- a/tests/openedx_content/applets/publishing/test_signals.py +++ b/tests/openedx_content/applets/publishing/test_signals.py @@ -3,12 +3,12 @@ """ from datetime import datetime, timezone +from typing import Any import pytest -from django.db import transaction from openedx_content import api -from tests.utils import capture_events +from tests.utils import abort_transaction, capture_events pytestmark = pytest.mark.django_db(transaction=True) now_time = datetime.now(tz=timezone.utc) @@ -64,14 +64,10 @@ def test_single_entity_changed_abort() -> None: entity = api.create_publishable_entity(learning_package.id, key="entity1", created=now_time, created_by=None) with capture_events(expected_count=0): - try: - with transaction.atomic(): - api.create_publishable_entity_version( - entity.id, version_num=1, title="Entity 1 V1", created=now_time, created_by=None - ) - raise DeliberateRollbackException() - except DeliberateRollbackException: - pass + with abort_transaction(): + api.create_publishable_entity_version( + entity.id, version_num=1, title="Entity 1 V1", created=now_time, created_by=None + ) def test_multiple_entites_changed(admin_user) -> None: @@ -121,3 +117,33 @@ def test_multiple_entites_changed(admin_user) -> None: api.signals.ChangeLogRecordData(entity_id=entity3.id, old_version=1, new_version=None), ] assert event.kwargs["metadata"].time == now_time + + +def test_multiple_entites_change_aborted() -> None: + """ + Test that LEARNING_PACKAGE_ENTITIES_CHANGED is NOT emitted when we roll back + a transaction that would have modified multiple entities in a bulk change. + """ + learning_package = api.create_learning_package(key="lp1", title="Test LP 📦") + created_args: dict[str, Any] = {"created": now_time, "created_by": None} + + # Entity 1 will have no initial version: + entity1 = api.create_publishable_entity(learning_package.id, key="entity1", **created_args) + # Entity 2 will have an initial version: + entity2 = api.create_publishable_entity(learning_package.id, key="entity2", **created_args) + api.create_publishable_entity_version(entity2.id, version_num=1, title="Entity 2 V1", **created_args) + # Entity 3 will have an initial version that later gets deleted: + entity3 = api.create_publishable_entity(learning_package.id, key="entity3", **created_args) + api.create_publishable_entity_version(entity3.id, version_num=1, title="Entity 3 V1", **created_args) + + with capture_events(expected_count=0): + with abort_transaction(): + with api.bulk_draft_changes_for(learning_package.id, changed_by=None, changed_at=now_time): + # Note: the 'created_args' values below get ignored because of the bulk context. + # Create two versions of entity1: + api.create_publishable_entity_version(entity1.id, version_num=1, title="Entity 1 V1", **created_args) + api.create_publishable_entity_version(entity1.id, version_num=2, title="Entity 1 V2", **created_args) + # Create a version 2 of entity 2: + api.create_publishable_entity_version(entity2.id, version_num=2, title="Entity 2 V2", **created_args) + # Delete entity 3: + api.set_draft_version(entity3.id, None, set_at=now_time, set_by=None) diff --git a/tests/utils.py b/tests/utils.py index 0926fea39..daa82f09d 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -8,6 +8,7 @@ from dataclasses import dataclass from typing import Generator +from django.db import transaction from openedx_events.tooling import OpenEdxPublicSignal # type: ignore[import-untyped] @@ -77,3 +78,27 @@ def receiver(sender, **kwargs): # pylint: disable=unused-argument assert len(captured) == expected_count, ( f"Expected {expected_count} event(s), got {len(captured)}: {[e.signal for e in captured]}" ) + + +class DeliberateRollbackException(Exception): + """Exception used to deliberately cancel and roll back a DB transaction""" + + +@contextmanager +def abort_transaction() -> Generator[None, None, None]: + """ + Context manager that wraps the block in a transaction that gets rolled back. + + Example usage:: + + with abort_transaction(): + api.do_something(...) + + assert nothing was done + """ + try: + with transaction.atomic(): + yield + raise DeliberateRollbackException + except DeliberateRollbackException: + pass From 72a9890d748b3e0c4f1db3a074b20f3ee0cd8b93 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 16 Apr 2026 12:13:25 -0700 Subject: [PATCH 08/30] docs: clarify event details --- src/openedx_content/applets/publishing/signals.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/openedx_content/applets/publishing/signals.py b/src/openedx_content/applets/publishing/signals.py index 0d67bce5d..a54a3d61e 100644 --- a/src/openedx_content/applets/publishing/signals.py +++ b/src/openedx_content/applets/publishing/signals.py @@ -73,10 +73,11 @@ class DraftChangeLogEventData: """ The draft version of one or more entities in a `LearningPackage` has changed. -This is emitted for when the first version of an entity is created, when a new -version of an entity is created (i.e. the entity is modified), when an entity is -reverted to an old version, or when an entity is deleted. (All referring to the -draft version of the entity.) +This is emitted for when the first version of an entity is **created**, when a +new version of an entity is created (i.e. an entity is **modified**), when an +entity is **reverted** to an old version, or when an entity is **deleted**. +(All referring to the draft version of the entity.) The ``old_version`` and +``new_version`` fields can be used to distinguish among these cases. This is a low-level batch event. It does not have any course or library context information available. It does not distinguish between Containers, Components, @@ -84,4 +85,8 @@ class DraftChangeLogEventData: Collections and tags are not `PublishableEntity`-based, so do not participate in this event. + +⏳ This **batch** event is emitted **synchronously**. Handlers that do anything +per-entity or that is possibly slow should dispatch an asynchronous task for +processing the event. """ From eb7b160ea6bbe45c261dce476d3cd1864f65a606 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 16 Apr 2026 15:15:59 -0700 Subject: [PATCH 09/30] feat: emit a bulk publish event when entities are published --- src/openedx_content/applets/publishing/api.py | 11 +- .../applets/publishing/signals.py | 56 +++++++++- .../applets/containers/test_api.py | 8 +- .../applets/publishing/test_signals.py | 105 +++++++++++++++++- .../applets/sections/test_api.py | 2 +- .../applets/subsections/test_api.py | 2 +- .../openedx_content/applets/units/test_api.py | 2 +- 7 files changed, 167 insertions(+), 19 deletions(-) diff --git a/src/openedx_content/applets/publishing/api.py b/src/openedx_content/applets/publishing/api.py index 9234b0ebb..481a5449d 100644 --- a/src/openedx_content/applets/publishing/api.py +++ b/src/openedx_content/applets/publishing/api.py @@ -502,6 +502,7 @@ def publish_from_drafts( published_draft_ids.add(draft.pk) _create_side_effects_for_change_log(publish_log) + _emit_event_for_change_log(publish_log, time_stamp=published_at, user_id=published_by) return publish_log @@ -928,9 +929,10 @@ def _emit_event_for_change_log( change_log: PublishLog | DraftChangeLog, time_stamp: datetime, user_id: int | None ) -> None: """ - Construct and emit the _CHANGED event when a set of entities is changed or published. + Construct and emit the _CHANGED / _PUBLISHED event when a set of entities is + changed or published. - Works with either `DraftChangeLog` or `PublishLog`. + Works with either ``DraftChangeLog`` or ``PublishLog``. """ learning_package_id = change_log.learning_package.id @@ -944,11 +946,14 @@ def _emit_event_for_change_log( for record in change_log.records.select_related("old_version", "new_version").all() ] + change_log_data: signals.DraftChangeLogEventData | signals.PublishLogEventData if isinstance(change_log, DraftChangeLog): signal = signals.LEARNING_PACKAGE_ENTITIES_CHANGED change_log_data = signals.DraftChangeLogEventData(draft_change_log_id=change_log.id, changes=changes) else: - raise NotImplementedError + assert isinstance(change_log, PublishLog) + signal = signals.LEARNING_PACKAGE_ENTITIES_PUBLISHED + change_log_data = signals.PublishLogEventData(publish_log_id=change_log.id, changes=changes) # Send out an event immediately after this database transaction commits. def send_change_event(): diff --git a/src/openedx_content/applets/publishing/signals.py b/src/openedx_content/applets/publishing/signals.py index a54a3d61e..a3c9f28e5 100644 --- a/src/openedx_content/applets/publishing/signals.py +++ b/src/openedx_content/applets/publishing/signals.py @@ -14,7 +14,9 @@ "UserAttributionEventData", "ChangeLogRecordData", "DraftChangeLogEventData", + "PublishLogEventData", "LEARNING_PACKAGE_ENTITIES_CHANGED", + "LEARNING_PACKAGE_ENTITIES_PUBLISHED", ] @@ -56,12 +58,20 @@ class ChangeLogRecordData: @define class DraftChangeLogEventData: - """Summary of a `DraftChangeLog`""" + """Summary of a `DraftChangeLog` for event purposes""" draft_change_log_id: int changes: list[ChangeLogRecordData] +@define +class PublishLogEventData: + """Summary of a `PublishLog` for event purposes""" + + publish_log_id: int + changes: list[ChangeLogRecordData] + + LEARNING_PACKAGE_ENTITIES_CHANGED = OpenEdxPublicSignal( event_type="org.openedx.content.publishing.lp_entities_changed.v1", data={ @@ -73,11 +83,45 @@ class DraftChangeLogEventData: """ The draft version of one or more entities in a `LearningPackage` has changed. -This is emitted for when the first version of an entity is **created**, when a -new version of an entity is created (i.e. an entity is **modified**), when an -entity is **reverted** to an old version, or when an entity is **deleted**. -(All referring to the draft version of the entity.) The ``old_version`` and -``new_version`` fields can be used to distinguish among these cases. +This is emitted when the first version of an entity is **created**, when a new +version of an entity is created (i.e. an entity is **modified**), when an entity +is **reverted** to an old version, or when an entity is **deleted**. (All +referring to the draft version of the entity.) + +The ``old_version`` and ``new_version`` fields can be used to distinguish among +these cases (e.g. ``old_version`` is ``None`` for newly-created entities). + +This is a low-level batch event. It does not have any course or library context +information available. It does not distinguish between Containers, Components, +or other entity types. + +Collections and tags are not `PublishableEntity`-based, so do not participate in +this event. + +⏳ This **batch** event is emitted **synchronously**. Handlers that do anything +per-entity or that is possibly slow should dispatch an asynchronous task for +processing the event. +""" + + +LEARNING_PACKAGE_ENTITIES_PUBLISHED = OpenEdxPublicSignal( + event_type="org.openedx.content.publishing.lp_entities_published.v1", + data={ + "learning_package": LearningPackageEventData, + "changed_by": UserAttributionEventData, + "change_log": PublishLogEventData, + }, +) +""" +The published version of one or more entities in a `LearningPackage` has +changed. + +This is emitted when **a newly-created entity is first published**, when +**changes to an existing entity** are published, when a published entity is +**reverted** to a previous version, or when **a "delete" is published**. + +The ``old_version`` and ``new_version`` fields can be used to distinguish among +these cases (e.g. ``old_version`` is ``None`` for newly-created entities). This is a low-level batch event. It does not have any course or library context information available. It does not distinguish between Containers, Components, diff --git a/tests/openedx_content/applets/containers/test_api.py b/tests/openedx_content/applets/containers/test_api.py index d41ab2932..e2b209254 100644 --- a/tests/openedx_content/applets/containers/test_api.py +++ b/tests/openedx_content/applets/containers/test_api.py @@ -925,7 +925,7 @@ def test_contains_unpublished_changes_queries( assert containers_api.contains_unpublished_changes(grandparent.id) # Publish grandparent and all its descendants: - with django_assert_num_queries(135): # TODO: investigate as this seems high! + with django_assert_num_queries(137): # TODO: investigate as this seems high! publish_entity(grandparent) # Tests: @@ -1244,7 +1244,7 @@ def test_uninstalled_publish( """Simple test of publishing a container of uninstalled type, plus its child, and reviewing the publish log""" # Publish container_of_uninstalled_type (and child_entity1). Should not affect anything else, # but we should see "child_entity1" omitted from the subsequent publish. - with django_assert_num_queries(49): + with django_assert_num_queries(51): publish_log = publish_entity(container_of_uninstalled_type) # Nothing else should have been affected by the publish: assert list(publish_log.records.order_by("entity__pk").values_list("entity__key", flat=True)) == [ @@ -1282,7 +1282,7 @@ def test_deep_publish_log( ) # Publish container_of_uninstalled_type (and child_entity1). Should not affect anything else, # but we should see "child_entity1" omitted from the subsequent publish. - with django_assert_num_queries(49): + with django_assert_num_queries(51): publish_log = publish_entity(container_of_uninstalled_type) # Nothing else should have been affected by the publish: assert list(publish_log.records.order_by("entity__pk").values_list("entity__key", flat=True)) == [ @@ -1291,7 +1291,7 @@ def test_deep_publish_log( ] # Publish great_grandparent. Should publish the whole tree. - with django_assert_num_queries(126): + with django_assert_num_queries(128): publish_log = publish_entity(great_grandparent) assert list(publish_log.records.order_by("entity__pk").values_list("entity__key", flat=True)) == [ "child_entity2", diff --git a/tests/openedx_content/applets/publishing/test_signals.py b/tests/openedx_content/applets/publishing/test_signals.py index 39cb5c18f..0f8976bbc 100644 --- a/tests/openedx_content/applets/publishing/test_signals.py +++ b/tests/openedx_content/applets/publishing/test_signals.py @@ -13,9 +13,7 @@ pytestmark = pytest.mark.django_db(transaction=True) now_time = datetime.now(tz=timezone.utc) - -class DeliberateRollbackException(Exception): - """Exception used to deliberately cancel and roll back a DB transaction""" +# LEARNING_PACKAGE_ENTITIES_CHANGED def test_single_entity_changed() -> None: @@ -147,3 +145,104 @@ def test_multiple_entites_change_aborted() -> None: api.create_publishable_entity_version(entity2.id, version_num=2, title="Entity 2 V2", **created_args) # Delete entity 3: api.set_draft_version(entity3.id, None, set_at=now_time, set_by=None) + + +# LEARNING_PACKAGE_ENTITIES_PUBLISHED + + +def test_publish_events(admin_user) -> None: + """ + Test that LEARNING_PACKAGE_ENTITIES_PUBLISHED is emitted when we publish + changes to entities in a learning package. + """ + learning_package = api.create_learning_package(key="lp1", title="Test LP 📦") + created_args = {"created": now_time, "created_by": admin_user.id} + + # Entity 1 will have no initial version: + entity1 = api.create_publishable_entity(learning_package.id, key="entity1", **created_args) + # Entity 2 will have an initial version with some changes: + entity2 = api.create_publishable_entity(learning_package.id, key="entity2", **created_args) + api.create_publishable_entity_version(entity2.id, version_num=1, title="Entity 2 V1", **created_args) + api.create_publishable_entity_version(entity2.id, version_num=2, title="Entity 2 V2", **created_args) + # Entity 3 will have an initial version that later gets deleted: + entity3 = api.create_publishable_entity(learning_package.id, key="entity3", **created_args) + api.create_publishable_entity_version(entity3.id, version_num=1, title="Entity 3 V1", **created_args) + + # Publish these initial changes: + first_publish_time = datetime.now(tz=timezone.utc) + with capture_events(expected_count=1) as captured: + first_log = api.publish_all_drafts( + learning_package.id, published_at=first_publish_time, published_by=admin_user.id + ) + + event = captured[0] + assert event.signal is api.signals.LEARNING_PACKAGE_ENTITIES_PUBLISHED + assert event.kwargs["learning_package"].id == learning_package.id + assert event.kwargs["learning_package"].title == "Test LP 📦" + assert event.kwargs["changed_by"].user_id is admin_user.id + assert event.kwargs["change_log"].publish_log_id == first_log.id + assert event.kwargs["change_log"].changes == [ + # Entity 1 is not yet published, since it has no draft version. + # Entity 2 is newly published, and now at v2: + api.signals.ChangeLogRecordData(entity_id=entity2.id, old_version=None, new_version=2), + # Entity 3 is newly published, and now at v1: + api.signals.ChangeLogRecordData(entity_id=entity3.id, old_version=None, new_version=1), + ] + assert event.kwargs["metadata"].time == first_publish_time + + # Now modify the entities again: + # Create a version of entity1: + api.create_publishable_entity_version(entity1.id, version_num=1, title="Entity 1 V1", **created_args) + # Create a version 3 of entity2: + api.create_publishable_entity_version(entity2.id, version_num=3, title="Entity 2 V3", **created_args) + # Delete entity 3: + api.set_draft_version(entity3.id, None, set_at=now_time, set_by=admin_user.id) + + # Publish these new changes: + second_publish_time = datetime.now(tz=timezone.utc) + with capture_events(expected_count=1) as captured: + second_log = api.publish_all_drafts( + learning_package.id, published_at=second_publish_time, published_by=admin_user.id + ) + + event = captured[0] + assert event.signal is api.signals.LEARNING_PACKAGE_ENTITIES_PUBLISHED + assert event.kwargs["learning_package"].id == learning_package.id + assert event.kwargs["learning_package"].title == "Test LP 📦" + assert event.kwargs["changed_by"].user_id is admin_user.id + assert event.kwargs["change_log"].publish_log_id == second_log.id + assert event.kwargs["change_log"].changes == [ + # Entity 1 is newly published at v1: + api.signals.ChangeLogRecordData(entity_id=entity1.id, old_version=None, new_version=1), + # Entity 2 jumps v2 -> v3: + api.signals.ChangeLogRecordData(entity_id=entity2.id, old_version=2, new_version=3), + # Entity 3 gets deleted: + api.signals.ChangeLogRecordData(entity_id=entity3.id, old_version=1, new_version=None), + ] + assert event.kwargs["metadata"].time == second_publish_time + + +def test_publish_events_aborted(admin_user) -> None: + """ + Test that LEARNING_PACKAGE_ENTITIES_PUBLISHED is NOT emitted when we roll + back a transaction that would have published some entities. + """ + learning_package = api.create_learning_package(key="lp1", title="Test LP 📦") + created_args = {"created": now_time, "created_by": admin_user.id} + + # Create an entity with some initial version: + entity1 = api.create_publishable_entity(learning_package.id, key="entity1", **created_args) + api.create_publishable_entity_version(entity1.id, version_num=1, title="Entity 1 V1", **created_args) + + def do_publish(): + draft_qset = api.get_all_drafts(learning_package.id).filter(entity=entity1) + api.publish_from_drafts( + learning_package.id, draft_qset=draft_qset, published_at=now_time, published_by=admin_user.id + ) + + with capture_events(expected_count=0): + with abort_transaction(): + do_publish() + + with capture_events(expected_count=1): + do_publish() diff --git a/tests/openedx_content/applets/sections/test_api.py b/tests/openedx_content/applets/sections/test_api.py index 3de23f97b..0e08e946a 100644 --- a/tests/openedx_content/applets/sections/test_api.py +++ b/tests/openedx_content/applets/sections/test_api.py @@ -155,7 +155,7 @@ def test_section_queries(self) -> None: """ with self.assertNumQueries(39): section = self.create_section_with_subsections([self.subsection_1, self.subsection_2_v1]) - with self.assertNumQueries(160): + with self.assertNumQueries(162): content_api.publish_from_drafts( self.learning_package.id, draft_qset=content_api.get_all_drafts(self.learning_package.id).filter(entity=section.id), diff --git a/tests/openedx_content/applets/subsections/test_api.py b/tests/openedx_content/applets/subsections/test_api.py index f52b965a6..228490a90 100644 --- a/tests/openedx_content/applets/subsections/test_api.py +++ b/tests/openedx_content/applets/subsections/test_api.py @@ -133,7 +133,7 @@ def test_subsection_queries(self) -> None: """ with self.assertNumQueries(39): subsection = self.create_subsection_with_units([self.unit_1, self.unit_1_v1]) - with self.assertNumQueries(102): # TODO: this seems high? + with self.assertNumQueries(104): # TODO: this seems high? content_api.publish_from_drafts( self.learning_package.id, draft_qset=content_api.get_all_drafts(self.learning_package.id).filter(entity=subsection.id), diff --git a/tests/openedx_content/applets/units/test_api.py b/tests/openedx_content/applets/units/test_api.py index c89a0e398..a704ae706 100644 --- a/tests/openedx_content/applets/units/test_api.py +++ b/tests/openedx_content/applets/units/test_api.py @@ -134,7 +134,7 @@ def test_unit_queries(self) -> None: """ with self.assertNumQueries(37): unit = self.create_unit_with_components([self.component_1, self.component_2_v1]) - with self.assertNumQueries(48): # TODO: this seems high? + with self.assertNumQueries(50): # TODO: this seems high? content_api.publish_from_drafts( self.learning_package.id, draft_qset=content_api.get_all_drafts(self.learning_package.id).filter(entity=unit.id), From 09562bc7f218cdacb4cfe221a09ba955b3a20566 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 16 Apr 2026 16:33:40 -0700 Subject: [PATCH 10/30] test: check that signals are correctly emitted for dependencies --- src/openedx_content/applets/publishing/api.py | 4 +- .../applets/publishing/signals.py | 10 +- .../applets/publishing/test_signals.py | 100 ++++++++++++++++++ 3 files changed, 108 insertions(+), 6 deletions(-) diff --git a/src/openedx_content/applets/publishing/api.py b/src/openedx_content/applets/publishing/api.py index 481a5449d..835f60260 100644 --- a/src/openedx_content/applets/publishing/api.py +++ b/src/openedx_content/applets/publishing/api.py @@ -192,7 +192,7 @@ def create_publishable_entity_version( created: datetime, created_by: int | None, *, - dependencies: list[int] | None = None, # PublishableEntity IDs + dependencies: list[PublishableEntity.ID] | None = None, ) -> PublishableEntityVersion: """ Create a PublishableEntityVersion. @@ -223,7 +223,7 @@ def create_publishable_entity_version( def set_version_dependencies( version_id: int, # PublishableEntityVersion.id, /, - dependencies: list[int] # List of PublishableEntity.id + dependencies: list[PublishableEntity.ID], ) -> None: """ Set the dependencies of a publishable entity version. diff --git a/src/openedx_content/applets/publishing/signals.py b/src/openedx_content/applets/publishing/signals.py index a3c9f28e5..f0c904b17 100644 --- a/src/openedx_content/applets/publishing/signals.py +++ b/src/openedx_content/applets/publishing/signals.py @@ -85,8 +85,8 @@ class PublishLogEventData: This is emitted when the first version of an entity is **created**, when a new version of an entity is created (i.e. an entity is **modified**), when an entity -is **reverted** to an old version, or when an entity is **deleted**. (All -referring to the draft version of the entity.) +is **reverted** to an old version, when **a dependency is modified**, or when an +entity is **deleted**. (All referring to the draft version of the entity.) The ``old_version`` and ``new_version`` fields can be used to distinguish among these cases (e.g. ``old_version`` is ``None`` for newly-created entities). @@ -117,8 +117,10 @@ class PublishLogEventData: changed. This is emitted when **a newly-created entity is first published**, when -**changes to an existing entity** are published, when a published entity is -**reverted** to a previous version, or when **a "delete" is published**. +**changes to an existing entity** are published, when **changes to a +dependency** (or a dependency's dependencies...) are published, when a published +entity is **reverted** to a previous version, or when **a "delete" is +published**. The ``old_version`` and ``new_version`` fields can be used to distinguish among these cases (e.g. ``old_version`` is ``None`` for newly-created entities). diff --git a/tests/openedx_content/applets/publishing/test_signals.py b/tests/openedx_content/applets/publishing/test_signals.py index 0f8976bbc..55e773e78 100644 --- a/tests/openedx_content/applets/publishing/test_signals.py +++ b/tests/openedx_content/applets/publishing/test_signals.py @@ -8,11 +8,19 @@ import pytest from openedx_content import api +from openedx_content.models_api import PublishableEntity, PublishLog from tests.utils import abort_transaction, capture_events pytestmark = pytest.mark.django_db(transaction=True) now_time = datetime.now(tz=timezone.utc) + +def publish_entity(obj: PublishableEntity) -> PublishLog: + """Helper method to publish a single entity.""" + lp_id = obj.learning_package_id + return api.publish_from_drafts(lp_id, draft_qset=api.get_all_drafts(lp_id).filter(entity=obj)) + + # LEARNING_PACKAGE_ENTITIES_CHANGED @@ -147,6 +155,38 @@ def test_multiple_entites_change_aborted() -> None: api.set_draft_version(entity3.id, None, set_at=now_time, set_by=None) +def test_changes_with_side_effects() -> None: + """ + Test that the LEARNING_PACKAGE_ENTITIES_CHANGED event handles dependencies + and side effects. + """ + learning_package = api.create_learning_package(key="lp1", title="Test LP 📦") + created_args: dict[str, Any] = {"created": now_time, "created_by": None} + + # Create entities with dependencies + + def create_entity(name: str, dependencies: list[PublishableEntity.ID] | None = None) -> PublishableEntity: + e = api.create_publishable_entity(learning_package.id, key=name, **created_args) + api.create_publishable_entity_version( + e.id, version_num=1, title=f"{name} V1", dependencies=dependencies, **created_args + ) + return e + + child1 = create_entity("child1") + parent1 = create_entity("parent1", dependencies=[child1.id]) + + # now, modifying child1 will affect parent1: + with capture_events(expected_count=1) as captured: + api.create_publishable_entity_version(child1.id, version_num=2, title="child1 V2", **created_args) + + event = captured[0] + assert event.signal is api.signals.LEARNING_PACKAGE_ENTITIES_CHANGED + assert event.kwargs["change_log"].changes == [ + api.signals.ChangeLogRecordData(entity_id=child1.id, old_version=1, new_version=2), # directly modified + api.signals.ChangeLogRecordData(entity_id=parent1.id, old_version=1, new_version=1), # side effect + ] + + # LEARNING_PACKAGE_ENTITIES_PUBLISHED @@ -246,3 +286,63 @@ def do_publish(): with capture_events(expected_count=1): do_publish() + + +def test_publish_with_dependencies() -> None: + """ + Test that the LEARNING_PACKAGE_ENTITIES_PUBLISHED event handles dependencies + and side effects. + """ + learning_package = api.create_learning_package(key="lp1", title="Test LP 📦") + created_args: dict[str, Any] = {"created": now_time, "created_by": None} + + # Create entities with dependencies + + def create_entity(name: str, dependencies: list[PublishableEntity.ID] | None = None) -> PublishableEntity: + e = api.create_publishable_entity(learning_package.id, key=name, **created_args) + api.create_publishable_entity_version( + e.id, version_num=1, title=f"{name} V1", dependencies=dependencies, **created_args + ) + return e + + # 👧👦 children + child1 = create_entity("child1") + child2 = create_entity("child2") + child3 = create_entity("child3") + # 🧓👩 parents + parent1 = create_entity("parent1", dependencies=[child1.id, child2.id]) + parent2 = create_entity("parent2", dependencies=[child2.id, child3.id]) + # 👴👵 grandparents + grandparent1 = create_entity("grandparent1", dependencies=[parent1.id]) + grandparent2 = create_entity("grandparent2", dependencies=[parent2.id]) + + # publish grandparent1 directly and all its dependencies indirectly: + with capture_events(expected_count=1) as captured: + publish_entity(grandparent1) + + event = captured[0] + assert event.signal is api.signals.LEARNING_PACKAGE_ENTITIES_PUBLISHED + assert event.kwargs["change_log"].changes == [ + api.signals.ChangeLogRecordData(entity_id=grandparent1.id, old_version=None, new_version=1), + api.signals.ChangeLogRecordData(entity_id=parent1.id, old_version=None, new_version=1), + api.signals.ChangeLogRecordData(entity_id=child1.id, old_version=None, new_version=1), + api.signals.ChangeLogRecordData(entity_id=child2.id, old_version=None, new_version=1), + ] + + # publish the rest: + with capture_events(expected_count=1): + api.publish_all_drafts(learning_package.id) + + # ✨ Now modify 'child3', causing side effects for parent2 and grandparent2 + api.create_publishable_entity_version(child3.id, version_num=2, title="child3 V2", **created_args) + + with capture_events(expected_count=1) as captured: + publish_entity(child3) + + event = captured[0] + assert event.signal is api.signals.LEARNING_PACKAGE_ENTITIES_PUBLISHED + assert event.kwargs["change_log"].changes == [ + api.signals.ChangeLogRecordData(entity_id=child3.id, old_version=1, new_version=2), # directly published + api.signals.ChangeLogRecordData(entity_id=parent2.id, old_version=1, new_version=1), # side effect + api.signals.ChangeLogRecordData(entity_id=grandparent2.id, old_version=1, new_version=1), # side effect + ] From 15aba5457eee3514c75b92132b1725e95c599912 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 16 Apr 2026 17:06:16 -0700 Subject: [PATCH 11/30] feat: indicate when things are directly published or not --- src/openedx_content/applets/publishing/api.py | 1 + .../applets/publishing/signals.py | 6 ++++- .../applets/publishing/test_signals.py | 24 +++++++++---------- 3 files changed, 18 insertions(+), 13 deletions(-) diff --git a/src/openedx_content/applets/publishing/api.py b/src/openedx_content/applets/publishing/api.py index e452d4bbd..6f7fe8abe 100644 --- a/src/openedx_content/applets/publishing/api.py +++ b/src/openedx_content/applets/publishing/api.py @@ -946,6 +946,7 @@ def _emit_event_for_change_log( entity_id=record.entity_id, old_version=record.old_version.version_num if record.old_version else None, new_version=record.new_version.version_num if record.new_version else None, + direct=record.direct if isinstance(record, PublishLogRecord) else None, ) for record in change_log.records.select_related("old_version", "new_version").all() ] diff --git a/src/openedx_content/applets/publishing/signals.py b/src/openedx_content/applets/publishing/signals.py index f0c904b17..68f1734da 100644 --- a/src/openedx_content/applets/publishing/signals.py +++ b/src/openedx_content/applets/publishing/signals.py @@ -53,7 +53,11 @@ class ChangeLogRecordData: This is None if the entity is now deleted. """ - # Future: direct? https://github.com/openedx/openedx-core/pull/539 + direct: bool | None = None + """ + Did the user chose to directly publish this specific thing, or was it auto-published because it's a dependency? + (if applicable/known) + """ @define diff --git a/tests/openedx_content/applets/publishing/test_signals.py b/tests/openedx_content/applets/publishing/test_signals.py index 55e773e78..dda95752f 100644 --- a/tests/openedx_content/applets/publishing/test_signals.py +++ b/tests/openedx_content/applets/publishing/test_signals.py @@ -224,9 +224,9 @@ def test_publish_events(admin_user) -> None: assert event.kwargs["change_log"].changes == [ # Entity 1 is not yet published, since it has no draft version. # Entity 2 is newly published, and now at v2: - api.signals.ChangeLogRecordData(entity_id=entity2.id, old_version=None, new_version=2), + api.signals.ChangeLogRecordData(entity_id=entity2.id, old_version=None, new_version=2, direct=True), # Entity 3 is newly published, and now at v1: - api.signals.ChangeLogRecordData(entity_id=entity3.id, old_version=None, new_version=1), + api.signals.ChangeLogRecordData(entity_id=entity3.id, old_version=None, new_version=1, direct=True), ] assert event.kwargs["metadata"].time == first_publish_time @@ -253,11 +253,11 @@ def test_publish_events(admin_user) -> None: assert event.kwargs["change_log"].publish_log_id == second_log.id assert event.kwargs["change_log"].changes == [ # Entity 1 is newly published at v1: - api.signals.ChangeLogRecordData(entity_id=entity1.id, old_version=None, new_version=1), + api.signals.ChangeLogRecordData(entity_id=entity1.id, old_version=None, new_version=1, direct=True), # Entity 2 jumps v2 -> v3: - api.signals.ChangeLogRecordData(entity_id=entity2.id, old_version=2, new_version=3), + api.signals.ChangeLogRecordData(entity_id=entity2.id, old_version=2, new_version=3, direct=True), # Entity 3 gets deleted: - api.signals.ChangeLogRecordData(entity_id=entity3.id, old_version=1, new_version=None), + api.signals.ChangeLogRecordData(entity_id=entity3.id, old_version=1, new_version=None, direct=True), ] assert event.kwargs["metadata"].time == second_publish_time @@ -323,10 +323,10 @@ def create_entity(name: str, dependencies: list[PublishableEntity.ID] | None = N event = captured[0] assert event.signal is api.signals.LEARNING_PACKAGE_ENTITIES_PUBLISHED assert event.kwargs["change_log"].changes == [ - api.signals.ChangeLogRecordData(entity_id=grandparent1.id, old_version=None, new_version=1), - api.signals.ChangeLogRecordData(entity_id=parent1.id, old_version=None, new_version=1), - api.signals.ChangeLogRecordData(entity_id=child1.id, old_version=None, new_version=1), - api.signals.ChangeLogRecordData(entity_id=child2.id, old_version=None, new_version=1), + api.signals.ChangeLogRecordData(entity_id=grandparent1.id, old_version=None, new_version=1, direct=True), + api.signals.ChangeLogRecordData(entity_id=parent1.id, old_version=None, new_version=1, direct=False), + api.signals.ChangeLogRecordData(entity_id=child1.id, old_version=None, new_version=1, direct=False), + api.signals.ChangeLogRecordData(entity_id=child2.id, old_version=None, new_version=1, direct=False), ] # publish the rest: @@ -342,7 +342,7 @@ def create_entity(name: str, dependencies: list[PublishableEntity.ID] | None = N event = captured[0] assert event.signal is api.signals.LEARNING_PACKAGE_ENTITIES_PUBLISHED assert event.kwargs["change_log"].changes == [ - api.signals.ChangeLogRecordData(entity_id=child3.id, old_version=1, new_version=2), # directly published - api.signals.ChangeLogRecordData(entity_id=parent2.id, old_version=1, new_version=1), # side effect - api.signals.ChangeLogRecordData(entity_id=grandparent2.id, old_version=1, new_version=1), # side effect + api.signals.ChangeLogRecordData(entity_id=child3.id, old_version=1, new_version=2, direct=True), + api.signals.ChangeLogRecordData(entity_id=parent2.id, old_version=1, new_version=1, direct=False), + api.signals.ChangeLogRecordData(entity_id=grandparent2.id, old_version=1, new_version=1, direct=False), ] From 5e11a127a96fece37632c99aa3d97219b2134a84 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 17 Apr 2026 14:23:50 -0700 Subject: [PATCH 12/30] feat: events for Collections --- .../applets/collections/api.py | 100 ++++- .../applets/collections/signals.py | 59 +++ .../applets/publishing/signals.py | 2 + .../applets/collections/test_signals.py | 351 ++++++++++++++++++ 4 files changed, 492 insertions(+), 20 deletions(-) create mode 100644 src/openedx_content/applets/collections/signals.py create mode 100644 tests/openedx_content/applets/collections/test_signals.py diff --git a/src/openedx_content/applets/collections/api.py b/src/openedx_content/applets/collections/api.py index b57b3bea8..ac721402a 100644 --- a/src/openedx_content/applets/collections/api.py +++ b/src/openedx_content/applets/collections/api.py @@ -1,15 +1,18 @@ """ Collections API (warning: UNSTABLE, in progress API) """ + from __future__ import annotations from datetime import datetime, timezone from django.core.exceptions import ValidationError from django.db.models import QuerySet +from django.db.transaction import on_commit from ..publishing import api as publishing_api from ..publishing.models import PublishableEntity +from . import signals from .models import Collection, CollectionPublishableEntity, LearningPackage # The public API that will be re-exported by openedx_content.api @@ -32,6 +35,42 @@ ] +def _queue_change_event( + collection: Collection, + *, + created: bool = False, + modified: bool = False, + deleted: bool = False, + entities_added: list[PublishableEntity.ID] | None = None, + entities_removed: list[PublishableEntity.ID] | None = None, + user_id: int | None = None, +) -> None: + """Helper for emitting the event when a collection has changed.""" + + learning_package_id = collection.learning_package.id + learning_package_title = collection.learning_package.title + + # Send out an event immediately after this database transaction commits. + def send_change_event(): + signals.LEARNING_PACKAGE_COLLECTION_CHANGED.send_event( + time=collection.modified, + learning_package=signals.LearningPackageEventData(id=learning_package_id, title=learning_package_title), + # FIXME: most collections APIs are missing a user_id parameter. + changed_by=signals.UserAttributionEventData(user_id=user_id), + change=signals.CollectionChangeData( + collection_id=collection.id, + collection_code=collection.key, # TODO: collection_code + created=created, + modified=modified, + deleted=deleted, + entities_added=entities_added or [], + entities_removed=entities_removed or [], + ), + ) + + on_commit(send_change_event) + + def create_collection( learning_package_id: LearningPackage.ID, key: str, @@ -52,6 +91,8 @@ def create_collection( description=description, enabled=enabled, ) + if enabled: + _queue_change_event(collection, created=True, user_id=created_by) return collection @@ -85,6 +126,7 @@ def update_collection( collection.description = description collection.save() + _queue_change_event(collection, modified=True) return collection @@ -101,12 +143,17 @@ def delete_collection( Soft-deleted collections can be re-enabled using restore_collection. """ collection = get_collection(learning_package_id, key) + entities_removed = list(collection.entities.order_by("id").values_list("id", flat=True)) if hard_delete: + collection.modified = datetime.now(tz=timezone.utc) # For the event timestamp; won't get saved to the DB + _queue_change_event(collection, deleted=True, entities_removed=entities_removed) + # Delete after enqueing the event: collection.delete() else: collection.enabled = False collection.save() + _queue_change_event(collection, deleted=True, entities_removed=entities_removed) return collection @@ -118,9 +165,11 @@ def restore_collection( Undo a "soft delete" by re-enabling a Collection. """ collection = get_collection(learning_package_id, key) + entities_added = list(collection.entities.order_by("id").values_list("id", flat=True)) collection.enabled = True collection.save() + _queue_change_event(collection, created=True, entities_added=entities_added) return collection @@ -150,12 +199,12 @@ def add_to_collection( ) collection = get_collection(learning_package_id, key) - collection.entities.add( - *entities_qset.all(), - through_defaults={"created_by_id": created_by}, - ) + 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}) collection.modified = datetime.now(tz=timezone.utc) collection.save() + _queue_change_event(collection, entities_added=list(set(ids_to_add) - existing_ids), user_id=created_by) return collection @@ -176,9 +225,12 @@ def remove_from_collection( """ collection = get_collection(learning_package_id, key) - collection.entities.remove(*entities_qset.all()) + ids_to_remove = list(entities_qset.values_list("id", flat=True)) + entities_removed = list(collection.entities.filter(id__in=ids_to_remove).values_list("id", flat=True)) + collection.entities.remove(*ids_to_remove) collection.modified = datetime.now(tz=timezone.utc) collection.save() + _queue_change_event(collection, entities_removed=entities_removed) return collection @@ -220,7 +272,7 @@ def get_collections(learning_package_id: LearningPackage.ID, enabled: bool | Non qs = Collection.objects.filter(learning_package_id=learning_package_id) if enabled is not None: qs = qs.filter(enabled=enabled) - return qs.select_related("learning_package").order_by('pk') + return qs.select_related("learning_package").order_by("pk") def set_collections( @@ -243,25 +295,33 @@ def set_collections( raise ValidationError( "Collection entities must be from the same learning package as the collection.", ) - 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) + current_relations = CollectionPublishableEntity.objects.filter(entity=publishable_entity).select_related( + "collection" ) - new_collections = set(collection_qset.exclude( - id__in=current_relations.values_list('collection', flat=True) - )) + # 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))) # Triggers a m2m_changed signal publishable_entity.collections.set( 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. - 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)) + affected_collections = removed_collections | new_collections + Collection.objects.filter(id__in=[collection.id for collection in affected_collections]).update( + modified=datetime.now(tz=timezone.utc) + ) - return affected_collection + # Emit one event per affected collection. Re-fetch with select_related so _queue_change_event + # can read collection.learning_package without extra queries; the re-fetch also picks up the + # updated modified timestamp from the bulk update above. + removed_ids = {c.id for c in removed_collections} + for collection in Collection.objects.filter(id__in=[c.id for c in affected_collections]).select_related( + "learning_package" + ): + if collection.id in removed_ids: + _queue_change_event(collection, entities_removed=[publishable_entity.id], user_id=created_by) + else: + _queue_change_event(collection, entities_added=[publishable_entity.id], user_id=created_by) + + return affected_collections diff --git a/src/openedx_content/applets/collections/signals.py b/src/openedx_content/applets/collections/signals.py new file mode 100644 index 000000000..20c0f30ec --- /dev/null +++ b/src/openedx_content/applets/collections/signals.py @@ -0,0 +1,59 @@ +""" +Low-level events/signals emitted by openedx_content +""" + +from attrs import define, field +from openedx_events.tooling import OpenEdxPublicSignal # type: ignore[import-untyped] + +from ..publishing.models.publishable_entity import PublishableEntity +from ..publishing.signals import LearningPackageEventData, UserAttributionEventData + +# Public API available via openedx_content.api +__all__ = [ + # All event data structures should end with "...Data": + "CollectionChangeData", + # All events: + "LEARNING_PACKAGE_COLLECTION_CHANGED", +] + + +@define +class CollectionChangeData: + """Summary of changes to a collection, for event purposes""" + + collection_id: int + collection_code: str + created: bool = False + """The collection is newly-created, or un-deleted. Some entities may be added simultaneously.""" + modified: bool = False + """The collection's title/description has changed. Does not indicate whether or not entities were added/removed.""" + deleted: bool = False + """ + The collection has been deleted. When this is true, the entities_removed list will have all entity IDs. + Does not distinguish between "soft" and "hard" deletion. + """ + entities_added: list[PublishableEntity.ID] = field(factory=list) + entities_removed: list[PublishableEntity.ID] = field(factory=list) + + +LEARNING_PACKAGE_COLLECTION_CHANGED = OpenEdxPublicSignal( + event_type="org.openedx.content.publishing.lp_collection_changed.v1", + data={ + "learning_package": LearningPackageEventData, + "changed_by": UserAttributionEventData, + "change": CollectionChangeData, + }, +) +""" +A ``Collection`` has been created, modified, or deleted, or its entities have +changed. + +This is a low-level batch event. It does not have any course or library context +information available. It does not distinguish between Containers, Components, +or other entity types. + +⏳ This **batch** event is emitted **synchronously**. Handlers that do anything +per-entity or that is possibly slow should dispatch an asynchronous task for +processing the event. +""" +# TODO: also emit an ENTITIES_META_CHANGED for this and tag changes? diff --git a/src/openedx_content/applets/publishing/signals.py b/src/openedx_content/applets/publishing/signals.py index 68f1734da..43feb29a6 100644 --- a/src/openedx_content/applets/publishing/signals.py +++ b/src/openedx_content/applets/publishing/signals.py @@ -10,11 +10,13 @@ # Public API available via openedx_content.api __all__ = [ + # All event data structures should end with "...Data": "LearningPackageEventData", "UserAttributionEventData", "ChangeLogRecordData", "DraftChangeLogEventData", "PublishLogEventData", + # All events: "LEARNING_PACKAGE_ENTITIES_CHANGED", "LEARNING_PACKAGE_ENTITIES_PUBLISHED", ] diff --git a/tests/openedx_content/applets/collections/test_signals.py b/tests/openedx_content/applets/collections/test_signals.py new file mode 100644 index 000000000..cb98bc8c2 --- /dev/null +++ b/tests/openedx_content/applets/collections/test_signals.py @@ -0,0 +1,351 @@ +""" +Tests for the LEARNING_PACKAGE_COLLECTION_CHANGED signal. +""" + +from datetime import datetime, timezone + +import pytest + +from openedx_content import api +from openedx_content.applets.collections.signals import LEARNING_PACKAGE_COLLECTION_CHANGED, CollectionChangeData +from openedx_content.models_api import Collection, LearningPackage, PublishableEntity +from tests.utils import abort_transaction, capture_events + +pytestmark = pytest.mark.django_db(transaction=True) +now_time = datetime.now(tz=timezone.utc) + + +@pytest.fixture(name="lp1") +def _lp1() -> LearningPackage: + """A learning package for use across collection signal tests.""" + return api.create_learning_package(key="lp1", title="Test LP 📦") + + +def _create_entity(learning_package_id: LearningPackage.ID, key: str) -> PublishableEntity: + """Helper: create a bare PublishableEntity in the given learning package.""" + return api.create_publishable_entity(learning_package_id, key=key, created=now_time, created_by=None) + + +# LEARNING_PACKAGE_COLLECTION_CHANGED — create_collection + + +def test_create_collection(lp1: LearningPackage, admin_user) -> None: + """ + Test that LEARNING_PACKAGE_COLLECTION_CHANGED is emitted with created=True + when a new collection is created. + """ + with capture_events(expected_count=1) as captured: + collection = api.create_collection( + lp1.id, + key="col1", + title="Collection 1", + created_by=admin_user.id, + ) + + event = captured[0] + assert event.signal is LEARNING_PACKAGE_COLLECTION_CHANGED + assert event.kwargs["learning_package"].id == lp1.id + assert event.kwargs["learning_package"].title == "Test LP 📦" + assert event.kwargs["changed_by"].user_id == admin_user.id + assert event.kwargs["change"] == CollectionChangeData( + collection_id=collection.id, + collection_code="col1", + created=True, + ) + collection.refresh_from_db() + # Note: unfortunately collection.modified is slightly different than collection.created + # (see https://code.djangoproject.com/ticket/16745). It would be nice if we made them exactly the same. + assert event.kwargs["metadata"].time == collection.modified + + +def test_create_collection_disabled(lp1: LearningPackage) -> None: + """ + Test that no event is emitted when a collection is created with enabled=False. + + A disabled collection is invisible to consumers, so there is nothing to notify about. + """ + with capture_events(expected_count=0): + api.create_collection( + lp1.id, + key="col1", + title="Collection 1", + created_by=None, + enabled=False, + ) + + +def test_create_collection_aborted(lp1: LearningPackage) -> None: + """ + Test that no event is emitted when a collection creation is rolled back. + """ + with capture_events(expected_count=0): + with abort_transaction(): + api.create_collection( + lp1.id, + key="col1", + title="Collection 1", + created_by=None, + ) + + +# LEARNING_PACKAGE_COLLECTION_CHANGED — update_collection + + +def test_update_collection(lp1: LearningPackage) -> None: + """ + Test that LEARNING_PACKAGE_COLLECTION_CHANGED is emitted with modified=True + when a collection's title or description is updated. + """ + collection = api.create_collection(lp1.id, key="col1", title="Collection 1", created_by=None) + orig_modified = collection.modified + + with capture_events(expected_count=1) as captured: + api.update_collection(lp1.id, "col1", title="Updated Title") + + event = captured[0] + assert event.signal is LEARNING_PACKAGE_COLLECTION_CHANGED + assert event.kwargs["learning_package"].id == lp1.id + assert event.kwargs["change"] == CollectionChangeData( + collection_id=collection.id, + collection_code="col1", + modified=True, + ) + collection.refresh_from_db() + assert collection.modified > orig_modified + assert event.kwargs["metadata"].time == collection.modified + + +def test_update_collection_no_op(lp1: LearningPackage) -> None: + """ + Test that LEARNING_PACKAGE_COLLECTION_CHANGED is NOT emitted when + update_collection is called without any fields to update. + """ + api.create_collection(lp1.id, key="col1", title="Collection 1", created_by=None) + + with capture_events(expected_count=0): + # No title or description provided — the API short-circuits with no DB write. + api.update_collection(lp1.id, "col1") + + +# LEARNING_PACKAGE_COLLECTION_CHANGED — delete_collection + + +def test_delete_collection_soft(lp1: LearningPackage) -> None: + """ + Test that LEARNING_PACKAGE_COLLECTION_CHANGED is emitted with deleted=True + when a collection is soft-deleted (enabled=False). + """ + collection = api.create_collection(lp1.id, key="col1", title="Collection 1", created_by=None) + entity1 = _create_entity(lp1.id, "entity1") + entity2 = _create_entity(lp1.id, "entity2") + api.add_to_collection( + lp1.id, + "col1", + PublishableEntity.objects.filter(id__in=[entity1.id, entity2.id]), + ) + + with capture_events(expected_count=1) as captured: + api.delete_collection(lp1.id, "col1") + + event = captured[0] + assert event.signal is LEARNING_PACKAGE_COLLECTION_CHANGED + assert event.kwargs["learning_package"].id == lp1.id + assert event.kwargs["change"] == CollectionChangeData( + collection_id=collection.id, + collection_code="col1", + deleted=True, + entities_removed=sorted([entity1.id, entity2.id]), + ) + + +def test_delete_collection_hard(lp1: LearningPackage) -> None: + """ + Test that LEARNING_PACKAGE_COLLECTION_CHANGED is emitted with deleted=True and + entities_removed populated when a collection is hard-deleted. + """ + collection = api.create_collection(lp1.id, key="col1", title="Collection 1", created_by=None) + entity1 = _create_entity(lp1.id, "entity1") + entity2 = _create_entity(lp1.id, "entity2") + api.add_to_collection( + lp1.id, + "col1", + PublishableEntity.objects.filter(id__in=[entity1.id, entity2.id]), + ) + + collection_id = collection.id # Capture before deletion + + with capture_events(expected_count=1) as captured: + api.delete_collection(lp1.id, "col1", hard_delete=True) + + event = captured[0] + assert event.signal is LEARNING_PACKAGE_COLLECTION_CHANGED + assert event.kwargs["learning_package"].id == lp1.id + assert event.kwargs["change"] == CollectionChangeData( + collection_id=collection_id, + collection_code="col1", + deleted=True, + entities_removed=sorted([entity1.id, entity2.id]), + ) + + +# LEARNING_PACKAGE_COLLECTION_CHANGED — restore_collection + + +def test_restore_collection(lp1: LearningPackage) -> None: + """ + Test that LEARNING_PACKAGE_COLLECTION_CHANGED is emitted with created=True + when a soft-deleted collection is restored. + """ + collection = api.create_collection(lp1.id, key="col1", title="Collection 1", created_by=None) + api.delete_collection(lp1.id, "col1") # soft-delete first + + with capture_events(expected_count=1) as captured: + api.restore_collection(lp1.id, "col1") + + event = captured[0] + assert event.signal is LEARNING_PACKAGE_COLLECTION_CHANGED + assert event.kwargs["learning_package"].id == lp1.id + assert event.kwargs["change"] == CollectionChangeData( + collection_id=collection.id, + collection_code="col1", + created=True, + ) + + +# LEARNING_PACKAGE_COLLECTION_CHANGED — add_to_collection + + +def test_add_to_collection(lp1: LearningPackage) -> None: + """ + Test that LEARNING_PACKAGE_COLLECTION_CHANGED is emitted with the correct + entities_added list when entities are added to a collection. + """ + collection = api.create_collection(lp1.id, key="col1", title="Collection 1", created_by=None) + entity1 = _create_entity(lp1.id, "entity1") + entity2 = _create_entity(lp1.id, "entity2") + + with capture_events(expected_count=1) as captured: + api.add_to_collection( + lp1.id, + "col1", + PublishableEntity.objects.filter(id__in=[entity1.id, entity2.id]), + ) + + event = captured[0] + assert event.signal is LEARNING_PACKAGE_COLLECTION_CHANGED + assert event.kwargs["learning_package"].id == lp1.id + assert event.kwargs["change"] == CollectionChangeData( + collection_id=collection.id, + collection_code="col1", + entities_added=sorted([entity1.id, entity2.id]), + ) + + +def test_add_to_collection_aborted(lp1: LearningPackage) -> None: + """ + Test that no event is emitted when adding entities to a collection is rolled back. + """ + api.create_collection(lp1.id, key="col1", title="Collection 1", created_by=None) + entity1 = _create_entity(lp1.id, "entity1") + + with capture_events(expected_count=0): + with abort_transaction(): + api.add_to_collection( + lp1.id, + "col1", + PublishableEntity.objects.filter(id=entity1.id), + ) + + +# LEARNING_PACKAGE_COLLECTION_CHANGED — remove_from_collection + + +def test_remove_from_collection(lp1: LearningPackage) -> None: + """ + Test that LEARNING_PACKAGE_COLLECTION_CHANGED is emitted with the correct + entities_removed list when entities are removed from a collection. + """ + collection = api.create_collection(lp1.id, key="col1", title="Collection 1", created_by=None) + entity1 = _create_entity(lp1.id, "entity1") + entity2 = _create_entity(lp1.id, "entity2") + api.add_to_collection( + lp1.id, + "col1", + PublishableEntity.objects.filter(id__in=[entity1.id, entity2.id]), + ) + + with capture_events(expected_count=1) as captured: + api.remove_from_collection( + lp1.id, + "col1", + PublishableEntity.objects.filter(id=entity1.id), + ) + + event = captured[0] + assert event.signal is LEARNING_PACKAGE_COLLECTION_CHANGED + assert event.kwargs["learning_package"].id == lp1.id + assert event.kwargs["change"] == CollectionChangeData( + collection_id=collection.id, + collection_code="col1", + entities_removed=[entity1.id], + ) + + +# LEARNING_PACKAGE_COLLECTION_CHANGED — set_collections + + +def test_set_collections(lp1: LearningPackage, admin_user) -> None: + """ + Test that LEARNING_PACKAGE_COLLECTION_CHANGED is emitted once per affected + collection when set_collections reassigns an entity's collections. + + In this scenario entity starts in col1+col2, then is moved to col2+col3. + We expect two events: one for col1 (entity removed) and one for col3 (entity added). + col2 is unchanged so it should not emit an event. + """ + col1 = api.create_collection(lp1.id, key="col1", title="Collection 1", created_by=None) + col2 = api.create_collection(lp1.id, key="col2", title="Collection 2", created_by=None) + col3 = api.create_collection(lp1.id, key="col3", title="Collection 3", created_by=None) + entity = _create_entity(lp1.id, "entity1") + + # Put entity in col1 + col2 to start with + api.set_collections(entity, Collection.objects.filter(id__in=[col1.id, col2.id])) + + # Reassign: entity goes into col2 + col3 (col1 removed, col3 added) + with capture_events(expected_count=2) as captured: + api.set_collections(entity, Collection.objects.filter(id__in=[col2.id, col3.id]), created_by=admin_user.id) + + events_by_collection = {e.kwargs["change"].collection_id: e for e in captured} + assert set(events_by_collection.keys()) == {col1.id, col3.id} + + # col1: entity was removed + col1_removed_event = events_by_collection[col1.id].kwargs + assert col1_removed_event["changed_by"].user_id == admin_user.id + assert col1_removed_event["change"] == CollectionChangeData( + collection_id=col1.id, + collection_code="col1", + entities_removed=[entity.id], + ) + + # col3: entity was added + col3_added_event = events_by_collection[col3.id].kwargs + assert col1_removed_event["changed_by"].user_id == admin_user.id + assert col3_added_event["change"] == CollectionChangeData( + collection_id=col3.id, + collection_code="col3", + entities_added=[entity.id], + ) + # The collections were modified simultaneously: + assert col1_removed_event["metadata"].time == col3_added_event["metadata"].time + + +def test_set_collections_aborted(lp1: LearningPackage) -> None: + """ + Test that no events are emitted when set_collections is rolled back. + """ + col1 = api.create_collection(lp1.id, key="col1", title="Collection 1", created_by=None) + entity = _create_entity(lp1.id, "entity1") + + with capture_events(expected_count=0): + with abort_transaction(): + api.set_collections(entity, Collection.objects.filter(id=col1.id)) From f2e3ebae5fdd64608b1f88c3bf0742fee1e9565a Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Mon, 20 Apr 2026 09:55:53 -0700 Subject: [PATCH 13/30] fix: test inconsistency on MySQL vs SQLite --- src/openedx_content/applets/publishing/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/openedx_content/applets/publishing/api.py b/src/openedx_content/applets/publishing/api.py index 6f7fe8abe..f0cd44ca8 100644 --- a/src/openedx_content/applets/publishing/api.py +++ b/src/openedx_content/applets/publishing/api.py @@ -948,7 +948,7 @@ def _emit_event_for_change_log( new_version=record.new_version.version_num if record.new_version else None, direct=record.direct if isinstance(record, PublishLogRecord) else None, ) - for record in change_log.records.select_related("old_version", "new_version").all() + for record in change_log.records.order_by("id").select_related("old_version", "new_version").all() ] change_log_data: signals.DraftChangeLogEventData | signals.PublishLogEventData From c9ed092f61a1c9e920740780ab5350942a56531c Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Mon, 20 Apr 2026 09:55:59 -0700 Subject: [PATCH 14/30] fix: domain of the collections event --- src/openedx_content/applets/collections/signals.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/openedx_content/applets/collections/signals.py b/src/openedx_content/applets/collections/signals.py index 20c0f30ec..255d595de 100644 --- a/src/openedx_content/applets/collections/signals.py +++ b/src/openedx_content/applets/collections/signals.py @@ -37,7 +37,7 @@ class CollectionChangeData: LEARNING_PACKAGE_COLLECTION_CHANGED = OpenEdxPublicSignal( - event_type="org.openedx.content.publishing.lp_collection_changed.v1", + event_type="org.openedx.content.collections.lp_collection_changed.v1", data={ "learning_package": LearningPackageEventData, "changed_by": UserAttributionEventData, From df3adc3fd57cbaa8de87a53cb4464a93f0c8d243 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Mon, 20 Apr 2026 17:51:05 -0700 Subject: [PATCH 15/30] refactor: Collection modified -> metadata_modified in event data --- src/openedx_content/applets/collections/api.py | 6 +++--- src/openedx_content/applets/collections/signals.py | 2 +- tests/openedx_content/applets/collections/test_signals.py | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/openedx_content/applets/collections/api.py b/src/openedx_content/applets/collections/api.py index 2c6861fae..bb023b532 100644 --- a/src/openedx_content/applets/collections/api.py +++ b/src/openedx_content/applets/collections/api.py @@ -39,7 +39,7 @@ def _queue_change_event( collection: Collection, *, created: bool = False, - modified: bool = False, + metadata_modified: bool = False, deleted: bool = False, entities_added: list[PublishableEntity.ID] | None = None, entities_removed: list[PublishableEntity.ID] | None = None, @@ -61,7 +61,7 @@ def send_change_event(): collection_id=collection.id, collection_code=collection.collection_code, created=created, - modified=modified, + metadata_modified=metadata_modified, deleted=deleted, entities_added=entities_added or [], entities_removed=entities_removed or [], @@ -128,7 +128,7 @@ def update_collection( collection.description = description collection.save() - _queue_change_event(collection, modified=True) + _queue_change_event(collection, metadata_modified=True) return collection diff --git a/src/openedx_content/applets/collections/signals.py b/src/openedx_content/applets/collections/signals.py index 255d595de..e075d1ed3 100644 --- a/src/openedx_content/applets/collections/signals.py +++ b/src/openedx_content/applets/collections/signals.py @@ -25,7 +25,7 @@ class CollectionChangeData: collection_code: str created: bool = False """The collection is newly-created, or un-deleted. Some entities may be added simultaneously.""" - modified: bool = False + metadata_modified: bool = False """The collection's title/description has changed. Does not indicate whether or not entities were added/removed.""" deleted: bool = False """ diff --git a/tests/openedx_content/applets/collections/test_signals.py b/tests/openedx_content/applets/collections/test_signals.py index 63b1a2bc3..bce449105 100644 --- a/tests/openedx_content/applets/collections/test_signals.py +++ b/tests/openedx_content/applets/collections/test_signals.py @@ -93,7 +93,7 @@ def test_create_collection_aborted(lp1: LearningPackage) -> None: def test_update_collection(lp1: LearningPackage) -> None: """ - Test that LEARNING_PACKAGE_COLLECTION_CHANGED is emitted with modified=True + Test that LEARNING_PACKAGE_COLLECTION_CHANGED is emitted with metadata_modified=True when a collection's title or description is updated. """ collection = api.create_collection(lp1.id, "col1", title="Collection 1", created_by=None) @@ -108,7 +108,7 @@ def test_update_collection(lp1: LearningPackage) -> None: assert event.kwargs["change"] == CollectionChangeData( collection_id=collection.id, collection_code="col1", - modified=True, + metadata_modified=True, ) collection.refresh_from_db() assert collection.modified > orig_modified From 45446471c9c3cb78b6fb3f507c035d0286937fdc Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Mon, 20 Apr 2026 18:06:29 -0700 Subject: [PATCH 16/30] docs: explain lack of CONTENT_OBJECT_ASSOCIATIONS_CHANGED event --- src/openedx_content/applets/collections/signals.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/openedx_content/applets/collections/signals.py b/src/openedx_content/applets/collections/signals.py index e075d1ed3..0f93f5838 100644 --- a/src/openedx_content/applets/collections/signals.py +++ b/src/openedx_content/applets/collections/signals.py @@ -56,4 +56,16 @@ class CollectionChangeData: per-entity or that is possibly slow should dispatch an asynchronous task for processing the event. """ -# TODO: also emit an ENTITIES_META_CHANGED for this and tag changes? + +# Note: at present, the openedx_tagging code (in this repo) emits a +# CONTENT_OBJECT_ASSOCIATIONS_CHANGED event whenever an entity's tags change. +# But we do NOT emit the same event when an entity's collections change; rather +# we expect code in the platform to listen for +# LEARNING_PACKAGE_COLLECTION_CHANGED and then re-emit '...ASSOCIATIONS_CHANGED' +# as needed. The reason we don't emit the '...ASSOCIATIONS_CHANGED' event here +# is simple: we know the entity IDs but not their opaque keys, and all of the +# code that listens for that event expects the entity's opaque keys. +# The tagging code can do it here because the `object_id` in the tagging models +# _is_ the opaque key ("lb:..."), but the collections code is too low-level to +# know about opaque keys of the entities. We don't even know which learning +# context (which content library) a given entity is in. From bc66b61750e0675a87233d901fa499c95c74abd8 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Mon, 20 Apr 2026 18:27:53 -0700 Subject: [PATCH 17/30] feat: add Collections events to the openedx_content public API --- src/openedx_content/api_signals.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/openedx_content/api_signals.py b/src/openedx_content/api_signals.py index 185fe51f6..258305db3 100644 --- a/src/openedx_content/api_signals.py +++ b/src/openedx_content/api_signals.py @@ -4,4 +4,5 @@ # These wildcard imports are okay because these api modules declare __all__. # pylint: disable=wildcard-import +from .applets.collections.signals import * from .applets.publishing.signals import * From 9afae5525e216dea62e4bcb38db86f44126cdb5a Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Mon, 20 Apr 2026 22:14:50 -0700 Subject: [PATCH 18/30] docs: note a TODO --- src/openedx_content/applets/collections/api.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/openedx_content/applets/collections/api.py b/src/openedx_content/applets/collections/api.py index bb023b532..68c8bb196 100644 --- a/src/openedx_content/applets/collections/api.py +++ b/src/openedx_content/applets/collections/api.py @@ -308,7 +308,7 @@ def set_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. + # Update modified date: affected_collections = removed_collections | new_collections Collection.objects.filter(id__in=[collection.id for collection in affected_collections]).update( modified=datetime.now(tz=timezone.utc) @@ -321,6 +321,7 @@ def set_collections( for collection in Collection.objects.filter(id__in=[c.id for c in affected_collections]).select_related( "learning_package" ): + # TODO: test performance of this and potentially send these async if > 1 affected collection. if collection.id in removed_ids: _queue_change_event(collection, entities_removed=[publishable_entity.id], user_id=created_by) else: From fa81324c518e984937f50330fc24c80482d94748 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Mon, 20 Apr 2026 22:44:20 -0700 Subject: [PATCH 19/30] feat: emit event that entities_removed=... from collection when entities deleted --- .../applets/collections/signal_handlers.py | 36 ++++++++ .../applets/collections/tasks.py | 68 +++++++++++++++ src/openedx_content/apps.py | 6 +- .../applets/collections/conftest.py | 15 ++++ .../applets/collections/test_signals.py | 86 +++++++++++++++++++ 5 files changed, 208 insertions(+), 3 deletions(-) create mode 100644 src/openedx_content/applets/collections/signal_handlers.py create mode 100644 src/openedx_content/applets/collections/tasks.py create mode 100644 tests/openedx_content/applets/collections/conftest.py diff --git a/src/openedx_content/applets/collections/signal_handlers.py b/src/openedx_content/applets/collections/signal_handlers.py new file mode 100644 index 000000000..14eab28e6 --- /dev/null +++ b/src/openedx_content/applets/collections/signal_handlers.py @@ -0,0 +1,36 @@ +"""Signal handlers for collections-related updates.""" + +from functools import partial + +from django.db import transaction +from django.dispatch import receiver + +from ..publishing.signals import LEARNING_PACKAGE_ENTITIES_CHANGED, DraftChangeLogEventData, UserAttributionEventData +from .tasks import emit_collections_changed_for_deleted_entities_task + + +@receiver(LEARNING_PACKAGE_ENTITIES_CHANGED) +def on_entities_changed( + change_log: DraftChangeLogEventData, + changed_by: UserAttributionEventData, + **kwargs, +): + """ + When entity drafts are deleted, update all affected collections. + + Finds all deleted entities (new_version=None) from the change log and + dispatches a task to emit LEARNING_PACKAGE_COLLECTION_CHANGED for any + collections that contain those entities. + """ + deleted_entity_ids = [record.entity_id for record in change_log.changes if record.new_version is None] + + if not deleted_entity_ids: + return + + transaction.on_commit( + partial( + emit_collections_changed_for_deleted_entities_task.delay, + entity_ids=deleted_entity_ids, + user_id=changed_by.user_id, + ) + ) diff --git a/src/openedx_content/applets/collections/tasks.py b/src/openedx_content/applets/collections/tasks.py new file mode 100644 index 000000000..74aaf7034 --- /dev/null +++ b/src/openedx_content/applets/collections/tasks.py @@ -0,0 +1,68 @@ +"""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 ( + LEARNING_PACKAGE_COLLECTION_CHANGED, + CollectionChangeData, + LearningPackageEventData, + UserAttributionEventData, +) + +logger = logging.getLogger(__name__) + + +@shared_task +def emit_collections_changed_for_deleted_entities_task( + entity_ids: list[int], + user_id: int | None, +) -> int: + """ + Emit LEARNING_PACKAGE_COLLECTION_CHANGED for each collection that contains + any of the given (now-deleted) entities, listing them as entities_removed. + + Triggered by LEARNING_PACKAGE_ENTITIES_CHANGED when entity drafts are deleted. + """ + affected_cpes = ( + CollectionPublishableEntity.objects + .filter(entity_id__in=entity_ids) + .select_related("collection__learning_package") + .order_by("collection_id", "entity_id") + ) + + collection_map: dict[int, Collection] = {} + entity_map: dict[int, list[PublishableEntity.ID]] = defaultdict(list) + for cpe in affected_cpes: + collection_map[cpe.collection_id] = cpe.collection + entity_map[cpe.collection_id].append(cpe.entity_id) + + emitted_events = 0 + for collection_id, collection in collection_map.items(): + # .. event_implemented_name: LEARNING_PACKAGE_COLLECTION_CHANGED + # .. event_type: org.openedx.content.collections.lp_collection_changed.v1 + LEARNING_PACKAGE_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=entity_map[collection_id], + ), + ) + emitted_events += 1 + + logger.info( + "Entities deleted (ids: %s): emitted LEARNING_PACKAGE_COLLECTION_CHANGED for %s collections.", + entity_ids, + emitted_events, + ) + return emitted_events diff --git a/src/openedx_content/apps.py b/src/openedx_content/apps.py index 563d5d03a..e89181377 100644 --- a/src/openedx_content/apps.py +++ b/src/openedx_content/apps.py @@ -45,8 +45,8 @@ def register_publishable_models(self): def ready(self): """ - Currently used to register publishable models. - - May later be used to register signal handlers as well. + Currently used to register publishable models and signal handlers. """ self.register_publishable_models() + # Import signal handlers so Django registers all @receiver callbacks. + from .applets.collections import signal_handlers # pylint: disable=unused-import diff --git a/tests/openedx_content/applets/collections/conftest.py b/tests/openedx_content/applets/collections/conftest.py new file mode 100644 index 000000000..f0996b32c --- /dev/null +++ b/tests/openedx_content/applets/collections/conftest.py @@ -0,0 +1,15 @@ +"""Shared fixtures for collection tests.""" + +import pytest +from celery import current_app # type: ignore[import] + + +@pytest.fixture(autouse=True) +def _celery_task_always_eager(): + """ + Run Celery tasks synchronously so per-entity CONTENT_OBJECT_ASSOCIATIONS_CHANGED + events fire inline during tests without needing a real broker. + """ + current_app.conf.task_always_eager = True + yield + current_app.conf.task_always_eager = False diff --git a/tests/openedx_content/applets/collections/test_signals.py b/tests/openedx_content/applets/collections/test_signals.py index bce449105..e4cd82cf1 100644 --- a/tests/openedx_content/applets/collections/test_signals.py +++ b/tests/openedx_content/applets/collections/test_signals.py @@ -349,3 +349,89 @@ def test_set_collections_aborted(lp1: LearningPackage) -> None: with capture_events(expected_count=0): with abort_transaction(): api.set_collections(entity, Collection.objects.filter(id=col1.id)) + + +# LEARNING_PACKAGE_COLLECTION_CHANGED — on entity draft deletion + + +def _create_entity_with_version(learning_package_id: LearningPackage.ID, key: str) -> PublishableEntity: + """Helper: create a PublishableEntity with an initial draft version (so its draft can be deleted).""" + entity = api.create_publishable_entity(learning_package_id, key=key, created=now_time, created_by=None) + api.create_publishable_entity_version(entity.id, version_num=1, title=key, created=now_time, created_by=None) + return entity + + +def test_entity_draft_deleted_in_collection(lp1: LearningPackage, admin_user) -> None: + """ + Test that LEARNING_PACKAGE_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=[LEARNING_PACKAGE_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 LEARNING_PACKAGE_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: + """ + Test that LEARNING_PACKAGE_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) + 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)) + + with capture_events(signals=[LEARNING_PACKAGE_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 LEARNING_PACKAGE_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=[LEARNING_PACKAGE_COLLECTION_CHANGED], expected_count=0): + api.soft_delete_draft(entity.id) + + +def test_entity_draft_deleted_aborted(lp1: LearningPackage) -> None: + """ + Test that no LEARNING_PACKAGE_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=[LEARNING_PACKAGE_COLLECTION_CHANGED], expected_count=0): + with abort_transaction(): + api.soft_delete_draft(entity.id) From 140e1b354c5248ef479ddbb147b3d5a9b4e1728e Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 21 Apr 2026 17:10:41 -0700 Subject: [PATCH 20/30] feat: emit event that entities_added=... from collection when entities un-deleted --- .../applets/collections/signal_handlers.py | 30 ++++--- .../applets/collections/tasks.py | 49 +++++++--- .../applets/collections/test_signals.py | 89 +++++++++++++++++++ 3 files changed, 144 insertions(+), 24 deletions(-) diff --git a/src/openedx_content/applets/collections/signal_handlers.py b/src/openedx_content/applets/collections/signal_handlers.py index 14eab28e6..b7e697cc9 100644 --- a/src/openedx_content/applets/collections/signal_handlers.py +++ b/src/openedx_content/applets/collections/signal_handlers.py @@ -6,7 +6,7 @@ from django.dispatch import receiver from ..publishing.signals import LEARNING_PACKAGE_ENTITIES_CHANGED, DraftChangeLogEventData, UserAttributionEventData -from .tasks import emit_collections_changed_for_deleted_entities_task +from .tasks import emit_collections_changed_for_entity_changes_task @receiver(LEARNING_PACKAGE_ENTITIES_CHANGED) @@ -16,21 +16,31 @@ def on_entities_changed( **kwargs, ): """ - When entity drafts are deleted, update all affected collections. + When entity drafts are deleted or restored, notify affected collections. - Finds all deleted entities (new_version=None) from the change log and - dispatches a task to emit LEARNING_PACKAGE_COLLECTION_CHANGED for any - collections that contain those entities. + Dispatches a task to emit LEARNING_PACKAGE_COLLECTION_CHANGED for any + collections that contain the changed entities. """ - deleted_entity_ids = [record.entity_id for record in change_log.changes if record.new_version is None] - - if not deleted_entity_ids: + removed_entity_ids = [record.entity_id for record in change_log.changes if record.new_version is None] + # old_version=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 is None and record.new_version is not None + ] + + if not removed_entity_ids and not added_entity_ids: return transaction.on_commit( partial( - emit_collections_changed_for_deleted_entities_task.delay, - entity_ids=deleted_entity_ids, + 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/tasks.py b/src/openedx_content/applets/collections/tasks.py index 74aaf7034..7205a49f6 100644 --- a/src/openedx_content/applets/collections/tasks.py +++ b/src/openedx_content/applets/collections/tasks.py @@ -18,28 +18,45 @@ @shared_task -def emit_collections_changed_for_deleted_entities_task( - entity_ids: list[int], +def emit_collections_changed_for_entity_changes_task( + removed_entity_ids: list[int], + added_entity_ids: list[int], user_id: int | None, ) -> int: """ - Emit LEARNING_PACKAGE_COLLECTION_CHANGED for each collection that contains - any of the given (now-deleted) entities, listing them as entities_removed. + Emit LEARNING_PACKAGE_COLLECTION_CHANGED for each collection affected by + entity draft deletions or restorations. - Triggered by LEARNING_PACKAGE_ENTITIES_CHANGED when entity drafts are deleted. + 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 LEARNING_PACKAGE_ENTITIES_CHANGED. New entities (old_version=None + → new_version 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=entity_ids) + .filter(entity_id__in=all_entity_ids) .select_related("collection__learning_package") .order_by("collection_id", "entity_id") ) collection_map: dict[int, Collection] = {} - entity_map: dict[int, list[PublishableEntity.ID]] = defaultdict(list) + 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 - entity_map[cpe.collection_id].append(cpe.entity_id) + 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(): @@ -55,14 +72,18 @@ def emit_collections_changed_for_deleted_entities_task( change=CollectionChangeData( collection_id=collection.id, collection_code=collection.collection_code, - entities_removed=entity_map[collection_id], + entities_removed=removed_map[collection_id], + entities_added=added_map[collection_id], ), ) emitted_events += 1 - logger.info( - "Entities deleted (ids: %s): emitted LEARNING_PACKAGE_COLLECTION_CHANGED for %s collections.", - entity_ids, - emitted_events, - ) + if emitted_events: + logger.info( + "Entity draft changes (removed=%s, added=%s): emitted LEARNING_PACKAGE_COLLECTION_CHANGED" + " for %s collections.", + removed_entity_ids, + added_entity_ids, + emitted_events, + ) return emitted_events diff --git a/tests/openedx_content/applets/collections/test_signals.py b/tests/openedx_content/applets/collections/test_signals.py index e4cd82cf1..12d0fdde5 100644 --- a/tests/openedx_content/applets/collections/test_signals.py +++ b/tests/openedx_content/applets/collections/test_signals.py @@ -435,3 +435,92 @@ def test_entity_draft_deleted_aborted(lp1: LearningPackage) -> None: with capture_events(signals=[LEARNING_PACKAGE_COLLECTION_CHANGED], expected_count=0): with abort_transaction(): api.soft_delete_draft(entity.id) + + +# LEARNING_PACKAGE_COLLECTION_CHANGED — on entity draft restore (deletion reverted) + + +def test_entity_draft_restored_in_collection(lp1: LearningPackage) -> None: + """ + Test that LEARNING_PACKAGE_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=[LEARNING_PACKAGE_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 LEARNING_PACKAGE_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 LEARNING_PACKAGE_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=[LEARNING_PACKAGE_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_aborted(lp1: LearningPackage) -> None: + """ + Test that no LEARNING_PACKAGE_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=[LEARNING_PACKAGE_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 LEARNING_PACKAGE_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=[LEARNING_PACKAGE_COLLECTION_CHANGED], expected_count=0): + _create_entity_with_version(lp1.id, "entity1") From d3708526557dae6f1ecca180128a9580f627af9e Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 21 Apr 2026 19:36:27 -0700 Subject: [PATCH 21/30] docs: mention transaction commit --- src/openedx_content/applets/collections/signals.py | 2 ++ src/openedx_content/applets/publishing/signals.py | 4 ++++ 2 files changed, 6 insertions(+) diff --git a/src/openedx_content/applets/collections/signals.py b/src/openedx_content/applets/collections/signals.py index 0f93f5838..1ea4148c3 100644 --- a/src/openedx_content/applets/collections/signals.py +++ b/src/openedx_content/applets/collections/signals.py @@ -52,6 +52,8 @@ class CollectionChangeData: information available. It does not distinguish between Containers, Components, or other entity types. +💾 This event is only emitted after any transaction has been committed. + ⏳ This **batch** event is emitted **synchronously**. Handlers that do anything per-entity or that is possibly slow should dispatch an asynchronous task for processing the event. diff --git a/src/openedx_content/applets/publishing/signals.py b/src/openedx_content/applets/publishing/signals.py index 43feb29a6..86a2afc6e 100644 --- a/src/openedx_content/applets/publishing/signals.py +++ b/src/openedx_content/applets/publishing/signals.py @@ -104,6 +104,8 @@ class PublishLogEventData: Collections and tags are not `PublishableEntity`-based, so do not participate in this event. +💾 This event is only emitted after any transaction has been committed. + ⏳ This **batch** event is emitted **synchronously**. Handlers that do anything per-entity or that is possibly slow should dispatch an asynchronous task for processing the event. @@ -138,6 +140,8 @@ class PublishLogEventData: Collections and tags are not `PublishableEntity`-based, so do not participate in this event. +💾 This event is only emitted after any transaction has been committed. + ⏳ This **batch** event is emitted **synchronously**. Handlers that do anything per-entity or that is possibly slow should dispatch an asynchronous task for processing the event. From f17cec51ffeaec8e29c67284c743bc44ded43100 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 22 Apr 2026 09:38:06 -0700 Subject: [PATCH 22/30] feat: emit version IDs in events as well as version numbers --- .../applets/collections/signal_handlers.py | 6 +-- .../applets/collections/tasks.py | 5 +- src/openedx_content/applets/publishing/api.py | 2 + .../applets/publishing/signals.py | 10 ++-- .../applets/publishing/test_signals.py | 50 ++++++++++++------- 5 files changed, 47 insertions(+), 26 deletions(-) diff --git a/src/openedx_content/applets/collections/signal_handlers.py b/src/openedx_content/applets/collections/signal_handlers.py index b7e697cc9..d47c8176f 100644 --- a/src/openedx_content/applets/collections/signal_handlers.py +++ b/src/openedx_content/applets/collections/signal_handlers.py @@ -21,8 +21,8 @@ def on_entities_changed( Dispatches a task to emit LEARNING_PACKAGE_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 is None] - # old_version=None covers both brand-new entities and restored soft-deletes; we can't distinguish + 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 @@ -30,7 +30,7 @@ def on_entities_changed( added_entity_ids = [ record.entity_id for record in change_log.changes - if record.old_version is None and record.new_version is not None + 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: diff --git a/src/openedx_content/applets/collections/tasks.py b/src/openedx_content/applets/collections/tasks.py index 7205a49f6..7993de43b 100644 --- a/src/openedx_content/applets/collections/tasks.py +++ b/src/openedx_content/applets/collections/tasks.py @@ -31,8 +31,9 @@ def emit_collections_changed_for_entity_changes_task( 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 LEARNING_PACKAGE_ENTITIES_CHANGED. New entities (old_version=None - → new_version is not None) that aren't in any collection result in a no-op. + Triggered by LEARNING_PACKAGE_ENTITIES_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: diff --git a/src/openedx_content/applets/publishing/api.py b/src/openedx_content/applets/publishing/api.py index f0cd44ca8..49ab2bfe5 100644 --- a/src/openedx_content/applets/publishing/api.py +++ b/src/openedx_content/applets/publishing/api.py @@ -945,7 +945,9 @@ def _emit_event_for_change_log( signals.ChangeLogRecordData( entity_id=record.entity_id, old_version=record.old_version.version_num if record.old_version else None, + old_version_id=record.old_version_id, 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, ) for record in change_log.records.order_by("id").select_related("old_version", "new_version").all() diff --git a/src/openedx_content/applets/publishing/signals.py b/src/openedx_content/applets/publishing/signals.py index 86a2afc6e..b7ef14138 100644 --- a/src/openedx_content/applets/publishing/signals.py +++ b/src/openedx_content/applets/publishing/signals.py @@ -44,14 +44,18 @@ class ChangeLogRecordData: entity_id: PublishableEntity.ID old_version: int | None + """The old version number of this entity. None if newly-created or un-deleted.""" + old_version_id: int | None """ - The old version_num of this entity (not the PublishableEntityVersion ID). - This is None if the entity is newly created. + The old version of this entity (the PublishableEntityVersion ID). + This is None if the entity is newly created (or un-deleted). """ new_version: int | None + """The old version number of this entity. None if newly-created or un-deleted.""" + new_version_id: int | None """ - The new version_num of this entity (not the PublishableEntityVersion ID). + The new version of this entity (the PublishableEntityVersion ID. This is None if the entity is now deleted. """ diff --git a/tests/openedx_content/applets/publishing/test_signals.py b/tests/openedx_content/applets/publishing/test_signals.py index dda95752f..d134bc6cb 100644 --- a/tests/openedx_content/applets/publishing/test_signals.py +++ b/tests/openedx_content/applets/publishing/test_signals.py @@ -21,6 +21,20 @@ def publish_entity(obj: PublishableEntity) -> PublishLog: return api.publish_from_drafts(lp_id, draft_qset=api.get_all_drafts(lp_id).filter(entity=obj)) +def change_record(obj: PublishableEntity, old_version: int | None, new_version: int | None, direct: bool | None = None): + """Helper function to construct ChangeLogRecordData() using only version numbers instead of numbers+IDs""" + old_version_id = obj.versions.get(version_num=old_version).id if old_version is not None else None + new_version_id = obj.versions.get(version_num=new_version).id if new_version is not None else None + return api.signals.ChangeLogRecordData( + entity_id=obj.id, + old_version=old_version, + old_version_id=old_version_id, + new_version=new_version, + new_version_id=new_version_id, + direct=direct, + ) + + # LEARNING_PACKAGE_ENTITIES_CHANGED @@ -55,7 +69,7 @@ def test_single_entity_changed() -> None: assert event.kwargs["changed_by"].user_id is None assert event.kwargs["change_log"].draft_change_log_id == expected_draft_change_log_id assert event.kwargs["change_log"].changes == [ - api.signals.ChangeLogRecordData(entity_id=entity.id, old_version=None, new_version=NEW_VERSION_NUM), + change_record(entity, old_version=None, new_version=NEW_VERSION_NUM), ] assert event.kwargs["metadata"].time == now_time @@ -116,11 +130,11 @@ def test_multiple_entites_changed(admin_user) -> None: assert event.kwargs["change_log"].draft_change_log_id == draft_change_log.id assert event.kwargs["change_log"].changes == [ # Entity 1 jumps from no version to version 2: - api.signals.ChangeLogRecordData(entity_id=entity1.id, old_version=None, new_version=2), + change_record(entity1, old_version=None, new_version=2), # Entity 2 jumps v1 -> v2: - api.signals.ChangeLogRecordData(entity_id=entity2.id, old_version=1, new_version=2), + change_record(entity2, old_version=1, new_version=2), # Entity 3 gets deleted: - api.signals.ChangeLogRecordData(entity_id=entity3.id, old_version=1, new_version=None), + change_record(entity3, old_version=1, new_version=None), ] assert event.kwargs["metadata"].time == now_time @@ -182,8 +196,8 @@ def create_entity(name: str, dependencies: list[PublishableEntity.ID] | None = N event = captured[0] assert event.signal is api.signals.LEARNING_PACKAGE_ENTITIES_CHANGED assert event.kwargs["change_log"].changes == [ - api.signals.ChangeLogRecordData(entity_id=child1.id, old_version=1, new_version=2), # directly modified - api.signals.ChangeLogRecordData(entity_id=parent1.id, old_version=1, new_version=1), # side effect + change_record(child1, old_version=1, new_version=2), # directly modified + change_record(parent1, old_version=1, new_version=1), # side effect ] @@ -224,9 +238,9 @@ def test_publish_events(admin_user) -> None: assert event.kwargs["change_log"].changes == [ # Entity 1 is not yet published, since it has no draft version. # Entity 2 is newly published, and now at v2: - api.signals.ChangeLogRecordData(entity_id=entity2.id, old_version=None, new_version=2, direct=True), + change_record(entity2, old_version=None, new_version=2, direct=True), # Entity 3 is newly published, and now at v1: - api.signals.ChangeLogRecordData(entity_id=entity3.id, old_version=None, new_version=1, direct=True), + change_record(entity3, old_version=None, new_version=1, direct=True), ] assert event.kwargs["metadata"].time == first_publish_time @@ -253,11 +267,11 @@ def test_publish_events(admin_user) -> None: assert event.kwargs["change_log"].publish_log_id == second_log.id assert event.kwargs["change_log"].changes == [ # Entity 1 is newly published at v1: - api.signals.ChangeLogRecordData(entity_id=entity1.id, old_version=None, new_version=1, direct=True), + change_record(entity1, old_version=None, new_version=1, direct=True), # Entity 2 jumps v2 -> v3: - api.signals.ChangeLogRecordData(entity_id=entity2.id, old_version=2, new_version=3, direct=True), + change_record(entity2, old_version=2, new_version=3, direct=True), # Entity 3 gets deleted: - api.signals.ChangeLogRecordData(entity_id=entity3.id, old_version=1, new_version=None, direct=True), + change_record(entity3, old_version=1, new_version=None, direct=True), ] assert event.kwargs["metadata"].time == second_publish_time @@ -323,10 +337,10 @@ def create_entity(name: str, dependencies: list[PublishableEntity.ID] | None = N event = captured[0] assert event.signal is api.signals.LEARNING_PACKAGE_ENTITIES_PUBLISHED assert event.kwargs["change_log"].changes == [ - api.signals.ChangeLogRecordData(entity_id=grandparent1.id, old_version=None, new_version=1, direct=True), - api.signals.ChangeLogRecordData(entity_id=parent1.id, old_version=None, new_version=1, direct=False), - api.signals.ChangeLogRecordData(entity_id=child1.id, old_version=None, new_version=1, direct=False), - api.signals.ChangeLogRecordData(entity_id=child2.id, old_version=None, new_version=1, direct=False), + change_record(grandparent1, old_version=None, new_version=1, direct=True), + change_record(parent1, old_version=None, new_version=1, direct=False), + change_record(child1, old_version=None, new_version=1, direct=False), + change_record(child2, old_version=None, new_version=1, direct=False), ] # publish the rest: @@ -342,7 +356,7 @@ def create_entity(name: str, dependencies: list[PublishableEntity.ID] | None = N event = captured[0] assert event.signal is api.signals.LEARNING_PACKAGE_ENTITIES_PUBLISHED assert event.kwargs["change_log"].changes == [ - api.signals.ChangeLogRecordData(entity_id=child3.id, old_version=1, new_version=2, direct=True), - api.signals.ChangeLogRecordData(entity_id=parent2.id, old_version=1, new_version=1, direct=False), - api.signals.ChangeLogRecordData(entity_id=grandparent2.id, old_version=1, new_version=1, direct=False), + change_record(child3, old_version=1, new_version=2, direct=True), + change_record(parent2, old_version=1, new_version=1, direct=False), + change_record(grandparent2, old_version=1, new_version=1, direct=False), ] From 627c574e7f7342ebd71bc74ef4264ce24b61f585 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 22 Apr 2026 15:31:24 -0700 Subject: [PATCH 23/30] test: disable celery for all publishing tests, not just collections --- tests/openedx_content/{applets/collections => }/conftest.py | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename tests/openedx_content/{applets/collections => }/conftest.py (100%) diff --git a/tests/openedx_content/applets/collections/conftest.py b/tests/openedx_content/conftest.py similarity index 100% rename from tests/openedx_content/applets/collections/conftest.py rename to tests/openedx_content/conftest.py From 7b62c3e0cbdcd48d609a18656f8062d375f4c08a Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 23 Apr 2026 09:08:22 -0700 Subject: [PATCH 24/30] feat: LP created/deleted events Co-Authored-By: Claude --- src/openedx_content/applets/publishing/api.py | 8 ++ .../applets/publishing/signal_handlers.py | 42 +++++++++ .../applets/publishing/signals.py | 61 +++++++++++- src/openedx_content/apps.py | 1 + .../applets/publishing/test_signals.py | 94 ++++++++++++++++++- 5 files changed, 203 insertions(+), 3 deletions(-) create mode 100644 src/openedx_content/applets/publishing/signal_handlers.py diff --git a/src/openedx_content/applets/publishing/api.py b/src/openedx_content/applets/publishing/api.py index 49ab2bfe5..674e52cf8 100644 --- a/src/openedx_content/applets/publishing/api.py +++ b/src/openedx_content/applets/publishing/api.py @@ -111,6 +111,14 @@ def create_learning_package( ) package.full_clean() package.save() + new_id = package.id + + def send_event(): + signals.LEARNING_PACKAGE_CREATED.send_event( + learning_package=signals.LearningPackageEventData(id=new_id, title=title), + ) + + on_commit(send_event) return package diff --git a/src/openedx_content/applets/publishing/signal_handlers.py b/src/openedx_content/applets/publishing/signal_handlers.py new file mode 100644 index 000000000..6e5d7f63b --- /dev/null +++ b/src/openedx_content/applets/publishing/signal_handlers.py @@ -0,0 +1,42 @@ +""" +Django signal handlers for the publishing applet. +""" + +from django.db import transaction +from django.db.models.signals import post_delete +from django.dispatch import receiver + +from .models.learning_package import LearningPackage +from .signals import LEARNING_PACKAGE_DELETED, LearningPackageEventData + + +@receiver(post_delete, sender=LearningPackage) +def _emit_learning_package_deleted(sender, instance, **kwargs): # pylint: disable=unused-argument + """ + Emit ``LEARNING_PACKAGE_DELETED`` after a ``LearningPackage`` is deleted. + + This fires for any deletion: single-object ``.delete()``, bulk + ``QuerySet.delete()`` (Django calls ``post_delete`` once per row), or + deletions performed via the Django admin. There is currently no official API + for deleting Learning Packages, but you can orhpan them by deleting any + references to them such as ``ContentLibrary`` instances in openedx-platform. + + The event is deferred via ``transaction.on_commit`` so that it is only + emitted once the enclosing database transaction has been committed. If + the transaction is rolled back, the row still exists and no event fires. + + Note: by the time this handler runs, the ``LearningPackage`` row has + already been removed from the database (Django preserves ``instance.pk`` + on the in-memory object, but the DB row is gone). We capture ``id`` and + ``title`` at handler-invocation time so that the event payload remains + correct even though the underlying record is no longer retrievable. + """ + lp_id = instance.id + lp_title = instance.title + + def send_event(): + LEARNING_PACKAGE_DELETED.send_event( + learning_package=LearningPackageEventData(id=lp_id, title=lp_title), + ) + + transaction.on_commit(send_event) diff --git a/src/openedx_content/applets/publishing/signals.py b/src/openedx_content/applets/publishing/signals.py index b7ef14138..4f716be88 100644 --- a/src/openedx_content/applets/publishing/signals.py +++ b/src/openedx_content/applets/publishing/signals.py @@ -17,6 +17,8 @@ "DraftChangeLogEventData", "PublishLogEventData", # All events: + "LEARNING_PACKAGE_CREATED", + "LEARNING_PACKAGE_DELETED", "LEARNING_PACKAGE_ENTITIES_CHANGED", "LEARNING_PACKAGE_ENTITIES_PUBLISHED", ] @@ -82,6 +84,59 @@ class PublishLogEventData: changes: list[ChangeLogRecordData] +LEARNING_PACKAGE_CREATED = OpenEdxPublicSignal( + event_type="org.openedx.content.publishing.lp_created.v1", + data={ + "learning_package": LearningPackageEventData, + }, +) +""" +A new ``LearningPackage`` has been created. + +This is emitted exactly once per ``LearningPackage``, after the row is inserted +in the database. This is a low-level event. It's most likely that the Learning +Package is still being prepared/populated, and any necessary relationships, +entities, metadata, or other data may not yet exist at the time this event is +emitted. + +💾 This event is only emitted after the enclosing database transaction has +been committed. If the transaction is rolled back, no event is emitted. + +⏳ This event is emitted synchronously. +""" + + +LEARNING_PACKAGE_DELETED = OpenEdxPublicSignal( + event_type="org.openedx.content.publishing.lp_deleted.v1", + data={ + "learning_package": LearningPackageEventData, + }, +) +""" +A ``LearningPackage`` has been deleted. + +This is emitted exactly once per ``LearningPackage``, after the row has been +removed from the database. It is emitted regardless of how the row was deleted +(via a direct ORM ``.delete()`` call, via the Django admin, or as part of a +``QuerySet.delete()``), because it is fired by a Django ``post_delete`` signal +on the ``LearningPackage`` model. + +Note: at the time this event is emitted, the ``LearningPackage`` and all of +its related content (entities, versions, drafts, publishes, etc.) have already +been removed from the database. Handlers cannot look up the learning package +by ID — they only get the ``id`` and ``title`` that are captured in the +``LearningPackageEventData`` payload. + +🗑️ Unlike other ``publishing`` events, the effects of this deletion are +completely irreversible and the LearningPackage cannot be restored/un-deleted. + +💾 This event is only emitted after the enclosing database transaction has +been committed. If the transaction is rolled back, no event is emitted. + +⏳ This event is emitted synchronously. +""" + + LEARNING_PACKAGE_ENTITIES_CHANGED = OpenEdxPublicSignal( event_type="org.openedx.content.publishing.lp_entities_changed.v1", data={ @@ -108,7 +163,8 @@ class PublishLogEventData: Collections and tags are not `PublishableEntity`-based, so do not participate in this event. -💾 This event is only emitted after any transaction has been committed. +💾 This event is only emitted after the enclosing database transaction has +been committed. If the transaction is rolled back, no event is emitted. ⏳ This **batch** event is emitted **synchronously**. Handlers that do anything per-entity or that is possibly slow should dispatch an asynchronous task for @@ -144,7 +200,8 @@ class PublishLogEventData: Collections and tags are not `PublishableEntity`-based, so do not participate in this event. -💾 This event is only emitted after any transaction has been committed. +💾 This event is only emitted after the enclosing database transaction has +been committed. If the transaction is rolled back, no event is emitted. ⏳ This **batch** event is emitted **synchronously**. Handlers that do anything per-entity or that is possibly slow should dispatch an asynchronous task for diff --git a/src/openedx_content/apps.py b/src/openedx_content/apps.py index e89181377..9f2e74f2d 100644 --- a/src/openedx_content/apps.py +++ b/src/openedx_content/apps.py @@ -50,3 +50,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/publishing/test_signals.py b/tests/openedx_content/applets/publishing/test_signals.py index d134bc6cb..42dd18840 100644 --- a/tests/openedx_content/applets/publishing/test_signals.py +++ b/tests/openedx_content/applets/publishing/test_signals.py @@ -8,7 +8,7 @@ import pytest from openedx_content import api -from openedx_content.models_api import PublishableEntity, PublishLog +from openedx_content.models_api import LearningPackage, PublishableEntity, PublishLog from tests.utils import abort_transaction, capture_events pytestmark = pytest.mark.django_db(transaction=True) @@ -35,6 +35,98 @@ def change_record(obj: PublishableEntity, old_version: int | None, new_version: ) +# LEARNING_PACKAGE_CREATED + + +def test_learning_package_created() -> None: + """ + Test that LEARNING_PACKAGE_CREATED is emitted when a new ``LearningPackage`` + is created. + """ + with capture_events(signals=[api.signals.LEARNING_PACKAGE_CREATED], expected_count=1) as captured: + learning_package = api.create_learning_package(key="lp1", title="Test LP 📦") + + event = captured[0] + assert event.signal is api.signals.LEARNING_PACKAGE_CREATED + assert event.kwargs["learning_package"].id == learning_package.id + assert event.kwargs["learning_package"].title == "Test LP 📦" + + +def test_learning_package_created_not_emitted_on_update() -> None: + """ + Test that updating an existing ``LearningPackage`` does NOT emit + LEARNING_PACKAGE_CREATED. The event is only for new rows. + """ + learning_package = api.create_learning_package(key="lp1", title="Test LP 📦") + + with capture_events(signals=[api.signals.LEARNING_PACKAGE_CREATED], expected_count=0): + api.update_learning_package(learning_package.id, title="Updated Title") + + +def test_learning_package_created_aborted() -> None: + """ + Test that LEARNING_PACKAGE_CREATED is NOT emitted when the transaction + that created the ``LearningPackage`` is rolled back. + """ + with capture_events(signals=[api.signals.LEARNING_PACKAGE_CREATED], expected_count=0): + with abort_transaction(): + api.create_learning_package(key="lp1", title="Test LP 📦") + + +# LEARNING_PACKAGE_DELETED + + +def test_learning_package_deleted() -> None: + """ + Test that LEARNING_PACKAGE_DELETED is emitted when a ``LearningPackage`` + is deleted. + """ + learning_package = api.create_learning_package(key="lp1", title="Test LP 📦") + lp_id = learning_package.id + + with capture_events(signals=[api.signals.LEARNING_PACKAGE_DELETED], expected_count=1) as captured: + learning_package.delete() + + event = captured[0] + assert event.signal is api.signals.LEARNING_PACKAGE_DELETED + assert event.kwargs["learning_package"].id == lp_id + assert event.kwargs["learning_package"].title == "Test LP 📦" + + +def test_learning_package_deleted_via_queryset() -> None: + """ + Test that LEARNING_PACKAGE_DELETED fires once per row when multiple + ``LearningPackage`` instances are deleted via a ``QuerySet.delete()``. + """ + lp1 = api.create_learning_package(key="lp1", title="LP 1") + lp2 = api.create_learning_package(key="lp2", title="LP 2") + + with capture_events(signals=[api.signals.LEARNING_PACKAGE_DELETED], expected_count=2) as captured: + LearningPackage.objects.filter(id__in=[lp1.id, lp2.id]).delete() + + deleted_ids = {event.kwargs["learning_package"].id for event in captured} + assert deleted_ids == {lp1.id, lp2.id} + + +def test_learning_package_deleted_aborted() -> None: + """ + Test that LEARNING_PACKAGE_DELETED is NOT emitted when the transaction + that would have deleted the ``LearningPackage`` is rolled back. + """ + learning_package = api.create_learning_package(key="lp1", title="Test LP 📦") + lp_id = learning_package.id + + with capture_events(signals=[api.signals.LEARNING_PACKAGE_DELETED], expected_count=0): + with abort_transaction(): + learning_package.delete() + + # Confirm it's still in the database (the row survived the rollback). + # Note: we can't use ``learning_package.id`` here because Django sets + # ``instance.id = None`` after ``.delete()``, even if the transaction + # ultimately rolls back; that's why we captured it beforehand. + assert LearningPackage.objects.filter(id=lp_id).exists() + + # LEARNING_PACKAGE_ENTITIES_CHANGED From d3537e4dc59255f6f4ed84ae3357ce499406d5ea Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 23 Apr 2026 09:47:41 -0700 Subject: [PATCH 25/30] feat: LP updated event Co-Authored-By: Claude --- src/openedx_content/applets/publishing/api.py | 15 ++++++ .../applets/publishing/signals.py | 30 ++++++++++++ .../applets/publishing/test_signals.py | 48 +++++++++++++++++++ 3 files changed, 93 insertions(+) diff --git a/src/openedx_content/applets/publishing/api.py b/src/openedx_content/applets/publishing/api.py index 674e52cf8..a4d926fef 100644 --- a/src/openedx_content/applets/publishing/api.py +++ b/src/openedx_content/applets/publishing/api.py @@ -157,6 +157,21 @@ def update_learning_package( lp.updated = updated lp.save() + + # Emit LEARNING_PACKAGE_UPDATED once the transaction commits. Note: we only + # reach this point if at least one of key/title/description/updated was + # passed in (the early-return above handles the no-op case), so the update + # really did touch the row. + lp_id = lp.id + lp_title = lp.title + + def send_event(): + signals.LEARNING_PACKAGE_UPDATED.send_event( + learning_package=signals.LearningPackageEventData(id=lp_id, title=lp_title), + ) + + on_commit(send_event) + return lp diff --git a/src/openedx_content/applets/publishing/signals.py b/src/openedx_content/applets/publishing/signals.py index 4f716be88..a83534316 100644 --- a/src/openedx_content/applets/publishing/signals.py +++ b/src/openedx_content/applets/publishing/signals.py @@ -18,6 +18,7 @@ "PublishLogEventData", # All events: "LEARNING_PACKAGE_CREATED", + "LEARNING_PACKAGE_UPDATED", "LEARNING_PACKAGE_DELETED", "LEARNING_PACKAGE_ENTITIES_CHANGED", "LEARNING_PACKAGE_ENTITIES_PUBLISHED", @@ -106,6 +107,35 @@ class PublishLogEventData: """ +LEARNING_PACKAGE_UPDATED = OpenEdxPublicSignal( + event_type="org.openedx.content.publishing.lp_updated.v1", + data={ + "learning_package": LearningPackageEventData, + }, +) +""" +A ``LearningPackage``'s own metadata (key, title, and/or description) has been +changed. + +This is emitted only when the ``update_learning_package`` API is called, with at +least one field change that actually modifies the row. + +This event covers changes to the ``LearningPackage`` row itself (its ``key``, +``title``, and ``description``). Changes to the content inside the package +(entities, versions, drafts, publishes) are covered by +``LEARNING_PACKAGE_ENTITIES_CHANGED`` and ``LEARNING_PACKAGE_ENTITIES_PUBLISHED`` +instead. + +The ``learning_package`` payload reflects the ``id`` and the post-update +``title`` of the package. + +💾 This event is only emitted after the enclosing database transaction has +been committed. If the transaction is rolled back, no event is emitted. + +⏳ This event is emitted synchronously. +""" + + LEARNING_PACKAGE_DELETED = OpenEdxPublicSignal( event_type="org.openedx.content.publishing.lp_deleted.v1", data={ diff --git a/tests/openedx_content/applets/publishing/test_signals.py b/tests/openedx_content/applets/publishing/test_signals.py index 42dd18840..1bec4e7e9 100644 --- a/tests/openedx_content/applets/publishing/test_signals.py +++ b/tests/openedx_content/applets/publishing/test_signals.py @@ -73,6 +73,54 @@ def test_learning_package_created_aborted() -> None: api.create_learning_package(key="lp1", title="Test LP 📦") +# LEARNING_PACKAGE_UPDATED + + +def test_learning_package_updated() -> None: + """ + Test that LEARNING_PACKAGE_UPDATED is emitted when + ``update_learning_package`` actually changes a field, and that the payload + reflects the post-update title. + """ + learning_package = api.create_learning_package(key="lp1", title="Original Title") + + with capture_events(signals=[api.signals.LEARNING_PACKAGE_UPDATED], expected_count=1) as captured: + api.update_learning_package(learning_package.id, title="New Title 📦") + + event = captured[0] + assert event.signal is api.signals.LEARNING_PACKAGE_UPDATED + assert event.kwargs["learning_package"].id == learning_package.id + assert event.kwargs["learning_package"].title == "New Title 📦" + + +def test_learning_package_updated_noop() -> None: + """ + Test that LEARNING_PACKAGE_UPDATED is NOT emitted when + ``update_learning_package`` is called with no field changes (the early + return in the API means the row is never saved). + """ + learning_package = api.create_learning_package(key="lp1", title="Test LP 📦") + + with capture_events(signals=[api.signals.LEARNING_PACKAGE_UPDATED], expected_count=0): + api.update_learning_package(learning_package.id) + + +def test_learning_package_updated_aborted() -> None: + """ + Test that LEARNING_PACKAGE_UPDATED is NOT emitted when the transaction + that would have updated the ``LearningPackage`` is rolled back. + """ + learning_package = api.create_learning_package(key="lp1", title="Original Title") + + with capture_events(signals=[api.signals.LEARNING_PACKAGE_UPDATED], expected_count=0): + with abort_transaction(): + api.update_learning_package(learning_package.id, title="Not going to stick") + + # Confirm the title was not actually changed: + learning_package.refresh_from_db() + assert learning_package.title == "Original Title" + + # LEARNING_PACKAGE_DELETED From 3902b85c95e9a53a821b483d027ce03f4b56c158 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 23 Apr 2026 10:11:17 -0700 Subject: [PATCH 26/30] fix: unsorted event payload causing flaky test --- src/openedx_content/applets/collections/api.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/openedx_content/applets/collections/api.py b/src/openedx_content/applets/collections/api.py index 3f1e3fbd9..286bd8c5b 100644 --- a/src/openedx_content/applets/collections/api.py +++ b/src/openedx_content/applets/collections/api.py @@ -206,7 +206,7 @@ def add_to_collection( collection.entities.add(*ids_to_add, through_defaults={"created_by_id": created_by}) collection.modified = datetime.now(tz=timezone.utc) collection.save() - _queue_change_event(collection, entities_added=list(set(ids_to_add) - existing_ids), user_id=created_by) + _queue_change_event(collection, entities_added=sorted(list(set(ids_to_add) - existing_ids)), user_id=created_by) return collection @@ -228,7 +228,7 @@ def remove_from_collection( collection = get_collection(learning_package_id, collection_code) ids_to_remove = list(entities_qset.values_list("id", flat=True)) - entities_removed = list(collection.entities.filter(id__in=ids_to_remove).values_list("id", flat=True)) + entities_removed = sorted(list(collection.entities.filter(id__in=ids_to_remove).values_list("id", flat=True))) collection.entities.remove(*ids_to_remove) collection.modified = datetime.now(tz=timezone.utc) collection.save() From a53166df0bbe7aacd834fa45d58f60f28fb700ba Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 23 Apr 2026 12:46:57 -0700 Subject: [PATCH 27/30] fix: address some feedback from review (collections) --- .../applets/collections/api.py | 34 +++++++++---------- src/openedx_content/applets/publishing/api.py | 24 ++++++------- .../applets/publishing/signal_handlers.py | 19 +++++------ tests/openedx_content/conftest.py | 2 +- 4 files changed, 37 insertions(+), 42 deletions(-) diff --git a/src/openedx_content/applets/collections/api.py b/src/openedx_content/applets/collections/api.py index 286bd8c5b..2766f216b 100644 --- a/src/openedx_content/applets/collections/api.py +++ b/src/openedx_content/applets/collections/api.py @@ -5,6 +5,7 @@ from __future__ import annotations from datetime import datetime, timezone +from functools import partial from django.core.exceptions import ValidationError from django.db.models import QuerySet @@ -51,24 +52,21 @@ def _queue_change_event( learning_package_title = collection.learning_package.title # Send out an event immediately after this database transaction commits. - def send_change_event(): - signals.LEARNING_PACKAGE_COLLECTION_CHANGED.send_event( - time=collection.modified, - learning_package=signals.LearningPackageEventData(id=learning_package_id, title=learning_package_title), - # FIXME: most collections APIs are missing a user_id parameter. - changed_by=signals.UserAttributionEventData(user_id=user_id), - change=signals.CollectionChangeData( - collection_id=collection.id, - collection_code=collection.collection_code, - created=created, - metadata_modified=metadata_modified, - deleted=deleted, - entities_added=entities_added or [], - entities_removed=entities_removed or [], - ), - ) - - on_commit(send_change_event) + on_commit(partial( + signals.LEARNING_PACKAGE_COLLECTION_CHANGED.send_event, + time=collection.modified, + learning_package=signals.LearningPackageEventData(id=learning_package_id, title=learning_package_title), + changed_by=signals.UserAttributionEventData(user_id=user_id), + change=signals.CollectionChangeData( + collection_id=collection.id, + collection_code=collection.collection_code, + created=created, + metadata_modified=metadata_modified, + deleted=deleted, + entities_added=entities_added or [], + entities_removed=entities_removed or [], + ), + )) def create_collection( diff --git a/src/openedx_content/applets/publishing/api.py b/src/openedx_content/applets/publishing/api.py index 8e733cd9f..a6671a505 100644 --- a/src/openedx_content/applets/publishing/api.py +++ b/src/openedx_content/applets/publishing/api.py @@ -529,7 +529,7 @@ def publish_from_drafts( published_draft_ids.add(draft.pk) _create_side_effects_for_change_log(publish_log) - _emit_event_for_change_log(publish_log, time_stamp=published_at, user_id=published_by) + _emit_event_for_change_log(publish_log, timestamp=published_at, user_id=published_by) return publish_log @@ -716,7 +716,7 @@ def set_draft_version( draft.save() _create_side_effects_for_change_log(change_log) # Send out an event immediately after this database transaction commits, since this is an isolated change. - _emit_event_for_change_log(change_log, time_stamp=set_at, user_id=set_by) + _emit_event_for_change_log(change_log, timestamp=set_at, user_id=set_by) def _add_to_existing_draft_change_log( @@ -953,7 +953,7 @@ def _create_side_effects_for_change_log(change_log: DraftChangeLog | PublishLog) def _emit_event_for_change_log( - change_log: PublishLog | DraftChangeLog, time_stamp: datetime, user_id: int | None + change_log: PublishLog | DraftChangeLog, timestamp: datetime, user_id: int | None ) -> None: """ Construct and emit the _CHANGED / _PUBLISHED event when a set of entities is @@ -986,15 +986,13 @@ def _emit_event_for_change_log( change_log_data = signals.PublishLogEventData(publish_log_id=change_log.id, changes=changes) # Send out an event immediately after this database transaction commits. - def send_change_event(): - signal.send_event( - time=time_stamp, - learning_package=signals.LearningPackageEventData(id=learning_package_id, title=learning_package_title), - changed_by=signals.UserAttributionEventData(user_id=user_id), - change_log=change_log_data, - ) - - on_commit(send_change_event) + on_commit(partial( + signal.send_event, + time=timestamp, + learning_package=signals.LearningPackageEventData(id=learning_package_id, title=learning_package_title), + changed_by=signals.UserAttributionEventData(user_id=user_id), + change_log=change_log_data, + )) def update_dependencies_hash_digests_for_log( @@ -1457,6 +1455,6 @@ def bulk_draft_changes_for( changed_by=changed_by, exit_callbacks=[ _create_side_effects_for_change_log, - partial(_emit_event_for_change_log, time_stamp=changed_at, user_id=changed_by), + partial(_emit_event_for_change_log, timestamp=changed_at, user_id=changed_by), ] ) diff --git a/src/openedx_content/applets/publishing/signal_handlers.py b/src/openedx_content/applets/publishing/signal_handlers.py index 6e5d7f63b..9fe088f2b 100644 --- a/src/openedx_content/applets/publishing/signal_handlers.py +++ b/src/openedx_content/applets/publishing/signal_handlers.py @@ -2,6 +2,8 @@ Django signal handlers for the publishing applet. """ +from functools import partial + from django.db import transaction from django.db.models.signals import post_delete from django.dispatch import receiver @@ -11,14 +13,14 @@ @receiver(post_delete, sender=LearningPackage) -def _emit_learning_package_deleted(sender, instance, **kwargs): # pylint: disable=unused-argument +def emit_learning_package_deleted(sender, instance, **kwargs): # pylint: disable=unused-argument """ Emit ``LEARNING_PACKAGE_DELETED`` after a ``LearningPackage`` is deleted. This fires for any deletion: single-object ``.delete()``, bulk ``QuerySet.delete()`` (Django calls ``post_delete`` once per row), or deletions performed via the Django admin. There is currently no official API - for deleting Learning Packages, but you can orhpan them by deleting any + for deleting Learning Packages, but you can orphan them by deleting any references to them such as ``ContentLibrary`` instances in openedx-platform. The event is deferred via ``transaction.on_commit`` so that it is only @@ -31,12 +33,9 @@ def _emit_learning_package_deleted(sender, instance, **kwargs): # pylint: disab ``title`` at handler-invocation time so that the event payload remains correct even though the underlying record is no longer retrievable. """ - lp_id = instance.id - lp_title = instance.title - - def send_event(): - LEARNING_PACKAGE_DELETED.send_event( - learning_package=LearningPackageEventData(id=lp_id, title=lp_title), + transaction.on_commit( + partial( + LEARNING_PACKAGE_DELETED.send_event, + learning_package=LearningPackageEventData(id=instance.id, title=instance.title), ) - - transaction.on_commit(send_event) + ) diff --git a/tests/openedx_content/conftest.py b/tests/openedx_content/conftest.py index f0996b32c..e7289f592 100644 --- a/tests/openedx_content/conftest.py +++ b/tests/openedx_content/conftest.py @@ -1,4 +1,4 @@ -"""Shared fixtures for collection tests.""" +"""Shared fixtures for openedx_content tests.""" import pytest from celery import current_app # type: ignore[import] From e42144bd7aa13b18bf9801456c6200a272cf2289 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 23 Apr 2026 16:26:14 -0700 Subject: [PATCH 28/30] refactor: api_signals.py -> signals.py --- src/openedx_content/api.py | 8 ++++++-- src/openedx_content/api_signals.py | 8 -------- src/openedx_content/signals.py | 20 ++++++++++++++++++++ 3 files changed, 26 insertions(+), 10 deletions(-) delete mode 100644 src/openedx_content/api_signals.py create mode 100644 src/openedx_content/signals.py diff --git a/src/openedx_content/api.py b/src/openedx_content/api.py index 34ad846dc..a0d7d0ab3 100644 --- a/src/openedx_content/api.py +++ b/src/openedx_content/api.py @@ -11,8 +11,12 @@ # These wildcard imports are okay because these api modules declare __all__. # pylint: disable=wildcard-import,unused-import -# Signals are kept in a separate namespace: -from . import api_signals as signals +# Signals are kept in a separate namespace, for two reasons: +# (1) so they can easily be imported/used as `api.signals` (e.g. `from openedx_content import api`, use `api.signals.x`) +# (2) to avoid confusion between event data structures and other API symbols with similar names (e.g. +# `DraftChangeLogEventData` vs `DraftChangeLogRecord` is clearer if the former is `signals.DraftChangeLogEventData`) +from . import signals +# The rest of the public API (other than models): from .applets.backup_restore.api import * from .applets.collections.api import * from .applets.components.api import * diff --git a/src/openedx_content/api_signals.py b/src/openedx_content/api_signals.py deleted file mode 100644 index 258305db3..000000000 --- a/src/openedx_content/api_signals.py +++ /dev/null @@ -1,8 +0,0 @@ -""" -Signals that are part of the public API of openedx_content -""" - -# These wildcard imports are okay because these api modules declare __all__. -# pylint: disable=wildcard-import -from .applets.collections.signals import * -from .applets.publishing.signals import * diff --git a/src/openedx_content/signals.py b/src/openedx_content/signals.py new file mode 100644 index 000000000..848ae550e --- /dev/null +++ b/src/openedx_content/signals.py @@ -0,0 +1,20 @@ +""" +Signals that are part of the public API of openedx_content. + +Import these as e.g. `from openedx_content.api import signals` or as +`from openedx_content import api as content_api` -> `content_api.signals._____` + +These signals may be moved into openedx_events at some point. +""" + +# This intermediate file is necessary so we can (1) filter the applet .signals +# module exports using `__all__` (we don't want to import models like +# `LearningPackage` that happen to be used in our `signals.py` files), and (2) +# so we can still namespace these under `api.signals.____` (see `api.py` for +# details on why). + +# These wildcard imports are okay because these api modules declare __all__ +# to define which symbols are public. +# pylint: disable=wildcard-import +from .applets.collections.signals import * +from .applets.publishing.signals import * From 26683ff3286fd28a52be0592d55d4ba962a68ce6 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 23 Apr 2026 16:54:38 -0700 Subject: [PATCH 29/30] refactor: rename the new signals for more clarity --- .../applets/collections/api.py | 2 +- .../applets/collections/signal_handlers.py | 6 +- .../applets/collections/signals.py | 12 +-- .../applets/collections/tasks.py | 33 +++---- src/openedx_content/applets/publishing/api.py | 4 +- .../applets/publishing/signals.py | 15 ++-- .../applets/collections/test_signals.py | 90 +++++++++---------- .../applets/publishing/test_signals.py | 34 ++++--- tests/utils.py | 2 +- 9 files changed, 94 insertions(+), 104 deletions(-) diff --git a/src/openedx_content/applets/collections/api.py b/src/openedx_content/applets/collections/api.py index 2766f216b..d85b76e1e 100644 --- a/src/openedx_content/applets/collections/api.py +++ b/src/openedx_content/applets/collections/api.py @@ -53,7 +53,7 @@ def _queue_change_event( # Send out an event immediately after this database transaction commits. on_commit(partial( - signals.LEARNING_PACKAGE_COLLECTION_CHANGED.send_event, + signals.COLLECTION_CHANGED.send_event, time=collection.modified, learning_package=signals.LearningPackageEventData(id=learning_package_id, title=learning_package_title), changed_by=signals.UserAttributionEventData(user_id=user_id), diff --git a/src/openedx_content/applets/collections/signal_handlers.py b/src/openedx_content/applets/collections/signal_handlers.py index d47c8176f..951840d4c 100644 --- a/src/openedx_content/applets/collections/signal_handlers.py +++ b/src/openedx_content/applets/collections/signal_handlers.py @@ -5,11 +5,11 @@ from django.db import transaction from django.dispatch import receiver -from ..publishing.signals import LEARNING_PACKAGE_ENTITIES_CHANGED, DraftChangeLogEventData, UserAttributionEventData +from ..publishing.signals import ENTITIES_DRAFT_CHANGED, DraftChangeLogEventData, UserAttributionEventData from .tasks import emit_collections_changed_for_entity_changes_task -@receiver(LEARNING_PACKAGE_ENTITIES_CHANGED) +@receiver(ENTITIES_DRAFT_CHANGED) def on_entities_changed( change_log: DraftChangeLogEventData, changed_by: UserAttributionEventData, @@ -18,7 +18,7 @@ def on_entities_changed( """ When entity drafts are deleted or restored, notify affected collections. - Dispatches a task to emit LEARNING_PACKAGE_COLLECTION_CHANGED for any + 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] diff --git a/src/openedx_content/applets/collections/signals.py b/src/openedx_content/applets/collections/signals.py index 1ea4148c3..701910239 100644 --- a/src/openedx_content/applets/collections/signals.py +++ b/src/openedx_content/applets/collections/signals.py @@ -13,7 +13,7 @@ # All event data structures should end with "...Data": "CollectionChangeData", # All events: - "LEARNING_PACKAGE_COLLECTION_CHANGED", + "COLLECTION_CHANGED", ] @@ -36,8 +36,8 @@ class CollectionChangeData: entities_removed: list[PublishableEntity.ID] = field(factory=list) -LEARNING_PACKAGE_COLLECTION_CHANGED = OpenEdxPublicSignal( - event_type="org.openedx.content.collections.lp_collection_changed.v1", +COLLECTION_CHANGED = OpenEdxPublicSignal( + event_type="org.openedx.content.collections.collection_changed.v1", data={ "learning_package": LearningPackageEventData, "changed_by": UserAttributionEventData, @@ -62,9 +62,9 @@ class CollectionChangeData: # Note: at present, the openedx_tagging code (in this repo) emits a # CONTENT_OBJECT_ASSOCIATIONS_CHANGED event whenever an entity's tags change. # But we do NOT emit the same event when an entity's collections change; rather -# we expect code in the platform to listen for -# LEARNING_PACKAGE_COLLECTION_CHANGED and then re-emit '...ASSOCIATIONS_CHANGED' -# as needed. The reason we don't emit the '...ASSOCIATIONS_CHANGED' event here +# we expect code in the platform to listen for COLLECTION_CHANGED and then +# re-emit '...ASSOCIATIONS_CHANGED' as needed. +# The reason we don't emit the '...ASSOCIATIONS_CHANGED' event here # is simple: we know the entity IDs but not their opaque keys, and all of the # code that listens for that event expects the entity's opaque keys. # The tagging code can do it here because the `object_id` in the tagging models diff --git a/src/openedx_content/applets/collections/tasks.py b/src/openedx_content/applets/collections/tasks.py index 7993de43b..6b5a43d9e 100644 --- a/src/openedx_content/applets/collections/tasks.py +++ b/src/openedx_content/applets/collections/tasks.py @@ -7,12 +7,7 @@ from ..publishing.models import PublishableEntity from .models import Collection, CollectionPublishableEntity -from .signals import ( - LEARNING_PACKAGE_COLLECTION_CHANGED, - CollectionChangeData, - LearningPackageEventData, - UserAttributionEventData, -) +from .signals import COLLECTION_CHANGED, CollectionChangeData, LearningPackageEventData, UserAttributionEventData logger = logging.getLogger(__name__) @@ -24,24 +19,23 @@ def emit_collections_changed_for_entity_changes_task( user_id: int | None, ) -> int: """ - Emit LEARNING_PACKAGE_COLLECTION_CHANGED for each collection affected by - entity draft deletions or restorations. + 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. + 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 LEARNING_PACKAGE_ENTITIES_CHANGED. New entities - (old_version_id=None, new_version_id is not None) that aren't in any - collection result in a no-op. + 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) + CollectionPublishableEntity.objects.filter(entity_id__in=all_entity_ids) .select_related("collection__learning_package") .order_by("collection_id", "entity_id") ) @@ -61,9 +55,9 @@ def emit_collections_changed_for_entity_changes_task( emitted_events = 0 for collection_id, collection in collection_map.items(): - # .. event_implemented_name: LEARNING_PACKAGE_COLLECTION_CHANGED - # .. event_type: org.openedx.content.collections.lp_collection_changed.v1 - LEARNING_PACKAGE_COLLECTION_CHANGED.send_event( + # .. 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, @@ -81,8 +75,7 @@ def emit_collections_changed_for_entity_changes_task( if emitted_events: logger.info( - "Entity draft changes (removed=%s, added=%s): emitted LEARNING_PACKAGE_COLLECTION_CHANGED" - " for %s collections.", + "Entity draft changes (removed=%s, added=%s): emitted COLLECTION_CHANGED for %s collections.", removed_entity_ids, added_entity_ids, emitted_events, diff --git a/src/openedx_content/applets/publishing/api.py b/src/openedx_content/applets/publishing/api.py index a6671a505..bd2507407 100644 --- a/src/openedx_content/applets/publishing/api.py +++ b/src/openedx_content/applets/publishing/api.py @@ -978,11 +978,11 @@ def _emit_event_for_change_log( change_log_data: signals.DraftChangeLogEventData | signals.PublishLogEventData if isinstance(change_log, DraftChangeLog): - signal = signals.LEARNING_PACKAGE_ENTITIES_CHANGED + signal = signals.ENTITIES_DRAFT_CHANGED change_log_data = signals.DraftChangeLogEventData(draft_change_log_id=change_log.id, changes=changes) else: assert isinstance(change_log, PublishLog) - signal = signals.LEARNING_PACKAGE_ENTITIES_PUBLISHED + signal = signals.ENTITIES_PUBLISHED change_log_data = signals.PublishLogEventData(publish_log_id=change_log.id, changes=changes) # Send out an event immediately after this database transaction commits. diff --git a/src/openedx_content/applets/publishing/signals.py b/src/openedx_content/applets/publishing/signals.py index a83534316..148c8c17d 100644 --- a/src/openedx_content/applets/publishing/signals.py +++ b/src/openedx_content/applets/publishing/signals.py @@ -20,8 +20,8 @@ "LEARNING_PACKAGE_CREATED", "LEARNING_PACKAGE_UPDATED", "LEARNING_PACKAGE_DELETED", - "LEARNING_PACKAGE_ENTITIES_CHANGED", - "LEARNING_PACKAGE_ENTITIES_PUBLISHED", + "ENTITIES_DRAFT_CHANGED", + "ENTITIES_PUBLISHED", ] @@ -123,8 +123,7 @@ class PublishLogEventData: This event covers changes to the ``LearningPackage`` row itself (its ``key``, ``title``, and ``description``). Changes to the content inside the package (entities, versions, drafts, publishes) are covered by -``LEARNING_PACKAGE_ENTITIES_CHANGED`` and ``LEARNING_PACKAGE_ENTITIES_PUBLISHED`` -instead. +``ENTITIES_DRAFT_CHANGED`` and ``ENTITIES_PUBLISHED`` instead. The ``learning_package`` payload reflects the ``id`` and the post-update ``title`` of the package. @@ -167,8 +166,8 @@ class PublishLogEventData: """ -LEARNING_PACKAGE_ENTITIES_CHANGED = OpenEdxPublicSignal( - event_type="org.openedx.content.publishing.lp_entities_changed.v1", +ENTITIES_DRAFT_CHANGED = OpenEdxPublicSignal( + event_type="org.openedx.content.publishing.entities_draft_changed.v1", data={ "learning_package": LearningPackageEventData, "changed_by": UserAttributionEventData, @@ -202,8 +201,8 @@ class PublishLogEventData: """ -LEARNING_PACKAGE_ENTITIES_PUBLISHED = OpenEdxPublicSignal( - event_type="org.openedx.content.publishing.lp_entities_published.v1", +ENTITIES_PUBLISHED = OpenEdxPublicSignal( + event_type="org.openedx.content.publishing.entities_published.v1", data={ "learning_package": LearningPackageEventData, "changed_by": UserAttributionEventData, diff --git a/tests/openedx_content/applets/collections/test_signals.py b/tests/openedx_content/applets/collections/test_signals.py index a1f09dc59..836c51435 100644 --- a/tests/openedx_content/applets/collections/test_signals.py +++ b/tests/openedx_content/applets/collections/test_signals.py @@ -1,5 +1,5 @@ """ -Tests for the LEARNING_PACKAGE_COLLECTION_CHANGED signal. +Tests for the COLLECTION_CHANGED signal. """ from datetime import datetime, timezone @@ -7,7 +7,7 @@ import pytest from openedx_content import api -from openedx_content.applets.collections.signals import LEARNING_PACKAGE_COLLECTION_CHANGED, CollectionChangeData +from openedx_content.applets.collections.signals import COLLECTION_CHANGED, CollectionChangeData from openedx_content.models_api import Collection, LearningPackage, PublishableEntity from tests.utils import abort_transaction, capture_events @@ -26,12 +26,12 @@ def _create_entity(learning_package_id: LearningPackage.ID, entity_ref: str) -> return api.create_publishable_entity(learning_package_id, entity_ref=entity_ref, created=now_time, created_by=None) -# LEARNING_PACKAGE_COLLECTION_CHANGED — create_collection +# COLLECTION_CHANGED — create_collection def test_create_collection(lp1: LearningPackage, admin_user) -> None: """ - Test that LEARNING_PACKAGE_COLLECTION_CHANGED is emitted with created=True + Test that COLLECTION_CHANGED is emitted with created=True when a new collection is created. """ with capture_events(expected_count=1) as captured: @@ -43,7 +43,7 @@ def test_create_collection(lp1: LearningPackage, admin_user) -> None: ) event = captured[0] - assert event.signal is LEARNING_PACKAGE_COLLECTION_CHANGED + assert event.signal is COLLECTION_CHANGED assert event.kwargs["learning_package"].id == lp1.id assert event.kwargs["learning_package"].title == "Test LP 📦" assert event.kwargs["changed_by"].user_id == admin_user.id @@ -88,12 +88,12 @@ def test_create_collection_aborted(lp1: LearningPackage) -> None: ) -# LEARNING_PACKAGE_COLLECTION_CHANGED — update_collection +# COLLECTION_CHANGED — update_collection def test_update_collection(lp1: LearningPackage) -> None: """ - Test that LEARNING_PACKAGE_COLLECTION_CHANGED is emitted with metadata_modified=True + Test that COLLECTION_CHANGED is emitted with metadata_modified=True when a collection's title or description is updated. """ collection = api.create_collection(lp1.id, "col1", title="Collection 1", created_by=None) @@ -103,7 +103,7 @@ def test_update_collection(lp1: LearningPackage) -> None: api.update_collection(lp1.id, "col1", title="Updated Title") event = captured[0] - assert event.signal is LEARNING_PACKAGE_COLLECTION_CHANGED + assert event.signal is COLLECTION_CHANGED assert event.kwargs["learning_package"].id == lp1.id assert event.kwargs["change"] == CollectionChangeData( collection_id=collection.id, @@ -117,7 +117,7 @@ def test_update_collection(lp1: LearningPackage) -> None: def test_update_collection_no_op(lp1: LearningPackage) -> None: """ - Test that LEARNING_PACKAGE_COLLECTION_CHANGED is NOT emitted when + Test that COLLECTION_CHANGED is NOT emitted when update_collection is called without any fields to update. """ api.create_collection(lp1.id, "col1", title="Collection 1", created_by=None) @@ -127,12 +127,12 @@ def test_update_collection_no_op(lp1: LearningPackage) -> None: api.update_collection(lp1.id, "col1") -# LEARNING_PACKAGE_COLLECTION_CHANGED — delete_collection +# COLLECTION_CHANGED — delete_collection def test_delete_collection_soft(lp1: LearningPackage) -> None: """ - Test that LEARNING_PACKAGE_COLLECTION_CHANGED is emitted with deleted=True + Test that COLLECTION_CHANGED is emitted with deleted=True when a collection is soft-deleted (enabled=False). """ collection = api.create_collection(lp1.id, "col1", title="Collection 1", created_by=None) @@ -148,7 +148,7 @@ def test_delete_collection_soft(lp1: LearningPackage) -> None: api.delete_collection(lp1.id, "col1") event = captured[0] - assert event.signal is LEARNING_PACKAGE_COLLECTION_CHANGED + assert event.signal is COLLECTION_CHANGED assert event.kwargs["learning_package"].id == lp1.id assert event.kwargs["change"] == CollectionChangeData( collection_id=collection.id, @@ -160,7 +160,7 @@ def test_delete_collection_soft(lp1: LearningPackage) -> None: def test_delete_collection_hard(lp1: LearningPackage) -> None: """ - Test that LEARNING_PACKAGE_COLLECTION_CHANGED is emitted with deleted=True and + Test that COLLECTION_CHANGED is emitted with deleted=True and entities_removed populated when a collection is hard-deleted. """ collection = api.create_collection(lp1.id, "col1", title="Collection 1", created_by=None) @@ -178,7 +178,7 @@ def test_delete_collection_hard(lp1: LearningPackage) -> None: api.delete_collection(lp1.id, "col1", hard_delete=True) event = captured[0] - assert event.signal is LEARNING_PACKAGE_COLLECTION_CHANGED + assert event.signal is COLLECTION_CHANGED assert event.kwargs["learning_package"].id == lp1.id assert event.kwargs["change"] == CollectionChangeData( collection_id=collection_id, @@ -188,12 +188,12 @@ def test_delete_collection_hard(lp1: LearningPackage) -> None: ) -# LEARNING_PACKAGE_COLLECTION_CHANGED — restore_collection +# COLLECTION_CHANGED — restore_collection def test_restore_collection(lp1: LearningPackage) -> None: """ - Test that LEARNING_PACKAGE_COLLECTION_CHANGED is emitted with created=True + Test that COLLECTION_CHANGED is emitted with created=True when a soft-deleted collection is restored. """ collection = api.create_collection(lp1.id, "col1", title="Collection 1", created_by=None) @@ -203,7 +203,7 @@ def test_restore_collection(lp1: LearningPackage) -> None: api.restore_collection(lp1.id, "col1") event = captured[0] - assert event.signal is LEARNING_PACKAGE_COLLECTION_CHANGED + assert event.signal is COLLECTION_CHANGED assert event.kwargs["learning_package"].id == lp1.id assert event.kwargs["change"] == CollectionChangeData( collection_id=collection.id, @@ -212,12 +212,12 @@ def test_restore_collection(lp1: LearningPackage) -> None: ) -# LEARNING_PACKAGE_COLLECTION_CHANGED — add_to_collection +# COLLECTION_CHANGED — add_to_collection def test_add_to_collection(lp1: LearningPackage) -> None: """ - Test that LEARNING_PACKAGE_COLLECTION_CHANGED is emitted with the correct + Test that COLLECTION_CHANGED is emitted with the correct entities_added list when entities are added to a collection. """ collection = api.create_collection(lp1.id, "col1", title="Collection 1", created_by=None) @@ -232,7 +232,7 @@ def test_add_to_collection(lp1: LearningPackage) -> None: ) event = captured[0] - assert event.signal is LEARNING_PACKAGE_COLLECTION_CHANGED + assert event.signal is COLLECTION_CHANGED assert event.kwargs["learning_package"].id == lp1.id assert event.kwargs["change"] == CollectionChangeData( collection_id=collection.id, @@ -257,12 +257,12 @@ def test_add_to_collection_aborted(lp1: LearningPackage) -> None: ) -# LEARNING_PACKAGE_COLLECTION_CHANGED — remove_from_collection +# COLLECTION_CHANGED — remove_from_collection def test_remove_from_collection(lp1: LearningPackage) -> None: """ - Test that LEARNING_PACKAGE_COLLECTION_CHANGED is emitted with the correct + Test that COLLECTION_CHANGED is emitted with the correct entities_removed list when entities are removed from a collection. """ collection = api.create_collection(lp1.id, "col1", title="Collection 1", created_by=None) @@ -282,7 +282,7 @@ def test_remove_from_collection(lp1: LearningPackage) -> None: ) event = captured[0] - assert event.signal is LEARNING_PACKAGE_COLLECTION_CHANGED + assert event.signal is COLLECTION_CHANGED assert event.kwargs["learning_package"].id == lp1.id assert event.kwargs["change"] == CollectionChangeData( collection_id=collection.id, @@ -291,12 +291,12 @@ def test_remove_from_collection(lp1: LearningPackage) -> None: ) -# LEARNING_PACKAGE_COLLECTION_CHANGED — set_collections +# COLLECTION_CHANGED — set_collections def test_set_collections(lp1: LearningPackage, admin_user) -> None: """ - Test that LEARNING_PACKAGE_COLLECTION_CHANGED is emitted once per affected + Test that COLLECTION_CHANGED is emitted once per affected collection when set_collections reassigns an entity's collections. In this scenario entity starts in col1+col2, then is moved to col2+col3. @@ -351,7 +351,7 @@ def test_set_collections_aborted(lp1: LearningPackage) -> None: api.set_collections(entity, Collection.objects.filter(id=col1.id)) -# LEARNING_PACKAGE_COLLECTION_CHANGED — on entity draft deletion +# COLLECTION_CHANGED — on entity draft deletion def _create_entity_with_version(learning_package_id: LearningPackage.ID, entity_ref: str) -> PublishableEntity: @@ -365,18 +365,18 @@ def _create_entity_with_version(learning_package_id: LearningPackage.ID, entity_ def test_entity_draft_deleted_in_collection(lp1: LearningPackage, admin_user) -> None: """ - Test that LEARNING_PACKAGE_COLLECTION_CHANGED is emitted with entities_removed + 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=[LEARNING_PACKAGE_COLLECTION_CHANGED], expected_count=1) as captured: + 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 LEARNING_PACKAGE_COLLECTION_CHANGED + 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( @@ -388,7 +388,7 @@ def test_entity_draft_deleted_in_collection(lp1: LearningPackage, admin_user) -> def test_entity_draft_deleted_multiple_collections(lp1: LearningPackage) -> None: """ - Test that LEARNING_PACKAGE_COLLECTION_CHANGED is emitted once per collection + 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) @@ -397,7 +397,7 @@ def test_entity_draft_deleted_multiple_collections(lp1: LearningPackage) -> None 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=[LEARNING_PACKAGE_COLLECTION_CHANGED], expected_count=2) as captured: + 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} @@ -416,35 +416,35 @@ def test_entity_draft_deleted_multiple_collections(lp1: LearningPackage) -> None def test_entity_draft_deleted_not_in_collection(lp1: LearningPackage) -> None: """ - Test that no LEARNING_PACKAGE_COLLECTION_CHANGED is emitted when the deleted + 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=[LEARNING_PACKAGE_COLLECTION_CHANGED], expected_count=0): + with capture_events(signals=[COLLECTION_CHANGED], expected_count=0): api.soft_delete_draft(entity.id) def test_entity_draft_deleted_aborted(lp1: LearningPackage) -> None: """ - Test that no LEARNING_PACKAGE_COLLECTION_CHANGED is emitted when the + 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=[LEARNING_PACKAGE_COLLECTION_CHANGED], expected_count=0): + with capture_events(signals=[COLLECTION_CHANGED], expected_count=0): with abort_transaction(): api.soft_delete_draft(entity.id) -# LEARNING_PACKAGE_COLLECTION_CHANGED — on entity draft restore (deletion reverted) +# COLLECTION_CHANGED — on entity draft restore (deletion reverted) def test_entity_draft_restored_in_collection(lp1: LearningPackage) -> None: """ - Test that LEARNING_PACKAGE_COLLECTION_CHANGED is emitted with entities_added + 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) @@ -452,13 +452,13 @@ def test_entity_draft_restored_in_collection(lp1: LearningPackage) -> None: api.add_to_collection(lp1.id, "col1", PublishableEntity.objects.filter(id=entity.id)) api.soft_delete_draft(entity.id) - with capture_events(signals=[LEARNING_PACKAGE_COLLECTION_CHANGED], expected_count=1) as captured: + 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 LEARNING_PACKAGE_COLLECTION_CHANGED + assert event.signal is COLLECTION_CHANGED assert event.kwargs["learning_package"].id == lp1.id assert event.kwargs["change"] == CollectionChangeData( collection_id=collection.id, @@ -469,7 +469,7 @@ def test_entity_draft_restored_in_collection(lp1: LearningPackage) -> None: def test_entity_draft_restored_multiple_collections(lp1: LearningPackage) -> None: """ - Test that LEARNING_PACKAGE_COLLECTION_CHANGED is emitted once per collection + 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) @@ -482,7 +482,7 @@ def test_entity_draft_restored_multiple_collections(lp1: LearningPackage) -> Non api.soft_delete_draft(entity.id) - with capture_events(signals=[LEARNING_PACKAGE_COLLECTION_CHANGED], expected_count=2) as captured: + 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) @@ -502,7 +502,7 @@ def test_entity_draft_restored_multiple_collections(lp1: LearningPackage) -> Non def test_entity_draft_restored_aborted(lp1: LearningPackage) -> None: """ - Test that no LEARNING_PACKAGE_COLLECTION_CHANGED is emitted when the + 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) @@ -510,7 +510,7 @@ def test_entity_draft_restored_aborted(lp1: LearningPackage) -> None: api.add_to_collection(lp1.id, "col1", PublishableEntity.objects.filter(id=entity.id)) api.soft_delete_draft(entity.id) - with capture_events(signals=[LEARNING_PACKAGE_COLLECTION_CHANGED], expected_count=0): + 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 @@ -519,10 +519,10 @@ def test_entity_draft_restored_aborted(lp1: LearningPackage) -> None: def test_entity_created_no_collection_event(lp1: LearningPackage) -> None: """ - Test that no LEARNING_PACKAGE_COLLECTION_CHANGED is emitted when a brand-new + 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=[LEARNING_PACKAGE_COLLECTION_CHANGED], expected_count=0): + with capture_events(signals=[COLLECTION_CHANGED], expected_count=0): _create_entity_with_version(lp1.id, "entity1") diff --git a/tests/openedx_content/applets/publishing/test_signals.py b/tests/openedx_content/applets/publishing/test_signals.py index 8895304f1..405f933ab 100644 --- a/tests/openedx_content/applets/publishing/test_signals.py +++ b/tests/openedx_content/applets/publishing/test_signals.py @@ -175,13 +175,12 @@ def test_learning_package_deleted_aborted() -> None: assert LearningPackage.objects.filter(id=lp_id).exists() -# LEARNING_PACKAGE_ENTITIES_CHANGED +# ENTITIES_DRAFT_CHANGED def test_single_entity_changed() -> None: """ - Test that LEARNING_PACKAGE_ENTITIES_CHANGED is emitted when we change a - publishable entity. + Test that ENTITIES_DRAFT_CHANGED is emitted when we change a publishable entity. """ learning_package = api.create_learning_package(package_ref="lp1", title="Test LP 📦") @@ -205,7 +204,7 @@ def test_single_entity_changed() -> None: expected_draft_change_log_id = v1.draftchangelogrecord_set.get().draft_change_log_id event = captured[0] # capture_events(...) context manager already asserted there's only one event. - assert event.signal is api.signals.LEARNING_PACKAGE_ENTITIES_CHANGED + assert event.signal is api.signals.ENTITIES_DRAFT_CHANGED assert event.kwargs["learning_package"].id == learning_package.id assert event.kwargs["learning_package"].title == "Test LP 📦" assert event.kwargs["changed_by"].user_id is None @@ -234,8 +233,7 @@ def test_single_entity_changed_abort() -> None: def test_multiple_entites_changed(admin_user) -> None: """ - Test that LEARNING_PACKAGE_ENTITIES_CHANGED is emitted when we change - several publishable entities in a single edit. + Test that ENTITIES_DRAFT_CHANGED is emitted when we change several publishable entities in a single edit. """ learning_package = api.create_learning_package(package_ref="lp1", title="Test LP 📦") created_args = {"created": now_time, "created_by": admin_user.id} @@ -265,7 +263,7 @@ def test_multiple_entites_changed(admin_user) -> None: api.set_draft_version(entity3.id, None, set_at=now_time, set_by=admin_user.id) event = captured[0] - assert event.signal is api.signals.LEARNING_PACKAGE_ENTITIES_CHANGED + assert event.signal is api.signals.ENTITIES_DRAFT_CHANGED assert event.kwargs["learning_package"].id == learning_package.id assert event.kwargs["learning_package"].title == "Test LP 📦" assert event.kwargs["changed_by"].user_id is admin_user.id @@ -283,7 +281,7 @@ def test_multiple_entites_changed(admin_user) -> None: def test_multiple_entites_change_aborted() -> None: """ - Test that LEARNING_PACKAGE_ENTITIES_CHANGED is NOT emitted when we roll back + Test that ENTITIES_DRAFT_CHANGED is NOT emitted when we roll back a transaction that would have modified multiple entities in a bulk change. """ learning_package = api.create_learning_package(package_ref="lp1", title="Test LP 📦") @@ -313,7 +311,7 @@ def test_multiple_entites_change_aborted() -> None: def test_changes_with_side_effects() -> None: """ - Test that the LEARNING_PACKAGE_ENTITIES_CHANGED event handles dependencies + Test that the ENTITIES_DRAFT_CHANGED event handles dependencies and side effects. """ learning_package = api.create_learning_package(package_ref="lp1", title="Test LP 📦") @@ -336,19 +334,19 @@ def create_entity(name: str, dependencies: list[PublishableEntity.ID] | None = N api.create_publishable_entity_version(child1.id, version_num=2, title="child1 V2", **created_args) event = captured[0] - assert event.signal is api.signals.LEARNING_PACKAGE_ENTITIES_CHANGED + assert event.signal is api.signals.ENTITIES_DRAFT_CHANGED assert event.kwargs["change_log"].changes == [ change_record(child1, old_version=1, new_version=2), # directly modified change_record(parent1, old_version=1, new_version=1), # side effect ] -# LEARNING_PACKAGE_ENTITIES_PUBLISHED +# ENTITIES_PUBLISHED def test_publish_events(admin_user) -> None: """ - Test that LEARNING_PACKAGE_ENTITIES_PUBLISHED is emitted when we publish + Test that ENTITIES_PUBLISHED is emitted when we publish changes to entities in a learning package. """ learning_package = api.create_learning_package(package_ref="lp1", title="Test LP 📦") @@ -372,7 +370,7 @@ def test_publish_events(admin_user) -> None: ) event = captured[0] - assert event.signal is api.signals.LEARNING_PACKAGE_ENTITIES_PUBLISHED + assert event.signal is api.signals.ENTITIES_PUBLISHED assert event.kwargs["learning_package"].id == learning_package.id assert event.kwargs["learning_package"].title == "Test LP 📦" assert event.kwargs["changed_by"].user_id is admin_user.id @@ -402,7 +400,7 @@ def test_publish_events(admin_user) -> None: ) event = captured[0] - assert event.signal is api.signals.LEARNING_PACKAGE_ENTITIES_PUBLISHED + assert event.signal is api.signals.ENTITIES_PUBLISHED assert event.kwargs["learning_package"].id == learning_package.id assert event.kwargs["learning_package"].title == "Test LP 📦" assert event.kwargs["changed_by"].user_id is admin_user.id @@ -420,7 +418,7 @@ def test_publish_events(admin_user) -> None: def test_publish_events_aborted(admin_user) -> None: """ - Test that LEARNING_PACKAGE_ENTITIES_PUBLISHED is NOT emitted when we roll + Test that ENTITIES_PUBLISHED is NOT emitted when we roll back a transaction that would have published some entities. """ learning_package = api.create_learning_package(package_ref="lp1", title="Test LP 📦") @@ -446,7 +444,7 @@ def do_publish(): def test_publish_with_dependencies() -> None: """ - Test that the LEARNING_PACKAGE_ENTITIES_PUBLISHED event handles dependencies + Test that the ENTITIES_PUBLISHED event handles dependencies and side effects. """ learning_package = api.create_learning_package(package_ref="lp1", title="Test LP 📦") @@ -477,7 +475,7 @@ def create_entity(name: str, dependencies: list[PublishableEntity.ID] | None = N publish_entity(grandparent1) event = captured[0] - assert event.signal is api.signals.LEARNING_PACKAGE_ENTITIES_PUBLISHED + assert event.signal is api.signals.ENTITIES_PUBLISHED assert event.kwargs["change_log"].changes == [ change_record(grandparent1, old_version=None, new_version=1, direct=True), change_record(parent1, old_version=None, new_version=1, direct=False), @@ -496,7 +494,7 @@ def create_entity(name: str, dependencies: list[PublishableEntity.ID] | None = N publish_entity(child3) event = captured[0] - assert event.signal is api.signals.LEARNING_PACKAGE_ENTITIES_PUBLISHED + assert event.signal is api.signals.ENTITIES_PUBLISHED assert event.kwargs["change_log"].changes == [ change_record(child3, old_version=1, new_version=2, direct=True), change_record(parent2, old_version=1, new_version=1, direct=False), diff --git a/tests/utils.py b/tests/utils.py index daa82f09d..337806911 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -46,7 +46,7 @@ def capture_events( with capture_events(expected_count=1) as captured: api.do_something(entity.id, ...) - assert captured[0].signal is LEARNING_PACKAGE_ENTITIES_CHANGED + assert captured[0].signal is ENTITIES_DRAFT_CHANGED assert captured[0].kwargs['learning_package'].id == learning_package.id """ if signals is None: From 5ff0496d0e75742e285bc28e3ebe741a2918762f Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 23 Apr 2026 17:06:30 -0700 Subject: [PATCH 30/30] fix: clarify collection event behavior around soft/hard deletion --- .../applets/collections/api.py | 7 ++-- .../applets/collections/test_signals.py | 34 +++++++++++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/src/openedx_content/applets/collections/api.py b/src/openedx_content/applets/collections/api.py index d85b76e1e..40692ca03 100644 --- a/src/openedx_content/applets/collections/api.py +++ b/src/openedx_content/applets/collections/api.py @@ -144,13 +144,16 @@ def delete_collection( """ collection = get_collection(learning_package_id, collection_code) entities_removed = list(collection.entities.order_by("id").values_list("id", flat=True)) + was_already_soft_deleted = not collection.enabled if hard_delete: collection.modified = datetime.now(tz=timezone.utc) # For the event timestamp; won't get saved to the DB - _queue_change_event(collection, deleted=True, entities_removed=entities_removed) + if not was_already_soft_deleted: # Send the deleted event unless this was already soft deleted. + _queue_change_event(collection, deleted=True, entities_removed=entities_removed) # Delete after enqueing the event: collection.delete() - else: + elif not was_already_soft_deleted: + # Soft delete: collection.enabled = False collection.save() _queue_change_event(collection, deleted=True, entities_removed=entities_removed) diff --git a/tests/openedx_content/applets/collections/test_signals.py b/tests/openedx_content/applets/collections/test_signals.py index 836c51435..7c5c5fab4 100644 --- a/tests/openedx_content/applets/collections/test_signals.py +++ b/tests/openedx_content/applets/collections/test_signals.py @@ -73,6 +73,40 @@ def test_create_collection_disabled(lp1: LearningPackage) -> None: enabled=False, ) + # And if that disabled collection is deleted, no event is emitted. We don't want to emit a deleted event for a + # collection that never had a created event. + with capture_events(expected_count=0): + api.delete_collection(lp1.id, collection_code="col1", hard_delete=True) + + +def test_create_collection_disabled_then_enabled(lp1: LearningPackage) -> None: + """ + Test that no event is emitted when a collection is created already soft + deleted (with enabled=False), but IS emitted when we enable/un-delete it. + """ + with capture_events(expected_count=0): + collection = api.create_collection( + lp1.id, + collection_code="col1", + title="Collection 1", + created_by=None, + enabled=False, + ) + + # Enabling (un-deleting) that collection will result in a "created" event: + with capture_events(expected_count=1) as captured: + api.restore_collection(lp1.id, collection_code="col1") # FIXME: we can't specify a user here. + + event = captured[0] + assert event.signal is COLLECTION_CHANGED + assert event.kwargs["learning_package"].id == lp1.id + assert event.kwargs["changed_by"].user_id is None + assert event.kwargs["change"] == CollectionChangeData( + collection_id=collection.id, + collection_code="col1", + created=True, + ) + def test_create_collection_aborted(lp1: LearningPackage) -> None: """