From a5fd161716d5787bfa8a04f57c7ec8f3c804405d Mon Sep 17 00:00:00 2001 From: taoerman Date: Mon, 26 Jan 2026 12:12:34 -0800 Subject: [PATCH 1/3] Compute license audit task results during publish workflow --- Makefile | 2 +- .../composables/__mocks__/useLicenseAudit.js | 8 +- .../composables/useLicenseAudit.js | 121 ++-------- .../index.vue | 5 +- .../frontend/shared/data/resources.js | 4 - .../backfill_channel_license_audits.py | 96 ++++++++ ...0162_rename_channelversion_audit_fields.py | 22 ++ contentcuration/contentcuration/models.py | 4 +- contentcuration/contentcuration/tasks.py | 4 - .../contentcuration/tests/test_asynctask.py | 226 ------------------ ...backfill_channel_license_audits_command.py | 46 ++++ .../contentcuration/tests/test_models.py | 14 +- .../tests/viewsets/test_channel.py | 140 +---------- .../utils/audit_channel_licenses.py | 102 ++++---- .../contentcuration/utils/publish.py | 10 +- .../contentcuration/viewsets/channel.py | 73 ++---- 16 files changed, 291 insertions(+), 586 deletions(-) create mode 100644 contentcuration/contentcuration/management/commands/backfill_channel_license_audits.py create mode 100644 contentcuration/contentcuration/migrations/0162_rename_channelversion_audit_fields.py create mode 100644 contentcuration/contentcuration/tests/test_backfill_channel_license_audits_command.py diff --git a/Makefile b/Makefile index dc1e70b51e..851d314ffe 100644 --- a/Makefile +++ b/Makefile @@ -38,7 +38,7 @@ migrate: # 4) Remove the management command from this `deploy-migrate` recipe # 5) Repeat! deploy-migrate: - echo "Nothing to do here!" + python contentcuration/manage.py backfill_channel_license_audits contentnodegc: python contentcuration/manage.py garbage_collect diff --git a/contentcuration/contentcuration/frontend/channelEdit/components/sidePanels/SubmitToCommunityLibrarySidePanel/composables/__mocks__/useLicenseAudit.js b/contentcuration/contentcuration/frontend/channelEdit/components/sidePanels/SubmitToCommunityLibrarySidePanel/composables/__mocks__/useLicenseAudit.js index a8d77b7526..2ae36a8c04 100644 --- a/contentcuration/contentcuration/frontend/channelEdit/components/sidePanels/SubmitToCommunityLibrarySidePanel/composables/__mocks__/useLicenseAudit.js +++ b/contentcuration/contentcuration/frontend/channelEdit/components/sidePanels/SubmitToCommunityLibrarySidePanel/composables/__mocks__/useLicenseAudit.js @@ -1,4 +1,4 @@ -import { computed, ref } from 'vue'; +import { computed } from 'vue'; const MOCK_DEFAULTS = { isLoading: computed(() => false), @@ -6,13 +6,7 @@ const MOCK_DEFAULTS = { invalidLicenses: computed(() => []), specialPermissions: computed(() => []), includedLicenses: computed(() => []), - isAuditing: ref(false), hasAuditData: computed(() => false), - auditTaskId: ref(null), - error: ref(null), - checkAndTriggerAudit: jest.fn(), - triggerAudit: jest.fn(), - fetchPublishedData: jest.fn(), }; export function useLicenseAuditMock(overrides = {}) { diff --git a/contentcuration/contentcuration/frontend/channelEdit/components/sidePanels/SubmitToCommunityLibrarySidePanel/composables/useLicenseAudit.js b/contentcuration/contentcuration/frontend/channelEdit/components/sidePanels/SubmitToCommunityLibrarySidePanel/composables/useLicenseAudit.js index d0ce03195e..a2ed9999a1 100644 --- a/contentcuration/contentcuration/frontend/channelEdit/components/sidePanels/SubmitToCommunityLibrarySidePanel/composables/useLicenseAudit.js +++ b/contentcuration/contentcuration/frontend/channelEdit/components/sidePanels/SubmitToCommunityLibrarySidePanel/composables/useLicenseAudit.js @@ -1,128 +1,51 @@ -import { computed, ref, unref, watch } from 'vue'; -import { Channel } from 'shared/data/resources'; - -export function useLicenseAudit(channelRef, channelVersionRef) { - const isAuditing = ref(false); - const auditTaskId = ref(null); - const auditError = ref(null); - const publishedData = ref(null); - - watch( - () => unref(channelRef)?.published_data, - newPublishedData => { - if (newPublishedData) { - publishedData.value = newPublishedData; - if (isAuditing.value) { - isAuditing.value = false; - auditError.value = null; - } - } - }, - { immediate: true, deep: true }, - ); - - const currentVersionData = computed(() => { - const version = unref(channelVersionRef); - if (!publishedData.value || version == null) { - return undefined; - } - return publishedData.value[version]; - }); +import { computed, unref } from 'vue'; +export function useLicenseAudit(versionDetailRef, isLoadingRef, isFinishedRef) { const hasAuditData = computed(() => { - const versionData = currentVersionData.value; + const versionData = unref(versionDetailRef); if (!versionData) { return false; } return ( - 'non_distributable_licenses_included' in versionData && - 'special_permissions_included' in versionData + 'non_distributable_included_licenses' in versionData && + 'included_special_permissions' in versionData ); }); const invalidLicenses = computed(() => { - const versionData = currentVersionData.value; - return versionData?.non_distributable_licenses_included || []; + const versionData = unref(versionDetailRef); + return versionData?.non_distributable_included_licenses || []; }); const specialPermissions = computed(() => { - const versionData = currentVersionData.value; - return versionData?.special_permissions_included || []; + const versionData = unref(versionDetailRef); + return versionData?.included_special_permissions || []; }); const includedLicenses = computed(() => { - const versionData = currentVersionData.value; + const versionData = unref(versionDetailRef); return versionData?.included_licenses || []; }); - const isAuditComplete = computed(() => { - return publishedData.value !== null && hasAuditData.value; - }); - - async function triggerAudit() { - if (isAuditing.value) return; - - try { - isAuditing.value = true; - auditError.value = null; - - const channelId = unref(channelRef)?.id; - if (!channelId) { - throw new Error('Channel ID is required to trigger audit'); - } - - const response = await Channel.auditLicenses(channelId); - auditTaskId.value = response.task_id; - } catch (error) { - isAuditing.value = false; - auditError.value = error; - throw error; - } - } - - async function fetchPublishedData() { - const channelId = unref(channelRef)?.id; - if (!channelId) return; - - try { - const data = await Channel.getPublishedData(channelId); - publishedData.value = data; - } catch (error) { - auditError.value = error; - throw error; - } - } - - async function checkAndTriggerAudit() { - if (!publishedData.value) { - await fetchPublishedData(); - } - - if (hasAuditData.value || isAuditing.value) { - return; - } - - await triggerAudit(); - } - return { isLoading: computed(() => { - if (isAuditComplete.value || auditError.value) return false; - return isAuditing.value; + const loading = unref(isLoadingRef); + if (typeof loading === 'boolean') { + return loading; + } + return !unref(versionDetailRef); + }), + isFinished: computed(() => { + const finished = unref(isFinishedRef); + if (typeof finished === 'boolean') { + return finished; + } + return Boolean(unref(versionDetailRef)); }), - isFinished: computed(() => isAuditComplete.value), - isAuditing, invalidLicenses, specialPermissions, includedLicenses, hasAuditData, - auditTaskId, - error: auditError, - - checkAndTriggerAudit, - triggerAudit, - fetchPublishedData, - currentVersionData, }; } diff --git a/contentcuration/contentcuration/frontend/channelEdit/components/sidePanels/SubmitToCommunityLibrarySidePanel/index.vue b/contentcuration/contentcuration/frontend/channelEdit/components/sidePanels/SubmitToCommunityLibrarySidePanel/index.vue index dcfe5b3e6a..3ff4707c1f 100644 --- a/contentcuration/contentcuration/frontend/channelEdit/components/sidePanels/SubmitToCommunityLibrarySidePanel/index.vue +++ b/contentcuration/contentcuration/frontend/channelEdit/components/sidePanels/SubmitToCommunityLibrarySidePanel/index.vue @@ -439,8 +439,7 @@ isFinished: licenseAuditIsFinished, invalidLicenses, includedLicenses, - checkAndTriggerAudit: checkAndTriggerLicenseAudit, - } = useLicenseAudit(props.channel, currentChannelVersion); + } = useLicenseAudit(versionDetail, publishedDataIsLoading, publishedDataIsFinished); const allSpecialPermissionsChecked = ref(true); @@ -470,7 +469,6 @@ watch(isPublishing, async (newIsPublishing, oldIsPublishing) => { if (oldIsPublishing === true && newIsPublishing === false) { await fetchPublishedData(); - await checkAndTriggerLicenseAudit(); } }); @@ -479,7 +477,6 @@ if (!isPublishing.value) { await fetchPublishedData(); - await checkAndTriggerLicenseAudit(); } }); diff --git a/contentcuration/contentcuration/frontend/shared/data/resources.js b/contentcuration/contentcuration/frontend/shared/data/resources.js index e7a941fbda..a90bfb546f 100644 --- a/contentcuration/contentcuration/frontend/shared/data/resources.js +++ b/contentcuration/contentcuration/frontend/shared/data/resources.js @@ -1416,10 +1416,6 @@ export const Channel = new CreateModelResource({ const response = await client.get(window.Urls.channel_version_detail(id)); return response.data; }, - async auditLicenses(id) { - const response = await client.post(window.Urls.channel_audit_licenses(id)); - return response.data; - }, }); function getChannelFromChannelScope() { diff --git a/contentcuration/contentcuration/management/commands/backfill_channel_license_audits.py b/contentcuration/contentcuration/management/commands/backfill_channel_license_audits.py new file mode 100644 index 0000000000..d9975b1b56 --- /dev/null +++ b/contentcuration/contentcuration/management/commands/backfill_channel_license_audits.py @@ -0,0 +1,96 @@ +from django.core.management.base import BaseCommand + +from contentcuration.models import Channel +from contentcuration.models import ChannelVersion +from contentcuration.utils.audit_channel_licenses import audit_channel_version + + +class Command(BaseCommand): + help = "Backfill license audit results for published channels." + + def add_arguments(self, parser): + parser.add_argument( + "--channel-id", + action="append", + dest="channel_ids", + help="Channel ID to backfill (repeatable).", + ) + parser.add_argument( + "--limit", + type=int, + default=None, + help="Limit number of channels to process.", + ) + parser.add_argument( + "--offset", + type=int, + default=0, + help="Offset into the channel list.", + ) + parser.add_argument( + "--dry-run", + action="store_true", + help="Report what would be updated without saving.", + ) + + def handle(self, *args, **options): + channel_ids = options.get("channel_ids") + limit = options.get("limit") + offset = options.get("offset") or 0 + dry_run = options.get("dry_run") + + queryset = Channel.objects.filter( + main_tree__published=True, + version__gt=0, + ).order_by("id") + + if channel_ids: + queryset = queryset.filter(id__in=channel_ids) + + if offset: + queryset = queryset[offset:] + + if limit: + queryset = queryset[:limit] + + processed = 0 + failures = 0 + + for channel in queryset.iterator(): + processed += 1 + try: + channel_version, _ = ChannelVersion.objects.get_or_create( + channel=channel, + version=channel.version, + ) + if channel.version_info_id != channel_version.id: + if dry_run: + self.stdout.write( + f"Would set version_info for channel {channel.id}" + ) + else: + Channel.objects.filter(pk=channel.pk).update( + version_info=channel_version + ) + + if dry_run: + self.stdout.write( + f"Would backfill audit results for channel {channel.id} " + f"version {channel_version.version}" + ) + continue + + audit_channel_version(channel_version) + self.stdout.write( + f"Backfilled audit results for channel {channel.id} " + f"version {channel_version.version}" + ) + except Exception as error: # noqa: BLE001 + failures += 1 + self.stderr.write( + f"Failed to backfill channel {channel.id}: {error}" + ) + + self.stdout.write( + f"Backfill complete. Processed={processed} Failures={failures}" + ) diff --git a/contentcuration/contentcuration/migrations/0162_rename_channelversion_audit_fields.py b/contentcuration/contentcuration/migrations/0162_rename_channelversion_audit_fields.py new file mode 100644 index 0000000000..aba2cb084f --- /dev/null +++ b/contentcuration/contentcuration/migrations/0162_rename_channelversion_audit_fields.py @@ -0,0 +1,22 @@ + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("contentcuration", "0161_update_channelversion_choices"), + ] + + operations = [ + migrations.RenameField( + model_name="channelversion", + old_name="non_distributable_licenses_included", + new_name="non_distributable_included_licenses", + ), + migrations.RenameField( + model_name="channelversion", + old_name="special_permissions_included", + new_name="included_special_permissions", + ), + ] diff --git a/contentcuration/contentcuration/models.py b/contentcuration/contentcuration/models.py index 6e08ce4206..091d7f4ea0 100644 --- a/contentcuration/contentcuration/models.py +++ b/contentcuration/contentcuration/models.py @@ -1542,12 +1542,12 @@ class ChannelVersion(models.Model): null=True, blank=True, ) - non_distributable_licenses_included = ArrayField( + non_distributable_included_licenses = ArrayField( models.IntegerField(choices=get_license_choices()), null=True, blank=True, ) - special_permissions_included = models.ManyToManyField( + included_special_permissions = models.ManyToManyField( "AuditedSpecialPermissionsLicense", related_name="channel_versions", blank=True, diff --git a/contentcuration/contentcuration/tasks.py b/contentcuration/contentcuration/tasks.py index e584deb4d6..e373ed984a 100644 --- a/contentcuration/contentcuration/tasks.py +++ b/contentcuration/contentcuration/tasks.py @@ -16,7 +16,6 @@ from contentcuration.models import Change from contentcuration.models import ContentNode from contentcuration.models import User -from contentcuration.utils.audit_channel_licenses import audit_channel_licenses from contentcuration.utils.csv_writer import write_user_csv from contentcuration.utils.nodes import calculate_resource_size from contentcuration.utils.nodes import generate_diff @@ -168,6 +167,3 @@ def ensure_versioned_database_exists_task(channel_id, channel_version): ensure_versioned_database_exists(channel_id, channel_version) -@app.task(name="audit-channel-licenses") -def audit_channel_licenses_task(channel_id, user_id): - audit_channel_licenses(channel_id, user_id) diff --git a/contentcuration/contentcuration/tests/test_asynctask.py b/contentcuration/contentcuration/tests/test_asynctask.py index 80d027bd04..a46ffe0635 100644 --- a/contentcuration/contentcuration/tests/test_asynctask.py +++ b/contentcuration/contentcuration/tests/test_asynctask.py @@ -10,14 +10,8 @@ from django.core.management import call_command from django.test import TransactionTestCase from django_celery_results.models import TaskResult -from le_utils.constants import licenses -from mock import patch - from . import testdata -from .base import StudioTestCase from .helpers import clear_tasks -from .helpers import EagerTasksTestMixin -from contentcuration import models as cc from contentcuration.celery import app logger = get_task_logger(__name__) @@ -281,223 +275,3 @@ def test_revoke_task(self): self.fail("Missing revoked task result") -class AuditChannelLicensesTaskTestCase(EagerTasksTestMixin, StudioTestCase): - """Tests for the audit_channel_licenses_task""" - - def setUp(self): - super().setUp() - self.setUpBase() - self.channel.main_tree.published = True - self.channel.main_tree.save() - self.channel.version = 1 - self.channel.save() - - @patch("contentcuration.utils.audit_channel_licenses.KolibriContentNode") - @patch( - "contentcuration.utils.audit_channel_licenses.using_temp_migrated_content_database" - ) - @patch("contentcuration.utils.audit_channel_licenses.storage.exists") - def test_audit_licenses_task__no_invalid_or_special_permissions( - self, mock_storage_exists, mock_using_db, mock_kolibri_node - ): - """Test audit task when channel has no invalid or special permissions licenses""" - from contentcuration.tasks import audit_channel_licenses_task - - license1, _ = cc.License.objects.get_or_create(license_name="CC BY") - license2, _ = cc.License.objects.get_or_create(license_name="CC BY-SA") - cc.License.objects.get_or_create(license_name=licenses.SPECIAL_PERMISSIONS) - node1 = testdata.node({"kind_id": "video", "title": "Video Node"}) - node1.parent = self.channel.main_tree - node1.license = license1 - node1.save() - node1.published = True - node1.save() - - node2 = testdata.node({"kind_id": "video", "title": "Video Node 2"}) - node2.parent = self.channel.main_tree - node2.license = license2 - node2.save() - node2.published = True - node2.save() - - mock_storage_exists.return_value = True - - mock_context = mock.MagicMock() - mock_using_db.return_value.__enter__ = mock.Mock(return_value=mock_context) - mock_using_db.return_value.__exit__ = mock.Mock(return_value=None) - - # Mock KolibriContentNode to return license names from the nodes we created - mock_license_names_distinct = ["CC BY", "CC BY-SA"] - mock_license_names_values_list = mock.Mock() - mock_license_names_values_list.distinct.return_value = ( - mock_license_names_distinct - ) - mock_license_names_exclude3 = mock.Mock() - mock_license_names_exclude3.values_list.return_value = ( - mock_license_names_values_list - ) - mock_license_names_exclude2 = mock.Mock() - mock_license_names_exclude2.exclude.return_value = mock_license_names_exclude3 - mock_license_names_exclude1 = mock.Mock() - mock_license_names_exclude1.exclude.return_value = mock_license_names_exclude2 - - mock_kolibri_node.objects = mock.Mock() - mock_kolibri_node.objects.exclude = mock.Mock( - return_value=mock_license_names_exclude1 - ) - - audit_channel_licenses_task.apply( - kwargs={"channel_id": self.channel.id, "user_id": self.user.id} - ) - - self.channel.refresh_from_db() - version_str = str(self.channel.version) - self.assertIn(version_str, self.channel.published_data) - published_data_version = self.channel.published_data[version_str] - - self.assertIn("included_licenses", published_data_version) - self.assertIsNone( - published_data_version.get("non_distributable_licenses_included") - ) - self.assertIsNone(published_data_version.get("special_permissions_included")) - - @patch("contentcuration.utils.audit_channel_licenses.KolibriContentNode") - @patch( - "contentcuration.utils.audit_channel_licenses.using_temp_migrated_content_database" - ) - @patch("contentcuration.utils.audit_channel_licenses.storage.exists") - def test_audit_licenses_task__with_all_rights_reserved( - self, mock_storage_exists, mock_using_db, mock_kolibri_node - ): - """Test audit task when channel has All Rights Reserved license""" - from contentcuration.tasks import audit_channel_licenses_task - - all_rights_license, _ = cc.License.objects.get_or_create( - license_name=licenses.ALL_RIGHTS_RESERVED - ) - - mock_storage_exists.return_value = True - - mock_context = mock.MagicMock() - mock_using_db.return_value.__enter__ = mock.Mock(return_value=mock_context) - mock_using_db.return_value.__exit__ = mock.Mock(return_value=None) - - mock_license_names_distinct = [licenses.ALL_RIGHTS_RESERVED] - mock_license_names_values_list = mock.Mock() - mock_license_names_values_list.distinct.return_value = ( - mock_license_names_distinct - ) - mock_license_names_exclude3 = mock.Mock() - mock_license_names_exclude3.values_list.return_value = ( - mock_license_names_values_list - ) - mock_license_names_exclude2 = mock.Mock() - mock_license_names_exclude2.exclude.return_value = mock_license_names_exclude3 - mock_license_names_exclude1 = mock.Mock() - mock_license_names_exclude1.exclude.return_value = mock_license_names_exclude2 - mock_license_names_base = mock.Mock() - mock_license_names_base.exclude.return_value = mock_license_names_exclude1 - - mock_kolibri_node.objects = mock.Mock() - mock_kolibri_node.objects.exclude = mock.Mock( - return_value=mock_license_names_exclude1 - ) - - audit_channel_licenses_task.apply( - kwargs={"channel_id": self.channel.id, "user_id": self.user.id} - ) - - self.channel.refresh_from_db() - version_str = str(self.channel.version) - published_data_version = self.channel.published_data[version_str] - - self.assertEqual( - published_data_version.get("non_distributable_licenses_included"), - [all_rights_license.id], - ) - - @patch("contentcuration.utils.audit_channel_licenses.KolibriContentNode") - @patch( - "contentcuration.utils.audit_channel_licenses.using_temp_migrated_content_database" - ) - @patch("contentcuration.utils.audit_channel_licenses.storage.exists") - def test_audit_licenses_task__with_special_permissions( - self, mock_storage_exists, mock_using_db, mock_kolibri_node - ): - """Test audit task when channel has Special Permissions licenses""" - from contentcuration.tasks import audit_channel_licenses_task - - special_perms_license, _ = cc.License.objects.get_or_create( - license_name="Special Permissions" - ) - node = testdata.node({"kind_id": "video", "title": "Video Node"}) - node.parent = self.channel.main_tree - node.license = special_perms_license - node.save() - node.published = True - node.save() - - mock_storage_exists.return_value = True - - mock_context = mock.MagicMock() - mock_using_db.return_value.__enter__ = mock.Mock(return_value=mock_context) - mock_using_db.return_value.__exit__ = mock.Mock(return_value=None) - - mock_license_names_distinct = [licenses.SPECIAL_PERMISSIONS] - mock_license_names_values_list = mock.Mock() - mock_license_names_values_list.distinct.return_value = ( - mock_license_names_distinct - ) - mock_license_names_exclude3 = mock.Mock() - mock_license_names_exclude3.values_list.return_value = ( - mock_license_names_values_list - ) - mock_license_names_exclude2 = mock.Mock() - mock_license_names_exclude2.exclude.return_value = mock_license_names_exclude3 - mock_license_names_exclude1 = mock.Mock() - mock_license_names_exclude1.exclude.return_value = mock_license_names_exclude2 - mock_license_names_base = mock.Mock() - mock_license_names_base.exclude.return_value = mock_license_names_exclude1 - mock_special_perms_distinct = ["Custom permission 1", "Custom permission 2"] - mock_special_perms_values_list = mock.Mock() - mock_special_perms_values_list.distinct.return_value = ( - mock_special_perms_distinct - ) - mock_special_perms_exclude3 = mock.Mock() - mock_special_perms_exclude3.values_list.return_value = ( - mock_special_perms_values_list - ) - mock_special_perms_exclude2 = mock.Mock() - mock_special_perms_exclude2.exclude.return_value = mock_special_perms_exclude3 - mock_special_perms_exclude1 = mock.Mock() - mock_special_perms_exclude1.exclude.return_value = mock_special_perms_exclude2 - mock_special_perms_filter = mock.Mock() - mock_special_perms_filter.exclude.return_value = mock_special_perms_exclude1 - - # Set up the mock to return different querysets based on the method called - mock_kolibri_node.objects = mock.Mock() - mock_kolibri_node.objects.exclude = mock.Mock( - return_value=mock_license_names_exclude1 - ) - mock_kolibri_node.objects.filter = mock.Mock( - return_value=mock_special_perms_filter - ) - - audit_channel_licenses_task.apply( - kwargs={"channel_id": self.channel.id, "user_id": self.user.id} - ) - - self.channel.refresh_from_db() - version_str = str(self.channel.version) - published_data_version = self.channel.published_data[version_str] - - special_perms = published_data_version.get("special_permissions_included") - self.assertIsNotNone(special_perms) - self.assertEqual(len(special_perms), 2) - - from contentcuration.models import AuditedSpecialPermissionsLicense - - audited_licenses = AuditedSpecialPermissionsLicense.objects.filter( - description__in=["Custom permission 1", "Custom permission 2"] - ) - self.assertEqual(audited_licenses.count(), 2) diff --git a/contentcuration/contentcuration/tests/test_backfill_channel_license_audits_command.py b/contentcuration/contentcuration/tests/test_backfill_channel_license_audits_command.py new file mode 100644 index 0000000000..68c2566a48 --- /dev/null +++ b/contentcuration/contentcuration/tests/test_backfill_channel_license_audits_command.py @@ -0,0 +1,46 @@ +from unittest import mock + +from django.core.management import call_command + +from contentcuration.models import Channel +from contentcuration.models import ChannelVersion +from contentcuration.tests import testdata +from contentcuration.tests.base import StudioTestCase + + +class BackfillChannelLicenseAuditsCommandTestCase(StudioTestCase): + def setUp(self): + super().setUp() + self.channel = testdata.channel() + self.channel.version = 1 + self.channel.main_tree.published = True + self.channel.main_tree.save() + self.channel.save() + + Channel.objects.filter(pk=self.channel.pk).update(version_info=None) + + def test_dry_run_does_not_update_version_info(self): + with mock.patch( + "contentcuration.management.commands.backfill_channel_license_audits.audit_channel_version" + ) as mock_audit: + call_command("backfill_channel_license_audits", dry_run=True) + + self.channel.refresh_from_db() + self.assertIsNone(self.channel.version_info) + self.assertTrue( + ChannelVersion.objects.filter( + channel=self.channel, version=self.channel.version + ).exists() + ) + mock_audit.assert_not_called() + + def test_backfill_updates_version_info_and_runs_audit(self): + with mock.patch( + "contentcuration.management.commands.backfill_channel_license_audits.audit_channel_version" + ) as mock_audit: + call_command("backfill_channel_license_audits") + + self.channel.refresh_from_db() + self.assertIsNotNone(self.channel.version_info) + self.assertEqual(self.channel.version_info.version, self.channel.version) + mock_audit.assert_called_once() diff --git a/contentcuration/contentcuration/tests/test_models.py b/contentcuration/contentcuration/tests/test_models.py index f0baf68855..93249ee349 100644 --- a/contentcuration/contentcuration/tests/test_models.py +++ b/contentcuration/contentcuration/tests/test_models.py @@ -1818,8 +1818,8 @@ def test_included_categories_invalid_choice(self): with self.assertRaises(ValidationError): cv.full_clean() - def test_non_distributable_licenses_included_valid_choices(self): - """Test that non_distributable_licenses_included accepts valid license IDs.""" + def test_non_distributable_included_licenses_valid_choices(self): + """Test that non_distributable_included_licenses accepts valid license IDs.""" call_command("loadconstants") valid_license = License.objects.first() @@ -1829,19 +1829,19 @@ def test_non_distributable_licenses_included_valid_choices(self): cv = ChannelVersion( channel=self.channel, version=1, - non_distributable_licenses_included=[valid_license.id], + non_distributable_included_licenses=[valid_license.id], ) cv.full_clean() cv.save() - self.assertEqual(cv.non_distributable_licenses_included, [valid_license.id]) + self.assertEqual(cv.non_distributable_included_licenses, [valid_license.id]) - def test_non_distributable_licenses_included_invalid_choice(self): - """Test that non_distributable_licenses_included rejects invalid license IDs.""" + def test_non_distributable_included_licenses_invalid_choice(self): + """Test that non_distributable_included_licenses rejects invalid license IDs.""" invalid_license_id = 99999 cv = ChannelVersion( channel=self.channel, version=1, - non_distributable_licenses_included=[invalid_license_id], + non_distributable_included_licenses=[invalid_license_id], ) with self.assertRaises(ValidationError): cv.full_clean() diff --git a/contentcuration/contentcuration/tests/viewsets/test_channel.py b/contentcuration/contentcuration/tests/viewsets/test_channel.py index 4b32ccdbe9..d96a20c6a8 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_channel.py +++ b/contentcuration/contentcuration/tests/viewsets/test_channel.py @@ -1351,120 +1351,6 @@ def test_get_version_detail__is_forbidden_user(self): ) self.assertEqual(response.status_code, 404, response.content) - def test_published_data_in_channel_list(self): - """Test that published_data is included in channel list response""" - self.client.force_authenticate(user=self.editor_user) - - response = self.client.get( - reverse("channel-list") + "?edit=true", format="json" - ) - self.assertEqual(response.status_code, 200, response.content) - - response_data = response.json() - channels = ( - response_data - if isinstance(response_data, list) - else response_data["results"] - ) - channel = next((c for c in channels if c["id"] == self.channel.id), None) - self.assertIsNotNone(channel) - self.assertIn("published_data", channel) - self.assertEqual(channel["published_data"], self.channel.published_data) - - -class AuditLicensesActionTestCase(StudioAPITestCase): - def setUp(self): - super().setUp() - - self.editor_user = testdata.user(email="editor@user.com") - self.forbidden_user = testdata.user(email="forbidden@user.com") - self.admin_user = self.admin_user - - self.channel = testdata.channel() - self.channel.editors.add(self.editor_user) - # Mark channel as published - self.channel.main_tree.published = True - self.channel.main_tree.save() - self.channel.version = 1 - self.channel.save() - - def test_audit_licenses__is_editor(self): - """Test that an editor can trigger license audit""" - from contentcuration.tasks import audit_channel_licenses_task - - self.client.force_authenticate(user=self.editor_user) - - with patch.object( - audit_channel_licenses_task, "fetch_or_enqueue" - ) as mock_enqueue: - mock_async_result = Mock() - mock_async_result.task_id = "test-task-id-123" - mock_enqueue.return_value = mock_async_result - - response = self.client.post( - reverse("channel-audit-licenses", kwargs={"pk": self.channel.id}), - format="json", - ) - - self.assertEqual(response.status_code, 200, response.content) - data = response.json() - self.assertIn("task_id", data) - self.assertEqual(data["task_id"], "test-task-id-123") - mock_enqueue.assert_called_once() - - def test_audit_licenses__is_admin(self): - """Test that an admin can trigger license audit""" - from contentcuration.tasks import audit_channel_licenses_task - - self.client.force_authenticate(user=self.admin_user) - - with patch.object( - audit_channel_licenses_task, "fetch_or_enqueue" - ) as mock_enqueue: - mock_async_result = Mock() - mock_async_result.task_id = "test-task-id-456" - mock_enqueue.return_value = mock_async_result - - response = self.client.post( - reverse("channel-audit-licenses", kwargs={"pk": self.channel.id}), - format="json", - ) - - self.assertEqual(response.status_code, 200, response.content) - data = response.json() - self.assertIn("task_id", data) - - def test_audit_licenses__is_forbidden_user(self): - """Test that a non-editor cannot trigger license audit""" - self.client.force_authenticate(user=self.forbidden_user) - - response = self.client.post( - reverse("channel-audit-licenses", kwargs={"pk": self.channel.id}), - format="json", - ) - - self.assertEqual(response.status_code, 404, response.content) - - def test_audit_licenses__channel_not_published(self): - """Test that audit fails when channel is not published""" - self.channel.main_tree.published = False - self.channel.main_tree.save() - - self.client.force_authenticate(user=self.editor_user) - - response = self.client.post( - reverse("channel-audit-licenses", kwargs={"pk": self.channel.id}), - format="json", - ) - - self.assertEqual(response.status_code, 400, response.content) - response_data = response.json() - error_message = ( - response_data["detail"] - if isinstance(response_data, dict) - else response_data[0] - ) - self.assertIn("must be published", str(error_message)) class GetVersionDetailEndpointTestCase(StudioAPITestCase): @@ -1553,17 +1439,18 @@ def test_get_version_detail_returns_all_fields(self): "included_languages", "included_licenses", "included_categories", - "non_distributable_licenses_included", + "non_distributable_included_licenses", + "included_special_permissions", ] for field in expected_fields: self.assertIn(field, data, f"Field '{field}' should be in response") - def test_get_version_detail_excludes_special_permissions_included(self): - """Test that special_permissions_included is not in the response.""" + def test_get_version_detail_includes_special_permissions(self): + """Test that included_special_permissions is returned in the response.""" special_license = AuditedSpecialPermissionsLicense.objects.create( description="Test special permissions license" ) - self.channel_version.special_permissions_included.add(special_license) + self.channel_version.included_special_permissions.add(special_license) url = reverse("channel-version-detail", kwargs={"pk": self.channel.id}) response = self.client.get(url) @@ -1571,15 +1458,11 @@ def test_get_version_detail_excludes_special_permissions_included(self): self.assertEqual(response.status_code, 200) data = response.json() - self.assertNotIn( - "special_permissions_included", - data, - "special_permissions_included should not be in the response. " - "Use /api/audited_special_permissions_license/?channel_version= instead.", - ) + self.assertIn("included_special_permissions", data) + self.assertEqual(data["included_special_permissions"], [special_license.id]) def test_get_version_detail_with_special_permissions_licenses(self): - """Test that get_version_detail works correctly even when special permissions licenses exist.""" + """Test that get_version_detail includes special permissions licenses.""" license1 = AuditedSpecialPermissionsLicense.objects.create( description="First special permissions license" ) @@ -1587,7 +1470,7 @@ def test_get_version_detail_with_special_permissions_licenses(self): description="Second special permissions license" ) - self.channel_version.special_permissions_included.add(license1, license2) + self.channel_version.included_special_permissions.add(license1, license2) url = reverse("channel-version-detail", kwargs={"pk": self.channel.id}) response = self.client.get(url) @@ -1595,6 +1478,9 @@ def test_get_version_detail_with_special_permissions_licenses(self): self.assertEqual(response.status_code, 200) data = response.json() - self.assertNotIn("special_permissions_included", data) + self.assertCountEqual( + data["included_special_permissions"], + [license1.id, license2.id], + ) self.assertEqual(data["version"], 3) self.assertEqual(data["resource_count"], 25) diff --git a/contentcuration/contentcuration/utils/audit_channel_licenses.py b/contentcuration/contentcuration/utils/audit_channel_licenses.py index fa201e8a5f..8bbb1f2a90 100644 --- a/contentcuration/contentcuration/utils/audit_channel_licenses.py +++ b/contentcuration/contentcuration/utils/audit_channel_licenses.py @@ -12,13 +12,11 @@ from le_utils.constants import licenses from contentcuration.models import AuditedSpecialPermissionsLicense -from contentcuration.models import Change from contentcuration.models import Channel +from contentcuration.models import ChannelVersion from contentcuration.models import License from contentcuration.models import User from contentcuration.utils.publish import get_content_db_path -from contentcuration.viewsets.sync.constants import CHANNEL -from contentcuration.viewsets.sync.utils import generate_update_event logger = logging.getLogger(__name__) @@ -50,10 +48,7 @@ def get_channel_and_user(channel_id, user_id): return None, None -def _process_content_database(channel_id, channel_version, published_data_version=None): - included_licenses = None - if published_data_version: - included_licenses = published_data_version.get("included_licenses") +def _process_content_database(channel_id, channel_version, included_licenses=None): db_path = _get_content_database_path(channel_id, channel_version) if not db_path: @@ -143,60 +138,63 @@ def check_invalid_licenses(included_licenses): return invalid_license_ids +def audit_channel_version(channel_version): + """ + Compute audit results for a specific ChannelVersion. + """ + included_licenses, special_permissions_license_ids = _process_content_database( + channel_version.channel_id, + channel_version.version, + included_licenses=channel_version.included_licenses, + ) + + invalid_license_ids = check_invalid_licenses(included_licenses) + + _save_audit_results( + channel_version, + invalid_license_ids, + special_permissions_license_ids, + included_licenses, + ) + + def _save_audit_results( - channel, - published_data_version, + channel_version, invalid_license_ids, special_permissions_license_ids, - user_id, + included_licenses, ): - """Save audit results to published_data and create change event.""" - published_data_version["non_distributable_licenses_included"] = ( + """Save audit results to ChannelVersion.""" + channel_version.included_licenses = included_licenses + channel_version.non_distributable_included_licenses = ( invalid_license_ids if invalid_license_ids else None ) - published_data_version["special_permissions_included"] = ( - special_permissions_license_ids if special_permissions_license_ids else None - ) - - channel.save() - - Change.create_change( - generate_update_event( - channel.id, - CHANNEL, - {"published_data": channel.published_data}, - channel_id=channel.id, - ), - applied=True, - created_by_id=user_id, - ) + channel_version.save() + if special_permissions_license_ids: + channel_version.included_special_permissions.set( + AuditedSpecialPermissionsLicense.objects.filter( + id__in=special_permissions_license_ids + ) + ) + else: + channel_version.included_special_permissions.clear() -def audit_channel_licenses(channel_id, user_id): - user, channel = get_channel_and_user(channel_id, user_id) - if not user or not channel: - return - - channel_version = channel.version - version_str = str(channel_version) - - if version_str not in channel.published_data: - channel.published_data[version_str] = {} - - published_data_version = channel.published_data[version_str] - - included_licenses, special_permissions_license_ids = _process_content_database( - channel_id, channel_version, published_data_version=published_data_version - ) - published_data_version["included_licenses"] = included_licenses +def audit_channel_licenses(channel_id, user_id=None): + if user_id: + user, channel = get_channel_and_user(channel_id, user_id) + if not user or not channel: + return + else: + channel = Channel.objects.select_related("main_tree").get(pk=channel_id) + if not channel: + return - invalid_license_ids = check_invalid_licenses(included_licenses) + if not channel.version: + return - _save_audit_results( - channel, - published_data_version, - invalid_license_ids, - special_permissions_license_ids, - user_id, + channel_version_obj, _ = ChannelVersion.objects.get_or_create( + channel=channel, version=channel.version ) + audit_channel_version(channel_version_obj) diff --git a/contentcuration/contentcuration/utils/publish.py b/contentcuration/contentcuration/utils/publish.py index 701787aa3d..fc4c544f10 100644 --- a/contentcuration/contentcuration/utils/publish.py +++ b/contentcuration/contentcuration/utils/publish.py @@ -970,7 +970,7 @@ def fill_published_fields(channel, version_notes): .first() ) - non_distributable_licenses = ( + non_distributable_included_licenses = ( [all_rights_reserved_id] if all_rights_reserved_id and all_rights_reserved_id in license_list else [] @@ -1015,19 +1015,19 @@ def fill_published_fields(channel, 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 = ( - non_distributable_licenses + channel.version_info.non_distributable_included_licenses = ( + non_distributable_included_licenses ) channel.version_info.save() if special_perms_descriptions: - channel.version_info.special_permissions_included.set( + channel.version_info.included_special_permissions.set( ccmodels.AuditedSpecialPermissionsLicense.objects.filter( description__in=special_perms_descriptions ) ) else: - channel.version_info.special_permissions_included.clear() + channel.version_info.included_special_permissions.clear() channel.save() diff --git a/contentcuration/contentcuration/viewsets/channel.py b/contentcuration/contentcuration/viewsets/channel.py index a10a3eb04c..7c6955e414 100644 --- a/contentcuration/contentcuration/viewsets/channel.py +++ b/contentcuration/contentcuration/viewsets/channel.py @@ -57,7 +57,6 @@ from contentcuration.models import generate_storage_url from contentcuration.models import SecretToken from contentcuration.models import User -from contentcuration.tasks import audit_channel_licenses_task from contentcuration.utils.garbage_collect import get_deleted_chefs_root from contentcuration.utils.pagination import CachedListPagination from contentcuration.utils.pagination import ValuesViewsetPageNumberPagination @@ -396,7 +395,6 @@ def format_demo_server_url(item): "staging_tree__id", "source_url", "demo_server_url", - "published_data", ) channel_field_map = { @@ -902,56 +900,35 @@ def get_version_detail(self, request, pk=None) -> Response: if not channel.version_info: return Response({}) - version_data = ( - ChannelVersion.objects.filter(id=channel.version_info.id) - .values( - "id", - "version", - "resource_count", - "kind_count", - "size", - "date_published", - "version_notes", - "included_languages", - "included_licenses", - "included_categories", - "non_distributable_licenses_included", - ) - .first() - ) + channel_version = ChannelVersion.objects.filter( + id=channel.version_info.id + ).first() - if not version_data: + if not channel_version: return Response({}) - return Response(version_data) - - @action( - detail=True, - methods=["post"], - url_path="audit_licenses", - url_name="audit-licenses", - ) - def audit_licenses(self, request, pk=None) -> Response: - """ - Trigger license audit for a channel's community library submission. - This will check for invalid licenses (All Rights Reserved) and special - permissions licenses, and update the channel's published_data with audit results. - - :param request: The request object - :param pk: The ID of the channel - :return: Response with task_id if task was enqueued - :rtype: Response - """ - channel = self.get_edit_object() - - if not channel.main_tree.published: - raise ValidationError("Channel must be published to audit licenses") - - async_result = audit_channel_licenses_task.fetch_or_enqueue( - request.user, channel_id=channel.id, user_id=request.user.id - ) + version_data = { + "id": channel_version.id, + "version": channel_version.version, + "resource_count": channel_version.resource_count, + "kind_count": channel_version.kind_count, + "size": channel_version.size, + "date_published": channel_version.date_published, + "version_notes": channel_version.version_notes, + "included_languages": channel_version.included_languages, + "included_licenses": channel_version.included_licenses, + "included_categories": channel_version.included_categories, + "non_distributable_included_licenses": ( + channel_version.non_distributable_included_licenses + ), + "included_special_permissions": list( + channel_version.included_special_permissions.values_list( + "id", flat=True + ) + ), + } - return Response({"task_id": async_result.task_id}) + return Response(version_data) @action( detail=True, From 7fc7deb15e2d9adcdb5bfc1af89493f64822d6f3 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci-lite[bot]" <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com> Date: Mon, 26 Jan 2026 20:17:49 +0000 Subject: [PATCH 2/3] [pre-commit.ci lite] apply automatic fixes --- .../management/commands/backfill_channel_license_audits.py | 4 +--- .../migrations/0162_rename_channelversion_audit_fields.py | 1 - contentcuration/contentcuration/tasks.py | 2 -- contentcuration/contentcuration/tests/test_asynctask.py | 3 +-- .../contentcuration/tests/viewsets/test_channel.py | 1 - 5 files changed, 2 insertions(+), 9 deletions(-) diff --git a/contentcuration/contentcuration/management/commands/backfill_channel_license_audits.py b/contentcuration/contentcuration/management/commands/backfill_channel_license_audits.py index d9975b1b56..f1a0e23cdb 100644 --- a/contentcuration/contentcuration/management/commands/backfill_channel_license_audits.py +++ b/contentcuration/contentcuration/management/commands/backfill_channel_license_audits.py @@ -87,9 +87,7 @@ def handle(self, *args, **options): ) except Exception as error: # noqa: BLE001 failures += 1 - self.stderr.write( - f"Failed to backfill channel {channel.id}: {error}" - ) + self.stderr.write(f"Failed to backfill channel {channel.id}: {error}") self.stdout.write( f"Backfill complete. Processed={processed} Failures={failures}" diff --git a/contentcuration/contentcuration/migrations/0162_rename_channelversion_audit_fields.py b/contentcuration/contentcuration/migrations/0162_rename_channelversion_audit_fields.py index aba2cb084f..5623a25df6 100644 --- a/contentcuration/contentcuration/migrations/0162_rename_channelversion_audit_fields.py +++ b/contentcuration/contentcuration/migrations/0162_rename_channelversion_audit_fields.py @@ -1,4 +1,3 @@ - from django.db import migrations diff --git a/contentcuration/contentcuration/tasks.py b/contentcuration/contentcuration/tasks.py index e373ed984a..cc4eb9c156 100644 --- a/contentcuration/contentcuration/tasks.py +++ b/contentcuration/contentcuration/tasks.py @@ -165,5 +165,3 @@ def sendcustomemails_task(subject, message, query): @app.task(name="ensure_versioned_database_exists_task") def ensure_versioned_database_exists_task(channel_id, channel_version): ensure_versioned_database_exists(channel_id, channel_version) - - diff --git a/contentcuration/contentcuration/tests/test_asynctask.py b/contentcuration/contentcuration/tests/test_asynctask.py index a46ffe0635..c3b92112f9 100644 --- a/contentcuration/contentcuration/tests/test_asynctask.py +++ b/contentcuration/contentcuration/tests/test_asynctask.py @@ -10,6 +10,7 @@ from django.core.management import call_command from django.test import TransactionTestCase from django_celery_results.models import TaskResult + from . import testdata from .helpers import clear_tasks from contentcuration.celery import app @@ -273,5 +274,3 @@ def test_revoke_task(self): TaskResult.objects.get(task_id=async_result.task_id, status=states.REVOKED) except TaskResult.DoesNotExist: self.fail("Missing revoked task result") - - diff --git a/contentcuration/contentcuration/tests/viewsets/test_channel.py b/contentcuration/contentcuration/tests/viewsets/test_channel.py index d96a20c6a8..7f099776bb 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_channel.py +++ b/contentcuration/contentcuration/tests/viewsets/test_channel.py @@ -1352,7 +1352,6 @@ def test_get_version_detail__is_forbidden_user(self): self.assertEqual(response.status_code, 404, response.content) - class GetVersionDetailEndpointTestCase(StudioAPITestCase): """Test get_version_detail API endpoint.""" From b007436ef3771aec6af0f5a5327e1ca2a982ab26 Mon Sep 17 00:00:00 2001 From: taoerman Date: Mon, 26 Jan 2026 12:49:53 -0800 Subject: [PATCH 3/3] fix linting --- contentcuration/contentcuration/tests/test_asynctask.py | 1 - contentcuration/contentcuration/tests/viewsets/test_channel.py | 1 - 2 files changed, 2 deletions(-) diff --git a/contentcuration/contentcuration/tests/test_asynctask.py b/contentcuration/contentcuration/tests/test_asynctask.py index c3b92112f9..79b239099b 100644 --- a/contentcuration/contentcuration/tests/test_asynctask.py +++ b/contentcuration/contentcuration/tests/test_asynctask.py @@ -2,7 +2,6 @@ import time import uuid -import mock import pytest from celery import states from celery.result import allow_join_result diff --git a/contentcuration/contentcuration/tests/viewsets/test_channel.py b/contentcuration/contentcuration/tests/viewsets/test_channel.py index 7f099776bb..d95fc5d69a 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_channel.py +++ b/contentcuration/contentcuration/tests/viewsets/test_channel.py @@ -7,7 +7,6 @@ from kolibri_public.models import ContentNode as PublicContentNode from le_utils.constants import content_kinds from le_utils.constants.labels import subjects -from mock import Mock from mock import patch from contentcuration import models