implement choice interaction parsing and serialization for QTIEditor#6012
implement choice interaction parsing and serialization for QTIEditor#6012Abhishek-Punhani wants to merge 5 commits into
Conversation
|
👋 Hi @Abhishek-Punhani, thanks for contributing! For the review process to begin, please verify that the following is satisfied:
Also check that issue requirements are satisfied & you ran Pull requests that don't follow the guidelines will be closed. Reviewer assignment can take up to 2 weeks. |
|
📢✨ Before we assign a reviewer, we'll turn on |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Synthesized review combining core-logic and frontend-pattern passes. Solid round-trip serialization tests and clean composable design overall; two blocking issues need fixing before merge, plus a few design/duplication concerns worth addressing.
CI passing. Manual QA was required (UI files changed) but did not complete — no visual/a11y verification was performed, so this is not approved on UI grounds.
Blocking:
- XML attribute injection in
reassembleItemXml(parseItem.js) — unescaped&/</"in title/identifier corruptsraw_dataon every edit. runValidation()never fires on prompt/choice blur, missing an explicit acceptance criterion for #5978.
Suggestions:
ChoiceInteractionEditor.vueduplicatesAnswersEditor.vue's CSS/markup/computed block near-verbatim instead of sharing it — including falling back to Options API just to copybuttonAppearanceOverrides, and JS-based hover tracking instead of the established$computedClass:hoverpattern.- Issue #5978 specified standalone
parse.js/validate.jsmodules; this PR consolidates them into aChoiceInteractionDescriptorclass instead (not blocking — functionally equivalent and consistent withuseInteractionDescriptor.js).
Nitpicks: two live ChoiceInteractionDescriptor instances in index.js/useChoiceInteraction.js; unused promptPlaceholder/choiceContentPlaceholder i18n strings; stray data-test attribute not used by any test.
@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
| * @param {string[]} params.responseDeclarations - Array of serialized declaration XML strings | ||
| * @returns {string} Full QTI XML string | ||
| */ | ||
| export function reassembleItemXml({ identifier, title, language, bodyXml, responseDeclarations }) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: reassembleItemXml interpolates identifier/title/language straight into "..."-delimited XML attributes (e.g. line 115 identifier="${id}") with no escaping. These values come from getAttribute() in parseItem (line 58-60 above), which returns decoded text — an item titled Math & Science "Quiz" produces an unescaped &/" that either breaks well-formedness or terminates the attribute early. This is on the live write path (QTIItemEditor/index.vue emitRawData → updateItemRawData → item.raw_data), so any edit to an item whose title/identifier contains &, <, or " silently corrupts raw_data and fails to re-parse on next load. buildXmlNode/XMLSerializer (used elsewhere in assembleItem.js) already handles this correctly — build the assessment-item root the same way, or at minimum escape &/</" before interpolating.
| } catch { | ||
| return { prompt: '', choices: [] }; | ||
| } | ||
| function closeQuestion() { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: Acceptance criteria for #5978 require calling runValidation() on blur of the prompt RTE and each choice RTE. closeQuestion (here) and closeChoice (line 339) are wired to TipTapEditor's @minimize event — the de-facto blur signal (lines 24, 172) — but neither calls runValidation(). The only call sites are onToggleCorrect/onAddChoice/onRemoveChoice (404-424), none of which cover leaving the prompt or a choice empty. An author can leave the prompt or a choice blank, click away, and see no validation error until an unrelated action triggers it. Add runValidation() to both functions.
| }, | ||
| }, | ||
|
|
||
| computed: { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: This computed: {} block with buttonAppearanceOverrides() is a byte-for-byte copy of AnswersEditor.vue's Options API computed of the same name (lines 271-282 there), including the legacy this.$themePalette access pattern. Everything else in this file is Composition API in setup() — this didn't need to fall back to Options API; themePalette() from kolibri-design-system/lib/styles/theme can be called inside setup() (see CommunityLibraryStatusButton.vue, SubmitToCommunityLibrarySidePanel/Box.vue) and used in a computed() there instead.
| class="answer-border" | ||
| role="listitem" | ||
| :class="{ 'is-clickable': mode === 'edit' && openChoiceId !== answer.id }" | ||
| :style="{ |
There was a problem hiding this comment.
suggestion: This entire choice-card block (markup, CSS classes .answer-border/.answer-card-text/.answer-layout/.answer-selection/.answer-content/.answer-actions, and the hover handling below) duplicates channelEdit/components/AnswersEditor/AnswersEditor.vue almost verbatim, including identical pixel values in the style rules (lines 586-664). Worth extracting the shared row layout/CSS into a common sub-component or SCSS partial so the two don't drift — this PR's hover-handling regression (next comment) is exactly the kind of drift that duplication invites.
| : null, | ||
| }" | ||
| @click="onRowClick($event, answer.id)" | ||
| @mouseenter="hoveredId = answer.id" |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Hover state here is tracked via a hoveredId ref + mouseenter/mouseleave listeners per row. AnswersEditor.vue (the component this was copied from) handles the same affordance with $computedClass + a :hover pseudo-selector key (see answerClasses(), AnswersEditor.vue:315-322) — pure CSS, no per-row listeners or reactive re-renders. Consider following that existing pattern instead.
…ement XML attribute escaping in reassembleItemXml Signed-off-by: Abhishek-Punhani <punhani.manavabhi@gmail.com>
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
4 of 9 prior findings resolved; 1 acknowledged; 4 still open (see below).
Still open:
- CSS/markup duplication between
ChoiceInteractionEditor.vueandAnswersEditor.vue(row layout,.answer-*classes) — previously flagged as a suggestion, still duplicated, no shared extraction. - Two live
ChoiceInteractionDescriptorinstances (index.js/useChoiceInteraction.js) — nitpick, unchanged. - Unused
promptPlaceholder/choiceContentPlaceholderi18n strings still present inqtiEditorStrings.js— nitpick, unchanged. - Stray
data-test="editChoiceBtn"attribute (ChoiceInteractionEditor.vue:192) still unused by any test — nitpick, unchanged.
New findings: see inline comments (redundant hover-class guard condition; missing regression test for the XML-escaping fix).
CI passing. Manual QA was required for this PR (UI files changed) but did not complete this round — not visually/interactively re-verified.
Prior-finding status
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/serialization/parseItem.js:114 — XML attribute injection in reassembleItemXml
RESOLVED — contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/choice/ChoiceInteractionEditor.vue:345 — missing runValidation() on prompt/choice blur
RESOLVED — ChoiceInteractionEditor.vue buttonAppearanceOverrides Options-API/$themePalette fallback
RESOLVED — ChoiceInteractionEditor.vue JS-based hover tracking (hoveredId/mouseenter/mouseleave) instead of $computedClass pattern
UNADDRESSED — ChoiceInteractionEditor.vue CSS/markup duplication with AnswersEditor.vue (row layout, .answer-* classes)
ACKNOWLEDGED — parse.js/validate.js consolidated into ChoiceInteractionDescriptor class instead of standalone modules
UNADDRESSED — two live ChoiceInteractionDescriptor instances (index.js / useChoiceInteraction.js)
UNADDRESSED — unused promptPlaceholder/choiceContentPlaceholder i18n strings still present in qtiEditorStrings.js
UNADDRESSED — stray data-test="editChoiceBtn" attribute (ChoiceInteractionEditor.vue:192) still unused by any test
@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
| role="listitem" | ||
| :class="[ | ||
| { 'is-clickable': mode === 'edit' && openChoiceId !== answer.id }, | ||
| mode === 'edit' && |
There was a problem hiding this comment.
nitpick: mode === 'edit' && ... && !(answer.correct && mode === 'view' && showAnswers) — the leading mode === 'edit' already rules out mode === 'view', so the trailing !(... && mode === 'view' && ...) conjunct is always true and never affects the result. Simplify to mode === 'edit' && openChoiceId !== answer.id.
| * @param {string[]} params.responseDeclarations - Array of serialized declaration XML strings | ||
| * @returns {string} Full QTI XML string | ||
| */ | ||
| function escapeXmlAttr(unsafe) { |
There was a problem hiding this comment.
suggestion: escapeXmlAttr/reassembleItemXml have no dedicated spec. Add a case with a title containing &, ", < to guard against regression.
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
Thanks @Abhishek-Punhani! I have left some comments. Generally looks great, but it still needs some polishing to match the Figma specs. Please let me know if you have any questions 👐.
| } | ||
|
|
||
| return { | ||
| ...base, |
There was a problem hiding this comment.
Could we return the state as read-only from the composable? So that we can ensure the state is only edited using the mutation methods.
| /** | ||
| * Convenience: create a blank response declaration using this descriptor's schema. | ||
| * @param {string} questionType | ||
| * @param {string} [identifier] | ||
| * @returns {QTIDeclaration} | ||
| */ | ||
| createDeclaration(questionType, identifier = 'RESPONSE') { | ||
| return QTIDeclaration.fromInteractionDescriptor(this, questionType, identifier); | ||
| } |
There was a problem hiding this comment.
Are we using this method somewhere?
| /** | ||
| * Parse <qti-choice-interaction> body XML + response declarations → ChoiceState. | ||
| * | ||
| * @param {string} bodyXml | ||
| * @param {string[]} responseDeclarations | ||
| * @returns {object} ChoiceState | ||
| */ | ||
| parse(bodyXml, responseDeclarations) { | ||
| if (!bodyXml) return _defaultState(); | ||
|
|
||
| let root; | ||
| try { | ||
| root = parseXML(bodyXml).documentElement; | ||
| } catch { | ||
| return _defaultState(); | ||
| } | ||
|
|
||
| const maxChoices = parseInt(root.getAttribute('max-choices') ?? '0', 10); | ||
| const minChoices = parseInt(root.getAttribute('min-choices') ?? '0', 10); | ||
| const shuffle = root.getAttribute('shuffle') === 'true'; | ||
| const orientation = root.getAttribute('orientation') ?? 'vertical'; | ||
| const prompt = getPromptHTML(root); | ||
|
|
||
| const correctIds = _extractCorrectIds(responseDeclarations); | ||
|
|
||
| const answers = [...root.querySelectorAll('qti-simple-choice')].map(el => ({ | ||
| id: el.getAttribute('identifier') || generateRandomSlug('choice'), | ||
| content: el.innerHTML, | ||
| correct: correctIds.has(el.getAttribute('identifier') ?? ''), | ||
| fixed: el.getAttribute('fixed') === 'true', | ||
| })); | ||
|
|
||
| return { prompt, answers, maxChoices, minChoices, shuffle, orientation }; | ||
| } | ||
|
|
||
| /** | ||
| * Serialize ChoiceState → { bodyXml, declarations }. | ||
| * | ||
| * @param {object} state - ChoiceState | ||
| * @param {string} questionType | ||
| * @returns {{ bodyXml: string, declarations: string[] }} | ||
| */ | ||
| buildXML(state, questionType) { | ||
| const { prompt, answers, maxChoices, minChoices, shuffle, orientation } = state; | ||
|
|
||
| const attrs = { | ||
| 'response-identifier': RESPONSE_IDENTIFIER, | ||
| 'max-choices': maxChoices, | ||
| shuffle: String(shuffle), | ||
| orientation, | ||
| }; | ||
| if (minChoices > 0) attrs['min-choices'] = minChoices; | ||
|
|
||
| const children = []; | ||
|
|
||
| if (prompt) { | ||
| children.push(buildXmlNode({ tag: 'qti-prompt', children: _parseHtmlFragment(prompt) })); | ||
| } | ||
|
|
||
| for (const answer of answers) { | ||
| const choiceAttrs = { identifier: answer.id }; | ||
| if (answer.fixed) choiceAttrs.fixed = 'true'; | ||
| children.push( | ||
| buildXmlNode({ | ||
| tag: 'qti-simple-choice', | ||
| attrs: choiceAttrs, | ||
| children: _parseHtmlFragment(answer.content), | ||
| }), | ||
| ); | ||
| } | ||
|
|
||
| const interactionEl = buildXmlNode({ tag: 'qti-choice-interaction', attrs, children }); | ||
| const bodyXml = serializer.serializeToString(interactionEl); | ||
|
|
||
| const { cardinality } = this.getDeclarationSchema(questionType); | ||
| const declaration = new QTIDeclaration({ | ||
| identifier: RESPONSE_IDENTIFIER, | ||
| baseType: BaseType.IDENTIFIER, | ||
| cardinality, | ||
| tag: 'qti-response-declaration', | ||
| }); | ||
| const correctIds = answers.filter(a => a.correct).map(a => a.id); | ||
| new CorrectResponse(correctIds, declaration); | ||
|
|
||
| const declarationXml = serializer.serializeToString(declaration.getXML()); | ||
| return { bodyXml, declarations: [declarationXml] }; | ||
| } |
There was a problem hiding this comment.
Could we move this logic (and helper logic below) to a parse.js file? Mainly to keep the logic separated and prevent these descriptor files from growing too much. We are already encapsulating them by having them live within the interaction folders.
| /** | ||
| * Validate ChoiceState → ValidationError[]. | ||
| * | ||
| * @param {object} state - ChoiceState | ||
| * @param {string} questionType | ||
| * @returns {Array<{ code: string, id?: string }>} | ||
| */ | ||
| validate(state, questionType) { | ||
| const errors = []; | ||
| const { prompt, answers } = state; | ||
|
|
||
| if (!_stripTags(prompt).trim()) { | ||
| errors.push({ code: ValidationError.PROMPT_REQUIRED }); | ||
| } | ||
|
|
||
| if (answers.length < 2) { | ||
| errors.push({ code: ValidationError.TOO_FEW_CHOICES }); | ||
| } | ||
|
|
||
| for (const answer of answers) { | ||
| if (!_stripTags(answer.content).trim()) { | ||
| errors.push({ code: ValidationError.EMPTY_CHOICE_CONTENT, id: answer.id }); | ||
| } | ||
| } | ||
|
|
||
| const correctCount = answers.filter(a => a.correct).length; | ||
| if (correctCount === 0) { | ||
| errors.push({ code: ValidationError.NO_CORRECT_ANSWER }); | ||
| } else if (questionType === QuestionType.SINGLE_SELECT && correctCount > 1) { | ||
| errors.push({ code: ValidationError.TOO_MANY_CORRECT_ANSWERS }); | ||
| } | ||
|
|
||
| return errors; | ||
| } | ||
| } |
There was a problem hiding this comment.
Idem, we can probably separate this into a validation.js to keep this module with fewer responsibilities.
| function _stripTags(html) { | ||
| return (html ?? '').replace(/<[^>]*>/g, ''); | ||
| } |
There was a problem hiding this comment.
I think we can move this method to a helper module :).
| v-if="mode === 'edit'" | ||
| appearance="flat-button" | ||
| :appearanceOverrides="buttonAppearanceOverrides" | ||
| class="answer-editor-button" |
There was a problem hiding this comment.
Is the name of this class correct? 😅 Feels like this is for the edit buttons above rather than for the add choice button.
| const childNode = child.ownerDocument === xmlDoc ? child : xmlDoc.importNode(child, true); | ||
| el.appendChild(childNode); |
There was a problem hiding this comment.
Could we have a comment on why this was needed?
| .replace(/'/g, '''); | ||
| } | ||
|
|
||
| export function reassembleItemXml({ identifier, title, language, bodyXml, responseDeclarations }) { |
There was a problem hiding this comment.
Could we have this method on the assembleItem.js module instead, and call it assemble... instead of reassemble... as it won't necessarily be reassembling
😅
| const id = escapeXmlAttr(identifier || 'item'); | ||
| const t = escapeXmlAttr(title || ''); | ||
|
|
||
| return [ |
There was a problem hiding this comment.
Could we also use the buildXmlNode here? 😅 So that it's easier to edit and less prone to errors.
| function setOrientation(val) { | ||
| state.value = { ...state.value, orientation: val }; | ||
| } | ||
|
|
||
| function setMaxChoices(n) { | ||
| state.value = { ...state.value, maxChoices: n }; | ||
| } |
There was a problem hiding this comment.
We don't really need to expose these, as we won't be using them.
3 participants
Summary
Implemented the QTI declaration serialization framework, base interaction architecture, and the complete Choice Interaction editor.
This PR adds:
QTIDeclaration.js— Central class representing aqti-response-declarationelement. Handles parsing from XML and generating valid DOM nodes for serialization.ValidationMessage.vue— KDS-compliant UI component for displaying inline validation errors without Vuetify dependencies.useInteraction.js— Base composable that provides standard reactive state, XML parsing/assembly, and validation lifecycle for all future QTI interaction plugins.ChoiceInteractionDescriptor.js— The core logic module for Choice interactions.useChoiceInteraction.js— Composable extending the base interaction to provide all structural and field mutations for choice authoring (e.g.,addChoice,toggleCorrectChoice,moveChoiceUp).ChoiceInteractionEditor.vue— UI component for Single Select and Multi Select questions. Features real-time XML synchronization, responsive collapsible toolbars, and localized error handling.assembleItem.js/parseItem.js— Utilities updated to support dynamic XML node building and HTML prompt extraction.References
Closes #5978
Reviewer guidance
Navigate to the QTI demo page. Test both Single Selection and Multiple Selection modes (pre-populated in the first two cards). Verify that adding, removing, reordering, and editing choices works flawlessly.
AI usage
Used Antigravity for final review and nitpicks.