fix: populate draft ChannelVersion metadata during draft publishes (learningequality#5841)

rtibblesbot · claude · web-flow · commit e36b8213326b · 2026-04-17T14:18:48.000-05:00
* feat: populate draft ChannelVersion metadata during draft publishes fill_published_fields now accepts an optional draft_channel_version parameter. When provided, ChannelVersion-level fields are written to the draft object while channel-level fields (total_resource_count, published_size, published_data, version_info) are left untouched. mark_channel_version_as_distributable is also skipped for draft publishes. publish_channel now calls fill_published_fields in the draft branch, passing the draft ChannelVersion returned by create_draft_channel_version. Closes learningequality#5839 * refactor: simplify draft publish metadata implementation after review * fix: use queryset update to bypass ChannelVersion full_clean validation ChannelVersion.save() always calls full_clean(), which validates choices on ArrayField(IntegerField(choices=...)) for included_licenses. Custom license IDs used in existing tests (e.g. IDs 100, 101) are not in the standard choices list from le_utils, so save() raised ValidationError. Replace version_obj.save() with a queryset .update() to write metadata fields directly without triggering model-level validation. The data originates from the DB so validation is unnecessary, and M2M operations (special_permissions_included) continue to use the model instance. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor: address review feedback on draft publish tests and publish_channel structure - Merge redundant 'if not is_draft_version:' block into the 'else:' branch of the is_draft_version conditional in publish_channel, as requested by reviewer. - Refactor FillPublishedFieldsDraftTestCase into DraftPublishChannelTestCase which tests the complete publish_channel flow (with save_export_database mocked) rather than calling fill_published_fields directly. Tests now use a Special Permissions license node with published=True to exercise the special_permissions_included logic. - test_second_draft_publish_replaces_special_permissions_included now makes two real publish_channel calls with different license_description values and verifies the M2M is replaced (not accumulated) between calls. - test_mark_channel_version_as_distributable_not_called replaced by test_special_permissions_distributable_false_for_draft_publish which asserts the distributable field stays False on the resulting AuditedSpecialPermissionsLicense objects after a draft publish of a public channel, rather than mocking the method. * fix: replace channel.included_languages on publish instead of accumulating Use .set() instead of .add() so languages removed from a channel are cleared on subsequent publishes rather than accumulated indefinitely. Flagged by AlexVelezLl, confirmed as a bug by rtibbles. * fix: use get() instead of create() for Special Permissions license in tests loadconstants inserts licenses with explicit PKs, leaving the PK sequence at 1. Calling License.objects.create() in setUp() then collides with the existing row. Use get() to fetch the pre-existing license — the same pattern used in test_create_channel_versions.py and test_sync.py. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: move delete_public_channel_cache_keys into else branch of is_draft_version The previous commit claimed to have merged all 'if not is_draft_version' blocks into the else branch, but missed the delete_public_channel_cache_keys call. Move it inside the else block and simplify the condition to 'if channel.public'. * fix: revert queryset update to version_obj.save() for ChannelVersion metadata Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
diff --git a/contentcuration/contentcuration/tests/utils/test_publish.py b/contentcuration/contentcuration/tests/utils/test_publish.py
@@ -4,7 +4,9 @@
 from unittest import mock
 
 from django.conf import settings
+from le_utils.constants import licenses
 
+import contentcuration.models as ccmodels
 from contentcuration.tests import testdata
 from contentcuration.tests.base import StudioTestCase
 from contentcuration.tests.utils.restricted_filesystemstorage import (
@@ -13,6 +15,7 @@
 from contentcuration.utils.publish import create_draft_channel_version
 from contentcuration.utils.publish import ensure_versioned_database_exists
 from contentcuration.utils.publish import increment_channel_version
+from contentcuration.utils.publish import publish_channel
 
 
 class EnsureVersionedDatabaseTestCase(StudioTestCase):
@@ -176,3 +179,203 @@ def test_mixed_draft_and_published_versions(self):
         self.assertIsNotNone(self.channel.version_info)
         self.assertEqual(self.channel.version_info.version, 1)
         self.assertIsNone(self.channel.version_info.secret_token)
+
+
+class DraftPublishChannelTestCase(StudioTestCase):
+    """
+    Tests for the draft publish flow using the full publish_channel function.
+
+    Verifies that draft publishes correctly populate the draft ChannelVersion
+    metadata while leaving channel-level fields untouched, and that
+    special_permissions_included is fully replaced (not accumulated) on each
+    draft publish.
+
+    save_export_database is mocked to avoid file-system operations; all other
+    publish_channel logic runs as normal.
+    """
+
+    @classmethod
+    def setUpClass(cls):
+        super().setUpClass()
+        cls._patch_save_export = mock.patch(
+            "contentcuration.utils.publish.save_export_database"
+        )
+        cls._patch_save_export.start()
+
+    @classmethod
+    def tearDownClass(cls):
+        cls._patch_save_export.stop()
+        super().tearDownClass()
+
+    def setUp(self):
+        super().setUp()
+        self.channel = testdata.channel()
+
+        # Fetch the Special Permissions license loaded by loadconstants.
+        # Do NOT call create() — loadconstants inserts licenses with explicit PKs
+        # so the PK sequence is still at 1, and create() would collide with id=1.
+        self.special_perms_license = ccmodels.License.objects.get(
+            license_name=licenses.SPECIAL_PERMISSIONS
+        )
+
+        # Give the channel an existing published version so version_info is set.
+        # increment_channel_version calls channel.save(), which triggers
+        # Channel.on_update() (models.py ~line 1392). on_update() runs
+        # ChannelVersion.objects.get_or_create(channel=self, version=self.version)
+        # and assigns the result to self.version_info — so after this call,
+        # channel.version_info is a real ChannelVersion, not None.
+        increment_channel_version(self.channel)
+        self.channel.refresh_from_db()
+
+        # Snapshot channel state before any draft publish.
+        self.original_total_resource_count = self.channel.total_resource_count
+        self.original_published_size = self.channel.published_size
+        self.original_published_data = dict(self.channel.published_data)
+        self.original_version_info_id = (
+            self.channel.version_info.id if self.channel.version_info else None
+        )
+
+    def _run_draft_publish(self, version_notes=""):
+        publish_channel(
+            self.admin_user.id,
+            self.channel.id,
+            version_notes=version_notes,
+            force=False,
+            force_exercises=False,
+            send_email=False,
+            progress_tracker=None,
+            is_draft_version=True,
+            use_staging_tree=False,
+        )
+
+    def _get_draft_version(self):
+        return ccmodels.ChannelVersion.objects.get(channel=self.channel, version=None)
+
+    # ------------------------------------------------------------------
+    # Test 1: draft ChannelVersion metadata fields are populated
+    # ------------------------------------------------------------------
+
+    def test_draft_channel_version_fields_are_populated(self):
+        """
+        After a draft publish, the draft ChannelVersion has its metadata fields set.
+
+        testdata.channel() has no *published* nodes, so counts are 0 and lists are [].
+        We assert specific values rather than assertIsNotNone to avoid vacuous passes
+        (e.g. assertIsNotNone(0) always passes even if the field was never written).
+        date_published is the only field we can only check for non-None, since its
+        exact value is non-deterministic.
+        """
+        self._run_draft_publish(version_notes="draft notes")
+        draft_version = self._get_draft_version()
+
+        self.assertEqual(draft_version.resource_count, 0)
+        self.assertEqual(draft_version.size, 0)
+        self.assertEqual(draft_version.kind_count, [])
+        self.assertIsNotNone(draft_version.date_published)
+        self.assertEqual(draft_version.version_notes, "draft notes")
+        self.assertEqual(draft_version.included_languages, [])
+        self.assertEqual(draft_version.included_licenses, [])
+        self.assertEqual(draft_version.included_categories, [])
+        self.assertEqual(draft_version.non_distributable_licenses_included, [])
+
+    # ------------------------------------------------------------------
+    # Test 2: channel-level fields are NOT touched
+    # ------------------------------------------------------------------
+
+    def test_channel_fields_not_modified_during_draft_publish(self):
+        """
+        A draft publish must not change channel.total_resource_count,
+        channel.published_size, channel.published_data, or channel.version_info.
+        """
+        self._run_draft_publish()
+        self.channel.refresh_from_db()
+
+        self.assertEqual(
+            self.channel.total_resource_count, self.original_total_resource_count
+        )
+        self.assertEqual(self.channel.published_size, self.original_published_size)
+        self.assertEqual(self.channel.published_data, self.original_published_data)
+
+        current_version_info_id = (
+            self.channel.version_info.id if self.channel.version_info else None
+        )
+        self.assertEqual(current_version_info_id, self.original_version_info_id)
+
+    # ------------------------------------------------------------------
+    # Test 3: second draft publish replaces special_permissions_included
+    # ------------------------------------------------------------------
+
+    def test_second_draft_publish_replaces_special_permissions_included(self):
+        """
+        On a second consecutive draft publish, special_permissions_included on the
+        draft ChannelVersion reflects only the current publish — licenses from the
+        previous draft publish that are no longer present are removed.
+
+        Two publish_channel calls are made with a different license_description on
+        the special-permissions node between them. After the second call the M2M
+        must contain only the description used in that second call.
+        """
+        # Get a video node from the channel's main tree to use as the content node.
+        sp_node = (
+            self.channel.main_tree.get_descendants().filter(kind_id="video").first()
+        )
+
+        # First draft publish: node has Special Permissions with "License A".
+        sp_node.license = self.special_perms_license
+        sp_node.license_description = "License A"
+        sp_node.published = True
+        sp_node.save()
+
+        self._run_draft_publish()
+        draft_version = self._get_draft_version()
+        self.assertEqual(draft_version.special_permissions_included.count(), 1)
+        self.assertEqual(
+            draft_version.special_permissions_included.first().description, "License A"
+        )
+
+        # Second draft publish: node's description changes to "License B".
+        sp_node.license_description = "License B"
+        sp_node.save()
+
+        self._run_draft_publish()
+        draft_version.refresh_from_db()
+        self.assertEqual(
+            draft_version.special_permissions_included.count(),
+            1,
+            "special_permissions_included should be fully replaced on each draft publish",
+        )
+        self.assertEqual(
+            draft_version.special_permissions_included.first().description, "License B"
+        )
+
+    # ------------------------------------------------------------------
+    # Test 4: distributable stays False for draft publishes of public channels
+    # ------------------------------------------------------------------
+
+    def test_special_permissions_distributable_false_for_draft_publish(self):
+        """
+        Even when channel.public is True, a draft publish must not mark
+        AuditedSpecialPermissionsLicense records as distributable — the
+        distributable field must remain False for all licenses linked to the
+        draft ChannelVersion.
+        """
+        sp_node = (
+            self.channel.main_tree.get_descendants().filter(kind_id="video").first()
+        )
+        sp_node.license = self.special_perms_license
+        sp_node.license_description = "Custom License"
+        sp_node.published = True
+        sp_node.save()
+
+        self.channel.public = True
+        self.channel.save()
+
+        self._run_draft_publish()
+        draft_version = self._get_draft_version()
+
+        self.assertGreater(draft_version.special_permissions_included.count(), 0)
+        for license_obj in draft_version.special_permissions_included.all():
+            self.assertFalse(
+                license_obj.distributable,
+                "distributable must stay False for draft publishes",
+            )
diff --git a/contentcuration/contentcuration/utils/publish.py b/contentcuration/contentcuration/utils/publish.py
@@ -915,23 +915,22 @@ def add_tokens_to_channel(channel):
         channel.make_token()
 
 
-def fill_published_fields(channel, version_notes):
-    channel.last_published = timezone.now()
+def fill_published_fields(channel, version_notes, draft_channel_version=None):
+    is_draft = draft_channel_version is not None
+    date_now = timezone.now()
+
     published_nodes = (
         channel.main_tree.get_descendants()
         .filter(published=True)
         .prefetch_related("files")
     )
-    channel.total_resource_count = published_nodes.exclude(
-        kind_id=content_kinds.TOPIC
-    ).count()
+    total_resource_count = published_nodes.exclude(kind_id=content_kinds.TOPIC).count()
     kind_counts = list(
         published_nodes.values("kind_id")
         .annotate(count=Count("kind_id"))
         .order_by("kind_id")
     )
-    channel.published_kind_count = json.dumps(kind_counts)
-    channel.published_size = (
+    published_size = (
         published_nodes.values("files__checksum", "files__file_size")
         .distinct()
         .aggregate(resource_size=Sum("files__file_size"))["resource_size"]
@@ -946,10 +945,6 @@ def fill_published_fields(channel, version_notes):
     )
     language_list = list(set(chain(node_languages, file_languages)))
 
-    for lang in language_list:
-        if lang:
-            channel.included_languages.add(lang)
-
     included_licenses = published_nodes.exclude(license=None).values_list(
         "license", flat=True
     )
@@ -969,24 +964,6 @@ def fill_published_fields(channel, version_notes):
         )
     )
 
-    # TODO: Eventually, consolidate above operations to just use this field for storing historical data
-    channel.published_data.update(
-        {
-            channel.version: {
-                "resource_count": channel.total_resource_count,
-                "kind_count": kind_counts,
-                "size": channel.published_size,
-                "date_published": channel.last_published.strftime(
-                    settings.DATE_TIME_FORMAT
-                ),
-                "version_notes": version_notes,
-                "included_languages": language_list,
-                "included_licenses": license_list,
-                "included_categories": category_list,
-            }
-        }
-    )
-
     # Calculate non-distributable licenses (All Rights Reserved)
     all_rights_reserved_id = (
         ccmodels.License.objects.filter(license_name=licenses.ALL_RIGHTS_RESERVED)
@@ -1030,36 +1007,60 @@ def fill_published_fields(channel, version_notes):
                 new_licenses, ignore_conflicts=True
             )
 
-    if channel.version_info:
-        channel.version_info.resource_count = channel.total_resource_count
-        channel.version_info.kind_count = kind_counts
-        channel.version_info.size = int(channel.published_size)
-        channel.version_info.date_published = channel.last_published
-        channel.version_info.version_notes = version_notes
-        channel.version_info.included_languages = language_list
-        channel.version_info.included_licenses = license_list
-        channel.version_info.included_categories = category_list
-        channel.version_info.non_distributable_licenses_included = (
+    if not is_draft:
+        channel.last_published = date_now
+        channel.total_resource_count = total_resource_count
+        channel.published_kind_count = json.dumps(kind_counts)
+        channel.published_size = published_size
+
+        channel.included_languages.set([lang for lang in language_list if lang])
+
+        # TODO: Eventually, consolidate above operations to just use this field for storing historical data
+        channel.published_data.update(
+            {
+                channel.version: {
+                    "resource_count": total_resource_count,
+                    "kind_count": kind_counts,
+                    "size": published_size,
+                    "date_published": date_now.strftime(settings.DATE_TIME_FORMAT),
+                    "version_notes": version_notes,
+                    "included_languages": language_list,
+                    "included_licenses": license_list,
+                    "included_categories": category_list,
+                }
+            }
+        )
+        channel.save()
+
+    version_obj = draft_channel_version if is_draft else channel.version_info
+    if version_obj is not None:
+        version_obj.resource_count = total_resource_count
+        version_obj.kind_count = kind_counts
+        version_obj.size = int(published_size)
+        version_obj.date_published = date_now
+        version_obj.version_notes = version_notes
+        version_obj.included_languages = language_list
+        version_obj.included_licenses = license_list
+        version_obj.included_categories = category_list
+        version_obj.non_distributable_licenses_included = (
             non_distributable_licenses_included
         )
-        channel.version_info.save()
+        version_obj.save()
 
         if special_perms_descriptions:
-            channel.version_info.special_permissions_included.set(
+            version_obj.special_permissions_included.set(
                 ccmodels.AuditedSpecialPermissionsLicense.objects.filter(
                     description__in=special_perms_descriptions
                 )
             )
         else:
-            channel.version_info.special_permissions_included.clear()
+            version_obj.special_permissions_included.clear()
 
-        if channel.public:
+        if not is_draft and channel.public:
             ccmodels.AuditedSpecialPermissionsLicense.mark_channel_version_as_distributable(
-                channel.version_info.id
+                version_obj.id
             )
 
-    channel.save()
-
 
 def sync_contentnode_and_channel_tsvectors(channel_id):
     """
@@ -1161,10 +1162,12 @@ def publish_channel(  # noqa: C901
         )
         add_tokens_to_channel(channel)
         if is_draft_version:
-            create_draft_channel_version(channel)
+            draft_channel_version = create_draft_channel_version(channel)
+            fill_published_fields(
+                channel, version_notes, draft_channel_version=draft_channel_version
+            )
         else:
             increment_channel_version(channel)
-        if not is_draft_version:
             ccmodels.ChannelVersion.objects.filter(
                 channel=channel, version=None
             ).delete()
@@ -1180,9 +1183,9 @@ def publish_channel(  # noqa: C901
             base_tree.published = True
             base_tree.save()
 
-        # Delete public channel cache.
-        if not is_draft_version and channel.public:
-            delete_public_channel_cache_keys()
+            # Delete public channel cache.
+            if channel.public:
+                delete_public_channel_cache_keys()
 
         if send_email:
             with override(language):
