From 636c27159a09caab49b5ae0ae7fe1e28977c1823 Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Mon, 5 Oct 2026 16:05:26 +0000 Subject: [PATCH 1/2] feat: show QTI items whose body the editor can't rebuild as unsupported - Block interactions: the body must hold only the interaction - Text entry: the interaction alone in a

, nothing after it - Attributes on the body or the text entry's

make the item unsupported Co-Authored-By: Claude Opus 5.5 (1M context) --- .../QTIEditor/__tests__/validateItem.spec.js | 11 + .../__tests__/QTIItemEditor.spec.js | 44 +++- .../components/QTIItemEditor/index.vue | 19 +- .../views/QTIEditor/composables/useQtiItem.js | 4 + .../interactions/InteractionDescriptor.js | 13 ++ .../__tests__/resolveDescriptor.spec.js | 219 +++++++++++++++++- .../interactions/resolveDescriptor.js | 57 ++++- .../interactions/textEntry/Descriptor.js | 43 +++- .../__tests__/convertedItem.spec.js | 13 ++ .../views/QTIEditor/serialization/xml.js | 27 +++ .../views/QTIEditor/utils/testingFixtures.js | 31 +++ .../shared/views/QTIEditor/validateItem.js | 11 +- 12 files changed, 454 insertions(+), 38 deletions(-) diff --git a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/__tests__/validateItem.spec.js b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/__tests__/validateItem.spec.js index a3c397f44b..c0084db52b 100644 --- a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/__tests__/validateItem.spec.js +++ b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/__tests__/validateItem.spec.js @@ -7,12 +7,14 @@ import { VALID_CHOICE_ITEM_DOCUMENT, CHOICE_ITEM_DOCUMENT_NO_PROMPT, CHOICE_ITEM_DOCUMENT_NO_CORRECT_ANSWER, + CHOICE_ITEM_DOCUMENT_NO_CORRECT_ANSWER_WITH_STIMULUS, NO_INTERACTION_ITEM_DOCUMENT, INLINE_CHOICE_ITEM_DOCUMENT, VALID_MATCH_ITEM_DOCUMENT, MATCH_THREE_SETS_XML, MATCH_XML, MULTI_TEXT_ENTRY_ITEM_DOCUMENT, + TEXT_ENTRY_ITEM_DOCUMENT_SHARED_PARAGRAPH, MULTI_INTERACTION_ITEM_DOCUMENT, UNRECOGNIZED_INTERACTION_ITEM_DOCUMENT, } from '../utils/testingFixtures'; @@ -41,6 +43,15 @@ describe('validateQtiItem', () => { expect(validateQtiItem(xml)).toEqual([]); }); + it('applies editor rules to an item whose body the editor cannot reproduce', () => { + expect(validateQtiItem(CHOICE_ITEM_DOCUMENT_NO_CORRECT_ANSWER_WITH_STIMULUS)).toEqual([ + { code: ValidationError.NO_CORRECT_ANSWER }, + ]); + expect( + validateQtiItem(TEXT_ENTRY_ITEM_DOCUMENT_SHARED_PARAGRAPH, { allowFreeResponse: false }), + ).toEqual([{ code: ValidationError.FREE_RESPONSE_NOT_ALLOWED }]); + }); + it('reports an item whose body holds no interaction', () => { expect(validateQtiItem(NO_INTERACTION_ITEM_DOCUMENT)).toEqual([ { code: ValidationError.NO_INTERACTION }, diff --git a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/__tests__/QTIItemEditor.spec.js b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/__tests__/QTIItemEditor.spec.js index 419f873f3f..dcff115371 100644 --- a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/__tests__/QTIItemEditor.spec.js +++ b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/__tests__/QTIItemEditor.spec.js @@ -7,7 +7,10 @@ import { AssessmentItemTypes } from '../../../constants'; import { VALID_CHOICE_ITEM_DOCUMENT, CHOICE_ITEM_DOCUMENT_NO_CORRECT_ANSWER, + CHOICE_ITEM_DOCUMENT_NO_CORRECT_ANSWER_WITH_STIMULUS, ORDERING_ITEM_DOCUMENT_NO_PROMPT, + CHOICE_ITEM_DOCUMENT_WITH_HINTS_AND_STIMULUS, + CHOICE_ITEM_DOCUMENT_WITH_STIMULUS, FREE_RESPONSE_ITEM_DOCUMENT, NO_INTERACTION_ITEM_DOCUMENT, CHOICE_ITEM_DOCUMENT_WITH_HINTS, @@ -16,6 +19,8 @@ import { MATCH_THREE_SETS_XML, MATCH_XML, MULTI_TEXT_ENTRY_ITEM_DOCUMENT, + TEXT_ENTRY_ITEM_DOCUMENT_SHARED_PARAGRAPH, + TEXT_ENTRY_ITEM_DOCUMENT_TRAILING_CONTENT, MULTI_INTERACTION_ITEM_DOCUMENT, UNRECOGNIZED_INTERACTION_ITEM_DOCUMENT, INLINE_CHOICE_ITEM_DOCUMENT, @@ -238,13 +243,18 @@ describe('QTIItemEditor', () => { expect(screen.getByText(unsupportedItemMessage$())).toBeInTheDocument(); }); - describe('items without exactly one editable interaction', () => { + describe('items the editor cannot rebuild', () => { const documents = { 'two interactions': MULTI_INTERACTION_ITEM_DOCUMENT, 'an interaction with no descriptor': UNRECOGNIZED_INTERACTION_ITEM_DOCUMENT, 'an interaction with no editor': INLINE_CHOICE_ITEM_DOCUMENT, 'no interaction': NO_INTERACTION_ITEM_DOCUMENT, 'several blanks in one text entry': MULTI_TEXT_ENTRY_ITEM_DOCUMENT, + 'a stimulus beside a block interaction': CHOICE_ITEM_DOCUMENT_WITH_STIMULUS, + 'a stimulus beside a hinted block interaction': + CHOICE_ITEM_DOCUMENT_WITH_HINTS_AND_STIMULUS, + 'text sharing the text entry paragraph': TEXT_ENTRY_ITEM_DOCUMENT_SHARED_PARAGRAPH, + 'content after the text entry paragraph': TEXT_ENTRY_ITEM_DOCUMENT_TRAILING_CONTENT, }; const renderDocument = (raw_data, props = {}) => renderComponent({ @@ -255,7 +265,9 @@ describe('QTIItemEditor', () => { describe.each(Object.entries(documents))('with %s', (_, raw_data) => { test('shows the unsupported message', () => { renderDocument(raw_data, { mode: 'view' }); - expect(screen.getByText(unsupportedItemMessage$())).toBeInTheDocument(); + expect(screen.getByTestId('unsupportedMessage')).toHaveTextContent( + unsupportedItemMessage$(), + ); }); test('offers no editable controls or hints in edit mode', () => { @@ -269,18 +281,16 @@ describe('QTIItemEditor', () => { expect(screen.queryByRole('combobox')).not.toBeInTheDocument(); expect(screen.queryAllByRole('textbox').filter(isAuthorInput)).toHaveLength(0); }); - }); - test('leaves raw_data untouched after the card is opened and closed', async () => { - const { emitted, updateProps } = renderDocument(MULTI_INTERACTION_ITEM_DOCUMENT, { - mode: 'edit', - }); + test('leaves raw_data untouched after the card is opened and closed', async () => { + const { emitted, updateProps } = renderDocument(raw_data, { mode: 'edit' }); - await fireEvent.click(screen.getByRole('button', { name: closeBtnLabel$() })); - await updateProps({ mode: 'view' }); - await nextTick(); + await fireEvent.click(screen.getByRole('button', { name: closeBtnLabel$() })); + await updateProps({ mode: 'view' }); + await nextTick(); - expect(emitted()['update:rawData']).toBeUndefined(); + expect(emitted()['update:rawData']).toBeUndefined(); + }); }); test('shows the hints of an unsupported item when answers are shown', () => { @@ -330,6 +340,18 @@ describe('QTIItemEditor', () => { expect(screen.getByText(incompleteItemIndicatorLabel$())).toBeInTheDocument(); }); + test('is shown for an invalid item whose body the editor cannot reproduce', async () => { + await renderAndValidate(CHOICE_ITEM_DOCUMENT_NO_CORRECT_ANSWER_WITH_STIMULUS); + expect(screen.getByTestId('incompleteIndicator')).toHaveTextContent( + incompleteItemIndicatorLabel$(), + ); + }); + + test('is not shown for a valid item whose body the editor cannot reproduce', async () => { + await renderAndValidate(CHOICE_ITEM_DOCUMENT_WITH_STIMULUS); + expect(screen.queryByTestId('incompleteIndicator')).not.toBeInTheDocument(); + }); + test('is not shown for a valid item with unsupported interactions', async () => { await renderAndValidate(MULTI_INTERACTION_ITEM_DOCUMENT); expect(screen.queryByText(incompleteItemIndicatorLabel$())).not.toBeInTheDocument(); diff --git a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/index.vue b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/index.vue index edb1017d5a..c83e867d28 100644 --- a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/index.vue +++ b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/index.vue @@ -148,20 +148,22 @@ // Parse the item XML. rawData is a computed inside useQtiItem that // re-assembles the full XML whenever identifier/title/language or the // editor refs change — no need to duplicate assembleItemXml here. - const { interactions, hints, parseError, rawData } = useQtiItem(props.item.raw_data, { - bodyXml: currentBodyXml, - responseDeclarations: currentResponseDeclarations, - }); + const { interactions, itemBodyXml, hints, parseError, rawData } = useQtiItem( + props.item.raw_data, + { bodyXml: currentBodyXml, responseDeclarations: currentResponseDeclarations }, + ); const isQti = computed(() => props.item.type === AssessmentItemTypes.QTI); /** * Whether this editor can edit the item's XML faithfully: it is readable, and it is - * either blank or holds exactly one interaction this editor knows. + * either blank or holds exactly one interaction this editor knows, in the body shape its + * builder writes. */ const isBlank = !props.item.raw_data; const isEditableQti = computed( - () => !parseError.value && (isBlank || isSupportedItem(interactions.value)), + () => + !parseError.value && (isBlank || isSupportedItem(interactions.value, itemBodyXml.value)), ); /** @@ -321,8 +323,9 @@ */ const isIncomplete = computed(() => { if (isUnsupported.value) { - // Unreadable or empty QTI can't be fixed here and blocks publishing; other - // unsupported items are publishable. + // Unreadable or empty QTI can't be fixed here and blocks publishing. So does + // an item unsupported only for its body shape that breaks the editor's rules; + // other unsupported items are publishable. return ( isQti.value && validateQtiItem(props.item.raw_data, { allowFreeResponse: props.allowFreeResponse }) diff --git a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/composables/useQtiItem.js b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/composables/useQtiItem.js index 63847bec11..5ac183c70a 100644 --- a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/composables/useQtiItem.js +++ b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/composables/useQtiItem.js @@ -17,6 +17,7 @@ import { assembleItemXml } from '../serialization/assembleItem'; * identifier: import('vue').Ref, * title: import('vue').Ref, * language: import('vue').Ref, + * itemBodyXml: import('vue').Ref, * interactions: import('vue').Ref>, * hints: import('vue').Ref>, * parseError: import('vue').Ref, @@ -27,6 +28,7 @@ export default function useQtiItem(rawXml, { bodyXml, responseDeclarations } = { const identifier = ref(''); const title = ref(''); const language = ref(''); + const itemBodyXml = ref(''); const interactions = ref([]); /** * Hints belong to the item, not to any one interaction, so they live here beside @@ -41,6 +43,7 @@ export default function useQtiItem(rawXml, { bodyXml, responseDeclarations } = { identifier.value = model.identifier; title.value = model.title; language.value = model.language; + itemBodyXml.value = model.itemBodyXml; interactions.value = model.interactions; hints.value = model.hints; } catch (e) { @@ -68,6 +71,7 @@ export default function useQtiItem(rawXml, { bodyXml, responseDeclarations } = { identifier, title, language, + itemBodyXml, interactions, hints, parseError, diff --git a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/InteractionDescriptor.js b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/InteractionDescriptor.js index 271e0b5ead..3cce104b50 100644 --- a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/InteractionDescriptor.js +++ b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/InteractionDescriptor.js @@ -11,6 +11,7 @@ */ import { Placement } from '../constants'; +import { isContentNode } from '../serialization/xml'; /** * Methods a subclass has to implement. `matches` and `getTypeOptions` are not listed @@ -69,6 +70,18 @@ export class InteractionDescriptor { return el.tagName.toLowerCase() === this.type; } + /** + * Whether the item body has the shape this interaction's builder writes, so that editing + * and rebuilding it loses nothing. The default is the block shape: the interaction alone. + * + * @param {Element} bodyEl + * @returns {boolean} + */ + isSupportedBody(bodyEl) { + const content = [...bodyEl.childNodes].filter(isContentNode); + return content.length === 1 && this.matches(content[0]); + } + /** * Options this interaction contributes to the question type selector. An interaction that * authors are not meant to pick directly contributes none. diff --git a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/__tests__/resolveDescriptor.spec.js b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/__tests__/resolveDescriptor.spec.js index 0b67080ee0..517512891f 100644 --- a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/__tests__/resolveDescriptor.spec.js +++ b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/__tests__/resolveDescriptor.spec.js @@ -1,22 +1,54 @@ import { parseItem } from '../../serialization/parseItem'; +import { assembleItemXml } from '../../serialization/assembleItem'; import { QtiInteraction, ValidationError } from '../../constants'; import { isSupportedItem, resolveDescriptor } from '../resolveDescriptor'; import { + CHOICE_ITEM_DOCUMENT_WITH_STIMULUS, CHOICE_SINGLE_SELECT_XML, + FREE_RESPONSE_ITEM_DOCUMENT, INLINE_CHOICE_ITEM_DOCUMENT, MATCH_THREE_SETS_XML, MULTI_INTERACTION_ITEM_DOCUMENT, MULTI_TEXT_ENTRY_ITEM_DOCUMENT, + ORDERING_ITEM_DOCUMENT_NO_PROMPT, + TEXT_ENTRY_ITEM_DOCUMENT_SHARED_PARAGRAPH, + TEXT_ENTRY_ITEM_DOCUMENT_TRAILING_CONTENT, TWO_INTERACTIONS_DOCUMENT, UNDESCRIBED_INTERACTION, UNRECOGNIZED_INTERACTION_ITEM_DOCUMENT, + VALID_ASSOCIATE_ITEM_DOCUMENT, VALID_CHOICE_ITEM_DOCUMENT, VALID_MATCH_ITEM_DOCUMENT, } from '../../utils/testingFixtures'; const UNDESCRIBED_XML = `<${UNDESCRIBED_INTERACTION} response-identifier="RESPONSE"/>`; +const TEXT_ENTRY_PARAGRAPH = '

'; const interactionsOf = document => parseItem(document).interactions; +const isSupported = document => { + const { interactions, itemBodyXml } = parseItem(document); + return isSupportedItem(interactions, itemBodyXml); +}; +const withBody = (document, body) => + document.replace( + /[\s\S]*<\/qti-item-body>/, + `${body}`, + ); +const CONVERTED_TEXT_ENTRY_DOCUMENT = withBody( + FREE_RESPONSE_ITEM_DOCUMENT, + `

Q?

${TEXT_ENTRY_PARAGRAPH}
`, +); + +const save = document => { + const item = parseItem(document); + const [{ bodyXml, responseDeclarations }] = item.interactions; + const { descriptor, questionType } = resolveDescriptor(bodyXml, responseDeclarations); + const interaction = descriptor.buildXML( + descriptor.parse(bodyXml, responseDeclarations), + questionType, + ); + return assembleItemXml({ ...item, ...interaction }); +}; describe('resolveDescriptor', () => { it('resolves no descriptor for an interaction nothing is registered for', () => { @@ -70,7 +102,192 @@ describe('isSupportedItem', () => { it.each([ ['one choice interaction', VALID_CHOICE_ITEM_DOCUMENT], ['a match interaction', VALID_MATCH_ITEM_DOCUMENT], + ['a text entry after its prompt', FREE_RESPONSE_ITEM_DOCUMENT], ])('accepts %s', (_, document) => { - expect(isSupportedItem(interactionsOf(document))).toBe(true); + expect(isSupported(document)).toBe(true); + }); + + describe('body content beside a block interaction', () => { + it.each([ + ['a stimulus before it', CHOICE_ITEM_DOCUMENT_WITH_STIMULUS], + [ + 'text after it', + VALID_CHOICE_ITEM_DOCUMENT.replace('', 'See below.'), + ], + [ + 'CDATA text before it', + VALID_CHOICE_ITEM_DOCUMENT.replace( + '', + '', + ), + ], + [ + 'an image after it', + VALID_CHOICE_ITEM_DOCUMENT.replace( + '', + '', + ), + ], + ])('rejects %s', (_, document) => { + expect(isSupported(document)).toBe(false); + }); + + it('rejects an interaction inside a wrapper element', () => { + const [{ bodyXml }] = interactionsOf(VALID_CHOICE_ITEM_DOCUMENT); + expect(isSupported(withBody(VALID_CHOICE_ITEM_DOCUMENT, `
${bodyXml}
`))).toBe( + false, + ); + }); + + it('rejects a no-break space beside it', () => { + expect( + isSupported(VALID_CHOICE_ITEM_DOCUMENT.replace('', ' ')), + ).toBe(false); + }); + + it('accepts whitespace and comments around it', () => { + const [{ bodyXml }] = interactionsOf(VALID_CHOICE_ITEM_DOCUMENT); + expect( + isSupported(withBody(VALID_CHOICE_ITEM_DOCUMENT, `\n \n${bodyXml}\n`)), + ).toBe(true); + }); + }); + + describe('body content around a text entry', () => { + it.each([ + ['a paragraph shared with other content', TEXT_ENTRY_ITEM_DOCUMENT_SHARED_PARAGRAPH], + [ + 'a paragraph shared with CDATA text', + FREE_RESPONSE_ITEM_DOCUMENT.replace( + '

', '/>

More.

'), + ], + [ + 'content after the wrapper of its paragraph', + CONVERTED_TEXT_ENTRY_DOCUMENT.replace('/>

', '/>

More.

'), + ], + [ + 'no paragraph of its own', + FREE_RESPONSE_ITEM_DOCUMENT.replace( + /

]*)\/><\/p>/, + '', + ), + ], + [ + 'a paragraph with an attribute', + FREE_RESPONSE_ITEM_DOCUMENT.replace( + '

Q?${TEXT_ENTRY_PARAGRAPH}`, + ), + ], + [ + 'a paragraph inside a list', + withBody( + FREE_RESPONSE_ITEM_DOCUMENT, + `

  1. Q?
  2. ${TEXT_ENTRY_PARAGRAPH}
`, + ), + ], + [ + 'a paragraph inside a wrapper with an attribute', + CONVERTED_TEXT_ENTRY_DOCUMENT.replace('
', '
'), + ], + [ + 'a wrapper after the prompt', + withBody(FREE_RESPONSE_ITEM_DOCUMENT, `

Q?

${TEXT_ENTRY_PARAGRAPH}
`), + ], + ])('rejects %s', (_, document) => { + expect(isSupported(document)).toBe(false); + }); + + it.each([ + [ + 'its paragraph', + FREE_RESPONSE_ITEM_DOCUMENT.replace( + '

', + '

', + ), + ], + ])('accepts namespace declarations on %s', (_, document) => { + expect(isSupported(document)).toBe(true); + }); + }); + + describe('attributes on the item body', () => { + it.each([ + ['dir', 'dir="rtl"'], + ['xml:lang', 'xml:lang="ar"'], + ['class', 'class="x"'], + ])('rejects a choice item whose body has %s', (_, attribute) => { + expect( + isSupported( + VALID_CHOICE_ITEM_DOCUMENT.replace('', ``), + ), + ).toBe(false); + }); + + it('rejects a text entry item whose body has an attribute', () => { + expect( + isSupported( + FREE_RESPONSE_ITEM_DOCUMENT.replace('', ''), + ), + ).toBe(false); + }); + + it('accepts namespace declarations on the body', () => { + expect( + isSupported( + VALID_CHOICE_ITEM_DOCUMENT.replace( + '', + '', + ), + ), + ).toBe(true); + }); + }); + + describe('an item the editor has saved', () => { + const textEntryAfter = prompt => + withBody(FREE_RESPONSE_ITEM_DOCUMENT, `${prompt}${TEXT_ENTRY_PARAGRAPH}`); + + it.each([ + ['a choice interaction', VALID_CHOICE_ITEM_DOCUMENT], + ['an ordering interaction', ORDERING_ITEM_DOCUMENT_NO_PROMPT], + ['an associate interaction', VALID_ASSOCIATE_ITEM_DOCUMENT], + ['a match interaction', VALID_MATCH_ITEM_DOCUMENT], + ['a text entry after a plain prompt', FREE_RESPONSE_ITEM_DOCUMENT], + ['a text entry with no prompt', textEntryAfter('')], + ['a text entry inside the converted wrapper', CONVERTED_TEXT_ENTRY_DOCUMENT], + ['a text entry after an image', textEntryAfter('

')], + [ + 'a text entry after MathML', + textEntryAfter('

x

'), + ], + ['a text entry after a right-to-left prompt', textEntryAfter('

Q?

')], + ['a text entry after a list', textEntryAfter('
  • One
  • Two
')], + ])('stays editable with %s, after one save and two', (_, document) => { + const once = save(document); + expect(isSupported(once)).toBe(true); + expect(isSupported(save(once))).toBe(true); + }); }); }); diff --git a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/resolveDescriptor.js b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/resolveDescriptor.js index de5a8aa17b..c9fe88e186 100644 --- a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/resolveDescriptor.js +++ b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/resolveDescriptor.js @@ -1,4 +1,4 @@ -import { parseXML } from '../serialization/xml'; +import { hasNonNamespaceAttributes, parseXML } from '../serialization/xml'; import { ValidationError } from '../constants'; import { HEADLESS_INTERACTIONS, SINGLE_INSTANCE_INTERACTIONS, descriptors } from './descriptors'; @@ -39,25 +39,58 @@ export function resolveDescriptor(bodyXml, responseDeclarations) { } /** - * Whether the editor can edit an item faithfully: it holds exactly one interaction block - * (one interaction, for descriptors in SINGLE_INSTANCE_INTERACTIONS), and a descriptor + * The descriptor that can edit an item's interaction: the item holds exactly one interaction + * block (one interaction, for descriptors in SINGLE_INSTANCE_INTERACTIONS), and a descriptor * with an editor exists for it that could read the interaction. * - * @param {Array<{ bodyXml: string, responseDeclarations: string[] }>} interactions - As - * returned by parseItem - * @returns {boolean} + * @param {Array<{ bodyXml: string, responseDeclarations: string[] }>} interactions + * @returns {object|null} */ -export function isSupportedItem(interactions) { +function editableDescriptor(interactions) { if (interactions.length !== 1) { - return false; + return null; } const [{ bodyXml, responseDeclarations }] = interactions; const { descriptor, error } = resolveDescriptor(bodyXml, responseDeclarations); if (descriptor === null || error || HEADLESS_INTERACTIONS.includes(descriptor.type)) { + return null; + } + if ( + SINGLE_INSTANCE_INTERACTIONS.includes(descriptor.type) && + parseXML(bodyXml).getElementsByTagName(descriptor.type).length > 1 + ) { + return null; + } + return descriptor; +} + +/** + * Whether the editor's rules apply to an item's interaction, whatever surrounds it in the + * body: see editableDescriptor. + * + * @param {Array<{ bodyXml: string, responseDeclarations: string[] }>} interactions - As + * returned by parseItem + * @returns {boolean} + */ +export function isSupportedInteraction(interactions) { + return editableDescriptor(interactions) !== null; +} + +/** + * Whether the editor can edit an item faithfully: its interaction is supported, and the body + * has the shape that interaction's builder writes. + * + * @param {Array<{ bodyXml: string, responseDeclarations: string[] }>} interactions - As + * returned by parseItem + * @param {string} itemBodyXml - The item's ``, as returned by parseItem + * @returns {boolean} + */ +export function isSupportedItem(interactions, itemBodyXml) { + const descriptor = editableDescriptor(interactions); + if (descriptor === null) { return false; } - return ( - !SINGLE_INSTANCE_INTERACTIONS.includes(descriptor.type) || - parseXML(bodyXml).getElementsByTagName(descriptor.type).length <= 1 - ); + const bodyEl = parseXML(itemBodyXml).documentElement; + // The builders write a bare , so any attribute would be dropped. + return !hasNonNamespaceAttributes(bodyEl) && descriptor.isSupportedBody(bodyEl); } diff --git a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/textEntry/Descriptor.js b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/textEntry/Descriptor.js index 934189eea7..1bc64c7100 100644 --- a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/textEntry/Descriptor.js +++ b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/textEntry/Descriptor.js @@ -1,9 +1,22 @@ import { QtiInteraction, QuestionType, BaseType, Cardinality, Placement } from '../../constants'; -import { parseXML } from '../../serialization/xml'; +import { hasNonNamespaceAttributes, isContentNode, parseXML } from '../../serialization/xml'; import { InteractionDescriptor } from '../InteractionDescriptor'; import { parseTextEntryInteraction, buildTextEntryInteractionXML } from './parse'; import { validateTextEntryInteraction } from './validation'; +function contentOf(el) { + return [...el.childNodes].filter(isContentNode); +} + +function hasContentAfter(node) { + for (let sibling = node.nextSibling; sibling; sibling = sibling.nextSibling) { + if (isContentNode(sibling)) { + return true; + } + } + return false; +} + /** * Owns all text-entry-specific interaction logic: schema, parse, buildXML, validate. * @@ -46,6 +59,34 @@ class TextEntryInteractionDescriptor extends InteractionDescriptor { return !!el.querySelector(QtiInteraction.TEXT_ENTRY); } + /** + * The builder writes the prompt, then a `

` holding only the interaction. Anything else + * would be dropped from the interaction's `

`, or moved ahead of it if it follows. + * Legacy conversion wraps the same shape in a bare `

`, the body's only content. + * + * @param {Element} bodyEl + * @returns {boolean} + */ + isSupportedBody(bodyEl) { + const paragraph = bodyEl.querySelector(this.type).parentElement; + if ( + paragraph.localName !== 'p' || + hasNonNamespaceAttributes(paragraph) || + contentOf(paragraph).length > 1 || + hasContentAfter(paragraph) + ) { + return false; + } + const container = paragraph.parentElement; + return ( + container === bodyEl || + (container.localName === 'div' && + !hasNonNamespaceAttributes(container) && + container.parentElement === bodyEl && + contentOf(bodyEl).length === 1) + ); + } + /** * Reads base-type from the response declaration to determine question type. * diff --git a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/serialization/__tests__/convertedItem.spec.js b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/serialization/__tests__/convertedItem.spec.js index 271700c0f6..9881ac94e1 100644 --- a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/serialization/__tests__/convertedItem.spec.js +++ b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/serialization/__tests__/convertedItem.spec.js @@ -13,6 +13,7 @@ import fs from 'fs'; import path from 'path'; import { parseItem } from '../parseItem'; import { assembleItemXml } from '../assembleItem'; +import { isSupportedItem } from '../../interactions/resolveDescriptor'; const FIXTURES = path.join(__dirname, '../../../../../../tests/utils/qti/fixtures'); @@ -28,6 +29,18 @@ const rebuild = item => hints: item.hints, }); +describe('converted items the editor opens', () => { + it.each( + fs + .readdirSync(FIXTURES) + .filter(file => file.endsWith('.xml')) + .map(file => path.basename(file, '.xml')), + )('%s', name => { + const { interactions, itemBodyXml } = parseItem(read(name)); + expect(isSupportedItem(interactions, itemBodyXml)).toBe(true); + }); +}); + describe('a converted single-selection item', () => { const original = read('single_selection'); diff --git a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/serialization/xml.js b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/serialization/xml.js index 3bb4333cb6..fcedc997ae 100644 --- a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/serialization/xml.js +++ b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/serialization/xml.js @@ -107,6 +107,33 @@ function adoptNode(node, doc = xmlDoc, plainNamespace = XHTML_NS) { return el; } +const XMLNS_NS = 'http://www.w3.org/2000/xmlns/'; + +/** + * Whether an element has attributes beyond namespace declarations. + * + * @param {Element} el + * @returns {boolean} + */ +export function hasNonNamespaceAttributes(el) { + return [...el.attributes].some(attr => attr.namespaceURI !== XMLNS_NS); +} + +/** + * Whether a node is content: an element or text (CDATA included) beyond whitespace. Comments + * and the like carry none. + * + * @param {Node} node + * @returns {boolean} + */ +export function isContentNode(node) { + return ( + node.nodeType === Node.ELEMENT_NODE || + ((node.nodeType === Node.TEXT_NODE || node.nodeType === Node.CDATA_SECTION_NODE) && + /[^ \t\r\n]/.test(node.nodeValue)) + ); +} + /** * Serialize XML nodes as an HTML string, for state that a rich text editor parses as HTML. * diff --git a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/utils/testingFixtures.js b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/utils/testingFixtures.js index ccf4bfc825..99aa0a4769 100644 --- a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/utils/testingFixtures.js +++ b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/utils/testingFixtures.js @@ -241,6 +241,32 @@ export const FREE_RESPONSE_ITEM_DOCUMENT = ` `; +const TEXT_ENTRY_PARAGRAPH = '

'; + +/** A block interaction with a stimulus beside it, which the editor would delete. */ +export const CHOICE_ITEM_DOCUMENT_WITH_STIMULUS = VALID_CHOICE_ITEM_DOCUMENT.replace( + '', + '

Read the passage.

', +); + +/** An incomplete item the editor cannot reproduce, which validation still reports. */ +export const CHOICE_ITEM_DOCUMENT_NO_CORRECT_ANSWER_WITH_STIMULUS = + CHOICE_ITEM_DOCUMENT_NO_CORRECT_ANSWER.replace( + '', + '

Read the passage.

', + ); + +export const TEXT_ENTRY_ITEM_DOCUMENT_SHARED_PARAGRAPH = FREE_RESPONSE_ITEM_DOCUMENT.replace( + TEXT_ENTRY_PARAGRAPH, + '

Answer: km

', +); + +/** Content after the interaction's `

`, which the editor would move before it. */ +export const TEXT_ENTRY_ITEM_DOCUMENT_TRAILING_CONTENT = FREE_RESPONSE_ITEM_DOCUMENT.replace( + '', + '

Show your working.

', +); + export const NO_INTERACTION_ITEM_DOCUMENT = ` } Empty when the item is valid. Items the - * editor shows read-only report only unreadable XML or a missing interaction. + * @returns {Array<{ code: string, id?: string }>} Empty when the item is valid. Items whose + * interactions the editor can't edit report only unreadable XML or a missing interaction. */ export function validateQtiItem(rawData, { allowFreeResponse = true } = {}) { if (!rawData) { @@ -60,8 +60,9 @@ export function validateQtiItem(rawData, { allowFreeResponse = true } = {}) { ...resolveDescriptor(interaction.bodyXml, interaction.responseDeclarations), })); - if (item.interactions.length && !isSupportedItem(item.interactions)) { - // Shown read-only: the editor's rules don't apply, only unreadable interactions count. + if (item.interactions.length && !isSupportedInteraction(item.interactions)) { + // The editor's rules don't apply, only unreadable interactions count. Body shape alone + // doesn't exempt an item: its interaction is still one those rules cover. return resolved.filter(({ error }) => error).map(({ error }) => ({ code: error })); } From 3895ab84dcd114eb1eba0b902c84e60007a59373 Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Tue, 6 Oct 2026 23:15:41 +0000 Subject: [PATCH 2/2] fix: exempt QTI items the editor can't rebuild from its rules - validateQtiItem uses isSupportedItem, so read-only items aren't left Incomplete - Move the text entry body-shape check into parse.js Co-Authored-By: Claude Opus 5.5 --- .../QTIEditor/__tests__/validateItem.spec.js | 14 ++--- .../__tests__/QTIItemEditor.spec.js | 9 +--- .../components/QTIItemEditor/index.vue | 5 +- .../interactions/resolveDescriptor.js | 12 ----- .../interactions/textEntry/Descriptor.js | 48 +++-------------- .../QTIEditor/interactions/textEntry/parse.js | 51 ++++++++++++++++++- .../views/QTIEditor/utils/testingFixtures.js | 2 +- .../shared/views/QTIEditor/validateItem.js | 11 ++-- 8 files changed, 74 insertions(+), 78 deletions(-) diff --git a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/__tests__/validateItem.spec.js b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/__tests__/validateItem.spec.js index c0084db52b..845b984f33 100644 --- a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/__tests__/validateItem.spec.js +++ b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/__tests__/validateItem.spec.js @@ -43,13 +43,13 @@ describe('validateQtiItem', () => { expect(validateQtiItem(xml)).toEqual([]); }); - it('applies editor rules to an item whose body the editor cannot reproduce', () => { - expect(validateQtiItem(CHOICE_ITEM_DOCUMENT_NO_CORRECT_ANSWER_WITH_STIMULUS)).toEqual([ - { code: ValidationError.NO_CORRECT_ANSWER }, - ]); - expect( - validateQtiItem(TEXT_ENTRY_ITEM_DOCUMENT_SHARED_PARAGRAPH, { allowFreeResponse: false }), - ).toEqual([{ code: ValidationError.FREE_RESPONSE_NOT_ALLOWED }]); + it('does not apply editor rules to an item whose body the editor cannot reproduce', () => { + const noPrompt = TEXT_ENTRY_ITEM_DOCUMENT_SHARED_PARAGRAPH.replace( + '

Tell us what you think.

', + '', + ); + expect(validateQtiItem(CHOICE_ITEM_DOCUMENT_NO_CORRECT_ANSWER_WITH_STIMULUS)).toEqual([]); + expect(validateQtiItem(noPrompt, { allowFreeResponse: false })).toEqual([]); }); it('reports an item whose body holds no interaction', () => { diff --git a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/__tests__/QTIItemEditor.spec.js b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/__tests__/QTIItemEditor.spec.js index dcff115371..661e0d7712 100644 --- a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/__tests__/QTIItemEditor.spec.js +++ b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/__tests__/QTIItemEditor.spec.js @@ -340,15 +340,8 @@ describe('QTIItemEditor', () => { expect(screen.getByText(incompleteItemIndicatorLabel$())).toBeInTheDocument(); }); - test('is shown for an invalid item whose body the editor cannot reproduce', async () => { + test('is not shown for an item whose body the editor cannot reproduce', async () => { await renderAndValidate(CHOICE_ITEM_DOCUMENT_NO_CORRECT_ANSWER_WITH_STIMULUS); - expect(screen.getByTestId('incompleteIndicator')).toHaveTextContent( - incompleteItemIndicatorLabel$(), - ); - }); - - test('is not shown for a valid item whose body the editor cannot reproduce', async () => { - await renderAndValidate(CHOICE_ITEM_DOCUMENT_WITH_STIMULUS); expect(screen.queryByTestId('incompleteIndicator')).not.toBeInTheDocument(); }); diff --git a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/index.vue b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/index.vue index c83e867d28..167df94589 100644 --- a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/index.vue +++ b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/components/QTIItemEditor/index.vue @@ -323,9 +323,8 @@ */ const isIncomplete = computed(() => { if (isUnsupported.value) { - // Unreadable or empty QTI can't be fixed here and blocks publishing. So does - // an item unsupported only for its body shape that breaks the editor's rules; - // other unsupported items are publishable. + // Unreadable or empty QTI can't be fixed here and blocks publishing; other + // unsupported items are publishable. return ( isQti.value && validateQtiItem(props.item.raw_data, { allowFreeResponse: props.allowFreeResponse }) diff --git a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/resolveDescriptor.js b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/resolveDescriptor.js index c9fe88e186..49f02599e8 100644 --- a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/resolveDescriptor.js +++ b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/resolveDescriptor.js @@ -64,18 +64,6 @@ function editableDescriptor(interactions) { return descriptor; } -/** - * Whether the editor's rules apply to an item's interaction, whatever surrounds it in the - * body: see editableDescriptor. - * - * @param {Array<{ bodyXml: string, responseDeclarations: string[] }>} interactions - As - * returned by parseItem - * @returns {boolean} - */ -export function isSupportedInteraction(interactions) { - return editableDescriptor(interactions) !== null; -} - /** * Whether the editor can edit an item faithfully: its interaction is supported, and the body * has the shape that interaction's builder writes. diff --git a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/textEntry/Descriptor.js b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/textEntry/Descriptor.js index 1bc64c7100..f411f39f74 100644 --- a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/textEntry/Descriptor.js +++ b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/textEntry/Descriptor.js @@ -1,22 +1,13 @@ import { QtiInteraction, QuestionType, BaseType, Cardinality, Placement } from '../../constants'; -import { hasNonNamespaceAttributes, isContentNode, parseXML } from '../../serialization/xml'; +import { parseXML } from '../../serialization/xml'; import { InteractionDescriptor } from '../InteractionDescriptor'; -import { parseTextEntryInteraction, buildTextEntryInteractionXML } from './parse'; +import { + parseTextEntryInteraction, + buildTextEntryInteractionXML, + isSupportedTextEntryBody, +} from './parse'; import { validateTextEntryInteraction } from './validation'; -function contentOf(el) { - return [...el.childNodes].filter(isContentNode); -} - -function hasContentAfter(node) { - for (let sibling = node.nextSibling; sibling; sibling = sibling.nextSibling) { - if (isContentNode(sibling)) { - return true; - } - } - return false; -} - /** * Owns all text-entry-specific interaction logic: schema, parse, buildXML, validate. * @@ -59,32 +50,9 @@ class TextEntryInteractionDescriptor extends InteractionDescriptor { return !!el.querySelector(QtiInteraction.TEXT_ENTRY); } - /** - * The builder writes the prompt, then a `

` holding only the interaction. Anything else - * would be dropped from the interaction's `

`, or moved ahead of it if it follows. - * Legacy conversion wraps the same shape in a bare `

`, the body's only content. - * - * @param {Element} bodyEl - * @returns {boolean} - */ + /** @param {Element} bodyEl */ isSupportedBody(bodyEl) { - const paragraph = bodyEl.querySelector(this.type).parentElement; - if ( - paragraph.localName !== 'p' || - hasNonNamespaceAttributes(paragraph) || - contentOf(paragraph).length > 1 || - hasContentAfter(paragraph) - ) { - return false; - } - const container = paragraph.parentElement; - return ( - container === bodyEl || - (container.localName === 'div' && - !hasNonNamespaceAttributes(container) && - container.parentElement === bodyEl && - contentOf(bodyEl).length === 1) - ); + return isSupportedTextEntryBody(bodyEl); } /** diff --git a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/textEntry/parse.js b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/textEntry/parse.js index 25bcf89cde..af520ec309 100644 --- a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/textEntry/parse.js +++ b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/interactions/textEntry/parse.js @@ -1,5 +1,12 @@ import { QTIDeclaration } from '../../serialization/qti/QTIDeclaration'; -import { buildXmlNode, parseXML, serializeAsHtml, wrapInlineRuns } from '../../serialization/xml'; +import { + buildXmlNode, + hasNonNamespaceAttributes, + isContentNode, + parseXML, + serializeAsHtml, + wrapInlineRuns, +} from '../../serialization/xml'; import CorrectResponse from '../../serialization/qti/declarations/correctResponse'; import Mapping from '../../serialization/qti/declarations/mapping'; import { generateRandomSlug } from '../../utils/generateRandomSlug'; @@ -44,6 +51,48 @@ export function _defaultState() { }; } +function contentOf(el) { + return [...el.childNodes].filter(isContentNode); +} + +function hasContentAfter(node) { + for (let sibling = node.nextSibling; sibling; sibling = sibling.nextSibling) { + if (isContentNode(sibling)) { + return true; + } + } + return false; +} + +/** + * Whether the item body has the shape buildTextEntryInteractionXML writes: the prompt, then a + * `

` holding only the interaction. Anything else would be dropped from the interaction's + * `

`, or moved ahead of it if it follows. Legacy conversion wraps the same shape in a bare + * `

`, the body's only content. + * + * @param {Element} bodyEl - The `` element + * @returns {boolean} + */ +export function isSupportedTextEntryBody(bodyEl) { + const paragraph = bodyEl.querySelector('qti-text-entry-interaction').parentElement; + if ( + paragraph.localName !== 'p' || + hasNonNamespaceAttributes(paragraph) || + contentOf(paragraph).length > 1 || + hasContentAfter(paragraph) + ) { + return false; + } + const container = paragraph.parentElement; + return ( + container === bodyEl || + (container.localName === 'div' && + !hasNonNamespaceAttributes(container) && + container.parentElement === bodyEl && + contentOf(bodyEl).length === 1) + ); +} + /** * Serializes the body element children to an HTML string, excluding the * element that directly contains the ``. diff --git a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/utils/testingFixtures.js b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/utils/testingFixtures.js index 99aa0a4769..4096e3684e 100644 --- a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/utils/testingFixtures.js +++ b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/utils/testingFixtures.js @@ -249,7 +249,7 @@ export const CHOICE_ITEM_DOCUMENT_WITH_STIMULUS = VALID_CHOICE_ITEM_DOCUMENT.rep '

Read the passage.

', ); -/** An incomplete item the editor cannot reproduce, which validation still reports. */ +/** An item the editor cannot reproduce, which would break its rules if they applied. */ export const CHOICE_ITEM_DOCUMENT_NO_CORRECT_ANSWER_WITH_STIMULUS = CHOICE_ITEM_DOCUMENT_NO_CORRECT_ANSWER.replace( '', diff --git a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/validateItem.js b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/validateItem.js index 862b708502..4dfad5ecc5 100644 --- a/contentcuration/contentcuration/frontend/shared/views/QTIEditor/validateItem.js +++ b/contentcuration/contentcuration/frontend/shared/views/QTIEditor/validateItem.js @@ -1,6 +1,6 @@ import { QuestionType, ValidationError } from './constants'; import { parseItem } from './serialization/parseItem'; -import { isSupportedInteraction, resolveDescriptor } from './interactions/resolveDescriptor'; +import { isSupportedItem, resolveDescriptor } from './interactions/resolveDescriptor'; /** * Validate what is wrong with an item as a whole, rather than with one of its interactions: @@ -40,8 +40,8 @@ export function validateItemShape({ interactions, questionTypes = [], allowFreeR * @param {object} [options] * @param {boolean} [options.allowFreeResponse] - Whether a free-response question counts * as valid. Consumers that only accept scorable questions pass false. - * @returns {Array<{ code: string, id?: string }>} Empty when the item is valid. Items whose - * interactions the editor can't edit report only unreadable XML or a missing interaction. + * @returns {Array<{ code: string, id?: string }>} Empty when the item is valid. Items the + * editor shows read-only report only unreadable XML or a missing interaction. */ export function validateQtiItem(rawData, { allowFreeResponse = true } = {}) { if (!rawData) { @@ -60,9 +60,8 @@ export function validateQtiItem(rawData, { allowFreeResponse = true } = {}) { ...resolveDescriptor(interaction.bodyXml, interaction.responseDeclarations), })); - if (item.interactions.length && !isSupportedInteraction(item.interactions)) { - // The editor's rules don't apply, only unreadable interactions count. Body shape alone - // doesn't exempt an item: its interaction is still one those rules cover. + if (item.interactions.length && !isSupportedItem(item.interactions, item.itemBodyXml)) { + // Shown read-only: the editor's rules don't apply, only unreadable interactions count. return resolved.filter(({ error }) => error).map(({ error }) => ({ code: error })); }