temp: a lot more iteration on draft side effect modeling

Author: ormsbee

openedx_learning/apps/authoring/publishing/api.py

@@ -23,6 +23,7 @@
     Draft,
     DraftChangeLogRecord,
     DraftChangeLog,
+    DraftSideEffect,
     EntityList,
     EntityListRow,
     LearningPackage,
@@ -474,57 +475,148 @@ def set_draft_version(
         # DraftChangeLog (i.e. what happens if the caller is using the public
         # bulk_draft_changes_for() API call), or if we have to make our own.
         active_change_log = DraftChangeLogContext.get_active_draft_change_log(learning_package_id)
+        change_log = active_change_log or DraftChangeLog.objects.create(
+            learning_package_id=learning_package_id,
+            changed_at=set_at,
+            changed_by_id=set_by,
+        )
 
         if active_change_log:
-            # If we get here, it means there is an active DraftChangeLog that may
-            # have many DraftChanges associated with it. A DraftChangeLog can only
-            # have one DraftChange per PublishableEntity, e.g. the same Component
-            # can't go from v1 to v3 and v1 to v4 in the same DraftChangeLog.
-            try:
-                # If there was already a DraftChange for this PublishableEntity,
-                # we update the new_version_id. We keep the old_version_id value
-                # though, because that represents what it was before this
-                # DraftChangeLog, and we don't want to lose that information.
-                change = DraftChangeLogRecord.objects.get(
-                    draft_change_log=active_change_log,
-                    entity_id=publishable_entity_id,
-                )
-                change.new_version_id = publishable_entity_version_pk
-                change.save()
-            except DraftChangeLogRecord.DoesNotExist:
-                # If we're here, this is the first DraftChange we're making for
-                # this PublishableEntity in the active DraftChangeLog. 
-                change = DraftChangeLogRecord.objects.create(
-                    draft_change_log=active_change_log,
-                    entity_id=publishable_entity_id,
-                    old_version_id=old_version_id,
-                    new_version_id=publishable_entity_version_pk,
-                )
+            change = _add_to_existing_draft_change_log(
+                active_change_log,
+                publishable_entity_id,
+                old_version_id=old_version_id,
+                new_version_id=publishable_entity_version_pk,
+            )
+            # We explicitly *don't* create container side effects here because
+            # there may be many changes in this DraftChangeLog, some of which
+            # haven't been made yet. It wouldn't make sense to create a side
+            # effect that says, "this Unit changed because this Component in it
+            # changed" if we were changing that same Unit later on in the same
+            # DraftChangeLog, because that new Unit version might not even
+            # include the child Component. So we'll do that work when the
+            # DraftChangeLogContext context manager closes.
         else:
             # This means there is no active DraftChangeLog, so we create our own
             # add add our DraftChange to it. This has the minor optimization
             # that we don't have to check for an existing DraftChange, because
             # we're creating the whole DraftChangeLog right here.
-            new_change_log = DraftChangeLog.objects.create(
-                learning_package_id=learning_package_id,
-                changed_at=set_at,
-                changed_by_id=set_by,
-            )
             change = DraftChangeLogRecord.objects.create(
-                draft_change_log=new_change_log,
+                draft_change_log=change_log,
                 entity_id=publishable_entity_id,
                 old_version_id=old_version_id,
                 new_version_id=publishable_entity_version_pk,
-            )
+            )    
+            _create_container_side_effects_for_draft_change(change)
+
+
+def _add_to_existing_draft_change_log(
+    active_change_log: DraftChangeLog,
+    entity_id: int,
+    old_version_id: int | None,
+    new_version_id: int | None,
+) -> DraftChangeLogRecord:
+    # If we get here, it means there is an active DraftChangeLog that may
+    # have many DraftChanges associated with it. A DraftChangeLog can only
+    # have one DraftChange per PublishableEntity, e.g. the same Component
+    # can't go from v1 to v3 and v1 to v4 in the same DraftChangeLog.
+    try:
+        # If there was already a DraftChange for this PublishableEntity,
+        # we update the new_version_id. We keep the old_version_id value
+        # though, because that represents what it was before this
+        # DraftChangeLog, and we don't want to lose that information.
+        change = DraftChangeLogRecord.objects.get(
+            draft_change_log=active_change_log,
+            entity_id=entity_id,
+        )
+        change.new_version_id = new_version_id
+        change.save()
+    except DraftChangeLogRecord.DoesNotExist:
+        # If we're here, this is the first DraftChange we're making for
+        # this PublishableEntity in the active DraftChangeLog. 
+        change = DraftChangeLogRecord.objects.create(
+            draft_change_log=active_change_log,
+            entity_id=entity_id,
+            old_version_id=old_version_id,
+            new_version_id=new_version_id,
+        )
 
-        # One way or another, we've created our DraftChange at this point. Now
-        # to see if we need to add any parent Draft containers that this change
-        # may have affected (i.e. all unpinned references from Draft containers
-        # to the entity that we just changed the Draft entry for)...
-        containers = get_containers_with_entity(publishable_entity_id, ignore_pinned=True)
+    return change
 
 
+def _create_container_side_effects_for_draft_change_log(change_log: DraftChangeLogRecord):
+    seen_container_pks = set()
+    for change in change_log.changes.all():
+        _create_container_side_effects_for_draft_change(
+            change,
+            already_processed_containers_pks=seen_container_pks,
+        )
+
+
+def _create_container_side_effects_for_draft_change(
+    original_change: DraftChangeLogRecord,
+    already_processed_containers_pks=None
+):
+    """
+    Given a draft change, add side effects for all affected containers.
 
+    This should only be run after the DraftChangeLogRecord has been otherwise
+    fully written out. We want to avoid the scenario where we create a
+    side-effect that a Component change affects a Unit if the Unit version is
+    also changed in the same DraftChangeLog.
+
+    TODO: This could get very expensive with the get_containers_with_entity
+    calls. We should measure the impact of this.
+    """
+    if already_processed_containers_pks is not None:
+        seen_container_pks = already_processed_containers_pks
+    else:
+        seen_container_pks = set()
+
+    changes_and_containers = [
+        (original_change, container)
+        for container
+        in get_containers_with_entity(original_change.entity_id, ignore_pinned=True)
+    ]
+    while changes_and_containers:
+        change, container = changes_and_containers.pop()
+        # If we've already seen this container, no need to handle it again.
+        # This also guards against infinite cycle chains of containers.
+        if container.pk in seen_container_pks:
+            continue
+        seen_container_pks.add(container.pk)
+
+        # If the container is not already in the DraftChangeLog, we need to
+        # add it. Since it's being caused as a DraftSideEffect, we're going
+        # add it with the old_version == new_version convention.
+        container_draft_version_pk = container.versioning.draft.pk
+        container_change, _created = DraftChangeLogRecord.objects.get_or_create(
+            draft_change_log=change.draft_change_log,
+            entity_id=container.pk,
+            old_version_id=container_draft_version_pk,
+            defaults={
+                'new_version_id': container_draft_version_pk
+            }
+        )
+
+        # Mark that change in the current loop has the side effect of changing
+        # the parent. We'll do this regardless of whether the container version
+        # itself also changed. If a Unit has a Component and both the Unit and
+        # Component have their versions incremented, then the Unit has changed
+        # in both ways (the Unit's internal metadata as well as the new version
+        # of the child component).
+        DraftSideEffect.objects.get_or_create(cause=change, effect=container_change)
+
+        # Now we find the next layer up of containers. So if the originally
+        # passed in publishable_entity_id was for a Component, then the
+        # ``container`` we've been creating the side effect for in this loop
+        # is the Unit, and ``parents_of_container`` would be any Sequences
+        # that contain the Unit.
+        parents_of_container = get_containers_with_entity(container.pk, ignore_pinned=True)
+        changes_and_containers.extend(
+            (container_change, container_parent)
+            for container_parent in parents_of_container
+        )
 
 def soft_delete_draft(publishable_entity_id: int, /, deleted_by: int | None = None) -> None:
     """
@@ -1107,10 +1199,17 @@ def get_containers_with_entity(
     return qs
 
 
-def bulk_draft_changes_for(learning_package_id: int) -> DraftChangeLog:
+def bulk_draft_changes_for(learning_package_id: int, changed_by=None, changed_at=None) -> DraftChangeLog:
     """
     Context manager to do a single batch of Draft changes in.
 
     TODO: better description here with example usage.
     """
-    return DraftChangeLogContext(learning_package_id)
+    return DraftChangeLogContext(
+        learning_package_id,
+        changed_by=changed_by,
+        changed_at=changed_at,
+        exit_callbacks=[
+            _create_container_side_effects_for_draft_change_log,
+        ]
+    )

openedx_learning/apps/authoring/publishing/contextmanagers.py

@@ -40,14 +40,15 @@ class DraftChangeLogContext(Atomic):
     any operation on multiple Drafts as part of a DraftChangeSet will want to be
     an atomic operation. 
     """
-    _draft_change_sets: ContextVar[list | None] = ContextVar('_draft_change_sets', default=None)
+    _draft_change_logs: ContextVar[list | None] = ContextVar('_draft_change_logs', default=None)
 
-    def __init__(self, learning_package_id, changed_by=None, changed_at=None):
+    def __init__(self, learning_package_id, changed_by=None, changed_at=None, exit_callbacks=None):
         super().__init__(using=None, savepoint=False, durable=False)
 
         self.learning_package_id = learning_package_id
         self.changed_by = changed_by
         self.changed_at = changed_at or datetime.now(tz=timezone.utc)
+        self.exit_callbacks = exit_callbacks or []
 
     @classmethod
     def get_active_draft_change_log(cls, learning_package_id: int) -> DraftChangeLog | None:
@@ -58,10 +59,10 @@ def get_active_draft_change_log(cls, learning_package_id: int) -> DraftChangeLog
         modifies Drafts. If there is no active DraftChangeSet, this method will
         return None, and the caller should create their own DraftChangeSet.
         """
-        draft_change_sets = cls._draft_change_sets.get()
+        draft_change_logs = cls._draft_change_logs.get()
 
         # If we've never used this manager...
-        if draft_change_sets is None:
+        if draft_change_logs is None:
             return None
 
         # Otherwise, find the most recently created DraftChangeSet *that matches
@@ -76,9 +77,9 @@ def get_active_draft_change_log(cls, learning_package_id: int) -> DraftChangeLog
         #    a performance penalty", as opposed to, "we accidentally gave you a
         #    DraftChangeSet for an entirely different LearningPackage, and now
         #    your Draft data is corrupted."
-        for draft_change_set in reversed(draft_change_sets):
-            if draft_change_set.learning_package_id == learning_package_id:
-                return draft_change_set
+        for draft_change_log in reversed(draft_change_logs):
+            if draft_change_log.learning_package_id == learning_package_id:
+                return draft_change_log
 
         # If we got here, then either the list was empty (the manager was used
         # at some point but exited), or none of the DraftChangeSets are for the
@@ -88,23 +89,26 @@ def get_active_draft_change_log(cls, learning_package_id: int) -> DraftChangeLog
     def __enter__(self):
         super().__enter__()
 
-        self.draft_change_set = DraftChangeLog.objects.create(
+        self.draft_change_log = DraftChangeLog.objects.create(
             learning_package_id=self.learning_package_id,
             changed_by=self.changed_by,
             changed_at=self.changed_at,
         )
-        draft_change_sets = self._draft_change_sets.get()
+        draft_change_sets = self._draft_change_logs.get()
         if not draft_change_sets:
             draft_change_sets = []
-        draft_change_sets.append(self.draft_change_set)
-        self._draft_change_sets.set(draft_change_sets)
+        draft_change_sets.append(self.draft_change_log)
+        self._draft_change_logs.set(draft_change_sets)
 
-        return self.draft_change_set
+        return self.draft_change_log
 
     def __exit__(self, type, value, traceback):
-        draft_change_sets = self._draft_change_sets.get()
-        if draft_change_sets:
-            draft_change_sets.pop()
-        self._draft_change_sets.set(draft_change_sets)
+        draft_change_logs = self._draft_change_logs.get()
+        if draft_change_logs:
+            draft_change_log = draft_change_logs.pop()
+            for exit_callback in self.exit_callbacks:
+                exit_callback(draft_change_log)
+
+        self._draft_change_logs.set(draft_change_logs)
 
         return super().__exit__(type, value, traceback)

openedx_learning/apps/authoring/publishing/migrations/0004_draftchangelog.py

@@ -1,4 +1,4 @@
-# Generated by Django 4.2.18 on 2025-03-27 20:01
+# Generated by Django 4.2.18 on 2025-03-28 02:21
 
 from django.conf import settings
 from django.db import migrations, models
@@ -25,8 +25,8 @@ class Migration(migrations.Migration):
                 ('learning_package', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='oel_publishing.learningpackage')),
             ],
             options={
-                'verbose_name': 'Draft Change Set',
-                'verbose_name_plural': 'Draft Change Sets',
+                'verbose_name': 'Draft Change Log',
+                'verbose_name_plural': 'Draft Change Logs',
             },
         ),
         migrations.CreateModel(
@@ -47,10 +47,14 @@ class Migration(migrations.Migration):
             name='DraftSideEffect',
             fields=[
                 ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
-                ('cause', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, related_name='+', to='oel_publishing.draftchangelogrecord')),
-                ('effect', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, related_name='+', to='oel_publishing.draftchangelogrecord')),
+                ('cause', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, related_name='causes', to='oel_publishing.draftchangelogrecord')),
+                ('effect', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, related_name='caused_by', to='oel_publishing.draftchangelogrecord')),
             ],
         ),
+        migrations.AddConstraint(
+            model_name='draftsideeffect',
+            constraint=models.UniqueConstraint(fields=('cause', 'effect'), name='oel_pub_dse_uniq_c_e'),
+        ),
         migrations.AddIndex(
             model_name='draftchangelogrecord',
             index=models.Index(fields=['entity', '-draft_change_log'], name='oel_dlr_idx_entity_rdcl'),