From 2e3dc7c849d82b7356bcaf51212c992fd572f9d2 Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Sat, 4 Jul 2026 21:47:36 -0700 Subject: [PATCH 1/3] feat: land ricecooker assessment items as QTI Refactor create_exercises to delegate item construction and schema validation to AssessmentItemSerializer instead of duplicating that logic. Native QTI (editor-shaped or arbitrary) is persisted and schema-validated directly; legacy/structured questions are converted to QTI on ingest; raw Perseus passes through unchanged; a Perseus custom interaction is unwrapped back to its raw perseus_question by reading its data-perseus-path payload. Referenced media is mapped via the existing map_files_to_assessment_item helper. Co-Authored-By: Claude Sonnet 5 --- .../tests/utils/qti/test_ingest.py | 78 +++++++ .../tests/utils/test_create_exercises.py | 39 ++++ .../tests/views/test_views_internal.py | 195 +++++++++++++++++- .../tests/viewsets/test_assessmentitem.py | 22 ++ .../utils/assessment/qti/ingest.py | 65 ++++++ .../contentcuration/views/internal.py | 114 ++++++++-- .../viewsets/assessmentitem.py | 4 +- 7 files changed, 495 insertions(+), 22 deletions(-) create mode 100644 contentcuration/contentcuration/tests/utils/qti/test_ingest.py create mode 100644 contentcuration/contentcuration/tests/utils/test_create_exercises.py create mode 100644 contentcuration/contentcuration/utils/assessment/qti/ingest.py diff --git a/contentcuration/contentcuration/tests/utils/qti/test_ingest.py b/contentcuration/contentcuration/tests/utils/qti/test_ingest.py new file mode 100644 index 0000000000..f210a6e2eb --- /dev/null +++ b/contentcuration/contentcuration/tests/utils/qti/test_ingest.py @@ -0,0 +1,78 @@ +import unittest + +from contentcuration.tests.utils.qti.test_validation import VALID_CHOICE_ITEM +from contentcuration.utils.assessment.qti.ingest import convert_legacy_question_to_qti +from contentcuration.utils.assessment.qti.ingest import ( + find_perseus_custom_interaction_path, +) +from contentcuration.utils.assessment.qti.ingest import ( + strip_content_storage_placeholder, +) +from contentcuration.utils.assessment.qti.media import get_qti_media_references +from contentcuration.utils.assessment.qti.validation import validate_qti_item + + +class StripContentStoragePlaceholderTests(unittest.TestCase): + def test_strips_placeholder_leaving_bare_filename(self): + text = "Look: ![](${☣ CONTENTSTORAGE}/abc123.png)" + self.assertEqual( + strip_content_storage_placeholder(text), "Look: ![](abc123.png)" + ) + + +class ConvertLegacyQuestionToQTITests(unittest.TestCase): + def test_convert_legacy_question_to_qti_strips_placeholder_from_question_and_answers( + self, + ): + question_data = { + "type": "multiple_selection", + "assessment_id": "abf45e8fd7f151adb1b3df2d751e945e", + "question": "Which is red? ![](${☣ CONTENTSTORAGE}/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.png)", # noqa + "answers": '[{"answer": "Apple ![](${☣ CONTENTSTORAGE}/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb.png)", "correct": true, "order": 0}, {"answer": "Sky", "correct": false, "order": 1}]', # noqa + "randomize": False, + } + result = convert_legacy_question_to_qti(question_data) + validation_result = validate_qti_item(result.xml) + self.assertTrue(validation_result.is_valid, validation_result.errors) + self.assertEqual( + get_qti_media_references(result.xml), + { + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.png", + "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb.png", + }, + ) + + +def _custom_interaction_item_xml(data_type, path_attr, path_value): + return """ + + + + +""" % ( + data_type, + path_attr, + path_value, + ) + + +class FindPerseusCustomInteractionPathTests(unittest.TestCase): + def test_find_perseus_custom_interaction_path_detects_wrapper(self): + raw_data = _custom_interaction_item_xml( + "perseus", "data-perseus-path", "cccccccccccccccccccccccccccccccc.json" + ) + self.assertEqual( + find_perseus_custom_interaction_path(raw_data), + "cccccccccccccccccccccccccccccccc.json", + ) + + def test_find_perseus_custom_interaction_path_returns_none_when_absent(self): + self.assertIsNone(find_perseus_custom_interaction_path(VALID_CHOICE_ITEM)) + + def test_find_perseus_custom_interaction_path_ignores_other_vendor_types(self): + raw_data = _custom_interaction_item_xml( + "other-vendor", "data-other-path", "cccccccccccccccccccccccccccccccc.json" + ) + self.assertIsNone(find_perseus_custom_interaction_path(raw_data)) diff --git a/contentcuration/contentcuration/tests/utils/test_create_exercises.py b/contentcuration/contentcuration/tests/utils/test_create_exercises.py new file mode 100644 index 0000000000..0373921bba --- /dev/null +++ b/contentcuration/contentcuration/tests/utils/test_create_exercises.py @@ -0,0 +1,39 @@ +from django.core.exceptions import ValidationError as DjangoValidationError +from le_utils.constants import exercises + +from contentcuration.tests.base import StudioTestCase +from contentcuration.tests.testdata import channel +from contentcuration.views.internal import create_exercises + + +class CreateExercisesInvalidQTITestCase(StudioTestCase): + def test_invalid_qti_raw_data_raises_node_validation_error(self): + node = channel().main_tree + questions = [ + { + "assessment_id": "dddddddddddddddddddddddddddddddd", + "type": exercises.QTI, + "raw_data": "this is not valid QTI", + "files": [], + "source_url": None, + "randomize": False, + } + ] + with self.assertRaises(DjangoValidationError): + create_exercises(self.admin_user, node, questions) + + def test_unsupported_legacy_question_type_raises_node_validation_error(self): + node = channel().main_tree + questions = [ + { + "assessment_id": "eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee", + "type": "not_a_real_question_type", + "question": "What?", + "answers": "[]", + "files": [], + "source_url": None, + "randomize": False, + } + ] + with self.assertRaises(DjangoValidationError): + create_exercises(self.admin_user, node, questions) diff --git a/contentcuration/contentcuration/tests/views/test_views_internal.py b/contentcuration/contentcuration/tests/views/test_views_internal.py index f503ebe538..98d45d0036 100644 --- a/contentcuration/contentcuration/tests/views/test_views_internal.py +++ b/contentcuration/contentcuration/tests/views/test_views_internal.py @@ -33,12 +33,15 @@ from ..testdata import fileobj_video from ..testdata import thumbnail_bytes from ..testdata import user +from ..utils.qti.test_validation import _item_xml from contentcuration import ricecooker_versions as rc from contentcuration.constants import completion_criteria as studio_completion_criteria from contentcuration.db.models.manager import ALLOWED_OVERRIDES from contentcuration.db.models.manager import EDIT_ALLOWED_OVERRIDES from contentcuration.models import Channel from contentcuration.models import ContentNode +from contentcuration.models import File +from contentcuration.utils.assessment.qti.validation import validate_qti_item from contentcuration.views import internal @@ -332,7 +335,7 @@ def setUp(self): "question": u"Which numbers are even?\n\nTest local image include: ![](${☣ CONTENTSTORAGE}/%s)" % self.exercise_image.filename(), "hints": "[]", - "answers": '[{"answer": "1", "correct": false, "order": 0}, {"answer": "2", "correct": True, "order": 1}, {"answer": "3", "correct": false, "order": 2}, {"answer": "4", "correct": true, "order": 3}, {"answer": "5", "correct": false, "order": 4}]', # noqa + "answers": '[{"answer": "1", "correct": false, "order": 0}, {"answer": "2", "correct": true, "order": 1}, {"answer": "3", "correct": false, "order": 2}, {"answer": "4", "correct": true, "order": 3}, {"answer": "5", "correct": false, "order": 4}]', # noqa "raw_data": "", "source_url": None, "randomize": False, @@ -473,6 +476,196 @@ def test_duplicate_assessment_item_returns_400_status_code(self): response.status_code ) + def test_native_qti_item_persisted_and_media_mapped(self): + """ + A native QTI question (raw_data XML + declared media) is persisted + verbatim as an exercises.QTI assessment item, and its media is mapped + without leaving stray/duplicate File rows behind. + """ + assessment_id = "1" * 32 + qti_exercise_image = fileobj_exercise_image(size=(50, 50), color="blue") + qti_raw_data = _item_xml( + "item_1", + "Sample Item", + '' + "choice_0" + "", + 'Pick the diagram. ' + 'diagram' + 'Option A' + 'Option B' + "" % qti_exercise_image.filename(), + ) + self.sample_data["content_data"][0]["questions"].append( + { + "assessment_id": assessment_id, + "type": "QTI", + "files": [ + { + "size": qti_exercise_image.file_size, + "preset": "exercise_image", + "filename": qti_exercise_image.filename(), + "original_filename": None, + "language": None, + "source_url": None, + } + ], + "question": "", + "hints": "[]", + "answers": "[]", + "raw_data": qti_raw_data, + "source_url": None, + "randomize": False, + } + ) + + response = self._make_request() + assert response.status_code == 200, response.content + + c = ContentNode.objects.get(title=self.title) + qti_item = c.assessment_items.get(assessment_id=assessment_id) + self.assertEqual(qti_item.type, exercises.QTI) + self.assertEqual(qti_item.raw_data, qti_raw_data) + self.assertEqual(qti_item.files.count(), 1) + attached_file = qti_item.files.first() + self.assertEqual(attached_file.filename(), qti_exercise_image.filename()) + self.assertEqual(attached_file.original_filename, "file") + # AssessmentItemSerializer.set_files() auto-creates its own File row + # for this checksum while saving (there's no pre-staged File to reuse, + # as ricecooker never stages one the way the browser editor does); that + # stray row must be cleaned up before map_files_to_assessment_item + # creates the real, correctly-attributed one. Only the untouched setUp + # fixture should remain unattached for this checksum. + self.assertEqual( + File.objects.filter( + checksum=qti_exercise_image.checksum, assessment_item__isnull=True + ).count(), + 1, + ) + # Total rows for this checksum: the untouched fixture File (unattached) + # plus the one properly-attached row from map_files_to_assessment_item - + # not a stray duplicate left behind by set_files(). + self.assertEqual( + File.objects.filter(checksum=qti_exercise_image.checksum).count(), 2 + ) + + def test_legacy_multiple_selection_question_converted_to_qti_on_ingest(self): + """ + A legacy multiple_selection question is converted to schema-valid QTI + on ingest, and its media stays attached through the conversion. + """ + response = self._make_request() + assert response.status_code == 200, response.content + + c = ContentNode.objects.get(title=self.title) + item = c.assessment_items.get(assessment_id="abf45e8fd7f151adb1b3df2d751e945e") + self.assertEqual(item.type, exercises.QTI) + result = validate_qti_item(item.raw_data) + self.assertTrue(result.is_valid, result.errors) + self.assertIn("Which numbers are even?", item.raw_data) + self.assertEqual(item.files.count(), 1) + self.assertEqual(item.files.first().filename(), self.exercise_image.filename()) + # same set_files stray-row risk as the native-QTI test above: exactly 2 + # rows for this checksum (the untouched setUp fixture, unattached, plus + # the one properly-attached row) - not a 3rd stray-attached row. + self.assertEqual( + File.objects.filter(checksum=self.exercise_image.checksum).count(), 2 + ) + + def test_invalid_qti_raw_data_returns_400_and_creates_nothing(self): + """ + A schema-invalid QTI upload is rejected with a 400 and rolls back the + whole request - it must not partially create the node/other items. + """ + self.sample_data["content_data"][0]["questions"].append( + { + "assessment_id": "2" * 32, + "type": "QTI", + "files": [], + "question": "", + "hints": "[]", + "answers": "[]", + "raw_data": "not schema compliant", + "source_url": None, + "randomize": False, + } + ) + response = self._make_request() + assert response.status_code == 400, response.content + # whole-request creation is atomic: the otherwise-valid node/questions + # from setUp must not have been persisted alongside the invalid one + assert not ContentNode.objects.filter(title=self.title).exists() + + def test_perseus_custom_interaction_unwrapped_to_raw_perseus_question(self): + """ + A QTI item wrapping a Perseus custom interaction is unwrapped back to + a raw perseus_question, reading its payload from the declared file. + """ + assessment_id = "3" * 32 + wrapped_perseus_json = self.sample_data["content_data"][0]["questions"][1][ + "raw_data" + ] + perseus_payload_file = create_studio_file(wrapped_perseus_json, ext="json") + perseus_payload_filename = perseus_payload_file["name"] + raw_data = _item_xml( + "item_2", + "Wrapped Perseus Item", + "", + '' % perseus_payload_filename, + ) + self.sample_data["content_data"][0]["questions"].append( + { + "assessment_id": assessment_id, + "type": "QTI", + "files": [ + { + "size": len(wrapped_perseus_json.encode("utf-8")), + "preset": "document", + "filename": perseus_payload_filename, + "original_filename": None, + "language": None, + "source_url": None, + } + ], + "question": "", + "hints": "[]", + "answers": "[]", + "raw_data": raw_data, + "source_url": None, + "randomize": False, + } + ) + + response = self._make_request() + assert response.status_code == 200, response.content + + c = ContentNode.objects.get(title=self.title) + item = c.assessment_items.get(assessment_id=assessment_id) + self.assertEqual(item.type, exercises.PERSEUS_QUESTION) + self.assertEqual(item.raw_data, wrapped_perseus_json) + # the perseus-path payload file was consumed to produce raw_data, not + # persisted as a real file reference + self.assertEqual(item.files.count(), 0) + + def test_raw_perseus_question_passthrough_unchanged(self): + """ + Regression guard: a raw (non-wrapped) perseus_question is completely + unaffected by the QTI ingest refactor. + """ + response = self._make_request() + assert response.status_code == 200, response.content + + c = ContentNode.objects.get(title=self.title) + item = c.assessment_items.get(assessment_id="98856e24d53b57ea9023782ab6018767") + self.assertEqual(item.type, exercises.PERSEUS_QUESTION) + self.assertIn("radio 1", item.raw_data) + self.assertEqual(item.files.count(), 1) + self.assertEqual( + item.files.first().filename(), self.exercise_graphie.filename() + ) + class ApiAddExerciseExtraFieldsMigrationTestCase(StudioTestCase): """ diff --git a/contentcuration/contentcuration/tests/viewsets/test_assessmentitem.py b/contentcuration/contentcuration/tests/viewsets/test_assessmentitem.py index 891d66c2f8..1f3d1330f8 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_assessmentitem.py +++ b/contentcuration/contentcuration/tests/viewsets/test_assessmentitem.py @@ -84,6 +84,28 @@ def test_create_assessmentitem(self): except models.AssessmentItem.DoesNotExist: self.fail("AssessmentItem was not created") + def test_create_perseus_question_with_raw_data(self): + self.client.force_authenticate(user=self.user) + assessmentitem = self.assessmentitem_metadata + assessmentitem["type"] = exercises.PERSEUS_QUESTION + assessmentitem["raw_data"] = json.dumps({"question": {"content": "hi"}}) + response = self.sync_changes( + [ + generate_create_event( + [assessmentitem["contentnode"], assessmentitem["assessment_id"]], + ASSESSMENTITEM, + assessmentitem, + channel_id=self.channel.id, + ) + ], + ) + self.assertEqual(response.status_code, 200, response.content) + ai = models.AssessmentItem.objects.get( + assessment_id=assessmentitem["assessment_id"] + ) + self.assertEqual(ai.type, exercises.PERSEUS_QUESTION) + self.assertEqual(ai.raw_data, assessmentitem["raw_data"]) + def test_create_assessmentitem_no_node_permission(self): self.client.force_authenticate(user=self.user) new_channel = testdata.channel() diff --git a/contentcuration/contentcuration/utils/assessment/qti/ingest.py b/contentcuration/contentcuration/utils/assessment/qti/ingest.py new file mode 100644 index 0000000000..38dc411e4a --- /dev/null +++ b/contentcuration/contentcuration/utils/assessment/qti/ingest.py @@ -0,0 +1,65 @@ +import json +from typing import Optional + +from django.core.files.storage import default_storage +from le_utils.constants import exercises +from lxml import etree + +from contentcuration.models import generate_object_storage_name +from contentcuration.utils.assessment.qti.convert import ( + convert_legacy_assessment_item_to_qti, +) +from contentcuration.utils.assessment.qti.convert import LegacyAssessmentItem +from contentcuration.utils.assessment.qti.convert import QTIConversionResult +from contentcuration.utils.assessment.qti.validation import parse_qti_xml + + +CONTENT_STORAGE_PREFIX = exercises.CONTENT_STORAGE_FORMAT.format("") + +_PERSEUS_CUSTOM_INTERACTION_XPATH = etree.XPath( + "//*[local-name()='qti-custom-interaction' and @data-type='perseus']" +) + + +def strip_content_storage_placeholder(text): + return text.replace(CONTENT_STORAGE_PREFIX, "") + + +def convert_legacy_question_to_qti(question_data: dict) -> QTIConversionResult: + answers = json.loads(question_data.get("answers") or "[]") + for answer in answers: + if isinstance(answer.get("answer"), str): + answer["answer"] = strip_content_storage_placeholder(answer["answer"]) + + item = LegacyAssessmentItem( + type=question_data["type"], + question=strip_content_storage_placeholder(question_data.get("question") or ""), + answers=answers, + randomize=question_data.get("randomize") or False, + assessment_id=question_data["assessment_id"], + title=question_data.get("assessment_id"), + language=question_data.get("language") or "en", + ) + return convert_legacy_assessment_item_to_qti(item) + + +def find_perseus_custom_interaction_path(raw_data) -> Optional[str]: + if isinstance(raw_data, str): + raw_data = raw_data.encode("utf-8") + try: + doc = parse_qti_xml(raw_data) + except etree.XMLSyntaxError: + return None + matches = _PERSEUS_CUSTOM_INTERACTION_XPATH(doc) + if not matches: + return None + return matches[0].get("data-perseus-path") + + +def read_uploaded_file_content(filename: str) -> str: + checksum = filename.split(".")[0] + file_path = generate_object_storage_name(checksum, filename) + if not default_storage.exists(file_path): + raise ValueError("{} not found".format(file_path)) + with default_storage.open(file_path, "rb") as f: + return f.read().decode("utf-8") diff --git a/contentcuration/contentcuration/views/internal.py b/contentcuration/contentcuration/views/internal.py index 09893e9e0c..ee7e3492fb 100644 --- a/contentcuration/contentcuration/views/internal.py +++ b/contentcuration/contentcuration/views/internal.py @@ -1,6 +1,7 @@ import json import logging from collections import namedtuple +from types import SimpleNamespace from distutils.version import LooseVersion from django.core.exceptions import ObjectDoesNotExist @@ -8,12 +9,15 @@ from django.core.exceptions import SuspiciousOperation from django.core.exceptions import ValidationError from django.db import transaction +from django.db.models import Q from django.http import HttpResponseBadRequest from django.http import HttpResponseForbidden from django.http import HttpResponseNotFound from django.http import HttpResponseServerError from django.http import JsonResponse +from django.utils import timezone from le_utils.constants import content_kinds +from le_utils.constants import exercises from le_utils.constants import roles from le_utils.constants.labels.accessibility_categories import ( ACCESSIBILITYCATEGORIESLIST, @@ -29,6 +33,7 @@ from rest_framework.decorators import api_view from rest_framework.decorators import authentication_classes from rest_framework.decorators import permission_classes +from rest_framework.exceptions import ValidationError as DRFValidationError from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response from sentry_sdk import capture_exception @@ -37,17 +42,22 @@ from contentcuration.api import write_file_to_storage from contentcuration.constants import completion_criteria from contentcuration.decorators import delay_user_storage_calculation -from contentcuration.models import AssessmentItem from contentcuration.models import Change from contentcuration.models import Channel from contentcuration.models import ContentNode from contentcuration.models import ContentTag +from contentcuration.models import File from contentcuration.models import License from contentcuration.models import SlideshowSlide from contentcuration.models import StagedFile from contentcuration.serializers import GetTreeDataSerializer from contentcuration.tasks import apply_channel_changes_task from contentcuration.tasks import generatenodediff_task +from contentcuration.utils.assessment.qti.ingest import convert_legacy_question_to_qti +from contentcuration.utils.assessment.qti.ingest import ( + find_perseus_custom_interaction_path, +) +from contentcuration.utils.assessment.qti.ingest import read_uploaded_file_content from contentcuration.utils.files import get_file_diff from contentcuration.utils.garbage_collect import get_deleted_chefs_root from contentcuration.utils.nodes import map_files_to_assessment_item @@ -55,6 +65,7 @@ from contentcuration.utils.nodes import map_files_to_slideshow_slide_item from contentcuration.utils.nodes import migrate_extra_fields from contentcuration.utils.sentry import report_exception +from contentcuration.viewsets.assessmentitem import AssessmentItemSerializer from contentcuration.viewsets.sync.constants import CHANNEL from contentcuration.viewsets.sync.utils import generate_publish_event from contentcuration.viewsets.sync.utils import generate_update_event @@ -929,7 +940,8 @@ def create_node(node_data, parent_node, sort_order): # noqa: C901 def create_exercises(user, node, data): - """ Generate exercise from data """ + """Generate exercise from data, delegating item construction and QTI schema + validation to AssessmentItemSerializer.""" # First check that all assessment_ids are unique within the node assessment_ids = [question.get("assessment_id") for question in data] if len(assessment_ids) != len(set(assessment_ids)): @@ -937,25 +949,89 @@ def create_exercises(user, node, data): "Duplicate assessment_ids found in node {}".format(node.node_id) ) + item_dicts = [] + perseus_paths = [] + for order, question in enumerate(data): + try: + item_dict, perseus_path = _build_assessment_item_dict(node, question, order) + except ValueError as e: + raise NodeValidationError( + "Invalid question data for node {}: {}".format(node.node_id, e) + ) + item_dicts.append(item_dict) + perseus_paths.append(perseus_path) + with transaction.atomic(): - order = 0 - - for question in data: - question_obj = AssessmentItem( - type=question.get("type"), - question=question.get("question"), - hints=question.get("hints"), - answers=question.get("answers"), - order=order, - contentnode=node, - assessment_id=question.get("assessment_id"), - raw_data=question.get("raw_data"), - source_url=question.get("source_url"), - randomize=question.get("randomize") or False, + serializer = AssessmentItemSerializer( + data=item_dicts, + many=True, + context={"request": SimpleNamespace(user=user)}, + ) + try: + serializer.is_valid(raise_exception=True) + except DRFValidationError as e: + raise NodeValidationError( + "Invalid assessment item data for node {}: {}".format( + node.node_id, e.detail + ) + ) + + # AssessmentItemSerializer.set_files() auto-creates a File row per checksum + # referenced in QTI raw_data (generic preset, no real metadata) since + # ricecooker never pre-stages one. map_files_to_assessment_item below + # creates the authoritative row from question["files"], so the + # auto-created one must be deleted - whether attached to `instances` or + # left orphaned (e.g. same checksum referenced by two items in this + # batch). File.id is a random UUID, so we can't watermark by id; use a + # timestamp on File.modified instead. + save_watermark = timezone.now() + instances = serializer.save() + File.objects.filter( + Q(assessment_item__in=instances) + | Q( + assessment_item__isnull=True, + contentnode__isnull=True, + uploaded_by=user, + modified__gte=save_watermark, ) - order += 1 - question_obj.save() - map_files_to_assessment_item(user, question_obj, question["files"]) + ).delete() + + for question, instance, perseus_path in zip(data, instances, perseus_paths): + files = question["files"] + if perseus_path: + files = [f for f in files if f and f.get("filename") != perseus_path] + map_files_to_assessment_item(user, instance, files) + + +def _build_assessment_item_dict(node, question, order): + common = { + "contentnode": node.pk, + "assessment_id": question.get("assessment_id"), + "order": order, + "source_url": question.get("source_url"), + "randomize": question.get("randomize") or False, + } + + if question.get("type") == exercises.QTI: + raw_data = question.get("raw_data") + perseus_path = find_perseus_custom_interaction_path(raw_data) + if perseus_path: + item_dict = dict( + common, + type=exercises.PERSEUS_QUESTION, + raw_data=read_uploaded_file_content(perseus_path), + ) + return item_dict, perseus_path + return dict(common, type=exercises.QTI, raw_data=raw_data), None + + if question.get("type") == exercises.PERSEUS_QUESTION: + item_dict = dict( + common, type=exercises.PERSEUS_QUESTION, raw_data=question.get("raw_data") + ) + return item_dict, None + + result = convert_legacy_question_to_qti(question) + return dict(common, type=exercises.QTI, raw_data=result.xml), None def create_slides(user, node, slideshow_data): diff --git a/contentcuration/contentcuration/viewsets/assessmentitem.py b/contentcuration/contentcuration/viewsets/assessmentitem.py index 30678b9570..e000c67371 100644 --- a/contentcuration/contentcuration/viewsets/assessmentitem.py +++ b/contentcuration/contentcuration/viewsets/assessmentitem.py @@ -144,11 +144,11 @@ def validate(self, data): raise ValidationError( {"raw_data": [error.message for error in result.errors]} ) - elif "raw_data" in data: + elif self._item_type != exercises.PERSEUS_QUESTION and "raw_data" in data: raise ValidationError( { "raw_data": [ - "This field can only be edited on a QTI assessment item." + "This field can only be edited on a QTI or Perseus assessment item." ] } ) From 57f555e1aa68dcef4e4bc74a1fbf0a8d69dd8640 Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Wed, 8 Jul 2026 11:41:12 -0700 Subject: [PATCH 2/3] fix: reject Perseus-wrapped QTI outright, reuse ricecooker's pre-staged File Per review: uploading a QTI item that wraps a Perseus custom interaction is a categorical mistake ricecooker should prevent on its side, so reject it instead of unwrapping it. Also drop the create-three-files-then-clean- up-two watermarking dance for QTI media - AssessmentItemSerializer.set_files() already reuses the File row ricecooker's file_upload_url request created, so just let it, and surface any storage-layer lookup failure as a NodeValidationError. Co-Authored-By: Claude Sonnet 5 --- .../contentcuration/tests/testdata.py | 16 +++-- .../tests/utils/test_create_exercises.py | 51 +++++++++++++ .../tests/views/test_views_internal.py | 71 +++++++++---------- .../utils/assessment/qti/ingest.py | 11 --- .../contentcuration/views/internal.py | 70 +++++++----------- 5 files changed, 123 insertions(+), 96 deletions(-) diff --git a/contentcuration/contentcuration/tests/testdata.py b/contentcuration/contentcuration/tests/testdata.py index 9f93c5ab79..962d2e0a5b 100644 --- a/contentcuration/contentcuration/tests/testdata.py +++ b/contentcuration/contentcuration/tests/testdata.py @@ -288,13 +288,16 @@ def create_temp_file(filebytes, preset="document", ext="pdf", original_filename= ) -def create_studio_file(filebytes, preset="document", ext="pdf", original_filename=None): +def create_studio_file( + filebytes, preset="document", ext="pdf", original_filename=None, uploaded_by=None +): """ Create a file with contents of `filebytes` and the associated cc.File object for it. :param filebytes: The data to be stored in the file (as bytes) :param preset: String identifying the format preset (defaults to ``document``) :param ext: File extension, omitting the initial period :param original_filename: Original filename (needed for exercise_images) + :param uploaded_by: User the file should appear to have been uploaded by Returns a dict containing the following: - name (str): the filename within the content storage system (= md5 hash of the contents + .ext ) - data (bytes): file content (echo of `filebytes`) @@ -335,6 +338,7 @@ def create_studio_file(filebytes, preset="document", ext="pdf", original_filenam preset=preset, original_filename=original_filename, file_on_disk=storage_file_path, + uploaded_by=uploaded_by, ) return { @@ -414,7 +418,7 @@ def create_test_file(filebytes, ext="pdf"): ] -def fileobj_exercise_image(size=(100, 100), color="red"): +def fileobj_exercise_image(size=(100, 100), color="red", uploaded_by=None): """ Create a generic exercise image file in storage and return a File model pointing to it. """ @@ -422,12 +426,15 @@ def fileobj_exercise_image(size=(100, 100), color="red"): buffer = BytesIO() image.save(buffer, "JPEG") temp_file_dict = create_studio_file( - buffer.getvalue(), preset=format_presets.EXERCISE_IMAGE, ext="jpg" + buffer.getvalue(), + preset=format_presets.EXERCISE_IMAGE, + ext="jpg", + uploaded_by=uploaded_by, ) return temp_file_dict["db_file"] -def fileobj_exercise_graphie(original_filename=None): +def fileobj_exercise_graphie(original_filename=None, uploaded_by=None): """ Create an graphi exercise image file in storage and return a File model pointing to it. """ @@ -439,6 +446,7 @@ def fileobj_exercise_graphie(original_filename=None): preset=format_presets.EXERCISE_GRAPHIE, ext="graphie", original_filename=original_filename or "theoriginalfilename", + uploaded_by=uploaded_by, ) return temp_file_dict["db_file"] diff --git a/contentcuration/contentcuration/tests/utils/test_create_exercises.py b/contentcuration/contentcuration/tests/utils/test_create_exercises.py index 0373921bba..5d081d3517 100644 --- a/contentcuration/contentcuration/tests/utils/test_create_exercises.py +++ b/contentcuration/contentcuration/tests/utils/test_create_exercises.py @@ -3,6 +3,7 @@ from contentcuration.tests.base import StudioTestCase from contentcuration.tests.testdata import channel +from contentcuration.tests.utils.qti.test_validation import _item_xml from contentcuration.views.internal import create_exercises @@ -37,3 +38,53 @@ def test_unsupported_legacy_question_type_raises_node_validation_error(self): ] with self.assertRaises(DjangoValidationError): create_exercises(self.admin_user, node, questions) + + def test_qti_raw_data_referencing_missing_file_raises_node_validation_error(self): + node = channel().main_tree + raw_data = _item_xml( + "item_1", + "Sample Item", + '' + "choice_0" + "", + 'Pick. ' + 'diagram' + 'A' + 'B' + "", + ) + questions = [ + { + "assessment_id": "ffffffffffffffffffffffffffffffff", + "type": exercises.QTI, + "raw_data": raw_data, + "files": [], + "source_url": None, + "randomize": False, + } + ] + with self.assertRaises(DjangoValidationError): + create_exercises(self.admin_user, node, questions) + + def test_qti_wrapping_perseus_custom_interaction_rejected(self): + node = channel().main_tree + raw_data = _item_xml( + "item_2", + "Sample Item", + "", + '', + ) + questions = [ + { + "assessment_id": "cccccccccccccccccccccccccccccccc", + "type": exercises.QTI, + "raw_data": raw_data, + "files": [], + "source_url": None, + "randomize": False, + } + ] + with self.assertRaises(DjangoValidationError): + create_exercises(self.admin_user, node, questions) diff --git a/contentcuration/contentcuration/tests/views/test_views_internal.py b/contentcuration/contentcuration/tests/views/test_views_internal.py index 98d45d0036..5d791f703e 100644 --- a/contentcuration/contentcuration/tests/views/test_views_internal.py +++ b/contentcuration/contentcuration/tests/views/test_views_internal.py @@ -296,10 +296,13 @@ def setUp(self): # get our random data from mixer random_data = mixer.blend(SampleContentNodeDataSchema) - # a vanilla image file associated with question - self.exercise_image = fileobj_exercise_image() + # a vanilla image file associated with question - uploaded_by matches + # self.admin_client() to mirror ricecooker's real upload_url flow, + # where the same authenticated user stages the file before referencing + # it in create_exercises + self.exercise_image = fileobj_exercise_image(uploaded_by=self.admin_user) # a perseus image file associated with question - self.exercise_graphie = fileobj_exercise_graphie() + self.exercise_graphie = fileobj_exercise_graphie(uploaded_by=self.admin_user) self.title = random_data.title self.sample_data = { "root_id": self.root_node.id, @@ -480,10 +483,13 @@ def test_native_qti_item_persisted_and_media_mapped(self): """ A native QTI question (raw_data XML + declared media) is persisted verbatim as an exercises.QTI assessment item, and its media is mapped - without leaving stray/duplicate File rows behind. + by reusing the File row ricecooker's file_upload_url request already + created for it, without leaving stray/duplicate File rows behind. """ assessment_id = "1" * 32 - qti_exercise_image = fileobj_exercise_image(size=(50, 50), color="blue") + qti_exercise_image = fileobj_exercise_image( + size=(50, 50), color="blue", uploaded_by=self.admin_user + ) qti_raw_data = _item_xml( "item_1", "Sample Item", @@ -530,24 +536,16 @@ def test_native_qti_item_persisted_and_media_mapped(self): self.assertEqual(qti_item.files.count(), 1) attached_file = qti_item.files.first() self.assertEqual(attached_file.filename(), qti_exercise_image.filename()) - self.assertEqual(attached_file.original_filename, "file") - # AssessmentItemSerializer.set_files() auto-creates its own File row - # for this checksum while saving (there's no pre-staged File to reuse, - # as ricecooker never stages one the way the browser editor does); that - # stray row must be cleaned up before map_files_to_assessment_item - # creates the real, correctly-attributed one. Only the untouched setUp - # fixture should remain unattached for this checksum. + # The File row is the exact one created when the image was staged + # (i.e. by ricecooker's file_upload_url request), not a new row - so + # its id and original_filename are preserved rather than overwritten. + self.assertEqual(attached_file.id, qti_exercise_image.id) self.assertEqual( - File.objects.filter( - checksum=qti_exercise_image.checksum, assessment_item__isnull=True - ).count(), - 1, - ) - # Total rows for this checksum: the untouched fixture File (unattached) - # plus the one properly-attached row from map_files_to_assessment_item - - # not a stray duplicate left behind by set_files(). + attached_file.original_filename, qti_exercise_image.original_filename + ) + # No stray/duplicate File row is left behind for this checksum. self.assertEqual( - File.objects.filter(checksum=qti_exercise_image.checksum).count(), 2 + File.objects.filter(checksum=qti_exercise_image.checksum).count(), 1 ) def test_legacy_multiple_selection_question_converted_to_qti_on_ingest(self): @@ -565,12 +563,12 @@ def test_legacy_multiple_selection_question_converted_to_qti_on_ingest(self): self.assertTrue(result.is_valid, result.errors) self.assertIn("Which numbers are even?", item.raw_data) self.assertEqual(item.files.count(), 1) - self.assertEqual(item.files.first().filename(), self.exercise_image.filename()) - # same set_files stray-row risk as the native-QTI test above: exactly 2 - # rows for this checksum (the untouched setUp fixture, unattached, plus - # the one properly-attached row) - not a 3rd stray-attached row. + attached_file = item.files.first() + self.assertEqual(attached_file.filename(), self.exercise_image.filename()) + # The pre-staged setUp fixture File is reused directly, not duplicated. + self.assertEqual(attached_file.id, self.exercise_image.id) self.assertEqual( - File.objects.filter(checksum=self.exercise_image.checksum).count(), 2 + File.objects.filter(checksum=self.exercise_image.checksum).count(), 1 ) def test_invalid_qti_raw_data_returns_400_and_creates_nothing(self): @@ -597,10 +595,12 @@ def test_invalid_qti_raw_data_returns_400_and_creates_nothing(self): # from setUp must not have been persisted alongside the invalid one assert not ContentNode.objects.filter(title=self.title).exists() - def test_perseus_custom_interaction_unwrapped_to_raw_perseus_question(self): + def test_perseus_custom_interaction_in_qti_rejected(self): """ - A QTI item wrapping a Perseus custom interaction is unwrapped back to - a raw perseus_question, reading its payload from the declared file. + A QTI item wrapping a Perseus custom interaction is a categorical + mistake on ricecooker's part (it should upload raw Perseus content as + a perseus_question instead) - reject it outright rather than trying + to unwrap it. """ assessment_id = "3" * 32 wrapped_perseus_json = self.sample_data["content_data"][0]["questions"][1][ @@ -639,15 +639,10 @@ def test_perseus_custom_interaction_unwrapped_to_raw_perseus_question(self): ) response = self._make_request() - assert response.status_code == 200, response.content - - c = ContentNode.objects.get(title=self.title) - item = c.assessment_items.get(assessment_id=assessment_id) - self.assertEqual(item.type, exercises.PERSEUS_QUESTION) - self.assertEqual(item.raw_data, wrapped_perseus_json) - # the perseus-path payload file was consumed to produce raw_data, not - # persisted as a real file reference - self.assertEqual(item.files.count(), 0) + assert response.status_code == 400, response.content + # whole-request creation is atomic: the otherwise-valid node/questions + # from setUp must not have been persisted alongside the rejected one + assert not ContentNode.objects.filter(title=self.title).exists() def test_raw_perseus_question_passthrough_unchanged(self): """ diff --git a/contentcuration/contentcuration/utils/assessment/qti/ingest.py b/contentcuration/contentcuration/utils/assessment/qti/ingest.py index 38dc411e4a..1a03e641fb 100644 --- a/contentcuration/contentcuration/utils/assessment/qti/ingest.py +++ b/contentcuration/contentcuration/utils/assessment/qti/ingest.py @@ -1,11 +1,9 @@ import json from typing import Optional -from django.core.files.storage import default_storage from le_utils.constants import exercises from lxml import etree -from contentcuration.models import generate_object_storage_name from contentcuration.utils.assessment.qti.convert import ( convert_legacy_assessment_item_to_qti, ) @@ -54,12 +52,3 @@ def find_perseus_custom_interaction_path(raw_data) -> Optional[str]: if not matches: return None return matches[0].get("data-perseus-path") - - -def read_uploaded_file_content(filename: str) -> str: - checksum = filename.split(".")[0] - file_path = generate_object_storage_name(checksum, filename) - if not default_storage.exists(file_path): - raise ValueError("{} not found".format(file_path)) - with default_storage.open(file_path, "rb") as f: - return f.read().decode("utf-8") diff --git a/contentcuration/contentcuration/views/internal.py b/contentcuration/contentcuration/views/internal.py index ee7e3492fb..ffb5d84b19 100644 --- a/contentcuration/contentcuration/views/internal.py +++ b/contentcuration/contentcuration/views/internal.py @@ -9,13 +9,11 @@ from django.core.exceptions import SuspiciousOperation from django.core.exceptions import ValidationError from django.db import transaction -from django.db.models import Q from django.http import HttpResponseBadRequest from django.http import HttpResponseForbidden from django.http import HttpResponseNotFound from django.http import HttpResponseServerError from django.http import JsonResponse -from django.utils import timezone from le_utils.constants import content_kinds from le_utils.constants import exercises from le_utils.constants import roles @@ -46,7 +44,6 @@ from contentcuration.models import Channel from contentcuration.models import ContentNode from contentcuration.models import ContentTag -from contentcuration.models import File from contentcuration.models import License from contentcuration.models import SlideshowSlide from contentcuration.models import StagedFile @@ -57,7 +54,6 @@ from contentcuration.utils.assessment.qti.ingest import ( find_perseus_custom_interaction_path, ) -from contentcuration.utils.assessment.qti.ingest import read_uploaded_file_content from contentcuration.utils.files import get_file_diff from contentcuration.utils.garbage_collect import get_deleted_chefs_root from contentcuration.utils.nodes import map_files_to_assessment_item @@ -939,7 +935,7 @@ def create_node(node_data, parent_node, sort_order): # noqa: C901 return node -def create_exercises(user, node, data): +def create_exercises(user, node, data): # noqa: C901 """Generate exercise from data, delegating item construction and QTI schema validation to AssessmentItemSerializer.""" # First check that all assessment_ids are unique within the node @@ -950,16 +946,13 @@ def create_exercises(user, node, data): ) item_dicts = [] - perseus_paths = [] for order, question in enumerate(data): try: - item_dict, perseus_path = _build_assessment_item_dict(node, question, order) + item_dicts.append(_build_assessment_item_dict(node, question, order)) except ValueError as e: raise NodeValidationError( "Invalid question data for node {}: {}".format(node.node_id, e) ) - item_dicts.append(item_dict) - perseus_paths.append(perseus_path) with transaction.atomic(): serializer = AssessmentItemSerializer( @@ -976,31 +969,25 @@ def create_exercises(user, node, data): ) ) - # AssessmentItemSerializer.set_files() auto-creates a File row per checksum - # referenced in QTI raw_data (generic preset, no real metadata) since - # ricecooker never pre-stages one. map_files_to_assessment_item below - # creates the authoritative row from question["files"], so the - # auto-created one must be deleted - whether attached to `instances` or - # left orphaned (e.g. same checksum referenced by two items in this - # batch). File.id is a random UUID, so we can't watermark by id; use a - # timestamp on File.modified instead. - save_watermark = timezone.now() - instances = serializer.save() - File.objects.filter( - Q(assessment_item__in=instances) - | Q( - assessment_item__isnull=True, - contentnode__isnull=True, - uploaded_by=user, - modified__gte=save_watermark, + try: + # AssessmentItemSerializer.set_files() attaches QTI media by + # reusing the File row ricecooker's file_upload_url request already + # created for each referenced checksum. A QTI item referencing a + # checksum nothing was ever uploaded for surfaces as a storage-layer + # error (FileNotFoundError locally, OSError on the S3 backend) here + # rather than as a DRF ValidationError. + instances = serializer.save() + except (DRFValidationError, OSError) as e: + raise NodeValidationError( + "Invalid assessment item data for node {}: {}".format(node.node_id, e) ) - ).delete() - for question, instance, perseus_path in zip(data, instances, perseus_paths): - files = question["files"] - if perseus_path: - files = [f for f in files if f and f.get("filename") != perseus_path] - map_files_to_assessment_item(user, instance, files) + # set_files() only scans raw_data for QTI items; perseus_question raw_data + # is opaque JSON, so its file (declared explicitly in question["files"]) + # is still attached here. + for question, instance in zip(data, instances): + if instance.type == exercises.PERSEUS_QUESTION: + map_files_to_assessment_item(user, instance, question["files"]) def _build_assessment_item_dict(node, question, order): @@ -1014,24 +1001,21 @@ def _build_assessment_item_dict(node, question, order): if question.get("type") == exercises.QTI: raw_data = question.get("raw_data") - perseus_path = find_perseus_custom_interaction_path(raw_data) - if perseus_path: - item_dict = dict( - common, - type=exercises.PERSEUS_QUESTION, - raw_data=read_uploaded_file_content(perseus_path), + if find_perseus_custom_interaction_path(raw_data): + raise ValueError( + "QTI item wraps a Perseus custom interaction; ricecooker must " + "upload raw Perseus content as a perseus_question instead of " + "wrapping it in QTI" ) - return item_dict, perseus_path - return dict(common, type=exercises.QTI, raw_data=raw_data), None + return dict(common, type=exercises.QTI, raw_data=raw_data) if question.get("type") == exercises.PERSEUS_QUESTION: - item_dict = dict( + return dict( common, type=exercises.PERSEUS_QUESTION, raw_data=question.get("raw_data") ) - return item_dict, None result = convert_legacy_question_to_qti(question) - return dict(common, type=exercises.QTI, raw_data=result.xml), None + return dict(common, type=exercises.QTI, raw_data=result.xml) def create_slides(user, node, slideshow_data): From 86188a00669edbcce611df1e1d18bfba28cccb5a Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Wed, 8 Jul 2026 11:52:30 -0700 Subject: [PATCH 3/3] refactor: simplify create_exercises error handling and dedupe test QTI XML builder Merge the two try/except blocks around serializer validation and save into one, and factor the custom-interaction test fixture to reuse _item_xml instead of duplicating the QTI item XML template. Co-Authored-By: Claude Sonnet 5 --- .../tests/utils/qti/test_ingest.py | 18 +++++++----------- .../contentcuration/views/internal.py | 15 +++++---------- 2 files changed, 12 insertions(+), 21 deletions(-) diff --git a/contentcuration/contentcuration/tests/utils/qti/test_ingest.py b/contentcuration/contentcuration/tests/utils/qti/test_ingest.py index f210a6e2eb..43eef6ffe9 100644 --- a/contentcuration/contentcuration/tests/utils/qti/test_ingest.py +++ b/contentcuration/contentcuration/tests/utils/qti/test_ingest.py @@ -1,5 +1,6 @@ import unittest +from contentcuration.tests.utils.qti.test_validation import _item_xml from contentcuration.tests.utils.qti.test_validation import VALID_CHOICE_ITEM from contentcuration.utils.assessment.qti.ingest import convert_legacy_question_to_qti from contentcuration.utils.assessment.qti.ingest import ( @@ -44,17 +45,12 @@ def test_convert_legacy_question_to_qti_strips_placeholder_from_question_and_ans def _custom_interaction_item_xml(data_type, path_attr, path_value): - return """ - - - - -""" % ( - data_type, - path_attr, - path_value, + return _item_xml( + "item1", + "t", + "", + '' + % (data_type, path_attr, path_value), ) diff --git a/contentcuration/contentcuration/views/internal.py b/contentcuration/contentcuration/views/internal.py index ffb5d84b19..952032ed63 100644 --- a/contentcuration/contentcuration/views/internal.py +++ b/contentcuration/contentcuration/views/internal.py @@ -935,7 +935,7 @@ def create_node(node_data, parent_node, sort_order): # noqa: C901 return node -def create_exercises(user, node, data): # noqa: C901 +def create_exercises(user, node, data): """Generate exercise from data, delegating item construction and QTI schema validation to AssessmentItemSerializer.""" # First check that all assessment_ids are unique within the node @@ -962,14 +962,6 @@ def create_exercises(user, node, data): # noqa: C901 ) try: serializer.is_valid(raise_exception=True) - except DRFValidationError as e: - raise NodeValidationError( - "Invalid assessment item data for node {}: {}".format( - node.node_id, e.detail - ) - ) - - try: # AssessmentItemSerializer.set_files() attaches QTI media by # reusing the File row ricecooker's file_upload_url request already # created for each referenced checksum. A QTI item referencing a @@ -978,8 +970,11 @@ def create_exercises(user, node, data): # noqa: C901 # rather than as a DRF ValidationError. instances = serializer.save() except (DRFValidationError, OSError) as e: + detail = e.detail if isinstance(e, DRFValidationError) else e raise NodeValidationError( - "Invalid assessment item data for node {}: {}".format(node.node_id, e) + "Invalid assessment item data for node {}: {}".format( + node.node_id, detail + ) ) # set_files() only scans raw_data for QTI items; perseus_question raw_data