temp: moved draft processing over

Author: ormsbee

openedx_learning/apps/authoring/publishing/api.py

@@ -359,6 +359,9 @@ def publish_from_drafts(
         # If the drafts include any containers, we need to auto-publish their descendants:
         # TODO: this only handles one level deep and would need to be updated to support sections > subsections > units
 
+        # note to self; manytomany through table for
+        # dependencies = draft_qset.filter(dependency_set)
+
         # Get the IDs of the ContainerVersion for any Containers whose drafts are slated to be published.
         container_version_ids = draft_qset.filter(entity__container__isnull=False) \
                                           .values_list("version_id", flat=True)
@@ -417,7 +420,7 @@ def publish_from_drafts(
             )
 
         # Calculate the side-effects...
-        _create_container_side_effects_for_publish_log(publish_log)
+        _create_side_effects_for_publish_log(publish_log)
 
         # Calculate state updates...
         _update_state_hash(publish_log)
@@ -562,7 +565,7 @@ def set_draft_version(
                 old_version_id=old_version_id,
                 new_version_id=publishable_entity_version_pk,
             )
-            _create_container_side_effects_for_draft_change(change)
+            _create_side_effects_for_draft_change(change)
 
 
 def _add_to_existing_draft_change_log(
@@ -628,7 +631,7 @@ def _add_to_existing_draft_change_log(
     return change
 
 
-def _create_container_side_effects_for_publish_log(publish_log: PublishLog):
+def _create_side_effects_for_publish_log(publish_log: PublishLog):
     """
     Iterate through PublishLog and add the appropriate PublishSideEffects.
 
@@ -711,24 +714,24 @@ def _create_container_side_effects_for_publish_log(publish_log: PublishLog):
             )
 
 
-def _create_container_side_effects_for_draft_change_log(change_log: DraftChangeLog):
+def _create_side_effects_for_draft_change_log(change_log: DraftChangeLog):
     """
     Iterate through the whole DraftChangeLog and process side-effects.
     """
     processed_entity_ids: set[int] = set()
     for change in change_log.records.all():
-        _create_container_side_effects_for_draft_change(
+        _create_side_effects_for_draft_change(
             change,
             processed_entity_ids=processed_entity_ids,
         )
 
 
-def _create_container_side_effects_for_draft_change(
+def _create_side_effects_for_draft_change(
     original_change: DraftChangeLogRecord,
     processed_entity_ids: set | None = None
 ):
     """
-    Given a draft change, add side effects for all affected containers.
+    Given a draft change, add side effects for all affected Drafts.
 
     This should only be run after the DraftChangeLogRecord has been otherwise
     fully written out. We want to avoid the scenario where we create a
@@ -741,32 +744,34 @@ def _create_container_side_effects_for_draft_change(
     calling this function in a loop for all the Components in a Unit, we won't
     be recalculating the Unit's side-effect on its Subsection, and its
     Subsection's side-effect on its Section.
-
-    TODO: This could get very expensive with the get_containers_with_entity
-    calls. We should measure the impact of this.
     """
     if processed_entity_ids is None:
         # An optimization, but also a guard against infinite side-effect loops.
         processed_entity_ids = set()
 
-    changes_and_containers = [
-        (original_change, container)
-        for container
-        in get_containers_with_entity(original_change.entity_id, ignore_pinned=True)
+    # This is basically original_change.entity.draft.causes_side_effects_for,
+    # but faster and safer (handles deleted drafts).
+    target_dependencies = (
+        DraftDependency.objects
+                       .filter(dependency_id=original_change.entity_id)
+                       .select_related("draft")
+    )
+    changes_and_side_effect_target_drafts = [
+        (original_change, target_dep.target) for target_dep in target_dependencies
     ]
-    while changes_and_containers:
-        change, container = changes_and_containers.pop()
+    while changes_and_side_effect_target_drafts:
+        change, draft = changes_and_side_effect_target_drafts.pop()
 
         # 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(
+        target_draft_version_pk = draft.version_id
+        target_change, _created = DraftChangeLogRecord.objects.get_or_create(
             draft_change_log=change.draft_change_log,
-            entity_id=container.pk,
+            entity_id=draft.pk,
             defaults={
-                'old_version_id': container_draft_version_pk,
-                'new_version_id': container_draft_version_pk
+                'old_version_id': target_draft_version_pk,
+                'new_version_id': target_draft_version_pk
             }
         )
 
@@ -776,20 +781,20 @@ def _create_container_side_effects_for_draft_change(
         # 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)
+        DraftSideEffect.objects.get_or_create(cause=change, effect=target_change)
         processed_entity_ids.add(change.entity_id)
 
         # 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 Subsections
         # that contain the Unit.
-        parents_of_container = get_containers_with_entity(container.pk, ignore_pinned=True)
+        target_deps_of_draft = list(draft.causes_side_effects_for.all().select_related("draft"))
 
-        changes_and_containers.extend(
-            (container_change, container_parent)
-            for container_parent in parents_of_container
-            if container_parent.pk not in processed_entity_ids
+        changes_and_side_effect_target_drafts.extend(
+            (target_change, target_dep.draft)
+            for target_dep in target_deps_of_draft
+            if target_dep.pk not in processed_entity_ids
         )
 
 
@@ -811,29 +816,16 @@ def set_draft_dependencies(draft: Draft, dependency_drafts: QuerySet[Draft]) ->
        like "all descendants" based on this.
     2. Declare dependencies from the bottom-up. In other words, if you're
        building an entire Subsection, set the Component dependencies for the
-       Units before you set the Unit dependencies for the Subsection.
-    3. Circular dependencies are not supported. It might not blow up when you
-       set them, and it won't bring down the system when you do operations on
-       them, but the behavior is undefined. See note below.
-
-    We're not currently guarding against the creation of circular dependencies
-    because a) checking can become expensive; and b) it's an edge case that is
-    unlikely to occur given the different types involved (e.g. Subsections can
-    contain Units but Units can't contain Subsections).
-
-    We *will* guard against infinite loops in the actions we want to take with
-    dependencies, like publishing them or calculating their side-effects.
-
-    That being said, we're defining the state for a Draft to be a function of
-    the UUIDs + states of all of its dependencies, and making a circular
-    dependency means that the state calculation will be incorrect and will
-    probably result in weird behavior.
+       Units before you set the Unit dependencies for the Subsection. This code
+       will still work if you build from the top-down, but we'll end up doing
+       many redundant re-calculations, since every change to a lower layer will
+       cause recalculation to the higher levels that depend on it.
+    3. Circular dependencies are not supported, and will raise a ``ValueError``.
     """
     new_dependency_drafts = (
         dependency_drafts.exclude(pk=draft.pk)  # draft can't depend on itself
                          .distinct()            # duplicates not allowed
     )
-    new_state_hash = _calcuate_dependencies_state_hash(new_dependency_drafts)
 
     # We're going to delete any DraftDependency rows that currently exist for
     # this draft but aren't represented in the new_dependency_drafts Draft qset.
@@ -849,7 +841,7 @@ def set_draft_dependencies(draft: Draft, dependency_drafts: QuerySet[Draft]) ->
         DraftDependency.objects.bulk_create(
             [
                 DraftDependency(
-                    draft=draft,
+                    target=draft,
                     dependency_id=dependency_draft.pk,
                     version_at_creation_id=draft.version_id,
                 )
@@ -859,7 +851,7 @@ def set_draft_dependencies(draft: Draft, dependency_drafts: QuerySet[Draft]) ->
             # because our value for "version_at_creation" will be incorrect.
             ignore_conflicts=True,
         )
-        draft.state_hash = new_state_hash
+        draft.state_hash = _calcuate_state_hash(draft)
 
         # We've updated this Draft's state_hash based on its dependencies, but
         # now we have to re-calculate the state_hash for things that are
@@ -875,19 +867,23 @@ def set_draft_dependencies(draft: Draft, dependency_drafts: QuerySet[Draft]) ->
                     "circular dependency."
                 )
 
-            # Maybe always write the DraftDependency rows first so this function always works on one
-            # queryset type?
-            draft_with_outdated_state.state_hash = _calcuate_dependencies_state_hash(
-                Draft.objects.filter(
-                    pk__in=draft_with_outdated_state.dependencies.all().values_list("draft_id", flat=True)
-                )
+            draft_with_outdated_state.state_hash = _calcuate_state_hash(draft_with_outdated_state)
+            drafts_with_outdated_state.extend(
+                dep.target for dep in draft_with_outdated_state.causes_side_effects_for.all().select_related("target")
             )
 
 
-def _calcuate_dependencies_state_hash(dependency_drafts: QuerySet[Draft]) -> str:
+def _calcuate_state_hash(draft: Draft) -> str:
     """
     Generate a new value that can be put in Draft.state_hash.
 
+    We calculate the state hash based on the versions (UUIDs) and state_hash
+    values of all of its dependencies. There is no sense of ordering with these
+    dependencies. A Unit with Components [C1, C2] is the same to us as a Unit
+    with [C2, C1], and both will have the same state_hash. We rely on the fact
+    that containers store their ordering and that changes in that ordering mean
+    that the version of the container itself (e.g. the Unit) changes.
+
     Edge case: Deleted Drafts (i.e. draft.version == None)
 
     When this happens, the deleted Draft still counts as a dependency because
@@ -898,13 +894,17 @@ def _calcuate_dependencies_state_hash(dependency_drafts: QuerySet[Draft]) -> str
     won't affect the top level Draft until the Unit is un-deleted, at which
     point we'd have to recalcuate the state_hash anyway.
     """
-    # We need to re-order for deterministic state calculation
-    dependency_drafts = dependency_drafts.order_by("version__uuid").select_related("version")
-
+    dependencies: QuerySet[DraftDependency] = (
+        draft.dependencies
+             .all()
+             .select_related("dependency__version")
+             # need to guarantee order for deterministic state calculation
+             .order_by("dependency__version__uuid")
+    )
     dependency_states = [
-        f"{dep_draft.version.uuid}:{dep_draft.state_hash}"
-        for dep_draft in dependency_drafts
-        if dep_draft.version  # skip deleted drafts
+        f"{dd.dependency.version.uuid}:{dd.dependency.state_hash}"
+        for dd in dependencies
+        if dd.dependency.version  # skip deleted drafts
     ]
     # Example line: [TODO: Fill this in]
     dependency_summary_text = "\n".join(dependency_states)
@@ -1689,6 +1689,6 @@ def bulk_draft_changes_for(
         changed_at=changed_at,
         changed_by=changed_by,
         exit_callbacks=[
-            _create_container_side_effects_for_draft_change_log,
+            _create_side_effects_for_draft_change_log,
         ]
     )

openedx_learning/apps/authoring/publishing/migrations/0011_remove_draftdependency_oel_pub_dd_uniq_draft_dep_and_more.py

@@ -0,0 +1,26 @@
+# Generated by Django 4.2.21 on 2025-06-07 00:56
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('oel_publishing', '0010_draft_dependency'),
+    ]
+
+    operations = [
+        migrations.RemoveConstraint(
+            model_name='draftdependency',
+            name='oel_pub_dd_uniq_draft_dep',
+        ),
+        migrations.RenameField(
+            model_name='draftdependency',
+            old_name='draft',
+            new_name='target',
+        ),
+        migrations.AddConstraint(
+            model_name='draftdependency',
+            constraint=models.UniqueConstraint(fields=('target', 'dependency'), name='oel_pub_dd_uniq_target_dep'),
+        ),
+    ]

openedx_learning/apps/authoring/publishing/models/draft_log.py

@@ -348,7 +348,7 @@ class DraftDependency(models.Model):
     """
 
     """
-    draft = models.ForeignKey(
+    target = models.ForeignKey(
         Draft,
         on_delete=models.CASCADE,
         related_name="dependencies",
@@ -369,7 +369,7 @@ class Meta:
             # Duplicate entries for a dependency are just redundant. This is
             # here to guard against weird bugs that might introduce this state.
             models.UniqueConstraint(
-                fields=["draft", "dependency"],
-                name="oel_pub_dd_uniq_draft_dep",
+                fields=["target", "dependency"],
+                name="oel_pub_dd_uniq_target_dep",
             )
         ]

openedx_learning/apps/authoring/publishing/models/publish_log.py

@@ -207,3 +207,6 @@ class Meta:
         ]
         verbose_name = _("Publish Side Effect")
         verbose_name_plural = _("Publish Side Effects")
+
+
+

tests/openedx_learning/apps/authoring/publishing/test_api.py

@@ -1199,3 +1199,48 @@ def test_multiple_layers_of_containers(self):
         assert unit_publish.affected_by.first().cause == component_publish
         assert subsection_publish.affected_by.count() == 1
         assert subsection_publish.affected_by.first().cause == unit_publish
+
+    def test_publish_all_layers(self):
+        """Test that we can publish multiple layers from one root."""
+        # Note that these aren't real "components" and "units". Everything being
+        # tested is confined to the publishing app, so those concepts shouldn't
+        # be imported here. They're just named this way to make it more obvious
+        # what the intended hierarchy is for testing container nesting.
+        component = publishing_api.create_publishable_entity(
+            self.learning_package.id, "component_1", created=self.now, created_by=None,
+        )
+        publishing_api.create_publishable_entity_version(
+            component.id, version_num=1, title="Component 1 🌴", created=self.now, created_by=None,
+        )
+        unit = publishing_api.create_container(
+            self.learning_package.id, "unit_1", created=self.now, created_by=None,
+        )
+        publishing_api.create_container_version(
+            unit.pk,
+            1,
+            title="My Unit",
+            entity_rows=[
+                publishing_api.ContainerEntityRow(entity_pk=component.pk),
+            ],
+            created=self.now,
+            created_by=None,
+        )
+        subsection = publishing_api.create_container(
+            self.learning_package.id, "subsection_1", created=self.now, created_by=None,
+        )
+        publishing_api.create_container_version(
+            subsection.pk,
+            1,
+            title="My Subsection",
+            entity_rows=[
+                publishing_api.ContainerEntityRow(entity_pk=unit.pk),
+            ],
+            created=self.now,
+            created_by=None,
+        )
+        publish_log = publishing_api.publish_from_drafts(
+            self.learning_package.id,
+            Draft.objects.filter(pk=subsection.pk),
+        )
+        assert publish_log.records.count() == 3
+

tests/openedx_learning/apps/authoring/sections/test_api.py

@@ -214,9 +214,9 @@ def test_create_section_queries(self):
         Test how many database queries are required to create a section
         """
         # The exact numbers here aren't too important - this is just to alert us if anything significant changes.
-        with self.assertNumQueries(27):
+        with self.assertNumQueries(29):
             _empty_section = self.create_section_with_subsections([])
-        with self.assertNumQueries(35):
+        with self.assertNumQueries(36):
             # And try with a non-empty section:
             self.create_section_with_subsections([self.subsection_1, self.subsection_2_v1], key="u2")
 

tests/openedx_learning/apps/authoring/subsections/test_api.py

@@ -197,9 +197,9 @@ def test_create_subsection_queries(self):
         Test how many database queries are required to create a subsection
         """
         # The exact numbers here aren't too important - this is just to alert us if anything significant changes.
-        with self.assertNumQueries(27):
+        with self.assertNumQueries(29):
             _empty_subsection = self.create_subsection_with_units([])
-        with self.assertNumQueries(35):
+        with self.assertNumQueries(36):
             # And try with a non-empty subsection:
             self.create_subsection_with_units([self.unit_1, self.unit_2_v1], key="u2")
 

tests/openedx_learning/apps/authoring/units/test_api.py

@@ -185,9 +185,9 @@ def test_create_unit_queries(self):
         Test how many database queries are required to create a unit
         """
         # The exact numbers here aren't too important - this is just to alert us if anything significant changes.
-        with self.assertNumQueries(25):
+        with self.assertNumQueries(27):
             _empty_unit = self.create_unit_with_components([])
-        with self.assertNumQueries(32):
+        with self.assertNumQueries(33):
             # And try with a non-empty unit:
             self.create_unit_with_components([self.component_1, self.component_2_v1], key="u2")