feat: emit change log details, don't emit if txn is rolled back

Author: bradenmacdonald

src/openedx_content/api.py

@@ -9,7 +9,7 @@
 """
 
 # These wildcard imports are okay because these api modules declare __all__.
-# pylint: disable=wildcard-import
+# pylint: disable=wildcard-import,unused-import
 from .applets.backup_restore.api import *
 from .applets.collections.api import *
 from .applets.components.api import *
@@ -19,3 +19,5 @@
 from .applets.sections.api import *
 from .applets.subsections.api import *
 from .applets.units.api import *
+# Signals are kept in a separate namespace to avoid collisions between data structures and models
+from . import api_signals as signals

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 *

src/openedx_content/applets/publishing/api.py

@@ -13,7 +13,7 @@
 
 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
 
@@ -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.ChangeLogRecord(
+            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,

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",
+    "ChangeLogRecord",
+    "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 ChangeLogRecord:
+    """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[ChangeLogRecord]
 
 
 LEARNING_PACKAGE_ENTITIES_CHANGED = OpenEdxPublicSignal(

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])
 
 

tests/openedx_content/applets/publishing/test_signals.py

@@ -4,37 +4,70 @@
 
 from datetime import datetime, timezone
 
+from django.db import transaction
 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
+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.ChangeLogRecord(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

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(

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(

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(

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))