From 0043f6d0dd29e27d2e9f66b3a3a9735a07cd87ee Mon Sep 17 00:00:00 2001 From: Jakub Komarek Date: Wed, 30 Jul 2025 13:59:29 +0200 Subject: [PATCH 01/11] Add to community library on submission resolve --- .../tests/viewsets/test_channel.py | 87 ++++++++++ .../test_community_library_submission.py | 31 +++- .../contentcuration/viewsets/channel.py | 46 ++++++ .../viewsets/community_library_submission.py | 25 +++ .../contentcuration/viewsets/sync/base.py | 2 + .../viewsets/sync/constants.py | 8 + .../contentcuration/viewsets/sync/endpoint.py | 7 +- .../contentcuration/viewsets/sync/utils.py | 20 +++ .../export_channels_to_kolibri_public.py | 38 +---- .../migrations/0007_new_channel_metadata.py | 26 +++ contentcuration/kolibri_public/models.py | 3 + .../test_export_channels_to_kolibri_public.py | 152 +++++++++++++++++ .../kolibri_public/tests/test_mapper.py | 154 +++++++++++++++--- .../kolibri_public/utils/annotation.py | 42 ++++- .../utils/export_channel_to_kolibri_public.py | 60 +++++++ .../kolibri_public/utils/mapper.py | 26 ++- 16 files changed, 663 insertions(+), 64 deletions(-) create mode 100644 contentcuration/kolibri_public/migrations/0007_new_channel_metadata.py create mode 100644 contentcuration/kolibri_public/tests/test_export_channels_to_kolibri_public.py create mode 100644 contentcuration/kolibri_public/utils/export_channel_to_kolibri_public.py diff --git a/contentcuration/contentcuration/tests/viewsets/test_channel.py b/contentcuration/contentcuration/tests/viewsets/test_channel.py index 067e442ede..9ea043966c 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_channel.py +++ b/contentcuration/contentcuration/tests/viewsets/test_channel.py @@ -11,7 +11,10 @@ from contentcuration import models from contentcuration import models as cc from contentcuration.constants import channel_history +from contentcuration.models import Change from contentcuration.models import ContentNode +from contentcuration.models import Country +from contentcuration.tasks import apply_channel_changes_task from contentcuration.tests import testdata from contentcuration.tests.base import StudioAPITestCase from contentcuration.tests.viewsets.base import generate_create_event @@ -24,6 +27,9 @@ from contentcuration.tests.viewsets.base import SyncTestMixin from contentcuration.viewsets.channel import _unpublished_changes_query from contentcuration.viewsets.sync.constants import CHANNEL +from contentcuration.viewsets.sync.utils import ( + generate_added_to_community_library_event, +) class SyncTestCase(SyncTestMixin, StudioAPITestCase): @@ -517,6 +523,87 @@ def test_publish_next_with_incomplete_staging_tree(self): modified_channel = models.Channel.objects.get(id=channel.id) self.assertEqual(modified_channel.staging_tree.published, False) + def test_sync_added_to_community_library_change(self): + # Syncing the change from the frontend should be disallowed + self.client.force_authenticate(self.admin_user) + + channel = testdata.channel() + channel.version = 1 + channel.public = True + channel.save() + + added_to_community_library_change = generate_added_to_community_library_event( + key=channel.id, + channel_version=1, + ) + response = self.sync_changes([added_to_community_library_change]) + + self.assertEqual(response.status_code, 200, response.content) + self.assertEqual(len(response.json()["allowed"]), 0, response.content) + self.assertEqual(len(response.json()["disallowed"]), 1, response.content) + + @mock.patch("contentcuration.viewsets.channel.export_channel_to_kolibri_public") + def test_process_added_to_community_library_change(self, mock_export_func): + # Creating the change on the backend should be supported + self.client.force_authenticate(self.admin_user) + + channel = testdata.channel() + channel.version = 1 + channel.public = True + channel.save() + + categories = { + "category1": True, + "category2": True, + } + + country1 = Country.objects.create(code="C1", name="Country 1") + country2 = Country.objects.create(code="C2", name="Country 2") + countries = [country1, country2] + country_codes = [country.code for country in countries] + + added_to_community_library_change = generate_added_to_community_library_event( + key=channel.id, + channel_version=1, + categories=categories, + country_codes=country_codes, + ) + Change.create_change( + added_to_community_library_change, created_by_id=self.admin_user.id + ) + + # This task will run immediately thanks to SyncTestMixin + apply_channel_changes_task.fetch_or_enqueue( + user=self.admin_user, + channel_id=channel.id, + ) + + # We cannot easily use the assert_called_once_with method here + # because we are not checking countries for strict equality, + # so we need to check the call arguments manually + mock_export_func.assert_called_once() + + (call_args, call_kwargs) = mock_export_func.call_args + self.assertEqual(len(call_args), 0) + self.assertCountEqual( + call_kwargs.keys(), + [ + "channel_id", + "channel_version", + "categories", + "countries", + "public", + ], + ) + self.assertEqual(call_kwargs["channel_id"], channel.id) + self.assertEqual(call_kwargs["channel_version"], 1) + self.assertEqual(call_kwargs["categories"], categories) + + # The countries argument used when creating the mapper is in fact + # not a list, but a QuerySet, but it contains the same elements + self.assertCountEqual(call_kwargs["countries"], countries) + self.assertEqual(call_kwargs["public"], False) + class CRUDTestCase(StudioAPITestCase): @property diff --git a/contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py b/contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py index 4d2808658f..f0fff43647 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py +++ b/contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py @@ -8,9 +8,11 @@ from contentcuration.constants import ( community_library_submission as community_library_submission_constants, ) +from contentcuration.models import Change from contentcuration.models import CommunityLibrarySubmission from contentcuration.tests import testdata from contentcuration.tests.base import StudioAPITestCase +from contentcuration.viewsets.sync.constants import ADDED_TO_COMMUNITY_LIBRARY def reverse_with_query( @@ -628,7 +630,10 @@ def test_resolve_submission__editor(self): ) self.assertEqual(response.status_code, 403, response.content) - def test_resolve_submission__accept_correct(self): + @mock.patch( + "contentcuration.viewsets.community_library_submission.apply_channel_changes_task" + ) + def test_resolve_submission__accept_correct(self, apply_task_mock): self.client.force_authenticate(user=self.admin_user) response = self.client.post( reverse( @@ -652,7 +657,21 @@ def test_resolve_submission__accept_correct(self): self.assertEqual(resolved_submission.resolved_by, self.admin_user) self.assertEqual(resolved_submission.date_resolved, self.resolved_time) - def test_resolve_submission__reject_correct(self): + self.assertTrue( + Change.objects.filter( + channel=self.submission.channel, + change_type=ADDED_TO_COMMUNITY_LIBRARY, + ).exists() + ) + apply_task_mock.fetch_or_enqueue.assert_called_once_with( + self.admin_user, + channel_id=self.submission.channel.id, + ) + + @mock.patch( + "contentcuration.viewsets.community_library_submission.apply_channel_changes_task" + ) + def test_resolve_submission__reject_correct(self, apply_task_mock): self.client.force_authenticate(user=self.admin_user) response = self.client.post( reverse( @@ -680,6 +699,14 @@ def test_resolve_submission__reject_correct(self): self.assertEqual(resolved_submission.resolved_by, self.admin_user) self.assertEqual(resolved_submission.date_resolved, self.resolved_time) + self.assertFalse( + Change.objects.filter( + channel=self.submission.channel, + change_type=ADDED_TO_COMMUNITY_LIBRARY, + ).exists() + ) + apply_task_mock.fetch_or_enqueue.assert_not_called() + def test_resolve_submission__reject_missing_resolution_reason(self): self.client.force_authenticate(user=self.admin_user) metadata = self.resolve_reject_metadata.copy() diff --git a/contentcuration/contentcuration/viewsets/channel.py b/contentcuration/contentcuration/viewsets/channel.py index 1fd6625030..5f8a33ade9 100644 --- a/contentcuration/contentcuration/viewsets/channel.py +++ b/contentcuration/contentcuration/viewsets/channel.py @@ -21,6 +21,9 @@ from django_cte import With from django_filters.rest_framework import BooleanFilter from django_filters.rest_framework import CharFilter +from kolibri_public.utils.export_channel_to_kolibri_public import ( + export_channel_to_kolibri_public, +) from le_utils.constants import content_kinds from le_utils.constants import roles from rest_framework import serializers @@ -43,6 +46,7 @@ from contentcuration.models import Change from contentcuration.models import Channel from contentcuration.models import ContentNode +from contentcuration.models import Country from contentcuration.models import File from contentcuration.models import generate_storage_url from contentcuration.models import SecretToken @@ -768,6 +772,48 @@ def deploy(self, user, pk): created_by_id=user.id, ) + def add_to_community_library_from_changes(self, changes): + errors = [] + for change in changes: + try: + self.add_to_community_library( + channel_id=change["channel_id"], + channel_version=change["channel_version"], + categories=change["categories"], + country_codes=change["country_codes"], + ) + except Exception as e: + log_sync_exception(e, user=self.request.user, change=change) + change["errors"] = [str(e)] + errors.append(change) + return errors + + def add_to_community_library( + self, channel_id, channel_version, categories, country_codes + ): + # The change to add a channel to the community library can only + # be created server-side, so in theory we should not be getting + # malformed requests here. However, just to be safe, we still + # do basic checks. + + channel = self.get_edit_queryset().get(pk=channel_id) + countries = Country.objects.filter(code__in=country_codes) + + if not channel.public: + raise ValidationError( + "Only public channels can be added to the community library" + ) + if channel_version <= 0 or channel_version > channel.version: + raise ValidationError("Invalid channel version") + + export_channel_to_kolibri_public( + channel_id=channel_id, + channel_version=channel_version, + public=False, # Community library + categories=categories, + countries=countries, + ) + @action( detail=True, methods=["get"], diff --git a/contentcuration/contentcuration/viewsets/community_library_submission.py b/contentcuration/contentcuration/viewsets/community_library_submission.py index 163ab50e77..d99786fd67 100644 --- a/contentcuration/contentcuration/viewsets/community_library_submission.py +++ b/contentcuration/contentcuration/viewsets/community_library_submission.py @@ -9,9 +9,11 @@ from contentcuration.constants import ( community_library_submission as community_library_submission_constants, ) +from contentcuration.models import Change from contentcuration.models import Channel from contentcuration.models import CommunityLibrarySubmission from contentcuration.models import Country +from contentcuration.tasks import apply_channel_changes_task from contentcuration.utils.pagination import ValuesViewsetCursorPagination from contentcuration.viewsets.base import BulkListSerializer from contentcuration.viewsets.base import BulkModelSerializer @@ -20,6 +22,9 @@ from contentcuration.viewsets.base import RESTDestroyModelMixin from contentcuration.viewsets.base import RESTUpdateModelMixin from contentcuration.viewsets.common import UserFilteredPrimaryKeyRelatedField +from contentcuration.viewsets.sync.utils import ( + generate_added_to_community_library_event, +) from contentcuration.viewsets.user import IsAdminUser @@ -232,10 +237,29 @@ def _mark_previous_pending_submissions_as_superseded(self, submission): channel_version__lt=submission.channel_version, ).update(status=community_library_submission_constants.STATUS_SUPERSEDED) + def _add_to_community_library(self, submission): + country_codes = sorted(country.code for country in submission.countries.all()) + + Change.create_change( + generate_added_to_community_library_event( + key=submission.channel.id, + channel_version=submission.channel_version, + categories=submission.categories, + country_codes=country_codes, + ), + created_by_id=submission.resolved_by_id, + ) + apply_channel_changes_task.fetch_or_enqueue( + submission.resolved_by, + channel_id=submission.channel.id, + ) + @action( methods=["post"], detail=True, serializer_class=CommunityLibrarySubmissionResolveSerializer, + url_name="resolve", + url_path="resolve", ) def resolve(self, request, pk=None): instance = self.get_edit_object() @@ -250,5 +274,6 @@ def resolve(self, request, pk=None): if submission.status == community_library_submission_constants.STATUS_APPROVED: self._mark_previous_pending_submissions_as_superseded(submission) + self._add_to_community_library(submission) return Response(self.serialize_object()) diff --git a/contentcuration/contentcuration/viewsets/sync/base.py b/contentcuration/contentcuration/viewsets/sync/base.py index 1d4be37c6b..2379e5b0c2 100644 --- a/contentcuration/contentcuration/viewsets/sync/base.py +++ b/contentcuration/contentcuration/viewsets/sync/base.py @@ -12,6 +12,7 @@ from contentcuration.viewsets.contentnode import PrerequisitesUpdateHandler from contentcuration.viewsets.file import FileViewSet from contentcuration.viewsets.invitation import InvitationViewSet +from contentcuration.viewsets.sync.constants import ADDED_TO_COMMUNITY_LIBRARY from contentcuration.viewsets.sync.constants import ASSESSMENTITEM from contentcuration.viewsets.sync.constants import BOOKMARK from contentcuration.viewsets.sync.constants import CHANNEL @@ -98,6 +99,7 @@ def get_change_type(obj): DEPLOYED: "deploy_from_changes", UPDATED_DESCENDANTS: "update_descendants_from_changes", PUBLISHED_NEXT: "publish_next_from_changes", + ADDED_TO_COMMUNITY_LIBRARY: "add_to_community_library_from_changes", } diff --git a/contentcuration/contentcuration/viewsets/sync/constants.py b/contentcuration/contentcuration/viewsets/sync/constants.py index 54091c9203..e45d30a3e0 100644 --- a/contentcuration/contentcuration/viewsets/sync/constants.py +++ b/contentcuration/contentcuration/viewsets/sync/constants.py @@ -9,6 +9,7 @@ DEPLOYED = 8 UPDATED_DESCENDANTS = 9 PUBLISHED_NEXT = 10 +ADDED_TO_COMMUNITY_LIBRARY = 11 ALL_CHANGES = set( @@ -23,6 +24,13 @@ DEPLOYED, UPDATED_DESCENDANTS, PUBLISHED_NEXT, + ADDED_TO_COMMUNITY_LIBRARY, + ] +) + +SERVER_ONLY_CHANGES = set( + [ + ADDED_TO_COMMUNITY_LIBRARY, ] ) diff --git a/contentcuration/contentcuration/viewsets/sync/endpoint.py b/contentcuration/contentcuration/viewsets/sync/endpoint.py index 6825833823..fcc5bed4fb 100644 --- a/contentcuration/contentcuration/viewsets/sync/endpoint.py +++ b/contentcuration/contentcuration/viewsets/sync/endpoint.py @@ -20,6 +20,7 @@ from contentcuration.tasks import apply_user_changes_task from contentcuration.viewsets.sync.constants import CHANNEL from contentcuration.viewsets.sync.constants import CREATED +from contentcuration.viewsets.sync.constants import SERVER_ONLY_CHANGES CHANGE_RETURN_LIMIT = 200 @@ -72,7 +73,11 @@ def handle_changes(self, request): # Changes that cannot be made disallowed_changes = [] for c in changes: - if c.get("channel_id") is None and c.get("user_id") == request.user.id: + if c.get("type") in SERVER_ONLY_CHANGES: + disallowed_changes.append(c) + elif ( + c.get("channel_id") is None and c.get("user_id") == request.user.id + ): user_only_changes.append(c) elif c.get("channel_id") in allowed_ids: channel_changes.append(c) diff --git a/contentcuration/contentcuration/viewsets/sync/utils.py b/contentcuration/contentcuration/viewsets/sync/utils.py index a0073ce731..e7df210926 100644 --- a/contentcuration/contentcuration/viewsets/sync/utils.py +++ b/contentcuration/contentcuration/viewsets/sync/utils.py @@ -3,6 +3,7 @@ from django.conf import settings from contentcuration.utils.sentry import report_exception +from contentcuration.viewsets.sync.constants import ADDED_TO_COMMUNITY_LIBRARY from contentcuration.viewsets.sync.constants import ALL_TABLES from contentcuration.viewsets.sync.constants import CHANNEL from contentcuration.viewsets.sync.constants import CONTENTNODE @@ -104,6 +105,25 @@ def generate_publish_next_event(key, version_notes="", language=None): return event +def generate_added_to_community_library_event( + key, + channel_version, + categories=None, + country_codes=None, +): + event = _generate_event( + key, + CHANNEL, + ADDED_TO_COMMUNITY_LIBRARY, + channel_id=key, + user_id=None, + ) + event["channel_version"] = channel_version + event["categories"] = categories or dict() + event["country_codes"] = country_codes or list() + return event + + def log_sync_exception(e, user=None, change=None, changes=None): # Capture exception and report, but allow sync # to complete properly. diff --git a/contentcuration/kolibri_public/management/commands/export_channels_to_kolibri_public.py b/contentcuration/kolibri_public/management/commands/export_channels_to_kolibri_public.py index 3a0aee8fcd..07080fb849 100644 --- a/contentcuration/kolibri_public/management/commands/export_channels_to_kolibri_public.py +++ b/contentcuration/kolibri_public/management/commands/export_channels_to_kolibri_public.py @@ -1,23 +1,14 @@ import logging import os -import shutil -import tempfile from datetime import datetime from datetime import timedelta -from django.conf import settings -from django.core.files.storage import default_storage as storage -from django.core.management import call_command from django.core.management.base import BaseCommand from django.db.models import F from django.db.models import Q from django.utils import timezone -from kolibri_content.apps import KolibriContentConfig -from kolibri_content.models import ChannelMetadata as ExportedChannelMetadata -from kolibri_content.router import get_active_content_database -from kolibri_content.router import using_content_database from kolibri_public.models import ChannelMetadata -from kolibri_public.utils.mapper import ChannelMapper +from kolibri_public.utils import export_channel_to_kolibri_public from contentcuration.models import Channel from contentcuration.models import User @@ -55,7 +46,7 @@ def handle(self, *args, **options): count = 0 for channel_id in ids_to_export: try: - self._export_channel(channel_id) + export_channel_to_kolibri_public(channel_id) count += 1 except FileNotFoundError: logger.warning( @@ -71,31 +62,6 @@ def handle(self, *args, **options): ) logger.info("Successfully put {} channels into kolibri_public".format(count)) - def _export_channel(self, channel_id): - logger.info("Putting channel {} into kolibri_public".format(channel_id)) - db_location = os.path.join( - settings.DB_ROOT, "{id}.sqlite3".format(id=channel_id) - ) - with storage.open(db_location) as storage_file: - with tempfile.NamedTemporaryFile(suffix=".sqlite3") as db_file: - shutil.copyfileobj(storage_file, db_file) - db_file.seek(0) - with using_content_database(db_file.name): - # Run migration to handle old content databases published prior to current fields being added. - call_command( - "migrate", - app_label=KolibriContentConfig.label, - database=get_active_content_database(), - ) - channel = ExportedChannelMetadata.objects.get(id=channel_id) - logger.info( - "Found channel {} for id: {} mapping now".format( - channel.name, channel_id - ) - ) - mapper = ChannelMapper(channel) - mapper.run() - def _republish_problem_channels(self): twenty_19 = datetime(year=2019, month=1, day=1) five_minutes = timedelta(minutes=5) diff --git a/contentcuration/kolibri_public/migrations/0007_new_channel_metadata.py b/contentcuration/kolibri_public/migrations/0007_new_channel_metadata.py new file mode 100644 index 0000000000..cfa24e91d2 --- /dev/null +++ b/contentcuration/kolibri_public/migrations/0007_new_channel_metadata.py @@ -0,0 +1,26 @@ +# Generated by Django 3.2.24 on 2025-07-22 10:38 +from django.db import migrations +from django.db import models + + +class Migration(migrations.Migration): + + dependencies = [ + ("contentcuration", "0157_alter_communitylibrarysubmission_resolution_reason"), + ("kolibri_public", "0006_auto_20250417_1516"), + ] + + operations = [ + migrations.AddField( + model_name="channelmetadata", + name="categories", + field=models.JSONField(blank=True, null=True), + ), + migrations.AddField( + model_name="channelmetadata", + name="countries", + field=models.ManyToManyField( + related_name="public_channels", to="contentcuration.Country" + ), + ), + ] diff --git a/contentcuration/kolibri_public/models.py b/contentcuration/kolibri_public/models.py index c0056d9cf9..35b3ede984 100644 --- a/contentcuration/kolibri_public/models.py +++ b/contentcuration/kolibri_public/models.py @@ -7,6 +7,7 @@ from mptt.managers import TreeManager from mptt.querysets import TreeQuerySet +from contentcuration.models import Country from contentcuration.models import Language @@ -104,6 +105,8 @@ class ChannelMetadata(base_models.ChannelMetadata): ) order = models.PositiveIntegerField(default=0, null=True, blank=True) public = models.BooleanField() + categories = models.JSONField(null=True, blank=True) + countries = models.ManyToManyField(Country, related_name="public_channels") class MPTTTreeIDManager(models.Model): diff --git a/contentcuration/kolibri_public/tests/test_export_channels_to_kolibri_public.py b/contentcuration/kolibri_public/tests/test_export_channels_to_kolibri_public.py new file mode 100644 index 0000000000..1a8fdba131 --- /dev/null +++ b/contentcuration/kolibri_public/tests/test_export_channels_to_kolibri_public.py @@ -0,0 +1,152 @@ +import os.path +import shutil +import tempfile +import uuid +from unittest import mock + +from django.conf import settings +from django.core.files.storage import FileSystemStorage +from django.core.management import call_command +from django.db import models as django_models +from django.test import TestCase +from kolibri_content.apps import KolibriContentConfig +from kolibri_content.fields import UUIDField +from kolibri_content.models import ChannelMetadata as ExportedChannelMetadata +from kolibri_content.router import get_active_content_database +from kolibri_content.router import using_content_database +from kolibri_public.utils.export_channel_to_kolibri_public import ( + export_channel_to_kolibri_public, +) +from mixer.backend.django import GenFactory +from mixer.backend.django import Mixer + +from contentcuration.models import Country + + +class CustomizedMixer(Mixer): + """Slightly modified Mixer that works correctly with the active + content database and with UUIDField. + """ + + def __init__(self, *args, **kwargs): + mixer_factory = GenFactory() + mixer_factory.generators[UUIDField] = mixer_factory.generators[ + django_models.UUIDField + ] + + return super().__init__(*args, factory=mixer_factory, **kwargs) + + def postprocess(self, target): + if self.params.get("commit"): + # Not sure why the `force_insert` is needed, but using the + # mixer causes "Save with update_fields did not affect any rows" error + # if this is not specified + target.save(using=get_active_content_database(), force_insert=True) + + return target + + +class ExportTestCase(TestCase): + def setUp(self): + super().setUp() + + self._temp_directory_ctx = tempfile.TemporaryDirectory() + test_db_root_dir = self._temp_directory_ctx.__enter__() + + storage = FileSystemStorage(location=test_db_root_dir) + + self._storage_patch_ctx = mock.patch( + "kolibri_public.utils.export_channel_to_kolibri_public.storage", + new=storage, + ) + self._storage_patch_ctx.__enter__() + + os.makedirs(os.path.join(test_db_root_dir, settings.DB_ROOT), exist_ok=True) + + self.channel_id = uuid.UUID(int=42).hex + self.channel_version = 1 + + db_path = os.path.join( + test_db_root_dir, + settings.DB_ROOT, + f"{self.channel_id}-{self.channel_version}.sqlite3", + ) + open(db_path, "w").close() + + with using_content_database(db_path): + call_command( + "migrate", + app_label=KolibriContentConfig.label, + database=get_active_content_database(), + ) + + mixer = CustomizedMixer() + self.exported_channel_metadata = mixer.blend( + ExportedChannelMetadata, + id=self.channel_id, + version=self.channel_version, + ) + + self.db_path_without_version = os.path.join( + test_db_root_dir, settings.DB_ROOT, f"{self.channel_id}.sqlite3" + ) + shutil.copyfile(db_path, self.db_path_without_version) + + def tearDown(self): + self._temp_directory_ctx.__exit__(None, None, None) + self._storage_patch_ctx.__exit__(None, None, None) + + super().tearDown() + + @mock.patch("kolibri_public.utils.export_channel_to_kolibri_public.ChannelMapper") + def test_export_channel_to_kolibri_public__existing_version( + self, mock_channel_mapper + ): + categories = { + "Category1": True, + "Category2": True, + } + country1 = Country.objects.create(code="C1", name="Country 1") + country2 = Country.objects.create(code="C2", name="Country 2") + countries = [country1, country2] + + export_channel_to_kolibri_public( + channel_id=self.channel_id, + channel_version=1, + public=True, + categories=categories, + countries=countries, + ) + + mock_channel_mapper.assert_called_once_with( + channel=self.exported_channel_metadata, + channel_version=1, + public=True, + categories=categories, + countries=countries, + ) + mock_channel_mapper.return_value.run.assert_called_once_with() + + @mock.patch("kolibri_public.utils.export_channel_to_kolibri_public.ChannelMapper") + def test_export_channel_to_kolibri_public__without_version( + self, mock_channel_mapper + ): + export_channel_to_kolibri_public( + channel_id=self.channel_id, + ) + + mock_channel_mapper.assert_called_once_with( + channel=self.exported_channel_metadata, + channel_version=None, + public=True, + categories=None, + countries=None, + ) + mock_channel_mapper.return_value.run.assert_called_once_with() + + def test_export_channel_to_kolibri_public__bad_version(self): + with self.assertRaises(FileNotFoundError): + export_channel_to_kolibri_public( + channel_id=self.channel_id, + channel_version=2, + ) diff --git a/contentcuration/kolibri_public/tests/test_mapper.py b/contentcuration/kolibri_public/tests/test_mapper.py index d938233e63..edefafcd25 100644 --- a/contentcuration/kolibri_public/tests/test_mapper.py +++ b/contentcuration/kolibri_public/tests/test_mapper.py @@ -1,6 +1,9 @@ +import datetime import os import tempfile +from unittest import mock +import pytz from django.core.management import call_command from django.test import TestCase from kolibri_content import models as kolibri_content_models @@ -14,6 +17,7 @@ from le_utils.constants import content_kinds from contentcuration.models import Channel +from contentcuration.models import Country from contentcuration.tests.testdata import user @@ -28,16 +32,24 @@ def overrides(self): kolibri_public_models.LocalFile: { "available": True, }, + kolibri_public_models.ChannelMetadata: { + "last_updated": self.dummy_date, + }, } - @classmethod - def setUpClass(cls): - super(ChannelMapperTest, cls).setUpClass() + def setUp(self): + super().setUp() call_command("loadconstants") - _, cls.tempdb = tempfile.mkstemp(suffix=".sqlite3") + _, self.tempdb = tempfile.mkstemp(suffix=".sqlite3") admin_user = user() - with using_content_database(cls.tempdb): + self.dummy_date = datetime.datetime(2020, 1, 1, 0, 0, 0, tzinfo=pytz.utc) + self._date_patcher = mock.patch( + "kolibri_public.utils.annotation.timezone.now", return_value=self.dummy_date + ) + self._date_patcher.start() + + with using_content_database(self.tempdb): call_command( "migrate", "content", @@ -52,23 +64,20 @@ def setUpClass(cls): }, ) builder.insert_into_default_db() - cls.source_root = kolibri_content_models.ContentNode.objects.get( + self.source_root = kolibri_content_models.ContentNode.objects.get( id=builder.root_node["id"] ) - cls.channel = kolibri_content_models.ChannelMetadata.objects.get( + self.channel = kolibri_content_models.ChannelMetadata.objects.get( id=builder.channel["id"] ) contentcuration_channel = Channel.objects.create( actor_id=admin_user.id, - id=cls.channel.id, - name=cls.channel.name, + id=self.channel.id, + name=self.channel.name, public=True, ) contentcuration_channel.main_tree.published = True contentcuration_channel.main_tree.save() - cls.mapper = ChannelMapper(cls.channel) - cls.mapper.run() - cls.mapped_root = cls.mapper.mapped_root def _assert_model(self, source, mapped, Model): for field in Model._meta.fields: @@ -79,7 +88,11 @@ def _assert_model(self, source, mapped, Model): self.overrides[Model][column], getattr(mapped, column) ) else: - self.assertEqual(getattr(source, column), getattr(mapped, column)) + self.assertEqual( + getattr(source, column), + getattr(mapped, column), + f"Mismatch in model {Model.__name__}, column {column}", + ) def _assert_node(self, source, mapped): """ @@ -133,6 +146,10 @@ def _recurse_and_assert(self, sources, mappeds, recursion_depth=0): def test_map(self): with using_content_database(self.tempdb): + self.mapper = ChannelMapper(self.channel) + self.mapper.run() + self.mapped_root = self.mapper.mapped_root + self._recurse_and_assert([self.source_root], [self.mapped_root]) self._assert_model( self.channel, @@ -142,6 +159,10 @@ def test_map(self): def test_map_replace(self): with using_content_database(self.tempdb): + self.mapper = ChannelMapper(self.channel) + self.mapper.run() + self.mapped_root = self.mapper.mapped_root + mapper = ChannelMapper(self.channel) mapper.run() self._recurse_and_assert([self.source_root], [mapper.mapped_root]) @@ -151,10 +172,105 @@ def test_map_replace(self): kolibri_public_models.ChannelMetadata, ) - @classmethod - def tearDownClass(cls): + def test_categories__none_provided(self): + with using_content_database(self.tempdb): + kolibri_content_models.ContentNode.objects.filter( + channel_id=self.channel.id, + ).update(categories=None) + + mapper = ChannelMapper(self.channel) + mapper.run() + + self.assertEqual(mapper.mapped_channel.categories, {}) + + def test_categories__only_provided(self): + with using_content_database(self.tempdb): + kolibri_content_models.ContentNode.objects.filter( + channel_id=self.channel.id, + ).update(categories=None) + + categories = {"Category1": True, "Category2": True} + mapper = ChannelMapper(self.channel, categories=categories) + mapper.run() + + self.assertEqual(mapper.mapped_channel.categories, categories) + + def test_categories__only_on_content_nodes(self): + with using_content_database(self.tempdb): + source_contentnodes_queryset = ( + kolibri_content_models.ContentNode.objects.filter( + channel_id=self.channel.id, + ) + ) + contentnode1 = source_contentnodes_queryset[0] + contentnode2 = source_contentnodes_queryset[1] + + source_contentnodes_queryset.update(categories=None) + contentnode1.categories = "Category1,Category2" + contentnode2.categories = "Category3" + contentnode1.save() + contentnode2.save() + + mapper = ChannelMapper(self.channel) + mapper.run() + + self.assertEqual( + mapper.mapped_channel.categories, + {"Category1": True, "Category2": True, "Category3": True}, + ) + + def test_categories__both_provided_and_on_content_nodes(self): + with using_content_database(self.tempdb): + source_contentnodes_queryset = ( + kolibri_content_models.ContentNode.objects.filter( + channel_id=self.channel.id, + ) + ) + contentnode1 = source_contentnodes_queryset[0] + contentnode2 = source_contentnodes_queryset[1] + + source_contentnodes_queryset.update(categories=None) + contentnode1.categories = "Category1,Category2" + contentnode2.categories = "Category3" + contentnode1.save() + contentnode2.save() + + categories = {"Category3": True, "Category4": True} + mapper = ChannelMapper(self.channel, categories=categories) + mapper.run() + + self.assertEqual( + mapper.mapped_channel.categories, + { + "Category1": True, + "Category2": True, + "Category3": True, + "Category4": True, + }, + ) + + def test_countries__none_provided(self): + with using_content_database(self.tempdb): + mapper = ChannelMapper(self.channel) + mapper.run() + + self.assertEqual(mapper.mapped_channel.countries.count(), 0) + + def test_countries__provided(self): + with using_content_database(self.tempdb): + country1 = Country.objects.create(code="C1", name="Country 1") + country2 = Country.objects.create(code="C2", name="Country 2") + + countries = [country1, country2] + mapper = ChannelMapper(self.channel, countries=countries) + mapper.run() + + self.assertCountEqual(mapper.mapped_channel.countries.all(), countries) + + def tearDown(self): # Clean up datbase connection after the test - cleanup_content_database_connection(cls.tempdb) - super(ChannelMapperTest, cls).tearDownClass() - if os.path.exists(cls.tempdb): - os.remove(cls.tempdb) + self._date_patcher.stop() + cleanup_content_database_connection(self.tempdb) + super().tearDown() + if os.path.exists(self.tempdb): + os.remove(self.tempdb) diff --git a/contentcuration/kolibri_public/utils/annotation.py b/contentcuration/kolibri_public/utils/annotation.py index 7295c97e39..c2889282ae 100644 --- a/contentcuration/kolibri_public/utils/annotation.py +++ b/contentcuration/kolibri_public/utils/annotation.py @@ -1,11 +1,12 @@ """ -Functions in here are the subset of annotation functions from Kolibri related to channel metadata. +Functions in here are a modified subset of annotation functions from Kolibri related to channel metadata. https://github.com/learningequality/kolibri/blob/caec91dd2da5617adfb50332fb698068248e8e47/kolibri/core/content/utils/annotation.py#L731 """ -import datetime +from itertools import chain from django.db.models import Q from django.db.models import Sum +from django.utils import timezone from kolibri_public.models import ChannelMetadata from kolibri_public.models import ContentNode from kolibri_public.models import LocalFile @@ -14,18 +15,29 @@ from contentcuration.models import Channel -def set_channel_metadata_fields(channel_id, public=None): +def set_channel_metadata_fields( + channel_id, + channel_version=None, + public=None, + categories=None, + countries=None, +): # Remove unneeded db_lock channel = ChannelMetadata.objects.get(id=channel_id) calculate_published_size(channel) calculate_total_resource_count(channel) calculate_included_languages(channel) + calculate_included_categories(channel, categories) calculate_next_order(channel, public=public) # Add this to ensure we keep this up to date. - channel.last_updated = datetime.datetime.now() + channel.last_updated = timezone.now() if public is not None: channel.public = public + if channel_version is not None: + channel.version = channel_version + if countries is not None: + channel.countries.set(countries) channel.save() @@ -67,6 +79,28 @@ def calculate_included_languages(channel): channel.included_languages.add(*list(languages)) +def calculate_included_categories(channel, categories): + content_nodes = ContentNode.objects.filter( + channel_id=channel.id, available=True + ).exclude(categories=None) + + categories_comma_separated_lists = content_nodes.values_list( + "categories", flat=True + ) + contentnode_categories = set( + chain.from_iterable( + ( + categories_comma_separated_list.split(",") + for categories_comma_separated_list in categories_comma_separated_lists + ) + ) + ) + categories_dict = {category: True for category in contentnode_categories} + categories_dict.update(categories or {}) + channel.categories = categories_dict + channel.save() + + def calculate_next_order(channel, public=False): # This has been edited from the source Kolibri, in order # to make the order match given by the public channel API on Studio. diff --git a/contentcuration/kolibri_public/utils/export_channel_to_kolibri_public.py b/contentcuration/kolibri_public/utils/export_channel_to_kolibri_public.py new file mode 100644 index 0000000000..4ba442dc87 --- /dev/null +++ b/contentcuration/kolibri_public/utils/export_channel_to_kolibri_public.py @@ -0,0 +1,60 @@ +import logging +import os +import shutil +import tempfile + +from django.conf import settings +from django.core.files.storage import default_storage as storage +from django.core.management import call_command +from kolibri_content.apps import KolibriContentConfig +from kolibri_content.models import ChannelMetadata as ExportedChannelMetadata +from kolibri_content.router import get_active_content_database +from kolibri_content.router import using_content_database +from kolibri_public.utils.mapper import ChannelMapper + + +logger = logging.getLogger(__file__) + + +def export_channel_to_kolibri_public( + channel_id, + channel_version=None, + public=True, + categories=None, + countries=None, +): + logger.info("Putting channel {} into kolibri_public".format(channel_id)) + + if channel_version is not None: + db_filename = "{id}-{version}.sqlite3".format( + id=channel_id, version=channel_version + ) + else: + db_filename = "{id}.sqlite3".format(id=channel_id) + db_location = os.path.join(settings.DB_ROOT, db_filename) + + with storage.open(db_location) as storage_file: + with tempfile.NamedTemporaryFile(suffix=".sqlite3") as db_file: + shutil.copyfileobj(storage_file, db_file) + db_file.seek(0) + with using_content_database(db_file.name): + # Run migration to handle old content databases published prior to current fields being added. + call_command( + "migrate", + app_label=KolibriContentConfig.label, + database=get_active_content_database(), + ) + channel = ExportedChannelMetadata.objects.get(id=channel_id) + logger.info( + "Found channel {} for id: {} mapping now".format( + channel.name, channel_id + ) + ) + mapper = ChannelMapper( + channel=channel, + channel_version=channel_version, + public=public, + categories=categories, + countries=countries, + ) + mapper.run() diff --git a/contentcuration/kolibri_public/utils/mapper.py b/contentcuration/kolibri_public/utils/mapper.py index 01dc1f0726..f9258bfb7d 100644 --- a/contentcuration/kolibri_public/utils/mapper.py +++ b/contentcuration/kolibri_public/utils/mapper.py @@ -19,9 +19,19 @@ class ChannelMapper(object): Foreign Keyed from the root ContentNode. """ - def __init__(self, channel, public=True): + def __init__( + self, + channel, + channel_version=None, + public=True, + categories=None, + countries=None, + ): self.channel = channel + self.channel_version = channel_version self.public = public + self.categories = categories + self.countries = countries @property def overrides(self): @@ -57,7 +67,19 @@ def run(self): annotate_label_bitmasks(self.mapped_root.get_descendants(include_self=True)) # Rather than set the ancestors fields after mapping, like it is done in Kolibri # here we set it during mapping as we are already recursing through the tree. - set_channel_metadata_fields(self.mapped_channel.id, public=self.public) + + set_channel_metadata_fields( + self.mapped_channel.id, + channel_version=self.channel_version, + public=self.public, + categories=self.categories, + countries=self.countries, + ) + + # Refreshing this is needed, because otherwise the fields set in the + # set_channel_metadata_fields function will not be reflected in the + # self.mapped_channel object. + self.mapped_channel.refresh_from_db() def _map_model(self, source, Model): properties = {} From 93f281045859ee7649df454b12409e88bcf75455 Mon Sep 17 00:00:00 2001 From: Jakub Komarek Date: Wed, 30 Jul 2025 16:07:15 +0200 Subject: [PATCH 02/11] Fix migration dependency --- .../kolibri_public/migrations/0007_new_channel_metadata.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contentcuration/kolibri_public/migrations/0007_new_channel_metadata.py b/contentcuration/kolibri_public/migrations/0007_new_channel_metadata.py index cfa24e91d2..1e2144d6b3 100644 --- a/contentcuration/kolibri_public/migrations/0007_new_channel_metadata.py +++ b/contentcuration/kolibri_public/migrations/0007_new_channel_metadata.py @@ -6,7 +6,7 @@ class Migration(migrations.Migration): dependencies = [ - ("contentcuration", "0157_alter_communitylibrarysubmission_resolution_reason"), + ("contentcuration", "0156_communitylibrarysubmission_admin_fields"), ("kolibri_public", "0006_auto_20250417_1516"), ] From e6d9950bceb58e0e63ff3c5d63acbb34ed3f327c Mon Sep 17 00:00:00 2001 From: Jakub Komarek Date: Mon, 4 Aug 2025 16:02:18 +0200 Subject: [PATCH 03/11] Use change key instead of channel_id --- contentcuration/contentcuration/viewsets/channel.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contentcuration/contentcuration/viewsets/channel.py b/contentcuration/contentcuration/viewsets/channel.py index 5f8a33ade9..a3cea25b35 100644 --- a/contentcuration/contentcuration/viewsets/channel.py +++ b/contentcuration/contentcuration/viewsets/channel.py @@ -777,7 +777,7 @@ def add_to_community_library_from_changes(self, changes): for change in changes: try: self.add_to_community_library( - channel_id=change["channel_id"], + channel_id=change["key"], channel_version=change["channel_version"], categories=change["categories"], country_codes=change["country_codes"], From 611ccd16d6c2912d17d8cdf1c79eea5c304a18d1 Mon Sep 17 00:00:00 2001 From: Jakub Komarek Date: Mon, 4 Aug 2025 16:34:55 +0200 Subject: [PATCH 04/11] Don't allow creating community library submissions for public channels --- contentcuration/contentcuration/viewsets/channel.py | 4 ++-- .../contentcuration/viewsets/community_library_submission.py | 5 +++++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/contentcuration/contentcuration/viewsets/channel.py b/contentcuration/contentcuration/viewsets/channel.py index a3cea25b35..adccaa4373 100644 --- a/contentcuration/contentcuration/viewsets/channel.py +++ b/contentcuration/contentcuration/viewsets/channel.py @@ -799,9 +799,9 @@ def add_to_community_library( channel = self.get_edit_queryset().get(pk=channel_id) countries = Country.objects.filter(code__in=country_codes) - if not channel.public: + if channel.public: raise ValidationError( - "Only public channels can be added to the community library" + "Public channels cannot be added to the community library" ) if channel_version <= 0 or channel_version > channel.version: raise ValidationError("Invalid channel version") diff --git a/contentcuration/contentcuration/viewsets/community_library_submission.py b/contentcuration/contentcuration/viewsets/community_library_submission.py index d99786fd67..8a19542b33 100644 --- a/contentcuration/contentcuration/viewsets/community_library_submission.py +++ b/contentcuration/contentcuration/viewsets/community_library_submission.py @@ -61,6 +61,11 @@ def create(self, validated_data): "unpublished channel." ) + if channel.public: + raise ValidationError( + "Cannot create a community library submission for a public channel." + ) + if not channel.editors.filter(id=user.id).exists(): raise ValidationError( "Only editors can create a community library " From 7fce8bcee2d3c85ea47084f65ce3afc7b408a3f0 Mon Sep 17 00:00:00 2001 From: Jakub Komarek Date: Mon, 4 Aug 2025 16:51:54 +0200 Subject: [PATCH 05/11] Fix tests to test with non-public channels --- .../tests/viewsets/test_channel.py | 2 +- .../test_community_library_submission.py | 25 ++++++++++++++++--- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/contentcuration/contentcuration/tests/viewsets/test_channel.py b/contentcuration/contentcuration/tests/viewsets/test_channel.py index 9ea043966c..bed1af5978 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_channel.py +++ b/contentcuration/contentcuration/tests/viewsets/test_channel.py @@ -549,7 +549,7 @@ def test_process_added_to_community_library_change(self, mock_export_func): channel = testdata.channel() channel.version = 1 - channel.public = True + channel.public = False channel.save() categories = { diff --git a/contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py b/contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py index f0fff43647..5ff525c4fa 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py +++ b/contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py @@ -61,21 +61,21 @@ def setUp(self): self.country2 = testdata.country(name="Country 2", code="C2") self.channel_with_submission1 = testdata.channel() - self.channel_with_submission1.public = True + self.channel_with_submission1.public = False self.channel_with_submission1.version = 1 self.channel_with_submission1.editors.add(self.author_user) self.channel_with_submission1.editors.add(self.editor_user) self.channel_with_submission1.save() self.channel_with_submission2 = testdata.channel() - self.channel_with_submission2.public = True + self.channel_with_submission2.public = False self.channel_with_submission2.version = 1 self.channel_with_submission2.editors.add(self.author_user) self.channel_with_submission2.editors.add(self.editor_user) self.channel_with_submission2.save() self.channel_without_submission = testdata.channel() - self.channel_without_submission.public = True + self.channel_without_submission.public = False self.channel_without_submission.version = 1 self.channel_without_submission.editors.add(self.author_user) self.channel_without_submission.editors.add(self.editor_user) @@ -88,6 +88,13 @@ def setUp(self): self.unpublished_channel.editors.add(self.editor_user) self.unpublished_channel.save() + self.public_channel = testdata.channel() + self.public_channel.public = True + self.public_channel.version = 1 + self.public_channel.editors.add(self.author_user) + self.public_channel.editors.add(self.editor_user) + self.public_channel.save() + self.existing_submission1 = testdata.community_library_submission() self.existing_submission1.channel = self.channel_with_submission1 self.existing_submission1.author = self.author_user @@ -134,6 +141,18 @@ def test_create_submission__unpublished_channel(self): ) self.assertEqual(response.status_code, 400, response.content) + def test_create_submission__public_channel(self): + self.client.force_authenticate(user=self.editor_user) + submission = self.new_submission_metadata + submission["channel"] = self.public_channel.id + + response = self.client.post( + reverse("community-library-submission-list"), + submission, + format="json", + ) + self.assertEqual(response.status_code, 400, response.content) + def test_create_submission__explicit_channel_version(self): self.client.force_authenticate(user=self.editor_user) submission_metadata = self.new_submission_metadata From b8fee43669c2b6c1aca4f247bb017dd6dfa7cbb8 Mon Sep 17 00:00:00 2001 From: Jakub Komarek Date: Mon, 4 Aug 2025 18:30:51 +0200 Subject: [PATCH 06/11] Set submission as live when added to community library --- .../tests/viewsets/test_channel.py | 36 +++++++++++++++++-- .../contentcuration/viewsets/channel.py | 21 +++++++++++ 2 files changed, 54 insertions(+), 3 deletions(-) diff --git a/contentcuration/contentcuration/tests/viewsets/test_channel.py b/contentcuration/contentcuration/tests/viewsets/test_channel.py index bed1af5978..932789de55 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_channel.py +++ b/contentcuration/contentcuration/tests/viewsets/test_channel.py @@ -11,7 +11,9 @@ from contentcuration import models from contentcuration import models as cc from contentcuration.constants import channel_history +from contentcuration.constants import community_library_submission from contentcuration.models import Change +from contentcuration.models import CommunityLibrarySubmission from contentcuration.models import ContentNode from contentcuration.models import Country from contentcuration.tasks import apply_channel_changes_task @@ -547,11 +549,27 @@ def test_process_added_to_community_library_change(self, mock_export_func): # Creating the change on the backend should be supported self.client.force_authenticate(self.admin_user) + editor_user = testdata.user("channel@editor.com") + channel = testdata.channel() - channel.version = 1 + channel.version = 2 channel.public = False + channel.editors.add(editor_user) channel.save() + current_live_submission = CommunityLibrarySubmission.objects.create( + channel=channel, + channel_version=1, + author=editor_user, + status=community_library_submission.STATUS_LIVE, + ) + new_submission = CommunityLibrarySubmission.objects.create( + channel=channel, + channel_version=2, + author=editor_user, + status=community_library_submission.STATUS_APPROVED, + ) + categories = { "category1": True, "category2": True, @@ -564,7 +582,7 @@ def test_process_added_to_community_library_change(self, mock_export_func): added_to_community_library_change = generate_added_to_community_library_event( key=channel.id, - channel_version=1, + channel_version=2, categories=categories, country_codes=country_codes, ) @@ -596,7 +614,7 @@ def test_process_added_to_community_library_change(self, mock_export_func): ], ) self.assertEqual(call_kwargs["channel_id"], channel.id) - self.assertEqual(call_kwargs["channel_version"], 1) + self.assertEqual(call_kwargs["channel_version"], 2) self.assertEqual(call_kwargs["categories"], categories) # The countries argument used when creating the mapper is in fact @@ -604,6 +622,18 @@ def test_process_added_to_community_library_change(self, mock_export_func): self.assertCountEqual(call_kwargs["countries"], countries) self.assertEqual(call_kwargs["public"], False) + # Check that the current submission became the live one + current_live_submission.refresh_from_db() + new_submission.refresh_from_db() + self.assertEqual( + current_live_submission.status, + community_library_submission.STATUS_APPROVED, + ) + self.assertEqual( + new_submission.status, + community_library_submission.STATUS_LIVE, + ) + class CRUDTestCase(StudioAPITestCase): @property diff --git a/contentcuration/contentcuration/viewsets/channel.py b/contentcuration/contentcuration/viewsets/channel.py index adccaa4373..7f4b3e8b5b 100644 --- a/contentcuration/contentcuration/viewsets/channel.py +++ b/contentcuration/contentcuration/viewsets/channel.py @@ -42,9 +42,13 @@ from search.utils import get_fts_search_query import contentcuration.models as models +from contentcuration.constants import ( + community_library_submission as community_library_submission_constants, +) from contentcuration.decorators import cache_no_user_data from contentcuration.models import Change from contentcuration.models import Channel +from contentcuration.models import CommunityLibrarySubmission from contentcuration.models import ContentNode from contentcuration.models import Country from contentcuration.models import File @@ -814,6 +818,23 @@ def add_to_community_library( countries=countries, ) + # Mark the submission corresponding to the channel version + # as the only live submission for the channel + CommunityLibrarySubmission.objects.filter( + channel_id=channel_id, + status=community_library_submission_constants.STATUS_LIVE, + ).update( + status=community_library_submission_constants.STATUS_APPROVED, + ) + + new_live_submission = CommunityLibrarySubmission.objects.get( + channel_id=channel_id, + channel_version=channel_version, + status=community_library_submission_constants.STATUS_APPROVED, + ) + new_live_submission.status = community_library_submission_constants.STATUS_LIVE + new_live_submission.save() + @action( detail=True, methods=["get"], From 6f5bdfbf8dd1759a4af1b5c4bc7ad398c724f68b Mon Sep 17 00:00:00 2001 From: Jakub Komarek Date: Mon, 4 Aug 2025 19:04:48 +0200 Subject: [PATCH 07/11] Fix handling partial submission update without countries --- .../test_community_library_submission.py | 24 ++++++++++++++++++- .../viewsets/community_library_submission.py | 5 ++-- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py b/contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py index 5ff525c4fa..58c3ae5871 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py +++ b/contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py @@ -338,7 +338,7 @@ def test_update_submission__is_editor(self): ) self.assertEqual(response.status_code, 404, response.content) - def test_update_submission__is_admin(self): + def test_update_submission__is_admin__change_countries(self): self.client.force_authenticate(user=self.admin_user) response = self.client.put( reverse( @@ -356,6 +356,28 @@ def test_update_submission__is_admin(self): self.assertEqual(updated_submission.countries.count(), 1) self.assertEqual(updated_submission.countries.first().code, "C2") + def test_update_submission__is_admin__keep_countries(self): + self.client.force_authenticate(user=self.admin_user) + + updated_submission_metadata = self.updated_submission_metadata.copy() + updated_submission_metadata.pop("countries") + + response = self.client.put( + reverse( + "community-library-submission-detail", + args=[self.existing_submission1.id], + ), + updated_submission_metadata, + format="json", + ) + self.assertEqual(response.status_code, 200, response.content) + + updated_submission = CommunityLibrarySubmission.objects.get( + id=self.existing_submission1.id + ) + self.assertEqual(updated_submission.countries.count(), 1) + self.assertEqual(updated_submission.countries.first().code, "C1") + def test_update_submission__change_channel(self): self.client.force_authenticate(user=self.admin_user) submission_metadata = self.updated_submission_metadata diff --git a/contentcuration/contentcuration/viewsets/community_library_submission.py b/contentcuration/contentcuration/viewsets/community_library_submission.py index 8a19542b33..5ae7562b2f 100644 --- a/contentcuration/contentcuration/viewsets/community_library_submission.py +++ b/contentcuration/contentcuration/viewsets/community_library_submission.py @@ -93,8 +93,9 @@ def update(self, instance, validated_data): "a community library submission." ) - countries = validated_data.pop("countries", []) - instance.countries.set(countries) + if "countries" in validated_data: + countries = validated_data.pop("countries") + instance.countries.set(countries) return super().update(instance, validated_data) From 8a0235e53d57fd971a9b1cba3d9eb813249a6549 Mon Sep 17 00:00:00 2001 From: Jakub Komarek Date: Mon, 4 Aug 2025 19:24:00 +0200 Subject: [PATCH 08/11] Remove explicit channel_version argument for ChannelMapper --- contentcuration/kolibri_public/utils/annotation.py | 3 --- .../utils/export_channel_to_kolibri_public.py | 1 - contentcuration/kolibri_public/utils/mapper.py | 6 +++--- 3 files changed, 3 insertions(+), 7 deletions(-) diff --git a/contentcuration/kolibri_public/utils/annotation.py b/contentcuration/kolibri_public/utils/annotation.py index c2889282ae..1c9a193511 100644 --- a/contentcuration/kolibri_public/utils/annotation.py +++ b/contentcuration/kolibri_public/utils/annotation.py @@ -17,7 +17,6 @@ def set_channel_metadata_fields( channel_id, - channel_version=None, public=None, categories=None, countries=None, @@ -34,8 +33,6 @@ def set_channel_metadata_fields( if public is not None: channel.public = public - if channel_version is not None: - channel.version = channel_version if countries is not None: channel.countries.set(countries) channel.save() diff --git a/contentcuration/kolibri_public/utils/export_channel_to_kolibri_public.py b/contentcuration/kolibri_public/utils/export_channel_to_kolibri_public.py index 4ba442dc87..d751bd6614 100644 --- a/contentcuration/kolibri_public/utils/export_channel_to_kolibri_public.py +++ b/contentcuration/kolibri_public/utils/export_channel_to_kolibri_public.py @@ -52,7 +52,6 @@ def export_channel_to_kolibri_public( ) mapper = ChannelMapper( channel=channel, - channel_version=channel_version, public=public, categories=categories, countries=countries, diff --git a/contentcuration/kolibri_public/utils/mapper.py b/contentcuration/kolibri_public/utils/mapper.py index f9258bfb7d..bd779ea6d2 100644 --- a/contentcuration/kolibri_public/utils/mapper.py +++ b/contentcuration/kolibri_public/utils/mapper.py @@ -22,13 +22,14 @@ class ChannelMapper(object): def __init__( self, channel, - channel_version=None, public=True, categories=None, countries=None, ): + # Note: The argument `channel` is an instance of `kolibri_content.models.ChannelMetadata,` + # which belongs to a specific channel version to be exported. Therefore, we do not + # need to explicitly pass the channel version as an argument here. self.channel = channel - self.channel_version = channel_version self.public = public self.categories = categories self.countries = countries @@ -70,7 +71,6 @@ def run(self): set_channel_metadata_fields( self.mapped_channel.id, - channel_version=self.channel_version, public=self.public, categories=self.categories, countries=self.countries, From 9152aeb2b8a1fa06c74453663ec4595abe3c644c Mon Sep 17 00:00:00 2001 From: Jakub Komarek Date: Mon, 4 Aug 2025 22:50:27 +0200 Subject: [PATCH 09/11] Fix tests expecting the `channel_version` argument --- .../tests/test_export_channels_to_kolibri_public.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/contentcuration/kolibri_public/tests/test_export_channels_to_kolibri_public.py b/contentcuration/kolibri_public/tests/test_export_channels_to_kolibri_public.py index 1a8fdba131..d0e22d4e2c 100644 --- a/contentcuration/kolibri_public/tests/test_export_channels_to_kolibri_public.py +++ b/contentcuration/kolibri_public/tests/test_export_channels_to_kolibri_public.py @@ -120,7 +120,6 @@ def test_export_channel_to_kolibri_public__existing_version( mock_channel_mapper.assert_called_once_with( channel=self.exported_channel_metadata, - channel_version=1, public=True, categories=categories, countries=countries, @@ -137,7 +136,6 @@ def test_export_channel_to_kolibri_public__without_version( mock_channel_mapper.assert_called_once_with( channel=self.exported_channel_metadata, - channel_version=None, public=True, categories=None, countries=None, From 550fe78fee720cc5d1b7ac45164507c74b774401 Mon Sep 17 00:00:00 2001 From: Jakub Komarek Date: Mon, 4 Aug 2025 22:51:12 +0200 Subject: [PATCH 10/11] Represent categories as a list after passing from changes --- .../tests/viewsets/test_channel.py | 2 +- .../contentcuration/viewsets/channel.py | 12 +++++++++++- contentcuration/kolibri_public/models.py | 2 ++ .../test_export_channels_to_kolibri_public.py | 5 +---- .../kolibri_public/tests/test_mapper.py | 15 +++++---------- .../kolibri_public/utils/annotation.py | 8 +++++--- .../utils/export_channel_to_kolibri_public.py | 1 + contentcuration/kolibri_public/utils/mapper.py | 3 +++ 8 files changed, 29 insertions(+), 19 deletions(-) diff --git a/contentcuration/contentcuration/tests/viewsets/test_channel.py b/contentcuration/contentcuration/tests/viewsets/test_channel.py index 932789de55..d0e1dd4fca 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_channel.py +++ b/contentcuration/contentcuration/tests/viewsets/test_channel.py @@ -615,7 +615,7 @@ def test_process_added_to_community_library_change(self, mock_export_func): ) self.assertEqual(call_kwargs["channel_id"], channel.id) self.assertEqual(call_kwargs["channel_version"], 2) - self.assertEqual(call_kwargs["categories"], categories) + self.assertCountEqual(call_kwargs["categories"], categories.keys()) # The countries argument used when creating the mapper is in fact # not a list, but a QuerySet, but it contains the same elements diff --git a/contentcuration/contentcuration/viewsets/channel.py b/contentcuration/contentcuration/viewsets/channel.py index 7f4b3e8b5b..4a51b37319 100644 --- a/contentcuration/contentcuration/viewsets/channel.py +++ b/contentcuration/contentcuration/viewsets/channel.py @@ -795,6 +795,10 @@ def add_to_community_library_from_changes(self, changes): def add_to_community_library( self, channel_id, channel_version, categories, country_codes ): + # Note: The `categories` field should contain a _dict_, with the category IDs as keys + # and `True` as a value. This is to match the representation + # of sets in the changes architecture. + # The change to add a channel to the community library can only # be created server-side, so in theory we should not be getting # malformed requests here. However, just to be safe, we still @@ -810,11 +814,17 @@ def add_to_community_library( if channel_version <= 0 or channel_version > channel.version: raise ValidationError("Invalid channel version") + # Because of changes architecture, the categories are passed as a dict + # with the category IDs as keys and `True` as a value. At this point, + # we are no longer working with changes, so we switch to the more + # convenient representation as a list. + categories_list = [key for key, val in categories.items() if val] + export_channel_to_kolibri_public( channel_id=channel_id, channel_version=channel_version, public=False, # Community library - categories=categories, + categories=categories_list, countries=countries, ) diff --git a/contentcuration/kolibri_public/models.py b/contentcuration/kolibri_public/models.py index 35b3ede984..7b10c4cee1 100644 --- a/contentcuration/kolibri_public/models.py +++ b/contentcuration/kolibri_public/models.py @@ -97,6 +97,8 @@ class AssessmentMetaData(base_models.AssessmentMetaData): class ChannelMetadata(base_models.ChannelMetadata): + # Note: The `categories` field should contain a _list_, NOT a _dict_. + # precalculated fields during annotation/migration published_size = models.BigIntegerField(default=0, null=True, blank=True) total_resource_count = models.IntegerField(default=0, null=True, blank=True) diff --git a/contentcuration/kolibri_public/tests/test_export_channels_to_kolibri_public.py b/contentcuration/kolibri_public/tests/test_export_channels_to_kolibri_public.py index d0e22d4e2c..d7be71395e 100644 --- a/contentcuration/kolibri_public/tests/test_export_channels_to_kolibri_public.py +++ b/contentcuration/kolibri_public/tests/test_export_channels_to_kolibri_public.py @@ -102,10 +102,7 @@ def tearDown(self): def test_export_channel_to_kolibri_public__existing_version( self, mock_channel_mapper ): - categories = { - "Category1": True, - "Category2": True, - } + categories = ["Category1", "Category2"] country1 = Country.objects.create(code="C1", name="Country 1") country2 = Country.objects.create(code="C2", name="Country 2") countries = [country1, country2] diff --git a/contentcuration/kolibri_public/tests/test_mapper.py b/contentcuration/kolibri_public/tests/test_mapper.py index edefafcd25..500f018c33 100644 --- a/contentcuration/kolibri_public/tests/test_mapper.py +++ b/contentcuration/kolibri_public/tests/test_mapper.py @@ -181,7 +181,7 @@ def test_categories__none_provided(self): mapper = ChannelMapper(self.channel) mapper.run() - self.assertEqual(mapper.mapped_channel.categories, {}) + self.assertEqual(mapper.mapped_channel.categories, []) def test_categories__only_provided(self): with using_content_database(self.tempdb): @@ -189,7 +189,7 @@ def test_categories__only_provided(self): channel_id=self.channel.id, ).update(categories=None) - categories = {"Category1": True, "Category2": True} + categories = ["Category1", "Category2"] mapper = ChannelMapper(self.channel, categories=categories) mapper.run() @@ -216,7 +216,7 @@ def test_categories__only_on_content_nodes(self): self.assertEqual( mapper.mapped_channel.categories, - {"Category1": True, "Category2": True, "Category3": True}, + ["Category1", "Category2", "Category3"], ) def test_categories__both_provided_and_on_content_nodes(self): @@ -235,18 +235,13 @@ def test_categories__both_provided_and_on_content_nodes(self): contentnode1.save() contentnode2.save() - categories = {"Category3": True, "Category4": True} + categories = ["Category3", "Category4"] mapper = ChannelMapper(self.channel, categories=categories) mapper.run() self.assertEqual( mapper.mapped_channel.categories, - { - "Category1": True, - "Category2": True, - "Category3": True, - "Category4": True, - }, + ["Category1", "Category2", "Category3", "Category4"], ) def test_countries__none_provided(self): diff --git a/contentcuration/kolibri_public/utils/annotation.py b/contentcuration/kolibri_public/utils/annotation.py index 1c9a193511..05b97f6c44 100644 --- a/contentcuration/kolibri_public/utils/annotation.py +++ b/contentcuration/kolibri_public/utils/annotation.py @@ -21,6 +21,8 @@ def set_channel_metadata_fields( categories=None, countries=None, ): + # Note: The `categories` argument should be a _list_, NOT a _dict_. + # Remove unneeded db_lock channel = ChannelMetadata.objects.get(id=channel_id) calculate_published_size(channel) @@ -92,9 +94,9 @@ def calculate_included_categories(channel, categories): ) ) ) - categories_dict = {category: True for category in contentnode_categories} - categories_dict.update(categories or {}) - channel.categories = categories_dict + + all_categories = sorted(set(categories or []).union(contentnode_categories)) + channel.categories = all_categories channel.save() diff --git a/contentcuration/kolibri_public/utils/export_channel_to_kolibri_public.py b/contentcuration/kolibri_public/utils/export_channel_to_kolibri_public.py index d751bd6614..ddbc19ca7b 100644 --- a/contentcuration/kolibri_public/utils/export_channel_to_kolibri_public.py +++ b/contentcuration/kolibri_public/utils/export_channel_to_kolibri_public.py @@ -23,6 +23,7 @@ def export_channel_to_kolibri_public( categories=None, countries=None, ): + # Note: The `categories` argument should be a _list_, NOT a _dict_. logger.info("Putting channel {} into kolibri_public".format(channel_id)) if channel_version is not None: diff --git a/contentcuration/kolibri_public/utils/mapper.py b/contentcuration/kolibri_public/utils/mapper.py index bd779ea6d2..3f5efdaeb9 100644 --- a/contentcuration/kolibri_public/utils/mapper.py +++ b/contentcuration/kolibri_public/utils/mapper.py @@ -29,6 +29,9 @@ def __init__( # Note: The argument `channel` is an instance of `kolibri_content.models.ChannelMetadata,` # which belongs to a specific channel version to be exported. Therefore, we do not # need to explicitly pass the channel version as an argument here. + + # Note: The `categories` argument should be a _list_, NOT a _dict_. + self.channel = channel self.public = public self.categories = categories From c2dec4630d7296a98d2ae9d0fd8544273219d704 Mon Sep 17 00:00:00 2001 From: Jakub Komarek Date: Wed, 6 Aug 2025 00:08:40 +0200 Subject: [PATCH 11/11] Move logic to mark live submission to submission model --- contentcuration/contentcuration/models.py | 15 ++++++++ .../contentcuration/tests/test_models.py | 36 +++++++++++++++++++ .../contentcuration/viewsets/channel.py | 12 +------ 3 files changed, 52 insertions(+), 11 deletions(-) diff --git a/contentcuration/contentcuration/models.py b/contentcuration/contentcuration/models.py index b62441402a..ad019b3d2f 100644 --- a/contentcuration/contentcuration/models.py +++ b/contentcuration/contentcuration/models.py @@ -2605,6 +2605,21 @@ def save(self, *args, **kwargs): super().save(*args, **kwargs) + def mark_live(self): + """ + Marks this submission as the live submission for the channel, + and marks any previously live submissions as approved but not live. + """ + CommunityLibrarySubmission.objects.filter( + channel=self.channel, + status=community_library_submission.STATUS_LIVE, + ).update( + status=community_library_submission.STATUS_APPROVED, + ) + + self.status = community_library_submission.STATUS_LIVE + self.save() + @classmethod def filter_view_queryset(cls, queryset, user): if user.is_anonymous: diff --git a/contentcuration/contentcuration/tests/test_models.py b/contentcuration/contentcuration/tests/test_models.py index d70bdc5d8d..55f31e252e 100644 --- a/contentcuration/contentcuration/tests/test_models.py +++ b/contentcuration/contentcuration/tests/test_models.py @@ -689,6 +689,42 @@ def test_filter_edit_queryset__admin(self): ) self.assertQuerysetContains(queryset, pk=submission_a.id) + def test_mark_live(self): + submission_a = testdata.community_library_submission() + submission_b = testdata.community_library_submission() + + channel = submission_a.channel + channel.version = 2 + submission_b.channel = channel + + submission_a.channel_version = 1 + submission_a.status = community_library_submission.STATUS_LIVE + submission_a.save() + + submission_b.channel_version = 2 + submission_b.author = submission_a.author + submission_b.status = community_library_submission.STATUS_APPROVED + submission_b.save() + + submission_other_channel = testdata.community_library_submission() + submission_other_channel.status = community_library_submission.STATUS_LIVE + submission_other_channel.save() + + submission_b.mark_live() + + submission_a.refresh_from_db() + submission_b.refresh_from_db() + submission_other_channel.refresh_from_db() + + self.assertEqual( + submission_a.status, community_library_submission.STATUS_APPROVED + ) + self.assertEqual(submission_b.status, community_library_submission.STATUS_LIVE) + self.assertEqual( + submission_other_channel.status, + community_library_submission.STATUS_LIVE, + ) + class AssessmentItemTestCase(PermissionQuerysetTestCase): @property diff --git a/contentcuration/contentcuration/viewsets/channel.py b/contentcuration/contentcuration/viewsets/channel.py index 4a51b37319..d18f58bb99 100644 --- a/contentcuration/contentcuration/viewsets/channel.py +++ b/contentcuration/contentcuration/viewsets/channel.py @@ -828,22 +828,12 @@ def add_to_community_library( countries=countries, ) - # Mark the submission corresponding to the channel version - # as the only live submission for the channel - CommunityLibrarySubmission.objects.filter( - channel_id=channel_id, - status=community_library_submission_constants.STATUS_LIVE, - ).update( - status=community_library_submission_constants.STATUS_APPROVED, - ) - new_live_submission = CommunityLibrarySubmission.objects.get( channel_id=channel_id, channel_version=channel_version, status=community_library_submission_constants.STATUS_APPROVED, ) - new_live_submission.status = community_library_submission_constants.STATUS_LIVE - new_live_submission.save() + new_live_submission.mark_live() @action( detail=True,