implement choice interaction parsing and serialization for QTIEditor#6012
implement choice interaction parsing and serialization for QTIEditor#6012Abhishek-Punhani wants to merge 4 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.
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.
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.
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.
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.
2 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.