refator: Set False ad default for direct

Author: ChrisChV

src/openedx_content/applets/publishing/api.py

@@ -848,25 +848,18 @@ def _create_side_effects_for_change_log(change_log: DraftChangeLog | PublishLog)
             # represents editing a Component, the side_effect_change is the
             # DraftChangeLogRecord that represents the fact that the containing
             # Unit was also altered (even if the Unit version doesn't change).
-            side_effect_defaults: dict = {
-                # If a change record already exists because the affected
-                # entity was separately modified, then we don't touch the
-                # old/new version entries. But if we're creating this change
-                # record as a pure side-effect, then we use the (old_version
-                # == new_version) convention to indicate that.
-                'old_version_id': affected.version_id,
-                'new_version_id': affected.version_id,
-            }
-            if branch_cls == Published:
-                # Pure side-effect records are never directly requested by
-                # the user, so mark them as indirect. If the record already
-                # exists (entity was explicitly selected or is a dependency),
-                # get_or_create won't overwrite the direct value it already has.
-                side_effect_defaults['direct'] = False
             side_effect_change, _created = change_record_cls.objects.get_or_create(
                 **change_log_param,
                 entity_id=affected.entity_id,
-                defaults=side_effect_defaults,
+                defaults={
+                    # If a change record already exists because the affected
+                    # entity was separately modified, then we don't touch the
+                    # old/new version entries. But if we're creating this change
+                    # record as a pure side-effect, then we use the (old_version
+                    # == new_version) convention to indicate that.
+                    'old_version_id': affected.version_id,
+                    'new_version_id': affected.version_id,
+                }
             )
             # Update the current branch pointer (Draft or Published) for this
             # entity to point to the side_effect_change (if it's not already).

src/openedx_content/applets/publishing/models/publish_log.py

@@ -111,16 +111,75 @@ class PublishLogRecord(models.Model):
     # the values may drift away from each other.
     dependencies_hash_digest = hash_field(blank=True, default='', max_length=8)
 
-    # True if this entity was explicitly requested to be published by the user
-    # (e.g. they clicked "publish" on this entity or selected it for bulk publish).
-    # False if it was indirectly published as a child/dependency of a directly
-    # published entity (e.g. a Component published because its parent Unit was
-    # published).
-    # None for historical records created before this field was added.
+    # The "direct" field captures user intent during the publishing process. It
+    # is True if the user explicitly requested to publish the entity represented
+    # by this PublishLogRecord—i.e. they clicked "publish" on this entity or
+    # selected it for bulk publish.
+    #
+    # This field is False if this entity was indirectly published either as a
+    # child/dependency or side-effect of a directly published entity.
+    #
+    # If this field is None, that means that this PublishLogRecord was created
+    # before we started capturing user intent (pre-Verawood release), and we
+    # cannot reliably infer what the user clicked on. For example, say we had a
+    # Subsection > Unit > Component arrangement where the Component had an
+    # unpublished change. The user is allowed to press the "publish" button at
+    # the Subsection, Unit, or Component levels in the UI. Before we started
+    # recording this field, the resulting PublishLogs would have looked
+    # identical in all three cases: a version change PublishLogRecord for
+    # Component, and side-effect records for the Unit and Subsection. Therefore,
+    # we cannot accurately backfill this field.
+    #
+    # Here are some examples to illustrate how "direct" gets set and why:
+    #
+    # Example 1: The user clicks "publish" on a Component that's in a Unit.
+    #
+    #   The Component has direct=True, but the side-effect PublishLogRecord for
+    #   the Unit has direct=False. Likewise, any side-effect records at higher
+    #   levels (subsection, section) also have direct=False.
+    #
+    # Example 2: The user clicks "publish" on a Unit, where both the Unit and
+    # Component have unpublished changes:
+    #
+    #   In this case, the Unit has direct=True, and the Component has
+    #   direct=False. The draft status of the Component is irrelevant. The user
+    #   asked for the Unit to the published, so the Unit's PublishLogRecord is
+    #   the only thing that gets direct=True.
+    #
+    # Example 3: The user clicks "publish" on a Unit that has no changes of its
+    # own (draft version == published version), but the Unit contains a Component
+    # that has changes.
+    #
+    #   Again, only the PublishLogRecord for the Unit has direct=True. The
+    #   Component's PublishLogRecord has direct=False. Even though the Unit's
+    #   published version_num does not change (i.e. it is purely a side-effect
+    #   publish), the user intent was to publish the Unit (and anything it
+    #   contains), so the Unit gets direct=True.
+    #
+    # Example 4: The user selects multiple entities for bulk publishing.
+    #
+    #   Those exact entities that the user selected get direct=True. It does not
+    #   matter if some of those entities are children of other selected items or
+    #   not. Other entries like dependencies or side-effects have direct=False.
+    #
+    # Example 5: The user selects "publish all".
+    #
+    #   Selecting "publish all" in our system currently translates into "publish
+    #   all the entities that have a draft version that is different from its
+    #   published version". Those entities would get PublishLogRecords with
+    #   direct=True, while all side-effects would get records with direct=False.
+    #   So if a Unit's draft and published versions match, and one of its
+    #   Components has unpublished changes, then "publish all" would cause the
+    #   Component's record to have direct=True and the Unit's record to have
+    #   direct=False.
+    #
+    #   All PublishLogRecords in the PublishLog have direct=True. The "publish
+    #   all" operation is indistinguishable from bulk publishing and selecting
+    #   every single item.
     direct = models.BooleanField(
         null=True,
         blank=True,
-        default=None,
+        default=False,
     )
 
     class Meta:

src/openedx_content/migrations/0007_publishlogrecord_direct.py

@@ -3,6 +3,16 @@
 from django.db import migrations, models
 
 
+def backfill_direct_to_none(apps, schema_editor):
+    """
+    Set direct=None for all pre-existing PublishLogRecords so they are treated
+    as historical records whose user intent cannot be determined retroactively.
+    New records created after this migration will default to direct=False.
+    """
+    PublishLogRecord = apps.get_model('openedx_content', 'PublishLogRecord')
+    PublishLogRecord.objects.update(direct=None)
+
+
 class Migration(migrations.Migration):
 
     dependencies = [
@@ -13,6 +23,10 @@ class Migration(migrations.Migration):
         migrations.AddField(
             model_name='publishlogrecord',
             name='direct',
-            field=models.BooleanField(blank=True, default=None, null=True),
+            field=models.BooleanField(blank=True, default=False, null=True),
+        ),
+        migrations.RunPython(
+            backfill_direct_to_none,
+            reverse_code=migrations.RunPython.noop,
         ),
     ]

tests/openedx_content/applets/publishing/test_api.py

@@ -20,6 +20,7 @@
     LearningPackage,
     PublishableEntity,
     PublishLog,
+    PublishLogRecord,
 )
 
 User = get_user_model()
@@ -937,16 +938,25 @@ def test_simple_publish_log(self) -> None:
 
     def test_publish_all_drafts_sets_direct_true(self) -> None:
         """publish_all_drafts() marks every PublishLogRecord as direct=True."""
-        entity = publishing_api.create_publishable_entity(
-            self.learning_package_1.id, "direct_entity",
+        entity_1 = publishing_api.create_publishable_entity(
+            self.learning_package_1.id, "direct_entity_1",
+            created=self.now, created_by=None,
+        )
+        publishing_api.create_publishable_entity_version(
+            entity_1.id, version_num=1, title="Direct Entity 1",
+            created=self.now, created_by=None,
+        )
+        entity_2 = publishing_api.create_publishable_entity(
+            self.learning_package_1.id, "direct_entity_2",
             created=self.now, created_by=None,
         )
         publishing_api.create_publishable_entity_version(
-            entity.id, version_num=1, title="Direct Entity",
+            entity_2.id, version_num=1, title="Direct Entity 2",
             created=self.now, created_by=None,
         )
         publish_log = publishing_api.publish_all_drafts(self.learning_package_1.id)
-        assert publish_log.records.get(entity=entity).direct is True
+        assert publish_log.records.get(entity=entity_1).direct is True
+        assert publish_log.records.get(entity=entity_2).direct is True
 
     def test_publish_from_drafts_sets_direct_true(self) -> None:
         """An explicitly selected entity in publish_from_drafts() gets direct=True."""
@@ -964,6 +974,18 @@ def test_publish_from_drafts_sets_direct_true(self) -> None:
         )
         assert publish_log.records.get(entity=entity).direct is True
 
+    def test_publish_log_record_direct_defaults_to_false(self) -> None:
+        """
+        New PublishLogRecords default to direct=False (not None).
+
+        None is reserved for historical records that pre-date the direct field
+        (set via the backfill data migration). Records created by the
+        application—e.g. side-effect records in _create_side_effects_for_change_log()
+        that don't explicitly set direct—should get False, not None.
+        """
+        field = PublishLogRecord._meta.get_field('direct')
+        assert field.default is False
+
 
 class EntitiesQueryTestCase(TestCase):
     """
@@ -1486,6 +1508,64 @@ def test_direct_field_publishing_container_marks_dependencies_indirect(self) ->
         assert publish_log.records.get(entity=unit.publishable_entity).direct is True
         assert publish_log.records.get(entity=component).direct is False
 
+    def test_direct_field_unit_no_version_change_still_direct_true(self) -> None:
+        """
+        Publishing a Unit that has no version change of its own (draft version
+        == published version) still marks the Unit's record as direct=True.
+
+        The user explicitly selected the Unit to publish, so it gets direct=True
+        even though the only actual change is in its Component child. The Unit's
+        record has old_version == new_version (pure side-effect in terms of
+        versioning), but user intent was directed at the Unit.
+        """
+        component = publishing_api.create_publishable_entity(
+            self.learning_package.id, "no_change_component",
+            created=self.now, created_by=None,
+        )
+        component_v1 = publishing_api.create_publishable_entity_version(
+            component.id, version_num=1, title="No-change Component",
+            created=self.now, created_by=None,
+        )
+        unit = containers_api.create_container(
+            self.learning_package.id, "no_change_unit",
+            created=self.now, created_by=None, container_cls=TestContainer,
+        )
+        unit_v1 = containers_api.create_container_version(
+            unit.id, 1, title="No-change Unit", entities=[component],
+            created=self.now, created_by=None,
+        )
+        # Initial publish so both Unit and Component have a published version.
+        publishing_api.publish_from_drafts(
+            self.learning_package.id,
+            Draft.objects.filter(entity=unit.publishable_entity),
+        )
+
+        # Create a new Component version. The Unit's draft stays at unit_v1,
+        # but its dependencies_hash_digest now differs from the published state.
+        publishing_api.create_publishable_entity_version(
+            component.id, version_num=2, title="No-change Component v2",
+            created=self.now, created_by=None,
+        )
+
+        # Publish the Unit explicitly. The Unit has no version change of its
+        # own (old_version == new_version == unit_v1).
+        publish_log = publishing_api.publish_from_drafts(
+            self.learning_package.id,
+            Draft.objects.filter(entity=unit.publishable_entity),
+        )
+        unit_record = publish_log.records.get(entity=unit.publishable_entity)
+        component_record = publish_log.records.get(entity=component)
+
+        # User selected the Unit → direct=True despite no version change.
+        assert unit_record.direct is True
+        assert unit_record.old_version_id == unit_v1.pk
+        assert unit_record.new_version_id == unit_v1.pk
+
+        # Component was pulled in as a dependency → direct=False.
+        assert component_record.direct is False
+        assert component_record.old_version == component_v1
+        assert component_record.new_version != component_v1
+
     def test_direct_field_publishing_component_marks_parent_indirect(self) -> None:
         """
         Publishing a Component directly marks the Component as direct=True.