Replace the legacy assessment editor with the QTI editor - #6095
Replace the legacy assessment editor with the QTI editor#6095AlexVelezLl wants to merge 12 commits into
Conversation
🟡 Waiting for changesLast updated: 2026-08-21 15:58 UTC |
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
| * 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 }).
|
|
||
| // 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.
| # 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.
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
59b139d to
b3b1c9b
Compare
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
b3b1c9b to
d97f062
Compare
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.
✅ Resolved — addressed in the current code.
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.
| await composable.applyUpdate([item('a', 0), added]); | ||
|
|
||
| const [, payload] = dispatched[0]; | ||
| expect(Object.getOwnPropertySymbols(payload)).toEqual([]); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
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.
d97f062 to
15a229e
Compare
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6095 — 13 of 14 prior findings resolved; 1 open.
New blocking inline on convert.py.
QTIEditor/components/InteractionSection/index.vue:62 — suggestion: onUpdateQuestionType drops authored choices, no undo; the deleted AssessmentEditor confirmed first.
CI pending; no manual QA.
Prior-finding status
RESOLVED — contentcuration/contentcuration/viewsets/assessmentitem.py:379 — contentnode__in
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/index.vue — isIncomplete / @update:errors
RESOLVED — contentcuration/contentcuration/viewsets/assessmentitem.py:360 — language="en"
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/index.vue — rawData
RESOLVED — contentcuration/contentcuration/frontend/channelEdit/vuex/assessmentItem/getters.js:45 — DOMParser
RESOLVED — contentcuration/contentcuration/frontend/channelEdit/components/AssessmentTab/AssessmentTab.vue:6 — role="status"
RESOLVED — contentcuration/contentcuration/utils/assessment/markdown.py:53 — QTI_CHECKSUM_FILENAME_REGEX
RESOLVED — contentcuration/contentcuration/frontend/channelEdit/components/edit/EditModal.vue — difxfTracker
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/descriptors.js:1 — .vue-free
RESOLVED — contentcuration/contentcuration/frontend/channelEdit/components/ResourcePanel.vue:620 — allowFreeResponse
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/descriptors.js — descriptors unused
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/serialization/xml.js — xmlns
ACKNOWLEDGED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/__tests__/registry.spec.js:13 — registry
UNADDRESSED — contentcuration/contentcuration/frontend/channelEdit/composables/__tests__/useAssessmentItems.spec.js:124 — getOwnPropertySymbols
@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
| # 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 |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: Div body, no interaction (single_selection_no_answers.xml), and QTIItemEditor mounts InteractionSection only when interactions.length > 0 — the card shows questionContentPlaceholder$, hides item.question, no type selector, incomplete (NO_INTERACTION) until deleted. Emit createBlankItem.js's shape: one empty qti-simple-choice, prompt from item.question, no qti-correct-response — editable, ungraded, and BlankEditorItemTests-valid.
| await composable.applyUpdate([item('a', 0), added]); | ||
|
|
||
| const [, payload] = dispatched[0]; | ||
| expect(Object.getOwnPropertySymbols(payload)).toEqual([]); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: (re-raised) Cannot fail — nothing attaches a symbol.
rtibblesbot
left a comment
There was a problem hiding this comment.
No new commits since my last review (15a229e).
@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
15a229e to
483a441
Compare
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6095 — 15 of 18 prior findings resolved or acknowledged; 3 still open (see below).
The hints work is well-shaped and well-tested, but it makes assembleItemXml the save path for converted questions, and that rebuild is narrower than what the converter emits — see the two new blocking comments.
CI green apart from the browser smoke test, which was cancelled rather than failed. Manual QA did not run — no UI was verified.
Still open:
- blocking
convert.py:156— answerless question converts to an unrepairable item (re-raised, no change or reply). - suggestion
QTIItemEditor/index.vue:201— dev-gated debugconsole.log. - suggestion
InteractionSection/index.vue:62—onUpdateQuestionTypediscards authored choices with no confirm. Not anchorable to this diff.
New:
- blocking
assembleItem.js:197— save resets a converted item's language and drops its outcome declaration and response processing. - blocking
hints.js:74— an image-only hint is silently dropped on save. - suggestion
QTIItemEditor/index.vue:220—hasHintsremoves a capability the legacy editor had.
Prior-finding status
RESOLVED — contentcuration/contentcuration/viewsets/assessmentitem.py:379 — contentnode__in per-item failure
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/index.vue — isIncomplete / @update:errors
RESOLVED — contentcuration/contentcuration/viewsets/assessmentitem.py:360 — hardcoded language="en"
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/index.vue — late rawData flush on close
RESOLVED — contentcuration/contentcuration/frontend/channelEdit/vuex/assessmentItem/getters.js:45 — DOMParser memoisation
RESOLVED — contentcuration/contentcuration/frontend/channelEdit/components/AssessmentTab/AssessmentTab.vue:6 — role="status"
RESOLVED — contentcuration/contentcuration/utils/assessment/markdown.py:53 — QTI_CHECKSUM_FILENAME_REGEX
RESOLVED — contentcuration/contentcuration/frontend/channelEdit/components/edit/EditModal.vue — difxfTracker
RESOLVED — contentcuration/contentcuration/frontend/channelEdit/components/ResourcePanel.vue:620 — allowFreeResponse vs invalidQuestionCount
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/descriptors.js — descriptors had no callers
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/serialization/xml.js — non-global xmlns strip
RESOLVED — contentcuration/contentcuration/frontend/channelEdit/composables/tests/useAssessmentItems.spec.js:124 — assertion that cannot fail
ACKNOWLEDGED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/descriptors.js:1 — .vue-free descriptors (praise)
ACKNOWLEDGED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/tests/registry.spec.js:13 — two-list registration parity (praise)
ACKNOWLEDGED — contentcuration/contentcuration/tests/utils/qti/test_validation.py:174 — createBlankItem.js pinned against the XSD (praise)
UNADDRESSED — contentcuration/contentcuration/utils/assessment/qti/convert.py:156 — answerless legacy question
UNADDRESSED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/InteractionSection/index.vue:62 — question-type switch drops choices
UNADDRESSED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/index.vue:201 — debug console.log
@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
| title: title || '', | ||
| adaptive: 'false', | ||
| 'time-dependent': 'false', | ||
| 'xml:lang': language || 'en', |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: A converted item carries its language as language="en-US" on <qti-assessment-item> (qti/base.py; every fixture under tests/utils/qti/fixtures/), not xml:lang. parseItem.js:37 reads only xml:lang, so language arrives '' and this line retags a Spanish exercise as en on the first edit — including a hint-only edit. Publish ships raw_data verbatim for QTI (qti/archive.py:87-116), so nothing re-applies the node language afterwards.
Line 200 loses more. convert.py:338-355 emits <qti-outcome-declaration identifier="SCORE"> and <qti-response-processing template=".../match_correct"/>; neither survives the rebuild, so an item Studio converted as gradable publishes ungradable. createBlankItem.js does not emit them either, so this is the shape of every item the new editor produces.
Accept both spellings on read (getAttribute('xml:lang') ?? getAttribute('language')) and drop the 'en' default; carry the declarations through parseItem/assembleItemXml as passthrough, or emit them from createBlankItem. Nothing catches this because testingFixtures.js is hand-written with xml:lang and no declarations — a single_selection.xml → parseItem → assembleItemXml round-trip test would pin both.
| * @returns {boolean} | ||
| */ | ||
| export function hintHasContent(hint) { | ||
| const text = (hint.content || '').replace(/<[^>]*>/g, '').replace(/ /g, ' '); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: Stripping tags makes an image-only hint count as empty — <p><img src="abc123.png"/></p> returns false — so assembleItem.js:123 filters it out and the hint is lost on the next save. Reachable both ways: markdown.py's new sized_image_func turns  in a Perseus hint into an <img>, and TipTap's image button creates one directly. Meanwhile the card renders as the grey placeholder, since HintsSection/index.vue:62 gates on the same predicate.
utils/stripTags.js already does this strip — reuse it, and treat embedded media as content: stripTags(content).replace(/ /g, ' ').trim() || /<(img|math|svg)\b/i.test(content). hints.spec.js:48-57 is the table to extend.
Same root cause, outside this delta and not raised separately: choice/validation.js:15 and :25 reject an image-only prompt or choice.
| # 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 |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking (re-raised, unaddressed): The Div body carries no interaction, so QTIItemEditor skips InteractionSection (index.vue:50) and renders questionContentPlaceholder$(). The question text in single_selection_no_answers.xml is never shown, there is no type selector, and validateItemShape returns NO_INTERACTION permanently — the card reports itself incomplete with no control an author can use to fix it, short of deleting the question.
Emitting createBlankItem.js's shape instead — one empty qti-simple-choice, the prompt from item.question, no qti-correct-response — gives an editable, ungraded item that BlankEditorItemTests already proves schema-valid.
483a441 to
4ec8efb
Compare
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6095 — 19 of 20 prior findings resolved; 1 open, 2 new. CI pending; no QA.
blocking — QTIItemEditor/index.vue:140 — a hint edit wipes the question bodyblocking — convert.py:156 — re-raised: answerless conversion still uneditablesuggestion — assembleItem.js:230 — natively authored items publish with no language
Prior-finding status
RESOLVED — viewsets/assessmentitem.py:379 — one bad row 500s the whole list
RESOLVED — QTIItemEditor/index.vue — isIncomplete and the banner disagree
RESOLVED — viewsets/assessmentitem.py:360 — hardcoded "en"
RESOLVED — QTIItemEditor/index.vue — late rawData flush on close, untested
RESOLVED — vuex/assessmentItem/getters.js:45 — DOMParser per item per call
RESOLVED — AssessmentTab/AssessmentTab.vue:6 — missing role="status"
RESOLVED — utils/assessment/markdown.py:53 — remote src becomes a local filename
RESOLVED — edit/EditModal.vue — difxfTracker typo
RESOLVED — interactions/descriptors.js:1 — praise: .vue-free descriptors
RESOLVED — ResourcePanel.vue:620 — invalidQuestionCount permissive default
RESOLVED — interactions/descriptors.js — resolveDescriptor has no callers
RESOLVED — serialization/xml.js — non-global xmlns strip hits nested MathML
RESOLVED — __tests__/useAssessmentItems.spec.js — assertion cannot fail
RESOLVED — interactions/__tests__/registry.spec.js:13 — praise: registry assertion
RESOLVED — tests/utils/qti/test_validation.py:178 — praise: createBlankItem vs XSD
RESOLVED — serialization/assembleItem.js:197 — language attribute, grading declarations
RESOLVED — serialization/hints.js:74 — image-only hint counts as empty
RESOLVED — QTIItemEditor/index.vue:201 — leftover debug logging
ACKNOWLEDGED — QTIItemEditor/index.vue:229 — hints only on converted questions: product decision
UNADDRESSED — utils/assessment/qti/convert.py:156 — answerless choice question uneditable
@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
| # 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 |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking (unaddressed): No interaction in the Div body: the card reports NO_INTERACTION forever, and feeds the wipe at QTIItemEditor/index.vue:140. Emit createBlankItem.js's shape — one empty qti-simple-choice, no qti-correct-response.
| 'xml:lang': language || 'en', | ||
| // Omitted rather than guessed when the item has no language: the schema allows an | ||
| // item without one, and inventing 'en' would relabel a converted Spanish question. | ||
| [languageAttribute]: language || null, |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Reverses my earlier request to drop 'en': right for converted items, wrong for new. createBlankItem.js:31 passes language: '', _create_native_qti_item ships raw_data verbatim, so they publish languageless. Stamp _node_language() there.
4ec8efb to
f330fcf
Compare
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6095 — delta review of 4ec8efbc4..f330fcf2c. 22 of 23 prior findings settled; 1 still open (inline).
Both prior blockers are closed: the answerless-body wipe is fixed and pinned by a fixture-driven test, and the answerless conversion itself is a maintainer-confirmed deferral to #6073. CI passing. Manual QA did not run — no UI verification behind this review.
- suggestion —
createBlankItem.js:32: a natively authored item publishes with noxml:langwhile every other generator path stamps_node_language().
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 and the banner disagree
RESOLVED — contentcuration/contentcuration/viewsets/assessmentitem.py:360 — conversion hardcodes "en"
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/index.vue — late rawData flush on close
RESOLVED — contentcuration/contentcuration/frontend/channelEdit/vuex/assessmentItem/getters.js:45 — uncached DOMParser per item
RESOLVED — contentcuration/contentcuration/frontend/channelEdit/components/AssessmentTab/AssessmentTab.vue:6 — missing role="status"
RESOLVED — contentcuration/contentcuration/utils/assessment/markdown.py:53 — gate remote src on the checksum regex
RESOLVED — contentcuration/contentcuration/frontend/channelEdit/components/edit/EditModal.vue — difxfTracker typo
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/descriptors.js:1 — .vue-free descriptors (praise)
RESOLVED — contentcuration/contentcuration/frontend/channelEdit/components/ResourcePanel.vue:620 — invalidQuestionCount took the permissive default
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/descriptors.js — resolveDescriptor had no callers
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/serialization/xml.js — non-global xmlns strip dropped MathML
RESOLVED — contentcuration/contentcuration/frontend/channelEdit/composables/tests/useAssessmentItems.spec.js — assertion cannot fail
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/tests/registry.spec.js:13 — two-list agreement (praise)
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/index.vue — leftover debug logging
RESOLVED — contentcuration/contentcuration/tests/utils/qti/test_validation.py:178 — pins createBlankItem output (praise)
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/serialization/assembleItem.js — language attribute and lost declarations
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/serialization/hints.js — image-only hint counted as empty
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/index.vue:152 — hint edit wrote an empty body over the question
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/serialization/tests/convertedItem.spec.js:17 — converter's own fixtures (praise)
ACKNOWLEDGED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/index.vue:231 — hints only on questions that arrived with them (product decision)
ACKNOWLEDGED — contentcuration/contentcuration/utils/assessment/qti/convert.py:156 — answerless conversion emits no interaction (deferred to #6073)
UNADDRESSED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/serialization/createBlankItem.js:32 — natively authored items publish without a language
@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
| identifier, | ||
| title, | ||
| language, | ||
| itemBodyXml: body ? serializer.serializeToString(body) : '', |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: Returning the body whether or not an interaction was found is what made the empty-body fix a one-liner rather than a special case.
f330fcf to
2f42340
Compare
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6095 — 23 of 23 prior findings resolved or acknowledged; none still open. Not approving only because CI is still pending and manual QA did not run.
Prior-finding status
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/serialization/createBlankItem.js:32 — natively authored items publish without a language
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 and the banner disagree
RESOLVED — contentcuration/contentcuration/viewsets/assessmentitem.py:360 — conversion hardcodes "en"
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/index.vue — late rawData flush on close
RESOLVED — contentcuration/contentcuration/frontend/channelEdit/vuex/assessmentItem/getters.js:45 — uncached DOMParser per item
RESOLVED — contentcuration/contentcuration/frontend/channelEdit/components/AssessmentTab/AssessmentTab.vue:6 — missing role="status"
RESOLVED — contentcuration/contentcuration/utils/assessment/markdown.py:53 — gate remote src on the checksum regex
RESOLVED — contentcuration/contentcuration/frontend/channelEdit/components/edit/EditModal.vue — difxfTracker typo
RESOLVED — contentcuration/contentcuration/frontend/channelEdit/components/ResourcePanel.vue:620 — invalidQuestionCount took the permissive default
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/descriptors.js — resolveDescriptor had no callers
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/serialization/xml.js — non-global xmlns strip dropped MathML
RESOLVED — contentcuration/contentcuration/frontend/channelEdit/composables/tests/useAssessmentItems.spec.js — assertion cannot fail
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/index.vue:201 — leftover debug logging
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/serialization/assembleItem.js — language attribute and lost declarations
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/serialization/hints.js — image-only hint counted as empty
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/index.vue:152 — hint edit wrote an empty body over the question
ACKNOWLEDGED — contentcuration/contentcuration/utils/assessment/qti/convert.py:156 — answerless conversion emits no interaction (deferred to #6073)
ACKNOWLEDGED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/index.vue:231 — hints only on questions that arrived with them (product decision)
ACKNOWLEDGED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/descriptors.js:1 — .vue-free descriptors (praise)
ACKNOWLEDGED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/tests/registry.spec.js:13 — two-list agreement (praise)
ACKNOWLEDGED — contentcuration/contentcuration/tests/utils/qti/test_validation.py:178 — pins createBlankItem output (praise)
ACKNOWLEDGED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/serialization/tests/convertedItem.spec.js:17 — converter's own fixtures (praise)
@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
2f42340 to
df0fbef
Compare
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6095 — all 23 prior findings resolved or acknowledged; no new findings.
CI green. Not approving: manual QA was required for this PR but did not run, so the UI is unverified.
Prior-finding status
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/serialization/createBlankItem.js:32 — natively authored items publish without a language
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 and the banner disagree
RESOLVED — contentcuration/contentcuration/viewsets/assessmentitem.py:360 — conversion hardcodes "en"
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/index.vue — late rawData flush on close
RESOLVED — contentcuration/contentcuration/frontend/channelEdit/vuex/assessmentItem/getters.js:45 — uncached DOMParser per item
RESOLVED — contentcuration/contentcuration/frontend/channelEdit/components/AssessmentTab/AssessmentTab.vue:6 — missing role="status"
RESOLVED — contentcuration/contentcuration/utils/assessment/markdown.py:53 — gate remote src on the checksum regex
RESOLVED — contentcuration/contentcuration/frontend/channelEdit/components/edit/EditModal.vue — difxfTracker typo
RESOLVED — contentcuration/contentcuration/frontend/channelEdit/components/ResourcePanel.vue:620 — invalidQuestionCount took the permissive default
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/descriptors.js — resolveDescriptor had no callers
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/serialization/xml.js — non-global xmlns strip dropped MathML
RESOLVED — contentcuration/contentcuration/frontend/channelEdit/composables/tests/useAssessmentItems.spec.js — assertion cannot fail
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/index.vue:210 — leftover debug logging
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/serialization/assembleItem.js — language attribute and lost declarations
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/serialization/hints.js — image-only hint counted as empty
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/index.vue:151 — hint edit wrote an empty body over the question
ACKNOWLEDGED — contentcuration/contentcuration/utils/assessment/qti/convert.py:156 — answerless conversion emits no interaction (deferred to #6073)
ACKNOWLEDGED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/index.vue:231 — hints only on questions that arrived with them (product decision)
ACKNOWLEDGED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/descriptors.js:1 — .vue-free descriptors (praise)
ACKNOWLEDGED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/tests/registry.spec.js:13 — two-list agreement (praise)
ACKNOWLEDGED — contentcuration/contentcuration/tests/utils/qti/test_validation.py:178 — pins createBlankItem output (praise)
ACKNOWLEDGED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/serialization/tests/convertedItem.spec.js:17 — converter's own fixtures (praise)
@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
df0fbef to
64a6a2b
Compare
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6095 — 24 prior findings settled, none open. CI pending; manual QA did not run.
Prior-finding status
RESOLVED — createBlankItem.js:32 — language on native items
RESOLVED — viewsets/assessmentitem.py:379 — bulk fetch 500s
RESOLVED — QTIItemEditor/index.vue — isIncomplete vs banner
RESOLVED — viewsets/assessmentitem.py:360 — hardcoded en
RESOLVED — QTIItemEditor/index.vue — late rawData flush
RESOLVED — assessmentItem/getters.js:45 — uncached DOMParser
RESOLVED — AssessmentTab.vue:6 — role=status
RESOLVED — markdown.py:53 — checksum-gated src
RESOLVED — EditModal.vue — difxfTracker typo
RESOLVED — ResourcePanel.vue:620 — invalidQuestionCount default
RESOLVED — interactions/descriptors.js — resolveDescriptor unused
RESOLVED — serialization/xml.js — non-global xmlns strip
RESOLVED — useAssessmentItems.spec.js — assertion cannot fail
RESOLVED — QTIItemEditor/index.vue:210 — debug logging
RESOLVED — serialization/assembleItem.js — language and declarations
RESOLVED — serialization/hints.js — image-only hint
RESOLVED — QTIItemEditor/index.vue:151 — empty body on hint edit
RESOLVED — qti/convert.py:156 — answerless conversion
ACKNOWLEDGED — QTIItemEditor/index.vue:231 — hints scope, product decision
ACKNOWLEDGED — interactions/descriptors.js:1 — praise
ACKNOWLEDGED — interactions/__tests__/registry.spec.js:13 — praise
ACKNOWLEDGED — tests/utils/qti/test_validation.py:178 — praise
ACKNOWLEDGED — serialization/__tests__/convertedItem.spec.js:17 — praise
ACKNOWLEDGED — serialization/parseItem.js:73 — praise
@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
| // Image srcs are resolved for display on the way in, so they are reduced | ||
| // back to their stored form here — leaving this the one place that reads | ||
| // content out, whichever form the editor happens to be holding. | ||
| if (props.format === 'html') return toStoredImageSrcs(editor.value.getHTML()); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: Underline and Strikethrough make a question unsaveable.
EditorToolbar mounts unfiltered (line 19), so useToolbarActions.js:364,371 give every QTI field <u> and <s>. Nothing strips them before raw_data; AssessmentItemSerializer.validate (viewsets/assessmentitem.py:166) rejects both against imsqti_itemv3p0p1_v1p0.xsd:
FAIL <p>a <u>b</u></p> Element '…}u': This element is not expected.
FAIL <p>a <s>b</s></p> Element '…}s': This element is not expected.
_strip_unsupported_markup (convert.py:89) has the rule. Filter both from formatActions when embedded here, or unwrap alongside toStoredImageSrcs.
| """ | ||
| Unwrap every tag a QTI item body cannot carry, keeping its content. | ||
|
|
||
| An anchor has nothing to navigate to on a device with no internet access. |
There was a problem hiding this comment.
suggestion: Stripping a drops links the editor still offers — useToolbarActions.js:426 registers Insert link. Drop that button too?
| self.assertNotIn("<s>", result.xml) | ||
| self.assertTrue(validate_qti_item(result.xml.encode("utf-8")).is_valid) | ||
|
|
||
| def test_raw_html_marks_are_stripped(self): |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: Per-tag validate_qti_item(...).is_valid, not just tag absence.
Inline maths: li, td, th, caption, dt, dd and qti-simple-choice took no math, though the XSD admits m3:math in all seven. Links and inline marks (a, s, del, ins, u, mark, strike): unwrapped in the rendered markup, keeping their text. An anchor has nothing to navigate to on a device with no internet access, and the QTI 3.0 HTML profile has no element for the marks. Publish and ricecooker upload reach the same converter, so both stop failing on these items too.
…ction Every newly added question is one: the editor writes type single_selection with no answers, and the XSD requires at least one qti-simple-choice, so conversion raised. Stand in the single empty choice the QTI editor opens a new choice interaction with.
consolidate() replaces each legacy row's type and raw_data with the converter's output, so the client only ever sees QTI. QTI and perseus_question rows pass through. Items are tagged with the content node's language, falling back to the channel's, matching what publish writes. An item that cannot be converted is logged and left as the legacy row it is, so one bad row does not cost the whole contentnode__in list; the editor renders it as unsupported. Goes away with the global backfill (learningequality#6007).
Three ways a question this editor produced was refused on its way to the server, none of them visible in the editor itself. 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. Capabilities now return null when they have nothing to serialize, and the declaration drops them instead of emitting an empty element. Authored markup lost the item's namespace. 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 schema expects inline content in the namespace the item root declares, so any question whose prompt or answers carried markup was rejected. 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. Keeping theirs also means not deleting them on the way in. parseXML removed an xmlns textually, and the pattern was not anchored, so on a body whose own root carries no declaration — every interaction this editor serializes — the first match was the MathML namespace on a nested <math>. assembleItemXml re-parses the body through there, so a question containing $$…$$ assembled to a <math> inheriting the QTI namespace, which the schema rejects. Nothing needed the declarations gone: element lookups here are by local name, and a type selector with no namespace prefix matches in any namespace. 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. The language went the same way. QTI declares `xml:lang` for it; the legacy conversion writes a `language` attribute the schema does not define, which the XSD tolerates only through a lax extension wildcard (learningequality#6098). parseItem read `xml:lang` alone, so a converted item's language arrived empty and was written back as a fabricated 'en' — relabelling a Spanish exercise on its first edit, including a hint-only one, with nothing downstream to put it right, since publishing ships raw_data verbatim for QTI. It now reads either attribute and always writes `xml:lang`; the fallback goes when the conversion is fixed. An item with no language at all carries none, rather than a guess. Which leaves a new item with no language, since the editor has none of its own to offer, and publishing shipped raw_data verbatim for a native QTI item while stamping the node's language on every other kind — so one package would declare a language for some items and not others, decided by which editor wrote them. Publishing now stamps it here too, which keeps the node the single source rather than freezing whatever the browser knew when the question was written. Nothing carried the grading rules through either. The conversion emits a SCORE outcome declaration, and a match_correct response processing template for an item that has something to answer, so an item Studio converted as gradable came back ungradable. Both are now written on assembly rather than preserved: an edit can invalidate whatever a previous tool recorded, and match_correct is the one template this editor knows how to keep true. Last, the stored type. It is 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. The modules that were already there join that convention: three interactions had three spellings of the same pair, so each one now has validation.js beside its parse.js, with a spec named after the module it covers. 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>
Two things Studio needs from an item it is not showing. 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. The other is a new question. It 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>
A question card gave no sign of what was wrong with the question inside it, or that this editor could not edit it at all — both of which the exercise editor it replaces did show. An incomplete question is now marked in the card header. Rather than validate the item a second time, each interaction editor reports the errors useInteraction already computes for the inline messages. What is wrong with the item itself an interaction editor structurally cannot report — an item with nothing to answer mounts no editor, and whether a free response is acceptable is the consumer's policy — so the card asks the item for that part, through a helper the headless validator shares, from what it has already parsed. A question this editor cannot read renders as read-only instead. 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. Reporting one as incomplete would ask the author to fix something they cannot reach, so it does not. A question with no correct answer is only acceptable where questions are not scored, so the type selector stops offering it on an exercise rather than letting an author choose it and then marking the question incomplete for it. A question that already is a free response keeps the option: removing it would leave the select showing some other type as though that were the question, and one nudge of the control would convert it. That one says underneath why it cannot stay. Finally, only the card being edited reports its XML. Every card re-assembles it 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. Whether the change came from an edit here is recorded as it happens rather than read when the watcher flushes: closing a card sets the parent's active item to none, and that re-render lands first, so a change made just before the close would otherwise look like it came from a card nobody was editing. 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>
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 test asserts the dispatched payload whole, so a marker asking for validation to be delayed cannot creep back in unnoticed — toEqual compares symbol-keyed properties too. The vuex actions stop stringifying answers and hints — the API rejects those fields on a QTI item, whose content lives in raw_data. 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. The store stops reshaping what the API no longer sends. The mutation parsed and sorted the answers and hints that used to arrive as JSON strings; nothing reads them, 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. Validation follows the same move. getAssessmentItemErrors judged every question by empty legacy fields, so it 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 caller 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. Whether a question's errors are shown yet is the editor's business now — the item always has them, and the card decides when they surface — so Studio's delayed validation goes too. Answering "is this question complete" with "unless it was created recently" made the tab icon and the incomplete-questions banner disagree with the card they describe. 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>
Every legacy item is served to the client as QTI, but two things about that round trip were wrong. 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. The other is images. A legacy question stores them 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>
64a6a2b to
40222d1
Compare
QTI has no hint element, so the conversion carries a legacy question's hints in the item's <qti-catalog-info> — dormant content the delivery engine never renders on its own — as cards tagged with a Kolibri support value. Studio already writes them there on read and reads them back out on publish; the editor in between did neither, so opening a converted question and saving it dropped every hint it had. Hints belong to the item rather than to any one interaction, so they come out of parseItem beside `interactions` rather than inside them, and useQtiItem holds them next to identifier and title — a ref the card mutates and the rawData computed reads. Cards are matched on their support value rather than the catalog they sit in, the same way the publish-side derivation does: a catalog id is a name, the support value is the contract. assembleItemXml writes the catalog after the item body, which is where the schema wants it — the one mistake this shape can make and still look right. A hint with nothing written in it is left out, and an item whose hints are all empty carries no catalog at all rather than an empty one, since a catalog has to hold at least one card. Two backend tests pin the result: the document this editor emits is schema-valid, and moving the catalog ahead of the body is not; and derive_perseus_item still recovers the hints from it, which is what makes editing a converted question safe to publish. Only a question that arrived with hints offers the section. That is a scope limit, not a technical one: assembleItemXml writes the catalog for whatever hints it is handed, and publishing derives legacy hints from the cards' support value without asking where the item came from, so hints authored here would survive too — except on the shapes Perseus cannot express, ordering and free response, where the QTI package would keep them and the derived Perseus item would not. Whether to offer them everywhere waits on that. The gate is read once from the parsed item rather than from the live list, so removing the last hint does not take the section away while the author is still working in it. A hint is the only thing an author can change on a question with nothing to answer, and that turned out to be enough to lose the question. Such an item has a body and no interaction, so no interaction editor mounts and the card held no body of its own; reassembling from that wrote an empty <qti-item-body/> over the question's text. The body is read from the item now and seeded whether or not an interaction was found. HintsSection keeps the shape and styling of the HintsEditor it replaces: bordered cards that open one at a time into a rich-text editor, and the same move/delete actions and dashed add button, now drawn with the components the interaction editors already share. Its header is labelled like the "Question" and "Answers" fields beside it and tints on hover the way a flat button does, so it reads as one of the editor's own sections rather than something bolted underneath. A card is only inviting where clicking it does something: in view mode the reader gets no pointer and no hover tint, because there is nothing to open. Its heading joins the ones already in the card: the field labels the interaction editors put above the question and the answers become headings at the same level, so a question reads as a section with named parts rather than a run of bold text. They set their own margins, since a heading brings the browser stylesheet's. The header is a disclosure, built like the one the community-library modal uses: a button that says whether what it controls is showing, inside a heading, so the hints sit under the question in the document outline. The controlled element stays in the document and is hidden rather than removed, so aria-controls always resolves, while what it holds is mounted only while open — a rich-text editor per hint per question is not worth paying for unseen. Theme values are bound in the style block rather than assembled in JavaScript, so the component needs no instance handle: the focus ring is :focus-visible, which distinguishes keyboard from pointer focus the same way $coreOutline does, without asking the theme plugin. Hints are supplementary to the question, so a closed card keeps them out of the way until the reader asks for answers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
40222d1 to
7dcd09e
Compare
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6095 — all 30 prior findings resolved; 3 new suggestions inline.
The underline/strike schema fix is right and well tested. align and link in the same commit stop at the toolbar; useEditor.js:28-31 already argues why that is not enough.
CI pending. Manual QA did not run.
Prior-finding status
RESOLVED — viewsets/assessmentitem.py — contentnode__in 500
RESOLVED — QTIItemEditor/index.vue — banner vs isIncomplete
RESOLVED — viewsets/assessmentitem.py:362 — "en" hardcoded
RESOLVED — QTIItemEditor/index.vue — rawData flush on close
RESOLVED — assessmentItem/getters.js:45 — DOMParser uncached
RESOLVED — AssessmentTab.vue:6 — role="status"
RESOLVED — markdown.py:53 — remote src
RESOLVED — EditModal.vue — difxfTracker
RESOLVED — interactions/descriptors.js:1 — praise
RESOLVED — ResourcePanel.vue:620 — invalidQuestionCount
RESOLVED — interactions/descriptors.js — no callers
RESOLVED — serialization/xml.js — xmlns strip
RESOLVED — ResourcePanel.vue:620 — invalidQuestionCount (re-raised)
RESOLVED — useAssessmentItems.spec.js — assertion cannot fail
RESOLVED — __tests__/registry.spec.js:13 — praise
RESOLVED — qti/convert.py — Div body, no interaction
RESOLVED — QTIItemEditor/index.vue — debug logging
RESOLVED — useAssessmentItems.spec.js — cannot fail (re-raised)
RESOLVED — test_validation.py:178 — praise
RESOLVED — serialization/assembleItem.js — en-US
RESOLVED — serialization/hints.js — image-only hint
RESOLVED — qti/convert.py — NO_INTERACTION (re-raised)
RESOLVED — QTIItemEditor/index.vue:231 — hints per type
RESOLVED — QTIItemEditor/index.vue — debug logging (re-raised)
RESOLVED — QTIItemEditor/index.vue:152 — empty <qti-item-body/>
RESOLVED — qti/convert.py — Div body (re-raised)
RESOLVED — serialization/assembleItem.js — 'en' default
RESOLVED — __tests__/convertedItem.spec.js:17 — praise
RESOLVED — serialization/createBlankItem.js:32 — 'en' default
RESOLVED — serialization/parseItem.js:73 — praise
@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
| // style attribute — the item schema admits one only through its lax wildcard, so an | ||
| // aligned paragraph saves and then ships as non-conformant QTI. The image extension | ||
| // carries alignment as `data-text-align` instead, which the schema does allow. | ||
| hide: true, |
There was a problem hiding this comment.
suggestion: hide closes the button, not the paste path. TextAlign stays registered for paragraph/heading (useEditor.js:42-44) and transformPastedHTML strips only mso- (pasteTransform.js:9-23), so a pasted <p style="text-align: center"> still saves and round-trips as style="text-align: …" — silently: QTISanitizer checks identifiers/base-types/cardinalities only, and the backend admits style via the lax wildcard.
Same half-fix the commit rejected for the marks (useEditor.js:28-31). Image.js:21-33 carries its own textAlign as data-text-align, so TextAlign.configure({ types: ['image'] }) looks like it keeps image alignment while dropping paragraph/heading alignment from the schema — can you confirm image alignment goes through the node attribute, not setTextAlign? Unchecked: whether TextAlign also adds Mod-Shift-l/e/r/j.
There was a problem hiding this comment.
Could you please open a follow-up issue for this and this?
| // Legacy conversion unwraps anchors, since one has nothing to navigate to on a | ||
| // device with no internet access (utils/assessment/qti/convert.py). Hidden so the | ||
| // editor does not offer what a converted question cannot keep. | ||
| hide: true, |
There was a problem hiding this comment.
suggestion: Mod-k still opens the link editor the toolbar no longer offers — extensions/Link.js:16-21 emits open-link-editor, useLinkHandling.js:188 binds it to openLinkEditor, TipTapEditor.vue:46-60 renders LinkEditor. The mark survives the save, since A is a valid item element (qti/html/text.py:24), so a new question keeps a link a converted one has stripped (convert.py:_strip_unsupported_markup) — the divergence this flag was meant to close. CustomLink also inherits autolink/linkOnPaste; worth confirming those defaults on @tiptap/extension-link@3.
If links are out, drop CustomLink and its bubble-menu/editor wiring, as useEditor.js does for the marks. If they stay for new items, the flag and its comment need revisiting.
| cursor: pointer; | ||
|
|
||
| &:hover { | ||
| background-color: v-bind('$themePalette.grey.v_100'); |
There was a problem hiding this comment.
suggestion: This hover colour never paints. ClickableRegion's absolutely-positioned .overlay-button sets $themeTokens.fineLine on hover across the whole area (ClickableRegion/index.vue:63-80), so fineLine wins and transition: background-color 0.3s no longer applies either. choice/Editor.vue:713-719 declares fineLine on its own is-clickable rule — match that, or drop this rule.
| @@ -0,0 +1,34 @@ | |||
| import { useEditor } from '../TipTapEditor/composables/useEditor'; | |||
There was a problem hiding this comment.
praise: Pins the schema, not the buttons — <u>/<s> in, text out without the tags, so paste and shortcuts are covered too.
Stacked on #6073, you can ignore first 3 commits and review the remaining 9. #6073 must be merged first.
Summary
AssessmentsTabto convert from the list-shape updates to the updates per assessments shape needed for sync.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'.validateQtiItemvalidates an item from its raw XML without rendering anything, so Studio can decide whether a node is complete headlessly.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..mdformat, so that images from the old editor can be seen on the new one.) 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.Grabacion.de.pantalla.2026-08-18.a.la.s.11.22.41.a.m.mov
References
Stacked on #6073.
Closes #5970, #5956, #5934, #5958.
Reviewer guidance
unstable, create several exercises, these will we created using the old data format.Known follow-ups, not in this PR
_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.AI usage
Used Claude (Opus 5) to brainstorm an initial plan and then to implement it. I reviewed the results and iteratively went on building and fixing remaining issues and proper commit history, until we got to a polished version of this PR.