Replace the legacy assessment editor with the QTI editor - #6095
Replace the legacy assessment editor with the QTI editor#6095AlexVelezLl wants to merge 24 commits into
Conversation
A choice question with no answers is what the editor writes for every newly added question, and is the model's own default shape - but the QTI XSD requires qti-choice-interaction to carry at least one qti-simple-choice, so conversion raised a ValidationError. Emit the question text alone, with no interaction, response declaration or response processing. The item body wraps that text in a div because rendered markdown can start with a top level <math>, which qti-item-body does not accept directly, and falls back to an empty paragraph because the container cannot be empty and a newly added question has no text yet. Publish and ricecooker upload reach the same converter, so both stop raising on these items too.
AssessmentItemViewSet.consolidate() replaces each still-legacy row's type and raw_data with the converter's output, so the frontend only ever receives type='QTI' with item XML in raw_data. QTI and perseus_question rows pass through as stored. The converted item is tagged with the bare lang_code of its content node's language, matching publish, so the XML the API hands out is the XML the channel publishes. A conversion failure surfaces as LegacyConversionError rather than the underlying ValueError, which serialize_object() would turn into a 404 - reporting a corrupt row as a missing one. Every read is already scoped to one content node by the required filter, so the cost of raising is that exercise, not the channel. This whole path goes away with the global backfill (learningequality#6007).
The block-maths converter test asserted only that the rendered <math> survived, which passes with or without the wrapping <div> that the test exists to justify - assert the wrapper it documents. Restore the lead sentence on PASSTHROUGH_TYPES so "everything else" has a referent, and note the answerless return in the choice-interaction helper's docstring now that it can return (None, None).
serialize_object() swallows IndexError alongside ValueError and TypeError into a 404, so an IndexError raised during conversion would report a corrupt row as a missing one - the failure mode consolidate()'s re-raise exists to prevent. Widen the caught tuple to match. Assert the node language the values tuple carries for the conversion does not leak into the response, on both the converted and passed-through branches; without the latter a pop placed after the passthrough check would ship an internal join key to the client. Correct the comment on the answerless item body: an empty <div /> inside qti-item-body is XSD-valid, so the empty <p /> is there to give the body a paragraph to render and edit, not to satisfy the schema. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
A declaration with no values serialized as an empty <qti-correct-response/>, which the QTI schema rejects: the element is optional, but must hold at least one <qti-value> when present. Every save of a question with no correct answer yet — including every newly created one — was refused by the server. Capabilities now return null when they have nothing to serialize, and the declaration drops them instead of emitting an empty element. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The HTML parser puts fragments in the XHTML namespace, and importing those nodes into the XML document made XMLSerializer write an explicit xmlns on every element — <p xmlns="http://www.w3.org/1999/xhtml">Lima</p>. The QTI schema expects inline content in the namespace the item root declares, so the server rejected any question whose prompt or answers carried markup. HTML-parsed nodes are now re-created in the XML document without a namespace, so they inherit the item's. Foreign subtrees (MathML from the formula button, SVG) keep theirs, which QTI does expect declared. The text-entry builder reached the same trap from the other side: it parsed the prompt itself and passed the nodes as `children`, which still go through importNode. It hands the prompt to buildXmlNode as innerHTML now, and appends the interaction paragraph afterwards, so there is one adoption path rather than two ways in. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The type is stored and served as "QTI" (le_utils exercises.QTI), not "qti". With the lowercase value nothing matched: every question rendered as "Unknown type" with editing disabled, and a newly created item would have failed the model's type choices on the way to the server. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Studio needs to know whether a question is complete without rendering it, and shared/utils/validation.js — where that check lives — is imported by shared views on every webpack entry. Reaching the descriptors through a registry that also holds the interaction editors would have pulled them, and TipTap with them, into every bundle. So an interaction is now registered in two places, each obvious from what it imports: descriptors.js imports Descriptor.js files and nothing else, index.js imports the Editor.vue files and re-exports the descriptors. A descriptor no longer carries its own editor component, which means nothing has to reach in and attach one — defineInteraction and the per-interaction index modules are gone, and InteractionSection resolves the component from the editors map by interaction type. The two lists have to agree, so a test asserts they do. Descriptors extend an InteractionDescriptor base class that checks the contract as the singleton is constructed, replacing defineInteraction's key check, and supplies the defaults that were repeated in each descriptor: matching by tag name, and contributing no question type options. Files are named for their role — choice/Descriptor.js, choice/Editor.vue — so a new interaction is two conventionally-named files and one line in each registry. Placement joins that contract rather than being assigned by hand afterwards, which lets it become the single source of truth for something constants.js used to restate: INLINE_INTERACTION_TAGS existed because parseItem could not ask the registry without a cycle, since the descriptors import parseItem for parseXML. That cycle was only there because one module held two layers, so the leaf DOM helpers move to serialization/xml.js — leaving parseItem free to ask the registry through isInlineInteraction, and leaving an inline interaction with nothing to declare beyond its own placement. Descriptor resolution moves out of useInteractionDescriptor into a pure resolveDescriptor, so the editor and the headless validator share one path, and reports its parse failure as a ValidationError code the caller presents. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The editor surfaces errors through useInteraction, which already holds the parsed interaction state, but Studio has to know whether every question of a node is complete while none of them are on screen. validateQtiItem walks the same descriptor parse/validate pair from raw XML, and reports an unreadable or interaction-less item as an error of its own. It also takes allowFreeResponse, for the caller that only accepts scorable questions — free response is only meaningful on a survey. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A new item had no raw_data at all, which left it unauthorable — the card only renders an interaction when the body holds one — and the server rejects an empty document outright, so "New question" could never have been saved. New items are now seeded with the default interaction's empty state, wrapped in an item that carries a generated identifier and a fixed title. A test asserts the skeleton round-trips to exactly one choice interaction, so a change to the default descriptor surfaces there, and a matching backend test validates the same document against the XSD to keep the two in step. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🟡 Waiting for changesLast updated: 2026-08-17 22:00 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6095 — CI green, manual QA not run.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran a phased review pipeline over the pull request diff:
- Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
- Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
- Specialized frontend/backend review passes applied framework-specific lenses where those files changed
- For UI changes: manual QA and an accessibility audit against a live dev server, when available
- Checked CI status and linked issue acceptance criteria
- Synthesized one review from those passes and chose the verdict from the findings, CI status, and QA evidence
| # 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( |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: Uncaught: one bad row 500s the whole contentnode__in list. Log and skip per item; isUnsupported renders it.
| * holds. The card only needs to know whether there are any. | ||
| */ | ||
| const errors = ref([]); | ||
| const isIncomplete = computed(() => errors.value.length > 0); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: isIncomplete reads @update:errors, the banner validateQtiItem; ordering/Editor.vue emits nothing, and PARSE_ERROR/NO_INTERACTION/FREE_RESPONSE_NOT_ALLOWED reach no card. Use validateQtiItem(props.item.raw_data, { allowFreeResponse }).
| # 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", |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Publish uses the channel lang_code; convert_legacy_question_to_qti hardcodes "en" — frozen on first write.
|
|
||
| // Emit only when the assembled XML actually changes after initial mount. | ||
| watch(rawData, newVal => { | ||
| if (props.mode !== 'edit') return; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Close flips mode; is a late rawData flush dropped? Untested.
| freeResponseInvalid, | ||
| ); | ||
| } | ||
| assessmentItemsErrors[assessmentItemId] = getAssessmentItemErrors(assessmentItem, { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Uncached: DOMParser per item per call. Memoise on raw_data.
| :style="alertStyle" | ||
| <div | ||
| v-if="invalidItemsCount" | ||
| class="incomplete-banner" |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Add role="status"; the replaced VAlert was announced.
| # writes it under the content-storage placeholder, stripped by the time we | ||
| # get here, so drop any remaining directory the same way the editor does. | ||
| token.attrs = { | ||
| "src": match["src"].rsplit("/", 1)[-1], |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: Remote src becomes a local filename; gate on QTI_CHECKSUM_FILENAME_REGEX, likewise imageSrc.js:68.
| this.updateAssessmentItems(assessmentItems); | ||
|
|
||
| // reaches into Details Tab to run save of diffTracker | ||
| // reaches into Details Tab to run save of difxfTracker |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: difxfTracker → diffTracker.
| @@ -0,0 +1,62 @@ | |||
| import { Placement, QtiInteraction } from '../constants'; | |||
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: .vue-free descriptors keep TipTap out of shared/utils/validation.js.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
No new commits since my last review (59b139d).
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Ran the same phased review passes as a first review (core, frontend/backend lenses, manual QA when required)
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
A question card gave no sign that the question inside it was unfinished, which the exercise editor it is replacing did show. Rather than validate the item a second time, each interaction editor reports the errors useInteraction already computes for the inline messages, and the card renders an indicator while there are any. Validation stays debounced, so the indicator appears once the author pauses rather than on every keystroke. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Perseus questions are passed through by the API rather than converted, and an item whose XML cannot be read has no interaction model to hand an editor. Both used to fall through to the "content editor coming soon" placeholder, which invites an author to edit something that would be overwritten. They now render a card that says so, with the edit action disabled and the card refusing to open, while move, add and remove keep working. Validation already leaves Perseus items alone, so they never count as incomplete. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every card re-assembles its XML on mount, and the serialized form rarely matches the stored one byte for byte, so simply opening a list of questions reported all of them as changed — which, once the editor is wired to the sync layer, would rewrite every question in an exercise just for being looked at. Only the card in edit mode reports its XML now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The questions tab now renders QTIEditor instead of the legacy AssessmentEditor. The editor stays a controlled list component that hands back the whole array, so useAssessmentItems does the translating: it diffs that array against what the store holds and dispatches one write per item, reordering before adding or removing so no two questions briefly claim the same position. A question the author adds counts as incomplete straight away, rather than being marked for delayed validation: the card already says so as soon as it renders, so the tab icon and the "N incomplete questions" banner would otherwise disagree with it until the next reload. The vuex actions stop stringifying answers and hints — the API rejects those fields on a QTI item, whose content lives in raw_data. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The resource panel's question preview read question, answers and hints, which the API no longer returns, so it rendered empty cards for every exercise. It now shows each question through the QTI card in view mode, which brings its own numbering and type label, so the panel drops the numbering column it wrapped around the old preview. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing renders AssessmentEditor or the components underneath it now that the questions tab and the resource panel both go through the QTI editor, and the question shapes they were built around no longer reach the client. Gone with them: the toolbar action and question type label constants, the answer-mapping helpers in channelEdit/utils, the array helpers in shared/utils/helpers that only those editors used, and the strings for all of it. The regex behind numeric answers is exercised by the QTI editor now, so its tests move there rather than disappearing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The mutation parsed and sorted the answers and hints the API used to send as JSON strings. Nothing reads them now that questions are QTI, and leaving the parsed arrays on the stored item invites them back into an update payload, which the API rejects for a QTI item. The mutation just merges what it is given. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
59b139d to
b3b1c9b
Compare
Questions no longer arrive with question, answers and hints — the API serves every item as QTI — so validating those fields judged every question by empty data. getAssessmentItemErrors now asks the QTI editor's validator about raw_data, and the sanitize helpers that only existed to tidy legacy answers before validating them are gone, along with the legacy question types and error codes nothing can produce any more. Studio keeps its own rule on top: a free-response question only counts as valid on a survey, which the getter derives from the node's modality and passes down. isNodeComplete keeps its previous, laxer treatment of free response so node completeness does not silently change. Closing the edit modal still stops delaying validation for questions the author has started writing, but the check reads the prompt out of the QTI, and commits to the store rather than dispatching a save — the flag is a display concern that never reaches the server. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every legacy item is served to the client as QTI, but the serializer refused raw_data unless the row itself already said QTI — so editing any question authored before the QTI editor failed, and the client cannot say otherwise: its change records carry only fields that differ from its local copy, which already reads QTI. An existing row that receives raw_data is now converted, which is the same migration the global backfill (learningequality#6007) will apply to every item, done one item at a time as authors touch them. Creates keep the old guard, and invalid QTI is still refused, leaving the row untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Validation waited 400 ms after the last state change before updating errors, so for that window the messages on screen described a state the editor had already left — and the card indicator built on those errors lagged with them. Nothing about validating is expensive: it reads the state the editor already holds. The watcher now calls runValidation directly. runValidation stays exposed for the explicit triggers the text-entry editor uses when closing a panel. The tests that asserted the debounce rather than the behaviour now say what the editor does: an incomplete question reports as soon as it renders, and a complete one reports nothing. The rest just lose their fake timers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Whether a question's errors are shown yet is the QTI editor's business now: the item always has them, and the editor decides when they surface. Studio only needs to know whether a question is complete, and answering that with "unless it was created recently" made the tab icon and the incomplete-questions banner disagree with the card they describe. So the DELAYED_VALIDATION symbol and the ignoreDelayed argument threaded through the assessmentItem getters are gone, along with the pass over the items on modal close that used to clear the flag, and the stripping of the symbol on the way to IndexedDB. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A legacy question stores its images as Perseus markdown, which extends the
CommonMark image with a size and alignment suffix:
Neither suffix is valid CommonMark, so the destination fails to parse, the
construct is not recognised as an image at all, and render_markdown emits it as
literal text — which is what the QTI editor then showed, verbatim, in place of
every pre-migration image. The old editor never hit this because it read the
markdown on the frontend, where IMAGE_REGEX does understand both suffixes.
An inline rule now claims the construct before markdown-it's image rule, but only
when a suffix is actually present, leaving plain images to the built-in rule. The
size becomes width/height, rounded because Perseus allows fractions where the Img
model wants integers. The alignment is consumed and dropped: QTI's Img has no
attribute to carry it, and the reverse conversion does not emit one either.
That leaves the src, which QTI stores as a bare <checksum>.<ext> — the form
publishing rewrites into a package's images/ directory, and the only form Img
accepts, since it rejects absolute paths. A browser cannot load it, so images
were resolved to a storage URL on the way into the editor and stored bare on the
way out. The markdown format already did this through preprocessMarkdown; the
html format, which the QTI editors use, did no resolution at all and worked only
because TipTap writes an absolute src at insert time.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three interactions had three conventions: choice paired validation.js with validate.spec.js, textEntry paired validation.js with validation.spec.js, and ordering paired validate.js with validate.spec.js. Each interaction now has validation.js beside its parse.js, with a spec named after the module it covers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6095 — 7 of 8 prior findings resolved or acknowledged; 1 still open, plus 2 new (1 blocking).
CI pending; manual QA did not run, so nothing here is visually verified.
- blocking —
ResourcePanelcounts incomplete questions withoutallowFreeResponse(inline). - suggestion —
getDescriptorForQuestionTypehas no callers (inline). - suggestion (still open) —
assessmentItem/getters.js:45re-parsesraw_datawithDOMParserper item per getter call;QTIItemEditor.isIncompletenow adds a second parse per card. Memoise onraw_data.
Prior-finding status
RESOLVED — shared/views/QTIEditor/components/QTIItemEditor/index.vue:197 — isIncomplete read interaction-reported errors, missing item-level codes
RESOLVED — shared/views/QTIEditor/components/QTIItemEditor/index.vue:179 — late rawData flush dropped when the card closes
RESOLVED — channelEdit/components/AssessmentTab/AssessmentTab.vue:6 — banner not announced; add role="status"
RESOLVED — channelEdit/components/edit/EditModal.vue:493 — difxfTracker typo
RESOLVED — shared/views/QTIEditor/interactions/descriptors.js:1 — praise: .vue-free descriptors keep TipTap out of shared/utils/validation.js
ACKNOWLEDGED — viewsets/assessmentitem.py:379 — one unconvertible row 500s the whole contentnode__in list
ACKNOWLEDGED — viewsets/assessmentitem.py:360 — conversion language frozen on first write
ACKNOWLEDGED — utils/assessment/markdown.py:53 — remote src reduced to a local filename
UNADDRESSED — channelEdit/vuex/assessmentItem/getters.js:45 — uncached DOMParser per item per getter call
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| return this.getAssessmentItems(this.nodeId); | ||
| }, | ||
| // Free-response questions cannot be scored, so they only count as complete on a survey. | ||
| allowFreeResponse() { |
There was a problem hiding this comment.
blocking: This computed reaches the preview cards (:allowFreeResponse at line 148) but not invalidQuestionCount at line 723, which still calls getAssessmentItemErrors(ai) with no options and so takes the permissive default.
On a non-survey exercise with a free-response question the card shows "Incomplete" while the banner and tab warning stay silent — the divergence this PR sets out to remove. AssessmentTab avoids it by counting via assessmentItem/getInvalidAssessmentItemsCount, which resolves the modality itself (vuex/assessmentItem/getters.js:33-40). Mapping that getter here drops both the divergence and the duplicated derivation:
...mapGetters('assessmentItem', ['getAssessmentItems', 'getInvalidAssessmentItemsCount']),
...
invalidQuestionCount() {
return this.isExercise && this.getInvalidAssessmentItemsCount({ contentNodeId: this.nodeId });
},allowFreeResponse defaults permissive in three places (validateItem.js:14, QTIEditor/index.vue:209, QTIItemEditor/index.vue:271), so a consumer that forgets it under-reports silently rather than failing.
| * @param {string} questionType | ||
| * @returns {import('./InteractionDescriptor').InteractionDescriptor|undefined} | ||
| */ | ||
| export function getDescriptorForQuestionType(questionType) { |
There was a problem hiding this comment.
suggestion: No callers — re-exported from interactions/index.js:24 and never imported. The one place that needs it, composables/useInteractionDescriptor.js:36-40, inlines the same descriptors.find(...) ?? registry[DEFAULT_INTERACTION]; using this instead also lets that file drop its descriptors/registry/DEFAULT_INTERACTION imports.
b3b1c9b to
d97f062
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6095 — 9 of 11 prior findings resolved; 2 still open, plus 2 new.
Two blocking: authored questions containing math are saved with the MathML namespace stripped, which the server's own XSD check then rejects; and ResourcePanel's incomplete count still takes the permissive free-response default (re-raised from the prior round).
Also still open, not re-raised inline: getDescriptorForQuestionType (interactions/descriptors.js:48) has no callers — useInteractionDescriptor.js:36-40 still inlines the same lookup.
CI: Python unit tests still running, everything else green. Manual QA did not run this round, so nothing here is visually verified.
Prior-finding status
RESOLVED — contentcuration/contentcuration/viewsets/assessmentitem.py:379 — one bad row 500s the whole contentnode__in list
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/index.vue — isIncomplete read @update:errors rather than the item's XML
RESOLVED — contentcuration/contentcuration/viewsets/assessmentitem.py:360 — conversion hardcoded "en" instead of the node language
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/index.vue — late rawData flush on close, untested
RESOLVED — contentcuration/contentcuration/frontend/channelEdit/vuex/assessmentItem/getters.js:45 — uncached DOMParser per item per call
RESOLVED — contentcuration/contentcuration/frontend/channelEdit/components/AssessmentTab/AssessmentTab.vue:6 — banner needs role="status"
RESOLVED — contentcuration/contentcuration/utils/assessment/markdown.py:53 — gate the src rewrite on the checksum filename regex
RESOLVED — contentcuration/contentcuration/frontend/channelEdit/components/edit/EditModal.vue — difxfTracker → diffTracker
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/descriptors.js:1 — praise, .vue-free descriptors
UNADDRESSED — contentcuration/contentcuration/frontend/channelEdit/components/ResourcePanel.vue:621 — invalidQuestionCount takes the permissive allowFreeResponse default
UNADDRESSED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/descriptors.js:48 — getDescriptorForQuestionType has no callers
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| export function parseXML(xmlString, mimeType = 'text/xml') { | ||
| let input = xmlString; | ||
| if (mimeType === 'text/xml') { | ||
| input = xmlString.replace(/ xmlns="[^"]*"/, ''); |
There was a problem hiding this comment.
blocking: This strip is non-global, so it removes the first xmlns="…" in the string — not necessarily the root's. assembleItemXml re-parses the serialized bodyXml through here (assembleItem.js:137); the interaction root is namespace-less, so the first match is the MathML declaration on a nested <math>.
Verified against this branch:
const prompt = buildXmlNode({ tag: 'qti-prompt',
innerHTML: '<p>What is <math xmlns="http://www.w3.org/1998/Math/MathML"><mi>x</mi></math>?</p>' });
const interaction = buildXmlNode({ tag: 'qti-choice-interaction',
attrs: { 'response-identifier': 'RESPONSE' }, children: [prompt] });
assembleItemXml({ identifier: 'i1', title: 't', language: 'en',
bodyXml: new XMLSerializer().serializeToString(interaction), responseDeclarations: [] });
// → …<qti-prompt><p>What is <math><mi>x</mi></math>?</p></qti-prompt>…Feeding that to the backend check the sync endpoint runs:
validate_qti_item(...).is_valid # False
Element '{…imsqtiasi_v3p0}math': This element is not expected.
A parseItem → assembleItemXml round trip loses it the same way, so editing any converted Perseus question that contained $$…$$ — exactly the questions convert_legacy_question_to_qti now renders to MathML and this editor now owns — produces a raw_data the server rejects.
The regex predates the branch, but the branch is what routes MathML through it and what makes this the production save path. Narrow fix: drop only the document element's xmlns (anchor the regex, or set the QTI namespace on the parse instead of deleting it textually). assembleItem.spec.js:168 already pins foreign-namespace preservation inside adoptHtmlNode — extend it one layer up to assembleItemXml.
| return this.getAssessmentItems(this.nodeId); | ||
| }, | ||
| // Free-response questions cannot be scored, so they only count as complete on a survey. | ||
| allowFreeResponse() { |
There was a problem hiding this comment.
blocking: (re-raised) This computed reaches the preview cards (:allowFreeResponse, line 148) but not invalidQuestionCount at line 723, which still calls getAssessmentItemErrors(ai) with no options and so takes the permissive default (shared/utils/validation.js:474). On a non-survey exercise with a free-response question the card shows "Incomplete" while the panel banner stays silent — the divergence this PR sets out to remove.
AssessmentTab avoids it by counting through assessmentItem/getInvalidAssessmentItemsCount, which resolves the modality itself (vuex/assessmentItem/getters.js:33-40). Mapping that getter here drops both the divergence and the duplicated derivation:
...mapGetters('assessmentItem', ['getAssessmentItems', 'getInvalidAssessmentItemsCount']),
...
invalidQuestionCount() {
return this.isExercise && this.getInvalidAssessmentItemsCount({ contentNodeId: this.nodeId });
},Two things compound it now. The errorsByAssessmentItem WeakMap holds one entry per item keyed on allowFreeResponse, so the two callers disagreeing also means every render re-parses instead of hitting the cache. And __tests__/validateItem.spec.js has no allowFreeResponse: false case, so nothing pins FREE_RESPONSE_NOT_ALLOWED at the level the disagreement lives.
| await composable.applyUpdate([item('a', 0), added]); | ||
|
|
||
| const [, payload] = dispatched[0]; | ||
| expect(Object.getOwnPropertySymbols(payload)).toEqual([]); |
There was a problem hiding this comment.
suggestion: This assertion cannot fail — it checks for the absence of a symbol the branch deleted (DELAYED_VALIDATION is gone from shared/constants.js), and no code path could put one on the payload. The name promises "counts as incomplete at once", which it doesn't check: getInvalidAssessmentItemsCount is stubbed to 0 in setup. Either assert the real behaviour (a stored item with blank raw_data counts toward invalidItemsCount) or drop the case — the surrounding add/reorder/delete tests already cover the dispatch shape.
| * opens the question. | ||
| */ | ||
| describe('interaction registry', () => { | ||
| it('registers an editor for every descriptor', () => { |
There was a problem hiding this comment.
praise: The two-list split buys a smaller bundle at the cost of a registration that can be done by halves — asserting both directions turns that into a test failure at the point of the omission rather than a blank editor later.
2 participants
Stacked on #6073
The first 5 commits of this branch are #6073 (Return QTI for still-legacy assessment items on read), which this work is built on — the editor can only be wired in once every item arrives as QTI. They are the same commits, rebased onto current
unstable, so #6073 should land first and this diff will shrink to the 19 commits above it. Reviewing fromrefactor: build the ungraded body in the choice converteronwards skips the duplicated part.What this does
Replaces
AssessmentEditorwithshared/views/QTIEditorin the exercise edit modal, and unties everything that still pointed at the old editor — validation, previews, constants, store shapes.Sync adaptation
QTIEditoremits the whole item list; the sync layer needs per-item changes.channelEdit/composables/useAssessmentItems.jsdiffs the emitted list against the store and dispatches the individual writes (reorders → adds → updates → deletes), so no component below it has to know changes are per-item.QTI serialization fixes
Each of these blocked saving in practice and was found by running the app, not by tests:
buildXmlNodenow re-creates them namespace-less.<qti-correct-response/>/<qti-default-value/>are omitted rather than emitted, since the schema requires at least oneqti-value.AssessmentItemTypes.QTIwas'qti'where the backend sends'QTI'.Interaction registry
An interaction is now registered in two places, each obvious from its imports:
descriptors.jsimportsDescriptor.jsfiles only,index.jsimports theEditor.vuefiles. That keeps TipTap out of the bundle of anything that only parses or validates QTI — which now includesshared/utils/validation.js, imported on every webpack entry. Descriptors extend anInteractionDescriptorbase class that checks the contract at construction; a parity test asserts the two lists agree.Validation
validateQtiItemvalidates an item from its raw XML without rendering anything, so Studio can decide whether a node is complete headlessly. The assessment-item getters read it instead of the legacy answer/hint shapes, and Studio's own delayed-validation machinery (DELAYED_VALIDATION,ignoreDelayed) is gone — whether errors are shown yet is the editor's business, and gating it outside made the tab icon and the incomplete-questions banner disagree with the card they described.Backend
An item is migrated to
QTIon its firstraw_datawrite. The client cannot sendtype— the change layer only sends fields that changed — so the type has to be inferred from the payload, otherwise every edit to a still-legacy row is rejected.Images from legacy questions
Perseus images carry a size suffix (
) that CommonMark has no notion of, so the converter emitted them as literal text. A markdown rule now claims the construct, andTipTapEditor's html path resolves a stored<checksum>.<ext>to a loadable URL on the way in and back on the way out.Ordering interaction (#6089)
Adapted to the conventions above, folded into the commits that introduce them: renamed to
Descriptor.js/Editor.vue, extends the base class, registered in both lists, and its editor test no longer drives the removed debounce. A follow-up commit gives every interaction the samevalidation.jsmodule name.Known follow-ups, not in this PR
align=) is dropped on conversion — QTI'sImgmodel has no attribute to carry it, and the reverse conversion does not emit one either. Needs a decision on widening the model._write_qti_media_filesneeds aFilerow linked to the assessment item, and nothing links exercise images (0 of 2,461Filerows in a dev DB haveassessment_itemset — this predates the new editor). Legacy items were fine because publish read them out of storage by checksum. Migrating an item moves it onto the stricter path.convert_legacy_question_to_qtiraises on math directly inside an<li>, whichFlowContentElementdoes not accept; the fix is to wrap it in a<p>.