refactor: extract reusable legacy→QTI conversion into qti/convert.py#6031
refactor: extract reusable legacy→QTI conversion into qti/convert.py#6031rtibblesbot wants to merge 4 commits into
Conversation
Pulls the per-type XML-building logic out of QTIExerciseGenerator into a standalone, storage/Django-decoupled convert_legacy_assessment_item_to_qti() so the API dual-read, ricecooker ingest, and global backfill can each call it directly instead of re-implementing conversion. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
This all makes sense to me, and it's pretty much just a code move rather than a full refactor - I think it can be done slightly more completely (especially with this intermediary state where the old tests still pass, so we can be confident of cleaning them up now).
| @@ -0,0 +1,251 @@ | |||
| # flake8: noqa: E501 | |||
There was a problem hiding this comment.
Rather than do this, maybe put the expected output XML into separate fixture files?
There was a problem hiding this comment.
Done — moved the expected QTI XML for each conversion test into fixture files under tests/utils/qti/fixtures/ (single_selection.xml, multiple_selection.xml, true_false.xml, input_question.xml, plus two new ones for the ported edge cases below) and load them via a small _load_fixture() helper instead of inline triple-quoted strings.
There was a problem hiding this comment.
OK, but you've left the noqa comment here? Remove it.
There was a problem hiding this comment.
Removed the stray # flake8: noqa: E501 — it was left over from when the expected XML lived inline as long strings. Now that it lives in fixture files the exception is unneeded; flake8 passes clean without it.
| return "".join(x.strip() for x in xml_string.split("\n")) | ||
|
|
||
|
|
||
| class ChoiceInteractionConversionTests(unittest.TestCase): |
There was a problem hiding this comment.
I agree that this is now the right place for this, but I think we should also clean up the conversion specific tests from the archive creation tests here: https://github.com/learningequality/studio/blob/unstable/contentcuration/contentcuration/tests/utils/test_exercise_creation.py#L1226
There was a problem hiding this comment.
Cleaned up TestQTIExerciseCreation in test_exercise_creation.py: removed test_multiple_selection_question, test_free_response_question, and test_input_question, which only re-asserted per-type conversion XML already covered by test_convert.py. Trimmed test_basic_qti_exercise_creation to drop its duplicate item-XML assertion (kept the zip/manifest checks, which are archive-specific). test_question_with_mathematical_content, test_free_response_question_no_answers, and test_free_response_question_with_maths tested conversion edge cases (MathML rendering, empty-answers response declaration) with no archive-specific behavior, so I ported them into test_convert.py as test_math_content_in_choice_interaction, test_free_response_no_answers, and test_free_response_with_maths (using fixture files per the other thread) rather than dropping that coverage. Left the tests that exercise genuinely archive-specific behavior (manifest structure, image bundling/resizing, native QTI passthrough, Perseus/unsupported-type dispatch) as-is. Full suite still green (29 passed in test_exercise_creation.py, 10 in test_convert.py).
…sion tests Addresses PR learningequality#6031 review: expected XML for qti/convert.py's tests now lives in fixture files instead of inline strings, and the per-type conversion assertions duplicated from test_exercise_creation.py's TestQTIExerciseCreation are removed there (archive-level packaging behavior stays); the math-rendering and no-answers edge cases they covered move to test_convert.py alongside the other conversion tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
This looks good - just one last piece of cleanup needed.
| @@ -0,0 +1,251 @@ | |||
| # flake8: noqa: E501 | |||
There was a problem hiding this comment.
OK, but you've left the noqa comment here? Remove it.
The noqa was needed when long XML strings lived inline; now that expected XML lives in fixture files the exception is unnecessary. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…em setup Consolidates the duplicated ResponseDeclaration construction in choice and text-entry conversion into one helper, and factors repeated LegacyAssessmentItem setup in tests into a _make_item helper. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2 participants
Summary
Legacy→QTI conversion logic lived only inside the archive-building
QTIExerciseGenerator, so other consumers needing the same per-type conversion would have to duplicate it. Extract the choice/text-entry XML-building logic into a standalone, storage/Django-decoupled function, and haveQTIExerciseGeneratordelegate to it instead of building QTI XML itself.References
Fixes #6003. Related: #6030, #6002, #6007, #6011.
Reviewer guidance
contentcuration/contentcuration/utils/assessment/qti/convert.py:169—convert_legacy_assessment_item_to_qtiand its per-type builders were moved fromarchive.py; check the extraction preserved output byte-for-byte rather than just structurally.contentcuration/contentcuration/utils/assessment/qti/archive.py:139— confirm the native-QTI / perseus-raise / delegate-to-convert dispatch order increate_assessment_itemis unchanged from before the refactor.AI usage
Used Claude Code to extract the conversion logic and write per-type tests. Verified with the full QTI test suite and flake8.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
🟡 Waiting for feedback
Last updated: 2026-07-05 01:41 UTC