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
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.
There was a problem hiding this comment.
We should match the behavior at https://github.com/Abhishek-Punhani/studio/blob/973482d788a6cec0ba54feca8fdca7c337f165a6/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/choice/parse.js#L31. One item is enough to avoid overloading the user; we will flag the error anyway.
There was a problem hiding this comment.
Understood — matching choice's single-item seed it is. Withdrawing the two-item requirement.
| :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.
There was a problem hiding this comment.
We do not need a blur here to execute runValidation, because update is already fired on blur, and this updates the interaction state, and it runs the validation on every state mutation, so this is not needed even if TipTapEditor emitted a blur event.
There was a problem hiding this comment.
You're right — I missed that TipTapEditor emits update only on blur (TipTapEditor.vue:250-263), so the state mutation and its validation already happen at that point. Withdrawing this finding.
| </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.
There was a problem hiding this comment.
Let's remove the orderLabel if it's not used! :)
| })); | ||
|
|
||
| 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.
AlexVelezLl
left a comment
There was a problem hiding this comment.
Thanks! Looking good! Found some additional comments, the remaining rtibblesbot comments are valid and should be handled 👐
| :imageProcessor="EditorImageProcessor" | ||
| class="editor" | ||
| @update="setPrompt" | ||
| @blur="runValidation" |
There was a problem hiding this comment.
We do not need a blur here to execute runValidation, because update is already fired on blur, and this updates the interaction state, and it runs the validation on every state mutation, so this is not needed even if TipTapEditor emitted a blur event.
| <div | ||
| v-if="mode === 'edit'" | ||
| class="ordering-sublabel" | ||
| :style="{ color: $themeTokens.annotation }" | ||
| > | ||
| {{ correctOrderDescription$() }} | ||
| </div> |
There was a problem hiding this comment.
Could you add this v-if="mode === 'edit'" to the choice interaction and text entry interaction please? so that we dont show the "Select one correct answer" if it's not on edit mode.
| class="item-layout" | ||
| :class="{ | ||
| 'is-open': isItemOpen(item.id), | ||
| 'small-screen': windowIsSmall, |
There was a problem hiding this comment.
For 1, if we don't need a specific style for small screens, then we can just remove this class here.
For 2, it'd be best to collapse the move up and move down buttons if windowIsSmall.value is true.
| return { | ||
| responseIdentifier: RESPONSE_IDENTIFIER, | ||
| prompt: '', | ||
| items: [], |
There was a problem hiding this comment.
We should match the behavior at https://github.com/Abhishek-Punhani/studio/blob/973482d788a6cec0ba54feca8fdca7c337f165a6/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/choice/parse.js#L31. One item is enough to avoid overloading the user; we will flag the error anyway.
| message: 'Order', | ||
| context: 'Display name for an order question type', | ||
| }, | ||
| orderingLabel: { |
There was a problem hiding this comment.
Let's remove the orderLabel if it's not used! :)
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.