implement choice interaction parsing and serialization for QTIEditor#6012
implement choice interaction parsing and serialization for QTIEditor#6012Abhishek-Punhani wants to merge 14 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.
| }, | ||
| }, | ||
|
|
||
| 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.
✅ Resolved — addressed in the current code.
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.
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.
✅ Resolved — addressed in the current code.
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.
✅ Resolved — addressed in the current code.
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?
| 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.
|
Ah, @Abhishek-Punhani, an update on the specs from a recent thread. We will now have this Answers setting section next to the type selector. So, for this, we will probably require a portal so that we can render the setting in that section, but let's not do that in the scope of this PR, we can introduce it once we have our type selector component. So, for now, could we render this just above the The "Show learners how many answers to select" option controls whether we set |
Abhishek-Punhani
force-pushed
the
Issue5965
branch
from
3f9409b to
58eba80
Compare
🔵 Review postedLast updated: 2026-07-08 18:47 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6012
8 of 11 prior findings resolved; 3 still open (see below).
One new blocking issue: the new update:interaction write path in QTIItemEditor doesn't actually reach rawData, so this PR's core deliverable (persisting choice-editor edits) is a no-op. CI passing; manual QA did not run this round (dev server did not come up), so this is not verified end-to-end in the browser.
- blocking:
update:interactionhandler inQTIItemEditor/index.vuenever updates state (see inline comment). - suggestion:
DUPLICATE_CHOICE_CONTENTvalidation rule is out-of-spec for #5978 (see inline comment). - Still open from prior rounds: CSS/markup duplication between
ChoiceInteractionEditor.vueandAnswersEditor.vue(suggestion, inline); two liveChoiceInteractionDescriptorinstances (nitpick, inline); unusedpromptPlaceholder/choiceContentPlaceholderi18n strings (nitpick, inline).
Prior-finding status
RESOLVED — parseItem.js:114 — XML attribute injection in reassembleItemXml (superseded by DOM-based buildXmlNode/assembleItemXml)
RESOLVED — ChoiceInteractionEditor.vue:339 — missing runValidation() on prompt/choice blur
RESOLVED — ChoiceInteractionEditor.vue — buttonAppearanceOverrides Options-API/$themePalette fallback duplicated from AnswersEditor
RESOLVED — ChoiceInteractionEditor.vue — JS-based hover tracking (hoveredId/mouseenter/mouseleave), now pure CSS :hover
RESOLVED — ChoiceInteractionEditor.vue — redundant mode === 'edit' && ... && !(... mode === 'view' ...) guard condition
RESOLVED — parseItem.spec.js:126-157 — missing regression test for XML escaping (&/"/< now covered)
RESOLVED — ChoiceInteractionEditor.vue — stray data-test="editChoiceBtn" attribute (no longer present)
RESOLVED — parse.js/validation.js — split into standalone modules per issue #5978 spec
UNADDRESSED — interactions/choice/index.js:23 — two live ChoiceInteractionDescriptor instances
UNADDRESSED — ChoiceInteractionEditor.vue:630 — CSS/markup duplication with AnswersEditor.vue (classes renamed answer-*→choice-* but block still copied, not shared)
UNADDRESSED — qtiEditorStrings.js:100 — unused promptPlaceholder/choiceContentPlaceholder i18n strings
@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
| @update:questionType="type => (currentQuestionType = type)" | ||
| @update:interaction=" | ||
| ({ bodyXml, responseDeclarations }) => { | ||
| currentBodyXml.value = bodyXml; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: This handler never actually mutates state, so every edit made in the choice editor is silently dropped and never reaches item.raw_data.
currentBodyXml/currentResponseDeclarations are declared as ref()s inside setup() (lines 131-136), but the return { ... } at line 153 never includes them — only currentQuestionType, interactions, questionNumberLabel, questionNumberAndTypeLabel, closeBtnLabel$, questionContentPlaceholder$ are exposed to the template. Since this SFC uses a plain setup(props, { emit }) (not <script setup>), the template has no compile-time binding metadata for currentBodyXml/currentResponseDeclarations — they resolve to undefined on the render context, so currentBodyXml.value = bodyXml here throws/no-ops against undefined, not the actual ref.
The downstream rawData computed (line 138) and the watch(rawData, ...) that emits update:rawData (line 149) therefore never see the new value — this is the entire write path the PR exists to deliver.
Fix: add currentBodyXml and currentResponseDeclarations to the return object at line 153 (or move this handler into a plain method inside setup() where .value assignment works as normal JS).
There was a problem hiding this comment.
There are also some errors on the console, potentially because of this ⬆️
There was a problem hiding this comment.
That's consistent with this finding — since currentBodyXml/currentResponseDeclarations aren't exposed via the return object, the template's .value accesses resolve against undefined, which would throw exactly the kind of console errors shown in the screenshot. Fixing the return block should resolve both the silent state-drop and these console errors.
| flex-direction: column; | ||
| } | ||
|
|
||
| .choice-border { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Still open from the last round — this row layout/CSS block (.choice-border/.choice-card-text/.choice-layout/.choice-content) is essentially the same markup and styles as AnswersEditor.vue's .answer-* block, just renamed. Worth extracting into a shared component now that both are stable, to avoid the two drifting apart.
There was a problem hiding this comment.
We are building a whole new component that will replace the old assessments editor, so it's not worth it.
There was a problem hiding this comment.
That makes sense — if the old assessments editor is being replaced by the new component, extracting a shared abstraction now isn't worth the investment. Thanks for the context.
| parse() { | ||
| return {}; | ||
| }, | ||
| const descriptor = new ChoiceInteractionDescriptor({ editorComponent: ChoiceInteractionEditor }); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: Still open from the last round — useChoiceInteraction.js imports the choiceInteractionDescriptor singleton from ChoiceInteractionDescriptor.js, while this file constructs its own separate new ChoiceInteractionDescriptor(...). Harmless today (no constructor side effects), but two instances of what's meant to be a stable descriptor invites divergence later.
| message: 'Add question below', | ||
| context: 'Action to add a new question below the current one', | ||
| }, | ||
| promptPlaceholder: { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: Still open from the last round — promptPlaceholder/choiceContentPlaceholder aren't referenced by any .vue file. Either wire them up or drop them.
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
Thanks @Abhishek-Punhani! Its looking way better! But some adjustments are still needed 👐.
|
|
||
| .validation-message { | ||
| margin: 2px 0 0; | ||
| font-size: 12px; |
There was a problem hiding this comment.
Could we use 14px to match the specs?
| * Track the current bodyXml and responseDeclarations for the interaction. | ||
| * Initialised from the parsed item; updated atomically when the editor | ||
| * emits update:interaction. | ||
| */ |
There was a problem hiding this comment.
Could we add a comment here that we are currently using only the first interaction, but can be expansible to multiple interactions per question in the future?
| <p | ||
| v-if="show" | ||
| class="validation-message" | ||
| role="alert" | ||
| :style="{ color: $themeTokens.error }" | ||
| > | ||
| <slot></slot> | ||
| </p> |
There was a problem hiding this comment.
We've recently had some issues when the root node of the component has a v-if, so could you wrap this with a <div> so that we aviod those issues?
| // Use the import-safe descriptor core — it owns parse/buildXML/validate schema | ||
| // without depending on choice/index.js or ChoiceInteractionEditor.vue. |
There was a problem hiding this comment.
I think we can remove this comment. Seems like a historical reflection rather than flagging something.
| ...state.value, | ||
| choices: [ | ||
| ...state.value.choices, | ||
| { id: generateRandomSlug('choice'), content: '', correct: false, fixed: false }, |
There was a problem hiding this comment.
Given that we won't use fixed for now, I think we can just omit this here.
| // Emit a single unified interaction object whenever either bodyXml or | ||
| // responseDeclarations changes, mirroring the shape of the incoming prop. | ||
| const updatedInteraction = computed(() => ({ | ||
| ...props.interaction, |
There was a problem hiding this comment.
Is this ...props.interaction really needed?
|
|
||
| // Emit a single unified interaction object whenever either bodyXml or | ||
| // responseDeclarations changes, mirroring the shape of the incoming prop. | ||
| const updatedInteraction = computed(() => ({ |
There was a problem hiding this comment.
Could we call it workingInteraction instead?
| const clickable = props.mode === 'edit' && closed; | ||
| const isCorrect = choice.correct && (props.mode === 'edit' || props.showAnswers); | ||
| const hoverBg = isCorrect | ||
| ? instance.proxy.$themePalette.green.v_100 |
There was a problem hiding this comment.
Could we use the themePalette method from KDS similar to how we have done so in other places?
| :mode="mode" | ||
| :showAnswers="showAnswers" | ||
| @update:questionType="type => (currentQuestionType = type)" | ||
| @update:interaction=" |
There was a problem hiding this comment.
Also, could we add a log only for dev mode of the resulting XML of this? So that we can see the resulting XML in the console.
…ement XML attribute escaping in reassembleItemXml Signed-off-by: Abhishek-Punhani <punhani.manavabhi@gmail.com>
…roduce improved state validation, and modernize component reactive data handling Signed-off-by: Abhishek-Punhani <punhani.manavabhi@gmail.com>
…n state and logic Signed-off-by: Abhishek-Punhani <punhani.manavabhi@gmail.com>
…:interaction event and update related tests Signed-off-by: Abhishek-Punhani <punhani.manavabhi@gmail.com>
…nd remove unused interaction state helpers Signed-off-by: Abhishek-Punhani <punhani.manavabhi@gmail.com>
Signed-off-by: Abhishek-Punhani <punhani.manavabhi@gmail.com>
Signed-off-by: Abhishek-Punhani <punhani.manavabhi@gmail.com>
Signed-off-by: Abhishek-Punhani <punhani.manavabhi@gmail.com>
…ML support to buildXmlNode Signed-off-by: Abhishek-Punhani <punhani.manavabhi@gmail.com>
…to TipTapEditor default styles Signed-off-by: Abhishek-Punhani <punhani.manavabhi@gmail.com>
Abhishek-Punhani
force-pushed
the
Issue5965
branch
from
34b3324 to
6896864
Compare
|
@AlexVelezLl |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6012 — delta re-review (core pass only; new commits landed after this review ran, so re-check on next push).
11 of 11 prior findings resolved/acknowledged; 1 new finding.
New finding
assembleItem.js:40 — buildXmlNode's new innerHTML branch re-parses RTE HTML output as strict XML with no <parsererror> check. See inline comment.
Manual QA was required but did not run (dev server didn't start) — UI not visually verified, not a basis for approval either way. CI passing.
Prior-finding status
RESOLVED — QTIItemEditor/index.vue:38 — update:interaction handler never mutated state
RESOLVED — ChoiceInteractionEditor.vue:280 — runValidation() on blur now wired for #5978
ACKNOWLEDGED — ChoiceInteractionEditor.vue — buttonAppearanceOverrides duplication with AnswersEditor.vue (maintainer: not worth extracting, legacy editor being replaced)
ACKNOWLEDGED — ChoiceInteractionEditor.vue — hover-state duplication with AnswersEditor.vue (same maintainer decision as above)
RESOLVED — ChoiceInteractionEditor.vue — mode === 'edit' && ... redundant leading clause simplified
RESOLVED — parseItem.js — escapeXmlAttr/reassembleItemXml spec coverage added (title/identifier/language with &, quotes, etc.)
RESOLVED — choice/index.js:23 — duplicate ChoiceInteractionDescriptor instances, now single singleton import
ACKNOWLEDGED — choice/validation.js:41 — DUPLICATE_CHOICE_CONTENT out-of-spec for #5978 (maintainer confirmed updated Figma spec; test coverage added)
ACKNOWLEDGED — qtiEditorStrings.js — unused placeholder strings (removed entirely, moot)
ACKNOWLEDGED — TOO_FEW_CHOICES removal (explicit maintainer decision per AlexVelezLl, diverges from literal issue text)
ACKNOWLEDGED — useInteraction.js auto-validation on state change (explicit maintainer decision per AlexVelezLl, covered by 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
| el.appendChild(xmlDoc.createTextNode(child)); | ||
| } else { | ||
| el.appendChild(child); | ||
| if (innerHTML !== undefined) { |
There was a problem hiding this comment.
blocking: This innerHTML branch parses RTE-produced HTML as strict XML with no error check. buildChoiceInteractionXML (interactions/choice/parse.js:108,118) feeds it state.prompt/choice.content from TipTap's getHTML() — useEditor.js enables default StarterKit (hard breaks not disabled) plus an Image extension, so HTML serialization can emit unclosed void elements (<br>, <img src="...">). DOMParser(...,'text/xml') on a fragment containing one of these doesn't throw — it silently produces a <parsererror> document, which then gets appended as a child and written into item.raw_data on save. parseXML in parseItem.js:13-23 already has the right pattern (explicit <parsererror> check + throw) — mirror it here. No test in the "innerHTML option" block covers an unclosed void element, so this wouldn't be caught by CI.
| return ids; | ||
| } | ||
|
|
||
| export function _parseHtmlFragment(html) { |
There was a problem hiding this comment.
nitpick: _parseHtmlFragment appears unused now that buildChoiceInteractionXML builds content via buildXmlNode's innerHTML option directly. Safe to delete if nothing references it.
For sure! Could you please open a follow up issue for this, to keep track of it? 👐 (only for the setting, the integration with the question type selector will be done separately) |
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
Almost there! Found a couple of things to fix on the test modules :). Will also invite @rtibbles to take a look 👐.
| <div | ||
| :class="promptWrapperClass" | ||
| :style="promptWrapperStyle" | ||
| @click="handlePromptClick" |
There was a problem hiding this comment.
Just a note that this is not the best accessible way to do this, but will create a follow up issue for this to avoid blocking the PR for this :)
| :buttonValue="choice.id" | ||
| :label="markCorrectLabel$()" | ||
| :showLabel="false" | ||
| :aria-label="markCorrectLabel$()" |
There was a problem hiding this comment.
We do not need this aria-label here, as label already handles this. (The same with KCheckbox)
|
|
||
| function onAddChoice() { | ||
| addChoice(); | ||
| openChoiceId.value = state.value.choices[state.value.choices.length - 1]?.id ?? null; |
There was a problem hiding this comment.
For this, could we open automatically the tip tap editor of the new added choice, similar to how its done in the legacy assessment editor?
| ? instance.proxy.$themeTokens.error | ||
| : instance.proxy.$themeTokens.fineLine, | ||
| cursor: props.mode === 'edit' ? 'pointer' : undefined, | ||
| }; | ||
| }); | ||
|
|
||
| const prompt = computed(() => parsed.value.prompt); | ||
| const choices = computed(() => parsed.value.choices); | ||
| function isChoiceClosed(id) { | ||
| return props.mode !== 'edit' || openChoiceId.value !== id; | ||
| } | ||
|
|
||
| function choiceHasError(id) { | ||
| return emptyChoiceIds.value.has(id) || duplicateChoiceIds.value.has(id); | ||
| } | ||
|
|
||
| function isChoiceOpen(id) { | ||
| return props.mode === 'edit' && openChoiceId.value === id; | ||
| } | ||
|
|
||
| function getChoiceClasses(choice) { | ||
| const closed = isChoiceClosed(choice.id); | ||
| const clickable = props.mode === 'edit' && closed; | ||
| const isCorrect = choice.correct && (props.mode === 'edit' || props.showAnswers); | ||
| const hoverBg = isCorrect ? palette.green.v_100 : instance.proxy.$themeTokens.fineLine; | ||
| return [ | ||
| { 'is-clickable': clickable }, | ||
| clickable | ||
| ? instance.proxy.$computedClass({ | ||
| ':hover': { backgroundColor: hoverBg }, | ||
| }) | ||
| : '', | ||
| ]; | ||
| } | ||
|
|
||
| return { prompt, choices, selectedValue, selectedValues, toggleValue, QuestionType }; | ||
| function getChoiceStyle(choice) { | ||
| const isCorrect = choice.correct && (props.mode === 'edit' || props.showAnswers); | ||
| const hasError = choiceHasError(choice.id); | ||
|
|
||
| let borderColor = instance.proxy.$themeTokens.fineLine; | ||
| if (hasError) { | ||
| borderColor = instance.proxy.$themeTokens.error; |
There was a problem hiding this comment.
Could we avoid using instance.proxy.$themeTokens, and instead use the themeTokens method like this?
| const { prompt, choices, maxChoices, minChoices, shuffle, orientation } = state; | ||
|
|
||
| const attrs = { | ||
| 'response-identifier': RESPONSE_IDENTIFIER, |
There was a problem hiding this comment.
seems like this is missing 👀
There was a problem hiding this comment.
Could we use the actual translator strings for DOM queries? rather than hardcoded /add choice/i, for example.
| // Flush Vue watcher queue. | ||
| await nextTick(); | ||
| // Advance past the 400ms debounce, then flush the resulting DOM update. | ||
| jest.advanceTimersByTime(400); |
There was a problem hiding this comment.
Just wonder if it'd be better if we just mock the debounce method for these?
|
|
||
| it('sets cardinality="single" for singleSelect', () => { | ||
| const { responseDeclarations } = buildXML(baseState, QuestionType.SINGLE_SELECT); | ||
| expect(responseDeclarations[0]).toContain('cardinality="single"'); |
There was a problem hiding this comment.
Could we be a little more rigorous with these checks for the buildXML() function, rather than just checking if the string contains? We can parse the resulting xml, and then check the actual properties of the xml tags, so that we also make sure the function is returning valid XML.
|
|
||
| // --------------------------------------------------------------------------- | ||
| // assembleItemXml — XML attribute escaping (regression) | ||
| // --------------------------------------------------------------------------- |
There was a problem hiding this comment.
Given that this is already described by the describe label below, I think we can remove these comments 👐
| @@ -1,4 +1,6 @@ | |||
| import { parseXML, parseItem } from '../parseItem'; | |||
| /* eslint-disable jest-dom/prefer-to-have-attribute, jest-dom/prefer-to-have-text-content */ | |||
| import { assembleItemXml } from '../assembleItem'; | |||
There was a problem hiding this comment.
Should we test this method on the assembleItem test module instead?
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.