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/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/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/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'); 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', () => { 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/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') { 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); 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/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/migrations/0168_alter_assessmentitem_type.py b/contentcuration/contentcuration/migrations/0168_alter_assessmentitem_type.py new file mode 100644 index 0000000000..cc17df476a --- /dev/null +++ b/contentcuration/contentcuration/migrations/0168_alter_assessmentitem_type.py @@ -0,0 +1,30 @@ +# Generated by Django 3.2.25 on 2026-07-12 22:39 +from django.db import migrations +from django.db import 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/contentcuration/contentcuration/models.py b/contentcuration/contentcuration/models.py index ae2ab2b615..a1918cdd85 100644 --- a/contentcuration/contentcuration/models.py +++ b/contentcuration/contentcuration/models.py @@ -48,7 +48,10 @@ 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 import translation from django.utils.translation import gettext as _ from django_cte import CTEManager from django_cte import CTEQuerySet @@ -79,13 +82,16 @@ 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 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.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 from contentcuration.viewsets.sync.constants import ALL_TABLES from contentcuration.viewsets.sync.constants import PUBLISHABLE_CHANGE_TABLES @@ -3048,6 +3054,49 @@ 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 + + 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()) + + 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 + ) + @classmethod def filter_view_queryset(cls, queryset, user): if user.is_anonymous: @@ -3255,6 +3304,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 +3317,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 +3334,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 +3547,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 = [ 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/templates/community_library/submission_resolved_email.html b/contentcuration/contentcuration/templates/community_library/submission_resolved_email.html new file mode 100644 index 0000000000..f5aec4f346 --- /dev/null +++ b/contentcuration/contentcuration/templates/community_library/submission_resolved_email.html @@ -0,0 +1,31 @@ + +{% load i18n %} +{% get_current_language as LANGUAGE_CODE %} +{% get_current_language_bidi as LANGUAGE_BIDI %} + + + + + + +

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

+ +

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

+ + {% 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 %} +

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

+ {% endif %} + +

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

+ + 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/contentcuration/contentcuration/tests/test_exportchannel.py b/contentcuration/contentcuration/tests/test_exportchannel.py index 786c4ccec1..fdd1d51269 100644 --- a/contentcuration/contentcuration/tests/test_exportchannel.py +++ b/contentcuration/contentcuration/tests/test_exportchannel.py @@ -16,9 +16,11 @@ 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 +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 @@ -56,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)) @@ -386,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 @@ -505,6 +541,37 @@ 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) + 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/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/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/tests/test_user.py b/contentcuration/contentcuration/tests/test_user.py index d772007d57..585932aa9c 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,80 @@ 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"], "") + + 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): 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/tests/viewsets/test_community_library_submission.py b/contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py index 4cd51fb2a1..149cd3ade6 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 ( @@ -16,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 @@ -731,6 +733,58 @@ 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("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" ) @@ -770,6 +824,20 @@ 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( + 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): self.client.force_authenticate(user=self.admin_user) metadata = self.resolve_reject_metadata.copy() 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/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: diff --git a/contentcuration/contentcuration/utils/files.py b/contentcuration/contentcuration/utils/files.py index 0cb447a601..aa7c3a9db4 100644 --- a/contentcuration/contentcuration/utils/files.py +++ b/contentcuration/contentcuration/utils/files.py @@ -1,5 +1,7 @@ import base64 +import codecs import copy +import mimetypes import os import re import tempfile @@ -14,8 +16,12 @@ 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 +# 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 @@ -24,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( @@ -45,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 @@ -93,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 @@ -196,3 +214,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..7ad581f56d 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,18 @@ 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): + # 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) 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/utils/publish.py b/contentcuration/contentcuration/utils/publish.py index 6e18dea57e..cca7cc272d 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 @@ -64,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 @@ -658,20 +661,47 @@ 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, }, ) + 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, 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, @@ -679,6 +709,7 @@ def create_associated_file_objects(kolibrinode, ccnode): thumbnail=preset.thumbnail, priority=preset.order, local_file=kolibrilocalfilemodel, + included_presets=included_presets, ) 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 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", diff --git a/contentcuration/contentcuration/viewsets/community_library_submission.py b/contentcuration/contentcuration/viewsets/community_library_submission.py index 33fc6f9a94..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,4 +362,13 @@ def resolve(self, request, pk=None): published_version.id ) + 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()) 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) diff --git a/contentcuration/kolibri_content/base_models.py b/contentcuration/kolibri_content/base_models.py index 220558a0bb..c2d5417fca 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 @@ -149,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/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 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..7345f9b3d0 --- /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 +from django.db import 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_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/0009_file_included_presets.py b/contentcuration/kolibri_public/migrations/0009_file_included_presets.py new file mode 100644 index 0000000000..17d77751bd --- /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 +from django.db import 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), + ), + ] 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), + ), + ] 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. 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..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 @@ -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..1a46dcee2d 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 @@ -177,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