Repository navigation
Show QTI questions read-only when an edit would drop item-level content - #6306
rtibblesbot wants to merge 1 commit into
Conversation
90c5b61 to
dbe8a96
Compare
|
Reworked per @AlexVelezLl's direction: items with item-level content the editor doesn't write now open read-only instead of carrying it. The PR description above still describes the carry-over approach.
Deleting a hint on the edit-schemalocation.webm |
f6778b6 to
0f4e0d1
Compare
rtibblesbot
changed the title
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
The high level strategy proposed for this issue was too risky and prone to errors, where new introducedn editor capabilities could potentially drop support for questions created with older versions of it. Let's allow some flexibility in favor of a safer strategy for 99.99999% of cases.
| } | ||
|
|
||
| /** Root metadata the converter writes and nothing in the item points at. */ | ||
| const METADATA_ROOT_ATTRIBUTES = ['label', 'tool-name', 'tool-version', 'language']; |
There was a problem hiding this comment.
suggestion: With an allowlist, root attributes need one too: identifier, title, xml:lang, label, tool-name, tool-version, xsi:schemaLocation, and adaptive/time-dependent only when false. Any other attribute locks the item.
To write them back, could assembleItemXml write the converter's root itself instead of copying it from the source? The Python AssessmentItem (utils/assessment/qti/assessment_item.py) always writes the same xmlns:xsi and xsi:schemaLocation. That removes the sourceXml parameter, isCarriedRootAttribute, and the prefix-declaration filter with its tests. label/tool-name/tool-version can either be written as the converter writes them (kolibri/0.1) or carried as plain attributes, without namespace handling.
Legacy language: items converted before c51c5e0 have a root language instead of xml:lang, and parseItem reads only xml:lang (parseItem.js:40). The PR currently carries language over, so an edited legacy item never gets xml:lang. I reproduced that with a converted fixture. Could parseItem fall back to language, and the allowlist accept it, so an edit writes it as xml:lang?
There was a problem hiding this comment.
Done in a3f4945.
isEditorRootAttribute(validateItem.js): your list plus legacylanguage. Any other attribute, namespaced ones included, locks the item.assembleItemXmlwritesxmlns:xsi,xsi:schemaLocation,tool-name="kolibri"andtool-version="0.1"as the converter does.sourceXml,isCarriedRootAttributeand the prefix filter are gone.labelis carried as a plain attribute viaparseItem→useQtiItem; the converter never sets one, so there is no fixed value to write.parseItemfalls back tolanguage, so an edit writes it asxml:lang(test inconvertedItem.spec.js).- Blank items now get the same root metadata.
Searched QTIEditor for other root-language reads: 1 (parseItem.js:43), changed.
| * @returns {boolean} Whether assembling the item unedited writes back each source root | ||
| * attribute and child as it was | ||
| */ | ||
| function writesBackItemContent(sourceXml) { |
There was a problem hiding this comment.
suggestion: Could the check be an allowlist of the root children the editor writes, instead of reassembling the item and comparing? qti-assessment-item has a closed set of 12 children (AssessmentItem.Type in utils/assessment/qti/schema/xsd/imsqti_itemv3p0p1_v1p0.xsd), and the editor writes 5 of them:
// The item children the editor writes; any other child makes the item read-only.
const EDITOR_CHILDREN = [
'qti-response-declaration',
'qti-outcome-declaration',
'qti-item-body',
'qti-catalog-info',
'qti-response-processing',
];
export function keepsItemContent(sourceXml) {
const root = parseXML(sourceXml).documentElement;
return (
attributesOf(root).every(isEditorRootAttribute) &&
[...root.children].every(
el =>
el.namespaceURI === root.namespaceURI &&
EDITOR_CHILDREN.includes(el.localName) &&
(el.localName !== 'qti-catalog-info' || holdsOnlyHints(el)),
)
);
}(A sketch; I haven't run it. isEditorRootAttribute and holdsOnlyHints are described in the other comments.)
The comparison makes the editor's current output the reference, so anything that differs locks the item, even when an edit would lose nothing. I ran these through isEditableItem on an otherwise editable choice item, and each opens read-only:
- explicit
match_correctrules instead of the template; - a
SCOREdefault of 1, which the template overwrites anyway; normal-maximumonSCORE, or aMAXSCOREoutcome, as other QTI tools export;- a multi-blank item with one blank unanswered. The editor then writes no processing, so the stored rules no longer match. Not reachable until a multi-blank editor ships, but then it is.
It also ties stored content to today's writer. If assembleItemXml later adds an attribute to SCORE, or changes the rules it writes for several declarations, every item saved before that change locks. The same goes for items stored by the Python converter. That has already happened once: STANDARD_TEMPLATE_URIS exists because the converter's template spelling changed in 90b0a08.
With the allowlist, response processing and outcome declarations belong to the editor, as buildScoringNodes already says ("Regenerated on every save rather than carried over"). The trade-off: a non-zero SCORE default or custom partial-credit processing gets replaced on edit instead of locking the item. That matches the interaction builders, which already rewrite mappings on edit. An allowlist rather than a list of unsupported children fails closed on content that isn't valid QTI, which matters because ricecooker raw_data isn't validated on upload.
writesBackItemContent, isSameContent, the outcome and default-value cases of holdsNothing, STANDARD_TEMPLATE_URIS and probably the cache would go. Most read-only cases in assembleItem.spec.js still apply. The scoring ones (a score maximum, non-zero defaults, processing the editor doesn't write) and the unused response declaration would become editable.
There was a problem hiding this comment.
Done in a3f4945: keepsItemContent in validateItem.js, as sketched.
- Children: your five, in the root namespace. Non-whitespace text at the root also locks.
- Gone:
writesBackItemContent,isSameContent, the outcome/default cases,STANDARD_TEMPLATE_URIS, the cache. - Now editable, with tests: explicit
match_correctrules,SCOREdefault 1,normal-maximumplusMAXSCORE, an unused response declaration. An edit replaces their scoring. - Still read-only: stylesheet, modal feedback, glossary catalog, non-hint cards,
adaptive/time-dependent="true".
Searched QTIEditor for other checks that reassemble and compare: 0. isSupportedItem matches body structure per descriptor, so it is unchanged.
The PR description's reviewer guidance still describes the comparison; this supersedes it.
| * @param {string} sourceXml | ||
| * @returns {boolean} | ||
| */ | ||
| export function keepsItemContent(sourceXml) { |
There was a problem hiding this comment.
nitpick: The check judges an item rather than assembling one. Could it live in validateItem.js, next to isEditableItem, with the hint rule in hints.js (see that comment)? assembleItem.js would then stay the write path only and wouldn't need to import parseItem. As a scan over the root's attributes and children, it probably doesn't need the cache either.
There was a problem hiding this comment.
Done in a3f4945: keepsItemContent sits beside isEditableItem in validateItem.js, uncached. assembleItem.js no longer imports parseItem.
| * @param {Element} card | ||
| * @returns {boolean} | ||
| */ | ||
| function isHintCard(card) { |
There was a problem hiding this comment.
praise: isHintCard is strict in the right way. It accepts only the card shape the hint editor reads in full, so a card with xml:lang, a second content element or a qti-file-href locks the item instead of being half-read.
There was a problem hiding this comment.
Kept as is, now in hints.js. A non-card child of the hint catalog also locks the item.
| * @param {Node} node | ||
| * @returns {boolean} | ||
| */ | ||
| function holdsNothing(node) { |
There was a problem hiding this comment.
suggestion: Keep this hint rule (isHintCard plus the qti-catalog and qti-catalog-info cases here), but move it to hints.js, e.g. as holdsOnlyHints(catalogInfo) next to parseHints. It decides what parseHints can read in full, so the two should change together. parseHints finds cards by support in any catalog and reads only the first qti-html-content; if it grows, for example to hints in several languages, this rule has to grow with it. With the allowlist, the outcome and default-value cases here can go.
There was a problem hiding this comment.
Done in a3f4945: holdsOnlyHints(catalogInfo) with isHintCard in hints.js, under parseHints. An attribute on qti-catalog-info also locks the item. The outcome and default-value cases are gone.
a3f4945 to
fa52f67
Compare
- An item opens read-only when its root has an attribute or child the editor doesn't write; scoring outcomes and processing are regenerated. - An item that reads an outcome (feedback, printed variable) opens read-only: the rich-text editors drop those elements. - holdsOnlyHints (hints.js) accepts only hint cards parseHints reads in full. - assembleItemXml writes the converter's root metadata, including xsi:schemaLocation. - parseItem reads a pre-c51c5e035 root `language`, so an edit writes it as xml:lang. - validateQtiItem still applies the editor's rules to these items, so an unanswerable one stays incomplete. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
fa52f67 to
b6cffde
Compare
2 participants
Summary
<qti-stylesheet>,<qti-modal-feedback>, a non-hint catalog,adaptive="true", or an outcome reference.xsi:schemaLocation,tool-name,tool-version,xml:langfor legacylanguage, and keeplabel.References
Fixes #6290.
Reviewer guidance
validateItem.js;<qti-catalog-info>may hold only hint cardsparseHintsreads in full.SCOREdefault or custom partial credit.QA: import "Skeletal Structure - Practice" (Kolibri QA Channel > Courses > Hummingbird Biology > Anatomy & Physiology > Skeletal Structure) into an editable channel; open Questions.
Evidence
Outcome references open read-only
SCOREquestion editable; edit dropped the variableAfter: read-only, covered by Jest.
Read-only items that break a rule
readonly-rules-delete-flagged.webm
Scoring extras stay editable
s1x-edit-correct-answer-save-reopen.webm
Root extras open read-only
adaptive="false",time-dependent="0": editableLegacy root
languages3-legacy-language-edit-save-reopen.webm
Converted legacy hints
s4-edit-delete-hint-save-reopen.webm
New questions
s5-text-entry-create-reopen.webm
More captures (17)
SCOREquestion in the editor, Beta markedSCOREquestion after savefalse/0question in the editorlanguagequestion: Edit enabledlanguagequestion in the editorlanguagequestion reopenedDeviations from the issue spec
<qti-stylesheet>,<qti-modal-feedback>, non-hint catalogsAI usage
Claude Code wrote and self-reviewed it; Jest and pre-commit pass.
🤖 Generated with Claude Code
Written by rtibblesbot, an LLM-based coding agent.
🟡 Waiting for feedback
Last updated: 2026-10-11 13:51 UTC