From 519f29522be27fe9cb416515d079c7edc7590b86 Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Wed, 30 Sep 2026 07:03:05 -0700 Subject: [PATCH 1/2] fix: keep animation, orientation and colour when resizing publish images - Animated GIFs resize every frame, keeping timing and loop; at natural size the original ships. - Past MAX_RESIZED_IMAGE_PIXELS the original ships. - EXIF-rotated photos come out upright; JPEGs keep their ICC profile. - Same-size re-encodes ship only when smaller than the original. - Sized SVGs get root width/height (and a viewBox when missing). - A failed resize ships the original instead of a dangling reference. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../tests/utils/test_exercise_creation.py | 349 ++++++++++++++++++ .../contentcuration/utils/assessment/base.py | 155 +++++++- 2 files changed, 488 insertions(+), 16 deletions(-) diff --git a/contentcuration/contentcuration/tests/utils/test_exercise_creation.py b/contentcuration/contentcuration/tests/utils/test_exercise_creation.py index 8c3680f335..0d8944b6c4 100644 --- a/contentcuration/contentcuration/tests/utils/test_exercise_creation.py +++ b/contentcuration/contentcuration/tests/utils/test_exercise_creation.py @@ -5,9 +5,11 @@ import json import os import re +import struct import zipfile from io import BytesIO from tempfile import TemporaryDirectory +from unittest import mock from uuid import uuid4 from django.core.files.storage import default_storage as storage @@ -15,6 +17,10 @@ from le_utils.constants import exercises from le_utils.constants import file_formats from le_utils.constants import format_presets +from lxml import etree +from PIL import Image +from PIL import ImageCms +from PIL import PngImagePlugin from contentcuration.models import AssessmentItem from contentcuration.models import ContentNode @@ -42,6 +48,122 @@ def _create_unresizable_image(): )["db_file"] +def _animated_gif_bytes(size=(120, 90)): + frames = [Image.new("RGB", size, color=c) for c in ("red", "blue", "green")] + buffer = BytesIO() + frames[0].save(buffer, "GIF", save_all=True, append_images=frames[1:]) + return buffer.getvalue() + + +def _moving_block_gif_bytes(): + # A red block crosses a transparent background, one third per frame + frames = [] + for left in (0, 40, 80): + frame = Image.new("RGBA", (120, 90), (0, 0, 0, 0)) + frame.paste((255, 0, 0, 255), (left, 0, left + 40, 90)) + frames.append(frame) + buffer = BytesIO() + frames[0].save(buffer, "GIF", save_all=True, append_images=frames[1:], disposal=2) + return buffer.getvalue() + + +def _dotted_gif_bytes(): + # 1px dots stay put while a block moves along the bottom edge + frames = [] + for left in (0, 100, 200): + frame = Image.new("RGB", (400, 300), "white") + for x in range(0, 400, 8): + for y in range(0, 240, 8): + frame.putpixel((x, y), (0, 0, 0)) + frame.paste((255, 0, 0), (left, 260, left + 40, 300)) + frames.append(frame) + buffer = BytesIO() + frames[0].save(buffer, "GIF", save_all=True, append_images=frames[1:]) + return buffer.getvalue() + + +def _red_thirds(content): + thirds = [] + with Image.open(BytesIO(content)) as f: + for index in range(f.n_frames): + f.seek(index) + frame = f.convert("RGBA") + thirds.append( + "".join( + "R" + if frame.getpixel((x, frame.height // 2)) == (255, 0, 0, 255) + else "." + for x in (frame.width // 6, frame.width // 2, frame.width * 5 // 6) + ) + ) + return thirds + + +def _assert_animated_60x45(test, content): + with Image.open(BytesIO(content)) as f: + test.assertEqual((f.size, f.n_frames), ((60, 45), 3)) + + +def _exif_rotated_jpeg_bytes(image_format="JPEG", **save_kwargs): + # Stored 400x300 with the left half red; orientation 6 displays it + # 300x400 with the red half on top. + stored = Image.new("RGB", (400, 300), color="blue") + stored.paste("red", (0, 0, 200, 300)) + exif = Image.Exif() + exif[0x0112] = 6 + buffer = BytesIO() + stored.save(buffer, image_format, exif=exif, **save_kwargs) + return buffer.getvalue() + + +def _jpeg_with_exif(tiff): + # A 400x300 JPEG whose EXIF segment holds ``tiff`` as written, valid or not + buffer = BytesIO() + Image.new("RGB", (400, 300), "blue").save(buffer, "JPEG") + payload = b"Exif\x00\x00" + tiff + app1 = b"\xff\xe1" + (len(payload) + 2).to_bytes(2, "big") + payload + # The APP1 segment goes straight after the start-of-image marker + return buffer.getvalue()[:2] + app1 + buffer.getvalue()[2:] + + +def _mpo_bytes(): + # A phone-camera JPEG with an embedded second image, which Pillow opens as MPO + return _exif_rotated_jpeg_bytes( + "MPO", save_all=True, append_images=[Image.new("RGB", (400, 300))] + ) + + +SVG_BYTES = b'' + + +def _svg_size(content): + root = etree.fromstring(content) + return root.get("width"), root.get("height") + + +def _noisy_jpeg_bytes(quality=95, **save_kwargs): + buffer = BytesIO() + Image.effect_noise((400, 300), 64).convert("RGB").save( + buffer, "JPEG", quality=quality, **save_kwargs + ) + return buffer.getvalue() + + +def _png_bytes(size): + buffer = BytesIO() + Image.new("RGB", size, color="blue").save(buffer, "PNG") + return buffer.getvalue() + + +def _assert_upright_150x200(test, content): + with Image.open(BytesIO(content)) as f: + test.assertEqual(f.size, (150, 200)) + red, _, blue = f.convert("RGB").getpixel((130, 20)) + test.assertGreater(red, blue) + red, _, blue = f.convert("RGB").getpixel((20, 180)) + test.assertGreater(blue, red) + + class TestPerseusExerciseCreation(StudioTestCase): """ Tests for the create_perseus_exercise function which handles exercise file generation. @@ -1061,6 +1183,233 @@ def test_image_resizing_in_hint(self): """Test image resizing functionality in hint content""" self._test_image_resizing_in_field("hint") + def _publish_sized_image(self, content, ext, width, height): + image_file = create_studio_file( + content, preset=format_presets.EXERCISE_IMAGE, ext=ext + )["db_file"] + image_url = exercises.CONTENT_STORAGE_FORMAT.format(image_file.filename()) + item = self._create_assessment_item( + exercises.SINGLE_SELECTION, + f"![shape]({image_url} ={width}x{height})", + [{"answer": "Answer A", "correct": True, "order": 1}], + ) + image_file.assessment_item = item + image_file.save() + + self._create_perseus_zip( + { + "mastery_model": exercises.M_OF_N, + "randomize": True, + "n": 1, + "m": 1, + "all_assessment_items": [item.assessment_id], + "assessment_mapping": {item.assessment_id: exercises.SINGLE_SELECTION}, + } + ) + + exercise_file = self.exercise_node.files.get(preset_id=format_presets.EXERCISE) + zip_file, _ = self._validate_perseus_zip(exercise_file) + image_files = sorted( + name for name in zip_file.namelist() if name.startswith("images/") + ) + return zip_file, image_file.filename(), image_files + + def test_sized_animated_gif_is_resized_with_its_frames(self): + zip_file, filename, (image,) = self._publish_sized_image( + _animated_gif_bytes(), "gif", 60, 45 + ) + self.assertNotEqual(image, f"images/{filename}") + _assert_animated_60x45(self, zip_file.read(image)) + + def test_sized_animated_gif_clears_each_frame_before_the_next(self): + zip_file, _, (image,) = self._publish_sized_image( + _moving_block_gif_bytes(), "gif", 60, 45 + ) + self.assertEqual(_red_thirds(zip_file.read(image)), ["R..", ".R.", "..R"]) + + def test_sized_animated_gif_resamples_every_frame_alike(self): + zip_file, _, (image,) = self._publish_sized_image( + _dotted_gif_bytes(), "gif", 200, 150 + ) + with Image.open(BytesIO(zip_file.read(image))) as f: + dots = [] + for index in range(f.n_frames): + f.seek(index) + dots.append(f.convert("RGB").crop((0, 0, 200, 120)).tobytes()) + self.assertEqual(dots[0], dots[1]) + + def test_sized_animated_png_keeps_each_frame(self): + # A red block moves along the bottom of a blue background and back + frames = [] + for left in (0, 40, 80, 40): + frame = Image.new("RGBA", (120, 90), "blue") + frame.paste("red", (left, 60, left + 40, 90)) + frames.append(frame) + buffer = BytesIO() + frames[0].save( + buffer, + "PNG", + save_all=True, + append_images=frames[1:], + duration=[100, 200, 300, 400], + ) + zip_file, _, (image,) = self._publish_sized_image( + buffer.getvalue(), "png", 60, 45 + ) + with Image.open(BytesIO(zip_file.read(image))) as f: + durations = [] + tops = [] + for index in range(f.n_frames): + f.seek(index) + f.load() + durations.append(f.info["duration"]) + tops.append(f.convert("RGBA").getpixel((30, 5))) + self.assertEqual(durations, [100, 200, 300, 400]) + self.assertEqual(tops, [(0, 0, 255, 255)] * 4) + + def test_sized_transparent_animated_png_leaves_no_trail(self): + # A green block moves across a transparent background + frames = [] + for left in (0, 50, 100, 150): + frame = Image.new("RGBA", (200, 150), (0, 0, 0, 0)) + frame.paste((0, 255, 0, 255), (left, 0, left + 50, 150)) + frames.append(frame) + buffer = BytesIO() + frames[0].save( + buffer, + "PNG", + save_all=True, + append_images=frames[1:], + duration=100, + disposal=PngImagePlugin.Disposal.OP_BACKGROUND, + blend=PngImagePlugin.Blend.OP_OVER, + ) + zip_file, _, (image,) = self._publish_sized_image( + buffer.getvalue(), "png", 100, 75 + ) + with Image.open(BytesIO(zip_file.read(image))) as f: + first_quarter_alphas = [] + for index in range(f.n_frames): + f.seek(index) + first_quarter_alphas.append(f.convert("RGBA").getpixel((12, 37))[3]) + self.assertEqual(first_quarter_alphas, [255, 0, 0, 0]) + + def test_sized_exif_rotated_photo_is_resized_upright(self): + zip_file, filename, (image,) = self._publish_sized_image( + _exif_rotated_jpeg_bytes(), "jpg", 150, 200 + ) + self.assertNotEqual(image, f"images/{filename}") + _assert_upright_150x200(self, zip_file.read(image)) + + def test_sized_photo_with_truncated_exif_is_resized(self): + zip_file, filename, (image,) = self._publish_sized_image( + _jpeg_with_exif(b"II"), "jpg", 200, 150 + ) + self.assertNotEqual(image, f"images/{filename}") + with Image.open(BytesIO(zip_file.read(image))) as f: + self.assertEqual(f.size, (200, 150)) + + def test_sized_rotated_photo_with_mistyped_exif_tag_is_resized(self): + # Orientation 6, then tag 0x0121, a LONG, stored as ASCII "Cam" + tiff = ( + b"II*\x00\x08\x00\x00\x00\x02\x00" + + struct.pack(" MAX_RESIZED_IMAGE_PIXELS: + return image_content + if img.n_frames * img.width * img.height > MAX_DECODED_ANIMATION_PIXELS: + return image_content + return _resize_animated_image(img, size) + if size[0] * size[1] > MAX_RESIZED_IMAGE_PIXELS: + return image_content + # Resize before rotating upright, so no full-size rotated copy is held. + # Orientations 5-8 swap the width and height. + orientation = _exif_orientation(img) + stored_size = size[::-1] if orientation in (5, 6, 7, 8) else size + same_size = stored_size == img.size + if not same_size: + img = img.resize(stored_size, Image.LANCZOS) + img = _transpose_upright(img, orientation) buffered = BytesIO() - img.save(buffered, format=original_format) - return buffered.getvalue() + img.save(buffered, format=original_format, icc_profile=icc_profile) + resized_content = buffered.getvalue() + if same_size and len(resized_content) >= len(image_content): + # Re-encoding at the same size only helps when it shrinks the file. + # Unchanged content hashes to the original's checksum, so it ships + # as the original. + return image_content + return resized_content except Exception as e: logging.warning(f"Error resizing image: {str(e)}") return None +def resize_svg(image_content, width, height): + """ + A copy of an SVG drawn at ``width``x``height``, or None when it doesn't + parse or has neither a ``viewBox`` nor a unitless size to scale from. + """ + try: + root = etree.fromstring(image_content, secure_parser()) + except etree.XMLSyntaxError as e: + logging.warning(f"Error resizing SVG: {str(e)}") + return None + if root.get("viewBox") is None: + natural_size = [ + SVG_PIXEL_LENGTH_REGEX.fullmatch(root.get(name) or "") + for name in ("width", "height") + ] + if not all(natural_size): + return None + # Without a viewBox, a new size crops the drawing instead of scaling it + root.set("viewBox", "0 0 {} {}".format(*(m[1] for m in natural_size))) + root.set("width", f"{width:g}") + root.set("height", f"{height:g}") + return etree.tostring(root.getroottree(), encoding="utf-8") + + def get_resized_image_checksum(image_content): return hashlib.md5(image_content).hexdigest() @@ -227,17 +354,16 @@ def _resize_image(self, checksum, ext, filename, width, height, new_file_path): ) as imgfile: original_content = imgfile.read() - resized_content = resize_image(original_content, width, height) + resize = resize_svg if ext.lower() == f".{file_formats.SVG}" else resize_image + resized_content = resize(original_content, width, height) - if not resized_content: + if resized_content: + new_img_ref = f"{get_resized_image_checksum(resized_content)}{ext}" + else: logging.warning(f"Failed to resize image {filename}. Using original image.") - self.add_file_to_write( - os.path.join(new_file_path, filename), original_content - ) - return - resized_checksum = get_resized_image_checksum(resized_content) + # Remember the original so later references skip the retry + new_img_ref, resized_content = filename, original_content - new_img_ref = f"{resized_checksum}{ext}" self.resized_images_map[filename][(width, height)] = new_img_ref self.add_file_to_write( os.path.join(new_file_path, new_img_ref), resized_content @@ -257,10 +383,7 @@ def _process_single_image( if similar_image: return similar_image - resized_image = self._resize_image( - checksum, ext, filename, width, height, new_file_path - ) - return resized_image or filename + return self._resize_image(checksum, ext, filename, width, height, new_file_path) def _is_valid_image_filename(self, filename): checksum, ext = os.path.splitext(filename) From 74880f01ef5635a66ab06342342216f726b6202b Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Wed, 30 Sep 2026 07:03:15 -0700 Subject: [PATCH 2/2] feat: resize QTI images at publish to their authored size - Sized (integer width and height) ship a resized copy per size in the QTI package; width/height stay in the XML. - Rewrites go per element, so one image at two sizes ships two files. - Derived Perseus markdown carries ` =WxH`, so publish resizes and fills the images map. - Legacy items keep the size on their QTI . Refs #6189 Co-Authored-By: Claude Opus 5.5 (1M context) --- .../tests/utils/qti/test_html_to_markdown.py | 16 + .../tests/utils/qti/test_media.py | 46 +++ .../tests/utils/test_exercise_creation.py | 288 +++++++++++++++++- .../contentcuration/utils/assessment/base.py | 7 +- .../utils/assessment/qti/archive.py | 52 +++- .../utils/assessment/qti/html_to_markdown.py | 7 +- .../utils/assessment/qti/media.py | 51 ++++ 7 files changed, 444 insertions(+), 23 deletions(-) diff --git a/contentcuration/contentcuration/tests/utils/qti/test_html_to_markdown.py b/contentcuration/contentcuration/tests/utils/qti/test_html_to_markdown.py index bdf37e3aaf..0cf4d04783 100644 --- a/contentcuration/contentcuration/tests/utils/qti/test_html_to_markdown.py +++ b/contentcuration/contentcuration/tests/utils/qti/test_html_to_markdown.py @@ -41,6 +41,19 @@ def test_image_gets_content_storage_prefix(): assert expected in result +def test_image_without_a_pixel_size_pair_carries_no_suffix(): + expected = "![d]({})".format(exercises.CONTENT_STORAGE_FORMAT.format("abc123.png")) + for size in ( + "", + 'width="200"', + 'height="150"', + 'width="50%" height="150"', + 'width="0" height="150"', + ): + result = _markdown_from_html(f'

d

') + assert result == expected, size + + def test_interaction_is_dropped_from_prompt(): fragment = "

Fill in

" assert html_to_markdown(_elements(fragment)) == "Fill in" @@ -172,6 +185,9 @@ def test_table(): "inline." ), "![alt text]({})".format(exercises.CONTENT_STORAGE_FORMAT.format("abc123.png")), + "![sized]({} =200x150)".format( + exercises.CONTENT_STORAGE_FORMAT.format("abc123.png") + ), "> A blockquote paragraph.", "- First bullet\n- Second bullet\n - Nested bullet\n- Third bullet", "1. First numbered\n2. Second numbered", diff --git a/contentcuration/contentcuration/tests/utils/qti/test_media.py b/contentcuration/contentcuration/tests/utils/qti/test_media.py index 1a3cd8e680..48b8320e1d 100644 --- a/contentcuration/contentcuration/tests/utils/qti/test_media.py +++ b/contentcuration/contentcuration/tests/utils/qti/test_media.py @@ -1,6 +1,7 @@ from contentcuration.tests.utils.qti.test_validation import VALID_CHOICE_ITEM from contentcuration.utils.assessment.qti.media import get_qti_media_references from contentcuration.utils.assessment.qti.media import rewrite_qti_media_paths +from contentcuration.utils.assessment.qti.media import rewrite_qti_sized_image_paths from contentcuration.utils.assessment.qti.media import set_qti_item_language from contentcuration.utils.assessment.qti.media import strip_studio_attributes from contentcuration.utils.assessment.qti.validation import parse_qti_xml @@ -87,6 +88,51 @@ def test_rewrite_ignores_values_not_in_mapping(): ) +def _path_for_size(filename, width, height): + return f"images/{width}x{height}-{filename}" + + +def test_sized_rewrite_points_each_sized_img_at_its_size(): + xml = ( + f'\n a > b' + f"" + f'' + ) + assert rewrite_qti_sized_image_paths(xml, _path_for_size) == ( + f'\n a > b' + f"" + f'' + ) + + +def test_sized_rewrite_leaves_imgs_without_a_pixel_size_or_checksum_src(): + xml = ( + f'' + f'' + f'' + f'' + f'' + f'' + f'\'src="{CHECKSUM_A}.png"\'' + '' + f'' + ) + assert rewrite_qti_sized_image_paths(xml, _path_for_size) == xml + + +def test_sized_rewrite_leaves_imgs_in_comments_and_cdata(): + sized_img = f'' + for hidden in (f"", f""): + xml = ( + f"{hidden}" + f'' + ) + assert rewrite_qti_sized_image_paths(xml, _path_for_size) == ( + f"{hidden}" + f'' + ) + + ITEM_WITHOUT_LANGUAGE = ( '' diff --git a/contentcuration/contentcuration/tests/utils/test_exercise_creation.py b/contentcuration/contentcuration/tests/utils/test_exercise_creation.py index 0d8944b6c4..e8579613d3 100644 --- a/contentcuration/contentcuration/tests/utils/test_exercise_creation.py +++ b/contentcuration/contentcuration/tests/utils/test_exercise_creation.py @@ -20,6 +20,7 @@ from lxml import etree from PIL import Image from PIL import ImageCms +from PIL import ImageDraw from PIL import PngImagePlugin from contentcuration.models import AssessmentItem @@ -155,6 +156,20 @@ def _png_bytes(size): return buffer.getvalue() +def _flat_colour_png_bytes(): + """A 400x300 diagram-like PNG whose downscale anti-aliases into a larger file.""" + image = Image.new("RGB", (400, 300), "white") + draw = ImageDraw.Draw(image) + for index in range(300): + x, y = index * 37 % 400, index * 53 % 300 + draw.ellipse( + [x, y, x + 20, y + 20], fill=(index * 71 % 256, index * 13 % 256, 90) + ) + buffer = BytesIO() + image.save(buffer, "PNG") + return buffer.getvalue() + + def _assert_upright_150x200(test, content): with Image.open(BytesIO(content)) as f: test.assertEqual(f.size, (150, 200)) @@ -1695,6 +1710,12 @@ class TestQTIExerciseCreation(StudioTestCase): "", ) + TWO_SIZE_IMGS = ( + 'a' + 'b' + 'c' + ) + def setUp(self): self.setUpBase() @@ -2122,9 +2143,9 @@ def test_image_resizing(self): -

First resized image: shape1

-

Second resized image (same): shape2

-

Third resized image (different): shape3

+

First resized image: shape1

+

Second resized image (same): shape2

+

Third resized image (different): shape3

Answer A

@@ -2184,6 +2205,92 @@ def test_image_resize_failure_uses_original(self): ).decode("utf-8") self.assertIn(f'src="images/{bad_image.filename()}"', item_xml) + def _publish_legacy_sized_image(self, content, ext, width, height, ref_ext=None): + image_file = create_studio_file( + content, preset=format_presets.EXERCISE_IMAGE, ext=ext + )["db_file"] + image_url = exercises.CONTENT_STORAGE_FORMAT.format( + f"{image_file.checksum}.{ref_ext or ext}" + ) + item = self._create_assessment_item( + exercises.SINGLE_SELECTION, + f"![shape]({image_url} ={width}x{height})", + [{"answer": "Answer A", "correct": True, "order": 1}], + ) + image_file.assessment_item = item + image_file.save() + + self._create_qti_zip(self._exercise_data([item])) + + exercise_file = self.exercise_node.files.get(preset_id=format_presets.QTI_ZIP) + zip_file = self._validate_qti_zip_structure(exercise_file) + image_files = sorted( + name for name in zip_file.namelist() if name.startswith("items/images/") + ) + item_doc = parse_qti_xml( + zip_file.read(f"items/{hex_to_qti_id(item.assessment_id)}.xml") + ) + (img,) = item_doc.iter("{http://www.imsglobal.org/xsd/imsqtiasi_v3p0}img") + return zip_file, image_file.filename(), image_files, img + + def test_legacy_sized_animated_gif_is_resized_with_its_frames(self): + zip_file, filename, (image,), img = self._publish_legacy_sized_image( + _animated_gif_bytes(), "gif", 60, 45 + ) + self.assertNotEqual(image, f"items/images/{filename}") + self.assertEqual(f"items/{img.get('src')}", image) + self.assertEqual((img.get("width"), img.get("height")), ("60", "45")) + _assert_animated_60x45(self, zip_file.read(image)) + manifest_xml = zip_file.read("imsmanifest.xml").decode("utf-8") + self.assertIn(f'', manifest_xml) + + def test_legacy_image_sized_above_natural_size_ships_a_copy_at_that_size(self): + zip_file, filename, (image,), img = self._publish_legacy_sized_image( + _png_bytes((400, 300)), "png", 800, 600 + ) + self.assertNotEqual(image, f"items/images/{filename}") + self.assertEqual(f"items/{img.get('src')}", image) + self.assertEqual((img.get("width"), img.get("height")), ("800", "600")) + with Image.open(BytesIO(zip_file.read(image))) as f: + self.assertEqual(f.size, (800, 600)) + + def test_legacy_image_with_a_fractional_size_carries_the_resized_size(self): + zip_file, _, (image,), img = self._publish_legacy_sized_image( + _png_bytes((400, 300)), "png", 200.6, 150.4 + ) + self.assertEqual((img.get("width"), img.get("height")), ("200", "150")) + with Image.open(BytesIO(zip_file.read(image))) as f: + self.assertEqual(f.size, (200, 150)) + + def test_legacy_image_with_a_size_below_one_pixel_carries_no_size(self): + for width, height in ((0.5, 0.5), (300, 0.6)): + with self.subTest(width=width, height=height): + *_, img = self._publish_legacy_sized_image( + _png_bytes((400, 300)), "png", width, height + ) + self.assertEqual((img.get("width"), img.get("height")), (None, None)) + + def _assert_legacy_uppercase_extension_resolves(self, width, height): + zip_file, _, (image,), img = self._publish_legacy_sized_image( + _png_bytes((400, 300)), "png", width, height, ref_ext="PNG" + ) + self.assertEqual(f"items/{img.get('src')}", image) + manifest_xml = zip_file.read("imsmanifest.xml").decode("utf-8") + self.assertIn(f'', manifest_xml) + + def test_legacy_resized_image_with_an_uppercase_extension_resolves(self): + self._assert_legacy_uppercase_extension_resolves(200, 150) + + def test_legacy_image_at_its_own_size_with_an_uppercase_extension_resolves(self): + self._assert_legacy_uppercase_extension_resolves(400, 300) + + def test_legacy_sized_svg_with_an_uppercase_extension_is_resized(self): + zip_file, _, (image,), img = self._publish_legacy_sized_image( + SVG_BYTES, "svg", 200, 150, ref_ext="SVG" + ) + self.assertEqual(f"items/{img.get('src')}", image) + self.assertEqual(_svg_size(zip_file.read(image)), ("200", "150")) + def test_multiple_question_types_mixed(self): """Test creating a QTI exercise with multiple supported question types""" # Create different types of supported questions with 32-char hex IDs @@ -2573,20 +2680,10 @@ def test_native_custom_processed_float_entry_is_packaged(self): zip_file = self._validate_qti_zip_structure(exercise_file) self.assertIn("items/item_text.xml", zip_file.namelist()) - def test_native_qti_item_missing_media_file_is_logged_and_omitted(self): - """A dangling media reference is logged and skipped, not fatal to publish.""" - raw_data = self.NATIVE_ITEM_XML.format(checksum="b" * 32, ext="png") + def _assert_missing_media_is_logged_and_omitted(self, raw_data): item = self._create_native_qti_item(raw_data) # no File row linked - exercise_data = { - "mastery_model": exercises.M_OF_N, - "randomize": True, - "n": 5, - "m": 3, - "all_assessment_items": [item.assessment_id], - "assessment_mapping": {item.assessment_id: exercises.QTI}, - } with self.assertLogs(level="ERROR") as logs: - self._create_qti_zip(exercise_data) + self._create_qti_zip(self._exercise_data([item])) self.assertTrue( any("no matching File record linked" in message for message in logs.output) ) @@ -2595,6 +2692,142 @@ def test_native_qti_item_missing_media_file_is_logged_and_omitted(self): self.assertIn("items/native_item_1.xml", zip_file.namelist()) self.assertNotIn(f"items/{'b' * 32}.png", zip_file.namelist()) + def test_native_qti_item_missing_media_file_is_logged_and_omitted(self): + """A dangling media reference is logged and skipped, not fatal to publish.""" + self._assert_missing_media_is_logged_and_omitted( + self.NATIVE_ITEM_XML.format(checksum="b" * 32, ext="png") + ) + + def test_native_qti_sized_image_missing_media_file_is_logged_and_omitted(self): + self._assert_missing_media_is_logged_and_omitted( + self.NATIVE_ITEM_XML.format(checksum="b" * 32, ext="png").replace( + 'alt="diagram"', 'alt="diagram" width="200" height="150"' + ) + ) + + def _create_native_qti_image_item(self, image_file, imgs): + raw_data = self.NATIVE_ITEM_XML.replace( + 'diagram', imgs + ) + item = self._create_native_qti_item(raw_data) + image_file.assessment_item = item + image_file.save() + return item + + def _publish_native_qti_images(self, image_file, imgs): + item = self._create_native_qti_image_item(image_file, imgs) + self._create_qti_zip(self._exercise_data([item])) + exercise_file = self.exercise_node.files.get(preset_id=format_presets.QTI_ZIP) + zip_file = self._validate_qti_zip_structure(exercise_file) + item_doc = parse_qti_xml(zip_file.read("items/native_item_1.xml")) + img_elements = item_doc.iter("{http://www.imsglobal.org/xsd/imsqtiasi_v3p0}img") + image_files = sorted( + name for name in zip_file.namelist() if name.startswith("items/images/") + ) + return zip_file, list(img_elements), image_files + + def test_native_qti_sized_images_ship_resized_per_size(self): + image_file = fileobj_exercise_image(size=(400, 300), color="blue") + filename = image_file.filename() + + zip_file, imgs, image_files = self._publish_native_qti_images( + image_file, self.TWO_SIZE_IMGS.format(filename=filename) + ) + + self.assertEqual(len(image_files), 2, image_files) + self.assertNotIn(f"items/images/{filename}", image_files) + self.assertEqual(imgs[0].get("src"), imgs[1].get("src")) + manifest = zip_file.read("imsmanifest.xml").decode("utf-8") + for img in imgs: + size = (int(img.get("width")), int(img.get("height"))) + with Image.open(BytesIO(zip_file.read(f"items/{img.get('src')}"))) as f: + self.assertEqual(f.size, size) + self.assertIn(img.get("src"), manifest) + + def test_native_qti_image_sized_and_unsized_ships_both(self): + image_file = fileobj_exercise_image(size=(400, 300), color="blue") + filename = image_file.filename() + + _, imgs, image_files = self._publish_native_qti_images( + image_file, + f'a' + f'b', + ) + + self.assertEqual(len(image_files), 2, image_files) + self.assertEqual(imgs[1].get("src"), f"images/{filename}") + self.assertNotEqual(imgs[0].get("src"), f"images/{filename}") + self.assertIn(f"items/{imgs[0].get('src')}", image_files) + self.assertIn(f"items/images/{filename}", image_files) + + def _assert_native_qti_ships_original(self, filename, imgs, image_files): + self.assertEqual(image_files, [f"items/images/{filename}"]) + self.assertEqual(imgs[0].get("src"), f"images/{filename}") + + def test_native_qti_sized_image_that_fails_to_resize_ships_original(self): + image_file = _create_unresizable_image() + filename = image_file.filename() + + _, imgs, image_files = self._publish_native_qti_images( + image_file, f'a' + ) + + self._assert_native_qti_ships_original(filename, imgs, image_files) + + def _publish_sized_native_qti_images(self, content, ext, *sizes): + image_file = create_studio_file( + content, preset=format_presets.EXERCISE_IMAGE, ext=ext + )["db_file"] + zip_file, imgs, image_files = self._publish_native_qti_images( + image_file, + "".join( + f'a' + for width, height in sizes + ), + ) + return zip_file, image_file.filename(), imgs, image_files + + def test_native_qti_image_sized_over_the_pixel_budget_ships_original(self): + _, filename, imgs, image_files = self._publish_sized_native_qti_images( + _png_bytes((40, 30)), "png", (20000, 15000) + ) + + self._assert_native_qti_ships_original(filename, imgs, image_files) + + def test_native_qti_sized_image_ships_the_resize_even_when_it_is_larger(self): + _, filename, imgs, image_files = self._publish_sized_native_qti_images( + _flat_colour_png_bytes(), "png", (200, 150) + ) + + self.assertEqual(image_files, [f"items/{imgs[0].get('src')}"]) + self.assertNotEqual(imgs[0].get("src"), f"images/{filename}") + + def test_native_qti_sized_svg_ships_a_scaled_copy_per_size(self): + with self.assertNoLogs(level="WARNING"): + ( + zip_file, + filename, + imgs, + image_files, + ) = self._publish_sized_native_qti_images( + SVG_BYTES, "svg", (200, 150), (200, 150), (100, 75) + ) + + self.assertEqual(len(image_files), 2, image_files) + self.assertNotIn(f"items/images/{filename}", image_files) + for img in imgs: + content = zip_file.read(f"items/{img.get('src')}") + self.assertEqual(_svg_size(content), (img.get("width"), img.get("height"))) + self.assertIn(b'viewBox="0 0 400 300"', content) + + def test_native_qti_sized_svg_without_a_natural_size_ships_original(self): + _, filename, imgs, image_files = self._publish_sized_native_qti_images( + b'', "svg", (200, 150) + ) + + self._assert_native_qti_ships_original(filename, imgs, image_files) + def test_native_qti_duplicate_identifier_raises(self): item1 = self._create_native_qti_item(VALID_CHOICE_ITEM) item2 = self._create_native_qti_item( @@ -2699,3 +2932,28 @@ def test_native_qti_perseus_derivation(self): # The kolibri-hint catalog card is derived back into a Perseus hint. hint_content = "".join(hint["content"] for hint in item_json["hints"]) self.assertIn("First hint.", hint_content) + + def test_native_qti_perseus_derivation_resizes_sized_images(self): + image_file = fileobj_exercise_image(size=(400, 300), color="blue") + filename = image_file.filename() + item = self._create_native_qti_image_item( + image_file, self.TWO_SIZE_IMGS.format(filename=filename) + ) + + self._create_perseus_zip(self._exercise_data([item])) + + exercise_file = self.exercise_node.files.get(preset_id=format_presets.EXERCISE) + with storage.open(exercise_file.file_on_disk.name, "rb") as f: + zip_file = zipfile.ZipFile(BytesIO(f.read())) + image_files = [n for n in zip_file.namelist() if n.startswith("images/")] + self.assertEqual(len(image_files), 2, image_files) + self.assertNotIn(f"images/{filename}", image_files) + + item_json = json.loads(zip_file.read("native_item_1.json").decode("utf-8")) + self.assertEqual( + sorted( + (size["width"], size["height"]) + for size in item_json["question"]["images"].values() + ), + [(100, 75), (200, 150)], + ) diff --git a/contentcuration/contentcuration/utils/assessment/base.py b/contentcuration/contentcuration/utils/assessment/base.py index d80adf9c52..fb0aafeae7 100644 --- a/contentcuration/contentcuration/utils/assessment/base.py +++ b/contentcuration/contentcuration/utils/assessment/base.py @@ -175,6 +175,8 @@ class ExerciseArchiveGenerator(ABC): ZIP_DATE_TIME = (2015, 10, 21, 7, 28, 0) ZIP_COMPRESS_TYPE = zipfile.ZIP_DEFLATED ZIP_COMMENT = "".encode() + # Keep the ` =WxH` suffix on sized images, for formats whose markup carries the size + KEEP_IMAGE_SIZES = False @property @abstractmethod @@ -432,6 +434,7 @@ def _replace_image(img_match): filename, checksum, ext, width, height, new_file_path ) + size_suffix = "" if width is not None and height is not None: image_list.append( { @@ -440,7 +443,9 @@ def _replace_image(img_match): "height": height, } ) - return f"![{img_match.group(1)}]({new_image_path}/{processed_filename})" + if self.KEEP_IMAGE_SIZES and width >= 1 and height >= 1: + size_suffix = f" ={int(width)}x{int(height)}" + return f"![{img_match.group(1)}]({new_image_path}/{processed_filename}{size_suffix})" content = re.sub(image_pattern, _replace_image, content) diff --git a/contentcuration/contentcuration/utils/assessment/qti/archive.py b/contentcuration/contentcuration/utils/assessment/qti/archive.py index f04c3889ad..af77f65d21 100644 --- a/contentcuration/contentcuration/utils/assessment/qti/archive.py +++ b/contentcuration/contentcuration/utils/assessment/qti/archive.py @@ -1,5 +1,6 @@ import json import logging +import os from dataclasses import dataclass from typing import Any from typing import Dict @@ -28,6 +29,7 @@ from contentcuration.utils.assessment.qti.imsmanifest import Resources from contentcuration.utils.assessment.qti.media import get_qti_media_references from contentcuration.utils.assessment.qti.media import rewrite_qti_media_paths +from contentcuration.utils.assessment.qti.media import rewrite_qti_sized_image_paths from contentcuration.utils.assessment.qti.media import set_qti_item_language from contentcuration.utils.assessment.qti.media import strip_studio_attributes from contentcuration.utils.assessment.qti.perseus_derive import ( @@ -51,6 +53,7 @@ class QTIExerciseGenerator(ExerciseArchiveGenerator): """ file_format = "zip" + KEEP_IMAGE_SIZES = True preset = format_presets.QTI_ZIP PERSEUS_IMAGE_DIR = "perseus/images" @@ -169,20 +172,42 @@ def _write_qti_media_files(self, assessment_item) -> Tuple[str, List[str]]: f"{', '.join(sorted(missing))}" ) + resized_paths = set() + + def _resized_path(filename, width, height): + file_obj = files_by_name.get(filename) + if file_obj is None: + return None + resized = self._process_single_image( + filename, + file_obj.checksum, + f".{file_obj.file_format_id}", + width, + height, + self.get_image_file_path(), + ) + path = f"{self.get_image_ref_prefix()}/{resized}" + resized_paths.add(path) + return path + + item_xml = rewrite_qti_sized_image_paths(raw_data, _resized_path) + # Media files can't stay bare in items/ alongside the item XML - they're # written to items/images/ (matching the legacy generator's layout) and # the item XML's references are remapped to the images/ prefix that - # resolves to that directory relative to the item file. + # resolves to that directory relative to the item file. Resized paths + # already carry that prefix, so only references to originals remain. + references = get_qti_media_references(item_xml) if resized_paths else filenames path_by_filename = {} - for filename in sorted(filenames - missing): + for filename in sorted(references - missing): file_obj = files_by_name[filename] self._add_original_image( file_obj.checksum, filename, self.get_image_file_path() ) path_by_filename[filename] = f"{self.get_image_ref_prefix()}/{filename}" - item_xml = rewrite_qti_media_paths(raw_data, path_by_filename) - return item_xml, sorted(path_by_filename.values()) + item_xml = rewrite_qti_media_paths(item_xml, path_by_filename) + return item_xml, sorted(resized_paths | set(path_by_filename.values())) def process_assessment_item(self, assessment_item): if assessment_item.type == exercises.PERSEUS_QUESTION: @@ -264,15 +289,30 @@ def create_assessment_item( ) result = convert_legacy_assessment_item_to_qti(legacy_item) + # Sized images come out of the conversion with a bare filename + image_entries = processed_data.get("question_images", []) + [ + image + for part in processed_data.get("answers", []) + + processed_data.get("hints", []) + for image in part.get("images", []) + ] + path_by_filename = { + os.path.basename(image["name"]): image["name"] for image in image_entries + } + item_xml = rewrite_qti_media_paths(result.xml, path_by_filename) + file_dependencies = list( + dict.fromkeys(path_by_filename.get(d, d) for d in result.file_dependencies) + ) + filename = self._qti_item_filepath(result.identifier) self._add_resource( QTIResource( identifier=result.identifier, filepath=filename, - file_dependencies=result.file_dependencies, + file_dependencies=file_dependencies, ) ) - return filename, result.xml.encode("utf-8") + return filename, item_xml.encode("utf-8") def _create_manifest_resources(self) -> List[Resource]: """Create manifest resources for all QTI items.""" diff --git a/contentcuration/contentcuration/utils/assessment/qti/html_to_markdown.py b/contentcuration/contentcuration/utils/assessment/qti/html_to_markdown.py index 388da67537..25af3f3c87 100644 --- a/contentcuration/contentcuration/utils/assessment/qti/html_to_markdown.py +++ b/contentcuration/contentcuration/utils/assessment/qti/html_to_markdown.py @@ -27,6 +27,7 @@ from contentcuration.utils.assessment.markdown import STRIKETHROUGH_DECORATION from contentcuration.utils.assessment.markdown import UNDERLINE_DECORATION +from contentcuration.utils.assessment.qti.media import img_pixel_size logger = logging.getLogger(__name__) @@ -75,7 +76,11 @@ def _render_img(el): src = el.get("src", "") if not src: return "" - return "![{}]({}{})".format(el.get("alt", ""), CONTENT_STORAGE_PREFIX, src) + size = img_pixel_size(el.get("width"), el.get("height")) + suffix = " ={}x{}".format(*size) if size else "" + return "![{}]({}{}{})".format( + el.get("alt", ""), CONTENT_STORAGE_PREFIX, src, suffix + ) def _text_decorations(el): diff --git a/contentcuration/contentcuration/utils/assessment/qti/media.py b/contentcuration/contentcuration/utils/assessment/qti/media.py index 325538def8..a2c2a1d607 100644 --- a/contentcuration/contentcuration/utils/assessment/qti/media.py +++ b/contentcuration/contentcuration/utils/assessment/qti/media.py @@ -1,4 +1,6 @@ import re +from typing import Optional +from typing import Tuple from lxml import etree @@ -25,6 +27,16 @@ STUDIO_ATTRIBUTE_OR_QUOTED_VALUE_REGEX = re.compile( r"""(?P\s+data-studio-[\w.-]+\s*=\s*(?:"[^"]*"|'[^']*'))|"[^"]*"|'[^']*'""" ) +PIXEL_LENGTH_REGEX = re.compile(r"[1-9][0-9]*") +# Comments and CDATA match whole, so an inside one is never read as a start tag +COMMENT_CDATA_OR_IMG_START_TAG_REGEX = re.compile( + r"""||])(?:[^>"']|"[^"]*"|'[^']*')*>""", + re.DOTALL, +) +XML_ATTRIBUTE_REGEX = re.compile( + r"""\s(?P[^\s=/>]+)\s*=\s*(?P["'])(?P.*?)(?P=quote)""", + re.DOTALL, +) QTI_MEDIA_ATTRIBUTE_VALUE_REGEX = re.compile( r"(?P" + "|".join(QTI_REFERENCE_ATTRIBUTES + ("srcset",)) + r")" @@ -32,6 +44,16 @@ ) +def img_pixel_size(width, height) -> Optional[Tuple[int, int]]: + """ + An ````'s size in pixels, or None unless both attributes are positive + integers: a percentage or a lone dimension gives no size to resize to. + """ + if not all(PIXEL_LENGTH_REGEX.fullmatch(value or "") for value in (width, height)): + return None + return int(width), int(height) + + def get_qti_media_references(raw_data): """ Scan QTI item XML for . media references in src/href/data/srcset @@ -90,6 +112,35 @@ def _replace_attribute(match): return QTI_MEDIA_ATTRIBUTE_VALUE_REGEX.sub(_replace_attribute, raw_data) +def rewrite_qti_sized_image_paths(raw_data, path_for_size): + """ + Point the ``src`` of each ```` with a pixel size at + ``path_for_size(filename, width, height)``, per element so one image at two + sizes gets two paths. A ``None`` path leaves the element alone. Every other + byte is kept, as in ``rewrite_qti_media_paths``. + """ + + def _replace_img(match): + tag = match.group(0) + if tag.startswith("