feat: emit version IDs in events as well as version numbers

Author: bradenmacdonald

src/openedx_content/applets/collections/signal_handlers.py

@@ -21,16 +21,16 @@ 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
     # 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 record.old_version_id is None and record.new_version_id is not None
     ]
 
     if not removed_entity_ids and not added_entity_ids:

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:

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

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.
     """
 

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