refactor: build the ungraded body in the choice converter

The answerless-choice case returned (None, None) and left
convert_legacy_assessment_item_to_qti to branch on it and rebuild the
body, splitting one decision across two functions. Return the Div
directly instead, so the caller always has an item body and only the
response declaration is optional.

Drop the defensive default on the contentnode__language__lang_code pop:
the key is in values, so a missing one is a bug, not a case to absorb.

Trim comments that restated the code they sat above.

Author: rtibblesbot

contentcuration/contentcuration/tests/utils/qti/test_convert.py

@@ -136,8 +136,8 @@ def test_single_selection_no_answers(self):
         self.assertTrue(validate_qti_item(result.xml.encode("utf-8")).is_valid)
 
     def test_choice_types_with_no_answers_omit_the_interaction(self):
-        # The guard is on the choice types as a group, not just SINGLE_SELECTION,
-        # which test_single_selection_no_answers already pins against the fixture.
+        # The guard covers the choice types as a group; test_single_selection_no_answers
+        # pins SINGLE_SELECTION against the fixture.
         for question_type in (exercises.MULTIPLE_SELECTION, "true_false"):
             with self.subTest(question_type=question_type):
                 item = _make_item(
@@ -156,8 +156,6 @@ def test_choice_types_with_no_answers_omit_the_interaction(self):
                 self.assertTrue(validate_qti_item(result.xml.encode("utf-8")).is_valid)
 
     def test_choice_type_with_no_answers_and_no_question(self):
-        # The model's own defaults - a question with nothing typed into it yet
-        # still carries an empty paragraph to render and edit.
         item = _make_item(
             type=exercises.MULTIPLE_SELECTION,
             question="",
@@ -171,10 +169,8 @@ def test_choice_type_with_no_answers_and_no_question(self):
         self.assertTrue(validate_qti_item(result.xml.encode("utf-8")).is_valid)
 
     def test_choice_type_with_no_answers_and_block_maths(self):
-        # Block maths renders as a top level <math>, which qti-item-body does not
-        # accept directly - hence the wrapping div. XSD validity is not asserted
-        # here: rendered MathML does not carry its namespace, the same gap
-        # test_free_response_with_maths lives with.
+        # Validity is not asserted: rendered MathML carries no namespace, the same
+        # gap test_free_response_with_maths lives with.
         item = _make_item(
             type=exercises.SINGLE_SELECTION,
             question="$$\\sum_n^sxa^n$$",

contentcuration/contentcuration/tests/viewsets/test_assessmentitem.py

@@ -1243,8 +1243,7 @@ def test_supported_legacy_types_returned_as_qti(self):
                 self.assertIn(interaction, item["raw_data"])
 
     def test_answerless_choice_item_is_returned_as_valid_qti(self):
-        # The shape the editor writes for every newly added question: a choice
-        # type with no answers and nothing typed into it yet.
+        # The shape the editor writes for every newly added question.
         assessment_id = self._create_item(type=exercises.SINGLE_SELECTION).assessment_id
 
         item = self._get_item(assessment_id)
@@ -1269,10 +1268,9 @@ def test_converted_item_has_no_legacy_field_content(self):
         self.assertNotIn("contentnode__language__lang_code", item)
 
     def test_converted_items_are_tagged_with_their_own_node_language(self):
-        # A contentnode__in read spans several nodes, so each item has to pick
-        # up its own node's language rather than one language for the batch.
-        # pt-BR has a subcode, so the bare lang_code publish tags items with is
-        # distinguishable from the Language primary key.
+        # A contentnode__in read spans several nodes, so each item must pick up its
+        # own node's language; pt-BR has a subcode, so the bare lang_code publish
+        # tags items with is distinguishable from the Language primary key.
         self.node.language = models.Language.objects.get(id="pt-BR")
         self.node.save()
         other_node = models.ContentNode.objects.create(
@@ -1360,8 +1358,8 @@ def test_unconvertible_type_raises_on_list_route(self):
             )
 
     def test_unconvertible_type_is_not_a_404_on_detail_route(self):
-        # serialize_object() turns ValueError into a 404, which would report a
-        # corrupt row as a missing one - the failure must surface instead.
+        # Only this route goes through serialize_object(), which is what would
+        # otherwise swallow the failure into a 404.
         assessmentitem = self._create_item(
             type="not_a_real_type", question="What is 2+2?"
         )

contentcuration/contentcuration/utils/assessment/qti/convert.py

@@ -7,6 +7,7 @@
 from typing import List
 from typing import Optional
 from typing import Tuple
+from typing import Union
 
 from le_utils.constants import exercises
 
@@ -142,17 +143,17 @@ def _response_declaration(
 
 def _create_choice_interaction_and_response(
     item: LegacyAssessmentItem,
-) -> Tuple[Optional[ChoiceInteraction], Optional[ResponseDeclaration]]:
-    """
-    Create a QTI choice interaction for multiple choice questions, or
-    ``(None, None)`` if the question has no answers to choose between.
-    """
+) -> Tuple[Union[ChoiceInteraction, Div], Optional[ResponseDeclaration]]:
+    """Create a QTI choice interaction for multiple choice questions."""
     if not item.answers:
-        # An answerless choice question is ordinary in-progress authoring state -
-        # it is what the editor writes for every newly added question - but the
-        # XSD requires a qti-choice-interaction to carry at least one
-        # qti-simple-choice, and there is nothing to bind a response to.
-        return None, None
+        # An answerless choice question is ordinary in-progress authoring state,
+        # but the XSD requires at least one qti-simple-choice and there is no
+        # response to bind, so emit the question alone, ungraded. Div because
+        # rendered markdown can start with a top level <math>, which
+        # qti-item-body does not accept; empty P so an untyped question still
+        # renders as an editable paragraph.
+        body = _create_html_content_from_text(item.question) or [P()]
+        return Div(children=body), None
 
     multiple_select = item.type == exercises.MULTIPLE_SELECTION
 
@@ -322,25 +323,18 @@ def convert_legacy_assessment_item_to_qti(
     else:
         raise ValueError(f"Unsupported question type: {item.type}")
 
-    if interaction is None:
-        # Emit the question text alone, ungraded. Div because rendered markdown
-        # can start with a top level <math>, which qti-item-body does not accept
-        # directly; the empty P stands in for the text a newly added question
-        # does not have yet, so the body is a paragraph to render and edit
-        # rather than a bare empty div.
-        item_body = ItemBody(
-            children=[
-                Div(children=_create_html_content_from_text(item.question) or [P()])
-            ]
-        )
-        response_declarations = []
-        response_processing = None
-    else:
-        item_body = ItemBody(children=[interaction])
-        response_declarations = [response_declaration]
-        response_processing = ResponseProcessing(
+    item_body = ItemBody(children=[interaction])
+
+    response_declarations = (
+        [response_declaration] if response_declaration is not None else []
+    )
+    response_processing = (
+        ResponseProcessing(
             template="https://purl.imsglobal.org/spec/qti/v3p0/rptemplates/match_correct"
         )
+        if response_declarations
+        else None
+    )
 
     outcome_declaration = OutcomeDeclaration(
         identifier="SCORE", cardinality=Cardinality.SINGLE, base_type=BaseType.FLOAT

contentcuration/contentcuration/viewsets/assessmentitem.py

@@ -32,9 +32,8 @@
     )
 )
 
-# Types the read path returns as stored. Everything else is a legacy type that is
-# converted to QTI on read until the global backfill (#6007) makes the conversion
-# permanent, at which point AssessmentItemViewSet.consolidate goes away.
+# Everything else is a legacy type, converted to QTI on read until the global
+# backfill (#6007) makes that permanent and consolidate() goes away.
 PASSTHROUGH_TYPES = (exercises.QTI, exercises.PERSEUS_QUESTION)
 
 
@@ -342,8 +341,8 @@ class AssessmentItemViewSet(BulkCreateMixin, BulkUpdateMixin, ValuesViewset):
         "source_url",
         "randomize",
         "deleted",
-        # Only consumed by consolidate(), which pops it back off - publish tags
-        # an item with the bare lang_code of its content node's language
+        # Only consumed by consolidate(), which pops it back off. Publish tags an
+        # item with the bare lang_code of its content node's language
         # (utils/assessment/qti/archive.py), so the read path matches.
         "contentnode__language__lang_code",
     )
@@ -354,17 +353,16 @@ class AssessmentItemViewSet(BulkCreateMixin, BulkUpdateMixin, ValuesViewset):
 
     def consolidate(self, items, queryset):
         for item in items:
-            language = item.pop("contentnode__language__lang_code", None)
+            language = item.pop("contentnode__language__lang_code")
             if item["type"] in PASSTHROUGH_TYPES:
                 continue
             try:
                 # A new dict, so the language does not leak into the response.
                 result = convert_legacy_question_to_qti(dict(item, language=language))
             except (IndexError, ValueError, TypeError) as e:
-                # serialize_object() turns IndexError/ValueError/TypeError into
-                # a 404 (base.py), reporting a corrupt row as a missing one;
-                # re-raise as a type it does not catch (pydantic and json errors
-                # both subclass ValueError).
+                # serialize_object() turns these into a 404 (base.py), reporting
+                # a corrupt row as a missing one; re-raise as a type it does not
+                # catch. pydantic and json errors both subclass ValueError.
                 raise LegacyConversionError(
                     f"Could not convert assessment item {item['assessment_id']} to QTI"
                 ) from e