fix: populate draft ChannelVersion metadata during draft publishes by rtibblesbot · Pull Request #5841 · learningequality/studio

rtibblesbot · 2026-04-16T19:52:47Z

Summary

fill_published_fields was never called during draft publishes, leaving draft ChannelVersion objects (those with version=None) without computed metadata. This adds a draft_channel_version parameter to fill_published_fields: when provided, ChannelVersion-level fields (resource_count, size, kind_count, included_languages, etc.) are written to the draft object while all channel-level fields (total_resource_count, published_size, published_data) and mark_channel_version_as_distributable are skipped.

publish_channel now passes the draft ChannelVersion returned by create_draft_channel_version to fill_published_fields.

Also fixes a pre-existing bug where channel.included_languages was accumulated (.add()) across publishes instead of replaced; it now uses .set() so removed languages are cleared.

References

Closes #5839

Reviewer guidance

The key area to scrutinise is the if not is_draft guard in fill_published_fields (contentcuration/utils/publish.py) — any regression here would corrupt channel.published_data or channel.included_languages for live (non-draft) publishes. The existing test suite exercises the non-draft path; four new tests in DraftPublishChannelTestCase cover the draft path.

pytest contentcuration/contentcuration/tests/utils/test_publish.py -v

AI usage

Implemented with Claude Code following a pre-written plan. Generated code reviewed against all acceptance criteria; unnecessary comments removed in a simplify pass. Full test suite run to confirm no regressions.

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Ran pre-flight CI checks (lint, format, tests) and verified all pass
Rebased onto the target branch and resolved any conflicts
Reorganized commit history into clean, logical commits
Audited the diff to ensure only issue-relevant files are changed
Built PR body from the repository's PR template with evidence blocks

AlexVelezLl

Code changes look good overall, just a request to improve the tests, and a couple of notes to fix things from the previous implementation.

AlexVelezLl · 2026-04-16T21:28:33Z

+        with mock.patch.object(
+            ccmodels.AuditedSpecialPermissionsLicense,
+            "mark_channel_version_as_distributable",
+        ) as mock_mark:
+            fill_published_fields(
+                self.channel, "draft notes", draft_channel_version=self.draft_version
+            )
+
+        mock_mark.assert_not_called()


Could you set some nodes with special permissions and then actually see that the distributable field is false?

Done. Replaced the mock-based test with test_special_permissions_distributable_false_for_draft_publish which sets a video node to the Special Permissions license (with a description), calls publish_channel with is_draft_version=True and channel.public=True, then asserts that all AuditedSpecialPermissionsLicense objects in the draft ChannelVersion's special_permissions_included have distributable=False.

AlexVelezLl · 2026-04-16T21:31:15Z

        self.assertIsNone(self.channel.version_info.secret_token)
+
+
+class FillPublishedFieldsDraftTestCase(StudioTestCase):


I think it'd be nice if we had tests for the entire publish flow instead, to make sure that the publish function is working correctly, and that it is actually sending the draft channel_version object correctly.

Refactored into DraftPublishChannelTestCase which calls publish_channel(is_draft_version=True) with save_export_database mocked (same pattern as PublishDraftUsingMainTreeTestCase). Tests now exercise the full draft publish flow.

AlexVelezLl · 2026-04-16T21:32:29Z

+    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.
+
+        We simulate 'previous' licenses by pre-loading the M2M, then running a
+        fresh fill_published_fields (which finds no special-permissions nodes in
+        testdata.channel(), so it calls .clear()), and assert the M2M is empty.
+        """
+        stale_license = ccmodels.AuditedSpecialPermissionsLicense.objects.create(
+            description="Stale license from previous draft publish",
+            distributable=False,
+        )
+        self.draft_version.special_permissions_included.add(stale_license)
+        self.assertEqual(self.draft_version.special_permissions_included.count(), 1)
+
+        # Second draft publish: testdata.channel() has no special-perms nodes,
+        # so fill_published_fields should clear the M2M entirely.
+        fill_published_fields(
+            self.channel, "second draft", draft_channel_version=self.draft_version
+        )
+        self.draft_version.refresh_from_db()
+
+        self.assertEqual(
+            self.draft_version.special_permissions_included.count(),
+            0,
+            "special_permissions_included should be fully replaced on each draft publish",
+        )


After changing these tests to use the actual publish function, it'd be nice if we can do this test with two different draft publish calls with different special permissions.

Done. test_second_draft_publish_replaces_special_permissions_included now makes two real publish_channel calls: first with license_description='License A', then with 'License B'. Asserts after each call that special_permissions_included contains exactly the current description.

AlexVelezLl · 2026-04-16T21:33:40Z

+            )
        else:
            increment_channel_version(channel)
        if not is_draft_version:


Now that you are here, could you please just put all content below this if not is_draft_version block in the else block, so that it is more readable?

Done. Merged the content of the standalone 'if not is_draft_version:' block into the else branch of the is_draft_version conditional, removing the redundant negated check.

Correction to the previous reply: the prior commit moved the inner if not is_draft_version: block into the else branch, but missed the delete_public_channel_cache_keys call which was also gated by if not is_draft_version. That is now fixed — moved into the else branch as if channel.public: in 6683776.

AlexVelezLl · 2026-04-16T21:35:51Z

+        for lang in language_list:
+            if lang:
+                channel.included_languages.add(lang)


I know this was copied from the old implementation, but this is a bug, right? @rtibbles. If a language doesn't exist anymore on the channel, it should be removed, right? Or should this included_languages have the list of historical languages on the channel?

Yes - if it's not being cleared earlier on, this is a mistake!

This behaviour () is copied from the original implementation and is pre-existing — it's not something introduced by this PR. Since the question is tagged at @rtibbles, I've left this for them to decide on the right semantics. Happy to address it in a follow-up if they confirm it should be a replace rather than an accumulate.

Fixed in a009cb5 — replaced the loop with so languages no longer present in the channel are removed on each publish rather than accumulated.

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

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>

…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.

…ating 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.

… 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>

…ft_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'.

AlexVelezLl

~~Code changes look good to me, and manual QA checks out. Thanks @rtibblesbot. cc: @rtibbles~~ Just one last thing.

AlexVelezLl · 2026-04-17T18:49:50Z

+        ccmodels.ChannelVersion.objects.filter(pk=version_obj.pk).update(
+            resource_count=total_resource_count,
+            kind_count=kind_counts,
+            size=int(published_size),
+            date_published=date_now,
+            version_notes=version_notes,
+            included_languages=language_list,
+            included_licenses=license_list,
+            included_categories=category_list,
+            non_distributable_licenses_included=non_distributable_licenses_included,


Nvm in my way to merge this PR, I noted the reason behind the change to use update, could you use the previous .save, please? And if there are any issues with the tests, the tests should be fixed instead.

Done — reverted to version_obj.save(). The current tests use only le_utils license IDs (1–9) and standard language codes, so full_clean() passes cleanly.

Thanks, waiting for tests to pass, and then will merge.

…metadata Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

AlexVelezLl

lgtm!

rtibblesbot marked this pull request as ready for review April 16, 2026 20:12

AlexVelezLl requested changes Apr 16, 2026

View reviewed changes

rtibblesbot and others added 5 commits April 16, 2026 15:24

refactor: simplify draft publish metadata implementation after review

656b4a4

fix: replace channel.included_languages on publish instead of accumul…

067248e

…ating 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.

rtibblesbot force-pushed the issue-5839-7033eb branch from a009cb5 to 067248e Compare April 16, 2026 22:24

rtibblesbot and others added 2 commits April 16, 2026 15:45

AlexVelezLl approved these changes Apr 17, 2026

View reviewed changes

AlexVelezLl requested changes Apr 17, 2026

View reviewed changes

fix: revert queryset update to version_obj.save() for ChannelVersion …

3ab0158

…metadata Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

AlexVelezLl approved these changes Apr 17, 2026

View reviewed changes

AlexVelezLl merged commit e36b821 into learningequality:unstable Apr 17, 2026
13 checks passed

		self.assertIsNone(self.channel.version_info.secret_token)


		class FillPublishedFieldsDraftTestCase(StudioTestCase):

Uh oh!

Conversation

rtibblesbot commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

References

Reviewer guidance

AI usage

Uh oh!

AlexVelezLl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexVelezLl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexVelezLl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rtibblesbot commented Apr 16, 2026 •

edited

Loading

AlexVelezLl left a comment •

edited

Loading