From ced33302ae6a4d1c7318ae3ecc14b1574c7ccbaf Mon Sep 17 00:00:00 2001 From: Alex Velez Date: Thu, 4 Jun 2026 12:12:55 -0500 Subject: [PATCH 01/25] refactor(TipTapEditor): emit markdown on blur and cache last emitted value Previously the editor emitted an `update` on every editor state change via a deep watcher on `editor.state`. This caused frequent re-serialization and required an `isUpdatingFromOutside` flag to break the feedback loop when applying parent-driven content updates. Now the markdown update is emitted only when the editor loses focus (blur), and the latest emitted markdown is cached. When `prop.value` changes, it is compared against the cache so an echoed-back value skips unnecessary re-rendering of the editor content. Co-Authored-By: Claude Opus 4.8 --- .../TipTapEditor/TipTapEditor.vue | 53 ++++++++++--------- 1 file changed, 29 insertions(+), 24 deletions(-) diff --git a/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/TipTapEditor.vue b/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/TipTapEditor.vue index f59e183e86..feb6e348f7 100644 --- a/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/TipTapEditor.vue +++ b/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/TipTapEditor.vue @@ -197,7 +197,10 @@ return editor.value.storage.markdown.getMarkdown(); }; - let isUpdatingFromOutside = false; // A flag to prevent infinite update loops + // Cache of the latest markdown value emitted by this component. Used to + // detect when an incoming prop.value is just our own emitted value echoed + // back, so we can skip unnecessary re-rendering of the editor content. + let lastEmittedMarkdown = null; watch( () => props.mode, @@ -217,6 +220,13 @@ watch( () => props.value, newValue => { + // If the incoming value matches what we last emitted, the editor + // already reflects this content, so skip re-rendering to avoid + // unnecessary work and resetting the editor state. + if (newValue === lastEmittedMarkdown) { + return; + } + const processedContent = preprocessMarkdown(newValue); if (!editor.value) { @@ -228,36 +238,31 @@ const editorContent = getMarkdownContent(); if (editorContent !== newValue) { - isUpdatingFromOutside = true; editor.value.commands.setContent(processedContent, false); - nextTick(() => { - isUpdatingFromOutside = false; - }); } }, { immediate: true }, ); - // sync changes from the editor to the parent component - watch( - () => editor.value?.state, - () => { - if ( - !editor.value || - !isReady.value || - isUpdatingFromOutside || - !editor.value.storage?.markdown - ) { - return; - } + // sync changes from the editor to the parent component, only on blur + const emitMarkdownUpdate = () => { + if (!editor.value || !isReady.value || !editor.value.storage?.markdown) { + return; + } - const markdown = getMarkdownContent(); - if (markdown !== props.value) { - emit('update', markdown); - } - }, - { deep: true }, - ); + const markdown = getMarkdownContent(); + if (markdown !== props.value) { + lastEmittedMarkdown = markdown; + emit('update', markdown); + } + }; + + // Emit the markdown update only when the editor loses focus (blur). + watch(isFocused, (focused, wasFocused) => { + if (wasFocused && !focused) { + emitMarkdownUpdate(); + } + }); const handleContainerKeydown = event => { if (event.key === 'Enter') { From f70930156e225f68bbc7be0c4d3b1719b7b5720f Mon Sep 17 00:00:00 2001 From: Blaine Jester Date: Thu, 11 Jun 2026 13:17:59 -0700 Subject: [PATCH 02/25] Additional coverage for user CSV export --- .../contentcuration/tests/test_user.py | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/contentcuration/contentcuration/tests/test_user.py b/contentcuration/contentcuration/tests/test_user.py index d772007d57..5e7354fe1b 100644 --- a/contentcuration/contentcuration/tests/test_user.py +++ b/contentcuration/contentcuration/tests/test_user.py @@ -16,7 +16,9 @@ from .base import StudioTestCase from .testdata import fileobj_video from contentcuration.models import DEFAULT_CONTENT_DEFAULTS +from contentcuration.models import File from contentcuration.models import Invitation +from contentcuration.models import Language from contentcuration.models import User from contentcuration.models import UserSubscription from contentcuration.tests import testdata @@ -163,6 +165,59 @@ def test_user_csv_export(self): self.assertIn(_format_size(videos[index - 1].file_size), row) self.assertEqual(index, len(videos)) + def test_user_csv_export_reports_channel_and_content_metadata(self): + language = Language.objects.create(lang_code="fr", readable_name="French") + file_record = File.objects.filter( + contentnode__tree_id=self.channel.main_tree.tree_id + ).first() + file_record.uploaded_by = self.user + file_record.original_filename = "sample-video.mp4" + file_record.language = None + file_record.save() + + contentnode = file_record.contentnode + contentnode.title = "CSV Content Title" + contentnode.description = "CSV Description" + contentnode.author = "CSV Author" + contentnode.language = language + contentnode.license_description = "CSV License Description" + contentnode.copyright_holder = "CSV Copyright Holder" + contentnode.save() + + with tempfile.NamedTemporaryFile(suffix=".csv") as tempf: + write_user_csv(self.user, path=tempf.name) + + with io.open(tempf.name, "r", encoding="utf-8") as csv_file: + rows = list(csv.DictReader(csv_file, delimiter=",")) + + self.assertTrue(rows) + row = rows[0] + self.assertEqual(row["Channel"], self.channel.name) + self.assertEqual(row["Title"], "CSV Content Title") + self.assertEqual(row["Filename"], "sample-video.mp4") + self.assertEqual(row["Description"], "CSV Description") + self.assertEqual(row["Author"], "CSV Author") + self.assertEqual(row["Language"], "French") + self.assertEqual(row["License Description"], "CSV License Description") + self.assertEqual(row["Copyright Holder"], "CSV Copyright Holder") + + def test_user_csv_export_reports_staged_files(self): + self.user.staged_files.create(checksum="stagedchecksum", file_size=2048) + + with tempfile.NamedTemporaryFile(suffix=".csv") as tempf: + write_user_csv(self.user, path=tempf.name) + + with io.open(tempf.name, "r", encoding="utf-8") as csv_file: + rows = list(csv.DictReader(csv_file, delimiter=",")) + + staged_rows = [row for row in rows if row["Filename"] == "Staged File"] + self.assertEqual(len(staged_rows), 1) + staged_row = staged_rows[0] + self.assertEqual(staged_row["Channel"], "No Channel") + self.assertEqual(staged_row["Title"], "No Resource") + self.assertEqual(staged_row["File Size"], _format_size(2048)) + self.assertEqual(staged_row["URL"], "") + class UserEffectiveDiskSpaceTest(StudioTestCase): def setUp(self): From 37454108319b201b67b1f19126d6fa9cce4332a6 Mon Sep 17 00:00:00 2001 From: Blaine Jester Date: Thu, 11 Jun 2026 14:56:42 -0700 Subject: [PATCH 03/25] Optimize user CSV export query --- .../contentcuration/utils/csv_writer.py | 136 +++++++++++++----- 1 file changed, 103 insertions(+), 33 deletions(-) diff --git a/contentcuration/contentcuration/utils/csv_writer.py b/contentcuration/contentcuration/utils/csv_writer.py index 0ceaefcd7c..babb5b6850 100644 --- a/contentcuration/contentcuration/utils/csv_writer.py +++ b/contentcuration/contentcuration/utils/csv_writer.py @@ -6,13 +6,17 @@ from django.conf import settings from django.contrib.sites.models import Site +from django.db.models import Exists +from django.db.models import F from django.db.models import OuterRef -from django.db.models import Q from django.db.models import Subquery +from django.db.models.sql.constants import LOUTER from django.utils.translation import gettext as _ from le_utils.constants import content_kinds +from contentcuration.db.models.query import With from contentcuration.models import Channel +from contentcuration.models import ContentNode from contentcuration.models import generate_storage_url if not os.path.exists(settings.CSV_ROOT): @@ -43,29 +47,24 @@ def generate_user_csv_filename(user): def _write_user_row(file, writer, domain): - filename = "{}.{}".format(file["checksum"], file["file_format__extension"]) + filename = "{}.{}".format(file["checksum"], file["file_extension"]) writer.writerow( [ file["channel_name"] or _("No Channel"), - file["contentnode__title"] or _("No resource"), + file["node_title"] or _("No resource"), next( - ( - k[1] - for k in content_kinds.choices - if k[0] == file["contentnode__kind_id"] - ), + (k[1] for k in content_kinds.choices if k[0] == file["node_kind_id"]), "", ), file["original_filename"], _format_size(file["file_size"] or 0), generate_storage_url(filename), - file["contentnode__description"], - file["contentnode__author"], - file["language__readable_name"] - or file["contentnode__language__readable_name"], - file["contentnode__license__license_name"], - file["contentnode__license_description"], - file["contentnode__copyright_holder"], + file["node_description"], + file["node_author"], + file["file_language"] or file["node_language"], + file["node_license_name"], + file["node_license_description"], + file["node_copyright_holder"], ] ) @@ -100,34 +99,105 @@ def write_user_csv(user, path=None): domain = Site.objects.get(pk=1).domain - # Get all user files - channel_query = Channel.objects.filter( - Q(main_tree__tree_id=OuterRef("contentnode__tree_id")) - | Q(trash_tree__tree_id=OuterRef("contentnode__tree_id")) + # Build CTEs so we first reduce to this user's files, then resolve only + # needed content node and channel fields. + user_files_cte = With( + user.files.values( + "id", + "contentnode_id", + "original_filename", + "file_size", + "checksum", + file_extension=F("file_format__extension"), + file_language=F("language__readable_name"), + ), + name="user_files", + ) + + content_nodes_cte = With( + user_files_cte.join( + ContentNode.objects.all(), + id=user_files_cte.col.contentnode_id, + ) + .values( + "id", + "tree_id", + node_title=F("title"), + node_kind_id=F("kind_id"), + node_description=F("description"), + node_author=F("author"), + node_language=F("language__readable_name"), + node_license_name=F("license__license_name"), + node_license_description=F("license_description"), + node_copyright_holder=F("copyright_holder"), + ) + .distinct(), + name="content_nodes", + ) + + main_channel_names = Channel.objects.filter( + Exists( + content_nodes_cte.queryset().filter( + tree_id=OuterRef("main_tree__tree_id") + ) + ) + ).values( + tree_id=F("main_tree__tree_id"), + channel_name=F("name"), + ) + trash_channel_names = Channel.objects.filter( + Exists( + content_nodes_cte.queryset().filter( + tree_id=OuterRef("trash_tree__tree_id") + ) + ) + ).values( + tree_id=F("trash_tree__tree_id"), + channel_name=F("name"), + ) + channel_names_cte = With( + main_channel_names.union(trash_channel_names), name="channel_names" ) user_files = ( - user.files.select_related("language", "contentnode", "file_format") + content_nodes_cte.join( + user_files_cte.queryset(), + contentnode_id=content_nodes_cte.col.id, + _join_type=LOUTER, + ) + .with_cte(user_files_cte) + .with_cte(content_nodes_cte) + .with_cte(channel_names_cte) .annotate( - channel_name=Subquery(channel_query.values_list("name", flat=True)[:1]) + channel_name=Subquery( + channel_names_cte.queryset() + .filter(tree_id=content_nodes_cte.col.tree_id) + .values("channel_name")[:1] + ), + node_title=content_nodes_cte.col.node_title, + node_kind_id=content_nodes_cte.col.node_kind_id, + node_description=content_nodes_cte.col.node_description, + node_author=content_nodes_cte.col.node_author, + node_language=content_nodes_cte.col.node_language, + node_license_name=content_nodes_cte.col.node_license_name, + node_license_description=content_nodes_cte.col.node_license_description, + node_copyright_holder=content_nodes_cte.col.node_copyright_holder, ) .values( "channel_name", "original_filename", "file_size", "checksum", - "file_format__extension", - "language__readable_name", - "contentnode__title", - "contentnode__language__readable_name", - "contentnode__license__license_name", - "contentnode__kind_id", - "contentnode__description", - "contentnode__author", - "contentnode__provider", - "contentnode__aggregator", - "contentnode__license_description", - "contentnode__copyright_holder", + "file_extension", + "file_language", + "node_title", + "node_kind_id", + "node_description", + "node_author", + "node_language", + "node_license_name", + "node_license_description", + "node_copyright_holder", ) ) for file in user_files: From eec65c0bffac795e8cda3601eaf1cbcef3f88612 Mon Sep 17 00:00:00 2001 From: Blaine Jester Date: Thu, 11 Jun 2026 15:37:46 -0700 Subject: [PATCH 04/25] Add test asserting join behavior --- .../contentcuration/tests/test_user.py | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/contentcuration/contentcuration/tests/test_user.py b/contentcuration/contentcuration/tests/test_user.py index 5e7354fe1b..585932aa9c 100644 --- a/contentcuration/contentcuration/tests/test_user.py +++ b/contentcuration/contentcuration/tests/test_user.py @@ -218,6 +218,27 @@ def test_user_csv_export_reports_staged_files(self): self.assertEqual(staged_row["File Size"], _format_size(2048)) self.assertEqual(staged_row["URL"], "") + def test_user_csv_export_includes_files_without_contentnode(self): + file_without_contentnode = fileobj_video() + self.assertIsNone(file_without_contentnode.contentnode_id) + file_without_contentnode.uploaded_by = self.user + file_without_contentnode.original_filename = "no-contentnode.mp4" + file_without_contentnode.save() + + with tempfile.NamedTemporaryFile(suffix=".csv") as tempf: + write_user_csv(self.user, path=tempf.name) + + with io.open(tempf.name, "r", encoding="utf-8") as csv_file: + rows = list(csv.DictReader(csv_file, delimiter=",")) + + row = next( + row + for row in rows + if row["Filename"] == file_without_contentnode.original_filename + ) + self.assertEqual(row["Title"], "No resource") + self.assertEqual(row["Channel"], "No Channel") + class UserEffectiveDiskSpaceTest(StudioTestCase): def setUp(self): From ae076bc2858bc8c0403a6abea5150a7500a8776b Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Thu, 25 Jun 2026 20:55:38 -0700 Subject: [PATCH 05/25] Route admin users from notification into submission review mode Gate SubmissionDetailsModal review actions on isAdmin from the Vuex store rather than an adminReview prop, so admin users arriving via any route automatically see the Review button and side panel. Remove the adminReview prop and all call sites that set it; simplify administration/router.js to props: true. --- .../frontend/administration/router.js | 6 +- .../__tests__/index.spec.js | 81 +++++++++++++++++++ .../SubmissionDetailsModal/index.vue | 13 ++- 3 files changed, 87 insertions(+), 13 deletions(-) create mode 100644 contentcuration/contentcuration/frontend/shared/views/communityLibrary/SubmissionDetailsModal/__tests__/index.spec.js diff --git a/contentcuration/contentcuration/frontend/administration/router.js b/contentcuration/contentcuration/frontend/administration/router.js index d5de3506f2..af9b716776 100644 --- a/contentcuration/contentcuration/frontend/administration/router.js +++ b/contentcuration/contentcuration/frontend/administration/router.js @@ -34,11 +34,7 @@ const router = new VueRouter({ name: RouteNames.COMMUNITY_LIBRARY_SUBMISSION, path: '/community-library/:channelId/:submissionId', component: SubmissionDetailsModal, - props: route => ({ - channelId: route.params.channelId, - submissionId: route.params.submissionId, - adminReview: true, - }), + props: true, }, // Catch-all redirect to channels tab { diff --git a/contentcuration/contentcuration/frontend/shared/views/communityLibrary/SubmissionDetailsModal/__tests__/index.spec.js b/contentcuration/contentcuration/frontend/shared/views/communityLibrary/SubmissionDetailsModal/__tests__/index.spec.js new file mode 100644 index 0000000000..54d19d794c --- /dev/null +++ b/contentcuration/contentcuration/frontend/shared/views/communityLibrary/SubmissionDetailsModal/__tests__/index.spec.js @@ -0,0 +1,81 @@ +import { shallowMount, createLocalVue } from '@vue/test-utils'; +import Vuex from 'vuex'; +import VueRouter from 'vue-router'; +import SubmissionDetailsModal from '../index.vue'; +import { + AdminCommunityLibrarySubmission, + ChannelVersion, + CommunityLibrarySubmission, +} from 'shared/data/resources'; + +jest.mock('shared/data/resources', () => ({ + AdminCommunityLibrarySubmission: { fetchModel: jest.fn() }, + ChannelVersion: { fetchCollection: jest.fn() }, + CommunityLibrarySubmission: { + fetchModel: jest.fn(), + fetchCollection: jest.fn(() => Promise.resolve({ results: [] })), + }, +})); + +const localVue = createLocalVue(); +localVue.use(Vuex); +localVue.use(VueRouter); + +const stubChannel = { + id: 'ch1', + name: 'Test', + thumbnail_url: null, + thumbnail_encoding: null, + description: '', +}; +const stubSubmission = { + id: 'sub1', + channel_id: 'ch1', + channel_version: 1, + status: 'PENDING', + version_token: null, +}; +const stubChannelVersion = { id: 'cv1' }; + +function makeStore(isAdmin) { + return new Vuex.Store({ + getters: { isAdmin: () => isAdmin }, + modules: { + channel: { + namespaced: true, + actions: { loadChannel: jest.fn(() => Promise.resolve(stubChannel)) }, + }, + errors: { namespaced: true, actions: { handleAxiosError: jest.fn() } }, + }, + }); +} + +describe('SubmissionDetailsModal', () => { + beforeEach(() => { + AdminCommunityLibrarySubmission.fetchModel.mockResolvedValue(stubSubmission); + CommunityLibrarySubmission.fetchModel.mockResolvedValue(stubSubmission); + ChannelVersion.fetchCollection.mockResolvedValue([stubChannelVersion]); + }); + + afterEach(() => jest.clearAllMocks()); + + it('uses AdminCommunityLibrarySubmission when user is admin', () => { + shallowMount(SubmissionDetailsModal, { + localVue, + store: makeStore(true), + router: new VueRouter(), + propsData: { channelId: 'ch1', submissionId: 'sub1' }, + }); + expect(AdminCommunityLibrarySubmission.fetchModel).toHaveBeenCalledWith('sub1'); + }); + + it('uses CommunityLibrarySubmission when user is not admin', () => { + shallowMount(SubmissionDetailsModal, { + localVue, + store: makeStore(false), + router: new VueRouter(), + propsData: { channelId: 'ch1', submissionId: 'sub1' }, + }); + expect(CommunityLibrarySubmission.fetchModel).toHaveBeenCalledWith('sub1'); + }); +}); diff --git a/contentcuration/contentcuration/frontend/shared/views/communityLibrary/SubmissionDetailsModal/index.vue b/contentcuration/contentcuration/frontend/shared/views/communityLibrary/SubmissionDetailsModal/index.vue index a0393fc8cb..30a460119b 100644 --- a/contentcuration/contentcuration/frontend/shared/views/communityLibrary/SubmissionDetailsModal/index.vue +++ b/contentcuration/contentcuration/frontend/shared/views/communityLibrary/SubmissionDetailsModal/index.vue @@ -74,12 +74,12 @@
@@ -105,7 +105,7 @@ :channelId="channelId" /> store.getters.isAdmin); const { windowBreakpoint } = useKResponsiveWindow(); const isModalOpen = computed({ @@ -206,7 +203,7 @@ } = useFetch({ asyncFetchFunc: async () => { try { - const Resource = props.adminReview + const Resource = isAdmin.value ? AdminCommunityLibrarySubmission : CommunityLibrarySubmission; const submission = await Resource.fetchModel(props.submissionId); From 101e2001be2a58248cfd12e41566ea595a27a097 Mon Sep 17 00:00:00 2001 From: Richard Tibbles Date: Mon, 22 Jun 2026 21:43:20 -0700 Subject: [PATCH 06/25] feat(migrations): add almost zero-downtime migration capability and linting - CI linting of new migrations on pull requests - Declarative dual-write trigger decorator (mirror_field) - Reusable batched-backfill command (idempotent, resumable, throttled) Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01LfZvkigk8hdsKdEif3hzBi --- .github/workflows/pythontest.yml | 13 ++ .../contentcuration/db/dual_write.py | 31 +++ .../management/commands/backfill_column.py | 100 +++++++++ .../not_production_settings.py | 9 + contentcuration/contentcuration/settings.py | 1 + .../tests/test_backfill_column.py | 191 ++++++++++++++++++ .../contentcuration/tests/test_dual_write.py | 51 +++++ requirements-dev.in | 1 + requirements-dev.txt | 9 +- requirements.in | 1 + requirements.txt | 5 +- 11 files changed, 410 insertions(+), 2 deletions(-) create mode 100644 contentcuration/contentcuration/db/dual_write.py create mode 100644 contentcuration/contentcuration/management/commands/backfill_column.py create mode 100644 contentcuration/contentcuration/tests/test_backfill_column.py create mode 100644 contentcuration/contentcuration/tests/test_dual_write.py diff --git a/.github/workflows/pythontest.yml b/.github/workflows/pythontest.yml index e77c613e69..609a0a933d 100644 --- a/.github/workflows/pythontest.yml +++ b/.github/workflows/pythontest.yml @@ -62,6 +62,8 @@ jobs: - 6379:6379 steps: - uses: actions/checkout@v6 + with: + fetch-depth: 0 - name: Set up minio run: | docker run -d -p 9000:9000 --name minio \ @@ -79,6 +81,17 @@ jobs: run: | # Use uv to install dependencies directly from requirements files uv pip sync requirements.txt requirements-dev.txt + - name: Lint new migrations for unsafe operations + if: github.event_name == 'pull_request' + env: + BASE_REF: ${{ github.base_ref }} + DJANGO_SETTINGS_MODULE: contentcuration.not_production_settings + run: | + set -euo pipefail + git fetch --no-tags origin "$BASE_REF" + base="$(git merge-base "origin/$BASE_REF" HEAD)" + test -n "$base" + python contentcuration/manage.py lintmigrations --git-commit-id "$base" --no-cache --warnings-as-errors - name: Test pytest run: | sh -c './contentcuration/manage.py makemigrations --check' diff --git a/contentcuration/contentcuration/db/dual_write.py b/contentcuration/contentcuration/db/dual_write.py new file mode 100644 index 0000000000..604875670e --- /dev/null +++ b/contentcuration/contentcuration/db/dual_write.py @@ -0,0 +1,31 @@ +import hashlib + +import pgtrigger + + +def mirror_field(source, target): + """Mirror Django field `source` into `target` via a BEFORE INSERT/UPDATE + trigger (expand/contract dual-write).""" + + def decorator(model): + source_col = model._meta.get_field(source).column + target_col = model._meta.get_field(target).column + name = "mirror_{}_to_{}".format(source_col, target_col) + if len(name) > 43: # stay safely under pgtrigger's trigger-name limit + digest = hashlib.sha1( + "{}_{}".format(source_col, target_col).encode() + ).hexdigest()[:8] + name = "mirror_{}".format(digest) + # Change-guard (IS DISTINCT FROM): keeps a read cutover from clobbering + # writes to the repointed column with the stale source value. + trigger = pgtrigger.Trigger( + name=name, + when=pgtrigger.Before, + operation=pgtrigger.Insert | pgtrigger.Update, + func="IF NEW.{s} IS DISTINCT FROM OLD.{s} THEN NEW.{t} = NEW.{s}; END IF; RETURN NEW;".format( + s=source_col, t=target_col + ), + ) + return pgtrigger.register(trigger)(model) + + return decorator diff --git a/contentcuration/contentcuration/management/commands/backfill_column.py b/contentcuration/contentcuration/management/commands/backfill_column.py new file mode 100644 index 0000000000..1920c47af1 --- /dev/null +++ b/contentcuration/contentcuration/management/commands/backfill_column.py @@ -0,0 +1,100 @@ +from django.apps import apps +from django.core.exceptions import FieldDoesNotExist +from django.core.management.base import BaseCommand +from django.core.management.base import CommandError +from django.db import transaction +from django.db.models import F + + +class Command(BaseCommand): + help = ( + "Idempotent, resumable online backfill of one column into another, in batches." + ) + + def add_arguments(self, parser): + parser.add_argument("--model", required=True, help="app_label.ModelName") + parser.add_argument("--source-field", required=True) + parser.add_argument("--target-field", required=True) + parser.add_argument("--batch-size", type=int, default=10000) + parser.add_argument("--start-id", default=None, help="resume from this pk") + parser.add_argument( + "--progress-check", + action="store_true", + help="report unbackfilled rows, exit nonzero if any", + ) + + def _resolve_model_fields(self, model_label, source, target): + try: + model = apps.get_model(model_label) + except (LookupError, ValueError) as e: + raise CommandError("Bad --model {!r}: {}".format(model_label, e)) + try: + model._meta.get_field(source) + model._meta.get_field(target) + except FieldDoesNotExist as e: + raise CommandError(str(e)) + return model + + def _batch_end_pk(self, queryset, pk_name, start_pk, batch_size): + """Last pk of the batch of `batch_size` rows starting at `start_pk`. + + Returns None when fewer than `batch_size` rows remain at/after + `start_pk` — the final, short batch. Keyset paging by pk, so it works + for any pk type (int or UUID). + """ + return ( + queryset.filter(pk__gte=start_pk) + .order_by(pk_name) + .values_list("pk", flat=True)[batch_size - 1 : batch_size] + .first() + ) + + def handle(self, *args, **options): + if options["batch_size"] < 1: + raise CommandError("--batch-size must be >= 1") + source = options["source_field"] + target = options["target_field"] + model = self._resolve_model_fields(options["model"], source, target) + + pk_name = model._meta.pk.name + batch_size = options["batch_size"] + only_unfilled = {target + "__isnull": True, source + "__isnull": False} + unfilled = model.objects.filter(**only_unfilled) + unfilled_pks = unfilled.order_by(pk_name).values_list("pk", flat=True) + + if options["progress_check"]: + # exists(), not count() — the target table can have millions of rows. + if unfilled.exists(): + raise CommandError("backfill incomplete: rows still pending") + self.stdout.write("Backfill complete: no rows pending.") + return + + # Start at the first unfilled pk (>= --start-id if given); re-runs and + # resumes skip straight past an already-filled prefix. + batch_start = unfilled_pks + if options["start_id"] is not None: + batch_start = batch_start.filter(pk__gte=options["start_id"]) + batch_start = batch_start.first() + + total = 0 + while batch_start is not None: + batch_end = self._batch_end_pk( + model.objects, pk_name, batch_start, batch_size + ) + if batch_end is None: + window = {"pk__gte": batch_start} + else: + window = {"pk__gte": batch_start, "pk__lte": batch_end} + with transaction.atomic(): + total += model.objects.filter(**window, **only_unfilled).update( + **{target: F(source)} + ) + self.stdout.write( + "backfilled through pk={} (updated {} so far)".format( + batch_start if batch_end is None else batch_end, total + ) + ) + if batch_end is None: + break + batch_start = unfilled_pks.filter(pk__gt=batch_end).first() + self.stdout.write("Done. {} rows updated.".format(total)) diff --git a/contentcuration/contentcuration/not_production_settings.py b/contentcuration/contentcuration/not_production_settings.py index afcc6460bc..35be6db3c2 100644 --- a/contentcuration/contentcuration/not_production_settings.py +++ b/contentcuration/contentcuration/not_production_settings.py @@ -20,5 +20,14 @@ AWS_AUTO_CREATE_BUCKET = True +INSTALLED_APPS += ("django_migration_linter",) # noqa F405 + +MIGRATION_LINTER_OPTIONS = { + "exclude_apps": [ + "kolibri_content" + ], # SQLite content-export app; not on the safe-DDL Postgres backend + "sql_analyser": "postgresql", +} + # Use local instance for curriculum automation for development CURRICULUM_AUTOMATION_API_URL = "http://localhost:8000" diff --git a/contentcuration/contentcuration/settings.py b/contentcuration/contentcuration/settings.py index 2d8bafaa9b..c726789b4e 100644 --- a/contentcuration/contentcuration/settings.py +++ b/contentcuration/contentcuration/settings.py @@ -92,6 +92,7 @@ "django_celery_results", "kolibri_public", "automation", + "pgtrigger", ) SESSION_ENGINE = "django.contrib.sessions.backends.cached_db" diff --git a/contentcuration/contentcuration/tests/test_backfill_column.py b/contentcuration/contentcuration/tests/test_backfill_column.py new file mode 100644 index 0000000000..467b967f0f --- /dev/null +++ b/contentcuration/contentcuration/tests/test_backfill_column.py @@ -0,0 +1,191 @@ +import uuid +from io import StringIO +from unittest.mock import patch + +from django.core.management import call_command +from django.core.management import CommandError +from django.db import connection +from django.db import models +from django.db.models import F +from django.test import SimpleTestCase +from django.test import TransactionTestCase +from django.test.utils import isolate_apps + + +def _make_probe_class(): + class Probe(models.Model): + source = models.IntegerField(null=True) + shadow = models.IntegerField(null=True) + + class Meta: + app_label = "contentcuration" + + return Probe + + +def _make_uuid_probe_class(): + class UUIDProbe(models.Model): + id = models.UUIDField(primary_key=True, default=uuid.uuid4) + source = models.IntegerField(null=True) + shadow = models.IntegerField(null=True) + + class Meta: + app_label = "contentcuration" + + return UUIDProbe + + +@isolate_apps("contentcuration") +class BackfillColumnTestCase(TransactionTestCase): + def _create_model(self, model): + with connection.schema_editor(atomic=False) as editor: + editor.create_model(model) + self.addCleanup(self._delete_model, model) + + def _delete_model(self, model): + with connection.schema_editor(atomic=False) as editor: + editor.delete_model(model) + + def _run_backfill(self, model, **kwargs): + out = StringIO() + with patch( + "contentcuration.management.commands.backfill_column.apps", + model._meta.apps, + ): + call_command( + "backfill_column", + stdout=out, + model="contentcuration.{}".format(model.__name__), + source_field="source", + target_field="shadow", + **kwargs, + ) + return out.getvalue() + + def _assert_all_synced(self, model): + self.assertEqual( + model.objects.count(), model.objects.filter(shadow=F("source")).count() + ) + + def _assert_synced_from(self, model, resume_pk): + below = model.objects.filter(pk__lt=resume_pk) + at_or_above = model.objects.filter(pk__gte=resume_pk) + self.assertEqual(below.count(), below.filter(shadow__isnull=True).count()) + self.assertEqual( + at_or_above.count(), at_or_above.filter(shadow=F("source")).count() + ) + + def test_backfills_all_rows(self): + Probe = _make_probe_class() + self._create_model(Probe) + for i in range(1, 6): + Probe.objects.create(source=i * 10, shadow=None) + + # batch_size=2 over 5 rows exercises the multi-batch loop. + self._run_backfill(Probe, batch_size=2) + + self._assert_all_synced(Probe) + + def test_idempotent(self): + Probe = _make_probe_class() + self._create_model(Probe) + for i in range(1, 4): + Probe.objects.create(source=i * 10, shadow=None) + + self._run_backfill(Probe, batch_size=10) + output = self._run_backfill(Probe, batch_size=10) + + self.assertIn("Done. 0 rows updated.", output) + self._assert_all_synced(Probe) + + def test_resumable(self): + Probe = _make_probe_class() + self._create_model(Probe) + objs = sorted( + [Probe.objects.create(source=i * 10, shadow=None) for i in range(1, 6)], + key=lambda o: o.pk, + ) + resume_pk = objs[2].pk + + self._run_backfill(Probe, batch_size=10, start_id=resume_pk) + + self._assert_synced_from(Probe, resume_pk) + + def test_null_source_safe(self): + Probe = _make_probe_class() + self._create_model(Probe) + Probe.objects.create(source=None, shadow=None) + Probe.objects.create(source=42, shadow=None) + + output = self._run_backfill(Probe, batch_size=10) + + self.assertIn("Done.", output) + self.assertIsNone(Probe.objects.get(source__isnull=True).shadow) + self.assertEqual(Probe.objects.get(source=42).shadow, 42) + + def test_backfills_uuid_pk_across_batches(self): + """Regression: paging must not assume an integer pk (File has a UUID pk).""" + UUIDProbe = _make_uuid_probe_class() + self._create_model(UUIDProbe) + for i in range(1, 6): + UUIDProbe.objects.create(source=i * 10, shadow=None) + + # batch_size=2 forces the lower-bound advance where integer arithmetic on a + # UUID pk would blow up. + self._run_backfill(UUIDProbe, batch_size=2) + + self._assert_all_synced(UUIDProbe) + + def test_resumable_uuid_pk(self): + """--start-id must accept a UUID and resume from it.""" + UUIDProbe = _make_uuid_probe_class() + self._create_model(UUIDProbe) + objs = sorted( + [UUIDProbe.objects.create(source=i * 10, shadow=None) for i in range(1, 6)], + key=lambda o: o.pk, + ) + resume_pk = objs[2].pk + + self._run_backfill(UUIDProbe, batch_size=10, start_id=str(resume_pk)) + + self._assert_synced_from(UUIDProbe, resume_pk) + + def test_progress_check_passes_when_complete(self): + Probe = _make_probe_class() + self._create_model(Probe) + Probe.objects.create(source=1, shadow=1) + Probe.objects.create(source=None, shadow=None) # null source doesn't count + + output = self._run_backfill(Probe, progress_check=True) + + self.assertIn("no rows pending", output) + + def test_progress_check_fails_and_writes_nothing_when_incomplete(self): + Probe = _make_probe_class() + self._create_model(Probe) + Probe.objects.create(source=7, shadow=None) + + with self.assertRaisesRegex( + CommandError, "backfill incomplete: rows still pending" + ): + self._run_backfill(Probe, progress_check=True) + + # --progress-check must not write + self.assertIsNone(Probe.objects.get(source=7).shadow) + + +class BackfillColumnArgValidationTestCase(SimpleTestCase): + def _call(self, **kwargs): + call_command( + "backfill_column", + model="contentcuration.Channel", + source_field="name", + target_field="name", + **kwargs, + ) + + def test_non_positive_batch_size_raises(self): + for bad in (0, -1): + with self.subTest(batch_size=bad): + with self.assertRaisesRegex(CommandError, "--batch-size must be >= 1"): + self._call(batch_size=bad) diff --git a/contentcuration/contentcuration/tests/test_dual_write.py b/contentcuration/contentcuration/tests/test_dual_write.py new file mode 100644 index 0000000000..f123b39ad8 --- /dev/null +++ b/contentcuration/contentcuration/tests/test_dual_write.py @@ -0,0 +1,51 @@ +from django.db import connection +from django.db import models +from django.test import TransactionTestCase +from django.test.utils import isolate_apps + +from contentcuration.db.dual_write import mirror_field + + +@isolate_apps("contentcuration") +class MirrorFieldTestCase(TransactionTestCase): + def _delete_model(self, model): + with connection.schema_editor(atomic=False) as editor: + editor.delete_model(model) + + def test_long_field_names_truncate_trigger_name(self): + """Trigger name from long field names is truncated, not raised.""" + + @mirror_field("a_very_long_source_field_name", "a_very_long_target_field_name") + class LongNameProbe(models.Model): + a_very_long_source_field_name = models.IntegerField() + a_very_long_target_field_name = models.IntegerField(null=True) + + class Meta: + app_label = "contentcuration" + + self.assertEqual(len(LongNameProbe._meta.triggers), 1) + self.assertLessEqual(len(LongNameProbe._meta.triggers[0].name), 43) + + def test_syncs_shadow_column_in_db(self): + @mirror_field("source", "shadow") + class Probe(models.Model): + source = models.IntegerField() + shadow = models.IntegerField(null=True) + + class Meta: + app_label = "contentcuration" + + with connection.schema_editor(atomic=False) as editor: + editor.create_model(Probe) + self.addCleanup(self._delete_model, Probe) + for trigger in Probe._meta.triggers: + trigger.install(Probe) + + obj = Probe.objects.create(source=5) + obj.refresh_from_db() + self.assertEqual(obj.shadow, 5) + + obj.source = 9 + obj.save() + obj.refresh_from_db() + self.assertEqual(obj.shadow, 9) diff --git a/requirements-dev.in b/requirements-dev.in index 8a9224102c..7cd6acd113 100644 --- a/requirements-dev.in +++ b/requirements-dev.in @@ -8,3 +8,4 @@ pytest-timeout pre-commit==4.5.1 nodeenv drf-yasg==1.21.10 +django-migration-linter==6.0.0 diff --git a/requirements-dev.txt b/requirements-dev.txt index ac6539f932..7391a7cb93 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -1,5 +1,7 @@ # This file was autogenerated by uv via the following command: -# uv pip compile requirements-dev.in --output-file requirements-dev.txt +# uv pip compile requirements-dev.in -o requirements-dev.txt +appdirs==1.4.4 + # via django-migration-linter asgiref==3.3.4 # via # -c requirements.txt @@ -11,10 +13,13 @@ distlib==0.3.9 django==3.2.24 # via # -c requirements.txt + # django-migration-linter # djangorestframework # drf-yasg django-concurrent-test-helper==0.7.0 # via -r requirements-dev.in +django-migration-linter==6.0.0 + # via -r requirements-dev.in djangorestframework==3.15.1 # via # -c requirements.txt @@ -91,6 +96,8 @@ sqlparse==0.4.1 # django tblib==1.7.0 # via django-concurrent-test-helper +toml==0.10.2 + # via django-migration-linter tomli==1.2.3 # via pytest typing-extensions==4.15.0 diff --git a/requirements.in b/requirements.in index 2d59962414..df5bd6d494 100644 --- a/requirements.in +++ b/requirements.in @@ -39,3 +39,4 @@ pydantic==2.12.5 latex2mathml==3.78.1 markdown-it-py==4.0.0 stripe>=5.0.0,<6.0.0 +django-pgtrigger<4.12.0 # 4.12 drops Django 3.2 support diff --git a/requirements.txt b/requirements.txt index 0d66015b7a..43044dfb9d 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,5 +1,5 @@ # This file was autogenerated by uv via the following command: -# uv pip compile requirements.in --output-file requirements.txt +# uv pip compile requirements.in -o requirements.txt amqp==5.1.1 # via kombu annotated-types==0.7.0 @@ -58,6 +58,7 @@ django==3.2.24 # django-filter # django-js-reverse # django-model-utils + # django-pgtrigger # django-redis # django-registration # django-s3-storage @@ -79,6 +80,8 @@ django-model-utils==5.0.0 # via -r requirements.in django-mptt==0.16.0 # via -r requirements.in +django-pgtrigger==4.11.0 + # via -r requirements.in django-postmark==0.1.6 # via -r requirements.in django-prometheus==2.3.1 From 659caccf970e4f36a1dc3f2e2a6b5bfd74229583 Mon Sep 17 00:00:00 2001 From: Richard Tibbles Date: Mon, 22 Jun 2026 21:43:47 -0700 Subject: [PATCH 07/25] docs(migrations): add expand/contract zero-downtime runbook Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01LfZvkigk8hdsKdEif3hzBi --- docs/_index.md | 4 ++ docs/zero_downtime_migrations.md | 104 +++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+) create mode 100644 docs/zero_downtime_migrations.md diff --git a/docs/_index.md b/docs/_index.md index c3e67006ba..072419ef13 100644 --- a/docs/_index.md +++ b/docs/_index.md @@ -17,6 +17,10 @@ - [Docker + Kubernetes Studio Instance Setup](./docker_kubernetes_setup.md) +## Database + +- [Zero-downtime migrations (expand/contract runbook)](./zero_downtime_migrations.md) + ## API - [API Endpoints](./api_endpoints.md) diff --git a/docs/zero_downtime_migrations.md b/docs/zero_downtime_migrations.md new file mode 100644 index 0000000000..0b921bfc3d --- /dev/null +++ b/docs/zero_downtime_migrations.md @@ -0,0 +1,104 @@ +# Almost zero-downtime migrations — expand/contract runbook + +On large tables (e.g. `File` has ~100 M rows) a single migration can cause downtime in two ways: +- by taking an `ACCESS EXCLUSIVE` lock / rewriting the table +- by shipping a schema the still-running old pods can't use (a dropped or renamed column). + +The expand/contract procedure below avoids both. Its one residual cost is the brief metadata-only lock taken for the drop + rename migration, hence "almost." + +## Linting (already configured) + +- `django-migration-linter` - flags backward-incompatible schema (drops, renames, NOT NULL adds) old pods would break on. + +## Procedure + +Goal: widen `File.file_size` from int to bigint with no table rewrite and no backward-incompatible window. The app-visible column stays named `file_size` throughout. Only its underlying storage swaps — from the int column to a pre-backfilled bigint column. Because the name is preserved, old pods keep writing to `file_size` (now bigint) without error. + +### Release 1 — expand + +Add the shadow field and the dual-write trigger: + +```python +from contentcuration.db.dual_write import mirror_field + +@mirror_field("file_size", "file_size_bigint") +class File(models.Model): + file_size = models.IntegerField(blank=True, null=True) + file_size_bigint = models.BigIntegerField(blank=True, null=True) +``` + +`makemigrations` emits a nullable `AddField` and the `CreateTrigger` — both safe (no rewrite, no lock). New writes now land in both columns. + +Backfill old rows in the same release: wire `backfill_column` as a `deploy-migrate` step in the Makefile, which runs after `migrate`, so the column and trigger already exist: + +```bash +python contentcuration/manage.py backfill_column \ + --model contentcuration.File --source-field file_size --target-field file_size_bigint +``` + +Can also run the above command with `--progress-check` as a read only to see if any backfills are still required. + +### Release 2 — swap (cutover + rename) + +After backfill completes, swap the storage in a single migration. Drop the shadow field and decorator; `file_size` is now bigint: + +```python +class File(models.Model): + file_size = models.BigIntegerField(blank=True, null=True) +``` + +The migration drops the trigger and the int column, then renames the bigint column onto `file_size`: + +```python +operations = [ + IgnoreMigration(), # safe: net change is an int->bigint widening; see note below + migrations.SeparateDatabaseAndState( + state_operations=[ + migrations.RemoveField("file", "file_size_bigint"), + migrations.AlterField( + "file", "file_size", models.BigIntegerField(blank=True, null=True) + ), + pgtrigger.migrations.RemoveTrigger( + "file", "mirror_file_size_to_file_size_bigint" + ), + ], + database_operations=[ + migrations.RunSQL( + sql=( + "DROP TRIGGER IF EXISTS pgtrigger_mirror_file_size_to_file_size_bigint_54326" + " ON contentcuration_file;" + 'ALTER TABLE contentcuration_file DROP COLUMN "file_size";' + 'ALTER TABLE contentcuration_file RENAME COLUMN "file_size_bigint" TO "file_size";' + ), + reverse_sql=( + 'ALTER TABLE contentcuration_file RENAME COLUMN "file_size" TO "file_size_bigint";' + 'ALTER TABLE contentcuration_file ADD COLUMN "file_size" integer;' + ), + ), + ], + ), +] +``` + +`SeparateDatabaseAndState` allows us to let Django know what has been migrated, while doing specific raw SQL operations to get the exact data preserving sequence of events that we want. Copy the trigger `pgid` from release 1's `AddTrigger`. + +Why the swap is transparent to old pods: + +- Their queries reference `file_size` by name; the swap preserves that name, so they keep working — their int writes fit the bigint column. +- The net app-visible change is an `int → bigint` widening, which is backward-compatible. +- The only disruption is the brief metadata-only lock while the DDL runs; `DROP COLUMN` / `RENAME COLUMN` don't rewrite the table. + +The linter flags the drop and rename as backward-incompatible; `IgnoreMigration()` acknowledges the sequencing makes them safe. + +**Don't cut over to the physical name first.** Aliasing the ORM field to `file_size_bigint` via `db_column` creates a pod generation that queries `file_size_bigint` by name. The later rename then breaks that generation for the whole rollover, and adds a release. Preserving `file_size` is what makes the rename free. + +## Tooling + +- **`@mirror_field(source, target)`** in `contentcuration/db/dual_write.py` — BEFORE INSERT/UPDATE trigger copying field `source` → `target`. Change-guarded: an unconditional copy corrupts data at swap. +- **`backfill_column`** — idempotent, resumable (`--start-id `), batched (`--batch-size`); one transaction per batch. `--progress-check` tests for remaining rows without writing and exits nonzero if any remain. +- **`lintmigrations`** — run locally before pushing: + ```bash + python contentcuration/manage.py lintmigrations --git-commit-id --no-cache --warnings-as-errors + ``` + `--git-commit-id` is a flag, not positional — a positional value is read as an app label and lints nothing. +- **`IgnoreMigration()`** — escape hatch for a migration whose backward-incompatibility is made safe by release sequencing. From 2c079c04b39e2c4413202576ae445748be99d096 Mon Sep 17 00:00:00 2001 From: Richard Tibbles Date: Mon, 22 Jun 2026 23:26:15 -0700 Subject: [PATCH 08/25] feat(models): widen File.file_size to bigint, expand stage (studio#5974) Expand stage of the zero-downtime int->bigint widening: - Add nullable file_size_bigint shadow column and its index (built CONCURRENTLY). - Mirror file_size into it via the change-guarded @mirror_field trigger. - Wire the online backfill as a commented deploy-migrate step. - Stage the cutover and contract steps as comments on the File model. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01LfZvkigk8hdsKdEif3hzBi --- Makefile | 3 +- .../0167_file_size_bigint_expand.py | 43 +++++++++++++++++++ contentcuration/contentcuration/models.py | 19 ++++++++ 3 files changed, 64 insertions(+), 1 deletion(-) create mode 100644 contentcuration/contentcuration/migrations/0167_file_size_bigint_expand.py diff --git a/Makefile b/Makefile index 27c222a7d2..2f7ff57212 100644 --- a/Makefile +++ b/Makefile @@ -39,7 +39,8 @@ migrate: # 4) Remove the management command from this `deploy-migrate` recipe # 5) Repeat! deploy-migrate: - echo "Nothing to do here!" + # studio#5974: remove at cutover. + python contentcuration/manage.py backfill_column --model contentcuration.File --source-field file_size --target-field file_size_bigint contentnodegc: python contentcuration/manage.py garbage_collect diff --git a/contentcuration/contentcuration/migrations/0167_file_size_bigint_expand.py b/contentcuration/contentcuration/migrations/0167_file_size_bigint_expand.py new file mode 100644 index 0000000000..6fc8d87188 --- /dev/null +++ b/contentcuration/contentcuration/migrations/0167_file_size_bigint_expand.py @@ -0,0 +1,43 @@ +# Generated by Django 3.2.24 on 2026-06-23 05:56 +import pgtrigger.compiler +import pgtrigger.migrations +from django.db import migrations +from django.db import models +from django.db.models import Q + + +class Migration(migrations.Migration): + + dependencies = [ + ("contentcuration", "0166_add_usersubscription"), + ] + + operations = [ + migrations.AddField( + model_name="file", + name="file_size_bigint", + field=models.BigIntegerField(blank=True, null=True), + ), + migrations.AddIndex( + model_name="file", + index=models.Index( + fields=["checksum", "file_size_bigint"], + name="file_checksum_fsizebig_idx", + condition=Q(file_size_bigint__isnull=False), + ), + ), + pgtrigger.migrations.AddTrigger( + model_name="file", + trigger=pgtrigger.compiler.Trigger( + name="mirror_file_size_to_file_size_bigint", + sql=pgtrigger.compiler.UpsertTriggerSql( + func="IF NEW.file_size IS DISTINCT FROM OLD.file_size THEN NEW.file_size_bigint = NEW.file_size; END IF; RETURN NEW;", + hash="051e321c4cdf91ea81f96b9f9a29e3b5015def67", + operation="INSERT OR UPDATE", + pgid="pgtrigger_mirror_file_size_to_file_size_bigint_54326", + table="contentcuration_file", + when="BEFORE", + ), + ), + ), + ] diff --git a/contentcuration/contentcuration/models.py b/contentcuration/contentcuration/models.py index ae2ab2b615..1f0c1ec8c5 100644 --- a/contentcuration/contentcuration/models.py +++ b/contentcuration/contentcuration/models.py @@ -79,6 +79,7 @@ from contentcuration.constants import feedback from contentcuration.constants import user_history from contentcuration.constants.contentnode import kind_activity_map +from contentcuration.db.dual_write import mirror_field from contentcuration.db.models.expressions import Array from contentcuration.db.models.functions import ArrayRemove from contentcuration.db.models.functions import Unnest @@ -3255,6 +3256,8 @@ class StagedFile(models.Model): FILE_DISTINCT_INDEX_NAME = "file_checksum_file_size_idx" +# studio#5974: bigint shadow of FILE_DISTINCT_INDEX_NAME, for the file_size widening. +FILE_DISTINCT_BIGINT_INDEX_NAME = "file_checksum_fsizebig_idx" FILE_MODIFIED_DESC_INDEX_NAME = "file_modified_desc_idx" FILE_DURATION_CONSTRAINT = "file_media_duration_int" MEDIA_PRESETS = [ @@ -3266,6 +3269,14 @@ class StagedFile(models.Model): ] +# studio#5974 swap (next release, after backfill completes). One migration: +# - drop the @mirror_field decorator and the file_size_bigint field below +# - file_size = models.BigIntegerField(blank=True, null=True) +# - DB ops: drop the trigger + int file_size column, then RENAME file_size_bigint -> file_size +# - wrap in SeparateDatabaseAndState so the int->bigint AlterField is state-only (no rewrite) +# Transparent to old pods: they keep writing file_size (now bigint); only a brief metadata lock. +# Do NOT add db_column to reach file_size_bigint first — that generation breaks at the rename. +@mirror_field("file_size", "file_size_bigint") # studio#5974: dual-write int->bigint class File(models.Model): """ The bottom layer of the contentDB schema, defines the basic building brick for content. @@ -3275,6 +3286,9 @@ class File(models.Model): id = UUIDField(primary_key=True, default=uuid.uuid4) checksum = models.CharField(max_length=400, blank=True, db_index=True) file_size = models.IntegerField(blank=True, null=True) + file_size_bigint = models.BigIntegerField( + blank=True, null=True + ) # studio#5974 shadow file_on_disk = models.FileField( upload_to=object_storage_name, storage=default_storage, @@ -3485,6 +3499,11 @@ class Meta: models.Index( fields=["checksum", "file_size"], name=FILE_DISTINCT_INDEX_NAME ), + models.Index( + fields=["checksum", "file_size_bigint"], + name=FILE_DISTINCT_BIGINT_INDEX_NAME, + condition=Q(file_size_bigint__isnull=False), + ), models.Index(fields=["-modified"], name=FILE_MODIFIED_DESC_INDEX_NAME), ] constraints = [ From e8951f753a013774da43b008cd99971bdc0de319 Mon Sep 17 00:00:00 2001 From: Richard Tibbles Date: Wed, 24 Jun 2026 22:51:28 -0700 Subject: [PATCH 09/25] feat: add GCS resumable upload storage helpers - supports_resumable flag on the GCS storage backends - get_stored_object_md5: dedup lookup against an object's GCS-computed md5 - create_resumable_upload_session: pins md5 + declared-size metadata - hex_to_base64 checksum helper Part of #5975. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01UzUP3UYP4cLyouvssXyekj --- .../tests/test_storage_common.py | 39 +++++++++++++------ .../contentcuration/utils/files.py | 31 +++++++++++++++ .../contentcuration/utils/gcs_storage.py | 28 +++++++++++-- .../contentcuration/utils/storage_common.py | 35 ++++------------- 4 files changed, 90 insertions(+), 43 deletions(-) diff --git a/contentcuration/contentcuration/tests/test_storage_common.py b/contentcuration/contentcuration/tests/test_storage_common.py index f89534c194..82feef09ea 100644 --- a/contentcuration/contentcuration/tests/test_storage_common.py +++ b/contentcuration/contentcuration/tests/test_storage_common.py @@ -1,4 +1,3 @@ -import codecs import hashlib from datetime import timedelta from io import BytesIO @@ -12,6 +11,7 @@ from .base import StudioTestCase from contentcuration.models import generate_object_storage_name +from contentcuration.utils.gcs_storage import GoogleCloudStorage from contentcuration.utils.storage_common import _get_gcs_presigned_put_url from contentcuration.utils.storage_common import determine_content_type from contentcuration.utils.storage_common import get_presigned_upload_url @@ -79,7 +79,7 @@ def test_raises_error(self): with pytest.raises(UnknownStorageBackendError): get_presigned_upload_url( "nice", - "err", + "d41d8cd98f00b204e9800998ecf8427e", 5, 0, storage=self.STORAGE, @@ -157,6 +157,23 @@ def test_generate_signed_url_called_with_required_arguments(self): content_type=mimetype, ) + def test_create_resumable_session_pins_md5_size_and_returns_url(self): + storage = GoogleCloudStorage(self.client, "bucket") + blob = self.client.get_bucket.return_value.blob.return_value + blob.create_resumable_upload_session.return_value = "https://session.url" + + url = storage.create_resumable_upload_session( + "storage/a/b/abc.jpg", + "d41d8cd98f00b204e9800998ecf8427e", + 2048, + ) + + assert url == "https://session.url" + assert blob.md5_hash == "1B2M2Y8AsgTpgAmY7PhCfg==" # hex checksum, b64-encoded + assert blob.content_type == "image/jpeg" + assert blob.metadata == {"declared-size": "2048"} + blob.create_resumable_upload_session.assert_called_once() + class S3StoragePresignedURLUnitTestCase(StudioTestCase): """ @@ -177,7 +194,12 @@ def test_returns_string_if_inputs_are_valid(self): # use a real connection here as a sanity check ret = get_presigned_upload_url( - "a/b/abc.jpg", "aBc", 10, 1, storage=self.STORAGE, client=None + "a/b/abc.jpg", + "d41d8cd98f00b204e9800998ecf8427e", + 10, + 1, + storage=self.STORAGE, + client=None, ) url = ret["uploadURL"] @@ -189,19 +211,12 @@ def test_can_upload_file_to_presigned_url(self): """ file_contents = b"blahfilecontents" file = BytesIO(file_contents) - # S3 expects a base64-encoded MD5 checksum - md5 = hashlib.md5(file_contents) - md5_checksum = md5.hexdigest() - md5_checksum_base64 = codecs.encode( - codecs.decode(md5_checksum, "hex"), "base64" - ).decode() + md5_checksum = hashlib.md5(file_contents).hexdigest() filename = "blahfile.jpg" filepath = generate_object_storage_name(md5_checksum, filename) - ret = get_presigned_upload_url( - filepath, md5_checksum_base64, 1000, len(file_contents) - ) + ret = get_presigned_upload_url(filepath, md5_checksum, 1000, len(file_contents)) url = ret["uploadURL"] content_type = ret["mimetype"] diff --git a/contentcuration/contentcuration/utils/files.py b/contentcuration/contentcuration/utils/files.py index 0cb447a601..785faba80b 100644 --- a/contentcuration/contentcuration/utils/files.py +++ b/contentcuration/contentcuration/utils/files.py @@ -1,5 +1,6 @@ import base64 import copy +import mimetypes import os import re import tempfile @@ -17,6 +18,12 @@ from contentcuration.models import File from contentcuration.models import generate_object_storage_name + +# Do this to ensure that we infer mimetypes for files properly, specifically +# zip file and epub files. +# to add additional files add them to the mime.types file +mimetypes.init([os.path.join(os.path.dirname(__file__), "mime.types")]) + ImageFile.LOAD_TRUNCATED_IMAGES = True THUMBNAIL_WIDTH = 400 @@ -196,3 +203,27 @@ def create_thumbnail_from_base64( ) finally: os.close(fd) + + +def determine_content_type(filename): + """ + Guesses the content type of a filename. Returns the mimetype of a file. + + Returns "application/octet-stream" if the type can't be guessed. + Raises an AssertionError if filename is not a string. + """ + + typ, _ = mimetypes.guess_type(filename) + + if not typ: + return "application/octet-stream" + return typ + + +def hex_to_base64(hexdigest): + """Convert a hex-encoded digest (e.g. an MD5 checksum) to base64.""" + return codecs.encode(codecs.decode(hexdigest, "hex"), "base64").decode().strip() + + +def base64_to_hex(b64): + return codecs.encode(codecs.decode(b64.encode(), "base64"), "hex").decode().strip() diff --git a/contentcuration/contentcuration/utils/gcs_storage.py b/contentcuration/contentcuration/utils/gcs_storage.py index 5c4a425aec..75987e443c 100644 --- a/contentcuration/contentcuration/utils/gcs_storage.py +++ b/contentcuration/contentcuration/utils/gcs_storage.py @@ -11,6 +11,10 @@ from google.cloud.storage import Client from google.cloud.storage.blob import Blob +from .files import determine_content_type +from contentcuration.utils.files import base64_to_hex +from contentcuration.utils.files import hex_to_base64 + OLD_STUDIO_STORAGE_PREFIX = "/contentworkshop_content/" CONTENT_DATABASES_MAX_AGE = 5 # seconds @@ -120,10 +124,6 @@ def save(self, name, fobj, max_length=None, blob_object=None): blob.content_encoding = "gzip" fobj = buffer - # determine the current file's mimetype based on the name - # import determine_content_type lazily in here, so we don't get into an infinite loop with circular dependencies - from contentcuration.utils.storage_common import determine_content_type - content_type = determine_content_type(name) # force the current file to be at file location 0, to @@ -215,6 +215,18 @@ def _is_file_empty(fobj): fobj.seek(current_location) return len(byt) == 0 + def create_resumable_upload_session(self, name, md5, size): + blob = self.bucket.blob(name) + md5_b64 = hex_to_base64(md5) + blob.md5_hash = md5_b64.strip() + blob.content_type = determine_content_type(name) + blob.metadata = {"declared-size": str(size)} + return blob.create_resumable_upload_session(client=self.client) + + def get_stored_object_md5(self, name): + blob = self.bucket.get_blob(name) + return base64_to_hex(blob.md5_hash) if blob is not None else None + class CompositeGCS(Storage): def __init__(self): @@ -283,3 +295,11 @@ def get_created_time(self, name): def get_modified_time(self, name): return self._get_readable_backend(name).get_modified_time(name) + + def create_resumable_upload_session(self, name, md5_b64, size): + return self._get_writeable_backend().create_resumable_upload_session( + name, md5_b64, size + ) + + def get_stored_object_md5(self, name): + return self._get_readable_backend(name).get_stored_object_md5(name) diff --git a/contentcuration/contentcuration/utils/storage_common.py b/contentcuration/contentcuration/utils/storage_common.py index 10d79bd5c5..6f347b27ca 100644 --- a/contentcuration/contentcuration/utils/storage_common.py +++ b/contentcuration/contentcuration/utils/storage_common.py @@ -1,43 +1,22 @@ -import mimetypes -import os from datetime import timedelta from django.conf import settings from django.core.files.storage import default_storage from django_s3_storage.storage import S3Storage +from .files import determine_content_type +from .files import hex_to_base64 from .gcs_storage import CompositeGCS from .gcs_storage import GoogleCloudStorage -# Do this to ensure that we infer mimetypes for files properly, specifically -# zip file and epub files. -# to add additional files add them to the mime.types file -mimetypes.init([os.path.join(os.path.dirname(__file__), "mime.types")]) - - class UnknownStorageBackendError(Exception): pass -def determine_content_type(filename): - """ - Guesses the content type of a filename. Returns the mimetype of a file. - - Returns "application/octet-stream" if the type can't be guessed. - Raises an AssertionError if filename is not a string. - """ - - typ, _ = mimetypes.guess_type(filename) - - if not typ: - return "application/octet-stream" - return typ - - def get_presigned_upload_url( filepath, - md5sum_b64, + md5_hex, lifetime_sec, content_length, storage=default_storage, @@ -48,9 +27,10 @@ def get_presigned_upload_url( contents with the contents of your PUT request. :param: filepath: the file path inside the bucket, to the file. - :param: md5sum_b64: the base64 encoded md5 hash of the file. The holder of the URL will - have to set a Content-MD5 HTTP header matching this md5sum once it - initiates the download. + :param: md5_hex: the hex-encoded md5 hash of the file. The base64 encoding + that GCS requires for the Content-MD5 header is handled internally; the + holder of the URL must set a Content-MD5 HTTP header matching that + base64-encoded value once it initiates the upload. :param: lifetime_sec: the lifetime of the generated upload url, in seconds. :param: content_length: the size of the content, in bytes. :param: client: the storage client that will be used to gennerate the presigned URL. @@ -67,6 +47,7 @@ def get_presigned_upload_url( # both storage types are having difficulties enforcing it. mimetype = determine_content_type(filepath) + md5sum_b64 = hex_to_base64(md5_hex) if isinstance(storage, (GoogleCloudStorage, CompositeGCS)): client = client or storage.get_client() bucket = settings.AWS_S3_BUCKET_NAME From a972a5c2db3f9528c69bad9ca68b3573f88b7bf2 Mon Sep 17 00:00:00 2001 From: Richard Tibbles Date: Wed, 24 Jun 2026 22:51:36 -0700 Subject: [PATCH 10/25] feat: add opt-in resumable scheme to the upload_url endpoint - accept a `resumable` flag (defaults off) - GCS: skip when the stored md5 matches the checksum, else return a server-initiated resumable session URI - non-GCS backends fall back to single-PUT - reject non-resumable uploads over 500 MB - make `size` an IntegerField, dropping the redundant float casts Part of #5975. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01UzUP3UYP4cLyouvssXyekj --- .../tests/viewsets/test_file.py | 83 +++++++++++++++++++ .../contentcuration/utils/files.py | 1 + .../contentcuration/viewsets/file.py | 47 +++++++---- 3 files changed, 115 insertions(+), 16 deletions(-) diff --git a/contentcuration/contentcuration/tests/viewsets/test_file.py b/contentcuration/contentcuration/tests/viewsets/test_file.py index 9737c7f4bd..b775827040 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_file.py +++ b/contentcuration/contentcuration/tests/viewsets/test_file.py @@ -1,4 +1,5 @@ import uuid +from unittest import mock from django.urls import reverse from le_utils.constants import content_kinds @@ -12,6 +13,8 @@ from contentcuration.tests.viewsets.base import generate_delete_event from contentcuration.tests.viewsets.base import generate_update_event from contentcuration.tests.viewsets.base import SyncTestMixin +from contentcuration.viewsets.file import FileUploadURLSerializer +from contentcuration.viewsets.file import MAX_NON_RESUMABLE_UPLOAD_SIZE from contentcuration.viewsets.sync.constants import CONTENTNODE from contentcuration.viewsets.sync.constants import FILE @@ -546,6 +549,9 @@ def test_mismatched_preset_upload(self): def test_insufficient_storage(self): self.file["size"] = 100000000000000 + self.file[ + "resumable" + ] = True # resumable bypasses the >500MB guard so this still exercises the quota (412) path self.client.force_authenticate(user=self.user) response = self.client.post( @@ -590,6 +596,26 @@ def test_duration_zero(self): self.assertEqual(response.status_code, 400) + def test_fractional_size_rejected(self): + s = FileUploadURLSerializer(data={**self.file, "size": 1000.5}) + assert not s.is_valid() + assert "size" in s.errors + + def test_large_non_resumable_rejected(self): + self.file["size"] = MAX_NON_RESUMABLE_UPLOAD_SIZE + 1 + self.client.force_authenticate(user=self.user) + resp = self.client.post(reverse("file-upload-url"), self.file, format="json") + assert resp.status_code == 400 + + def test_large_resumable_allowed(self): + self.user.disk_space = 10 * 1024 * 1024 * 1024 + self.user.save() + self.file["size"] = MAX_NON_RESUMABLE_UPLOAD_SIZE + 1 + self.file["resumable"] = True + self.client.force_authenticate(user=self.user) + resp = self.client.post(reverse("file-upload-url"), self.file, format="json") + assert resp.status_code == 200 + class ContentIDTestCase(SyncTestMixin, StudioAPITestCase): def setUp(self): @@ -763,3 +789,60 @@ def test_content_id__thumbnails_dont_update_content_id(self): self.assertEqual( copied_node_content_id_before_upload, copied_node_content_id_after_upload ) + + +class ResumableUploadURLTestCase(StudioAPITestCase): + def setUp(self): + super(ResumableUploadURLTestCase, self).setUp() + self.user = testdata.user() + # Give user enough quota to handle resumable uploads + self.user.disk_space = 10 * 1024 * 1024 * 1024 + self.user.save() + self.file = { + "size": 1000, + "checksum": uuid.uuid4().hex, + "name": "le_studio", + "file_format": file_formats.MP3, + "preset": format_presets.AUDIO, + "duration": 10.123, + "resumable": True, + } + + @mock.patch("contentcuration.viewsets.file.default_storage") + def test_resumable_returns_session_when_not_stored(self, mock_storage): + mock_storage.get_stored_object_md5.return_value = None + mock_storage.create_resumable_upload_session.return_value = ( + "https://session.url" + ) + self.client.force_authenticate(user=self.user) + resp = self.client.post(reverse("file-upload-url"), self.file, format="json") + assert resp.status_code == 200 + data = resp.json() + assert data["resumable"] is True + assert data["uploadURL"] == "https://session.url" + assert data["alreadyUploaded"] is False + assert "file" in data + assert data["file"]["id"] + mock_storage.create_resumable_upload_session.assert_called_once() + + @mock.patch("contentcuration.viewsets.file.default_storage") + def test_resumable_skips_when_already_stored(self, mock_storage): + mock_storage.get_stored_object_md5.return_value = self.file["checksum"] + self.client.force_authenticate(user=self.user) + resp = self.client.post(reverse("file-upload-url"), self.file, format="json") + data = resp.json() + assert data["resumable"] is True and data["alreadyUploaded"] is True + assert data["uploadURL"] is None + assert "file" in data + assert data["file"]["id"] + mock_storage.create_resumable_upload_session.assert_not_called() + + def test_resumable_falls_back_to_single_put_on_s3(self): + # default_storage is S3 in the test env → no resumable support + self.client.force_authenticate(user=self.user) + resp = self.client.post(reverse("file-upload-url"), self.file, format="json") + data = resp.json() + assert data["resumable"] is False + assert "uploadURL" in data + assert "file" in data + assert data["file"]["id"] diff --git a/contentcuration/contentcuration/utils/files.py b/contentcuration/contentcuration/utils/files.py index 785faba80b..dca7502262 100644 --- a/contentcuration/contentcuration/utils/files.py +++ b/contentcuration/contentcuration/utils/files.py @@ -1,4 +1,5 @@ import base64 +import codecs import copy import mimetypes import os diff --git a/contentcuration/contentcuration/viewsets/file.py b/contentcuration/contentcuration/viewsets/file.py index afadbff0cb..577eb827e1 100644 --- a/contentcuration/contentcuration/viewsets/file.py +++ b/contentcuration/contentcuration/viewsets/file.py @@ -1,7 +1,7 @@ -import codecs import math from django.core.exceptions import PermissionDenied +from django.core.files.storage import default_storage from django.http import HttpResponseBadRequest from le_utils.constants import file_formats from le_utils.constants import format_presets @@ -34,6 +34,8 @@ PRESET_LOOKUP = {p.id: p for p in format_presets.PRESETLIST} +MAX_NON_RESUMABLE_UPLOAD_SIZE = 500 * 1024 * 1024 + class StrictFloatField(serializers.FloatField): def to_internal_value(self, data): @@ -48,7 +50,7 @@ class FileUploadURLSerializer(serializers.Serializer): """ Serializer to validate inputs for the upload_url endpoint. Required: - - size: a float value + - size: an integer value (bytes) - checksum: a 32-digit hex string - name: a string (note: mapped from request.data['name']) - file_format: a valid file format choice from file_formats.choices @@ -57,12 +59,13 @@ class FileUploadURLSerializer(serializers.Serializer): - duration: a number that will be floored to an integer and must be > 0 """ - size = serializers.FloatField(required=True) + size = serializers.IntegerField(required=True) checksum = serializers.RegexField(regex=r"^[0-9a-f]{32}$", required=True) name = serializers.CharField(required=True) file_format = serializers.ChoiceField(choices=file_formats.choices, required=True) preset = serializers.ChoiceField(choices=format_presets.choices, required=True) duration = StrictFloatField(required=False, allow_null=True) + resumable = serializers.BooleanField(required=False, default=False) def validate_duration(self, value): if value is None: @@ -89,6 +92,10 @@ def validate(self, attrs): raise serializers.ValidationError( f"File format {attrs['file_format']} is not an allowed format for this preset {attrs['preset']}" ) + if not attrs["resumable"] and attrs["size"] > MAX_NON_RESUMABLE_UPLOAD_SIZE: + raise serializers.ValidationError( + "Files larger than 500 MB must use a resumable upload." + ) return attrs @@ -235,26 +242,37 @@ def upload_url(self, request): file_format = validated_data["file_format"] preset = validated_data["preset"] duration = validated_data.get("duration") + resumable = validated_data["resumable"] try: - request.user.check_space(float(size), checksum) + request.user.check_space(size, checksum) except PermissionDenied: return HttpResponseBadRequest( reason="Not enough space. Check your storage under Settings page.", status=412, ) - might_skip = File.objects.filter(checksum=checksum).exists() - filepath = generate_object_storage_name( checksum, filename, default_ext=file_format ) - checksum_base64 = codecs.encode( - codecs.decode(checksum, "hex"), "base64" - ).decode() - retval = get_presigned_upload_url( - filepath, checksum_base64, 600, content_length=size - ) + if resumable and hasattr(default_storage, "create_resumable_upload_session"): + # Resumable response omits mimetype/might_skip. + stored = default_storage.get_stored_object_md5(filepath) == checksum + retval = { + "resumable": True, + "uploadURL": None + if stored + else default_storage.create_resumable_upload_session( + filepath, checksum, size + ), + "alreadyUploaded": stored, + } + else: + retval = get_presigned_upload_url( + filepath, checksum, 600, content_length=size + ) + retval["resumable"] = False + retval["might_skip"] = File.objects.filter(checksum=checksum).exists() file = File( file_size=size, @@ -270,8 +288,5 @@ def upload_url(self, request): # Avoid using our file_on_disk attribute for checks file.save(set_by_file_on_disk=False) - retval.update( - {"might_skip": might_skip, "file": self.serialize_object(id=file.id)} - ) - + retval["file"] = self.serialize_object(id=file.id) return Response(retval) From 5dab9221da149cca868a8d8780f4ebc42c3b52ef Mon Sep 17 00:00:00 2001 From: Richard Tibbles Date: Fri, 10 Jul 2026 20:46:49 -0700 Subject: [PATCH 11/25] fix: scope models imports in utils/files.py to break boot-time circular import The GCS storage backend imports utils.files at module load. files' top-level `from contentcuration.models import File / generate_object_storage_name` then re-entered a partially initialized contentcuration.models during app boot (DEFAULT_FILE_STORAGE=GCS), raising ImportError and crash-looping the pods. The test suite missed it: pytest imports models early, so models is complete before the storage modules load files. Move File and generate_object_storage_name imports into their call sites (create_file_from_contents, get_file_diff, get_thumbnail_encoding). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_019ineqU2EGLAcWW2WPWR3mE --- contentcuration/contentcuration/utils/files.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/contentcuration/contentcuration/utils/files.py b/contentcuration/contentcuration/utils/files.py index dca7502262..aa7c3a9db4 100644 --- a/contentcuration/contentcuration/utils/files.py +++ b/contentcuration/contentcuration/utils/files.py @@ -16,8 +16,6 @@ from PIL import ImageFile from contentcuration.api import write_raw_content_to_storage -from contentcuration.models import File -from contentcuration.models import generate_object_storage_name # Do this to ensure that we infer mimetypes for files properly, specifically @@ -32,6 +30,11 @@ def create_file_from_contents( contents, ext=None, node=None, preset_id=None, uploaded_by=None ): + # Imported here rather than at module level to avoid a circular import: + # the GCS storage backend imports this module, and importing models at load + # time re-enters a partially initialized contentcuration.models during boot. + from contentcuration.models import File + checksum, _, path = write_raw_content_to_storage(contents, ext=ext) result = File( @@ -53,6 +56,10 @@ def get_file_diff(files): """ + # Imported here rather than at module level to avoid a circular import (see + # create_file_from_contents). + from contentcuration.models import generate_object_storage_name + # We use a thread pool in here, making direct HEAD requests to the storage URL # to see if the objects exist. # The threaded method is found to be the fastest -- see @@ -101,6 +108,9 @@ def get_thumbnail_encoding(filename, dimension=THUMBNAIL_WIDTH): dimension (int, optional): desired width of thumbnail. Defaults to 400. Returns base64 encoding of resized thumbnail """ + # Imported here rather than at module level to avoid a circular import (see + # create_file_from_contents). + from contentcuration.models import generate_object_storage_name if filename.startswith("data:image"): return filename From 7366349caae2697174e6b087a6ccb0c16bbbf656 Mon Sep 17 00:00:00 2001 From: Richard Tibbles Date: Fri, 10 Jul 2026 21:56:11 -0700 Subject: [PATCH 12/25] fix: return None from CompositeGCS.get_stored_object_md5 for missing objects The resumable upload_url path calls get_stored_object_md5(filepath) to dedup against a stored object's md5. On CompositeGCS this delegated to _get_readable_backend(name), which raises FileNotFoundError when the object is in no backend -- i.e. every not-yet-uploaded file -- 500ing upload_url for all new resumable uploads. Catch FileNotFoundError and return None, matching GoogleCloudStorage.get_stored_object_md5 for a missing blob. Add regression tests for the found and not-found cases; the not-found test fails against the pre-fix code (FileNotFoundError). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_019ineqU2EGLAcWW2WPWR3mE --- .../contentcuration/tests/test_gcs_storage.py | 19 +++++++++++++++++++ .../contentcuration/utils/gcs_storage.py | 9 ++++++++- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/contentcuration/contentcuration/tests/test_gcs_storage.py b/contentcuration/contentcuration/tests/test_gcs_storage.py index a58420873e..7f19cfb175 100755 --- a/contentcuration/contentcuration/tests/test_gcs_storage.py +++ b/contentcuration/contentcuration/tests/test_gcs_storage.py @@ -8,6 +8,7 @@ from google.cloud.storage.blob import Blob from mixer.main import mixer +from contentcuration.utils.files import hex_to_base64 from contentcuration.utils.gcs_storage import CompositeGCS from contentcuration.utils.gcs_storage import GoogleCloudStorage @@ -232,3 +233,21 @@ def test_get_created_time(self): self.storage.get_created_time("blob"), self.blob_cls.return_value.time_created, ) + + def test_get_stored_object_md5(self): + mock_blob = self.blob_cls("blob", "blob") + mock_blob.md5_hash = hex_to_base64("d41d8cd98f00b204e9800998ecf8427e") + self.mock_default_bucket.get_blob.return_value = mock_blob + self.assertEqual( + self.storage.get_stored_object_md5("blob"), + "d41d8cd98f00b204e9800998ecf8427e", + ) + + def test_get_stored_object_md5__returns_none_if_not_found(self): + # Regression: a not-yet-uploaded object is in no backend, so + # _get_readable_backend raises FileNotFoundError. get_stored_object_md5 + # must swallow that and return None (else the resumable upload_url + # endpoint 500s on every new file), not propagate the error. + self.mock_default_bucket.get_blob.return_value = None + self.mock_anon_bucket.get_blob.return_value = None + self.assertIsNone(self.storage.get_stored_object_md5("blob")) diff --git a/contentcuration/contentcuration/utils/gcs_storage.py b/contentcuration/contentcuration/utils/gcs_storage.py index 75987e443c..7ad581f56d 100644 --- a/contentcuration/contentcuration/utils/gcs_storage.py +++ b/contentcuration/contentcuration/utils/gcs_storage.py @@ -302,4 +302,11 @@ def create_resumable_upload_session(self, name, md5_b64, size): ) def get_stored_object_md5(self, name): - return self._get_readable_backend(name).get_stored_object_md5(name) + # A not-yet-uploaded object exists in no backend; treat that as "no + # stored md5" (None) rather than raising, matching + # GoogleCloudStorage.get_stored_object_md5 for a missing blob. + try: + backend = self._get_readable_backend(name) + except FileNotFoundError: + return None + return backend.get_stored_object_md5(name) From 2e326c0ec2306da04b35fa688888c4360c8b4958 Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Sun, 12 Jul 2026 15:47:01 -0700 Subject: [PATCH 13/25] chore: bump le-utils to 0.2.18 Provides RENDERABLE_PRESETS_ORDER for the included_presets bitmask. The new le-utils adds a QTI question type to exercises.question_choices, which Django picks up as an assessmentitem.type AlterField migration. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../0168_alter_assessmentitem_type.py | 18 ++++++++++++++++++ requirements.in | 2 +- requirements.txt | 2 +- 3 files changed, 20 insertions(+), 2 deletions(-) create mode 100644 contentcuration/contentcuration/migrations/0168_alter_assessmentitem_type.py diff --git a/contentcuration/contentcuration/migrations/0168_alter_assessmentitem_type.py b/contentcuration/contentcuration/migrations/0168_alter_assessmentitem_type.py new file mode 100644 index 0000000000..ffa3f454bc --- /dev/null +++ b/contentcuration/contentcuration/migrations/0168_alter_assessmentitem_type.py @@ -0,0 +1,18 @@ +# Generated by Django 3.2.25 on 2026-07-12 22:39 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('contentcuration', '0167_file_size_bigint_expand'), + ] + + operations = [ + migrations.AlterField( + model_name='assessmentitem', + name='type', + field=models.CharField(choices=[('input_question', 'Input Question'), ('multiple_selection', 'Multiple Selection'), ('single_selection', 'Single Selection'), ('free_response', 'Free Response'), ('perseus_question', 'Perseus Question'), ('QTI', 'QTI'), ('true_false', 'True/False')], default='multiple_selection', max_length=50), + ), + ] diff --git a/requirements.in b/requirements.in index df5bd6d494..a55ac35c84 100644 --- a/requirements.in +++ b/requirements.in @@ -5,7 +5,7 @@ djangorestframework==3.15.1 psycopg2-binary==2.9.11 django-js-reverse==0.10.2 django-registration==3.4 -le-utils==0.2.17 +le-utils==0.2.18 gunicorn==25.1.0 django-postmark==0.1.6 jsonfield==3.1.0 diff --git a/requirements.txt b/requirements.txt index 43044dfb9d..1a46dcee2d 100644 --- a/requirements.txt +++ b/requirements.txt @@ -180,7 +180,7 @@ langcodes==3.5.1 # via -r requirements.in latex2mathml==3.78.1 # via -r requirements.in -le-utils==0.2.17 +le-utils==0.2.18 # via -r requirements.in markdown-it-py==4.0.0 # via -r requirements.in From c50f3b05fb561fe72eb3f145ee775e9af9dbb48b Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Sun, 12 Jul 2026 15:47:12 -0700 Subject: [PATCH 14/25] feat: add included_presets column to File mirrors Add a nullable included_presets IntegerField to the abstract kolibri_content.base_models.File, inherited by both the kolibri_content (sqlite export) and kolibri_public (import-metadata API) File mirrors. It holds a bitmask of the renderable presets a device needs to render a file. The column is additive and nullable, so old Kolibri ignores it. Co-Authored-By: Claude Opus 4.8 (1M context) --- contentcuration/kolibri_content/base_models.py | 3 +++ .../migrations/0024_file_included_presets.py | 18 ++++++++++++++++++ .../migrations/0009_file_included_presets.py | 18 ++++++++++++++++++ 3 files changed, 39 insertions(+) create mode 100644 contentcuration/kolibri_content/migrations/0024_file_included_presets.py create mode 100644 contentcuration/kolibri_public/migrations/0009_file_included_presets.py diff --git a/contentcuration/kolibri_content/base_models.py b/contentcuration/kolibri_content/base_models.py index 220558a0bb..b5ed17e1bc 100644 --- a/contentcuration/kolibri_content/base_models.py +++ b/contentcuration/kolibri_content/base_models.py @@ -132,6 +132,9 @@ class File(models.Model): supplementary = models.BooleanField(default=False) thumbnail = models.BooleanField(default=False) priority = models.IntegerField(blank=True, null=True, db_index=True) + # Bitmask of the renderable presets a device needs to render this file, + # including the file's own preset. NULL for supplementary files. + included_presets = models.IntegerField(blank=True, null=True) class Meta: abstract = True diff --git a/contentcuration/kolibri_content/migrations/0024_file_included_presets.py b/contentcuration/kolibri_content/migrations/0024_file_included_presets.py new file mode 100644 index 0000000000..a56bfe5a1e --- /dev/null +++ b/contentcuration/kolibri_content/migrations/0024_file_included_presets.py @@ -0,0 +1,18 @@ +# Generated by Django 3.2.25 on 2026-07-12 22:27 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('content', '0023_auto_20250417_1516'), + ] + + operations = [ + migrations.AddField( + model_name='file', + name='included_presets', + field=models.IntegerField(blank=True, null=True), + ), + ] diff --git a/contentcuration/kolibri_public/migrations/0009_file_included_presets.py b/contentcuration/kolibri_public/migrations/0009_file_included_presets.py new file mode 100644 index 0000000000..42fc3ae74b --- /dev/null +++ b/contentcuration/kolibri_public/migrations/0009_file_included_presets.py @@ -0,0 +1,18 @@ +# Generated by Django 3.2.25 on 2026-07-12 22:27 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('kolibri_public', '0008_channelmetadata_categories_bitmask_0'), + ] + + operations = [ + migrations.AddField( + model_name='file', + name='included_presets', + field=models.IntegerField(blank=True, null=True), + ), + ] From fe88cd4cc280433964b47bc59d7d69f306199949 Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Sun, 12 Jul 2026 15:47:12 -0700 Subject: [PATCH 15/25] feat: populate included_presets at publish In create_associated_file_objects, set each renderable (non-supplementary) file's own-preset bit (2 ** RENDERABLE_PRESETS_ORDER.index(preset_id)) and leave supplementary files NULL. A preset missing from the append-only ordering is logged and skipped rather than aborting the channel publish. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../contentcuration/tests/test_exportchannel.py | 16 ++++++++++++++++ .../contentcuration/utils/publish.py | 17 +++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/contentcuration/contentcuration/tests/test_exportchannel.py b/contentcuration/contentcuration/tests/test_exportchannel.py index 786c4ccec1..2817ed840e 100644 --- a/contentcuration/contentcuration/tests/test_exportchannel.py +++ b/contentcuration/contentcuration/tests/test_exportchannel.py @@ -19,6 +19,7 @@ from le_utils.constants import exercises from le_utils.constants import format_presets from le_utils.constants import modalities +from le_utils.constants.format_presets import RENDERABLE_PRESETS_ORDER from le_utils.constants.labels import accessibility_categories from le_utils.constants.labels import learning_activities from le_utils.constants.labels import levels @@ -505,6 +506,21 @@ def test_contentnode_file_size_data(self): for file in files.prefetch_related("local_file"): self.assertEqual(file.file_size, file.local_file.file_size) + def test_file_included_presets_renderable(self): + # Every non-supplementary (renderable) exported file carries its own preset bit. + files = kolibri_models.File.objects.filter(supplementary=False) + assert files.count() > 0 + for file in files: + expected = 2 ** RENDERABLE_PRESETS_ORDER.index(file.preset) + self.assertEqual(file.included_presets, expected) + + def test_file_included_presets_supplementary_null(self): + # Supplementary files (e.g. thumbnails) leave included_presets NULL. + files = kolibri_models.File.objects.filter(supplementary=True) + assert files.count() > 0 + for file in files: + self.assertIsNone(file.included_presets) + def test_channel_icon_encoding(self): self.assertIsNotNone(self.content_channel.icon_encoding) diff --git a/contentcuration/contentcuration/utils/publish.py b/contentcuration/contentcuration/utils/publish.py index 6e18dea57e..9b4cfe2c3b 100644 --- a/contentcuration/contentcuration/utils/publish.py +++ b/contentcuration/contentcuration/utils/publish.py @@ -38,6 +38,7 @@ from le_utils.constants import licenses from le_utils.constants import modalities from le_utils.constants import roles +from le_utils.constants.format_presets import RENDERABLE_PRESETS_ORDER from search.models import ChannelFullTextSearch from search.models import ContentNodeFullTextSearch from search.utils import get_fts_annotated_channel_qs @@ -666,6 +667,21 @@ def create_associated_file_objects(kolibrinode, ccnode): }, ) + included_presets = None + if not preset.supplementary: + try: + included_presets = 2 ** RENDERABLE_PRESETS_ORDER.index(preset.pk) + except ValueError: + # Renderable preset not in the (append-only) ordering — e.g. a newer + # le-utils preset. Log and leave included_presets NULL for this file + # rather than aborting the whole channel publish. + logging.warning( + "Preset %s missing from RENDERABLE_PRESETS_ORDER; leaving " + "included_presets NULL for file %s", + preset.pk, + ccfilemodel.pk, + ) + kolibrimodels.File.objects.create( pk=ccfilemodel.pk, checksum=ccfilemodel.checksum, @@ -679,6 +695,7 @@ def create_associated_file_objects(kolibrinode, ccnode): thumbnail=preset.thumbnail, priority=preset.order, local_file=kolibrilocalfilemodel, + included_presets=included_presets, ) From 78596acd1fa81dd3f5d4d219e0a217e38885b061 Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Sun, 12 Jul 2026 15:47:12 -0700 Subject: [PATCH 16/25] feat: advertise content schema version 6 Add VERSION_6 carrying included_presets and set it as CONTENT_SCHEMA_VERSION so new Kolibri knows an export carries the column. MIN_CONTENT_SCHEMA_VERSION stays VERSION_5 since the column is additive and returned regardless. Co-Authored-By: Claude Opus 4.8 (1M context) --- contentcuration/kolibri_content/constants/schema_versions.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/contentcuration/kolibri_content/constants/schema_versions.py b/contentcuration/kolibri_content/constants/schema_versions.py index 01e7ac1f88..86e930ee52 100644 --- a/contentcuration/kolibri_content/constants/schema_versions.py +++ b/contentcuration/kolibri_content/constants/schema_versions.py @@ -19,9 +19,12 @@ VERSION_5 = "5" +VERSION_6 = "6" + # List of the content db schema versions, ordered from most recent to least recent. # When a new schema version is generated, it should be added here, at the top of the list. CONTENT_DB_SCHEMA_VERSIONS = [ + VERSION_6, VERSION_5, VERSION_4, VERSION_3, @@ -33,7 +36,7 @@ ] # The latest compatible exported schema version for this version of Kolibri -CONTENT_SCHEMA_VERSION = VERSION_5 +CONTENT_SCHEMA_VERSION = VERSION_6 # The version name for the current content schema, # which may have schema modifications not present in the export schema From 21edf6f8b4f3b1cc8a105b9719bdb8cf354c24d1 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci-lite[bot]" <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com> Date: Sun, 12 Jul 2026 22:50:52 +0000 Subject: [PATCH 17/25] [pre-commit.ci lite] apply automatic fixes --- .../0168_alter_assessmentitem_type.py | 24 ++++++++++++++----- .../migrations/0024_file_included_presets.py | 10 ++++---- .../migrations/0009_file_included_presets.py | 10 ++++---- 3 files changed, 28 insertions(+), 16 deletions(-) diff --git a/contentcuration/contentcuration/migrations/0168_alter_assessmentitem_type.py b/contentcuration/contentcuration/migrations/0168_alter_assessmentitem_type.py index ffa3f454bc..cc17df476a 100644 --- a/contentcuration/contentcuration/migrations/0168_alter_assessmentitem_type.py +++ b/contentcuration/contentcuration/migrations/0168_alter_assessmentitem_type.py @@ -1,18 +1,30 @@ # Generated by Django 3.2.25 on 2026-07-12 22:39 - -from django.db import migrations, models +from django.db import migrations +from django.db import models class Migration(migrations.Migration): dependencies = [ - ('contentcuration', '0167_file_size_bigint_expand'), + ("contentcuration", "0167_file_size_bigint_expand"), ] operations = [ migrations.AlterField( - model_name='assessmentitem', - name='type', - field=models.CharField(choices=[('input_question', 'Input Question'), ('multiple_selection', 'Multiple Selection'), ('single_selection', 'Single Selection'), ('free_response', 'Free Response'), ('perseus_question', 'Perseus Question'), ('QTI', 'QTI'), ('true_false', 'True/False')], default='multiple_selection', max_length=50), + model_name="assessmentitem", + name="type", + field=models.CharField( + choices=[ + ("input_question", "Input Question"), + ("multiple_selection", "Multiple Selection"), + ("single_selection", "Single Selection"), + ("free_response", "Free Response"), + ("perseus_question", "Perseus Question"), + ("QTI", "QTI"), + ("true_false", "True/False"), + ], + default="multiple_selection", + max_length=50, + ), ), ] diff --git a/contentcuration/kolibri_content/migrations/0024_file_included_presets.py b/contentcuration/kolibri_content/migrations/0024_file_included_presets.py index a56bfe5a1e..7345f9b3d0 100644 --- a/contentcuration/kolibri_content/migrations/0024_file_included_presets.py +++ b/contentcuration/kolibri_content/migrations/0024_file_included_presets.py @@ -1,18 +1,18 @@ # Generated by Django 3.2.25 on 2026-07-12 22:27 - -from django.db import migrations, models +from django.db import migrations +from django.db import models class Migration(migrations.Migration): dependencies = [ - ('content', '0023_auto_20250417_1516'), + ("content", "0023_auto_20250417_1516"), ] operations = [ migrations.AddField( - model_name='file', - name='included_presets', + model_name="file", + name="included_presets", field=models.IntegerField(blank=True, null=True), ), ] diff --git a/contentcuration/kolibri_public/migrations/0009_file_included_presets.py b/contentcuration/kolibri_public/migrations/0009_file_included_presets.py index 42fc3ae74b..17d77751bd 100644 --- a/contentcuration/kolibri_public/migrations/0009_file_included_presets.py +++ b/contentcuration/kolibri_public/migrations/0009_file_included_presets.py @@ -1,18 +1,18 @@ # Generated by Django 3.2.25 on 2026-07-12 22:27 - -from django.db import migrations, models +from django.db import migrations +from django.db import models class Migration(migrations.Migration): dependencies = [ - ('kolibri_public', '0008_channelmetadata_categories_bitmask_0'), + ("kolibri_public", "0008_channelmetadata_categories_bitmask_0"), ] operations = [ migrations.AddField( - model_name='file', - name='included_presets', + model_name="file", + name="included_presets", field=models.IntegerField(blank=True, null=True), ), ] From ba73334e6abed650b532232d4680ddc36a95be6a Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Sun, 12 Jul 2026 18:14:14 -0700 Subject: [PATCH 18/25] feat: add file_size_bigint column to LocalFile mirrors Co-Authored-By: Claude Opus 4.8 (1M context) --- contentcuration/kolibri_content/base_models.py | 1 + .../0025_localfile_file_size_bigint.py | 17 +++++++++++++++++ .../0010_localfile_file_size_bigint.py | 17 +++++++++++++++++ 3 files changed, 35 insertions(+) create mode 100644 contentcuration/kolibri_content/migrations/0025_localfile_file_size_bigint.py create mode 100644 contentcuration/kolibri_public/migrations/0010_localfile_file_size_bigint.py diff --git a/contentcuration/kolibri_content/base_models.py b/contentcuration/kolibri_content/base_models.py index b5ed17e1bc..c2d5417fca 100644 --- a/contentcuration/kolibri_content/base_models.py +++ b/contentcuration/kolibri_content/base_models.py @@ -152,6 +152,7 @@ class LocalFile(models.Model): ) available = models.BooleanField(default=False) file_size = models.IntegerField(blank=True, null=True) + file_size_bigint = models.BigIntegerField(blank=True, null=True) class Meta: abstract = True diff --git a/contentcuration/kolibri_content/migrations/0025_localfile_file_size_bigint.py b/contentcuration/kolibri_content/migrations/0025_localfile_file_size_bigint.py new file mode 100644 index 0000000000..02956bb005 --- /dev/null +++ b/contentcuration/kolibri_content/migrations/0025_localfile_file_size_bigint.py @@ -0,0 +1,17 @@ +from django.db import migrations +from django.db import models + + +class Migration(migrations.Migration): + + dependencies = [ + ("content", "0024_file_included_presets"), + ] + + operations = [ + migrations.AddField( + model_name="localfile", + name="file_size_bigint", + field=models.BigIntegerField(blank=True, null=True), + ), + ] diff --git a/contentcuration/kolibri_public/migrations/0010_localfile_file_size_bigint.py b/contentcuration/kolibri_public/migrations/0010_localfile_file_size_bigint.py new file mode 100644 index 0000000000..aaaae04079 --- /dev/null +++ b/contentcuration/kolibri_public/migrations/0010_localfile_file_size_bigint.py @@ -0,0 +1,17 @@ +from django.db import migrations +from django.db import models + + +class Migration(migrations.Migration): + + dependencies = [ + ("kolibri_public", "0009_file_included_presets"), + ] + + operations = [ + migrations.AddField( + model_name="localfile", + name="file_size_bigint", + field=models.BigIntegerField(blank=True, null=True), + ), + ] From f2cb89b48dfba8ec835c0a4c0727648192d732e8 Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Sun, 12 Jul 2026 18:14:19 -0700 Subject: [PATCH 19/25] feat: populate file_size_bigint at publish Co-Authored-By: Claude Opus 4.8 (1M context) --- .../tests/test_exportchannel.py | 53 +++++++++++++++++++ .../contentcuration/utils/publish.py | 18 ++++++- 2 files changed, 69 insertions(+), 2 deletions(-) diff --git a/contentcuration/contentcuration/tests/test_exportchannel.py b/contentcuration/contentcuration/tests/test_exportchannel.py index 2817ed840e..09113398f1 100644 --- a/contentcuration/contentcuration/tests/test_exportchannel.py +++ b/contentcuration/contentcuration/tests/test_exportchannel.py @@ -16,6 +16,7 @@ from kolibri_content.router import cleanup_content_database_connection from kolibri_content.router import get_active_content_database from kolibri_content.router import set_active_content_database +from le_utils.constants import content_kinds from le_utils.constants import exercises from le_utils.constants import format_presets from le_utils.constants import modalities @@ -57,6 +58,10 @@ pytestmark = pytest.mark.django_db +# Larger than the signed 32-bit maximum (2_147_483_647); ~3 GB. +LARGE_FILE_SIZE = 3 * 1024 ** 3 + + def description(): return "".join(random.sample(string.printable, 20)) @@ -387,6 +392,36 @@ def setUp(self): lesson_topic.extra_fields = {"options": {"modality": modalities.LESSON}} lesson_topic.save() + document_kind, _ = cc.ContentKind.objects.get_or_create( + kind=content_kinds.DOCUMENT + ) + large_file_node = cc.ContentNode( + kind=document_kind, + parent=self.content_channel.main_tree, + title="Large file node", + node_id=uuid.uuid4(), + content_id=uuid.uuid4(), + sort_order=1, + complete=True, + ) + large_file_node.save() + + large_db_file = create_studio_file( + b"large file body", preset="document", ext="pdf" + )["db_file"] + # A >2.1 GB file cannot fit the legacy 32-bit File.file_size column; its + # true size lives in the studio#5974 file_size_bigint shadow, with the + # legacy file_size left NULL. + large_db_file.file_size = None + large_db_file.contentnode = large_file_node + large_db_file.save() + # Set the shadow directly; the mirror trigger leaves it alone because + # file_size is unchanged (NULL). + cc.File.objects.filter(pk=large_db_file.pk).update( + file_size_bigint=LARGE_FILE_SIZE + ) + self.large_file_checksum = large_db_file.checksum + set_channel_icon_encoding(self.content_channel) self.tempdb = create_content_database( self.content_channel, True, self.admin_user.id, True @@ -506,6 +541,24 @@ def test_contentnode_file_size_data(self): for file in files.prefetch_related("local_file"): self.assertEqual(file.file_size, file.local_file.file_size) + def test_localfile_file_size_bigint_matches_small_files(self): + # Files that fit in 32 bits write the same value to both columns. + local_files = kolibri_models.LocalFile.objects.exclude( + pk=self.large_file_checksum + ) + assert local_files.count() > 0 + for local_file in local_files: + self.assertEqual(local_file.file_size_bigint, local_file.file_size) + + def test_localfile_large_file_size_bigint(self): + # A >2.1 GB file keeps its real size in file_size_bigint and NULLs the + # legacy 32-bit file_size. + local_file = kolibri_models.LocalFile.objects.get( + pk=self.large_file_checksum + ) + self.assertEqual(local_file.file_size_bigint, LARGE_FILE_SIZE) + self.assertIsNone(local_file.file_size) + def test_file_included_presets_renderable(self): # Every non-supplementary (renderable) exported file carries its own preset bit. files = kolibri_models.File.objects.filter(supplementary=False) diff --git a/contentcuration/contentcuration/utils/publish.py b/contentcuration/contentcuration/utils/publish.py index 9b4cfe2c3b..cca7cc272d 100644 --- a/contentcuration/contentcuration/utils/publish.py +++ b/contentcuration/contentcuration/utils/publish.py @@ -65,6 +65,8 @@ PERSEUS_IMG_DIR = exercises.IMG_PLACEHOLDER + "/images" THUMBNAIL_DIMENSION = 128 MIN_SCHEMA_VERSION = "1" +# Largest value the legacy 32-bit LocalFile.file_size / File.file_size columns hold. +INT_32BIT_MAX = 2 ** 31 - 1 PUBLISHING_UPDATE_THRESHOLD = 3600 @@ -659,11 +661,23 @@ def create_associated_file_objects(kolibrinode, ccnode): create_associated_thumbnail(ccnode, ccfilemodel) or ccfilemodel ) + # The true size lives in the studio#5974 file_size_bigint shadow (the + # legacy 32-bit file_size cannot hold >2.1 GB); fall back to file_size + # for rows the shadow has not been backfilled onto yet. + real_size = ccfilemodel.file_size_bigint + if real_size is None: + real_size = ccfilemodel.file_size + if real_size is not None and real_size > INT_32BIT_MAX: + legacy_size = None + else: + legacy_size = real_size + kolibrilocalfilemodel, new = kolibrimodels.LocalFile.objects.get_or_create( pk=ccfilemodel.checksum, defaults={ "extension": fformat.extension, - "file_size": ccfilemodel.file_size, + "file_size": legacy_size, + "file_size_bigint": real_size, }, ) @@ -687,7 +701,7 @@ def create_associated_file_objects(kolibrinode, ccnode): checksum=ccfilemodel.checksum, extension=fformat.extension, available=True, # TODO: Set this to False, once we have availability stamping implemented in Kolibri - file_size=ccfilemodel.file_size, + file_size=legacy_size, contentnode=kolibrinode, preset=preset.pk, supplementary=preset.supplementary, From f5f87d0f7f5b37a84c4e72e678fca31600bb76d1 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, 13 Jul 2026 01:17:16 +0000 Subject: [PATCH 20/25] [pre-commit.ci lite] apply automatic fixes --- contentcuration/contentcuration/tests/test_exportchannel.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/contentcuration/contentcuration/tests/test_exportchannel.py b/contentcuration/contentcuration/tests/test_exportchannel.py index 09113398f1..fdd1d51269 100644 --- a/contentcuration/contentcuration/tests/test_exportchannel.py +++ b/contentcuration/contentcuration/tests/test_exportchannel.py @@ -553,9 +553,7 @@ def test_localfile_file_size_bigint_matches_small_files(self): def test_localfile_large_file_size_bigint(self): # A >2.1 GB file keeps its real size in file_size_bigint and NULLs the # legacy 32-bit file_size. - local_file = kolibri_models.LocalFile.objects.get( - pk=self.large_file_checksum - ) + local_file = kolibri_models.LocalFile.objects.get(pk=self.large_file_checksum) self.assertEqual(local_file.file_size_bigint, LARGE_FILE_SIZE) self.assertIsNone(local_file.file_size) From d0b294be0ee3ef447b072adf64819173d06fa172 Mon Sep 17 00:00:00 2001 From: Marcella Maki Date: Tue, 21 Jul 2026 17:01:11 -0400 Subject: [PATCH 21/25] Add ability to send a notification email on review of community library submission --- contentcuration/contentcuration/models.py | 51 +++++++++++++++++++ .../submission_resolved_email.html | 27 ++++++++++ .../test_community_library_submission.py | 16 ++++++ .../viewsets/community_library_submission.py | 2 + 4 files changed, 96 insertions(+) create mode 100644 contentcuration/contentcuration/templates/community_library/submission_resolved_email.html diff --git a/contentcuration/contentcuration/models.py b/contentcuration/contentcuration/models.py index 1f0c1ec8c5..bcf5b9bdc2 100644 --- a/contentcuration/contentcuration/models.py +++ b/contentcuration/contentcuration/models.py @@ -48,6 +48,8 @@ from django.db.models.query_utils import DeferredAttribute from django.db.models.sql import Query from django.dispatch import receiver +from django.template.loader import render_to_string +from django.urls import reverse from django.utils import timezone from django.utils.translation import gettext as _ from django_cte import CTEManager @@ -86,7 +88,9 @@ from contentcuration.db.models.manager import CustomContentNodeTreeManager from contentcuration.db.models.manager import CustomManager from contentcuration.utils.cache import delete_public_channel_cache_keys +from contentcuration.utils.messages import get_messages from contentcuration.utils.parser import load_json_string +from contentcuration.utils.urls import canonical_url from contentcuration.viewsets.sync.constants import ALL_CHANGES from contentcuration.viewsets.sync.constants import ALL_TABLES from contentcuration.viewsets.sync.constants import PUBLISHABLE_CHANGE_TABLES @@ -3049,6 +3053,53 @@ def notify_update_to_channel_editors(self, exclude_user_id=None): User.notify_users(editors, date=self.date_updated) + def send_resolution_email(self): + """ + Send an email to the submission author letting them know their + Community Library submission has been resolved (approved or + rejected). + """ + is_approved = self.status == community_library_submission.STATUS_APPROVED + + community_strings = get_messages().get("CommunityChannelsStrings", {}) + status_message = ( + community_strings["approvedStatus"] + if is_approved + else community_strings["flaggedStatus"] + ) + subject_text = "{}: {}".format( + community_strings["communityLibrarySubmissionLabel"], status_message + ) + + subject = render_to_string( + "registration/custom_email_subject.txt", + {"subject": subject_text}, + ) + subject = "".join(subject.splitlines()) + + message = render_to_string( + "community_library/submission_resolved_email.html", + { + "name": self.author.get_full_name(), + "channel": self.channel, + "channel_url": canonical_url( + reverse("channel", kwargs={"channel_id": self.channel.pk}) + ), + "approved": is_approved, + "status_message": ( + community_strings["availableStatus"] + if is_approved + else community_strings["needsChangesPrimaryInfo"] + ), + "feedback_notes_label": community_strings["feedbackNotesLabel"], + "feedback_notes": self.feedback_notes, + }, + ) + + self.author.email_user( + subject, message, settings.DEFAULT_FROM_EMAIL, html_message=message + ) + @classmethod def filter_view_queryset(cls, queryset, user): if user.is_anonymous: diff --git a/contentcuration/contentcuration/templates/community_library/submission_resolved_email.html b/contentcuration/contentcuration/templates/community_library/submission_resolved_email.html new file mode 100644 index 0000000000..0c187e947a --- /dev/null +++ b/contentcuration/contentcuration/templates/community_library/submission_resolved_email.html @@ -0,0 +1,27 @@ + +{% load i18n %} + + + + + + + {% autoescape off %} +

{% blocktrans with name=name %}Hello {{ name }},{% endblocktrans %}

+ +

{% blocktrans with channel_name=channel.name %}{{ channel_name }}{% endblocktrans %} ({{ channel_url }})

+ +

{{ status_message }}

+ + {% if feedback_notes %} +

{{ feedback_notes_label }}: {{ feedback_notes }}

+ {% endif %} + +

+ {% translate "Thanks for using Kolibri Studio!" %} +
+ {% translate "The Learning Equality Team" %} +

+ {% endautoescape %} + + diff --git a/contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py b/contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py index 4cd51fb2a1..9c960149ad 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py +++ b/contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py @@ -2,6 +2,7 @@ from unittest import mock import pytz +from django.core import mail from django.urls import reverse from contentcuration.constants import ( @@ -731,6 +732,13 @@ def test_resolve_submission__accept_correct(self, apply_task_mock): channel_id=self.submission.channel.id, ) + self.assertEqual(len(mail.outbox), 1) + sent_email = mail.outbox[0] + self.assertEqual(sent_email.to, [self.submission.author.email]) + self.assertIn("approved", sent_email.subject.lower()) + self.assertIn("available in community library", sent_email.body.lower()) + self.assertIn(self.submission.channel.name, sent_email.body) + @mock.patch( "contentcuration.viewsets.community_library_submission.apply_channel_changes_task" ) @@ -770,6 +778,14 @@ def test_resolve_submission__reject_correct(self, apply_task_mock): ) apply_task_mock.fetch_or_enqueue.assert_not_called() + self.assertEqual(len(mail.outbox), 1) + sent_email = mail.outbox[0] + self.assertEqual(sent_email.to, [self.submission.author.email]) + self.assertIn("needs changes", sent_email.subject.lower()) + self.assertIn("needs changes", sent_email.body.lower()) + self.assertIn(self.submission.channel.name, sent_email.body) + self.assertIn(self.feedback_notes, sent_email.body) + 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/community_library_submission.py b/contentcuration/contentcuration/viewsets/community_library_submission.py index 33fc6f9a94..ff9723f92f 100644 --- a/contentcuration/contentcuration/viewsets/community_library_submission.py +++ b/contentcuration/contentcuration/viewsets/community_library_submission.py @@ -358,4 +358,6 @@ def resolve(self, request, pk=None): published_version.id ) + submission.send_resolution_email() + return Response(self.serialize_object()) From 5e176a5d2818ac9d22aea83d61e2ddba92558e4c Mon Sep 17 00:00:00 2001 From: Alex Velez Date: Fri, 31 Jul 2026 14:52:09 -0500 Subject: [PATCH 22/25] fix: dedupe ChannelVersion.included_licenses in API responses Some ChannelVersion rows were backfilled with duplicate license IDs by a past one-off migration command, causing repeated license chips/names to show in the community library submission and review UIs. Dedupe defensively in the two viewset read paths that serve this field, rather than backfilling the affected rows. Co-Authored-By: Claude Sonnet 5 --- .../tests/viewsets/test_channel.py | 35 +++++++++++++++++++ .../contentcuration/viewsets/channel.py | 17 +++++++++ 2 files changed, 52 insertions(+) diff --git a/contentcuration/contentcuration/tests/viewsets/test_channel.py b/contentcuration/contentcuration/tests/viewsets/test_channel.py index 2c72cf2dcc..d36866aca7 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_channel.py +++ b/contentcuration/contentcuration/tests/viewsets/test_channel.py @@ -1478,6 +1478,21 @@ def test_get_version_detail_returns_all_fields(self): for field in expected_fields: self.assertIn(field, data, f"Field '{field}' should be in response") + def test_get_version_detail_dedupes_duplicated_licenses(self): + """Test that duplicated license ids stored on the ChannelVersion (e.g. from a + backfill bug) are deduped in the response.""" + self.channel_version.included_licenses = [1, 2, 2, 1] + self.channel_version.non_distributable_licenses_included = [1, 1] + self.channel_version.save() + + url = reverse("channel-version-detail", kwargs={"pk": self.channel.id}) + response = self.client.get(url) + + self.assertEqual(response.status_code, 200) + data = response.json() + self.assertEqual(data["included_licenses"], [1, 2]) + self.assertEqual(data["non_distributable_licenses_included"], [1]) + def test_get_version_detail_excludes_special_permissions_included(self): """Test that special_permissions_included is not in the response.""" special_license = AuditedSpecialPermissionsLicense.objects.create( @@ -1587,6 +1602,26 @@ def test_get_specific_channel_version(self): self.assertEqual(data["channel"], self.channel.id) self.assertEqual(data["version"], channel_version.version) + def test_get_channel_version_dedupes_duplicated_licenses(self): + """Test that duplicated license ids stored on the ChannelVersion (e.g. from a + backfill bug) are deduped in both list and detail responses.""" + channel_version = ChannelVersion.objects.filter(channel=self.channel).first() + channel_version.included_licenses = [2, 1, 2, 1] + channel_version.save() + + detail_url = reverse("channelversion-detail", kwargs={"pk": channel_version.id}) + detail_response = self.client.get(detail_url) + self.assertEqual(detail_response.status_code, 200) + self.assertEqual(detail_response.json()["included_licenses"], [1, 2]) + + list_url = reverse("channelversion-list") + f"?channel={self.channel.id}" + list_response = self.client.get(list_url) + self.assertEqual(list_response.status_code, 200) + data = list_response.json() + results = data["results"] if "results" in data else data + version_data = next(r for r in results if r["id"] == channel_version.id) + self.assertEqual(version_data["included_licenses"], [1, 2]) + def test_get_channel_versions_ordering(self): """Test ordering of channel versions.""" url = ( diff --git a/contentcuration/contentcuration/viewsets/channel.py b/contentcuration/contentcuration/viewsets/channel.py index 3175e8180a..238224226c 100644 --- a/contentcuration/contentcuration/viewsets/channel.py +++ b/contentcuration/contentcuration/viewsets/channel.py @@ -953,6 +953,17 @@ def get_version_detail(self, request, pk=None) -> Response: if not version_data: return Response({}) + # Older ChannelVersion rows may have been backfilled with duplicate + # license IDs - dedupe defensively. + if version_data.get("included_licenses") is not None: + version_data["included_licenses"] = sorted( + set(version_data["included_licenses"]) + ) + if version_data.get("non_distributable_licenses_included") is not None: + version_data["non_distributable_licenses_included"] = sorted( + set(version_data["non_distributable_licenses_included"]) + ) + return Response(version_data) @action( @@ -1073,6 +1084,12 @@ class ChannelVersionViewSet(ReadOnlyValuesViewset): ordering_fields = ["version"] ordering = "-version" + # Older ChannelVersion rows may have been backfilled with duplicate + # license IDs - dedupe defensively. + field_map = { + "included_licenses": lambda item: sorted(set(item["included_licenses"] or [])) + } + values = ( "id", "channel", From 9406e653e13566db4f0e378d4d5b601564931df9 Mon Sep 17 00:00:00 2001 From: Alex Velez Date: Fri, 31 Jul 2026 15:16:19 -0500 Subject: [PATCH 23/25] fix: preserve channel status filter when navigating back to channels table Navigating back to the admin Channels table (e.g. after opening a channel and using the browser back button) reset the status filter even though it was correctly restored from the URL. An immediate watcher unconditionally overwrote it to the first available option on every mount instead of only defaulting when the filter is actually unset. Since the status filter is derived from the current channel type's options, a status no longer valid for a new type already reads back as unset, so a single "is it set" check covers both the type-change reset and back-navigation preservation. Co-Authored-By: Claude Sonnet 5 --- .../pages/Channels/ChannelTable.vue | 9 +++++-- .../Channels/__tests__/channelTable.spec.js | 27 ++++++++++++++++--- 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/contentcuration/contentcuration/frontend/administration/pages/Channels/ChannelTable.vue b/contentcuration/contentcuration/frontend/administration/pages/Channels/ChannelTable.vue index 2dfdec7fae..c5ae96e7d5 100644 --- a/contentcuration/contentcuration/frontend/administration/pages/Channels/ChannelTable.vue +++ b/contentcuration/contentcuration/frontend/administration/pages/Channels/ChannelTable.vue @@ -304,11 +304,16 @@ fetchQueryParams: keywordSearchFetchQueryParams, } = useKeywordSearch(); + // channelStatusFilter is derived from the current channelType's options, so an + // existing status that's no longer valid for the new type already reads back as + // unset - only default it to the first option in that case. watch( channelTypeFilter, () => { - const options = channelStatusOptions.value; - channelStatusFilter.value = options.length ? options[0].value : null; + if (!channelStatusFilter.value) { + const options = channelStatusOptions.value; + channelStatusFilter.value = options.length ? options[0].value : null; + } }, { immediate: true }, ); diff --git a/contentcuration/contentcuration/frontend/administration/pages/Channels/__tests__/channelTable.spec.js b/contentcuration/contentcuration/frontend/administration/pages/Channels/__tests__/channelTable.spec.js index 07c08be509..ec50bc8925 100644 --- a/contentcuration/contentcuration/frontend/administration/pages/Channels/__tests__/channelTable.spec.js +++ b/contentcuration/contentcuration/frontend/administration/pages/Channels/__tests__/channelTable.spec.js @@ -11,8 +11,8 @@ localVue.use(router); const channelList = ['test', 'channel', 'table']; -function makeWrapper(store) { - router.replace({ name: RouteNames.CHANNELS }); +function makeWrapper(store, query = {}) { + router.replace({ name: RouteNames.CHANNELS, query }); return mount(ChannelTable, { router, @@ -71,13 +71,32 @@ describe('channelTable', () => { expect(router.currentRoute.query.keywords).toBe('keyword test'); }); - it('changing channel type filter should reset channel status filter', async () => { + it('changing channel type filter should reset channel status filter when it is no longer valid', async () => { + wrapper.vm.channelTypeFilter = ChannelTypeFilter.COMMUNITY_LIBRARY; + wrapper.vm.channelStatusFilter = 'needsReview'; + await wrapper.vm.$nextTick(); + // Kolibri library channels have no "needs review" status + wrapper.vm.channelTypeFilter = ChannelTypeFilter.KOLIBRI_LIBRARY; + await wrapper.vm.$nextTick(); + expect(wrapper.vm.channelStatusFilter).toBe('live'); + }); + it('changing channel type filter should keep the channel status filter when it is still valid', async () => { wrapper.vm.channelTypeFilter = ChannelTypeFilter.COMMUNITY_LIBRARY; wrapper.vm.channelStatusFilter = 'published'; await wrapper.vm.$nextTick(); + // "published" is a valid status for unlisted channels too wrapper.vm.channelTypeFilter = ChannelTypeFilter.UNLISTED; await wrapper.vm.$nextTick(); - expect(wrapper.vm.channelStatusFilter).toBe('live'); + expect(wrapper.vm.channelStatusFilter).toBe('published'); + }); + it('should preserve a valid channel status filter already present in the URL on mount', () => { + // Simulates navigating back to this page with filters still in the URL + // (e.g. after opening a channel and hitting the browser back button). + const backNavWrapper = makeWrapper(store, { + channelType: ChannelTypeFilter.COMMUNITY_LIBRARY, + channelStatus: 'needsReview', + }); + expect(backNavWrapper.vm.channelStatusFilter).toBe('needsReview'); }); }); describe('selection', () => { From 7295cc799511c1eef1e5a84ec84029322741f937 Mon Sep 17 00:00:00 2001 From: Alex Velez Date: Fri, 31 Jul 2026 15:25:05 -0500 Subject: [PATCH 24/25] fix: remove fixed width from community library status button The fixed width clipped longer status labels. Let it size to content. Co-Authored-By: Claude Sonnet 5 --- .../administration/components/CommunityLibraryStatusButton.vue | 1 - 1 file changed, 1 deletion(-) diff --git a/contentcuration/contentcuration/frontend/administration/components/CommunityLibraryStatusButton.vue b/contentcuration/contentcuration/frontend/administration/components/CommunityLibraryStatusButton.vue index 46006c211b..ab95f69dcb 100644 --- a/contentcuration/contentcuration/frontend/administration/components/CommunityLibraryStatusButton.vue +++ b/contentcuration/contentcuration/frontend/administration/components/CommunityLibraryStatusButton.vue @@ -105,7 +105,6 @@ .community-library-status-button { @extend %md-standard-func; - width: 9em; padding: 4px; color: v-bind('labelColor'); background-color: v-bind('color'); From 3fab968b80e53cc2b5cfd65faf58ba2b3f03bf60 Mon Sep 17 00:00:00 2001 From: Marcella Maki Date: Tue, 4 Aug 2026 17:42:38 -0400 Subject: [PATCH 25/25] Simplify strings and update the place in the workflow that the mail is initiated --- contentcuration/contentcuration/models.py | 63 +++++++++---------- .../submission_resolved_email.html | 16 +++-- .../test_community_library_submission.py | 54 +++++++++++++++- contentcuration/contentcuration/utils/i18n.py | 14 +++++ .../viewsets/community_library_submission.py | 13 +++- 5 files changed, 119 insertions(+), 41 deletions(-) diff --git a/contentcuration/contentcuration/models.py b/contentcuration/contentcuration/models.py index bcf5b9bdc2..a1918cdd85 100644 --- a/contentcuration/contentcuration/models.py +++ b/contentcuration/contentcuration/models.py @@ -51,6 +51,7 @@ from django.template.loader import render_to_string from django.urls import reverse from django.utils import timezone +from django.utils import translation from django.utils.translation import gettext as _ from django_cte import CTEManager from django_cte import CTEQuerySet @@ -88,7 +89,7 @@ from contentcuration.db.models.manager import CustomContentNodeTreeManager from contentcuration.db.models.manager import CustomManager from contentcuration.utils.cache import delete_public_channel_cache_keys -from contentcuration.utils.messages import get_messages +from contentcuration.utils.i18n import closest_supported_locale from contentcuration.utils.parser import load_json_string from contentcuration.utils.urls import canonical_url from contentcuration.viewsets.sync.constants import ALL_CHANGES @@ -3061,40 +3062,36 @@ def send_resolution_email(self): """ is_approved = self.status == community_library_submission.STATUS_APPROVED - community_strings = get_messages().get("CommunityChannelsStrings", {}) - status_message = ( - community_strings["approvedStatus"] - if is_approved - else community_strings["flaggedStatus"] - ) - subject_text = "{}: {}".format( - community_strings["communityLibrarySubmissionLabel"], status_message - ) + channel_language = self.channel.language + locale_code = ( + closest_supported_locale(channel_language.lang_code) + if channel_language + else None + ) or settings.LANGUAGE_CODE + with translation.override(locale_code): + if is_approved: + subject_text = _("Your Community Library submission has been approved") + else: + subject_text = _("Your Community Library submission needs changes") - subject = render_to_string( - "registration/custom_email_subject.txt", - {"subject": subject_text}, - ) - subject = "".join(subject.splitlines()) + subject = render_to_string( + "registration/custom_email_subject.txt", + {"subject": subject_text}, + ) + subject = "".join(subject.splitlines()) - message = render_to_string( - "community_library/submission_resolved_email.html", - { - "name": self.author.get_full_name(), - "channel": self.channel, - "channel_url": canonical_url( - reverse("channel", kwargs={"channel_id": self.channel.pk}) - ), - "approved": is_approved, - "status_message": ( - community_strings["availableStatus"] - if is_approved - else community_strings["needsChangesPrimaryInfo"] - ), - "feedback_notes_label": community_strings["feedbackNotesLabel"], - "feedback_notes": self.feedback_notes, - }, - ) + message = render_to_string( + "community_library/submission_resolved_email.html", + { + "name": self.author.get_full_name(), + "channel": self.channel, + "channel_url": canonical_url( + reverse("channel", kwargs={"channel_id": self.channel.pk}) + ), + "approved": is_approved, + "feedback_notes": self.feedback_notes, + }, + ) self.author.email_user( subject, message, settings.DEFAULT_FROM_EMAIL, html_message=message diff --git a/contentcuration/contentcuration/templates/community_library/submission_resolved_email.html b/contentcuration/contentcuration/templates/community_library/submission_resolved_email.html index 0c187e947a..f5aec4f346 100644 --- a/contentcuration/contentcuration/templates/community_library/submission_resolved_email.html +++ b/contentcuration/contentcuration/templates/community_library/submission_resolved_email.html @@ -1,20 +1,25 @@ {% load i18n %} - +{% get_current_language as LANGUAGE_CODE %} +{% get_current_language_bidi as LANGUAGE_BIDI %} + - {% autoescape off %}

{% blocktrans with name=name %}Hello {{ name }},{% endblocktrans %}

-

{% blocktrans with channel_name=channel.name %}{{ channel_name }}{% endblocktrans %} ({{ channel_url }})

+

{{ channel.name }} ({{ channel_url }})

-

{{ status_message }}

+ {% if approved %} +

{% translate "Your submission has been approved and will be added to the Community Library soon." %}

+ {% else %} +

{% translate "Your submission needs changes. Please review the notes below and resubmit after all feedback has been addressed." %}

+ {% endif %} {% if feedback_notes %} -

{{ feedback_notes_label }}: {{ feedback_notes }}

+

{% translate "Notes from the reviewer" %}: {{ feedback_notes }}

{% endif %}

@@ -22,6 +27,5 @@
{% translate "The Learning Equality Team" %}

- {% endautoescape %} diff --git a/contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py b/contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py index 9c960149ad..149cd3ade6 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py +++ b/contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py @@ -17,6 +17,7 @@ from contentcuration.tests import testdata from contentcuration.tests.base import StudioAPITestCase from contentcuration.tests.helpers import reverse_with_query +from contentcuration.utils.urls import canonical_url from contentcuration.viewsets.sync.constants import ADDED_TO_COMMUNITY_LIBRARY @@ -736,8 +737,53 @@ def test_resolve_submission__accept_correct(self, apply_task_mock): sent_email = mail.outbox[0] self.assertEqual(sent_email.to, [self.submission.author.email]) self.assertIn("approved", sent_email.subject.lower()) - self.assertIn("available in community library", sent_email.body.lower()) + self.assertIn("approved", sent_email.body.lower()) self.assertIn(self.submission.channel.name, sent_email.body) + self.assertIn( + canonical_url( + reverse("channel", kwargs={"channel_id": self.submission.channel.pk}) + ), + sent_email.body, + ) + + @mock.patch( + "contentcuration.viewsets.community_library_submission.apply_channel_changes_task" + ) + @mock.patch( + "contentcuration.models.CommunityLibrarySubmission.send_resolution_email", + side_effect=Exception("SMTP is down"), + ) + def test_resolve_submission__accept_correct_when_email_fails( + self, send_email_mock, apply_task_mock + ): + """A failure to notify the author shouldn't undo or fail the resolution.""" + self.client.force_authenticate(user=self.admin_user) + response = self.client.post( + reverse( + "admin-community-library-submission-resolve", + args=[self.submission.id], + ), + self.resolve_approve_metadata, + format="json", + ) + self.assertEqual(response.status_code, 200, response.content) + + resolved_submission = CommunityLibrarySubmission.objects.get( + id=self.submission.id + ) + self.assertEqual( + resolved_submission.status, + community_library_submission_constants.STATUS_APPROVED, + ) + Change.objects.get( + channel=self.submission.channel, + change_type=ADDED_TO_COMMUNITY_LIBRARY, + ) + apply_task_mock.fetch_or_enqueue.assert_called_once_with( + self.admin_user, + channel_id=self.submission.channel.id, + ) + self.assertEqual(len(mail.outbox), 0) @mock.patch( "contentcuration.viewsets.community_library_submission.apply_channel_changes_task" @@ -784,6 +830,12 @@ def test_resolve_submission__reject_correct(self, apply_task_mock): self.assertIn("needs changes", sent_email.subject.lower()) self.assertIn("needs changes", sent_email.body.lower()) self.assertIn(self.submission.channel.name, sent_email.body) + self.assertIn( + canonical_url( + reverse("channel", kwargs={"channel_id": self.submission.channel.pk}) + ), + sent_email.body, + ) self.assertIn(self.feedback_notes, sent_email.body) def test_resolve_submission__reject_missing_resolution_reason(self): diff --git a/contentcuration/contentcuration/utils/i18n.py b/contentcuration/contentcuration/utils/i18n.py index dd60689ce6..cde1b00d72 100644 --- a/contentcuration/contentcuration/utils/i18n.py +++ b/contentcuration/contentcuration/utils/i18n.py @@ -41,6 +41,20 @@ def _get_language_info(): LANGUAGE_INFO = _get_language_info() +def closest_supported_locale(lang_code): + """ + Given a content language's primary code (e.g. "es", "fr"), return the + Studio UI locale in SUPPORTED_LANGUAGES that matches it, ignoring region, + or None if Studio has no UI translation for that language. + """ + if not lang_code: + return None + for supported in SUPPORTED_LANGUAGES: + if supported.split("-")[0] == lang_code: + return supported + return None + + def language_globals(): language_code = get_language() lang_dir = "rtl" if get_language_bidi() else "ltr" diff --git a/contentcuration/contentcuration/viewsets/community_library_submission.py b/contentcuration/contentcuration/viewsets/community_library_submission.py index ff9723f92f..167798d973 100644 --- a/contentcuration/contentcuration/viewsets/community_library_submission.py +++ b/contentcuration/contentcuration/viewsets/community_library_submission.py @@ -1,3 +1,5 @@ +import logging + from django.db.models import OuterRef from django.db.models import Subquery from django_filters import BaseInFilter @@ -36,6 +38,8 @@ ) from contentcuration.viewsets.user import IsAdminUser +logger = logging.getLogger(__name__) + class ChoiceInFilter(BaseInFilter, ChoiceFilter): """ @@ -358,6 +362,13 @@ def resolve(self, request, pk=None): published_version.id ) - submission.send_resolution_email() + try: + submission.send_resolution_email() + except Exception: + # The resolution itself has already been committed; a failure to + # notify the author shouldn't turn that into a 500 response. + logger.exception( + "Failed to send resolution email for submission %s", submission.pk + ) return Response(self.serialize_object())