feat: implement QTI declaration serialization framework and validation utilities#5984
feat: implement QTI declaration serialization framework and validation utilities#5984Abhishek-Punhani wants to merge 1 commit into
Conversation
…n utilities Signed-off-by: Abhishek-Punhani <punhani.manavabhi@gmail.com>
|
👋 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. |
|
@AlexVelezLl — I added a QTISanitizer class that wasn't in the original scope, but felt necessary. Since QTI XML is authored content that eventually gets serialized and consumed by backend and database. |
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
Looking great! I have added some questions/comments to make this a bit more flexible. Please let me know if you have any questions/suggestions :)
| const attrs = { | ||
| identifier: this.identifier, | ||
| cardinality: this.cardinality, | ||
| }; |
There was a problem hiding this comment.
Could we throw an error here if either identifier or cardinality is not defined or is not correct? The same if base-type is not a recognized base-type.
| static fromXML(xmlNode) { | ||
| const identifier = xmlNode.getAttribute('identifier') ?? ''; | ||
| const baseType = xmlNode.getAttribute('base-type') ?? null; | ||
| const cardinality = xmlNode.getAttribute('cardinality') ?? 'single'; |
There was a problem hiding this comment.
I think it'd be great to have some constants declarations for cardinality and base-type.
There was a problem hiding this comment.
I'm a little unsure about how to define the schemas per question type in this file. On one side, it'd be one additional place we should remember to edit when we create a new question type. Also, it comes at the cost of some flexibility, because if the cardinality is dynamic, then we will need to add an additional workaround for them. (For example, with text entry interactions, we will probably need to set cardinality='single' if there is just one response or 'multi' if there are many correct responses).
So, I'd say this logic should be deferred to the parse modules that each interaction is going to have. This way we keep it a bit more flexible, and it's one place less to change when we add a new interaction.
| convertTo(newQuestionType, { keepFirst = true } = {}) { | ||
| const schema = getSchemaForType(newQuestionType); | ||
| if (!schema) throw new Error(`Unknown question type: ${newQuestionType}`); | ||
|
|
||
| const newDecl = new QTIDeclaration({ | ||
| identifier: this.identifier, | ||
| baseType: schema.baseType, | ||
| cardinality: schema.cardinality, | ||
| tag: this.tag, | ||
| }); | ||
|
|
||
| const baseTypeChanged = schema.baseType !== this.baseType; | ||
|
|
||
| const currentCR = this.correctResponse; | ||
| if ( | ||
| currentCR !== null && | ||
| !baseTypeChanged && | ||
| schema.capabilities.includes(CAPABILITY.CORRECT_RESPONSE) | ||
| ) { | ||
| const migrated = migrateValues(currentCR, this.cardinality, schema.cardinality, keepFirst); | ||
| if (migrated.length > 0) { | ||
| CorrectResponse.fromPlain(migrated, newDecl); | ||
| } | ||
| } | ||
|
|
||
| const currentMapping = this.mapping; | ||
| if ( | ||
| currentMapping !== null && | ||
| !baseTypeChanged && | ||
| schema.capabilities.includes(CAPABILITY.MAPPING) | ||
| ) { | ||
| Mapping.fromPlain(currentMapping, newDecl); | ||
| } | ||
|
|
||
| const currentAreaMapping = this.areaMapping; | ||
| if ( | ||
| currentAreaMapping !== null && | ||
| !baseTypeChanged && | ||
| schema.capabilities.includes(CAPABILITY.AREA_MAPPING) | ||
| ) { | ||
| AreaMapping.fromPlain(currentAreaMapping, newDecl); | ||
| } | ||
|
|
||
| return newDecl; | ||
| } | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Module-private helpers | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * Migrate correctResponse values across a cardinality change. | ||
| * | ||
| * @param {string[]} values - Current values | ||
| * @param {string} fromCard - Source cardinality | ||
| * @param {string} toCard - Target cardinality | ||
| * @param {boolean} keepFirst - On downgrade to single, keep the first value | ||
| * @returns {string[]} | ||
| */ | ||
| function migrateValues(values, fromCard, toCard, keepFirst) { | ||
| const arr = Array.isArray(values) ? values : [values]; | ||
|
|
||
| if (toCard === 'single') { | ||
| return keepFirst && arr.length > 0 ? [arr[0]] : []; | ||
| } | ||
|
|
||
| // single → multiple/ordered, or multiple ↔ ordered: values transfer unchanged. | ||
| return arr; | ||
| } |
There was a problem hiding this comment.
I'm also a bit unsure about this. The general strategy to convert from question types will be to just drop the old descriptor and mount the new one with the same xml that was saved from the previous descriptor. If there are some parts of it that the new descriptor can recognize, it will use them (e.g., if the previous used baseType="identifier", and we also use baseType="identifier", then they will be "compatible" without really needing to compare both descriptors).
Also, we probably won't have a good place to call convertTo as we're trying to keep the interactionSection (the one that can actually detect the change) as independent from each interaction detail as possible.
|
|
||
| for (const child of children) { | ||
| if (typeof child === 'string') { | ||
| el.appendChild(xmlDoc.createTextNode(child)); |
There was a problem hiding this comment.
I think we may need this to set the innerHTML property at some point when we need to build the prompt nodes, but we can defer this change to when we actually need it :)
There was a problem hiding this comment.
I think we do need a sanitizer somewhere, but I think we should have a general XML sanitizer. The current sanitizer is also doing validation, and I think validation can be done within the QTIDeclaration class instead, as it would not be a general qti validation, but declaration validations. For example, we should have a BaseType constant in constants.js instead, because we will need it when we build the xml back from the interactions state, and then we can validate the baseType value at export time (on the .getXML, and we can potentially validate in other places too, but the most important one is that.) against the available values in the BaseType constant.
Identifier validation will also be needed in several places, so we can potentially move this to a helper module. But if invalid identifiers are fed into the editor, we should probably just drop them and generate proper ones (or drop them, depending on the case).
In any case, we should do XML sanitization, but not only for qti but rather for the entire XML assembly logic (also because RTE doesn't currently escape/sanitize the input/output #5956).
| constructor(values) { | ||
| /** @type {string[]} */ | ||
| this._values = values; | ||
| } | ||
|
|
||
| /** | ||
| * Parse a <qti-correct-response> element and register on the parent declaration. | ||
| * Note: the optional `interpretation` attribute (QTI 3.0 §3.1.1.2) is not | ||
| * preserved — it is not used by the authoring editor. | ||
| * | ||
| * @param {Element} xmlNode | ||
| * @param {import('../QTIDeclaration.js').QTIDeclaration} declaration | ||
| * @returns {CorrectResponse} | ||
| */ | ||
| static fromXML(xmlNode, declaration) { | ||
| const values = [...xmlNode.querySelectorAll('qti-value')].map(v => v.textContent.trim()); | ||
| const instance = new CorrectResponse(values); | ||
| declaration.registerCapability(CAPABILITY.CORRECT_RESPONSE, instance); | ||
| return instance; | ||
| } | ||
|
|
||
| /** | ||
| * Build from plain JS data and register on the parent declaration. | ||
| * Used by QTIDeclaration.convertTo() to carry forward values without XML serialization. | ||
| * | ||
| * @param {string[]} values | ||
| * @param {import('../QTIDeclaration.js').QTIDeclaration} declaration | ||
| * @returns {CorrectResponse} | ||
| */ | ||
| static fromPlain(values, declaration) { | ||
| const instance = new CorrectResponse(values); | ||
| declaration.registerCapability(CAPABILITY.CORRECT_RESPONSE, instance); | ||
| return instance; | ||
| } |
There was a problem hiding this comment.
I'd say we should receive the declaration as a constructor argument and call registerCapability on the constructor to prevent potential deviations between the fromXML and fromPlain. With this, I'd say that we can live without fromPlain and just directly create the object by calling the constructor on the consumer. What do you think?
| * @returns {CorrectResponse} | ||
| */ | ||
| static fromXML(xmlNode, declaration) { | ||
| const values = [...xmlNode.querySelectorAll('qti-value')].map(v => v.textContent.trim()); |
There was a problem hiding this comment.
Could we also bring the parseValues and coerceValues, and their respective counterparts for the XML build, to the QTIDeclaration class? It'd be great to have the additional coercion and validation we have there, and it would also be more comfortable to use the JS values instead of transforming them to strings before using them in the QTIDeclaration.
I think bringing support for cardinality="record" is going to be complex, so it'd probably be best not to do it right now, as there isn't yet a proper use case for it. So we can just add some errors or warnings flagging that cardinality="record" is not yet supported?
2 participants
Summary
Implemented QTI declaration serialization framework and validation utilities
This PR add:
QTIDeclaration.js— Central class representing aqti-response-declarationorqti-outcome-declarationelement. HandlesfromXML(parse from existing XML),fromPlain(construct from flat JSON), andgetXML(emit a DOM node for assembly). Supports all cardinality and base-type combinations defined in the QTI 3.0 spec.QTISanitizer.js— Defensive validation and XSS mitigation utility. Validates structural attributes (identifier,base-type,cardinality) with fail-fast error semantics, and strips unsafe HTML tags from string content using a DOM-based approach that removes<script>/<style>elements entirely rather than extracting their bodies.assembleItem.js—buildXmlNodehelper that constructs DOM element trees from a plain descriptor object{ tag, attrs, children }, used by all declaration strategies.interactionSchema.js— Registry mapping QTI interaction type names to their descriptor shapes, used to validate and look up interaction metadata.References
closes #5965
AI usage
Used Antigravity for final review and nitpicks.