Qti exercise export #5103
Qti exercise export #5103
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughAdd full QTI support alongside existing Perseus export by introducing Pydantic-based QTI element models (AssessmentItem, IMSManifest, etc.), a QTIExerciseGenerator, and dynamic publisher logic; refactor Perseus generator; include TeX math extension for Markdown; update tests; and adjust dependencies. Changes
Assessment against linked issues
Out-of-scope changes
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🔭 Outside diff range comments (2)
contentcuration/contentcuration/utils/assessment/qti/interaction_types/simple.py (1)
30-38: 🛠️ Refactor suggestionEnforce consistency between
max_choices,min_choices, andanswerslengthNothing currently stops
min_choicesfrom exceedingmax_choices, ormax_choicesfrom exceedinglen(answers). Introducing aroot_validatorwill prevent creation of invalid assessment items at model-instantiation time.+from pydantic import root_validator + class ChoiceInteraction(BlockInteraction): @@ answers: List[SimpleChoice] = Field(default_factory=list) + @root_validator + def _check_choice_bounds(cls, values): + min_c, max_c, answers = ( + values.get("min_choices"), + values.get("max_choices"), + values.get("answers", []), + ) + if min_c > max_c: + raise ValueError("`min_choices` cannot exceed `max_choices`") + if max_c > len(answers): + raise ValueError("`max_choices` cannot exceed number of answers") + return valuescontentcuration/contentcuration/utils/assessment/qti/html/sequence.py (1)
47-49:⚠️ Potential issue
Dl.childrenlacks a default list → required field at runtimeEvery other container element provides
Field(default_factory=list).
Without it, constructing a bare<dl>raises a validation error.-class Dl(BlockContentElement): - children: List[Union[FlowContent, Dt, Dd]] +class Dl(BlockContentElement): + children: List[Union[FlowContent, Dt, Dd]] = Field(default_factory=list)
🧹 Nitpick comments (25)
requirements-dev.in (1)
8-8: Pinpytest-subteststo a fixed version for deterministic CI runsAll other dev dependencies are pinned; leaving this one unpinned may lead to sudden breakages when a new major version is released.
-pytest-subtests +pytest-subtests==0.12.1 # or the latest version verified to work with the test-suitecontentcuration/contentcuration/utils/assessment/qti/__init__.py (1)
1-6: Minor style nit – make__all__immutableUsing a tuple instead of a list prevents accidental mutation and is customary for
__all__.-__all__ = [ - "ElementTreeBase", -] +__all__ = ("ElementTreeBase",)contentcuration/contentcuration/tests/testdata.py (1)
220-222: Prefer a language constant over hard-coding"en"Hard-coding the ISO code is fine for tests, but using the canonical constant keeps the fixture aligned with production code and avoids typos:
-from ... import models as cc -... -channel = cc.Channel.objects.create( - name=name, actor_id=channel_creator.id, language_id="en" +# from le_utils.constants.languages import EN # example +... +channel = cc.Channel.objects.create( + name=name, actor_id=channel_creator.id, language_id=EN.id )contentcuration/contentcuration/utils/assessment/qti/prompt.py (2)
1-9: Imports look correct; consider using Python 3.10 union syntax
All required classes are imported. For brevity and modern typing, you could combine imports and switch to native union types (children: list[Math | FlowContent]), aligning with Pydantic v2 patterns.
11-11: DefinePromptas QTI element
Inheriting fromQTIBasecorrectly sets up XML naming. Consider adding a brief class docstring to describe the prompt’s role in assessment items.contentcuration/contentcuration/utils/assessment/qti/html/breaks.py (1)
1-2: Merge base imports
You can consolidate these into a single line:from contentcuration.utils.assessment.qti.html.base import InlineContentElement, BlockContentElementcontentcuration/contentcuration/utils/assessment/qti/interaction_types/base.py (2)
1-2: Combine imports from the same module
You can merge these into one line to tidy imports:from contentcuration.utils.assessment.qti.base import BaseSequence, QTIBase
5-11:Interactionbase class setup is correct
Inheriting fromQTIBaseandBaseSequenceprovides XML naming and common sequence attributes. Consider marking this class as abstract (abc.ABC) to prevent instantiation without specific interaction logic.contentcuration/contentcuration/tests/utils/qti/test_qti.py (2)
27-39: String-level XML assertions are brittleRelying on raw string equality for XML makes the tests sensitive to non-semantic changes (attribute order, insignificant whitespace, namespace prefixes, etc.). Consider parsing both strings into
xml.etree.ElementTree.Elementobjects (or using canonicalisation) before comparison to avoid false negatives across Python versions.-import xml.etree.ElementTree as ET -self.assertEqual(value_element.to_xml_string(), "<qti-value>10</qti-value>") +import xml.etree.ElementTree as ET +expected = ET.fromstring("<qti-value>10</qti-value>") +actual = ET.fromstring(value_element.to_xml_string()) +self.assertEqual( + ET.tostring(actual, method="c14n"), + ET.tostring(expected, method="c14n"), +)
78-88: Add negative-path testsAll tests cover “happy-path” serialization. Including a few cases that intentionally omit required fields (e.g. missing
identifierforAssessmentItem) would guard the Pydantic validation layer and prevent silent regressions.contentcuration/contentcuration/tests/utils/qti/test_imsmanifest.py (2)
90-121: XML attribute-order coupling
expected_xmlstrings assert a specific attribute order (identifier,version, …). Although currentElementTreepreserves insertion order, the spec does not guarantee this ordering and future refactors could re-order fields without breaking correctness. Recommend structural comparison or canonical XML in these high-value tests.
172-199: Readability & maintainability of very long literalsThe full-integration manifest literal is ~900 chars. Hard-wrapped XML literals hurt diff readability. Consider:
- Serialising the expected object again and comparing against itself (round-trip test), or
- Storing golden XML files under
tests/fixtures/and loading them.Either approach keeps tests readable while still guaranteeing byte-for-byte equality.
contentcuration/contentcuration/utils/assessment/qti/html/__init__.py (1)
90-188: Manually-curated__all__is prone to driftThe 90-line export list must be updated every time an element is added/removed, risking silent omissions. Auto-generate it instead:
-__all__ = [ - # … long list … -] +import sys as _sys +__all__ = [name for name, obj in _sys.modules[__name__].__dict__.items() + if getattr(obj, "__module__", "") == __name__]This keeps the public surface authoritative and maintenance-free.
contentcuration/contentcuration/utils/assessment/qti/mathml/__init__.py (1)
35-77: Duplicate maintenance burden mirrors HTML initSimilar to the HTML package, the hard-coded
__all__list will drift as MathML classes evolve. Consider the same automatic export pattern suggested forhtml/__init__.py.contentcuration/contentcuration/utils/assessment/qti/constants.py (1)
42-45: Minor: consider usingenum.StrEnumIf you are already on Python 3.11+, inheriting from
StrEnumremoves the need for explicitvalueaccess (json.dumps, pydantic serialisation, …) and keeps the string behaviour.Not blocking, just a heads-up.
contentcuration/contentcuration/utils/assessment/qti/html/flow.py (1)
16-17: Mutable default list handled correctly – but addmin_items=1validator if empty lists aren’t allowedAt the moment every flow container serialises as an empty element when no children are appended.
If an empty container is never valid in QTI, enforce this at model level:children: FlowGroupList = Field(default_factory=list, min_items=1)contentcuration/contentcuration/tests/utils/test_markdown.py (1)
10-13: String equality is brittle – compare parsed DOM insteadWhitespace or attribute re-ordering will break these assertions even though the output is logically identical.
Consider parsing both strings withxml.etree.ElementTree(orlxml) and comparing canonicalised serialisations.contentcuration/contentcuration/tests/utils/qti/test_assessment_items.py (1)
281-284: Prompt plain string vsPydanticelementYou wrap prompt text in
Prompt(children=[prompt_text]). A regular string might bypass validation; preferPrompt(children=[TextNode(text=prompt_text)])or expose a helper to accept raw strings and convert internally.contentcuration/contentcuration/utils/assessment/qti/html/display.py (1)
20-30: Optional – declareelement_namefor non-lowercase tagsIf
ElementTreeBasedoes not automatically convertSummary/Details/Figcaption/Figureto lowercase tag names, overrideelement_namejust likeSourcedoes inbase.py.
This avoids accidental<Summary>tags in the output.contentcuration/contentcuration/utils/assessment/qti/html/sequence.py (1)
27-33: Minor: attribute nametypeshadows builtinUsing
typefor theOlstyle is HTML-accurate but hides Python’s builtintypewithin the class scope.
Consider renaming the field (e.g.list_type) withField(alias="type")for clarity.contentcuration/contentcuration/utils/assessment/qti/archive.py (1)
43-49: Consider using an enum or constants module for question types.The sets
choice_interactionsandtext_entry_interactionsgroup question types, but "true_false" is a string literal while others use constants fromexercises. This inconsistency could lead to maintenance issues.choice_interactions = { exercises.MULTIPLE_SELECTION, exercises.SINGLE_SELECTION, - "true_false", + exercises.TRUE_FALSE, # Assuming this constant exists or should be added }contentcuration/contentcuration/tests/utils/qti/test_fields.py (1)
258-261: Combine nestedwithstatements for better readability.The static analysis tool correctly identifies that nested
withstatements can be combined.for srcset in invalid_srcsets: - with self.subTest(srcset=srcset): - with self.assertRaises(ValueError): - validate_local_srcset(srcset) + with self.subTest(srcset=srcset), self.assertRaises(ValueError): + validate_local_srcset(srcset)🧰 Tools
🪛 Ruff (0.11.9)
258-259: Use a single
withstatement with multiple contexts instead of nestedwithstatementsCombine
withstatements(SIM117)
contentcuration/contentcuration/tests/utils/qti/test_mathml.py (1)
1420-1422: Use underscore for unused loop variable.The loop variable
iis not used within the loop body.# Nest 10 levels deep - for i in range(10): + for _ in range(10): content = Mrow(children=[content])🧰 Tools
🪛 Ruff (0.11.9)
1421-1421: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
contentcuration/contentcuration/utils/assessment/markdown.py (1)
60-60: Simplify the chained comparison.The condition can be written more concisely using Python's chained comparison syntax.
- if endpos >= state.bMarks[line] and endpos <= state.eMarks[line]: + if state.bMarks[line] <= endpos <= state.eMarks[line]:🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 60-60: Simplify chained comparison between the operands
(R1716)
contentcuration/contentcuration/tests/utils/test_exercise_creation.py (1)
1024-1024: Use context manager for ZipFile.Consider using a context manager for better resource handling:
- zip_file = zipfile.ZipFile(BytesIO(zip_data)) + with zipfile.ZipFile(BytesIO(zip_data)) as zip_file: + # Check that the imsmanifest.xml file exists + assert ( + "imsmanifest.xml" in zip_file.namelist() + ), "imsmanifest.xml not found in zip file" + + return zip_fileNote: This would require adjusting the return value and subsequent usage in calling code.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 1024-1024: Consider using 'with' for resource-allocating operations
(R1732)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
contentcuration/contentcuration/tests/test_exportchannel.py(1 hunks)contentcuration/contentcuration/tests/testdata.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_assessment_items.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_fields.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_html.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_imsmanifest.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_mathml.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_qti.py(1 hunks)contentcuration/contentcuration/tests/utils/test_exercise_creation.py(15 hunks)contentcuration/contentcuration/tests/utils/test_markdown.py(1 hunks)contentcuration/contentcuration/utils/assessment/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/markdown.py(1 hunks)contentcuration/contentcuration/utils/assessment/perseus.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/__init__.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/archive.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/assessment_item.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/constants.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/fields.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/__init__.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/breaks.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/content_types.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/display.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/embed.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/flow.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/media.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/sequence.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/table.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/text.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/imsmanifest.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/interaction_types/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/interaction_types/simple.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/interaction_types/text_based.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/__init__.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/core.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/fields.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/prompt.py(1 hunks)contentcuration/contentcuration/utils/publish.py(3 hunks)requirements-dev.in(1 hunks)requirements-dev.txt(2 hunks)requirements.in(1 hunks)requirements.txt(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (19)
contentcuration/contentcuration/utils/assessment/qti/html/breaks.py (1)
contentcuration/contentcuration/utils/assessment/qti/html/base.py (2)
BlockContentElement(28-29)InlineContentElement(24-25)
contentcuration/contentcuration/utils/assessment/qti/prompt.py (2)
contentcuration/contentcuration/utils/assessment/qti/base.py (1)
QTIBase(143-152)contentcuration/contentcuration/utils/assessment/qti/mathml/core.py (1)
Math(29-32)
contentcuration/contentcuration/utils/assessment/qti/archive.py (11)
contentcuration/contentcuration/utils/assessment/base.py (2)
ExerciseArchiveGenerator(41-382)add_file_to_write(125-136)contentcuration/contentcuration/utils/assessment/markdown.py (1)
render_markdown(119-120)contentcuration/contentcuration/utils/assessment/qti/assessment_item.py (6)
CorrectResponse(41-44)ItemBody(79-84)OutcomeDeclaration(58-76)ResponseDeclaration(147-160)ResponseProcessing(163-171)Value(22-38)contentcuration/contentcuration/utils/assessment/qti/base.py (4)
ElementTreeBase(221-283)from_string(275-283)to_xml_string(132-135)get_file_dependencies(137-140)contentcuration/contentcuration/utils/assessment/qti/constants.py (4)
BaseType(14-25)Cardinality(7-11)Orientation(59-61)ShowHide(42-44)contentcuration/contentcuration/utils/assessment/qti/html/flow.py (1)
Div(23-24)contentcuration/contentcuration/utils/assessment/qti/html/text.py (1)
P(29-30)contentcuration/contentcuration/utils/assessment/qti/imsmanifest.py (5)
IMSManifest(112-127)Metadata(25-33)Organizations(60-67)Resource(88-99)Resources(102-109)contentcuration/contentcuration/utils/assessment/qti/interaction_types/simple.py (2)
ChoiceInteraction(29-37)SimpleChoice(15-26)contentcuration/contentcuration/utils/assessment/qti/interaction_types/text_based.py (1)
TextEntryInteraction(11-19)contentcuration/contentcuration/utils/assessment/qti/prompt.py (1)
Prompt(11-13)
contentcuration/contentcuration/tests/utils/qti/test_imsmanifest.py (2)
contentcuration/contentcuration/utils/assessment/qti/imsmanifest.py (9)
Dependency(80-85)File(70-77)IMSManifest(112-127)Item(36-44)Metadata(25-33)Organization(47-57)Organizations(60-67)Resource(88-99)Resources(102-109)contentcuration/contentcuration/utils/assessment/qti/base.py (1)
to_xml_string(132-135)
contentcuration/contentcuration/tests/utils/qti/test_fields.py (1)
contentcuration/contentcuration/utils/assessment/qti/fields.py (4)
validate_data_uri(35-45)validate_local_href_path(48-63)validate_local_src_path(66-77)validate_local_srcset(88-102)
contentcuration/contentcuration/utils/assessment/qti/__init__.py (1)
contentcuration/contentcuration/utils/assessment/qti/base.py (1)
ElementTreeBase(221-283)
contentcuration/contentcuration/utils/assessment/qti/interaction_types/text_based.py (3)
contentcuration/contentcuration/utils/assessment/qti/constants.py (1)
Format(53-56)contentcuration/contentcuration/utils/assessment/qti/interaction_types/base.py (2)
BlockInteraction(13-14)InlineInteraction(17-18)contentcuration/contentcuration/utils/assessment/qti/prompt.py (1)
Prompt(11-13)
contentcuration/contentcuration/utils/assessment/qti/interaction_types/base.py (1)
contentcuration/contentcuration/utils/assessment/qti/base.py (2)
BaseSequence(171-178)QTIBase(143-152)
contentcuration/contentcuration/tests/utils/test_markdown.py (2)
contentcuration/contentcuration/utils/assessment/markdown.py (1)
render_markdown(119-120)contentcuration/contentcuration/utils/assessment/qti/base.py (3)
ElementTreeBase(221-283)from_string(275-283)to_xml_string(132-135)
contentcuration/contentcuration/utils/assessment/qti/html/table.py (1)
contentcuration/contentcuration/utils/assessment/qti/html/base.py (2)
BlockContentElement(28-29)HTMLElement(9-17)
contentcuration/contentcuration/utils/assessment/qti/html/content_types.py (3)
contentcuration/contentcuration/utils/assessment/qti/html/base.py (2)
FlowContentElement(20-21)InlineContentElement(24-25)contentcuration/contentcuration/utils/assessment/qti/interaction_types/base.py (2)
BlockInteraction(13-14)InlineInteraction(17-18)contentcuration/contentcuration/utils/assessment/qti/mathml/core.py (1)
Math(29-32)
contentcuration/contentcuration/utils/assessment/qti/html/display.py (2)
contentcuration/contentcuration/utils/assessment/qti/html/base.py (2)
FlowContentElement(20-21)HTMLElement(9-17)contentcuration/contentcuration/utils/assessment/qti/html/text.py (2)
BlockHTMLText(20-21)InlineHTMLText(16-17)
contentcuration/contentcuration/utils/assessment/qti/html/sequence.py (1)
contentcuration/contentcuration/utils/assessment/qti/html/base.py (2)
BlockContentElement(28-29)HTMLElement(9-17)
contentcuration/contentcuration/utils/assessment/qti/interaction_types/simple.py (4)
contentcuration/contentcuration/utils/assessment/qti/base.py (2)
BaseSequence(171-178)QTIBase(143-152)contentcuration/contentcuration/utils/assessment/qti/constants.py (2)
Orientation(59-61)ShowHide(42-44)contentcuration/contentcuration/utils/assessment/qti/interaction_types/base.py (1)
BlockInteraction(13-14)contentcuration/contentcuration/utils/assessment/qti/prompt.py (1)
Prompt(11-13)
contentcuration/contentcuration/utils/assessment/base.py (2)
contentcuration/contentcuration/utils/assessment/perseus.py (6)
get_image_file_path(112-113)get_image_ref_prefix(115-116)create_assessment_item(97-110)_process_content(78-80)process_assessment_item(82-85)handle_before_assessment_items(118-125)contentcuration/contentcuration/models.py (1)
generate_object_storage_name(637-645)
contentcuration/contentcuration/utils/assessment/qti/html/media.py (1)
contentcuration/contentcuration/utils/assessment/qti/html/base.py (3)
BlockContentElement(28-29)HTMLElement(9-17)Source(32-51)
contentcuration/contentcuration/utils/assessment/qti/html/flow.py (1)
contentcuration/contentcuration/utils/assessment/qti/html/base.py (1)
BlockContentElement(28-29)
contentcuration/contentcuration/utils/assessment/qti/base.py (1)
contentcuration/contentcuration/utils/assessment/qti/constants.py (1)
Dir(47-50)
contentcuration/contentcuration/tests/utils/test_exercise_creation.py (4)
contentcuration/contentcuration/utils/assessment/perseus.py (1)
PerseusExerciseGenerator(16-125)contentcuration/contentcuration/utils/assessment/qti/archive.py (1)
QTIExerciseGenerator(51-278)contentcuration/contentcuration/tests/testdata.py (3)
channel(218-228)user(242-251)fileobj_exercise_image(392-402)contentcuration/contentcuration/tests/base.py (2)
StudioTestCase(19-63)setUpBase(28-32)
🪛 Ruff (0.11.9)
contentcuration/contentcuration/tests/utils/qti/test_fields.py
258-259: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
270-271: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
282-283: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
293-294: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
contentcuration/contentcuration/utils/assessment/qti/fields.py
25-25: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
contentcuration/contentcuration/utils/assessment/base.py
335-336: ExerciseArchiveGenerator.handle_before_assessment_items is an empty method in an abstract base class, but has no abstract decorator
(B027)
338-339: ExerciseArchiveGenerator.handle_after_assessment_items is an empty method in an abstract base class, but has no abstract decorator
(B027)
349-349: Use a context manager for opening files
(SIM115)
357-357: Use a context manager for opening files
(SIM115)
contentcuration/contentcuration/tests/utils/qti/test_mathml.py
123-124: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
174-175: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
210-211: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
1421-1421: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
🪛 Pylint (3.3.7)
contentcuration/contentcuration/utils/assessment/qti/html/display.py
[refactor] 16-16: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/utils/assessment/base.py
[refactor] 41-41: Too many instance attributes (9/7)
(R0902)
[refactor] 91-91: Too many arguments (6/5)
(R0913)
[refactor] 91-91: Too many positional arguments (6/5)
(R0917)
[refactor] 146-146: Either all return statements in a function should return an expression, or none of them should.
(R1710)
[refactor] 160-160: Too many arguments (7/5)
(R0913)
[refactor] 160-160: Too many positional arguments (7/5)
(R0917)
[refactor] 160-160: Either all return statements in a function should return an expression, or none of them should.
(R1710)
[refactor] 181-181: Too many arguments (7/5)
(R0913)
[refactor] 181-181: Too many positional arguments (7/5)
(R0917)
[refactor] 349-349: Consider using 'with' for resource-allocating operations
(R1732)
[refactor] 357-357: Consider using 'with' for resource-allocating operations
(R1732)
contentcuration/contentcuration/tests/utils/qti/test_html.py
[refactor] 280-280: Too many local variables (31/15)
(R0914)
[refactor] 280-280: Too many statements (61/50)
(R0915)
[refactor] 373-373: Too many local variables (22/15)
(R0914)
[refactor] 373-373: Too many statements (56/50)
(R0915)
contentcuration/contentcuration/utils/assessment/markdown.py
[refactor] 60-60: Simplify chained comparison between the operands
(R1716)
contentcuration/contentcuration/utils/assessment/qti/html/flow.py
[refactor] 19-19: Too many ancestors (8/7)
(R0901)
[refactor] 23-23: Too many ancestors (8/7)
(R0901)
[refactor] 27-27: Too many ancestors (8/7)
(R0901)
[refactor] 31-31: Too many ancestors (8/7)
(R0901)
[refactor] 35-35: Too many ancestors (8/7)
(R0901)
[refactor] 39-39: Too many ancestors (8/7)
(R0901)
[refactor] 43-43: Too many ancestors (8/7)
(R0901)
[refactor] 47-47: Too many ancestors (8/7)
(R0901)
[refactor] 51-51: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/utils/assessment/qti/html/text.py
[refactor] 24-24: Too many ancestors (8/7)
(R0901)
[refactor] 29-29: Too many ancestors (8/7)
(R0901)
[refactor] 33-33: Too many ancestors (8/7)
(R0901)
[refactor] 37-37: Too many ancestors (8/7)
(R0901)
[refactor] 41-41: Too many ancestors (8/7)
(R0901)
[refactor] 45-45: Too many ancestors (8/7)
(R0901)
[refactor] 49-49: Too many ancestors (8/7)
(R0901)
[refactor] 53-53: Too many ancestors (8/7)
(R0901)
[refactor] 57-57: Too many ancestors (8/7)
(R0901)
[refactor] 61-61: Too many ancestors (8/7)
(R0901)
[refactor] 65-65: Too many ancestors (8/7)
(R0901)
[refactor] 69-69: Too many ancestors (8/7)
(R0901)
[refactor] 73-73: Too many ancestors (8/7)
(R0901)
[refactor] 77-77: Too many ancestors (8/7)
(R0901)
[refactor] 81-81: Too many ancestors (8/7)
(R0901)
[refactor] 85-85: Too many ancestors (8/7)
(R0901)
[refactor] 89-89: Too many ancestors (8/7)
(R0901)
[refactor] 93-93: Too many ancestors (8/7)
(R0901)
[refactor] 97-97: Too many ancestors (8/7)
(R0901)
[refactor] 101-101: Too many ancestors (8/7)
(R0901)
[refactor] 105-105: Too many ancestors (8/7)
(R0901)
[refactor] 109-109: Too many ancestors (8/7)
(R0901)
[refactor] 113-113: Too many ancestors (8/7)
(R0901)
[refactor] 117-117: Too many ancestors (8/7)
(R0901)
[refactor] 121-121: Too many ancestors (8/7)
(R0901)
[refactor] 130-130: Too many ancestors (8/7)
(R0901)
[refactor] 134-134: Too many ancestors (8/7)
(R0901)
[refactor] 138-138: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/utils/assessment/qti/base.py
[refactor] 39-39: Too few public methods (0/2)
(R0903)
[refactor] 84-106: Unnecessary "else" after "continue", remove the "else" and de-indent the code inside it
(R1724)
[refactor] 72-126: Too many nested blocks (6/5)
(R1702)
[refactor] 63-63: Too many branches (17/12)
(R0912)
contentcuration/contentcuration/tests/utils/test_exercise_creation.py
[refactor] 984-984: Too many arguments (6/5)
(R0913)
[refactor] 984-984: Too many positional arguments (6/5)
(R0917)
[refactor] 1024-1024: Consider using 'with' for resource-allocating operations
(R1732)
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py
[refactor] 112-112: Too many ancestors (8/7)
(R0901)
[refactor] 116-116: Too many ancestors (8/7)
(R0901)
[refactor] 120-120: Too many ancestors (8/7)
(R0901)
[refactor] 124-124: Too many ancestors (8/7)
(R0901)
[refactor] 129-129: Too many ancestors (8/7)
(R0901)
[refactor] 134-134: Too many ancestors (8/7)
(R0901)
[refactor] 144-144: Too many ancestors (8/7)
(R0901)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Python unit tests
🔇 Additional comments (43)
requirements.in (1)
36-39: Verify that the new libraries are compatible with the existing stack
pydantic>=2introduces several breaking changes relative to the 1.x series, and downstream packages (e.g., FastAPI, dj-antic, or internal utilities) may still depend on<2.
Please run the resolver withpip check(orpip-tools compile && pip check) and the full test-suite to ensure no conflicts.If incompatibilities surface, consider pinning
pydantic<2.0or upgrading the dependent packages in the same PR.contentcuration/contentcuration/tests/test_exportchannel.py (1)
401-406: LGTM – updated assertion correctly reflects language inheritanceThe new expectation that child topics inherit
channel.language_idlooks accurate and keeps the test aligned with the fixture change. No further action needed.requirements-dev.txt (2)
11-14: Pinnedattrsto support new testing dependencies
Lockingattrs==23.1.0aligns withpytest-subtests' requirements and ensures a reproducible dev environment.
82-87: Add explicitpytest-subtestspin
Pinningpytest-subtests==0.14.1guarantees subtest support in CI runs.contentcuration/contentcuration/utils/assessment/qti/prompt.py (1)
13-13:childrenfield default factory is correct
Usingdefault_factory=listensures each instance gets its own list. The type union ofMathandFlowContentappropriately models mixed content.contentcuration/contentcuration/utils/assessment/qti/html/breaks.py (1)
5-11: Simple HTML element classes are valid
BrandHrcorrectly subclass the inline and block content bases. No additional attributes are required.requirements.txt (5)
9-10: Addannotated-typesfor advanced type annotations
Pinningannotated-types==0.7.0supports the new field validation patterns in Pydantic models.
159-164: Includelangcodesandlanguage-datafor BCP47 validation
These packages enable proper language-tag checking in QTI field validators.
167-172: Add Markdown and MathML conversion tools
markdown-it-py,mdurl, andlatex2mathmlare essential for parsing Markdown with TeX math and converting to MathML. Pins look correct.
205-208: Pin Pydantic v2.x dependencies
Lockingpydantic==2.11.5andpydantic-core==2.33.2ensures compatibility with the new Pydantic-based QTI data models.
258-264: Updatetyping-extensionsconstraint metadata
Reflecting new dependencies in the autogenerated commentary keeps the requirements file accurate.contentcuration/contentcuration/utils/assessment/qti/interaction_types/base.py (1)
13-19: Concrete interaction subclasses are valid
BlockInteractionandInlineInteractioncleanly represent block- and inline-level interactions without extra fields.contentcuration/contentcuration/utils/assessment/qti/interaction_types/text_based.py (1)
25-33:min_strings/max_stringsrelationship uncheckedAdd a validator to guarantee
min_strings <= max_stringswhen both are provided; this prevents impossible interaction configurations.contentcuration/contentcuration/utils/assessment/qti/html/embed.py (2)
14-25: Well-structured HTML element models!The
Imgclass correctly models all standard HTML img attributes with appropriate types. Good use ofLocalSrcPathandLocalSrcSetfor path validation.
32-49: Flexible and accurate Object element implementation.The
Objectclass properly handles the complex children model with params followed by flow content. The string type for width/height to support percentages is a thoughtful design choice.contentcuration/contentcuration/utils/assessment/perseus.py (3)
73-76: Efficient formula processing implementation.The regex pattern correctly matches and removes outer dollar signs from LaTeX formulas while preserving the inner content.
97-102: Excellent error handling for unrecognized question types.The TypeError with a descriptive message including the assessment_id provides clear debugging information.
87-95: Thoughtful handling of edge cases in answer processing.Good implementation that explicitly handles the zero value case (
answer == 0) to ensure it's not filtered out as falsy.contentcuration/contentcuration/utils/assessment/qti/html/table.py (2)
33-39: Comprehensive enum for table header scope values.The
ThScopeenum correctly includes all valid HTML5 th element scope attribute values.
69-73: Well-structured Table class with proper child element constraints.The Table class correctly models the HTML table element structure by allowing all valid child elements (Caption, Colgroup, THead, TBody, TFoot, Tr) in the appropriate order.
contentcuration/contentcuration/utils/assessment/qti/html/media.py (2)
24-30: Proper use of specialized field types for Track element.Good use of
LocalSrcPathfor src validation andBCP47Languagefor language code validation. The default kind of SUBTITLES is a sensible choice.
38-49: Pragmatic approach to modeling complex media element children.The simplified children model for Audio and Video elements is a reasonable design choice given the complexity of the actual HTML5 content model. The comment clearly documents this simplification.
Also applies to: 51-62
contentcuration/contentcuration/utils/assessment/qti/mathml/base.py (2)
12-26: Well-designed base class with comprehensive MathML global attributes.The
MathMLElementbase class correctly includes all standard MathML global attributes with appropriate specialized field types. Good separation from HTML elements.
27-70: Clear and logical MathML element class hierarchy.The hierarchy properly separates presentation, token, layout, script, and grouping elements with descriptive docstrings explaining each category's purpose. This structure will make it easy to implement specific MathML elements.
contentcuration/contentcuration/utils/assessment/qti/html/content_types.py (1)
14-34: Type-alias block looks goodThe unions cleanly express the permissible HTML/QTI element mixes and will help mypy/IDE code-completion without introducing runtime overhead. 👍
contentcuration/contentcuration/utils/assessment/qti/mathml/fields.py (1)
7-20: Regex is complex but soundRegex covers the full CSS length/percentage grammar and includes unit-less zero. Good job capturing edge-cases.
contentcuration/contentcuration/utils/assessment/qti/archive.py (1)
276-278: Good practice: Sorting files for predictable archive generation.The sorting of
files_to_writeensures consistent and predictable ZIP file generation, which aids in testing and debugging.contentcuration/contentcuration/tests/utils/qti/test_fields.py (1)
162-180: Well-structured test cases for srcset validation.The test cases comprehensively cover various valid srcset formats including single images, data URIs, and different descriptors (x, w, h). Good coverage!
contentcuration/contentcuration/utils/assessment/qti/fields.py (1)
83-86: Well-designed regex patterns for srcset validation.The regex patterns correctly handle the complex srcset syntax including data URIs and various descriptors. The use of raw strings and clear variable names makes the patterns maintainable.
contentcuration/contentcuration/utils/publish.py (1)
149-151: Good validation: Ensuring channel language is set before publishing.This validation prevents publishing channels without language metadata, which is essential for QTI generation and proper content localization.
contentcuration/contentcuration/tests/utils/qti/test_mathml.py (2)
1-23: Excellent documentation of test generation methodology.The detailed comments explaining how the test suite was generated (using Gemini and Claude) and refined manually demonstrates good documentation practices and transparency about the test development process.
849-1263: Exceptional test coverage for complex mathematical expressions.The test suite covers a wide range of real-world mathematical expressions including:
- Quadratic formula
- Integrals with limits
- Matrix operations
- Summations
- Chemical equations
- Nested fractions
- Multiscript notation
This comprehensive coverage ensures the MathML implementation can handle practical use cases.
contentcuration/contentcuration/utils/assessment/qti/html/text.py (1)
1-152: Well-structured HTML element models.The Pydantic-based HTML element classes are well-designed with proper inheritance hierarchy, consistent patterns, and appropriate type hints. The separation between inline and block elements is clear, and the use of enums for constrained values is appropriate.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 24-24: Too many ancestors (8/7)
(R0901)
[refactor] 29-29: Too many ancestors (8/7)
(R0901)
[refactor] 33-33: Too many ancestors (8/7)
(R0901)
[refactor] 37-37: Too many ancestors (8/7)
(R0901)
[refactor] 41-41: Too many ancestors (8/7)
(R0901)
[refactor] 45-45: Too many ancestors (8/7)
(R0901)
[refactor] 49-49: Too many ancestors (8/7)
(R0901)
[refactor] 53-53: Too many ancestors (8/7)
(R0901)
[refactor] 57-57: Too many ancestors (8/7)
(R0901)
[refactor] 61-61: Too many ancestors (8/7)
(R0901)
[refactor] 65-65: Too many ancestors (8/7)
(R0901)
[refactor] 69-69: Too many ancestors (8/7)
(R0901)
[refactor] 73-73: Too many ancestors (8/7)
(R0901)
[refactor] 77-77: Too many ancestors (8/7)
(R0901)
[refactor] 81-81: Too many ancestors (8/7)
(R0901)
[refactor] 85-85: Too many ancestors (8/7)
(R0901)
[refactor] 89-89: Too many ancestors (8/7)
(R0901)
[refactor] 93-93: Too many ancestors (8/7)
(R0901)
[refactor] 97-97: Too many ancestors (8/7)
(R0901)
[refactor] 101-101: Too many ancestors (8/7)
(R0901)
[refactor] 105-105: Too many ancestors (8/7)
(R0901)
[refactor] 109-109: Too many ancestors (8/7)
(R0901)
[refactor] 113-113: Too many ancestors (8/7)
(R0901)
[refactor] 117-117: Too many ancestors (8/7)
(R0901)
[refactor] 121-121: Too many ancestors (8/7)
(R0901)
[refactor] 130-130: Too many ancestors (8/7)
(R0901)
[refactor] 134-134: Too many ancestors (8/7)
(R0901)
[refactor] 138-138: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/tests/utils/qti/test_html.py (1)
1-579: Excellent test coverage for HTML elements.The test suite provides comprehensive coverage for all HTML element types with well-organized test methods grouped by element category. The integration tests effectively validate HTML parsing and roundtrip conversion. This thorough testing ensures the reliability of the QTI HTML element implementation.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 280-280: Too many local variables (31/15)
(R0914)
[refactor] 280-280: Too many statements (61/50)
(R0915)
[refactor] 373-373: Too many local variables (22/15)
(R0914)
[refactor] 373-373: Too many statements (56/50)
(R0915)
contentcuration/contentcuration/utils/assessment/qti/base.py (4)
22-37: LGTM! Well-structured enumerations.The ResourceType and InteractionType enums provide clear type definitions for QTI elements. Including an UNKNOWN type in InteractionType is good defensive programming.
143-169: LGTM! Clean implementation of QTI naming conventions.The QTIBase class correctly implements the QTI-specific element naming convention, and the string coercion helpers provide a clean API for handling text content.
186-220: Excellent use of metaclass for element registry.The RegistryMeta implementation provides a clean registry pattern with:
- Prevention of duplicate element registrations
- Lazy loading of related modules
- Clear error messages
This design ensures extensibility while maintaining uniqueness constraints.
221-284: Well-implemented XML parsing with proper text node handling.The parsing implementation correctly:
- Handles unknown tags with informative errors
- Extracts text nodes including tail text (often overlooked)
- Wraps content in a root element to handle multiple top-level elements
- Provides clear error messages for invalid markup
contentcuration/contentcuration/tests/utils/test_exercise_creation.py (2)
86-95: Good refactoring to use the new generator pattern.The introduction of
_create_perseus_zipmethod properly encapsulates the use ofPerseusExerciseGenerator, aligning with the new architecture while maintaining the existing test coverage.
947-1625: Comprehensive QTI test suite with excellent coverage.The TestQTIExerciseCreation class provides thorough testing:
- Multiple question types (single/multiple selection, free response)
- Mathematical content with MathML conversion
- Error handling for unsupported types and Perseus questions
- Manifest structure validation
- Deterministic checksums for regression testing
The
_normalize_xmlhelper is a good approach for XML comparison.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 984-984: Too many arguments (6/5)
(R0913)
[refactor] 984-984: Too many positional arguments (6/5)
(R0917)
[refactor] 1024-1024: Consider using 'with' for resource-allocating operations
(R1732)
contentcuration/contentcuration/utils/assessment/qti/assessment_item.py (1)
22-194: Well-structured QTI data models with comprehensive field coverage.The assessment item models are excellently designed:
- Clear documentation explaining field purposes (especially for Value class)
- Comprehensive optional fields with appropriate defaults
- Proper use of enums for constrained values
- Hardcoded XML namespaces ensure QTI compliance
- Good use of factory defaults for list fields
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py (2)
29-148: Comprehensive MathML Core implementation following specifications.The MathML element classes are well-designed:
- Appropriate use of annotated types for child count constraints
- Comprehensive attributes for each element type (especially Mo)
- Good documentation (e.g., mathvariant comment in Mi)
The static analysis warnings about "too many ancestors" can be safely ignored as the deep hierarchy is inherent to the MathML specification structure.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 112-112: Too many ancestors (8/7)
(R0901)
[refactor] 116-116: Too many ancestors (8/7)
(R0901)
[refactor] 120-120: Too many ancestors (8/7)
(R0901)
[refactor] 124-124: Too many ancestors (8/7)
(R0901)
[refactor] 129-129: Too many ancestors (8/7)
(R0901)
[refactor] 134-134: Too many ancestors (8/7)
(R0901)
[refactor] 144-144: Too many ancestors (8/7)
(R0901)
179-203: Excellent validation logic for Semantics element.The Semantics class validation is thorough and well-implemented:
- Ensures at least one child exists
- Validates first child is presentation content
- Ensures subsequent children are annotations only
- Provides clear, actionable error messages
This enforcement of MathML structure rules at the model level prevents invalid document generation.
contentcuration/contentcuration/utils/assessment/qti/interaction_types/simple.py
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/utils/assessment/qti/constants.py
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/utils/assessment/qti/html/flow.py
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/utils/assessment/qti/interaction_types/text_based.py
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/utils/assessment/qti/imsmanifest.py
Outdated
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
contentcuration/contentcuration/utils/assessment/base.py (1)
357-359:open(tempf.name, "rb")still lacks a context managerThe surrounding discussion acknowledged the first loop fix, but this second handle remains. Wrapping it is a one-liner and silences SIM115/R1732 without altering behaviour:
- assessment_file_obj = models.File.objects.create( - file_on_disk=File(open(tempf.name, "rb"), name=filename), + with open(tempf.name, "rb") as f: + assessment_file_obj = models.File.objects.create( + file_on_disk=File(f, name=filename), ... )🧰 Tools
🪛 Ruff (0.11.9)
358-358: Use a context manager for opening files
(SIM115)
🪛 Pylint (3.3.7)
[refactor] 358-358: Consider using 'with' for resource-allocating operations
(R1732)
contentcuration/contentcuration/utils/assessment/qti/archive.py (1)
233-237: Prefix removal still fragile – past feedback un-addressed
replace("item_", "")will strip every occurrence, not just the leading one.
Re-use the previously suggestedstartswith()check or store the assessment ID separately.(See prior bot comment; marking as duplicate.)
contentcuration/contentcuration/utils/assessment/qti/base.py (1)
63-129:to_elementis still deeply nested and hard to follow
(see previous feedback)The method maintains 6-level nesting and 16 branches. Extracting helpers (
_append_children,_set_attr, …) would improve readability, unit-testability and open the door for caching or performance tweaks later.Not blocking, just reiterating earlier recommendation.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 83-112: Unnecessary "elif" after "raise", remove the leading "el" from "elif"
(R1720)
[refactor] 72-125: Too many nested blocks (6/5)
(R1702)
[refactor] 63-63: Too many branches (16/12)
(R0912)
🧹 Nitpick comments (18)
contentcuration/contentcuration/utils/assessment/base.py (1)
335-341: Mark optional hook methods as abstract or document intent
handle_before_assessment_items/handle_after_assessment_itemsare no-ops in the base class but are invoked unconditionally.
Either:
- Decorate them with
@abstractmethodso subclasses must implement, or- Add docstrings clarifying they are optional extension points and will be skipped if left un-overridden.
This avoids the Ruff B027 warning and makes the contract explicit.
🧰 Tools
🪛 Ruff (0.11.9)
335-336:
ExerciseArchiveGenerator.handle_before_assessment_itemsis an empty method in an abstract base class, but has no abstract decorator(B027)
338-339:
ExerciseArchiveGenerator.handle_after_assessment_itemsis an empty method in an abstract base class, but has no abstract decorator(B027)
contentcuration/contentcuration/utils/assessment/qti/html/flow.py (1)
20-22: Consider a looser URL type for internal package links
cite: Optional[HttpUrl]enforces fully-qualified HTTP(S) URIs.
The QTI spec (and common HTML practice) also allows relative or fragment-only references inside the package. Switching to the existingLocalHrefPath(or a custom regex/AnyUrl) would avoid false validation failures when authors reference sibling documents.-from pydantic import HttpUrl +from contentcuration.utils.assessment.qti.fields import LocalHrefPath # already used elsewhere ... - cite: Optional[HttpUrl] = None + cite: Optional[LocalHrefPath] = None🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 20-20: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/utils/assessment/qti/interaction_types/simple.py (1)
44-55: Validator raises plainValueError– surface a PydanticValidationErrorinsteadReturning/raising
ValueErrorinside afield_validatoris fine, but wrapping it in Pydantic’sValueErrorsubclass (e.g.ValueErrorviapydantic.ValidationError) yields richer error context and consistent downstream handling.-from pydantic import Field, field_validator +from pydantic import Field, field_validator, ValidationError ... - raise ValueError( + raise ValidationError( "Duplicate identifiers detected in ChoiceInteraction.answers; " "each SimpleChoice.identifier must be unique." )🧰 Tools
🪛 Pylint (3.3.7)
[error] 45-45: Method '_unique_answer_identifiers' should have "self" as first argument
(E0213)
contentcuration/contentcuration/utils/assessment/qti/fields.py (1)
80-98: Large regex +findallreturns huge tuples
entry_patternnests the fulldata_uri_pattern, leading to 15-element tuples fromre.findall().
Only element[0]is used, so most captures are wasted and memory/time scale with tuple size.Optimise by:
-# keep only the URL as a single capture group -entry_pattern = rf"((?:{data_uri_pattern}|[^\s,]+))(?:\s+\d*\.?\d+[xwh])+" +entry_pattern = rf"((?:{data_uri_pattern}|[^\s,]+))(?:\s+\d*\.?\d+[xwh])+"(or switch to
finditer+group(1)).Minor but pays off when validating long srcset strings.
contentcuration/contentcuration/tests/utils/qti/test_mathml.py (1)
1421-1422: Rename unused loop variable for clarity
for i in range(10):–iis never used.
Conventional underscore avoids “unused variable” warnings.-for i in range(10): - content = Mrow(children=[content]) +for _ in range(10): + content = Mrow(children=[content])🧰 Tools
🪛 Ruff (0.11.9)
1421-1421: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
contentcuration/contentcuration/tests/utils/qti/test_fields.py (1)
140-148: Combine nestedwithblocks for brevityMultiple places use:
with self.subTest(...): with self.assertRaises(...): ...Python allows a single
with:with self.subTest(val=val), self.assertRaises(ValueError): ...Reduces indentation noise across the test file.
contentcuration/contentcuration/utils/assessment/qti/html/text.py (1)
24-27: Add an explicit alias for thetypeattribute
type_is mapped totypebyrstrip("_"), which works, but relying on this implicit convention is fragile and makes IDE / linters complain. Being explicit also keeps the rule obvious for future contributors.-class A(InlineHTMLText): - href: LocalHrefPath - type_: Optional[str] = None +class A(InlineHTMLText): + href: LocalHrefPath + type_: Optional[str] = Field(default=None, alias="type")No behaviour change – only extra clarity.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 24-24: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/tests/utils/qti/test_html.py (1)
282-354: Consider parameterising repetitive testsThe text-element block is largely copy-paste with only the element class changing. Parametrising with
subTestorpytest.mark.parametrizewould slash ~70 lines and keep the intent sharper.No action required for functionality – just a maintainability hint.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 282-282: Too many local variables (31/15)
(R0914)
[refactor] 282-282: Too many statements (61/50)
(R0915)
contentcuration/contentcuration/utils/assessment/qti/imsmanifest.py (1)
94-101: Mark optional child elements asOptional[…]for clarity
metadata,organizations, andresourcesdefault toNonebut their types are non-optional.
While Pydantic v2 tolerates this, explicitOptionalavoids mypy/pyright warnings and self-documents optionality.- metadata: Metadata = None - organizations: Organizations = None - resources: Resources = None + metadata: Optional[Metadata] = None + organizations: Optional[Organizations] = None + resources: Optional[Resources] = Nonecontentcuration/contentcuration/tests/utils/test_exercise_creation.py (6)
721-732: Regex is too restrictive for generated image filenames
_test_image_resizing_in_fieldassumes the resized filenames only contain lowercase hex characters:pattern = r"images/([a-f0-9]+\.(png|jpg|jpeg|gif))"
fileobj_exercise_image()currently emits hex‐only names, but if that helper ever switches to UUID v4, random words, or preserves original (mixed-case) filenames, the test will begin to fail spuriously.
Switch to a more permissive pattern:-pattern = r"images/([a-f0-9]+\.(png|jpg|jpeg|gif))" +pattern = r"images/([^/]+\.(?:png|jpg|jpeg|gif))"This still validates paths stay under
images/but avoids coupling the test to an implementation detail.
1551-1576: Don’t shadow business-logic validation with generic string constantThe unsupported-type test relies on the generator raising
ValueErrorfor the literal string"UNSUPPORTED_TYPE".
If a new legitimate assessment type happens to adopt that exact name, the test will become invalid but still pass the “setup” phase. To make the intention explicit and future-proof, create a sentinel clearly outside the domain, e.g.:-type="UNSUPPORTED_TYPE", +type="__INTENTIONALLY_INVALID__",and update the mapping/expectations accordingly.
187-196: Hard-coded checksum makes the test brittleself.assertEqual(exercise_file.checksum, "0ec7e964b466ebc76e81e175570e97f1")The checksum will change whenever we legitimately tweak the archive algorithm (ordering, compression level, metadata timestamps, etc.). This turns refactors into test-fix hunts.
Prefer verifying deterministic contents (e.g. required filenames, JSON fields) and drop the checksum assertion, or compute it on-the-fly from an immutable golden zip archived under
tests/fixtures/.
1455-1458: Non-hex character safety checkThe IDs
111…1a,222…2b,333…3cuse suffixesa/b/c, which are valid hex, but it is easy for a future edit to slip ing-z. Consider adding a helper likemake_hex_id()to generate guaranteed 32-char hex strings instead of hand-writing them in every test.Also applies to: 1489-1497
66-84: Muted pylint warnings:_create_assessment_itemsignatureStatic analysis flags “too many arguments (6/5)”. Given this is test util code, that’s acceptable, but we can silence the warning locally and keep global pylint strict:
def _create_assessment_item( # pylint: disable=too-many-argumentsNo functional change, keeps CI green if we tighten pylint later.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 66-66: Too many arguments (6/5)
(R0913)
[refactor] 66-66: Too many positional arguments (6/5)
(R0917)
528-536: Repetition of generator-bootstrapping can be collapsedEvery test repeats:
self._create_perseus_zip(exercise_data) exercise_file = self.exercise_node.files.get(...) zip_file, parsed = self._validate_perseus_zip(exercise_file)Extract a helper returning
(zip_file, parsed_data)to DRY 30+ occurrences and make individual tests focus on assertions, not plumbing.contentcuration/contentcuration/utils/assessment/qti/mathml/core.py (1)
165-168: Redundanttextattribute duplicateschildren
Annotationdefines bothtext: Optional[str]andchildren: List[TextType]. XML serialisation will pick one; keeping both risks inconsistencies (e.g.text != "".join(children)). Drop the extra field or derive one from the other inside a validator.contentcuration/contentcuration/utils/assessment/perseus.py (2)
88-95: Minor: avoid mutating the originalanswerslist when filtering.The current loop mutates each
answerdict in place before rebuilding a list; this side effect isn’t needed.- non_empty_answers = [] - for answer in processed_data["answers"]: - answer["answer"] = extract_value(answer["answer"]) - if answer["answer"] or answer["answer"] == 0: - non_empty_answers.append(answer) - - return {**processed_data, "answers": non_empty_answers} + cleaned_answers = [ + {**ans, "answer": extract_value(ans["answer"])} + for ans in processed_data["answers"] + if extract_value(ans["answer"]) or extract_value(ans["answer"]) == 0 + ] + return {**processed_data, "answers": cleaned_answers}
21-24: Consider enabling compression for generated zips.
ZIP_STOREDyields uncompressed archives; switching tozipfile.ZIP_DEFLATEDor making it configurable will produce significantly smaller exports with no code changes for consumers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
contentcuration/contentcuration/tests/test_exportchannel.py(1 hunks)contentcuration/contentcuration/tests/testdata.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_assessment_items.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_fields.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_html.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_imsmanifest.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_mathml.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_qti.py(1 hunks)contentcuration/contentcuration/tests/utils/test_exercise_creation.py(15 hunks)contentcuration/contentcuration/tests/utils/test_markdown.py(1 hunks)contentcuration/contentcuration/utils/assessment/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/markdown.py(1 hunks)contentcuration/contentcuration/utils/assessment/perseus.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/__init__.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/archive.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/assessment_item.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/constants.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/fields.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/__init__.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/breaks.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/content_types.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/display.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/embed.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/flow.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/media.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/sequence.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/table.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/text.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/imsmanifest.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/interaction_types/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/interaction_types/simple.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/interaction_types/text_based.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/__init__.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/core.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/fields.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/prompt.py(1 hunks)contentcuration/contentcuration/utils/publish.py(3 hunks)requirements-dev.in(1 hunks)requirements-dev.txt(2 hunks)requirements.in(1 hunks)requirements.txt(4 hunks)
✅ Files skipped from review due to trivial changes (4)
- requirements.in
- contentcuration/contentcuration/tests/utils/test_markdown.py
- contentcuration/contentcuration/utils/assessment/qti/constants.py
- contentcuration/contentcuration/utils/assessment/qti/html/init.py
🚧 Files skipped from review as they are similar to previous changes (23)
- requirements-dev.in
- contentcuration/contentcuration/utils/assessment/qti/init.py
- contentcuration/contentcuration/tests/test_exportchannel.py
- requirements-dev.txt
- requirements.txt
- contentcuration/contentcuration/utils/assessment/qti/html/breaks.py
- contentcuration/contentcuration/tests/testdata.py
- contentcuration/contentcuration/utils/assessment/qti/prompt.py
- contentcuration/contentcuration/tests/utils/qti/test_imsmanifest.py
- contentcuration/contentcuration/utils/assessment/qti/interaction_types/base.py
- contentcuration/contentcuration/utils/assessment/qti/mathml/init.py
- contentcuration/contentcuration/utils/assessment/qti/interaction_types/text_based.py
- contentcuration/contentcuration/utils/assessment/qti/html/base.py
- contentcuration/contentcuration/utils/assessment/qti/mathml/base.py
- contentcuration/contentcuration/tests/utils/qti/test_assessment_items.py
- contentcuration/contentcuration/utils/assessment/qti/html/content_types.py
- contentcuration/contentcuration/utils/assessment/qti/html/media.py
- contentcuration/contentcuration/utils/assessment/qti/html/sequence.py
- contentcuration/contentcuration/utils/assessment/qti/mathml/fields.py
- contentcuration/contentcuration/utils/assessment/qti/html/embed.py
- contentcuration/contentcuration/utils/assessment/qti/html/table.py
- contentcuration/contentcuration/utils/assessment/qti/assessment_item.py
- contentcuration/contentcuration/tests/utils/qti/test_qti.py
🧰 Additional context used
🧬 Code Graph Analysis (6)
contentcuration/contentcuration/utils/assessment/qti/imsmanifest.py (1)
contentcuration/contentcuration/utils/assessment/qti/base.py (2)
generate_coerced_string_type(161-164)XMLElement(45-139)
contentcuration/contentcuration/utils/assessment/qti/base.py (1)
contentcuration/contentcuration/utils/assessment/qti/constants.py (1)
Dir(47-50)
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py (3)
contentcuration/contentcuration/utils/assessment/qti/mathml/base.py (6)
MathMLElement(12-24)MathMLGroupingElement(63-69)MathMLLayoutElement(45-51)MathMLPresentationElement(27-33)MathMLScriptElement(54-60)MathMLTokenElement(36-42)contentcuration/contentcuration/utils/assessment/qti/mathml/fields.py (2)
MathMLDisplay(58-60)MathMLForm(63-66)contentcuration/contentcuration/utils/assessment/qti/base.py (3)
TextNode(39-42)element_name(60-61)element_name(148-151)
contentcuration/contentcuration/tests/utils/qti/test_fields.py (1)
contentcuration/contentcuration/utils/assessment/qti/fields.py (4)
validate_data_uri(32-42)validate_local_href_path(45-60)validate_local_src_path(63-74)validate_local_srcset(85-99)
contentcuration/contentcuration/utils/assessment/qti/html/display.py (2)
contentcuration/contentcuration/utils/assessment/qti/html/base.py (2)
FlowContentElement(22-23)HTMLElement(11-19)contentcuration/contentcuration/utils/assessment/qti/html/text.py (2)
BlockHTMLText(20-21)InlineHTMLText(16-17)
contentcuration/contentcuration/utils/assessment/qti/html/flow.py (1)
contentcuration/contentcuration/utils/assessment/qti/html/base.py (1)
BlockContentElement(30-31)
🪛 Pylint (3.3.7)
contentcuration/contentcuration/utils/assessment/qti/interaction_types/simple.py
[error] 45-45: Method '_unique_answer_identifiers' should have "self" as first argument
(E0213)
contentcuration/contentcuration/utils/assessment/qti/base.py
[refactor] 39-39: Too few public methods (0/2)
(R0903)
[refactor] 83-112: Unnecessary "elif" after "raise", remove the leading "el" from "elif"
(R1720)
[refactor] 72-125: Too many nested blocks (6/5)
(R1702)
[refactor] 63-63: Too many branches (16/12)
(R0912)
contentcuration/contentcuration/utils/assessment/base.py
[refactor] 41-41: Too many instance attributes (9/7)
(R0902)
[refactor] 91-91: Too many arguments (6/5)
(R0913)
[refactor] 91-91: Too many positional arguments (6/5)
(R0917)
[refactor] 146-146: Either all return statements in a function should return an expression, or none of them should.
(R1710)
[refactor] 160-160: Too many arguments (7/5)
(R0913)
[refactor] 160-160: Too many positional arguments (7/5)
(R0917)
[refactor] 160-160: Either all return statements in a function should return an expression, or none of them should.
(R1710)
[refactor] 181-181: Too many arguments (7/5)
(R0913)
[refactor] 181-181: Too many positional arguments (7/5)
(R0917)
[refactor] 358-358: Consider using 'with' for resource-allocating operations
(R1732)
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py
[refactor] 112-112: Too many ancestors (8/7)
(R0901)
[refactor] 116-116: Too many ancestors (8/7)
(R0901)
[refactor] 120-120: Too many ancestors (8/7)
(R0901)
[refactor] 124-124: Too many ancestors (8/7)
(R0901)
[refactor] 129-129: Too many ancestors (8/7)
(R0901)
[refactor] 134-134: Too many ancestors (8/7)
(R0901)
[refactor] 144-144: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/tests/utils/test_exercise_creation.py
[refactor] 984-984: Too many arguments (6/5)
(R0913)
[refactor] 984-984: Too many positional arguments (6/5)
(R0917)
[refactor] 1024-1024: Consider using 'with' for resource-allocating operations
(R1732)
contentcuration/contentcuration/tests/utils/qti/test_html.py
[refactor] 282-282: Too many local variables (31/15)
(R0914)
[refactor] 282-282: Too many statements (61/50)
(R0915)
[refactor] 375-375: Too many local variables (22/15)
(R0914)
[refactor] 375-375: Too many statements (56/50)
(R0915)
contentcuration/contentcuration/utils/assessment/markdown.py
[refactor] 60-60: Simplify chained comparison between the operands
(R1716)
contentcuration/contentcuration/utils/assessment/qti/html/display.py
[refactor] 16-16: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/utils/assessment/qti/html/flow.py
[refactor] 20-20: Too many ancestors (8/7)
(R0901)
[refactor] 24-24: Too many ancestors (8/7)
(R0901)
[refactor] 28-28: Too many ancestors (8/7)
(R0901)
[refactor] 32-32: Too many ancestors (8/7)
(R0901)
[refactor] 36-36: Too many ancestors (8/7)
(R0901)
[refactor] 40-40: Too many ancestors (8/7)
(R0901)
[refactor] 44-44: Too many ancestors (8/7)
(R0901)
[refactor] 48-48: Too many ancestors (8/7)
(R0901)
[refactor] 52-52: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/utils/assessment/qti/html/text.py
[refactor] 24-24: Too many ancestors (8/7)
(R0901)
[refactor] 29-29: Too many ancestors (8/7)
(R0901)
[refactor] 33-33: Too many ancestors (8/7)
(R0901)
[refactor] 37-37: Too many ancestors (8/7)
(R0901)
[refactor] 41-41: Too many ancestors (8/7)
(R0901)
[refactor] 45-45: Too many ancestors (8/7)
(R0901)
[refactor] 49-49: Too many ancestors (8/7)
(R0901)
[refactor] 53-53: Too many ancestors (8/7)
(R0901)
[refactor] 57-57: Too many ancestors (8/7)
(R0901)
[refactor] 61-61: Too many ancestors (8/7)
(R0901)
[refactor] 65-65: Too many ancestors (8/7)
(R0901)
[refactor] 69-69: Too many ancestors (8/7)
(R0901)
[refactor] 73-73: Too many ancestors (8/7)
(R0901)
[refactor] 77-77: Too many ancestors (8/7)
(R0901)
[refactor] 81-81: Too many ancestors (8/7)
(R0901)
[refactor] 85-85: Too many ancestors (8/7)
(R0901)
[refactor] 89-89: Too many ancestors (8/7)
(R0901)
[refactor] 93-93: Too many ancestors (8/7)
(R0901)
[refactor] 97-97: Too many ancestors (8/7)
(R0901)
[refactor] 101-101: Too many ancestors (8/7)
(R0901)
[refactor] 105-105: Too many ancestors (8/7)
(R0901)
[refactor] 109-109: Too many ancestors (8/7)
(R0901)
[refactor] 113-113: Too many ancestors (8/7)
(R0901)
[refactor] 117-117: Too many ancestors (8/7)
(R0901)
[refactor] 121-121: Too many ancestors (8/7)
(R0901)
[refactor] 130-130: Too many ancestors (8/7)
(R0901)
[refactor] 134-134: Too many ancestors (8/7)
(R0901)
[refactor] 138-138: Too many ancestors (8/7)
(R0901)
🪛 Ruff (0.11.9)
contentcuration/contentcuration/tests/utils/qti/test_mathml.py
123-124: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
174-175: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
210-211: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
1421-1421: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
contentcuration/contentcuration/utils/assessment/base.py
335-336: ExerciseArchiveGenerator.handle_before_assessment_items is an empty method in an abstract base class, but has no abstract decorator
(B027)
338-339: ExerciseArchiveGenerator.handle_after_assessment_items is an empty method in an abstract base class, but has no abstract decorator
(B027)
358-358: Use a context manager for opening files
(SIM115)
contentcuration/contentcuration/tests/utils/qti/test_fields.py
258-259: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
270-271: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
282-283: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
293-294: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
🔇 Additional comments (4)
contentcuration/contentcuration/utils/assessment/qti/html/display.py (1)
16-18: LGTM:for_attribute handled by custom serializerThe previous concern about needing an alias is resolved by the custom XML serializer that strips trailing underscores. No change required.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 16-16: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/utils/publish.py (1)
324-332: Input-question exercises never reach the QTI generator
any_free_responseonly checksexercises.FREE_RESPONSE, but the QTI generator also supportsexercises.INPUT_QUESTION(text-entry interaction).
Pure input-question exercises will therefore be routed to the Perseus generator unnecessarily.-any_free_response = any( - t == exercises.FREE_RESPONSE +any_qti = any( + t in {exercises.FREE_RESPONSE, exercises.INPUT_QUESTION} for t in exercise_data["assessment_mapping"].values() ) -generator_class = ( - QTIExerciseGenerator - if any_free_response - else PerseusExerciseGenerator -) +generator_class = QTIExerciseGenerator if any_qti else PerseusExerciseGeneratorPlease confirm intended behaviour.
contentcuration/contentcuration/utils/assessment/markdown.py (1)
15-17: 🛠️ Refactor suggestionBlock-math regex drops new-lines
BLOCK_PATTERNuses[^$]+?which stops on the first newline. Multiline expressions such as matrices will not parse.-BLOCK_PATTERN = re.compile(r"^\$\$([^$]+?)\$\$", re.M) +BLOCK_PATTERN = re.compile(r"^\$\$([\s\S]+?)\$\$", re.M)The
[\s\S]class lets the dot match newlines without switching to the slower DOTALL flag.Likely an incorrect or invalid review comment.
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py (1)
145-148:Mmultiscripts.childrenallows mixed order – consider stricter typingThe union
List[Union[PresentationContent, Mprescripts]]lets<mprescripts>appear anywhere, e.g.[mprescripts, base, sub], which violates the element’s grammar (prescripts must come after initial sub/sup pairs). If not handled elsewhere, consider a validator enforcing correct ordering to prevent invalid MathML from being serialised.
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py
Outdated
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
contentcuration/contentcuration/utils/assessment/qti/html/flow.py (1)
20-22: 🛠️ Refactor suggestion
HttpUrlis too restrictive forcite; allow local/relative URIs.
HttpUrlenforces a full absolute HTTP/HTTPS URL. QTI packages commonly use fragment, relative ("credits.html#ref"), or intra-package paths.
Switch to a looser validator (e.g.AnyUrlor the project’sLocalHrefPath) to avoid false-positives during export.-from pydantic import HttpUrl +from pydantic import AnyUrl # or: from …qti.fields import LocalHrefPath ... - cite: Optional[HttpUrl] = None + cite: Optional[AnyUrl] = None # accepts relative & absolute🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 20-20: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/tests/utils/qti/test_assessment_items.py (2)
221-225:⚠️ Potential issueSame mismatch in multiple-choice test.
- language="EN-US", + language="en-US",
103-107:⚠️ Potential issueTest will fail – language case mismatch.
AssessmentItem(language="EN-US")serialises verbatim, but the expected XML fixture useslanguage="en-US".
Either change the argument or update the expectation.- language="EN-US", + language="en-US",contentcuration/contentcuration/utils/assessment/qti/archive.py (1)
235-237: Robust assessment-ID extraction (duplicate from earlier review).Using plain
replace("item_", "")risks mangling identifiers containing the substring elsewhere. Prefer an explicit prefix check as previously suggested.contentcuration/contentcuration/utils/assessment/qti/base.py (1)
80-106:to_elementstill contains unreachable/over-indentedelseblockThe
elseaftercontinueis unreachable and keeps the cyclomatic complexity that we flagged earlier. The exact diff suggested in the previous round is still applicable and would also remove one level of nesting.if all(isinstance(item, (XMLElement, TextNode)) for item in value): ... continue - else: - raise ValueError( - "List types should only contain XMLElement or TextNodes" - ) + + raise ValueError( + "List types should only contain XMLElement or TextNodes" + )
🧹 Nitpick comments (14)
contentcuration/contentcuration/utils/assessment/base.py (1)
339-344: Mark hook methods as concrete or declare them@abstractmethod.
handle_before_assessment_itemsandhandle_after_assessment_itemsare stubs in the ABC but lack behavioural implementation.
Static analysis (ruff B027) warns because empty methods in an abstract base class are usually meant to be abstract or protected helpers.Two options:
@@ - def handle_before_assessment_items(self): - pass - - def handle_after_assessment_items(self): - pass + # Optional hooks – subclasses may override. + def handle_before_assessment_items(self): # noqa: B027 + """Hook executed before assessment-item processing.""" + return None + + def handle_after_assessment_items(self): # noqa: B027 + """Hook executed after assessment-item processing.""" + return Noneor
from abc import abstractmethod ... @abstractmethod def handle_before_assessment_items(self): ...Either clears the linter and makes intent explicit.
🧰 Tools
🪛 Ruff (0.11.9)
339-340:
ExerciseArchiveGenerator.handle_before_assessment_itemsis an empty method in an abstract base class, but has no abstract decorator(B027)
342-343:
ExerciseArchiveGenerator.handle_after_assessment_itemsis an empty method in an abstract base class, but has no abstract decorator(B027)
contentcuration/contentcuration/tests/utils/qti/test_fields.py (1)
258-261: Combine nestedwithstatements for terser tests.Ruff flags repetitive nesting (SIM117). You can shorten:
with self.assertRaises(ValueError), self.subTest(srcset=srcset): validate_local_srcset(srcset)Nice-to-have only.
🧰 Tools
🪛 Ruff (0.11.9)
258-259: Use a single
withstatement with multiple contexts instead of nestedwithstatementsCombine
withstatements(SIM117)
contentcuration/contentcuration/utils/assessment/qti/fields.py (1)
89-98: Minor: reduce regex back-reference overhead.
re.findall(entry_pattern, value)returns large tuples due to many capture groups and you only use element 0. Consider turning inner groups into non-capturing(?:…)to avoid unnecessary allocations.No functional impact, purely perf/readability.
contentcuration/contentcuration/tests/utils/qti/test_mathml.py (3)
120-130: Consolidate nestedwithstatementsThe pattern
with self.subTest(value=value): with self.assertRaises(ValidationError): ...appears several times (e.g. lines 123-126, 174-177, 210-213). It can be flattened into a single
withblock to eliminate one level of indentation and satisfy Ruff SIM117:-with self.subTest(value=value): - with self.assertRaises(ValidationError): - Mi(mathsize=value, children=["x"]) +with self.subTest(value=value), self.assertRaises(ValidationError): + Mi(mathsize=value, children=["x"])🧰 Tools
🪛 Ruff (0.11.9)
123-124: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
1420-1423: Rename unused loop variable
for i in range(10):–iis never read inside the loop; rename to_(or remove the variable completely) to silence Ruff B007 and make intention explicit.-for i in range(10): - content = Mrow(children=[content]) +for _ in range(10): + content = Mrow(children=[content])🧰 Tools
🪛 Ruff (0.11.9)
1421-1421: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
548-579: String‐equality assertions are brittleThe tests assert exact XML strings including attribute order (e.g.
expected_xml_str = '<math display="block" alttext="Equation"><mi>y</mi></math>').
Any change in attribute ordering byElementTreeor a future Python upgrade will break these tests while the semantic output is still correct.Prefer structural assertions:
root = ET.fromstring(xml_str) self.assertEqual(root.tag, "math") self.assertEqual(root.attrib["display"], "block") ...This keeps the test resilient to harmless re-ordering.
contentcuration/contentcuration/utils/assessment/qti/html/text.py (1)
125-132: Avoid shadowing the built-indirfunctionUsing
diras an attribute name works but shadows Python’s built-in, hurts auto-completion, and breaks the_ + "dir"/dir_naming convention already used forclass_,id_, etc. Recommend aliasing with a trailing underscore and preserving the rendered attribute usingField(alias="dir"):-class Bdo(InlineHTMLText): - dir: BdoDir +class Bdo(InlineHTMLText): + dir_: BdoDir = Field(alias="dir")Call-site stays readable (
Bdo(dir_=BdoDir.LTR, …)), avoids shadowing, and keeps consistency.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 130-130: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/utils/assessment/markdown.py (1)
57-64: Safety fallback to guarantee forward progressIf the closing
$$delimiter is never located (regex mismatch or later refactor), thewhileloop exits without updatingstate.line, risking an infinite loop. A one-line guard after the loop eliminates that possibility with negligible overhead:while line < endLine: ... line += 1 - return True + # Ensure we always advance at least one line + state.line = max(state.line, begLine + 1) + return True🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 60-60: Simplify chained comparison between the operands
(R1716)
contentcuration/contentcuration/tests/utils/qti/test_html.py (1)
548-552: Round-trip comparison too strict
xml_outputis compared against the raw HTML with whitespace stripped line-by-line.
Because attribute ordering and self-closing semantics (<br/>vs<br></br>) are implementation-defined, this equality can fail even when the structure is identical.Suggest parsing both sides with a tolerant HTML/XML parser (e.g.
ElementTreeorlxml) and comparing the normalized DOM instead of raw strings.contentcuration/contentcuration/utils/assessment/qti/base.py (1)
60-70: Consider extracting attribute/child handling helpers
to_elementnow runs at ~140 LOC, 16 branches, 6 nested blocks – well above typical readability thresholds.
Moving the three independent concerns into helpers would shorten the method dramatically:
_serialize_attribute(field_name, value, element)_serialize_child(child, element)_finalize_dependencies(attr_name, value)This would also make unit-testing of edge-cases (e.g.
srccollection) straightforward.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 63-63: Too many branches (16/12)
(R0912)
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py (1)
180-183: Enforce at least one annotation childThe
Len(2)annotation guarantees ≥2 children – good – but the error message forlen==0is now unreachable.
You can drop the explicit length-0 guard or update it to a clearer “must have ≥2 children” error.contentcuration/contentcuration/utils/assessment/qti/assessment_item.py (1)
174-193: Optional: drop hard-coded tool metadata or make it configurableHard-coding
tool-name="kolibri"andtool-version="0.1"in every item leaks internal branding/versioning into exported packages and complicates future upgrades.
Consider:tool_name: str = Field(default_factory=lambda: settings.QTI_TOOL_NAME) tool_version: str = Field(default_factory=lambda: settings.QTI_TOOL_VERSION)with sensible defaults in Django settings.
contentcuration/contentcuration/tests/utils/test_exercise_creation.py (1)
160-170: Deterministic checksum assertions are brittleThe hard-coded MD5s will fail on any non-semantic change (zip order, metadata timestamp, future Python zipfile implementation).
Relying on structural/semantic assertions (file list, XML equivalence) provides the same coverage without future maintenance overhead.contentcuration/contentcuration/utils/publish.py (1)
149-151: Consider i18n for newly-raisedChannelIncompleteErrormessageThe message is plain text, whereas most user-visible strings in this module use
gettext_lazy. If this exception can bubble up to the UI, wrapping the message with_()will keep the error translatable.- raise ChannelIncompleteError("Channel must have a language set to be published") + raise ChannelIncompleteError(_("Channel must have a language set to be published"))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
contentcuration/contentcuration/tests/utils/qti/test_assessment_items.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_fields.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_html.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_imsmanifest.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_mathml.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_qti.py(1 hunks)contentcuration/contentcuration/tests/utils/test_exercise_creation.py(17 hunks)contentcuration/contentcuration/tests/utils/test_markdown.py(1 hunks)contentcuration/contentcuration/utils/assessment/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/markdown.py(1 hunks)contentcuration/contentcuration/utils/assessment/perseus.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/__init__.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/archive.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/assessment_item.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/constants.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/fields.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/__init__.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/breaks.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/content_types.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/display.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/embed.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/flow.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/media.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/sequence.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/table.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/text.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/imsmanifest.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/interaction_types/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/interaction_types/simple.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/interaction_types/text_based.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/__init__.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/core.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/fields.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/prompt.py(1 hunks)contentcuration/contentcuration/utils/publish.py(3 hunks)requirements-dev.in(1 hunks)requirements-dev.txt(2 hunks)requirements.in(1 hunks)requirements.txt(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- contentcuration/contentcuration/utils/assessment/qti/html/table.py
🚧 Files skipped from review as they are similar to previous changes (22)
- requirements-dev.in
- contentcuration/contentcuration/utils/assessment/qti/init.py
- contentcuration/contentcuration/utils/assessment/qti/html/content_types.py
- requirements.in
- contentcuration/contentcuration/utils/assessment/qti/prompt.py
- contentcuration/contentcuration/utils/assessment/qti/html/breaks.py
- contentcuration/contentcuration/tests/utils/qti/test_imsmanifest.py
- contentcuration/contentcuration/tests/utils/qti/test_qti.py
- requirements-dev.txt
- requirements.txt
- contentcuration/contentcuration/utils/assessment/qti/interaction_types/base.py
- contentcuration/contentcuration/utils/assessment/qti/mathml/init.py
- contentcuration/contentcuration/utils/assessment/qti/interaction_types/text_based.py
- contentcuration/contentcuration/utils/assessment/qti/constants.py
- contentcuration/contentcuration/utils/assessment/qti/mathml/base.py
- contentcuration/contentcuration/utils/assessment/qti/html/base.py
- contentcuration/contentcuration/tests/utils/test_markdown.py
- contentcuration/contentcuration/utils/assessment/qti/html/embed.py
- contentcuration/contentcuration/utils/assessment/qti/html/media.py
- contentcuration/contentcuration/utils/assessment/qti/mathml/fields.py
- contentcuration/contentcuration/utils/assessment/qti/html/sequence.py
- contentcuration/contentcuration/utils/assessment/qti/html/init.py
🧰 Additional context used
🧬 Code Graph Analysis (9)
contentcuration/contentcuration/utils/assessment/qti/archive.py (9)
contentcuration/contentcuration/utils/assessment/base.py (2)
ExerciseArchiveGenerator(41-387)add_file_to_write(125-136)contentcuration/contentcuration/utils/assessment/markdown.py (1)
render_markdown(119-120)contentcuration/contentcuration/utils/assessment/qti/assessment_item.py (6)
CorrectResponse(41-44)ItemBody(79-84)OutcomeDeclaration(58-76)ResponseDeclaration(147-160)ResponseProcessing(163-171)Value(22-38)contentcuration/contentcuration/utils/assessment/qti/base.py (5)
ElementTreeBase(220-282)ResourceType(22-27)from_string(274-282)to_xml_string(131-134)get_file_dependencies(136-139)contentcuration/contentcuration/utils/assessment/qti/html/flow.py (1)
Div(24-25)contentcuration/contentcuration/utils/assessment/qti/html/text.py (1)
P(29-30)contentcuration/contentcuration/utils/assessment/qti/interaction_types/simple.py (2)
ChoiceInteraction(34-62)SimpleChoice(20-31)contentcuration/contentcuration/utils/assessment/qti/interaction_types/text_based.py (1)
TextEntryInteraction(14-25)contentcuration/contentcuration/utils/assessment/qti/prompt.py (1)
Prompt(11-13)
contentcuration/contentcuration/tests/utils/qti/test_mathml.py (5)
contentcuration/contentcuration/utils/assessment/qti/base.py (5)
TextNode(39-42)element_name(60-61)element_name(148-151)to_xml_string(131-134)from_string(274-282)contentcuration/contentcuration/utils/assessment/qti/constants.py (1)
Dir(47-50)contentcuration/contentcuration/utils/assessment/qti/mathml/core.py (29)
Annotation(164-167)AnnotationXml(170-176)Math(29-32)Mfrac(78-80)Mi(35-38)Mn(41-42)Mo(45-57)Mrow(74-75)Mspace(68-71)Msubsup(120-121)Mtable(160-161)Mtd(150-153)Mtr(156-157)Semantics(179-202)Merror(95-96)Mmultiscripts(144-147)Mover(129-131)Mphantom(108-109)Mprescripts(140-141)Mroot(87-88)Ms(64-65)Msqrt(83-84)Mstyle(91-92)Msub(112-113)Msup(116-117)Mtext(60-61)Munder(124-126)Munderover(134-137)element_name(175-176)contentcuration/contentcuration/utils/assessment/qti/mathml/fields.py (2)
MathMLDisplay(58-60)MathMLForm(63-66)contentcuration/contentcuration/utils/assessment/qti/mathml/base.py (5)
MathMLElement(12-24)MathMLGroupingElement(63-69)MathMLLayoutElement(45-51)MathMLScriptElement(54-60)MathMLTokenElement(36-42)
contentcuration/contentcuration/utils/assessment/qti/base.py (1)
contentcuration/contentcuration/utils/assessment/qti/constants.py (1)
Dir(47-50)
contentcuration/contentcuration/utils/assessment/qti/assessment_item.py (4)
contentcuration/contentcuration/utils/assessment/qti/base.py (2)
BaseSequence(170-177)QTIBase(142-151)contentcuration/contentcuration/utils/assessment/qti/constants.py (4)
BaseType(14-25)Cardinality(7-11)ExternalScored(37-39)View(28-34)contentcuration/contentcuration/utils/assessment/qti/html/base.py (1)
BlockContentElement(30-31)contentcuration/contentcuration/utils/assessment/qti/interaction_types/base.py (1)
BlockInteraction(14-15)
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py (3)
contentcuration/contentcuration/utils/assessment/qti/mathml/base.py (6)
MathMLElement(12-24)MathMLGroupingElement(63-69)MathMLLayoutElement(45-51)MathMLPresentationElement(27-33)MathMLScriptElement(54-60)MathMLTokenElement(36-42)contentcuration/contentcuration/utils/assessment/qti/mathml/fields.py (2)
MathMLDisplay(58-60)MathMLForm(63-66)contentcuration/contentcuration/utils/assessment/qti/base.py (3)
TextNode(39-42)element_name(60-61)element_name(148-151)
contentcuration/contentcuration/tests/utils/qti/test_fields.py (1)
contentcuration/contentcuration/utils/assessment/qti/fields.py (4)
validate_data_uri(32-42)validate_local_href_path(45-60)validate_local_src_path(63-74)validate_local_srcset(85-99)
contentcuration/contentcuration/utils/assessment/qti/html/display.py (2)
contentcuration/contentcuration/utils/assessment/qti/html/base.py (2)
FlowContentElement(22-23)HTMLElement(11-19)contentcuration/contentcuration/utils/assessment/qti/html/text.py (2)
BlockHTMLText(20-21)InlineHTMLText(16-17)
contentcuration/contentcuration/utils/assessment/qti/html/flow.py (1)
contentcuration/contentcuration/utils/assessment/qti/html/base.py (1)
BlockContentElement(30-31)
contentcuration/contentcuration/utils/assessment/qti/html/text.py (2)
contentcuration/contentcuration/utils/assessment/qti/html/base.py (2)
BlockContentElement(30-31)InlineContentElement(26-27)contentcuration/contentcuration/utils/assessment/qti/base.py (2)
element_name(60-61)element_name(148-151)
🪛 Ruff (0.11.9)
contentcuration/contentcuration/tests/utils/qti/test_mathml.py
123-124: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
174-175: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
210-211: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
1421-1421: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
contentcuration/contentcuration/tests/utils/qti/test_fields.py
258-259: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
270-271: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
282-283: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
293-294: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
contentcuration/contentcuration/utils/assessment/base.py
339-340: ExerciseArchiveGenerator.handle_before_assessment_items is an empty method in an abstract base class, but has no abstract decorator
(B027)
342-343: ExerciseArchiveGenerator.handle_after_assessment_items is an empty method in an abstract base class, but has no abstract decorator
(B027)
362-362: Use a context manager for opening files
(SIM115)
🪛 Pylint (3.3.7)
contentcuration/contentcuration/utils/assessment/qti/base.py
[refactor] 39-39: Too few public methods (0/2)
(R0903)
[refactor] 83-112: Unnecessary "elif" after "raise", remove the leading "el" from "elif"
(R1720)
[refactor] 72-125: Too many nested blocks (6/5)
(R1702)
[refactor] 63-63: Too many branches (16/12)
(R0912)
contentcuration/contentcuration/tests/utils/test_exercise_creation.py
[refactor] 1076-1076: Too many arguments (6/5)
(R0913)
[refactor] 1076-1076: Too many positional arguments (6/5)
(R0917)
[refactor] 1116-1116: Consider using 'with' for resource-allocating operations
(R1732)
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py
[refactor] 112-112: Too many ancestors (8/7)
(R0901)
[refactor] 116-116: Too many ancestors (8/7)
(R0901)
[refactor] 120-120: Too many ancestors (8/7)
(R0901)
[refactor] 124-124: Too many ancestors (8/7)
(R0901)
[refactor] 129-129: Too many ancestors (8/7)
(R0901)
[refactor] 134-134: Too many ancestors (8/7)
(R0901)
[refactor] 144-144: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/tests/utils/qti/test_html.py
[refactor] 282-282: Too many local variables (31/15)
(R0914)
[refactor] 282-282: Too many statements (61/50)
(R0915)
[refactor] 375-375: Too many local variables (22/15)
(R0914)
[refactor] 375-375: Too many statements (56/50)
(R0915)
contentcuration/contentcuration/utils/assessment/base.py
[refactor] 41-41: Too many instance attributes (9/7)
(R0902)
[refactor] 91-91: Too many arguments (6/5)
(R0913)
[refactor] 91-91: Too many positional arguments (6/5)
(R0917)
[refactor] 146-146: Either all return statements in a function should return an expression, or none of them should.
(R1710)
[refactor] 160-160: Too many arguments (7/5)
(R0913)
[refactor] 160-160: Too many positional arguments (7/5)
(R0917)
[refactor] 160-160: Either all return statements in a function should return an expression, or none of them should.
(R1710)
[refactor] 181-181: Too many arguments (7/5)
(R0913)
[refactor] 181-181: Too many positional arguments (7/5)
(R0917)
[refactor] 362-362: Consider using 'with' for resource-allocating operations
(R1732)
contentcuration/contentcuration/utils/assessment/markdown.py
[refactor] 60-60: Simplify chained comparison between the operands
(R1716)
contentcuration/contentcuration/utils/assessment/qti/html/display.py
[refactor] 16-16: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/utils/assessment/qti/html/flow.py
[refactor] 20-20: Too many ancestors (8/7)
(R0901)
[refactor] 24-24: Too many ancestors (8/7)
(R0901)
[refactor] 28-28: Too many ancestors (8/7)
(R0901)
[refactor] 32-32: Too many ancestors (8/7)
(R0901)
[refactor] 36-36: Too many ancestors (8/7)
(R0901)
[refactor] 40-40: Too many ancestors (8/7)
(R0901)
[refactor] 44-44: Too many ancestors (8/7)
(R0901)
[refactor] 48-48: Too many ancestors (8/7)
(R0901)
[refactor] 52-52: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/utils/assessment/qti/html/text.py
[refactor] 24-24: Too many ancestors (8/7)
(R0901)
[refactor] 29-29: Too many ancestors (8/7)
(R0901)
[refactor] 33-33: Too many ancestors (8/7)
(R0901)
[refactor] 37-37: Too many ancestors (8/7)
(R0901)
[refactor] 41-41: Too many ancestors (8/7)
(R0901)
[refactor] 45-45: Too many ancestors (8/7)
(R0901)
[refactor] 49-49: Too many ancestors (8/7)
(R0901)
[refactor] 53-53: Too many ancestors (8/7)
(R0901)
[refactor] 57-57: Too many ancestors (8/7)
(R0901)
[refactor] 61-61: Too many ancestors (8/7)
(R0901)
[refactor] 65-65: Too many ancestors (8/7)
(R0901)
[refactor] 69-69: Too many ancestors (8/7)
(R0901)
[refactor] 73-73: Too many ancestors (8/7)
(R0901)
[refactor] 77-77: Too many ancestors (8/7)
(R0901)
[refactor] 81-81: Too many ancestors (8/7)
(R0901)
[refactor] 85-85: Too many ancestors (8/7)
(R0901)
[refactor] 89-89: Too many ancestors (8/7)
(R0901)
[refactor] 93-93: Too many ancestors (8/7)
(R0901)
[refactor] 97-97: Too many ancestors (8/7)
(R0901)
[refactor] 101-101: Too many ancestors (8/7)
(R0901)
[refactor] 105-105: Too many ancestors (8/7)
(R0901)
[refactor] 109-109: Too many ancestors (8/7)
(R0901)
[refactor] 113-113: Too many ancestors (8/7)
(R0901)
[refactor] 117-117: Too many ancestors (8/7)
(R0901)
[refactor] 121-121: Too many ancestors (8/7)
(R0901)
[refactor] 130-130: Too many ancestors (8/7)
(R0901)
[refactor] 134-134: Too many ancestors (8/7)
(R0901)
[refactor] 138-138: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/utils/assessment/qti/interaction_types/simple.py
[error] 45-45: Method '_unique_answer_identifiers' should have "self" as first argument
(E0213)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Python unit tests
- GitHub Check: Build frontend assets
- GitHub Check: Build all message files
- GitHub Check: All file linting
🔇 Additional comments (7)
contentcuration/contentcuration/utils/assessment/qti/interaction_types/simple.py (1)
44-54: Validator looks good – ignore Pylint false positive.The
@field_validatorcorrectly receivesclsandanswers; Pydantic wraps the function soselfis not required.
Implementation enforces identifier uniqueness – nice.🧰 Tools
🪛 Pylint (3.3.7)
[error] 45-45: Method '_unique_answer_identifiers' should have "self" as first argument
(E0213)
contentcuration/contentcuration/utils/assessment/perseus.py (1)
70-79: Nice safeguard on Graphie delimiter.Validating the split length prevents silent index errors – good defensive coding.
contentcuration/contentcuration/utils/assessment/qti/html/display.py (1)
16-37: Looks good.All new HTML element stubs are minimal, type-correct, and integrate with the existing base classes. No issues spotted.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 16-16: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/utils/assessment/qti/archive.py (1)
138-150:CorrectResponsecan violate cardinality.When
cardinality=SINGLEyou must emit exactly one<value>.
If authors mark multiple answers as correct this will generate invalid QTI.
Either:
- restrict to one correct answer and warn, or
- switch to
Cardinality.MULTIPLE.contentcuration/contentcuration/utils/assessment/markdown.py (1)
68-77: Verify latex2mathml API – may raise at runtime
latex2mathml.converter.convertaccepts only the LaTeX string; recent versions do not support adisplay=keyword. Passing it silently succeeds on older versions but will raiseTypeErroron ≥ 5.0.Please confirm the version pinned in
requirementsexposes this signature, or gate the call:try: markup = convert(latex, display="inline" if inline else "block") except TypeError: # Fallback for newer latex2mathml markup = convert(latex, inline=inline) # or whichever API is correctcontentcuration/contentcuration/utils/assessment/qti/mathml/core.py (1)
45-57:maxsize/minsizedo not permit the literalinfinity
<mo>attributesmaxsize&minsizeaccept either a<length-percentage>or the literalinfinity(MathML Core §7.4.6).
Current typing (LengthPercentage) rejects valid markup. Consider:from typing import Union, Literal SizeAttr = Union[LengthPercentage, Literal["infinity"]] maxsize: Optional[SizeAttr] = None minsize: Optional[SizeAttr] = Nonecontentcuration/contentcuration/utils/publish.py (1)
46-48: Import additions look correctImports for both generator classes are accurate and keep the module structure clean.
contentcuration/contentcuration/utils/assessment/qti/imsmanifest.py
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py
Outdated
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
contentcuration/contentcuration/tests/utils/test_exercise_creation.py (1)
187-191: 🛠️ Refactor suggestionHard-coding archive checksums is brittle
The tests assert exact MD5 values (
exercise_file.checksum == …).
Any innocuous change in archive generation (timestamp ordering, JSON formatting, image-compression library upgrades, etc.) will break these tests despite functional equivalence.Recommend asserting structural/content invariants instead (e.g., expected filenames, JSON keys, manifest entries) or recomputing the checksum inside the assertion:
expected = hashlib.md5(zip_file.read(...)).hexdigest() self.assertEqual(exercise_file.checksum, expected)This keeps the test meaningful while decoupling it from incidental byte-level changes.
Also applies to: 628-633, 1475-1482, 1639-1642
♻️ Duplicate comments (4)
contentcuration/contentcuration/utils/assessment/qti/html/flow.py (1)
20-22: 🛠️ Refactor suggestionBroaden
citevalidation beyond absolute HTTP URLsUsing
HttpUrlrejects perfectly valid local, fragment, or data-URI citations that QTI packages often embed.
Swap to the customLocalHrefPathtype (orAnyUrl) so relative paths/IDs pass while still being validated.-from pydantic import HttpUrl +from contentcuration.utils.assessment.qti.fields import LocalHrefPath ... - cite: Optional[HttpUrl] = None + cite: Optional[LocalHrefPath] = None🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 20-20: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/utils/assessment/qti/archive.py (1)
233-236: Use safer prefix stripping when building file paths
identifier.replace("item_", "")will also strip any internal “item_” substring and could produce duplicate names.
Guard withstartswith(or store the bare ID separately when the item is created).-if qti_item.identifier.startswith("item_"): - assessment_id = qti_item.identifier[5:] -else: - raise ValueError(f"Unexpected identifier format: {qti_item.identifier}") -qti_item_filepath = self._qti_item_filepath(assessment_id) +prefix = "item_" +if not qti_item.identifier.startswith(prefix): + raise ValueError(f"Unexpected identifier format: {qti_item.identifier}") +assessment_id = qti_item.identifier[len(prefix):] +qti_item_filepath = self._qti_item_filepath(assessment_id)contentcuration/contentcuration/utils/assessment/qti/mathml/core.py (1)
186-194:⚠️ Potential issue
Semanticsrequires ≥ 2 children, not ≥ 1MathML Core §4.7 mandates a
<semantics>element contain the expression plus at least one annotation.
The current guard only rejects an empty list, so a lone presentation child (no annotations) incorrectly passes validation.- if len(v) == 0: - raise ValueError("Semantics must have at least one child") + if len(v) < 2: + raise ValueError( + "Semantics must have at least two children: presentation content " + "followed by one or more annotations" + )contentcuration/contentcuration/utils/assessment/qti/html/display.py (1)
16-18: Comment already addressed – no further action required
The earlier discussion about aliasingfor_→forwas resolved by theto_elementnormalisation logic. Re-raising it would be redundant.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 16-16: Too many ancestors (8/7)
(R0901)
🧹 Nitpick comments (16)
contentcuration/contentcuration/utils/assessment/qti/interaction_types/simple.py (1)
44-54: Mark validator as@classmethodto silence lintersPydantic works without it, but tools flag the missing
self/clsconvention.
Prepend@classmethodfor clarity and to avoid E0213 noise.- @field_validator("answers") - def _unique_answer_identifiers( + @field_validator("answers") + @classmethod + def _unique_answer_identifiers( cls, answers: List[SimpleChoice]🧰 Tools
🪛 Pylint (3.3.7)
[error] 45-45: Method '_unique_answer_identifiers' should have "self" as first argument
(E0213)
contentcuration/contentcuration/tests/utils/qti/test_fields.py (1)
255-260: Flatten nestedwithstatements for cleaner testsRuff warns (SIM117). Combine the context managers:
- for srcset in invalid_srcsets: - with self.subTest(srcset=srcset): - with self.assertRaises(ValueError): - validate_local_srcset(srcset) + for srcset in invalid_srcsets: + with self.subTest(srcset=srcset), self.assertRaises(ValueError): + validate_local_srcset(srcset)🧰 Tools
🪛 Ruff (0.11.9)
258-259: Use a single
withstatement with multiple contexts instead of nestedwithstatementsCombine
withstatements(SIM117)
contentcuration/contentcuration/utils/publish.py (1)
59-60: Remove now-unused constants to avoid dead code
PERSEUS_IMG_DIRandTHUMBNAIL_DIMENSIONare no longer referenced after the Perseus zip helpers were removed. Dead constants can confuse future readers.-PERSEUS_IMG_DIR = exercises.IMG_PLACEHOLDER + "/images" -THUMBNAIL_DIMENSION = 128contentcuration/contentcuration/utils/assessment/markdown.py (1)
60-63: Minor style: chained comparison can be simplified
endpos >= state.bMarks[line] and endpos <= state.eMarks[line]→state.bMarks[line] <= endpos <= state.eMarks[line]
No functional change; just terser.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 60-60: Simplify chained comparison between the operands
(R1716)
contentcuration/contentcuration/utils/assessment/qti/html/text.py (1)
146-152: Explicit tag name override is redundant
Ruby.element_name()lower-cases the class name manually butHTMLElement.element_name()already does this (cls.__name__.lower()). Unless the class will be subclassed further, the override can be removed to reduce noise.-class Ruby(InlineContentElement): - @classmethod - def element_name(cls): - return "ruby" +class Ruby(InlineContentElement):contentcuration/contentcuration/tests/utils/qti/test_html.py (2)
80-372: Consider parameterised, tree-based assertionsThe suite repeats the same “create element → compare hard-coded XML string” pattern hundreds of times. Using
pytest.mark.parametrize(orsubTestin unittest) plus a helper that round-trips(element → ET.tostring)would shrink the file dramatically and avoid the R0914/R0915 complaints.
Additionally, asserting on raw XML strings is brittle because attribute order is not guaranteed; comparingET.fromstring(actual).attribor usingxmltodictwill be more robust.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 282-282: Too many local variables (31/15)
(R0914)
[refactor] 282-282: Too many statements (61/50)
(R0915)
375-552: Large integration test is excellent but could be split
test_complex_html_parsingverifies many behaviours at once (22+ asserts). If any single sub-case fails the whole test is reported as one failure. Splitting into smaller focused tests (or usingsubTest) will localise failures and speed up debugging.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 375-375: Too many local variables (22/15)
(R0914)
[refactor] 375-375: Too many statements (56/50)
(R0915)
contentcuration/contentcuration/utils/assessment/base.py (1)
345-368: Open file without context managerOpening
tempf.namedirectly leaks a file descriptor until GC:- assessment_file_obj = models.File.objects.create( - file_on_disk=File(open(tempf.name, "rb"), name=filename), + with open(tempf.name, "rb") as f: + assessment_file_obj = models.File.objects.create( + file_on_disk=File(f, name=filename),(Previous discussion noted Studio’s
Filewrapper, butwithstill works and keeps linter quiet.)🧰 Tools
🪛 Ruff (0.11.9)
362-362: Use a context manager for opening files
(SIM115)
🪛 Pylint (3.3.7)
[refactor] 362-362: Consider using 'with' for resource-allocating operations
(R1732)
contentcuration/contentcuration/utils/assessment/qti/base.py (1)
124-126: Onlysrccaptured as a file dependencyAssets referenced via
href,srcset,data-src, etc., are ignored, so packages may miss required files.
Expand the predicate to a small whitelist (or regex) to ensure all external file references are tracked.contentcuration/contentcuration/utils/assessment/qti/assessment_item.py (1)
174-194: Defaulttool_versionhard-coded
tool_version="0.1"will quickly diverge from real Studio releases.
Populate this dynamically from package metadata (e.g.import kolibri; kolibri.__version__) to aid downstream tooling.contentcuration/contentcuration/utils/assessment/qti/mathml/core.py (1)
178-182: Redundant length constraint aliasYou already constrain
childrenviaLen(2)inside theAnnotated[...]declaration; adding a second manuallen(v)check (after fixing tolen(v) < 2) partially duplicates that logic.
If you tighten the first check as above, consider dropping theLen(2)here to avoid double-maintenance.contentcuration/contentcuration/tests/utils/test_exercise_creation.py (3)
1031-1036: Mismatched assertion messageThe assertion expects exactly 1 resized image but the failure message still says “Expected 2 resized images”.
- f"Expected 2 resized images, found {len(image_files)}: {image_files}", + f"Expected 1 resized image, found {len(image_files)}: {image_files}",Minor, yet misleading when the test fails.
1150-1194: String-compare entire XML ⇒ extremely fragile testsThe QTI tests inline full XML and compare string-normalised output.
Any non-semantic change (attribute order, pretty-printer whitespace, namespace declaration order) will trigger false negatives.Parse both documents with
xml.etree.ElementTree, then compare:def _eq_xml(a: str, b: str) -> bool: return ElementTree.tostring(ElementTree.fromstring(a)) == \ ElementTree.tostring(ElementTree.fromstring(b))or use
lxml.diff.This keeps coverage of critical semantics while insulating against formatter noise.
Also applies to: 1248-1281, 1515-1536
811-899: Test module size & scopeAt ~1 700 LOC this single test file couples Perseus-specific, QTI, and image-resizing tests.
Splitting into thematic modules (test_perseus_archive.py,test_qti_archive.py,test_image_resize.py) improves readability, speeds isolated failures, and sidesteps flake8R0915/R0912complaints.contentcuration/contentcuration/tests/utils/qti/test_mathml.py (2)
1-24: Monolithic test module – consider splitting for maintainability
At ~1500 LOC this single test file is hard to navigate and slows down IDE tooling. Splitting into focused modules (e.g.test_validation.py,test_serialization.py,test_edge_cases.py) will improve readability and execution parallelism without affecting coverage.
1420-1423: Unused loop variable – rename to “_”for i in range(10): content = Mrow(children=[content])
iisn’t referenced; Ruff flags this (B007). Rename to_(or_iif you prefer) to silence the warning:-for i in range(10): +for _ in range(10):🧰 Tools
🪛 Ruff (0.11.9)
1421-1421: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
contentcuration/contentcuration/tests/test_exportchannel.py(1 hunks)contentcuration/contentcuration/tests/testdata.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_assessment_items.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_fields.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_html.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_imsmanifest.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_mathml.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_qti.py(1 hunks)contentcuration/contentcuration/tests/utils/test_exercise_creation.py(17 hunks)contentcuration/contentcuration/tests/utils/test_markdown.py(1 hunks)contentcuration/contentcuration/utils/assessment/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/markdown.py(1 hunks)contentcuration/contentcuration/utils/assessment/perseus.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/__init__.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/archive.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/assessment_item.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/constants.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/fields.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/__init__.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/breaks.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/content_types.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/display.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/embed.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/flow.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/media.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/sequence.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/table.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/text.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/imsmanifest.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/interaction_types/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/interaction_types/simple.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/interaction_types/text_based.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/__init__.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/core.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/fields.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/prompt.py(1 hunks)contentcuration/contentcuration/utils/publish.py(3 hunks)requirements-dev.in(1 hunks)requirements-dev.txt(2 hunks)requirements.in(1 hunks)requirements.txt(4 hunks)
✅ Files skipped from review due to trivial changes (3)
- contentcuration/contentcuration/utils/assessment/qti/html/table.py
- contentcuration/contentcuration/utils/assessment/qti/html/base.py
- contentcuration/contentcuration/utils/assessment/qti/html/init.py
🚧 Files skipped from review as they are similar to previous changes (26)
- requirements-dev.in
- contentcuration/contentcuration/utils/assessment/qti/init.py
- requirements.in
- contentcuration/contentcuration/tests/testdata.py
- contentcuration/contentcuration/tests/test_exportchannel.py
- contentcuration/contentcuration/tests/utils/qti/test_qti.py
- contentcuration/contentcuration/utils/assessment/qti/html/breaks.py
- contentcuration/contentcuration/tests/utils/qti/test_imsmanifest.py
- contentcuration/contentcuration/utils/assessment/qti/interaction_types/base.py
- requirements-dev.txt
- contentcuration/contentcuration/utils/assessment/qti/prompt.py
- contentcuration/contentcuration/utils/assessment/perseus.py
- contentcuration/contentcuration/utils/assessment/qti/interaction_types/text_based.py
- contentcuration/contentcuration/utils/assessment/qti/mathml/init.py
- contentcuration/contentcuration/utils/assessment/qti/html/embed.py
- contentcuration/contentcuration/utils/assessment/qti/constants.py
- contentcuration/contentcuration/tests/utils/qti/test_assessment_items.py
- contentcuration/contentcuration/utils/assessment/qti/mathml/base.py
- contentcuration/contentcuration/tests/utils/test_markdown.py
- contentcuration/contentcuration/utils/assessment/qti/html/content_types.py
- contentcuration/contentcuration/utils/assessment/qti/fields.py
- contentcuration/contentcuration/utils/assessment/qti/mathml/fields.py
- contentcuration/contentcuration/utils/assessment/qti/html/sequence.py
- requirements.txt
- contentcuration/contentcuration/utils/assessment/qti/html/media.py
- contentcuration/contentcuration/utils/assessment/qti/imsmanifest.py
🧰 Additional context used
🧬 Code Graph Analysis (7)
contentcuration/contentcuration/tests/utils/qti/test_fields.py (1)
contentcuration/contentcuration/utils/assessment/qti/fields.py (4)
validate_data_uri(32-42)validate_local_href_path(45-60)validate_local_src_path(63-74)validate_local_srcset(85-99)
contentcuration/contentcuration/utils/assessment/qti/assessment_item.py (4)
contentcuration/contentcuration/utils/assessment/qti/base.py (2)
BaseSequence(170-177)QTIBase(142-151)contentcuration/contentcuration/utils/assessment/qti/constants.py (4)
BaseType(14-25)Cardinality(7-11)ExternalScored(37-39)View(28-34)contentcuration/contentcuration/utils/assessment/qti/html/base.py (1)
BlockContentElement(30-31)contentcuration/contentcuration/utils/assessment/qti/interaction_types/base.py (1)
BlockInteraction(14-15)
contentcuration/contentcuration/utils/assessment/qti/base.py (1)
contentcuration/contentcuration/utils/assessment/qti/constants.py (1)
Dir(47-50)
contentcuration/contentcuration/utils/assessment/qti/html/display.py (2)
contentcuration/contentcuration/utils/assessment/qti/html/base.py (2)
FlowContentElement(22-23)HTMLElement(11-19)contentcuration/contentcuration/utils/assessment/qti/html/text.py (2)
BlockHTMLText(20-21)InlineHTMLText(16-17)
contentcuration/contentcuration/utils/assessment/qti/html/flow.py (1)
contentcuration/contentcuration/utils/assessment/qti/html/base.py (1)
BlockContentElement(30-31)
contentcuration/contentcuration/utils/assessment/qti/html/text.py (2)
contentcuration/contentcuration/utils/assessment/qti/html/base.py (2)
BlockContentElement(30-31)InlineContentElement(26-27)contentcuration/contentcuration/utils/assessment/qti/base.py (2)
element_name(60-61)element_name(148-151)
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py (3)
contentcuration/contentcuration/utils/assessment/qti/mathml/base.py (6)
MathMLElement(12-24)MathMLGroupingElement(63-69)MathMLLayoutElement(45-51)MathMLPresentationElement(27-33)MathMLScriptElement(54-60)MathMLTokenElement(36-42)contentcuration/contentcuration/utils/assessment/qti/mathml/fields.py (2)
MathMLDisplay(58-60)MathMLForm(63-66)contentcuration/contentcuration/utils/assessment/qti/base.py (2)
element_name(60-61)element_name(148-151)
🪛 Ruff (0.11.9)
contentcuration/contentcuration/tests/utils/qti/test_fields.py
258-259: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
270-271: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
282-283: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
293-294: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
contentcuration/contentcuration/tests/utils/qti/test_mathml.py
123-124: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
174-175: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
210-211: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
1421-1421: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
contentcuration/contentcuration/utils/assessment/base.py
339-340: ExerciseArchiveGenerator.handle_before_assessment_items is an empty method in an abstract base class, but has no abstract decorator
(B027)
342-343: ExerciseArchiveGenerator.handle_after_assessment_items is an empty method in an abstract base class, but has no abstract decorator
(B027)
362-362: Use a context manager for opening files
(SIM115)
🪛 Pylint (3.3.7)
contentcuration/contentcuration/utils/assessment/base.py
[refactor] 41-41: Too many instance attributes (9/7)
(R0902)
[refactor] 91-91: Too many arguments (6/5)
(R0913)
[refactor] 91-91: Too many positional arguments (6/5)
(R0917)
[refactor] 146-146: Either all return statements in a function should return an expression, or none of them should.
(R1710)
[refactor] 160-160: Too many arguments (7/5)
(R0913)
[refactor] 160-160: Too many positional arguments (7/5)
(R0917)
[refactor] 160-160: Either all return statements in a function should return an expression, or none of them should.
(R1710)
[refactor] 181-181: Too many arguments (7/5)
(R0913)
[refactor] 181-181: Too many positional arguments (7/5)
(R0917)
[refactor] 362-362: Consider using 'with' for resource-allocating operations
(R1732)
contentcuration/contentcuration/tests/utils/test_exercise_creation.py
[refactor] 1076-1076: Too many arguments (6/5)
(R0913)
[refactor] 1076-1076: Too many positional arguments (6/5)
(R0917)
[refactor] 1116-1116: Consider using 'with' for resource-allocating operations
(R1732)
contentcuration/contentcuration/utils/assessment/qti/base.py
[refactor] 39-39: Too few public methods (0/2)
(R0903)
[refactor] 83-112: Unnecessary "elif" after "raise", remove the leading "el" from "elif"
(R1720)
[refactor] 72-125: Too many nested blocks (6/5)
(R1702)
[refactor] 63-63: Too many branches (16/12)
(R0912)
contentcuration/contentcuration/tests/utils/qti/test_html.py
[refactor] 282-282: Too many local variables (31/15)
(R0914)
[refactor] 282-282: Too many statements (61/50)
(R0915)
[refactor] 375-375: Too many local variables (22/15)
(R0914)
[refactor] 375-375: Too many statements (56/50)
(R0915)
contentcuration/contentcuration/utils/assessment/markdown.py
[refactor] 60-60: Simplify chained comparison between the operands
(R1716)
contentcuration/contentcuration/utils/assessment/qti/html/display.py
[refactor] 16-16: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/utils/assessment/qti/html/flow.py
[refactor] 20-20: Too many ancestors (8/7)
(R0901)
[refactor] 24-24: Too many ancestors (8/7)
(R0901)
[refactor] 28-28: Too many ancestors (8/7)
(R0901)
[refactor] 32-32: Too many ancestors (8/7)
(R0901)
[refactor] 36-36: Too many ancestors (8/7)
(R0901)
[refactor] 40-40: Too many ancestors (8/7)
(R0901)
[refactor] 44-44: Too many ancestors (8/7)
(R0901)
[refactor] 48-48: Too many ancestors (8/7)
(R0901)
[refactor] 52-52: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/utils/assessment/qti/html/text.py
[refactor] 24-24: Too many ancestors (8/7)
(R0901)
[refactor] 29-29: Too many ancestors (8/7)
(R0901)
[refactor] 33-33: Too many ancestors (8/7)
(R0901)
[refactor] 37-37: Too many ancestors (8/7)
(R0901)
[refactor] 41-41: Too many ancestors (8/7)
(R0901)
[refactor] 45-45: Too many ancestors (8/7)
(R0901)
[refactor] 49-49: Too many ancestors (8/7)
(R0901)
[refactor] 53-53: Too many ancestors (8/7)
(R0901)
[refactor] 57-57: Too many ancestors (8/7)
(R0901)
[refactor] 61-61: Too many ancestors (8/7)
(R0901)
[refactor] 65-65: Too many ancestors (8/7)
(R0901)
[refactor] 69-69: Too many ancestors (8/7)
(R0901)
[refactor] 73-73: Too many ancestors (8/7)
(R0901)
[refactor] 77-77: Too many ancestors (8/7)
(R0901)
[refactor] 81-81: Too many ancestors (8/7)
(R0901)
[refactor] 85-85: Too many ancestors (8/7)
(R0901)
[refactor] 89-89: Too many ancestors (8/7)
(R0901)
[refactor] 93-93: Too many ancestors (8/7)
(R0901)
[refactor] 97-97: Too many ancestors (8/7)
(R0901)
[refactor] 101-101: Too many ancestors (8/7)
(R0901)
[refactor] 105-105: Too many ancestors (8/7)
(R0901)
[refactor] 109-109: Too many ancestors (8/7)
(R0901)
[refactor] 113-113: Too many ancestors (8/7)
(R0901)
[refactor] 117-117: Too many ancestors (8/7)
(R0901)
[refactor] 121-121: Too many ancestors (8/7)
(R0901)
[refactor] 130-130: Too many ancestors (8/7)
(R0901)
[refactor] 134-134: Too many ancestors (8/7)
(R0901)
[refactor] 138-138: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/utils/assessment/qti/interaction_types/simple.py
[error] 45-45: Method '_unique_answer_identifiers' should have "self" as first argument
(E0213)
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py
[refactor] 111-111: Too many ancestors (8/7)
(R0901)
[refactor] 115-115: Too many ancestors (8/7)
(R0901)
[refactor] 119-119: Too many ancestors (8/7)
(R0901)
[refactor] 123-123: Too many ancestors (8/7)
(R0901)
[refactor] 128-128: Too many ancestors (8/7)
(R0901)
[refactor] 133-133: Too many ancestors (8/7)
(R0901)
[refactor] 143-143: Too many ancestors (8/7)
(R0901)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build frontend assets
- GitHub Check: Python unit tests
🔇 Additional comments (2)
contentcuration/contentcuration/utils/publish.py (1)
334-343: Great addition: stale archive cleanup prevents duplicate ZIPs
stale_presetslogic ensures we don’t leave both Perseus and QTI archives attached when the generator switches – this plugs a storage/consistency gap flagged earlier.
Nice, self-contained fix.contentcuration/contentcuration/utils/assessment/markdown.py (1)
15-17:⚠️ Potential issueBlock-math regex disallows multi-line expressions
BLOCK_PATTERN = r"^\$\$([^$]+?)\$\$"uses[^$]+?, so it stops at the first newline and fails for perfectly legal multi-line TeX blocks like:$$ \begin{aligned} a &= b+c\\ d &= e-f \end{aligned} $$Switch to a DOTALL / non-greedy pattern so
.matches newlines:-INLINE_PATTERN = re.compile(r"^\$\$([^$]+?)\$\$") -BLOCK_PATTERN = re.compile(r"^\$\$([^$]+?)\$\$", re.M) +INLINE_PATTERN = re.compile(r"^\$\$(.+?)\$\$") # still single-line +# DOTALL allows newlines inside the block match +BLOCK_PATTERN = re.compile(r"^\$\$(.+?)\$\$", re.M | re.S)Without this, any multi-line
$$ … $$silently goes unmatched and renders raw in output.Likely an incorrect or invalid review comment.
contentcuration/contentcuration/utils/assessment/qti/html/display.py
Outdated
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
contentcuration/contentcuration/tests/utils/test_exercise_creation.py (1)
96-116: 🛠️ Refactor suggestionPrefer context-manager when opening ZipFile
zipfile.ZipFileowns system resources that should be released promptly.
Using it without awithblock works but risks leaving file handles open, and
also triggers thepylint R1732warning you’ve seen.- zip_file = zipfile.ZipFile(BytesIO(zip_data)) + with zipfile.ZipFile(BytesIO(zip_data)) as zip_file: + # … existing assertions … + # Return the zip object and data for additional assertions + return zip_file, exercise_data - - # Return the zip object and data for additional assertions - return zip_file, exercise_dataYou can store the file list or any needed artefacts before the
with
statement ends.
Same pattern is repeated in_validate_qti_zip_structure; worth fixing there
too.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 102-102: Consider using 'with' for resource-allocating operations
(R1732)
♻️ Duplicate comments (2)
contentcuration/contentcuration/utils/assessment/base.py (1)
146-158:⚠️ Potential issue
_get_similar_imagecrashes when only one dimension is suppliedWhen either
widthorheightisNone(legal, given the regex and upstream
call-site) the divisionabs(key[0] - width) / widthraisesTypeError.- if ( - abs(key[0] - width) / width < 0.01 - and abs(key[1] - height) / height < 0.01 - ): + if width and height and ( + abs(key[0] - width) / width < 0.01 + and abs(key[1] - height) / height < 0.01 + ):Alternatively short-circuit earlier and only attempt similarity matching when
both dimensions are present.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 146-146: Either all return statements in a function should return an expression, or none of them should.
(R1710)
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py (1)
178-183: 🛠️ Refactor suggestionAdd structural validator to guarantee ≥ 1 annotation
Current logic checks first-child type but does not enforce that any
annotation follows; a list like[Mrow(...)]will passLen(2)typing but be
rejected only by annotated-types at runtime, giving a cryptic message.
Add an explicit length check for clarity:@@ - # Remaining children must be annotations + if len(v) < 2: + raise ValueError( + "Semantics requires at least one annotation child after the " + "presentation element" + ) + # Remaining children must be annotationsThis yields a domain-specific error instead of a generic “too short” one.
🧹 Nitpick comments (12)
contentcuration/contentcuration/utils/assessment/perseus.py (1)
81-83: Optional: pre-compile regex at module import for marginal speed.
_DOUBLE_DOLLAR_REis already compiled – all good. Consider addingre.MULTILINEif you expect multiple blocks per line, but non-blocking.contentcuration/contentcuration/tests/utils/qti/test_fields.py (1)
258-261: Style nit: merge nestedwithblocks for readability.Ruff SIM117 flags several spots; combining the contexts keeps tests terse:
with self.assertRaises(ValueError), self.subTest(srcset=srcset): validate_local_srcset(srcset)🧰 Tools
🪛 Ruff (0.11.9)
258-259: Use a single
withstatement with multiple contexts instead of nestedwithstatementsCombine
withstatements(SIM117)
contentcuration/contentcuration/utils/publish.py (3)
324-333: Selection logic is clear but misses mixed-type optimisationIf an exercise contains BOTH free-response and choice items we still switch
to the QTI generator; great.
Consider renamingany_free_response→needs_qti(or similar) to reflect this
broader intent and aid readability for future maintainers.
336-343: Good call removing stale archives – tiny safety tweakDeleting old
Filerows blindly could orphan their linkedLocalFile
instances if no otherFiles reference them, potentially triggering stale-file
cleanup later.
A quick safeguard could be to chain a.exists()check before delete:stale_qs = node.files.filter(preset_id__in=stale_presets) if stale_qs.exists(): stale_qs.delete()Not critical, but avoids an unnecessary DELETE when nothing is present.
350-357: Minor: avoid repeated lang_code lookup
self.default_language.lang_codeis dereferenced for every exercise. Cache it
once inTreeMapper.__init__(e.g.self.default_lang_code) to shave a few
thousand attribute hits in large channels.contentcuration/contentcuration/tests/utils/qti/test_mathml.py (2)
1415-1431: Rename unused loop variable to silence linters
for i in range(10):–iis never used.-for i in range(10): - content = Mrow(children=[content]) +for _ in range(10): + content = Mrow(children=[content])🧰 Tools
🪛 Ruff (0.11.9)
1421-1421: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
1398-1413: Very long string slows the suiteCreating/serialising 10 000-char text every run adds noticeable time. Dropping
to 2 000 chars keeps edge-case coverage while cutting runtime.contentcuration/contentcuration/utils/assessment/markdown.py (1)
57-63: Micro-nit: chained comparison is clearer & silences the linter- if endpos >= state.bMarks[line] and endpos <= state.eMarks[line]: + if state.bMarks[line] <= endpos <= state.eMarks[line]:Purely stylistic – feel free to ignore.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 60-60: Simplify chained comparison between the operands
(R1716)
contentcuration/contentcuration/utils/assessment/base.py (1)
344-348: Mark optional hooks as@abstractmethodor document they’re truly optionalStatic-analysis warnings (
B027) point out that
handle_before_assessment_itemsandhandle_after_assessment_itemslook like
abstract hooks but are not marked as such.
If subclasses are expected to override them, add@abstractmethod; if not,
leave them as is but add a docstring clarifying they are no-ops by design.🧰 Tools
🪛 Ruff (0.11.9)
344-345:
ExerciseArchiveGenerator.handle_before_assessment_itemsis an empty method in an abstract base class, but has no abstract decorator(B027)
347-348:
ExerciseArchiveGenerator.handle_after_assessment_itemsis an empty method in an abstract base class, but has no abstract decorator(B027)
contentcuration/contentcuration/utils/assessment/qti/assessment_item.py (1)
36-39: Field namevaluecollides with attribute name in XML vocabularyInside
<qti-value>the spec uses element content rather than an attribute
named “value”, so the current mapping is fine.
Just flagging that if a future serialiser treats unknown scalar fields as
attributes, you might accidentally emitvalue="…", duplicating the text
node.contentcuration/contentcuration/tests/utils/test_exercise_creation.py (1)
187-191: Hard-coded checksum is brittleThe test asserts on a fixed checksum that will change any time the archive
generation code is legitimately refactored (even whitespace or timestamp
differences). This makes the test fragile and hinders refactors that don’t
affect functional behaviour.Consider:
- asserting on deterministic aspects you actually care about (file list, item
JSON, etc.—all of which you are already checking), and- dropping the checksum assertion, or computing an SHA only over the
exercise.jsoncanonical content.contentcuration/contentcuration/utils/assessment/qti/mathml/core.py (1)
186-189: Validator message inconsistent with requirement
Semanticsmust have at least two children (presentation + ≥1 annotation).
The error now says “at least one child”, which can mislead users.- raise ValueError("Semantics must have at least one child") + raise ValueError("Semantics must have at least two children " + "(presentation element plus ≥1 annotation)")No functional change—just clearer diagnostics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
contentcuration/contentcuration/tests/test_exportchannel.py(1 hunks)contentcuration/contentcuration/tests/testdata.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_assessment_items.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_fields.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_html.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_imsmanifest.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_mathml.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_qti.py(1 hunks)contentcuration/contentcuration/tests/utils/test_exercise_creation.py(18 hunks)contentcuration/contentcuration/tests/utils/test_markdown.py(1 hunks)contentcuration/contentcuration/utils/assessment/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/markdown.py(1 hunks)contentcuration/contentcuration/utils/assessment/perseus.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/__init__.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/archive.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/assessment_item.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/constants.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/fields.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/__init__.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/breaks.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/content_types.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/display.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/embed.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/flow.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/media.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/sequence.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/table.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/text.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/imsmanifest.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/interaction_types/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/interaction_types/simple.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/interaction_types/text_based.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/__init__.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/core.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/fields.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/prompt.py(1 hunks)contentcuration/contentcuration/utils/publish.py(3 hunks)requirements-dev.in(1 hunks)requirements-dev.txt(2 hunks)requirements.in(1 hunks)requirements.txt(4 hunks)
✅ Files skipped from review due to trivial changes (4)
- contentcuration/contentcuration/utils/assessment/qti/constants.py
- contentcuration/contentcuration/utils/assessment/qti/html/table.py
- contentcuration/contentcuration/utils/assessment/qti/html/base.py
- contentcuration/contentcuration/utils/assessment/qti/html/init.py
🚧 Files skipped from review as they are similar to previous changes (24)
- requirements-dev.in
- requirements.in
- contentcuration/contentcuration/tests/test_exportchannel.py
- contentcuration/contentcuration/utils/assessment/qti/prompt.py
- contentcuration/contentcuration/tests/testdata.py
- contentcuration/contentcuration/utils/assessment/qti/init.py
- requirements-dev.txt
- contentcuration/contentcuration/utils/assessment/qti/html/breaks.py
- contentcuration/contentcuration/utils/assessment/qti/interaction_types/base.py
- requirements.txt
- contentcuration/contentcuration/tests/utils/qti/test_qti.py
- contentcuration/contentcuration/tests/utils/qti/test_assessment_items.py
- contentcuration/contentcuration/tests/utils/qti/test_imsmanifest.py
- contentcuration/contentcuration/utils/assessment/qti/html/media.py
- contentcuration/contentcuration/utils/assessment/qti/html/content_types.py
- contentcuration/contentcuration/utils/assessment/qti/mathml/init.py
- contentcuration/contentcuration/tests/utils/test_markdown.py
- contentcuration/contentcuration/utils/assessment/qti/interaction_types/text_based.py
- contentcuration/contentcuration/utils/assessment/qti/html/embed.py
- contentcuration/contentcuration/utils/assessment/qti/mathml/base.py
- contentcuration/contentcuration/utils/assessment/qti/mathml/fields.py
- contentcuration/contentcuration/utils/assessment/qti/fields.py
- contentcuration/contentcuration/utils/assessment/qti/html/sequence.py
- contentcuration/contentcuration/utils/assessment/qti/imsmanifest.py
🧰 Additional context used
🧬 Code Graph Analysis (5)
contentcuration/contentcuration/tests/utils/qti/test_mathml.py (5)
contentcuration/contentcuration/utils/assessment/qti/base.py (5)
TextNode(39-42)element_name(67-68)element_name(155-158)to_xml_string(138-141)from_string(281-289)contentcuration/contentcuration/utils/assessment/qti/constants.py (1)
Dir(47-50)contentcuration/contentcuration/utils/assessment/qti/mathml/core.py (28)
Annotation(163-166)AnnotationXml(169-175)Math(28-31)Mfrac(77-79)Mi(34-37)Mn(40-41)Mo(44-56)Mspace(67-70)Msubsup(119-120)Mtable(159-160)Mtd(149-152)Mtr(155-156)Semantics(178-203)Merror(94-95)Mmultiscripts(143-146)Mover(128-130)Mphantom(107-108)Mprescripts(139-140)Mroot(86-87)Ms(63-64)Msqrt(82-83)Mstyle(90-91)Msub(111-112)Msup(115-116)Mtext(59-60)Munder(123-125)Munderover(133-136)element_name(174-175)contentcuration/contentcuration/utils/assessment/qti/mathml/fields.py (2)
MathMLDisplay(58-60)MathMLForm(63-66)contentcuration/contentcuration/utils/assessment/qti/mathml/base.py (5)
MathMLElement(12-24)MathMLGroupingElement(63-69)MathMLLayoutElement(45-51)MathMLScriptElement(54-60)MathMLTokenElement(36-42)
contentcuration/contentcuration/utils/assessment/qti/assessment_item.py (4)
contentcuration/contentcuration/utils/assessment/qti/base.py (2)
BaseSequence(177-184)QTIBase(149-158)contentcuration/contentcuration/utils/assessment/qti/constants.py (4)
BaseType(14-25)Cardinality(7-11)ExternalScored(37-39)View(28-34)contentcuration/contentcuration/utils/assessment/qti/html/base.py (1)
BlockContentElement(30-31)contentcuration/contentcuration/utils/assessment/qti/interaction_types/base.py (1)
BlockInteraction(14-15)
contentcuration/contentcuration/tests/utils/qti/test_fields.py (1)
contentcuration/contentcuration/utils/assessment/qti/fields.py (4)
validate_data_uri(32-42)validate_local_href_path(45-60)validate_local_src_path(63-74)validate_local_srcset(85-99)
contentcuration/contentcuration/utils/assessment/qti/html/flow.py (1)
contentcuration/contentcuration/utils/assessment/qti/html/base.py (1)
BlockContentElement(30-31)
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py (3)
contentcuration/contentcuration/utils/assessment/qti/mathml/base.py (6)
MathMLElement(12-24)MathMLGroupingElement(63-69)MathMLLayoutElement(45-51)MathMLPresentationElement(27-33)MathMLScriptElement(54-60)MathMLTokenElement(36-42)contentcuration/contentcuration/utils/assessment/qti/mathml/fields.py (2)
MathMLDisplay(58-60)MathMLForm(63-66)contentcuration/contentcuration/utils/assessment/qti/base.py (2)
element_name(67-68)element_name(155-158)
🪛 Ruff (0.11.9)
contentcuration/contentcuration/tests/utils/qti/test_mathml.py
123-124: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
174-175: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
210-211: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
1421-1421: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
contentcuration/contentcuration/tests/utils/qti/test_fields.py
258-259: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
270-271: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
282-283: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
293-294: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
contentcuration/contentcuration/utils/assessment/base.py
344-345: ExerciseArchiveGenerator.handle_before_assessment_items is an empty method in an abstract base class, but has no abstract decorator
(B027)
347-348: ExerciseArchiveGenerator.handle_after_assessment_items is an empty method in an abstract base class, but has no abstract decorator
(B027)
367-367: Use a context manager for opening files
(SIM115)
🪛 Pylint (3.3.7)
contentcuration/contentcuration/utils/assessment/qti/html/display.py
[refactor] 17-17: Too many ancestors (8/7)
(R0901)
[error] 34-34: Method 'validate_summary_position' should have "self" as first argument
(E0213)
[error] 58-58: Method 'validate_figcaption_position' should have "self" as first argument
(E0213)
contentcuration/contentcuration/utils/assessment/qti/base.py
[refactor] 39-39: Too few public methods (0/2)
(R0903)
[refactor] 90-119: Unnecessary "elif" after "raise", remove the leading "el" from "elif"
(R1720)
[refactor] 79-132: Too many nested blocks (6/5)
(R1702)
[refactor] 70-70: Too many branches (16/12)
(R0912)
contentcuration/contentcuration/tests/utils/qti/test_html.py
[refactor] 322-322: Too many local variables (31/15)
(R0914)
[refactor] 322-322: Too many statements (61/50)
(R0915)
[refactor] 415-415: Too many local variables (22/15)
(R0914)
[refactor] 415-415: Too many statements (56/50)
(R0915)
contentcuration/contentcuration/tests/utils/test_exercise_creation.py
[refactor] 192-192: Too many local variables (20/15)
(R0914)
[refactor] 1154-1154: Too many arguments (6/5)
(R0913)
[refactor] 1154-1154: Too many positional arguments (6/5)
(R0917)
[refactor] 1194-1194: Consider using 'with' for resource-allocating operations
(R1732)
contentcuration/contentcuration/utils/assessment/base.py
[refactor] 41-41: Too many instance attributes (9/7)
(R0902)
[refactor] 91-91: Too many arguments (6/5)
(R0913)
[refactor] 91-91: Too many positional arguments (6/5)
(R0917)
[refactor] 146-146: Either all return statements in a function should return an expression, or none of them should.
(R1710)
[refactor] 160-160: Too many arguments (7/5)
(R0913)
[refactor] 160-160: Too many positional arguments (7/5)
(R0917)
[refactor] 160-160: Either all return statements in a function should return an expression, or none of them should.
(R1710)
[refactor] 181-181: Too many arguments (7/5)
(R0913)
[refactor] 181-181: Too many positional arguments (7/5)
(R0917)
[refactor] 367-367: Consider using 'with' for resource-allocating operations
(R1732)
contentcuration/contentcuration/utils/assessment/markdown.py
[refactor] 60-60: Simplify chained comparison between the operands
(R1716)
contentcuration/contentcuration/utils/assessment/qti/html/flow.py
[refactor] 20-20: Too many ancestors (8/7)
(R0901)
[refactor] 24-24: Too many ancestors (8/7)
(R0901)
[refactor] 28-28: Too many ancestors (8/7)
(R0901)
[refactor] 32-32: Too many ancestors (8/7)
(R0901)
[refactor] 36-36: Too many ancestors (8/7)
(R0901)
[refactor] 40-40: Too many ancestors (8/7)
(R0901)
[refactor] 44-44: Too many ancestors (8/7)
(R0901)
[refactor] 48-48: Too many ancestors (8/7)
(R0901)
[refactor] 52-52: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/utils/assessment/qti/html/text.py
[refactor] 24-24: Too many ancestors (8/7)
(R0901)
[refactor] 29-29: Too many ancestors (8/7)
(R0901)
[refactor] 33-33: Too many ancestors (8/7)
(R0901)
[refactor] 37-37: Too many ancestors (8/7)
(R0901)
[refactor] 41-41: Too many ancestors (8/7)
(R0901)
[refactor] 45-45: Too many ancestors (8/7)
(R0901)
[refactor] 49-49: Too many ancestors (8/7)
(R0901)
[refactor] 53-53: Too many ancestors (8/7)
(R0901)
[refactor] 57-57: Too many ancestors (8/7)
(R0901)
[refactor] 61-61: Too many ancestors (8/7)
(R0901)
[refactor] 65-65: Too many ancestors (8/7)
(R0901)
[refactor] 69-69: Too many ancestors (8/7)
(R0901)
[refactor] 73-73: Too many ancestors (8/7)
(R0901)
[refactor] 77-77: Too many ancestors (8/7)
(R0901)
[refactor] 81-81: Too many ancestors (8/7)
(R0901)
[refactor] 85-85: Too many ancestors (8/7)
(R0901)
[refactor] 89-89: Too many ancestors (8/7)
(R0901)
[refactor] 93-93: Too many ancestors (8/7)
(R0901)
[refactor] 97-97: Too many ancestors (8/7)
(R0901)
[refactor] 101-101: Too many ancestors (8/7)
(R0901)
[refactor] 105-105: Too many ancestors (8/7)
(R0901)
[refactor] 109-109: Too many ancestors (8/7)
(R0901)
[refactor] 113-113: Too many ancestors (8/7)
(R0901)
[refactor] 117-117: Too many ancestors (8/7)
(R0901)
[refactor] 121-121: Too many ancestors (8/7)
(R0901)
[refactor] 130-130: Too many ancestors (8/7)
(R0901)
[refactor] 134-134: Too many ancestors (8/7)
(R0901)
[refactor] 138-138: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/utils/assessment/qti/interaction_types/simple.py
[error] 45-45: Method '_unique_answer_identifiers' should have "self" as first argument
(E0213)
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py
[refactor] 111-111: Too many ancestors (8/7)
(R0901)
[refactor] 115-115: Too many ancestors (8/7)
(R0901)
[refactor] 119-119: Too many ancestors (8/7)
(R0901)
[refactor] 123-123: Too many ancestors (8/7)
(R0901)
[refactor] 128-128: Too many ancestors (8/7)
(R0901)
[refactor] 133-133: Too many ancestors (8/7)
(R0901)
[refactor] 143-143: Too many ancestors (8/7)
(R0901)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build all message files
- GitHub Check: Build frontend assets
- GitHub Check: All file linting
- GitHub Check: Python unit tests
🔇 Additional comments (6)
contentcuration/contentcuration/utils/assessment/qti/interaction_types/simple.py (1)
44-54: Pylint false-positive:_unique_answer_identifiersdoesn’t needself.The method signature (
cls, answers) is correct for a@field_validator; Pylint R0203/E0213 can safely be silenced via noqa or cfg.🧰 Tools
🪛 Pylint (3.3.7)
[error] 45-45: Method '_unique_answer_identifiers' should have "self" as first argument
(E0213)
contentcuration/contentcuration/utils/assessment/perseus.py (1)
70-79: Nice guard on Graphie delimiter – improves robustness.contentcuration/contentcuration/utils/assessment/qti/html/display.py (2)
29-48: Validator logic looks sound – pylint warning is a false positive
field_validator("children")is correctly declared withclsfor Pydantic v2.
The E0213 lint error can be ignored.🧰 Tools
🪛 Pylint (3.3.7)
[error] 34-34: Method 'validate_summary_position' should have "self" as first argument
(E0213)
55-68: Nice spec-compliant guard ✅Single-
<figcaption>check plus first/last-position rule is spot-on.🧰 Tools
🪛 Pylint (3.3.7)
[error] 58-58: Method 'validate_figcaption_position' should have "self" as first argument
(E0213)
contentcuration/contentcuration/utils/assessment/qti/base.py (1)
59-65: 👍 Cache-invalidating__setattr__fixes the stale XML bugGreat to see the cache invalidation added; this resolves the earlier concern
aboutto_element()returning outdated XML after mutations.contentcuration/contentcuration/tests/utils/qti/test_html.py (1)
182-190: URL normalisation in expectation may be brittleThe assertion expects
cite="http://test.com/"(trailing slash) while the input
was"http://test.com". ElementTree does not append the slash, so this test
may fail on some platforms. Consider asserting withassertIn('cite="http://test.com', …)or normalising withurlparse.
contentcuration/contentcuration/utils/assessment/qti/archive.py
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py
Outdated
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
contentcuration/contentcuration/utils/assessment/qti/html/flow.py (1)
20-22:citestill too restrictive – allow package-internal refsUsing
HttpUrlrejects relative or fragment paths that are valid inside a QTI package (e.g.#ref,images/foo.svg). Switch to the customLocalHrefPathorAnyUrlto avoid false validation failures.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 20-20: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/tests/utils/qti/test_assessment_items.py (1)
103-108:⚠️ Potential issueTest fails: language case mismatch
AssessmentItem.languageis set to"EN-US"butexpected_xmluses"en-US".
Update the code or expectation so both sides agree.- language="EN-US", + language="en-US",contentcuration/contentcuration/utils/assessment/qti/html/text.py (1)
121-123:citestill requires absolute URLs – previous feedback standsThe
Q.citefield is typed asAnyUrl, which rejects perfectly valid relative references ("#ref","notes.html").
As flagged in an earlier review, consider relaxing this tostrwith a custom validator if/when relative URLs become necessary.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 121-121: Too many ancestors (8/7)
(R0901)
🧹 Nitpick comments (15)
contentcuration/contentcuration/utils/assessment/qti/interaction_types/simple.py (1)
44-54: Silence the Pylint false-positive on validatorPylint complains about the first arg of
_unique_answer_identifiers; the signature is correct for@field_validator.
Add a local disable to keep CI green:@field_validator("answers") # pylint: disable=no-self-argument🧰 Tools
🪛 Pylint (3.3.7)
[error] 45-45: Method '_unique_answer_identifiers' should have "self" as first argument
(E0213)
contentcuration/contentcuration/tests/utils/qti/test_fields.py (1)
257-260: CombinesubTestandassertRaisesinto a singlewithRuff (SIM117) is flagging the nested
withblocks. Flattening them makes each test case more concise without affecting readability:-for srcset in invalid_srcsets: - with self.subTest(srcset=srcset): - with self.assertRaises(ValueError): - validate_local_srcset(srcset) +for srcset in invalid_srcsets: + with self.subTest(srcset=srcset), self.assertRaises(ValueError): + validate_local_srcset(srcset)Applies equally to the other invalid-case loops highlighted above.
Also applies to: 269-273, 281-285, 293-295
🧰 Tools
🪛 Ruff (0.11.9)
258-259: Use a single
withstatement with multiple contexts instead of nestedwithstatementsCombine
withstatements(SIM117)
contentcuration/contentcuration/tests/utils/qti/test_mathml.py (2)
122-126: Consolidate nestedwithstatementsThe three test blocks that raise
ValidationErroruse two nestedwithstatements.
PEP 343 allows combining them to reduce indentation and noise:-with self.assertRaises(ValidationError): - Mi(mathsize=value, children=["x"]) +with self.assertRaises(ValidationError), \ + self.subTest(value=value): + Mi(mathsize=value, children=["x"])Doing this across the three highlighted sections removes seven indentation levels and satisfies Ruff SIM117 without altering behaviour.
Also applies to: 174-176, 210-212
🧰 Tools
🪛 Ruff (0.11.9)
123-124: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
1420-1423: Rename unused loop variable
for i in range(10):never usesi; rename to_(or drop the loop entirely viafor _ in range(10):) to silence Ruff B007 and clarify intent.-for i in range(10): - content = Mrow(children=[content]) +for _ in range(10): + content = Mrow(children=[content])🧰 Tools
🪛 Ruff (0.11.9)
1421-1421: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
contentcuration/contentcuration/utils/assessment/markdown.py (1)
60-62: Minor readability win – use chained comparisonThe condition can be written more idiomatically and silences Pylint R1716:
- if endpos >= state.bMarks[line] and endpos <= state.eMarks[line]: + if state.bMarks[line] <= endpos <= state.eMarks[line]:No functional change, just clearer code.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 60-60: Simplify chained comparison between the operands
(R1716)
contentcuration/contentcuration/utils/assessment/base.py (4)
355-364: Use a context-manager when re-reading the temporary ZIP for DB upload
open(tempf.name, "rb")is left un-closed.
Although the process ends soon after, it’s best practice to wrap this in a
withstatement just like the earlier loop above. This prevents a dangling
file-handle and makes the intent clearer.- assessment_file_obj = models.File.objects.create( - file_on_disk=File(open(tempf.name, "rb"), name=filename), + with open(tempf.name, "rb") as f: + assessment_file_obj = models.File.objects.create( + file_on_disk=File(f, name=filename), ... )
348-353: Mark hook methods as@abstractmethodor document default no-op
handle_before_assessment_itemsandhandle_after_assessment_itemsare intended
extension points but are currently empty concrete methods.
Static-analysis flags them (Ruff B027) because subclasses may forget to
override. Either:
- Decorate them with
@abstractmethod, or- Add a short docstring explaining that the empty implementation is the
default behaviour.The first option is safer.
🧰 Tools
🪛 Ruff (0.11.9)
348-349:
ExerciseArchiveGenerator.handle_before_assessment_itemsis an empty method in an abstract base class, but has no abstract decorator(B027)
351-352:
ExerciseArchiveGenerator.handle_after_assessment_itemsis an empty method in an abstract base class, but has no abstract decorator(B027)
146-158: Guard against division by zero in_get_similar_image
abs(key[0] - width) / widthwill still raise if a caller accidentally passes
width == 0despite the earlier filter. A defensive early-return keeps this
helper robust in isolation:- for key, resized_image in self.resized_images_map[filename].items(): - if ( - abs(key[0] - width) / width < 0.01 - and abs(key[1] - height) / height < 0.01 - ): - return resized_image + if not width or not height: + return None + for key, resized_image in self.resized_images_map[filename].items(): + if ( + abs(key[0] - width) / width < 0.01 + and abs(key[1] - height) / height < 0.01 + ): + return resized_image🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 146-146: Either all return statements in a function should return an expression, or none of them should.
(R1710)
219-231: Consider whitelisting allowed image extensions
_is_valid_image_filenamecurrently checks only for the presence of an
extension and a hex checksum prefix. Supplyingimage.exewould pass this test.
Explicitly allow a set of safe extensions (.png,.jpg,.jpeg,.gif,
.webp, …) to avoid mistakenly packaging unsupported or unsafe files.contentcuration/contentcuration/tests/utils/qti/test_html.py (1)
322-349: Split monolithic “text elements” assertion block for clearer failures
test_text_elementscontains 30 + independent assertions. When one assertion
fails the entire block aborts, making it harder to see additional breakages.Consider parameterising with
subTestorpytest.param(if using pytest) so
each element is verified in its own sub-case:for constructor, expected in [ (lambda: A(children=['x'], href='y'), '<a href="y">x</a>'), (lambda: P(children=['x']), '<p>x</p>'), ... ]: with self.subTest(expected=expected): self.assertEqual(constructor().to_xml_string(), expected)This keeps the test readable and pin-points the exact element that regresses.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 322-322: Too many local variables (31/15)
(R0914)
[refactor] 322-322: Too many statements (61/50)
(R0915)
contentcuration/contentcuration/utils/assessment/qti/base.py (1)
70-132: Cyclomatic complexity still high; extract helpers for readability
to_elementhandles attributes, child lists, enums, booleans and dependency
tracking in one 60-line method. Extracting helpers like
_serialize_attribute,_serialize_children,_append_text_tailwould shrink
branch depth and make future maintenance (e.g., adding CDATA support) easier.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 90-119: Unnecessary "elif" after "raise", remove the leading "el" from "elif"
(R1720)
[refactor] 79-132: Too many nested blocks (6/5)
(R1702)
[refactor] 70-70: Too many branches (16/12)
(R0912)
contentcuration/contentcuration/tests/utils/test_exercise_creation.py (2)
280-286: Rename unused loop variable to underscore
image_nameisn’t referenced inside the loop – onlywidthandheightare used. Keeping an unused variable trips Ruff’sB007and slightly mis-leads readers.-for image_name, image_data in question_images.items(): +for _image_name, image_data in question_images.items():🧰 Tools
🪛 Ruff (0.11.9)
280-280: Loop control variable
image_namenot used within loop bodyRename unused
image_nameto_image_name(B007)
1260-1290: XML assertions are extremely brittle – compare the DOM, not raw stringsThe tests hard-code entire XML documents and then compare the raw serialisation (after newline-stripping). Any innocuous change such as attribute order, pretty-printer differences, or namespace prefix variations will break these tests while the generated XML is still perfectly valid and schema-compliant.
Consider parsing both strings with
xml.etree.ElementTree(orlxml) and asserting structural equivalence:from xml.etree import ElementTree as ET def assert_xml_equal(expected: str, actual: str): exp_tree = ET.fromstring(expected) act_tree = ET.fromstring(actual) assert ET.tostring(exp_tree) == ET.tostring(act_tree)This will drastically reduce false-negative test failures and maintenance overhead.
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py (2)
48-52: Comment contradicts spec — remove “infinity” noteThe latest MathML Core draft (per earlier discussion with @rtibbles) no longer allows the literal string
"infinity"formaxsize/minsize. The inline comment therefore misleads future readers.- maxsize: Optional[LengthPercentage] = None # Can be "infinity" + maxsize: Optional[LengthPercentage] = None
189-204: Ensure at least one annotation child
Len(2)already enforces a minimum of two children, yet an explicit runtime guard is still valuable for clarity and for the (unlikely) case that the type annotation is bypassed.Add an early check for
len(v) < 2and a dedicated error message before iterating overv[1:].- if len(v) == 0: - raise ValueError("Semantics must have at least one child") + if len(v) < 2: + raise ValueError("Semantics requires a presentation element followed by at least one annotation")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
contentcuration/contentcuration/tests/test_exportchannel.py(1 hunks)contentcuration/contentcuration/tests/testdata.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_assessment_items.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_fields.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_html.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_imsmanifest.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_mathml.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_qti.py(1 hunks)contentcuration/contentcuration/tests/utils/test_exercise_creation.py(18 hunks)contentcuration/contentcuration/tests/utils/test_markdown.py(1 hunks)contentcuration/contentcuration/utils/assessment/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/markdown.py(1 hunks)contentcuration/contentcuration/utils/assessment/perseus.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/__init__.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/archive.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/assessment_item.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/constants.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/fields.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/__init__.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/breaks.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/content_types.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/display.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/embed.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/flow.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/media.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/sequence.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/table.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/text.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/imsmanifest.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/interaction_types/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/interaction_types/simple.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/interaction_types/text_based.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/__init__.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/core.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/fields.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/prompt.py(1 hunks)contentcuration/contentcuration/utils/publish.py(3 hunks)requirements-dev.in(1 hunks)requirements-dev.txt(2 hunks)requirements.in(1 hunks)requirements.txt(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- contentcuration/contentcuration/utils/assessment/qti/constants.py
🚧 Files skipped from review as they are similar to previous changes (27)
- contentcuration/contentcuration/utils/assessment/qti/init.py
- contentcuration/contentcuration/tests/testdata.py
- contentcuration/contentcuration/utils/assessment/qti/prompt.py
- requirements.in
- contentcuration/contentcuration/utils/assessment/qti/html/breaks.py
- contentcuration/contentcuration/tests/utils/qti/test_imsmanifest.py
- requirements-dev.txt
- contentcuration/contentcuration/utils/assessment/qti/interaction_types/base.py
- contentcuration/contentcuration/tests/utils/qti/test_qti.py
- contentcuration/contentcuration/utils/assessment/qti/interaction_types/text_based.py
- contentcuration/contentcuration/tests/utils/test_markdown.py
- contentcuration/contentcuration/utils/assessment/qti/mathml/base.py
- contentcuration/contentcuration/utils/assessment/qti/html/content_types.py
- contentcuration/contentcuration/utils/assessment/qti/html/embed.py
- contentcuration/contentcuration/utils/assessment/qti/html/base.py
- contentcuration/contentcuration/tests/test_exportchannel.py
- contentcuration/contentcuration/utils/assessment/qti/mathml/init.py
- contentcuration/contentcuration/utils/assessment/qti/html/sequence.py
- requirements-dev.in
- contentcuration/contentcuration/utils/assessment/qti/mathml/fields.py
- requirements.txt
- contentcuration/contentcuration/utils/assessment/qti/html/media.py
- contentcuration/contentcuration/utils/assessment/qti/fields.py
- contentcuration/contentcuration/utils/assessment/qti/assessment_item.py
- contentcuration/contentcuration/utils/assessment/qti/imsmanifest.py
- contentcuration/contentcuration/utils/assessment/qti/html/table.py
- contentcuration/contentcuration/utils/assessment/qti/html/init.py
🧰 Additional context used
🧠 Learnings (2)
contentcuration/contentcuration/utils/assessment/qti/archive.py (1)
Learnt from: rtibbles
PR: learningequality/studio#5103
File: contentcuration/contentcuration/utils/assessment/qti/archive.py:233-236
Timestamp: 2025-06-13T18:21:18.001Z
Learning: `assessment_id` values in the Studio codebase are always 32-character hexadecimal strings, so stripping the `"item_"` prefix with `replace("item_", "")` does not risk altering other parts of the identifier.
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py (1)
Learnt from: rtibbles
PR: learningequality/studio#5103
File: contentcuration/contentcuration/utils/assessment/qti/mathml/core.py:48-51
Timestamp: 2025-06-13T18:24:09.818Z
Learning: In the latest MathML Core specification, the literal string "infinity" is no longer an allowed value for the `maxsize` and `minsize` attributes on the `<mo>` element; only length-percentage values are valid.
🧬 Code Graph Analysis (8)
contentcuration/contentcuration/tests/utils/qti/test_mathml.py (5)
contentcuration/contentcuration/utils/assessment/qti/base.py (5)
TextNode(39-42)element_name(67-68)element_name(155-158)to_xml_string(138-141)from_string(281-289)contentcuration/contentcuration/utils/assessment/qti/constants.py (1)
Dir(47-50)contentcuration/contentcuration/utils/assessment/qti/mathml/core.py (26)
Annotation(166-169)AnnotationXml(172-178)Math(28-31)Mfrac(80-82)Mi(34-37)Mn(40-41)Mo(44-56)Mrow(76-77)Mspace(67-73)Msubsup(122-123)Mtable(162-163)Mtd(152-155)Mtr(158-159)Semantics(181-206)Mmultiscripts(146-149)Mover(131-133)Mphantom(110-111)Mprescripts(142-143)Mroot(89-90)Ms(63-64)Msub(114-115)Msup(118-119)Mtext(59-60)Munder(126-128)Munderover(136-139)element_name(177-178)contentcuration/contentcuration/utils/assessment/qti/mathml/fields.py (2)
MathMLDisplay(58-60)MathMLForm(63-66)contentcuration/contentcuration/utils/assessment/qti/mathml/base.py (5)
MathMLElement(12-24)MathMLGroupingElement(63-69)MathMLLayoutElement(45-51)MathMLScriptElement(54-60)MathMLTokenElement(36-42)
contentcuration/contentcuration/utils/publish.py (3)
contentcuration/contentcuration/utils/assessment/perseus.py (1)
PerseusExerciseGenerator(19-131)contentcuration/contentcuration/utils/assessment/qti/archive.py (1)
QTIExerciseGenerator(50-276)contentcuration/contentcuration/tests/testdata.py (1)
node(123-206)
contentcuration/contentcuration/tests/utils/qti/test_fields.py (1)
contentcuration/contentcuration/utils/assessment/qti/fields.py (4)
validate_data_uri(32-42)validate_local_href_path(45-60)validate_local_src_path(63-74)validate_local_srcset(85-99)
contentcuration/contentcuration/utils/assessment/base.py (2)
contentcuration/contentcuration/utils/assessment/perseus.py (6)
get_image_file_path(118-119)get_image_ref_prefix(121-122)create_assessment_item(103-116)_process_content(84-86)process_assessment_item(88-91)handle_before_assessment_items(124-131)contentcuration/contentcuration/models.py (1)
generate_object_storage_name(637-645)
contentcuration/contentcuration/utils/assessment/qti/html/display.py (2)
contentcuration/contentcuration/utils/assessment/qti/html/base.py (2)
FlowContentElement(22-23)HTMLElement(11-19)contentcuration/contentcuration/utils/assessment/qti/html/text.py (2)
BlockHTMLText(20-21)InlineHTMLText(16-17)
contentcuration/contentcuration/utils/assessment/qti/html/flow.py (1)
contentcuration/contentcuration/utils/assessment/qti/html/base.py (1)
BlockContentElement(30-31)
contentcuration/contentcuration/utils/assessment/qti/html/text.py (2)
contentcuration/contentcuration/utils/assessment/qti/html/base.py (2)
BlockContentElement(30-31)InlineContentElement(26-27)contentcuration/contentcuration/utils/assessment/qti/base.py (2)
element_name(67-68)element_name(155-158)
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py (3)
contentcuration/contentcuration/utils/assessment/qti/mathml/base.py (6)
MathMLElement(12-24)MathMLGroupingElement(63-69)MathMLLayoutElement(45-51)MathMLPresentationElement(27-33)MathMLScriptElement(54-60)MathMLTokenElement(36-42)contentcuration/contentcuration/utils/assessment/qti/mathml/fields.py (2)
MathMLDisplay(58-60)MathMLForm(63-66)contentcuration/contentcuration/utils/assessment/qti/base.py (2)
element_name(67-68)element_name(155-158)
🪛 Pylint (3.3.7)
contentcuration/contentcuration/utils/assessment/markdown.py
[refactor] 60-60: Simplify chained comparison between the operands
(R1716)
contentcuration/contentcuration/tests/utils/test_exercise_creation.py
[refactor] 192-192: Too many local variables (25/15)
(R0914)
[refactor] 1171-1171: Too many arguments (6/5)
(R0913)
[refactor] 1171-1171: Too many positional arguments (6/5)
(R0917)
[refactor] 1211-1211: Consider using 'with' for resource-allocating operations
(R1732)
contentcuration/contentcuration/tests/utils/qti/test_html.py
[refactor] 322-322: Too many local variables (31/15)
(R0914)
[refactor] 322-322: Too many statements (61/50)
(R0915)
[refactor] 415-415: Too many local variables (22/15)
(R0914)
[refactor] 415-415: Too many statements (56/50)
(R0915)
contentcuration/contentcuration/utils/assessment/base.py
[refactor] 41-41: Too many instance attributes (9/7)
(R0902)
[refactor] 91-91: Too many arguments (6/5)
(R0913)
[refactor] 91-91: Too many positional arguments (6/5)
(R0917)
[refactor] 146-146: Either all return statements in a function should return an expression, or none of them should.
(R1710)
[refactor] 160-160: Too many arguments (7/5)
(R0913)
[refactor] 160-160: Too many positional arguments (7/5)
(R0917)
[refactor] 160-160: Either all return statements in a function should return an expression, or none of them should.
(R1710)
[refactor] 181-181: Too many arguments (7/5)
(R0913)
[refactor] 181-181: Too many positional arguments (7/5)
(R0917)
[refactor] 371-371: Consider using 'with' for resource-allocating operations
(R1732)
contentcuration/contentcuration/utils/assessment/qti/base.py
[refactor] 39-39: Too few public methods (0/2)
(R0903)
[refactor] 90-119: Unnecessary "elif" after "raise", remove the leading "el" from "elif"
(R1720)
[refactor] 79-132: Too many nested blocks (6/5)
(R1702)
[refactor] 70-70: Too many branches (16/12)
(R0912)
contentcuration/contentcuration/utils/assessment/qti/html/display.py
[refactor] 17-17: Too many ancestors (8/7)
(R0901)
[error] 34-34: Method 'validate_summary_position' should have "self" as first argument
(E0213)
[error] 58-58: Method 'validate_figcaption_position' should have "self" as first argument
(E0213)
contentcuration/contentcuration/utils/assessment/qti/html/flow.py
[refactor] 20-20: Too many ancestors (8/7)
(R0901)
[refactor] 24-24: Too many ancestors (8/7)
(R0901)
[refactor] 28-28: Too many ancestors (8/7)
(R0901)
[refactor] 32-32: Too many ancestors (8/7)
(R0901)
[refactor] 36-36: Too many ancestors (8/7)
(R0901)
[refactor] 40-40: Too many ancestors (8/7)
(R0901)
[refactor] 44-44: Too many ancestors (8/7)
(R0901)
[refactor] 48-48: Too many ancestors (8/7)
(R0901)
[refactor] 52-52: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/utils/assessment/qti/html/text.py
[refactor] 24-24: Too many ancestors (8/7)
(R0901)
[refactor] 29-29: Too many ancestors (8/7)
(R0901)
[refactor] 33-33: Too many ancestors (8/7)
(R0901)
[refactor] 37-37: Too many ancestors (8/7)
(R0901)
[refactor] 41-41: Too many ancestors (8/7)
(R0901)
[refactor] 45-45: Too many ancestors (8/7)
(R0901)
[refactor] 49-49: Too many ancestors (8/7)
(R0901)
[refactor] 53-53: Too many ancestors (8/7)
(R0901)
[refactor] 57-57: Too many ancestors (8/7)
(R0901)
[refactor] 61-61: Too many ancestors (8/7)
(R0901)
[refactor] 65-65: Too many ancestors (8/7)
(R0901)
[refactor] 69-69: Too many ancestors (8/7)
(R0901)
[refactor] 73-73: Too many ancestors (8/7)
(R0901)
[refactor] 77-77: Too many ancestors (8/7)
(R0901)
[refactor] 81-81: Too many ancestors (8/7)
(R0901)
[refactor] 85-85: Too many ancestors (8/7)
(R0901)
[refactor] 89-89: Too many ancestors (8/7)
(R0901)
[refactor] 93-93: Too many ancestors (8/7)
(R0901)
[refactor] 97-97: Too many ancestors (8/7)
(R0901)
[refactor] 101-101: Too many ancestors (8/7)
(R0901)
[refactor] 105-105: Too many ancestors (8/7)
(R0901)
[refactor] 109-109: Too many ancestors (8/7)
(R0901)
[refactor] 113-113: Too many ancestors (8/7)
(R0901)
[refactor] 117-117: Too many ancestors (8/7)
(R0901)
[refactor] 121-121: Too many ancestors (8/7)
(R0901)
[refactor] 130-130: Too many ancestors (8/7)
(R0901)
[refactor] 134-134: Too many ancestors (8/7)
(R0901)
[refactor] 138-138: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/utils/assessment/qti/interaction_types/simple.py
[error] 45-45: Method '_unique_answer_identifiers' should have "self" as first argument
(E0213)
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py
[refactor] 114-114: Too many ancestors (8/7)
(R0901)
[refactor] 118-118: Too many ancestors (8/7)
(R0901)
[refactor] 122-122: Too many ancestors (8/7)
(R0901)
[refactor] 126-126: Too many ancestors (8/7)
(R0901)
[refactor] 131-131: Too many ancestors (8/7)
(R0901)
[refactor] 136-136: Too many ancestors (8/7)
(R0901)
[refactor] 146-146: Too many ancestors (8/7)
(R0901)
🪛 Ruff (0.11.9)
contentcuration/contentcuration/tests/utils/qti/test_mathml.py
123-124: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
174-175: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
210-211: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
1421-1421: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
contentcuration/contentcuration/tests/utils/qti/test_fields.py
258-259: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
270-271: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
282-283: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
293-294: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
contentcuration/contentcuration/tests/utils/test_exercise_creation.py
280-280: Loop control variable image_name not used within loop body
Rename unused image_name to _image_name
(B007)
contentcuration/contentcuration/utils/assessment/base.py
348-349: ExerciseArchiveGenerator.handle_before_assessment_items is an empty method in an abstract base class, but has no abstract decorator
(B027)
351-352: ExerciseArchiveGenerator.handle_after_assessment_items is an empty method in an abstract base class, but has no abstract decorator
(B027)
371-371: Use a context manager for opening files
(SIM115)
🔇 Additional comments (3)
contentcuration/contentcuration/utils/assessment/perseus.py (1)
72-78: Delimiter validation & regex refactor look greatThe added delimiter check prevents crashes on malformed Graphie files, and the non-greedy
re.subcleans formulas in one pass—nice improvements.contentcuration/contentcuration/utils/publish.py (1)
334-343: 👍 Stale archive clean-up logic looks solidNice touch adding the
stale_presetsset and deleting outdated archives before regenerating. This prevents multiple ZIPs from lingering on a node and matches the earlier refactor recommendation.contentcuration/contentcuration/tests/utils/qti/test_html.py (1)
88-98: Attribute-order fragile string comparisonThe assertions compare the raw XML string. Attribute order is not guaranteed by
xml.etree.ElementTree; relying on ordering may lead to sporadic failures if
the underlying implementation changes. Prefer parsing back with
ET.fromstring()and comparing tag/attribs instead.
contentcuration/contentcuration/tests/utils/qti/test_assessment_items.py
Show resolved
Hide resolved
contentcuration/contentcuration/utils/assessment/qti/archive.py
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/utils/assessment/qti/html/display.py
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py
Outdated
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
contentcuration/contentcuration/utils/assessment/qti/html/flow.py (1)
20-22:citetype remains over-restrictiveUsing
HttpUrlstill blocks relative or fragment-only references that are valid inside a packaged QTI item (e.g.#ref,images/foo.svg). Consider switching to the existingLocalHrefPath/AnyUrlhelper instead.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 20-20: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/tests/utils/qti/test_assessment_items.py (1)
222-226: Same mismatch in multiple-choice testRepeat of the
"EN-US"vs"en-US"inconsistency – adjust as above.contentcuration/contentcuration/utils/assessment/qti/archive.py (1)
234-236: Prefix stripping still fragile
identifier.replace("item_", "")removes every occurrence, not just the prefix. Usestartswith+ slice for safety even if current data is “clean”.contentcuration/contentcuration/utils/assessment/qti/base.py (1)
85-107: Deep nesting previously flagged – no changes madeThe
listbranch insideto_element()still contains the “elseaftercontinue” pattern and six nested levels, hurting readability and future maintenance.
See earlier review on the same method for the suggested de-indent and helper extraction.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 85-114: Unnecessary "elif" after "raise", remove the leading "el" from "elif"
(R1720)
🧹 Nitpick comments (10)
contentcuration/contentcuration/tests/utils/qti/test_fields.py (1)
257-260: Minor: collapse nestedwithblocks- with self.assertRaises(ValueError): - validate_local_srcset(srcset) + with self.assertRaises(ValueError), self.subTest(srcset=srcset): + validate_local_srcset(srcset)Combining contexts reduces indentation noise; repeat for similar spots flagged by Ruff SIM117.
🧰 Tools
🪛 Ruff (0.11.9)
258-259: Use a single
withstatement with multiple contexts instead of nestedwithstatementsCombine
withstatements(SIM117)
contentcuration/contentcuration/tests/utils/qti/test_mathml.py (2)
123-124: Combine nested with statements for cleaner code.Consider combining the nested
withstatements as suggested by static analysis:- with self.subTest(value=value): - with self.assertRaises(ValidationError): + with self.subTest(value=value), self.assertRaises(ValidationError): Mi(mathsize=value, children=["x"])Apply similar changes to lines 174-175 and 210-211.
Also applies to: 174-175, 210-211
🧰 Tools
🪛 Ruff (0.11.9)
123-124: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
1421-1421: Use underscore for unused loop variable.- for i in range(10): + for _ in range(10): content = Mrow(children=[content])🧰 Tools
🪛 Ruff (0.11.9)
1421-1421: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
contentcuration/contentcuration/utils/assessment/markdown.py (1)
60-60: Simplify chained comparison for better readability.- if endpos >= state.bMarks[line] and endpos <= state.eMarks[line]: + if state.bMarks[line] <= endpos <= state.eMarks[line]:🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 60-60: Simplify chained comparison between the operands
(R1716)
contentcuration/contentcuration/utils/assessment/qti/assessment_item.py (1)
124-146: Duplicate cardinality validation logic
OutcomeDeclarationandContextDeclarationrepeat identical post-validators.
Factor this into a shared helper to keep rules in one place and avoid divergence.🧰 Tools
🪛 Ruff (0.11.9)
136-141: Use a single
ifstatement instead of nestedifstatements(SIM102)
contentcuration/contentcuration/tests/utils/test_exercise_creation.py (2)
280-284: Rename unused loop variable
for image_name, image_data in question_images.items():–image_nameis never used.
Rename to_to silence linters and signal intent.🧰 Tools
🪛 Ruff (0.11.9)
280-280: Loop control variable
image_namenot used within loop bodyRename unused
image_nameto_image_name(B007)
1260-1289: Exact-string XML assertions are brittle
_normalize_xmlremoves whitespace but still relies on attribute ordering that ElementTree does not guarantee.
Consider parsing both strings withxml.etree.ElementTreeand comparing canonicalised trees instead, making tests robust to harmless re-ordering.contentcuration/contentcuration/utils/assessment/qti/mathml/core.py (3)
49-50: Out-of-date spec note – drop the “infinity” referenceThe literal string
"infinity"was removed from MathML Core (§4.3.3).
Leaving the comment implies it is still valid and may mislead future contributors.- maxsize: Optional[LengthPercentage] = None # Can be "infinity" + # MathML Core no longer permits the literal string “infinity”. + maxsize: Optional[LengthPercentage] = None
166-170: Duplicate text representation inAnnotation
<annotation>already stores its raw content as children; a separatetext
field is redundant and risks divergence. A single source of truth keeps the
model simpler and serialisation unambiguous.-class Annotation(MathMLElement): - encoding: Optional[str] = None - text: Optional[str] = None - children: List[TextType] = Field(default_factory=list) +class Annotation(MathMLElement): + encoding: Optional[str] = None + children: List[TextType] = Field(default_factory=list) + + @field_validator("children") + @classmethod + def _non_empty(cls, v): + if len(v) == 0: + raise ValueError("<annotation> must contain text or markup") + return v
186-198: Minor: redundant length check inSemanticsvalidator
childrenis already typed withLen(2), guaranteeing ≥2 items.
The explicitlen(v) == 0guard can be removed or tightened tolen(v) < 2
to avoid dead/duplicate code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
contentcuration/contentcuration/tests/utils/qti/test_assessment_items.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_fields.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_html.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_imsmanifest.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_mathml.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_qti.py(1 hunks)contentcuration/contentcuration/tests/utils/test_exercise_creation.py(18 hunks)contentcuration/contentcuration/tests/utils/test_markdown.py(1 hunks)contentcuration/contentcuration/utils/assessment/markdown.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/__init__.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/archive.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/assessment_item.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/constants.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/fields.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/__init__.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/breaks.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/content_types.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/display.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/embed.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/flow.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/media.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/sequence.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/table.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/text.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/imsmanifest.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/interaction_types/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/interaction_types/simple.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/interaction_types/text_based.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/__init__.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/core.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/fields.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/prompt.py(1 hunks)contentcuration/contentcuration/utils/publish.py(3 hunks)requirements-dev.in(1 hunks)requirements-dev.txt(2 hunks)requirements.in(1 hunks)requirements.txt(4 hunks)
✅ Files skipped from review due to trivial changes (6)
- contentcuration/contentcuration/utils/assessment/qti/constants.py
- contentcuration/contentcuration/utils/assessment/qti/mathml/base.py
- contentcuration/contentcuration/tests/utils/qti/test_imsmanifest.py
- contentcuration/contentcuration/tests/utils/test_markdown.py
- contentcuration/contentcuration/utils/assessment/qti/html/base.py
- contentcuration/contentcuration/utils/assessment/qti/html/table.py
🚧 Files skipped from review as they are similar to previous changes (19)
- requirements-dev.in
- contentcuration/contentcuration/utils/assessment/qti/init.py
- requirements.in
- contentcuration/contentcuration/utils/assessment/qti/prompt.py
- contentcuration/contentcuration/utils/assessment/qti/interaction_types/base.py
- requirements-dev.txt
- contentcuration/contentcuration/utils/assessment/qti/html/breaks.py
- requirements.txt
- contentcuration/contentcuration/utils/assessment/qti/interaction_types/text_based.py
- contentcuration/contentcuration/utils/publish.py
- contentcuration/contentcuration/tests/utils/qti/test_qti.py
- contentcuration/contentcuration/utils/assessment/qti/mathml/init.py
- contentcuration/contentcuration/utils/assessment/qti/html/content_types.py
- contentcuration/contentcuration/utils/assessment/qti/html/media.py
- contentcuration/contentcuration/utils/assessment/qti/html/embed.py
- contentcuration/contentcuration/utils/assessment/qti/html/sequence.py
- contentcuration/contentcuration/utils/assessment/qti/mathml/fields.py
- contentcuration/contentcuration/utils/assessment/qti/imsmanifest.py
- contentcuration/contentcuration/utils/assessment/qti/html/init.py
🧰 Additional context used
🧠 Learnings (3)
contentcuration/contentcuration/utils/assessment/qti/archive.py (1)
Learnt from: rtibbles
PR: learningequality/studio#5103
File: contentcuration/contentcuration/utils/assessment/qti/archive.py:233-236
Timestamp: 2025-06-13T18:21:18.001Z
Learning: `assessment_id` values in the Studio codebase are always 32-character hexadecimal strings, so stripping the `"item_"` prefix with `replace("item_", "")` does not risk altering other parts of the identifier.
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py (2)
Learnt from: rtibbles
PR: learningequality/studio#5103
File: contentcuration/contentcuration/utils/assessment/qti/mathml/core.py:48-51
Timestamp: 2025-06-13T18:24:09.818Z
Learning: In the latest MathML Core specification, the literal string "infinity" is no longer an allowed value for the `maxsize` and `minsize` attributes on the `<mo>` element; only length-percentage values are valid.
Learnt from: rtibbles
PR: learningequality/studio#5103
File: contentcuration/contentcuration/utils/assessment/qti/mathml/core.py:210-212
Timestamp: 2025-06-15T23:46:31.075Z
Learning: The `maction` element is deprecated in MathML Core and should be removed rather than maintained or fixed in MathML-related codebases.
contentcuration/contentcuration/utils/assessment/qti/base.py (1)
Learnt from: rtibbles
PR: learningequality/studio#5103
File: contentcuration/contentcuration/utils/assessment/qti/base.py:59-65
Timestamp: 2025-06-15T23:49:02.211Z
Learning: In the QTI assessment codebase, prefer immutable collections (tuples) over mutable ones (lists) for child elements to ensure cache invalidation works correctly and maintain API predictability.
🧬 Code Graph Analysis (9)
contentcuration/contentcuration/utils/assessment/qti/archive.py (11)
contentcuration/contentcuration/utils/assessment/base.py (2)
ExerciseArchiveGenerator(41-396)add_file_to_write(125-136)contentcuration/contentcuration/utils/assessment/markdown.py (1)
render_markdown(119-120)contentcuration/contentcuration/utils/assessment/qti/assessment_item.py (6)
CorrectResponse(42-45)ItemBody(103-108)OutcomeDeclaration(59-100)ResponseDeclaration(194-225)ResponseProcessing(228-236)Value(23-39)contentcuration/contentcuration/utils/assessment/qti/base.py (2)
ElementTreeBase(222-284)from_string(276-284)contentcuration/contentcuration/utils/assessment/qti/constants.py (4)
BaseType(14-25)Cardinality(7-11)Orientation(59-61)ShowHide(42-44)contentcuration/contentcuration/utils/assessment/qti/html/flow.py (1)
Div(24-25)contentcuration/contentcuration/utils/assessment/qti/html/text.py (1)
P(29-30)contentcuration/contentcuration/utils/assessment/qti/imsmanifest.py (4)
Manifest(91-102)Metadata(36-40)Resource(75-82)Resources(85-88)contentcuration/contentcuration/utils/assessment/qti/interaction_types/simple.py (2)
ChoiceInteraction(34-62)SimpleChoice(20-31)contentcuration/contentcuration/utils/assessment/qti/interaction_types/text_based.py (1)
TextEntryInteraction(14-25)contentcuration/contentcuration/utils/assessment/qti/prompt.py (1)
Prompt(11-13)
contentcuration/contentcuration/utils/assessment/qti/assessment_item.py (4)
contentcuration/contentcuration/utils/assessment/qti/base.py (2)
BaseSequence(172-179)QTIBase(144-153)contentcuration/contentcuration/utils/assessment/qti/constants.py (4)
BaseType(14-25)Cardinality(7-11)ExternalScored(37-39)View(28-34)contentcuration/contentcuration/utils/assessment/qti/html/base.py (1)
BlockContentElement(30-31)contentcuration/contentcuration/utils/assessment/qti/interaction_types/base.py (1)
BlockInteraction(14-15)
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py (3)
contentcuration/contentcuration/utils/assessment/qti/mathml/base.py (6)
MathMLElement(12-24)MathMLGroupingElement(63-69)MathMLLayoutElement(45-51)MathMLPresentationElement(27-33)MathMLScriptElement(54-60)MathMLTokenElement(36-42)contentcuration/contentcuration/utils/assessment/qti/mathml/fields.py (2)
MathMLDisplay(58-60)MathMLForm(63-66)contentcuration/contentcuration/utils/assessment/qti/base.py (2)
element_name(62-63)element_name(150-153)
contentcuration/contentcuration/tests/utils/qti/test_fields.py (1)
contentcuration/contentcuration/utils/assessment/qti/fields.py (4)
validate_data_uri(32-42)validate_local_href_path(45-60)validate_local_src_path(63-74)validate_local_srcset(85-99)
contentcuration/contentcuration/tests/utils/qti/test_mathml.py (5)
contentcuration/contentcuration/utils/assessment/qti/base.py (5)
TextNode(39-42)element_name(62-63)element_name(150-153)to_xml_string(133-136)from_string(276-284)contentcuration/contentcuration/utils/assessment/qti/constants.py (1)
Dir(47-50)contentcuration/contentcuration/utils/assessment/qti/mathml/core.py (29)
Annotation(166-169)AnnotationXml(172-178)Math(28-31)Mfrac(80-82)Mi(34-37)Mn(40-41)Mo(44-56)Mrow(76-77)Mspace(67-73)Msubsup(122-123)Mtable(162-163)Mtd(152-155)Mtr(158-159)Semantics(181-206)Merror(97-98)Mmultiscripts(146-149)Mover(131-133)Mphantom(110-111)Mprescripts(142-143)Mroot(89-90)Ms(63-64)Msqrt(85-86)Mstyle(93-94)Msub(114-115)Msup(118-119)Mtext(59-60)Munder(126-128)Munderover(136-139)element_name(177-178)contentcuration/contentcuration/utils/assessment/qti/mathml/fields.py (2)
MathMLDisplay(58-60)MathMLForm(63-66)contentcuration/contentcuration/utils/assessment/qti/mathml/base.py (5)
MathMLElement(12-24)MathMLGroupingElement(63-69)MathMLLayoutElement(45-51)MathMLScriptElement(54-60)MathMLTokenElement(36-42)
contentcuration/contentcuration/utils/assessment/qti/base.py (1)
contentcuration/contentcuration/utils/assessment/qti/constants.py (1)
Dir(47-50)
contentcuration/contentcuration/utils/assessment/qti/html/display.py (2)
contentcuration/contentcuration/utils/assessment/qti/html/base.py (2)
FlowContentElement(22-23)HTMLElement(11-19)contentcuration/contentcuration/utils/assessment/qti/html/text.py (2)
BlockHTMLText(20-21)InlineHTMLText(16-17)
contentcuration/contentcuration/utils/assessment/qti/html/flow.py (1)
contentcuration/contentcuration/utils/assessment/qti/html/base.py (1)
BlockContentElement(30-31)
contentcuration/contentcuration/utils/assessment/qti/html/text.py (2)
contentcuration/contentcuration/utils/assessment/qti/html/base.py (2)
BlockContentElement(30-31)InlineContentElement(26-27)contentcuration/contentcuration/utils/assessment/qti/base.py (2)
element_name(62-63)element_name(150-153)
🪛 Pylint (3.3.7)
contentcuration/contentcuration/tests/utils/qti/test_html.py
[refactor] 322-322: Too many local variables (31/15)
(R0914)
[refactor] 322-322: Too many statements (61/50)
(R0915)
[refactor] 415-415: Too many local variables (22/15)
(R0914)
[refactor] 415-415: Too many statements (56/50)
(R0915)
contentcuration/contentcuration/tests/utils/test_exercise_creation.py
[refactor] 192-192: Too many local variables (25/15)
(R0914)
[refactor] 1171-1171: Too many arguments (6/5)
(R0913)
[refactor] 1171-1171: Too many positional arguments (6/5)
(R0917)
[refactor] 1211-1211: Consider using 'with' for resource-allocating operations
(R1732)
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py
[refactor] 114-114: Too many ancestors (8/7)
(R0901)
[refactor] 118-118: Too many ancestors (8/7)
(R0901)
[refactor] 122-122: Too many ancestors (8/7)
(R0901)
[refactor] 126-126: Too many ancestors (8/7)
(R0901)
[refactor] 131-131: Too many ancestors (8/7)
(R0901)
[refactor] 136-136: Too many ancestors (8/7)
(R0901)
[refactor] 146-146: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/utils/assessment/markdown.py
[refactor] 60-60: Simplify chained comparison between the operands
(R1716)
contentcuration/contentcuration/utils/assessment/qti/base.py
[refactor] 39-39: Too few public methods (0/2)
(R0903)
[refactor] 85-114: Unnecessary "elif" after "raise", remove the leading "el" from "elif"
(R1720)
[refactor] 74-127: Too many nested blocks (6/5)
(R1702)
[refactor] 65-65: Too many branches (16/12)
(R0912)
contentcuration/contentcuration/utils/assessment/qti/html/display.py
[refactor] 17-17: Too many ancestors (8/7)
(R0901)
[error] 34-34: Method 'validate_summary_position' should have "self" as first argument
(E0213)
[error] 58-58: Method 'validate_figcaption_position' should have "self" as first argument
(E0213)
contentcuration/contentcuration/utils/assessment/qti/html/flow.py
[refactor] 20-20: Too many ancestors (8/7)
(R0901)
[refactor] 24-24: Too many ancestors (8/7)
(R0901)
[refactor] 28-28: Too many ancestors (8/7)
(R0901)
[refactor] 32-32: Too many ancestors (8/7)
(R0901)
[refactor] 36-36: Too many ancestors (8/7)
(R0901)
[refactor] 40-40: Too many ancestors (8/7)
(R0901)
[refactor] 44-44: Too many ancestors (8/7)
(R0901)
[refactor] 48-48: Too many ancestors (8/7)
(R0901)
[refactor] 52-52: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/utils/assessment/qti/html/text.py
[refactor] 24-24: Too many ancestors (8/7)
(R0901)
[refactor] 29-29: Too many ancestors (8/7)
(R0901)
[refactor] 33-33: Too many ancestors (8/7)
(R0901)
[refactor] 37-37: Too many ancestors (8/7)
(R0901)
[refactor] 41-41: Too many ancestors (8/7)
(R0901)
[refactor] 45-45: Too many ancestors (8/7)
(R0901)
[refactor] 49-49: Too many ancestors (8/7)
(R0901)
[refactor] 53-53: Too many ancestors (8/7)
(R0901)
[refactor] 57-57: Too many ancestors (8/7)
(R0901)
[refactor] 61-61: Too many ancestors (8/7)
(R0901)
[refactor] 65-65: Too many ancestors (8/7)
(R0901)
[refactor] 69-69: Too many ancestors (8/7)
(R0901)
[refactor] 73-73: Too many ancestors (8/7)
(R0901)
[refactor] 77-77: Too many ancestors (8/7)
(R0901)
[refactor] 81-81: Too many ancestors (8/7)
(R0901)
[refactor] 85-85: Too many ancestors (8/7)
(R0901)
[refactor] 89-89: Too many ancestors (8/7)
(R0901)
[refactor] 93-93: Too many ancestors (8/7)
(R0901)
[refactor] 97-97: Too many ancestors (8/7)
(R0901)
[refactor] 101-101: Too many ancestors (8/7)
(R0901)
[refactor] 105-105: Too many ancestors (8/7)
(R0901)
[refactor] 109-109: Too many ancestors (8/7)
(R0901)
[refactor] 113-113: Too many ancestors (8/7)
(R0901)
[refactor] 117-117: Too many ancestors (8/7)
(R0901)
[refactor] 121-121: Too many ancestors (8/7)
(R0901)
[refactor] 130-130: Too many ancestors (8/7)
(R0901)
[refactor] 134-134: Too many ancestors (8/7)
(R0901)
[refactor] 138-138: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/utils/assessment/qti/interaction_types/simple.py
[error] 45-45: Method '_unique_answer_identifiers' should have "self" as first argument
(E0213)
🪛 Ruff (0.11.9)
contentcuration/contentcuration/utils/assessment/qti/assessment_item.py
91-96: Use a single if statement instead of nested if statements
(SIM102)
136-141: Use a single if statement instead of nested if statements
(SIM102)
contentcuration/contentcuration/tests/utils/test_exercise_creation.py
280-280: Loop control variable image_name not used within loop body
Rename unused image_name to _image_name
(B007)
contentcuration/contentcuration/tests/utils/qti/test_fields.py
258-259: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
270-271: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
282-283: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
293-294: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
contentcuration/contentcuration/tests/utils/qti/test_mathml.py
123-124: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
174-175: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
210-211: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
1421-1421: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Python unit tests
🔇 Additional comments (9)
contentcuration/contentcuration/utils/assessment/qti/interaction_types/simple.py (1)
44-55: Validator LGTM – ignore linter noiseThe
@field_validatorcorrectly receivescls, answers; the Pylint E0213 complaint can be safely ignored.🧰 Tools
🪛 Pylint (3.3.7)
[error] 45-45: Method '_unique_answer_identifiers' should have "self" as first argument
(E0213)
contentcuration/contentcuration/utils/assessment/qti/html/display.py (3)
17-19: Class implementation looks good.The
for_attribute is correctly handled without an alias, as the serialization logic strips trailing underscores.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 17-17: Too many ancestors (8/7)
(R0901)
29-48: Details validator correctly enforces HTML spec requirements.The implementation properly validates that:
- At least one child exists (and must be Summary as first child)
- Summary is the first child
- Only one Summary element is allowed
The use of
clsin the validator is correct for Pydantic field validators.🧰 Tools
🪛 Pylint (3.3.7)
[error] 34-34: Method 'validate_summary_position' should have "self" as first argument
(E0213)
54-68: Figure validator properly enforces figcaption constraints.The validator correctly implements HTML spec requirements:
- At most one Figcaption element
- If present, must be first or last child
Good implementation with proper error messages.
🧰 Tools
🪛 Pylint (3.3.7)
[error] 58-58: Method 'validate_figcaption_position' should have "self" as first argument
(E0213)
contentcuration/contentcuration/utils/assessment/qti/fields.py (2)
10-23: BCP47 language validation is well implemented.Good use of the langcodes library for validation and normalization, with proper exception chaining for debugging context.
45-61: Excellent security-conscious path validation.The function properly restricts paths to relative URLs suitable for offline bundling, while allowing data URIs for embedded content. Good rejection of absolute URLs and paths.
contentcuration/contentcuration/utils/assessment/qti/html/text.py (1)
1-152: HTML text element classes are well-structured.The deep inheritance hierarchy is appropriate for modeling the HTML element taxonomy. All element classes follow consistent patterns with proper base class selection.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 24-24: Too many ancestors (8/7)
(R0901)
[refactor] 29-29: Too many ancestors (8/7)
(R0901)
[refactor] 33-33: Too many ancestors (8/7)
(R0901)
[refactor] 37-37: Too many ancestors (8/7)
(R0901)
[refactor] 41-41: Too many ancestors (8/7)
(R0901)
[refactor] 45-45: Too many ancestors (8/7)
(R0901)
[refactor] 49-49: Too many ancestors (8/7)
(R0901)
[refactor] 53-53: Too many ancestors (8/7)
(R0901)
[refactor] 57-57: Too many ancestors (8/7)
(R0901)
[refactor] 61-61: Too many ancestors (8/7)
(R0901)
[refactor] 65-65: Too many ancestors (8/7)
(R0901)
[refactor] 69-69: Too many ancestors (8/7)
(R0901)
[refactor] 73-73: Too many ancestors (8/7)
(R0901)
[refactor] 77-77: Too many ancestors (8/7)
(R0901)
[refactor] 81-81: Too many ancestors (8/7)
(R0901)
[refactor] 85-85: Too many ancestors (8/7)
(R0901)
[refactor] 89-89: Too many ancestors (8/7)
(R0901)
[refactor] 93-93: Too many ancestors (8/7)
(R0901)
[refactor] 97-97: Too many ancestors (8/7)
(R0901)
[refactor] 101-101: Too many ancestors (8/7)
(R0901)
[refactor] 105-105: Too many ancestors (8/7)
(R0901)
[refactor] 109-109: Too many ancestors (8/7)
(R0901)
[refactor] 113-113: Too many ancestors (8/7)
(R0901)
[refactor] 117-117: Too many ancestors (8/7)
(R0901)
[refactor] 121-121: Too many ancestors (8/7)
(R0901)
[refactor] 130-130: Too many ancestors (8/7)
(R0901)
[refactor] 134-134: Too many ancestors (8/7)
(R0901)
[refactor] 138-138: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/utils/assessment/markdown.py (1)
1-121: Excellent implementation of TeX math to MathML conversion.The module provides a clean, well-structured solution for parsing and rendering math expressions:
- Proper regex patterns handling multi-line content
- Clean separation of concerns between parsing and rendering
- Good namespace handling for cleaner MathML output
- Proper plugin architecture integration
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 60-60: Simplify chained comparison between the operands
(R1716)
contentcuration/contentcuration/tests/utils/qti/test_html.py (1)
80-412: Solid, exhaustive coverage of HTML element serialisation/parsing. No concerns.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 322-322: Too many local variables (31/15)
(R0914)
[refactor] 322-322: Too many statements (61/50)
(R0915)
contentcuration/contentcuration/tests/utils/qti/test_assessment_items.py
Show resolved
Hide resolved
contentcuration/contentcuration/utils/assessment/qti/archive.py
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/utils/assessment/qti/assessment_item.py
Outdated
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
contentcuration/contentcuration/utils/assessment/qti/html/flow.py (1)
20-22:citetype still overly restrictive – useAnyUrl/LocalHrefPathinstead ofHttpUrlPrevious feedback pointed out that
HttpUrldisallows fragment-only or package-internal refs (e.g.#footnote,images/pic.svg) that are perfectly valid inside a QTI package.
Replacing it withAnyUrlor the existingLocalHrefPathcustom type will avoid needless validation failures.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 20-20: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/tests/utils/qti/test_assessment_items.py (1)
103-108:⚠️ Potential issueTest will fail – language code case mismatch persists
The expected XML hard-codes
language="en-US"but theAssessmentIteminstances are still initialised with"EN-US". The string comparison will therefore fail for the True/False and Multiple-Choice tests.- language="EN-US", + language="en-US",Please apply the same change at both call-sites (lines 106 and 224) or normalise case during serialisation.
Also applies to: 221-225
contentcuration/contentcuration/utils/assessment/markdown.py (1)
57-64:⚠️ Potential issueGuard against unmatched end-delimiter → infinite loop
If
endposis never found inside any scanned line (e.g. a malformed block missing the closing$$), thewhileloop completes without updatingstate.line, so the parser re-enters the same rule on the next dispatch and hangs.- while line < endLine: - if endpos >= state.bMarks[line] and endpos <= state.eMarks[line]: + while line < endLine: + if state.bMarks[line] <= endpos <= state.eMarks[line]: state.line = line + 1 break line += 1 + + # Fallback – always advance at least one line to guarantee progress + if state.line == begLine: + state.line = min(endLine, line + 1)🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 60-60: Simplify chained comparison between the operands
(R1716)
🧹 Nitpick comments (6)
contentcuration/contentcuration/tests/utils/qti/test_fields.py (1)
258-259: Collapse nestedwithstatements (Ruff SIM117)You can silence the linter and shave a few lines by combining the nested contexts:
- with self.subTest(srcset=srcset): - with self.assertRaises(ValueError): + with self.subTest(srcset=srcset), self.assertRaises(ValueError): validate_local_srcset(srcset)Apply to the four highlighted blocks.
Also applies to: 270-271, 282-283, 293-294
🧰 Tools
🪛 Ruff (0.11.9)
258-259: Use a single
withstatement with multiple contexts instead of nestedwithstatementsCombine
withstatements(SIM117)
contentcuration/contentcuration/utils/assessment/qti/html/text.py (1)
146-152: Redundantelement_nameoverride
Ruby.element_name()returns"ruby", which is already exactly what the default implementation inElementTreeBaseproduces (cls.__name__.lower()). The override adds an extra method lookup without changing behaviour; consider removing it for clarity.contentcuration/contentcuration/tests/utils/qti/test_mathml.py (1)
1480-1484: Unused loop variable – rename to “_” for clarity
for i in range(10):never usesi. Renaming avoids the Ruff B007 warning and signals intent.- for i in range(10): - content = Mrow(children=[content]) + for _ in range(10): + content = Mrow(children=[content])🧰 Tools
🪛 Ruff (0.11.9)
1482-1482: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
contentcuration/contentcuration/tests/utils/qti/test_html.py (1)
80-104: Consider parametrising repetitive element-serialization tests
HTMLDataClassTests.test_display_elements,test_text_elements, etc. manually instantiate dozens of element classes and assertto_xml_string()output. While readable, the method now exceeds 50 statements and 30 locals (Pylint R0914/R0915 triggers).A parametric approach:
@pytest.mark.parametrize( "cls, kwargs, expected", [ (Label, dict(children=["Test"], for_="id"), '<label for="id">Test</label>'), (Hr, {}, "<hr />"), ... ], ) def test_serialisation(cls, kwargs, expected): assert cls(**kwargs).to_xml_string() == expectedreduces duplication and makes failures easier to spot.
contentcuration/contentcuration/tests/utils/test_exercise_creation.py (1)
280-285: Rename unused loop variable
image_nameis never referenced inside the loop; use_(or remove the unpack) to silence Ruff B007 and make intent clear.-for image_name, image_data in question_images.items(): +for _, image_data in question_images.items():🧰 Tools
🪛 Ruff (0.11.9)
280-280: Loop control variable
image_namenot used within loop bodyRename unused
image_nameto_image_name(B007)
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py (1)
159-170: Comment contradicts implemented logic for<mprescripts>position.Line 159 says “if present it must be last”, yet the subsequent code expects children after
<mprescripts>(pre_scripts_count).
Either update the comment or add a guard (if prescripts and prescripts[0] != len(v) - 1:) to enforce the stated rule. Prefer aligning with MathML Core §3.5, which allows prescripts, so the comment should be fixed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
contentcuration/contentcuration/tests/utils/qti/test_assessment_items.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_fields.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_html.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_imsmanifest.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_mathml.py(1 hunks)contentcuration/contentcuration/tests/utils/qti/test_qti.py(1 hunks)contentcuration/contentcuration/tests/utils/test_exercise_creation.py(18 hunks)contentcuration/contentcuration/tests/utils/test_markdown.py(1 hunks)contentcuration/contentcuration/utils/assessment/markdown.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/__init__.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/archive.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/assessment_item.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/constants.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/fields.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/__init__.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/breaks.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/content_types.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/display.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/embed.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/flow.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/media.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/sequence.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/table.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/html/text.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/imsmanifest.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/interaction_types/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/interaction_types/simple.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/interaction_types/text_based.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/__init__.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/base.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/core.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/mathml/fields.py(1 hunks)contentcuration/contentcuration/utils/assessment/qti/prompt.py(1 hunks)contentcuration/contentcuration/utils/publish.py(3 hunks)requirements-dev.in(1 hunks)requirements-dev.txt(2 hunks)requirements.in(1 hunks)requirements.txt(4 hunks)
✅ Files skipped from review due to trivial changes (5)
- contentcuration/contentcuration/utils/assessment/qti/constants.py
- contentcuration/contentcuration/utils/assessment/qti/html/base.py
- contentcuration/contentcuration/utils/assessment/qti/html/table.py
- contentcuration/contentcuration/utils/assessment/qti/archive.py
- contentcuration/contentcuration/utils/assessment/qti/assessment_item.py
🚧 Files skipped from review as they are similar to previous changes (23)
- requirements-dev.in
- contentcuration/contentcuration/utils/assessment/qti/init.py
- requirements.in
- contentcuration/contentcuration/utils/assessment/qti/prompt.py
- requirements-dev.txt
- contentcuration/contentcuration/utils/assessment/qti/html/breaks.py
- contentcuration/contentcuration/tests/utils/qti/test_imsmanifest.py
- requirements.txt
- contentcuration/contentcuration/utils/assessment/qti/interaction_types/base.py
- contentcuration/contentcuration/utils/assessment/qti/html/media.py
- contentcuration/contentcuration/utils/assessment/qti/interaction_types/text_based.py
- contentcuration/contentcuration/utils/assessment/qti/html/content_types.py
- contentcuration/contentcuration/utils/assessment/qti/mathml/base.py
- contentcuration/contentcuration/tests/utils/qti/test_qti.py
- contentcuration/contentcuration/utils/assessment/qti/html/embed.py
- contentcuration/contentcuration/tests/utils/test_markdown.py
- contentcuration/contentcuration/utils/assessment/qti/html/sequence.py
- contentcuration/contentcuration/utils/publish.py
- contentcuration/contentcuration/utils/assessment/qti/mathml/fields.py
- contentcuration/contentcuration/utils/assessment/qti/html/init.py
- contentcuration/contentcuration/utils/assessment/qti/mathml/init.py
- contentcuration/contentcuration/utils/assessment/qti/fields.py
- contentcuration/contentcuration/utils/assessment/qti/imsmanifest.py
🧰 Additional context used
🧠 Learnings (2)
contentcuration/contentcuration/utils/assessment/qti/base.py (1)
Learnt from: rtibbles
PR: learningequality/studio#5103
File: contentcuration/contentcuration/utils/assessment/qti/base.py:59-65
Timestamp: 2025-06-15T23:49:02.211Z
Learning: In the QTI assessment codebase, prefer immutable collections (tuples) over mutable ones (lists) for child elements to ensure cache invalidation works correctly and maintain API predictability.
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py (2)
Learnt from: rtibbles
PR: learningequality/studio#5103
File: contentcuration/contentcuration/utils/assessment/qti/mathml/core.py:48-51
Timestamp: 2025-06-13T18:24:09.818Z
Learning: In the latest MathML Core specification, the literal string "infinity" is no longer an allowed value for the `maxsize` and `minsize` attributes on the `<mo>` element; only length-percentage values are valid.
Learnt from: rtibbles
PR: learningequality/studio#5103
File: contentcuration/contentcuration/utils/assessment/qti/mathml/core.py:210-212
Timestamp: 2025-06-15T23:46:31.075Z
Learning: The `maction` element is deprecated in MathML Core and should be removed rather than maintained or fixed in MathML-related codebases.
🧬 Code Graph Analysis (7)
contentcuration/contentcuration/tests/utils/qti/test_fields.py (1)
contentcuration/contentcuration/utils/assessment/qti/fields.py (4)
validate_data_uri(32-42)validate_local_href_path(45-60)validate_local_src_path(63-74)validate_local_srcset(85-99)
contentcuration/contentcuration/utils/assessment/qti/base.py (1)
contentcuration/contentcuration/utils/assessment/qti/constants.py (1)
Dir(47-50)
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py (3)
contentcuration/contentcuration/utils/assessment/qti/mathml/base.py (6)
MathMLElement(12-24)MathMLGroupingElement(63-69)MathMLLayoutElement(45-51)MathMLPresentationElement(27-33)MathMLScriptElement(54-60)MathMLTokenElement(36-42)contentcuration/contentcuration/utils/assessment/qti/mathml/fields.py (2)
MathMLDisplay(58-60)MathMLForm(63-66)contentcuration/contentcuration/utils/assessment/qti/base.py (2)
element_name(62-63)element_name(150-153)
contentcuration/contentcuration/tests/utils/qti/test_mathml.py (5)
contentcuration/contentcuration/utils/assessment/qti/base.py (5)
TextNode(39-42)element_name(62-63)element_name(150-153)to_xml_string(133-136)from_string(276-284)contentcuration/contentcuration/utils/assessment/qti/constants.py (1)
Dir(47-50)contentcuration/contentcuration/utils/assessment/qti/mathml/core.py (26)
Annotation(203-206)AnnotationXml(209-215)Math(28-31)Mfrac(80-82)Mi(34-37)Mn(40-41)Mo(44-56)Mrow(76-77)Mspace(67-73)Msubsup(122-123)Mtable(199-200)Mtd(189-192)Mtr(195-196)Semantics(218-243)Mmultiscripts(146-186)Mover(131-133)Mphantom(110-111)Mprescripts(142-143)Mroot(89-90)Ms(63-64)Msub(114-115)Msup(118-119)Mtext(59-60)Munder(126-128)Munderover(136-139)element_name(214-215)contentcuration/contentcuration/utils/assessment/qti/mathml/fields.py (2)
MathMLDisplay(58-60)MathMLForm(63-66)contentcuration/contentcuration/utils/assessment/qti/mathml/base.py (5)
MathMLElement(12-24)MathMLGroupingElement(63-69)MathMLLayoutElement(45-51)MathMLScriptElement(54-60)MathMLTokenElement(36-42)
contentcuration/contentcuration/utils/assessment/qti/html/display.py (2)
contentcuration/contentcuration/utils/assessment/qti/html/base.py (2)
FlowContentElement(22-23)HTMLElement(11-19)contentcuration/contentcuration/utils/assessment/qti/html/text.py (2)
BlockHTMLText(20-21)InlineHTMLText(16-17)
contentcuration/contentcuration/utils/assessment/qti/html/flow.py (1)
contentcuration/contentcuration/utils/assessment/qti/html/base.py (1)
BlockContentElement(30-31)
contentcuration/contentcuration/utils/assessment/qti/html/text.py (2)
contentcuration/contentcuration/utils/assessment/qti/html/base.py (2)
BlockContentElement(30-31)InlineContentElement(26-27)contentcuration/contentcuration/utils/assessment/qti/base.py (2)
element_name(62-63)element_name(150-153)
🪛 Ruff (0.11.9)
contentcuration/contentcuration/tests/utils/qti/test_fields.py
258-259: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
270-271: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
282-283: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
293-294: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
contentcuration/contentcuration/tests/utils/test_exercise_creation.py
280-280: Loop control variable image_name not used within loop body
Rename unused image_name to _image_name
(B007)
contentcuration/contentcuration/tests/utils/qti/test_mathml.py
123-124: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
174-175: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
210-211: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
1482-1482: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
🪛 Pylint (3.3.7)
contentcuration/contentcuration/utils/assessment/qti/base.py
[refactor] 39-39: Too few public methods (0/2)
(R0903)
[refactor] 85-114: Unnecessary "elif" after "raise", remove the leading "el" from "elif"
(R1720)
[refactor] 74-127: Too many nested blocks (6/5)
(R1702)
[refactor] 65-65: Too many branches (16/12)
(R0912)
contentcuration/contentcuration/tests/utils/test_exercise_creation.py
[refactor] 192-192: Too many local variables (25/15)
(R0914)
[refactor] 1171-1171: Too many arguments (6/5)
(R0913)
[refactor] 1171-1171: Too many positional arguments (6/5)
(R0917)
[refactor] 1211-1211: Consider using 'with' for resource-allocating operations
(R1732)
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py
[refactor] 114-114: Too many ancestors (8/7)
(R0901)
[refactor] 118-118: Too many ancestors (8/7)
(R0901)
[refactor] 122-122: Too many ancestors (8/7)
(R0901)
[refactor] 126-126: Too many ancestors (8/7)
(R0901)
[refactor] 131-131: Too many ancestors (8/7)
(R0901)
[refactor] 136-136: Too many ancestors (8/7)
(R0901)
[refactor] 146-146: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/tests/utils/qti/test_html.py
[refactor] 322-322: Too many local variables (31/15)
(R0914)
[refactor] 322-322: Too many statements (61/50)
(R0915)
[refactor] 415-415: Too many local variables (22/15)
(R0914)
[refactor] 415-415: Too many statements (56/50)
(R0915)
contentcuration/contentcuration/utils/assessment/markdown.py
[refactor] 60-60: Simplify chained comparison between the operands
(R1716)
contentcuration/contentcuration/utils/assessment/qti/html/display.py
[refactor] 17-17: Too many ancestors (8/7)
(R0901)
[error] 34-34: Method 'validate_summary_position' should have "self" as first argument
(E0213)
[error] 58-58: Method 'validate_figcaption_position' should have "self" as first argument
(E0213)
contentcuration/contentcuration/utils/assessment/qti/html/flow.py
[refactor] 20-20: Too many ancestors (8/7)
(R0901)
[refactor] 24-24: Too many ancestors (8/7)
(R0901)
[refactor] 28-28: Too many ancestors (8/7)
(R0901)
[refactor] 32-32: Too many ancestors (8/7)
(R0901)
[refactor] 36-36: Too many ancestors (8/7)
(R0901)
[refactor] 40-40: Too many ancestors (8/7)
(R0901)
[refactor] 44-44: Too many ancestors (8/7)
(R0901)
[refactor] 48-48: Too many ancestors (8/7)
(R0901)
[refactor] 52-52: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/utils/assessment/qti/html/text.py
[refactor] 24-24: Too many ancestors (8/7)
(R0901)
[refactor] 29-29: Too many ancestors (8/7)
(R0901)
[refactor] 33-33: Too many ancestors (8/7)
(R0901)
[refactor] 37-37: Too many ancestors (8/7)
(R0901)
[refactor] 41-41: Too many ancestors (8/7)
(R0901)
[refactor] 45-45: Too many ancestors (8/7)
(R0901)
[refactor] 49-49: Too many ancestors (8/7)
(R0901)
[refactor] 53-53: Too many ancestors (8/7)
(R0901)
[refactor] 57-57: Too many ancestors (8/7)
(R0901)
[refactor] 61-61: Too many ancestors (8/7)
(R0901)
[refactor] 65-65: Too many ancestors (8/7)
(R0901)
[refactor] 69-69: Too many ancestors (8/7)
(R0901)
[refactor] 73-73: Too many ancestors (8/7)
(R0901)
[refactor] 77-77: Too many ancestors (8/7)
(R0901)
[refactor] 81-81: Too many ancestors (8/7)
(R0901)
[refactor] 85-85: Too many ancestors (8/7)
(R0901)
[refactor] 89-89: Too many ancestors (8/7)
(R0901)
[refactor] 93-93: Too many ancestors (8/7)
(R0901)
[refactor] 97-97: Too many ancestors (8/7)
(R0901)
[refactor] 101-101: Too many ancestors (8/7)
(R0901)
[refactor] 105-105: Too many ancestors (8/7)
(R0901)
[refactor] 109-109: Too many ancestors (8/7)
(R0901)
[refactor] 113-113: Too many ancestors (8/7)
(R0901)
[refactor] 117-117: Too many ancestors (8/7)
(R0901)
[refactor] 121-121: Too many ancestors (8/7)
(R0901)
[refactor] 130-130: Too many ancestors (8/7)
(R0901)
[refactor] 134-134: Too many ancestors (8/7)
(R0901)
[refactor] 138-138: Too many ancestors (8/7)
(R0901)
contentcuration/contentcuration/utils/assessment/qti/interaction_types/simple.py
[error] 45-45: Method '_unique_answer_identifiers' should have "self" as first argument
(E0213)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Python unit tests
- GitHub Check: Build frontend assets
🔇 Additional comments (3)
contentcuration/contentcuration/utils/assessment/qti/interaction_types/simple.py (1)
44-55: Validator looks goodThe duplicate-identifier guard is correctly implemented and returned; nice addition.
🧰 Tools
🪛 Pylint (3.3.7)
[error] 45-45: Method '_unique_answer_identifiers' should have "self" as first argument
(E0213)
contentcuration/contentcuration/utils/assessment/qti/html/display.py (1)
33-47: Positional validators implemented correctlyBoth
DetailsandFigurenow enforce spec-compliant structure with after-parse validators—good catch on usingmode="after".Also applies to: 57-68
🧰 Tools
🪛 Pylint (3.3.7)
[error] 34-34: Method 'validate_summary_position' should have "self" as first argument
(E0213)
contentcuration/contentcuration/utils/assessment/qti/base.py (1)
65-72: Cached_elementprevents updated file-dependency collectionOn first call,
_file_dependenciesis rebuilt, after whichto_element()short-circuits via the cache.
If any descendant element is generated after the first call (common in incremental workflows), itssrcvalues will never reach the root’s dependency set because the parent will not re-walk the tree.Either:
- Drop the cache (it’s cheap – ElementTree building is local and small), or
- Keep the cache but always return a copy of
_file_dependenciesthat merges children each time, or- Invalidate the cache whenever a child element’s
to_element()is called the first time.The first option is the simplest and safest.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 65-65: Too many branches (16/12)
(R0912)
contentcuration/contentcuration/tests/utils/test_exercise_creation.py
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/utils/assessment/qti/mathml/core.py
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/utils/assessment/qti/archive.py
Outdated
Show resolved
Hide resolved
…us class. Add fixes for formula and image processing.
Generalize tree generation code from HTML for reuse in MathML.
| @@ -151,7 +145,9 @@ def create_content_database( | |||
| """ | |||
| :type progress_tracker: contentcuration.utils.celery.ProgressTracker|None | |||
| """ | |||
| # increment the channel version | |||
| if not channel.language: | |||
There was a problem hiding this comment.
We enforce this in the frontend, but it was not being enforced during publish, so I added this here - it is required for creating QTI assessment items.
This is also why I had to update some tests and test data in order to ensure the channel had a language set.
| Exercise zip generator for Perseus format exercises. | ||
| """ | ||
|
|
||
| ZIP_DATE_TIME = (2013, 3, 14, 1, 59, 26) |
There was a problem hiding this comment.
These are the values for adding files to the perseus zip that were previously used, overwriting the ricecooker aligned values in the base class.
| "true_false": "perseus/multiple_selection.json", | ||
| } | ||
|
|
||
| def _write_raw_perseus_image_files(self, assessment_item): |
There was a problem hiding this comment.
Perseus specific logic for handling image replacement in raw Perseus JSON. This handles both regular image files and the special "GRAPHIE" files, which are actually a file containing both SVG and JSON, to describe and label diagrams.
| self.add_file_to_write(json_name, content[1]) | ||
|
|
||
| def _process_formulas(self, content): | ||
| return _DOUBLE_DOLLAR_RE.sub(r"$\1$", content) |
There was a problem hiding this comment.
We double wrap formulae in $ signs to allow literal $ to be used in formulae, but Perseus parses single wrapped $ - I don't actually know how it handles $ literals.
| """Extract input answer processing logic""" | ||
| non_empty_answers = [] | ||
| for answer in processed_data["answers"]: | ||
| answer["answer"] = extract_value(answer["answer"]) |
There was a problem hiding this comment.
We do special coercion here to change the numeric answer into the format needed by Perseus.
| # Look for both item references and inline items | ||
| qti_ns = {"qti": "http://www.imsglobal.org/xsd/imsqti_v3p0"} | ||
| item_refs = test_root.findall(".//qti:qti-assessment-item-ref", qti_ns) | ||
| inline_items = test_root.findall(".//qti:qti-assessment-item", qti_ns) |
There was a problem hiding this comment.
I think this is also a no op, as I don't think assessment items can be directly included in assessment tests (or assessment sections).
| @@ -319,20 +323,43 @@ def recurse_nodes(self, node, inherited_fields): # noqa C901 | |||
| ) | |||
|
|
|||
| if node.kind_id == content_kinds.EXERCISE: | |||
| exercise_data = process_assessment_metadata(node, kolibrinode) | |||
| exercise_data = process_assessment_metadata(node) | |||
There was a problem hiding this comment.
We defer generation of the kolibri AssessmentMetadata object until after file generation has completed.
| @@ -319,20 +323,43 @@ def recurse_nodes(self, node, inherited_fields): # noqa C901 | |||
| ) | |||
|
|
|||
| if node.kind_id == content_kinds.EXERCISE: | |||
| exercise_data = process_assessment_metadata(node, kolibrinode) | |||
| exercise_data = process_assessment_metadata(node) | |||
| any_free_response = any( | |||
There was a problem hiding this comment.
Only use QTI for free response containing exercises.
|
|
||
| # If this exercise previously had a file generated by a different | ||
| # generator, make sure we clean it up here. | ||
| stale_presets = { |
There was a problem hiding this comment.
Ensure we cleanup old files if an exercise now has free response or did previously have free response.
| return exercise_data | ||
|
|
||
|
|
||
| def create_kolibri_assessment_metadata(ccnode, kolibrinode): |
There was a problem hiding this comment.
Small refactor to move this after file generation and make it conditional depending on the file type.
Summary
References
Fixes #4878
Reviewer guidance
Creating a "Survey" exercise with a free response question and then checking you can successfully publish it is the main manual check.
The generated QTI item XML can be cross checked against https://qti.amp-up.io/testrunner/sandbox/
The IMSManifest format should be checked against the examples here: https://www.imsglobal.org/spec/qti/v3p0/guide#h.r1pkoq891wk7 and here: https://www.imsglobal.org/spec/qti/v3p0/impl#h.5esb962k7g83
The QTI schema is incredibly verbose. Where I can, I have attempted to slightly reduce the verbosity if possible, but mostly the verbosity of the schema has resulted in a lot of structural code.