diff --git a/src/openedx_content/applets/publishing/admin.py b/src/openedx_content/applets/publishing/admin.py index 64b0035fc..85adcc5d9 100644 --- a/src/openedx_content/applets/publishing/admin.py +++ b/src/openedx_content/applets/publishing/admin.py @@ -43,6 +43,7 @@ class PublishLogRecordTabularInline(admin.TabularInline): "old_version_num", "new_version_num", "dependencies_hash_digest", + "direct", ) readonly_fields = fields diff --git a/src/openedx_content/applets/publishing/api.py b/src/openedx_content/applets/publishing/api.py index 3bbcec19c..2fe45a7c4 100644 --- a/src/openedx_content/applets/publishing/api.py +++ b/src/openedx_content/applets/publishing/api.py @@ -445,6 +445,9 @@ def publish_from_drafts( else: dependency_drafts_qsets = [] + # Collect PKs of directly-requested drafts before expanding dependencies. + direct_draft_ids = set(draft_qset.values_list('pk', flat=True)) + # One PublishLog for this entire publish operation. publish_log = PublishLog( learning_package_id=learning_package_id, @@ -484,6 +487,7 @@ def publish_from_drafts( entity=draft.entity, old_version=old_version, new_version=draft.version, + direct=draft.pk in direct_draft_ids, ) publish_log_record.full_clean() publish_log_record.save(force_insert=True) diff --git a/src/openedx_content/applets/publishing/models/publish_log.py b/src/openedx_content/applets/publishing/models/publish_log.py index 3f88f31e2..e6f8799fb 100644 --- a/src/openedx_content/applets/publishing/models/publish_log.py +++ b/src/openedx_content/applets/publishing/models/publish_log.py @@ -111,6 +111,77 @@ class PublishLogRecord(models.Model): # the values may drift away from each other. dependencies_hash_digest = hash_field(blank=True, default='', max_length=8) + # 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=False, + ) + class Meta: constraints = [ # A Publishable can have only one PublishLogRecord per PublishLog. diff --git a/src/openedx_content/migrations/0007_publishlogrecord_direct.py b/src/openedx_content/migrations/0007_publishlogrecord_direct.py new file mode 100644 index 000000000..a3cd3be10 --- /dev/null +++ b/src/openedx_content/migrations/0007_publishlogrecord_direct.py @@ -0,0 +1,32 @@ +# Generated by Django 5.2.13 on 2026-04-13 21:28 + +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 = [ + ('openedx_content', '0006_typed_ids'), + ] + + operations = [ + migrations.AddField( + model_name='publishlogrecord', + name='direct', + field=models.BooleanField(blank=True, default=False, null=True), + ), + migrations.RunPython( + backfill_direct_to_none, + reverse_code=migrations.RunPython.noop, + ), + ] diff --git a/tests/openedx_content/applets/containers/test_api.py b/tests/openedx_content/applets/containers/test_api.py index 9a5fdb431..f40f16cd5 100644 --- a/tests/openedx_content/applets/containers/test_api.py +++ b/tests/openedx_content/applets/containers/test_api.py @@ -925,7 +925,7 @@ def test_contains_unpublished_changes_queries( assert containers_api.contains_unpublished_changes(grandparent.id) # Publish grandparent and all its descendants: - with django_assert_num_queries(135): # TODO: investigate as this seems high! + with django_assert_num_queries(136): # TODO: investigate as this seems high! publish_entity(grandparent) # Tests: @@ -1244,7 +1244,7 @@ def test_uninstalled_publish( """Simple test of publishing a container of uninstalled type, plus its child, and reviewing the publish log""" # Publish container_of_uninstalled_type (and child_entity1). Should not affect anything else, # but we should see "child_entity1" omitted from the subsequent publish. - with django_assert_num_queries(49): + with django_assert_num_queries(50): publish_log = publish_entity(container_of_uninstalled_type) # Nothing else should have been affected by the publish: assert list(publish_log.records.order_by("entity__pk").values_list("entity__key", flat=True)) == [ @@ -1282,7 +1282,7 @@ def test_deep_publish_log( ) # Publish container_of_uninstalled_type (and child_entity1). Should not affect anything else, # but we should see "child_entity1" omitted from the subsequent publish. - with django_assert_num_queries(49): + with django_assert_num_queries(50): publish_log = publish_entity(container_of_uninstalled_type) # Nothing else should have been affected by the publish: assert list(publish_log.records.order_by("entity__pk").values_list("entity__key", flat=True)) == [ @@ -1291,7 +1291,7 @@ def test_deep_publish_log( ] # Publish great_grandparent. Should publish the whole tree. - with django_assert_num_queries(126): + with django_assert_num_queries(127): publish_log = publish_entity(great_grandparent) assert list(publish_log.records.order_by("entity__pk").values_list("entity__key", flat=True)) == [ "child_entity2", diff --git a/tests/openedx_content/applets/publishing/test_api.py b/tests/openedx_content/applets/publishing/test_api.py index 8da51120a..b39a74ea3 100644 --- a/tests/openedx_content/applets/publishing/test_api.py +++ b/tests/openedx_content/applets/publishing/test_api.py @@ -20,6 +20,7 @@ LearningPackage, PublishableEntity, PublishLog, + PublishLogRecord, ) User = get_user_model() @@ -935,6 +936,56 @@ def test_simple_publish_log(self) -> None: assert e1_pub_record.old_version == entity1_v1 assert e1_pub_record.new_version == entity1_v2 + def test_publish_all_drafts_sets_direct_true(self) -> None: + """publish_all_drafts() marks every PublishLogRecord as direct=True.""" + 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_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_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.""" + entity = publishing_api.create_publishable_entity( + self.learning_package_1.id, "explicit_entity", + created=self.now, created_by=None, + ) + publishing_api.create_publishable_entity_version( + entity.id, version_num=1, title="Explicit Entity", + created=self.now, created_by=None, + ) + publish_log = publishing_api.publish_from_drafts( + self.learning_package_1.id, + Draft.objects.filter(entity=entity), + ) + 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): """ @@ -1429,6 +1480,158 @@ def test_publish_all_layers(self) -> None: # the publish log records. assert publish_log.records.count() == 3 + def test_direct_field_publishing_container_marks_dependencies_indirect(self) -> None: + """ + Publishing a Unit explicitly marks the Unit as direct=True and its + unpublished Component dependency as direct=False. + """ + component = publishing_api.create_publishable_entity( + self.learning_package.id, "direct_component", + created=self.now, created_by=None, + ) + publishing_api.create_publishable_entity_version( + component.id, version_num=1, title="Direct Component", + created=self.now, created_by=None, + ) + unit = containers_api.create_container( + self.learning_package.id, "direct_unit", + created=self.now, created_by=None, container_cls=TestContainer, + ) + containers_api.create_container_version( + unit.id, 1, title="Direct Unit", entities=[component], + created=self.now, created_by=None, + ) + publish_log = publishing_api.publish_from_drafts( + self.learning_package.id, + Draft.objects.filter(entity=unit.publishable_entity), + ) + 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. + The parent Unit also gets a PublishLogRecord (because it has an unpinned + reference to the Component and its dependencies_hash_digest now differs + from the published state) with direct=False. + """ + component = publishing_api.create_publishable_entity( + self.learning_package.id, "leaf_component", + created=self.now, created_by=None, + ) + publishing_api.create_publishable_entity_version( + component.id, version_num=1, title="Leaf Component", + created=self.now, created_by=None, + ) + unit = containers_api.create_container( + self.learning_package.id, "leaf_unit", + created=self.now, created_by=None, container_cls=TestContainer, + ) + containers_api.create_container_version( + unit.id, 1, title="Leaf Unit", entities=[component], + created=self.now, created_by=None, + ) + # First publish everything to establish a published baseline for the Unit + publishing_api.publish_all_drafts(self.learning_package.id) + + # Create a new component version so it has unpublished changes + publishing_api.create_publishable_entity_version( + component.id, version_num=2, title="Leaf Component v2", + created=self.now, created_by=None, + ) + publish_log = publishing_api.publish_from_drafts( + self.learning_package.id, + Draft.objects.filter(entity=component), + ) + assert publish_log.records.get(entity=component).direct is True + assert publish_log.records.get(entity=unit.publishable_entity).direct is False + + def test_direct_field_both_selected_both_direct(self) -> None: + """ + When both a Unit and its Component are explicitly selected, both + get direct=True even though Component is also a dependency of Unit. + """ + component = publishing_api.create_publishable_entity( + self.learning_package.id, "both_component", + created=self.now, created_by=None, + ) + publishing_api.create_publishable_entity_version( + component.id, version_num=1, title="Both Component", + created=self.now, created_by=None, + ) + unit = containers_api.create_container( + self.learning_package.id, "both_unit", + created=self.now, created_by=None, container_cls=TestContainer, + ) + containers_api.create_container_version( + unit.id, 1, title="Both Unit", entities=[component], + created=self.now, created_by=None, + ) + publish_log = publishing_api.publish_from_drafts( + self.learning_package.id, + Draft.objects.filter(entity__in=[component, unit.publishable_entity]), + ) + assert publish_log.records.get(entity=component).direct is True + assert publish_log.records.get(entity=unit.publishable_entity).direct is True + def test_container_next_version(self) -> None: """Test that next_version works for containers.""" child_1 = publishing_api.create_publishable_entity( diff --git a/tests/openedx_content/applets/sections/test_api.py b/tests/openedx_content/applets/sections/test_api.py index d3464b6bb..3e873915a 100644 --- a/tests/openedx_content/applets/sections/test_api.py +++ b/tests/openedx_content/applets/sections/test_api.py @@ -155,7 +155,7 @@ def test_section_queries(self) -> None: """ with self.assertNumQueries(37): section = self.create_section_with_subsections([self.subsection_1, self.subsection_2_v1]) - with self.assertNumQueries(160): + with self.assertNumQueries(161): content_api.publish_from_drafts( self.learning_package.id, draft_qset=content_api.get_all_drafts(self.learning_package.id).filter(entity=section.id), diff --git a/tests/openedx_content/applets/subsections/test_api.py b/tests/openedx_content/applets/subsections/test_api.py index 344951304..e4bc871e2 100644 --- a/tests/openedx_content/applets/subsections/test_api.py +++ b/tests/openedx_content/applets/subsections/test_api.py @@ -133,7 +133,7 @@ def test_subsection_queries(self) -> None: """ with self.assertNumQueries(37): subsection = self.create_subsection_with_units([self.unit_1, self.unit_1_v1]) - with self.assertNumQueries(102): # TODO: this seems high? + with self.assertNumQueries(103): # TODO: this seems high? content_api.publish_from_drafts( self.learning_package.id, draft_qset=content_api.get_all_drafts(self.learning_package.id).filter(entity=subsection.id), diff --git a/tests/openedx_content/applets/units/test_api.py b/tests/openedx_content/applets/units/test_api.py index 944c5d076..6ca67fdf0 100644 --- a/tests/openedx_content/applets/units/test_api.py +++ b/tests/openedx_content/applets/units/test_api.py @@ -134,7 +134,7 @@ def test_unit_queries(self) -> None: """ with self.assertNumQueries(35): unit = self.create_unit_with_components([self.component_1, self.component_2_v1]) - with self.assertNumQueries(48): # TODO: this seems high? + with self.assertNumQueries(49): # TODO: this seems high? content_api.publish_from_drafts( self.learning_package.id, draft_qset=content_api.get_all_drafts(self.learning_package.id).filter(entity=unit.id),