feat: add Ordering Interaction Editor - #6089
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. |
…ation and tests Signed-off-by: Abhishek-Punhani <punhani.manavabhi@gmail.com>
ab9baff to
973482d
Compare
|
📢✨ Before we assign a reviewer, we'll turn on |
🔵 Review postedLast updated: 2026-08-13 16:38 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6089 — the ordering plugin mirrors interactions/choice/ closely and the view-mode emit guard the issue asked for is correctly implemented. Three explicit issue requirements are missing, two of which cause real defects: a newly created ordering item serializes to schema-invalid XML with zero choices, and max-choices/min-choices are dropped when an imported item is edited. Separately, a cluster of wiring in the editor is dead — @blur, the item refs, and the small-screen class.
CI passing. Manual QA did not run (dev server failed to start), so nothing here is visually verified — the narrow-viewport and focus/screen-reader findings need QA confirmation.
- blocking: zero-item default state; dropped
max-choices/min-choices; noteleportTargetId/AnswerSettings/setShuffle, soshuffleis unreachable - suggestion: dead
@blurand ref/focus plumbing, unimplemented small-screen handling, focus lost on delete/move-to-top, no reorder announcement, unspecified duplicate-content rule, weak tests - nitpick: duplicated string, doubled
modewatcher, view-mode header
@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
| return { | ||
| responseIdentifier: RESPONSE_IDENTIFIER, | ||
| prompt: '', | ||
| items: [], |
There was a problem hiding this comment.
blocking: items: [] deviates from the issue ("Crucial: It must seed the items array with two empty items"), and it produces invalid XML.
InteractionSection/index.vue:68 builds fresh state via newDescriptor.parse('', []) when the author picks Ordering, so buildOrderingInteractionXML emits a childless <qti-order-interaction/>. OrderInteractionDType requires at least one choice (schema/xsd/imsqti_itemv3p0p1_v1p0.xsd:29157, minOccurs="1"). OrderingInteractionEditor.vue:293 then emits update:interaction on mount in edit mode, pushing that invalid XML up to QTIItemEditor before the author types anything — and the author sees "At least 2 items are required" on a pristine question.
Restoring the two-item seed fixes all of it. parse.spec.js:21 and OrderingInteractionEditor.spec.js:234 currently pin the deviation and need updating too.
| } | ||
|
|
||
| const responseIdentifier = root.getAttribute('response-identifier') || RESPONSE_IDENTIFIER; | ||
| const orientation = root.getAttribute('orientation') ?? Orientation.VERTICAL; |
There was a problem hiding this comment.
blocking: max-choices / min-choices are never read or emitted, so editing drops them.
The issue requires reading all three attributes and lists them in the OrderingState typedef, with two acceptance criteria. Since bodyXml is regenerated from state on every edit, opening an imported partial-ordering item (<qti-order-interaction max-choices="3" min-choices="3">, valid per OrderInteractionDType) and touching anything silently rewrites "order any 3 of these" into "order all of them".
Read them alongside orientation, leave them undefined when absent, and add to attrs only when defined — the way buildChoiceInteractionXML conditionally adds min-choices at interactions/choice/parse.js:146.
| }; | ||
| }, | ||
|
|
||
| props: { |
There was a problem hiding this comment.
blocking: no teleportTargetId prop, no AnswerSettings teleport, and no setShuffle — shuffle is unreachable.
The issue lists teleportTargetId: String in the props block and requires setShuffle(shuffle) in §7; neither exists (useOrderingInteraction.js:58-67 returns no setter). InteractionSection/index.vue:25 passes :teleportTargetId="settingsTargetId" to every interaction editor and QuestionTypeSelector/index.vue:53-56 renders the target div, so for ordering the settings panel has no Answer settings group at all — compare ChoiceInteractionEditor.vue:4-15.
Consequence: an imported item with no shuffle attribute parses to false (line 78 — === 'true', despite _defaultState() and the typedef saying default true), is written back as shuffle="false", and can never be changed — while line 62 unconditionally tells the author "Learners will see these shuffled". The issue flagged only the presentation as pending design; the prop and setter were specified outright.
| :imageProcessor="EditorImageProcessor" | ||
| class="editor" | ||
| @update="setPrompt" | ||
| @blur="runValidation" |
There was a problem hiding this comment.
suggestion: @blur="runValidation" never fires (here and on each item, line 116). TipTapEditor declares emits: ['update', 'minimize', 'open-editor'] (TipTapEditor.vue:329) and emits no blur; in Vue 2 a listener without .native only receives custom events.
So the issue's "calls runValidation() on prompt blur and each item content RTE blur" does not happen — validation comes solely from the 400 ms debounced watcher in useInteraction.js:60, which fires right after addItem() and turns the pristine new row red. That is the harsh UX the issue wanted to avoid, and OrderingInteractionEditor.spec.js:215-228 asserts it.
Either add a blur emit to TipTapEditor, or use @minimize (which already fires on click-outside and Escape), or drop the listeners.
| </div> | ||
| <div class="item-content"> | ||
| <TipTapEditor | ||
| :ref="el => setItemRef(el, index)" |
There was a problem hiding this comment.
suggestion: the ref/focus plumbing is dead code. TipTapEditor's setup() return exposes no focus, so typeof editorEl.focus === 'function' at line 375 is always false. Focus on a new item comes from :autofocus="isItemOpen(item.id)" plus openItem(newId).
itemEditorRefs (274), setItemRef (276-278), this :ref binding and the focus block (374-375) can all go, along with the await nextTick() in onAddItem. Also, itemEditorRefs.value is assigned by index and never pruned, so removals leave stale entries. ChoiceInteractionEditor.onAddChoice has none of this.
| return doc.documentElement; | ||
| } | ||
|
|
||
| describe('_defaultState()', () => { |
There was a problem hiding this comment.
nitpick: these three cases reach into the _-prefixed export and assert the object literal back at itself, with no code path in between. parse('', []) (line 37) and parse('<not-valid', []) (line 42) already exercise the same default through the public descriptor, which is where a caller can observe it.
| emit('update:interaction', newVal); | ||
| }); | ||
|
|
||
| watch( |
There was a problem hiding this comment.
nitpick: two watchers on props.mode with { immediate: true } (257-272 and here). Merging them makes the mount-time ordering — open prompt/item, then emit — explicit instead of dependent on declaration order.
| message: 'Order', | ||
| context: 'Display name for an order question type', | ||
| }, | ||
| orderingLabel: { |
There was a problem hiding this comment.
nitpick: orderLabel ("Order", same context) sits four lines above and is unused. Two near-identical strings for one concept is extra translator work — reuse one or drop the other.
| class="ordering-header-label" | ||
| :style="{ color: $themePalette.grey.v_700 }" | ||
| > | ||
| {{ correctOrderLabel$() }} |
There was a problem hiding this comment.
nitpick: the issue specifies an "Answers" header for view mode with showAnswers: true, but the header is correctOrderLabel$() in both modes (only the sublabel is hidden). answersLabel$ already exists and is what ChoiceInteractionEditor.vue:71 uses.
| })); | ||
|
|
||
| watch(workingInteraction, newVal => { | ||
| if (props.mode !== 'edit') return; |
There was a problem hiding this comment.
praise: the props.mode !== 'edit' guard before emitting is the fix the issue asked for, and it is genuinely new — ChoiceInteractionEditor.vue:332 still emits unconditionally. OrderingInteractionEditor.spec.js:194 locks it in.
2 participants
Summary
Implemented the QTI Ordering Interaction editor, expanding the assessment authoring framework to support sequence-based questions.
OrderingInteractionEditor.vue — UI component for Ordering questions. Features real-time XML synchronization, responsive collapsible toolbars, ordered positional badges, and strict validation state handling.
OrderingInteractionDescriptor.js — Core interaction descriptor mapping the
qti-order-interactionschema to the authoring UI.parse.js — Parsing and XML serialization logic for Ordering interactions, ensuring the
qti-correct-responsesequence strictly matches the author's defined order.validate.js — Validation rules ensuring ordering questions strictly contain at least two choices and no duplicate or empty content.
tests/* — Comprehensive test suites for parsing, validation, and editor interactions, adhering to ARIA-role querying standards.
References
Closes #6085
Reviewer guidance
Navigate to the QTI demo page. Test the Ordering Interaction editor. Verify that adding, removing, and reordering items works. Verify that validation correctly flags empty/duplicate items and warns if fewer than 2 items are present. Check that the responsive layout collapses correctly on smaller screens and that the correct order is preserved across XML serialisation cycles.
AI usage
Used Antigravity for final review and nitpicks.