Initial integration of QTI and IMSCP import

[rtibbles]

• Does an initial integration of IMSCP import into ricecooker.
• Adds some simple utilities for mapping IMSCP/SCORM metadata fields to our metadata
• Allows proper mapping of embedded webcontent files to the entry option
• Writes tests to cover this behaviour

Fixes #332
Fixes #337

[rtibbles]

Looks like the way I am doing the zipfile testing is not compatible with Windows, open file handles etc - will need to update for that.

[jredrejo]

I had two problems ithe these few lines of code:
language was en-US
metadata.dict was this one:

(Pdb) metadata_dict
{'title': 'Work with computers', 'description': 'This learning path will introduce you to the different parts and types of the computer and their functions. You will also learn the difference between operating systems and applications and their functions. Peripherals and portable storage devices will be discussed as well.', 'language': 'en-US', 'keyword': 'Default'}

It worked with these changes (I ignore if the keyword change will work with other scorm files)

    if metadata_dict.get("language"):
        node.set_language(metadata_dict.get("language").lower().split("-")[0])
    keyword = metadata_dict.get("keyword")
    if keyword:
        node.tags = node.tags + [keyword,]

[rtibbles]

Updated - hopefully has a similar effect, add some additional validation to make sure it's a language we know.

[jredrejo]

This is producing a server error in Studio https://learningequality.sentry.io/issues/4997106816/?project=1252819

[jredrejo]

I've been playing with the latest code from this PR:

• Ricecooker works
• Uploading to Studio unstable works
• The channel appears correctly (preview doesn't work, but that's not a ricecooker problem) https://unstable.studio.learningequality.org/es-es/channels/6877a75b029e5259a02c93046ee6dd80 , token: kokom-fitih
• Using kolibri v0.16.1, I can see the channel, the structure is a bit weird, with more levels than expected: 1 single scorm file has up to four levels to access to the resource:
  
• After importing it, it does not work correctly, this is the error message:
  

[jredrejo]

Left a detailed comment with the two issues found

[rtibblesbot]

Solid initial integration of IMSCP import into ricecooker — the manifest parsing, SCORM metadata mapping, and pipeline handler structure are well-designed. Two blocking issues around license propagation and dynamic child processing in nested structures need fixing before merge.

CI passing. All tests pass across Python 3.9-3.13 on all platforms.

Blocking:

• License not propagated through nested topics — leaf ContentNodes under TopicNodes get license=None (see inline)
• process_files dynamic children loop doesn't recurse into TopicNode children's descendants (see inline)
• assert used for runtime validation of untrusted XML input in imscp.py — disabled under python -O (see inline)

Suggestions:

• QTIZipFile added to files.py but no corresponding QTI handler or tests — consider deferring to a follow-up PR or adding a comment (see inline)
• _extract_lom_text can return mixed types (string, list, or None) which could surprise callers (see inline)

[rtibblesbot]

blocking: self.license is None when this method is called recursively on a TopicNode (created at line 790). TopicNode inherits from TreeNode where license defaults to None. So leaf ContentNodes created under topic nodes will have license=None, which will fail ContentNode.validate() at runtime (line 924: "ContentNode must have a license").

Consider propagating the license explicitly, e.g. adding a license parameter to _build_children_from_metadata similar to how parent_files is propagated:

def _build_children_from_metadata(self, children_list, parent_files=None, license=None):
    license = license or self.license
    ...
    node = ContentNode(source_id, title, license, ...)
    ...
    node._build_children_from_metadata(nested_children, parent_files=parent_files, license=license)

[rtibblesbot]

blocking: This loop only processes direct children (self.children). For TopicNode children that contain their own leaf ContentNode grandchildren, those grandchildren are never validated and their filenames are never collected. With a structure like ContentNode -> TopicNode -> ContentNode(leaf), the leaf's files won't appear in filenames.

This needs to recurse into TopicNode children, e.g.:

def _collect_dynamic_filenames(self, node):
    filenames = []
    for child in node.children:
        if isinstance(child, ContentNode) and not child._files_processed:
            child._files_processed = True
            child.validate()
        for f in child.files:
            if f.filename:
                filenames.append(f.filename)
        if isinstance(child, TopicNode):
            filenames.extend(self._collect_dynamic_filenames(child))
    return filenames

[rtibblesbot]

blocking: assert is disabled when Python runs with -O (optimized mode). Since this validates untrusted XML input from content packages, it should use a proper exception:

if not text.strip():
    raise ManifestParseError(
        "Title element has no title: {}".format(
            etree.tostring(title_elem, pretty_print=True)
        )
    )

[rtibblesbot]

suggestion: QTIZipFile is added here but there's no corresponding QTIConversionHandler or QTIMetadataExtractor in the pipeline, and no tests for QTI handling. The PR title mentions "QTI and IMSCP" but only IMSCP is implemented. Consider either removing QTIZipFile from this PR (deferring to a follow-up) or adding a code comment noting it's a placeholder for upcoming QTI work.

[rtibblesbot]

suggestion: _extract_lom_text returns a str (single string match), list[str] (multiple strings), or None — three different types. Callers in _collect_field pass the result through without normalization, so downstream code (e.g. metadata_dict_to_content_node_fields) needs to handle all three. This works for fields like keyword where _normalize_keywords handles both, but for title or description a list would be unexpected. Consider always returning a string for single-valued fields, or documenting the contract clearly.

[rtibblesbot]

Solid initial integration of IMSCP and QTI import into ricecooker. The architecture is well-considered — standalone manifest parsing, metadata mapping utilities, pipeline handler integration, and nested child node building all fit cleanly into the existing codebase patterns.

CI passing (linting). Test coverage is thorough across all new modules.

• suggestion (1): Consider handler ordering for edge cases (convert.py)
• suggestion (2): Potential AttributeError when TreeNode._build_children_from_metadata is called on a TopicNode with no files attribute default (nodes.py)
• suggestion (3): _apply_content_node_metadata unconditionally overwrites list fields from pipeline (nodes.py)
• nitpick (1): Metadata field application list is duplicated between two methods (nodes.py)
• nitpick (2): _recursive_update could be a dict in-place update note (context.py)

[rtibblesbot]

suggestion: license = license or getattr(self, "license", None) — if self.license is a falsy value (e.g. an empty string was somehow set), this would fall through to None. In practice this is likely fine since ContentNode.__init__ requires a license object, but worth noting that getattr with a default of None combined with or means the fallback is only used when the attribute is missing or falsy, not just missing.

[rtibblesbot]

nitpick: This metadata_fields list overlaps significantly with _METADATA_APPLY_FIELDS (defined on ContentNode at line 998). The two lists serve the same purpose (applying metadata to a node) but differ slightly (this one includes tags but omits thumbnail, author, aggregator, provider, role, etc.). Consider extracting a shared constant, or documenting why the child-building path intentionally uses a narrower set of fields.

[rtibblesbot]

suggestion: _apply_content_node_metadata unconditionally overwrites list fields like learning_activities, resource_types, grade_levels, etc. via setattr. If the node had pre-existing values set in the constructor (e.g. by the sushi chef), the pipeline metadata would silently replace them rather than merge. Contrast with the tags handling two lines below (line 1061) which correctly appends. This may be intentional (pipeline metadata should take precedence for these fields) but it's worth a comment documenting the design choice, or making the merge-vs-replace behavior consistent.

[rtibblesbot]

suggestion: fields["title"] = item.get("title", fields.get("title")) — if the item has no "title" key and metadata_dict_to_content_node_fields also produced no title, fields["title"] will be set to None. Downstream in _build_children_from_metadata (nodes.py line 801), the fallback child_meta.title or "Untitled" handles this, but _content_node_metadata_from_dict will store None as the title. This is fine but slightly surprising — a missing title propagates as None through the metadata layer and only gets defaulted at the node-building layer.

[rtibblesbot]

suggestion: if resource_type == "webcontent" or is_qti_resource(resource_type): — any other resource types (e.g. "associatedcontent/imscc_xmlv1p1/learning-application-resource") won't get files populated. This means resources of unrecognized types are silently dropped. Consider logging a warning for unrecognized resource types that have file references, similar to how dangling identifierref is logged on line 301.

[rtibblesbot]

nitpick: The chardet fallback opens the file in a non-context-manager style (f = zf.open(...); data = f.read(); f.close()). Using a with statement would be marginally cleaner:

with zf.open("imsmanifest.xml", "r") as f:
    data = f.read()