Courses, Units, Lessons, and KPUB by rtibbles · Pull Request #653 · learningequality/ricecooker

rtibbles · 2026-01-20T00:03:47Z

Summary

Adds support for uploading KPUB files
Adds Courses, Units, and Lessons nodes with validation
Adds example courses script for two sample courses.

References

Fixes #638

Reviewer guidance

bjester

Needs to update the le_utils version

rtibbles · 2026-02-08T21:25:24Z

Updated!

bjester · 2026-02-17T22:18:00Z

tests/pipeline/test_convert.py

+    """A file with .kpub extension that isn't a valid zip should fail."""
+    temp_file = tempfile.NamedTemporaryFile(suffix=".kpub", delete=False)
+    temp_file.write(b"not a zip file")
+    temp_file.close()
+
+    try:
+        handler = KPUBConversionHandler()
+        with pytest.raises(InvalidFileException) as exc_info:
+            handler.validate_archive(temp_file.name)
+
+        assert "zip" in str(exc_info.value).lower()
+    finally:
+        os.unlink(temp_file.name)


I presume these are LLM generated?

6 out of 7 tests follow pretty much the same pattern, with this test being the outlier. A test case would eliminate the redundancy.

Although, the approach is somewhat poor. It could use a context manager without delete=False and eliminate the try / finally / unlink. With that change, a test case isn't quite the dramatic cleanup.

Yes - sorry, clearly I forgot to tag this, and I am now remembering that I never came around and did another pass over the code, just pushed the draft once I had the test channels in place.

Will rejig the tests, then double check the implementation.

bjester · 2026-02-17T22:20:02Z

ricecooker/classes/curriculum.py

+        """
+        # Text must be non-empty and not just whitespace
+        # Pattern from le_utils schema: ^\s*\S[\s\S]*$
+        if not self.text or not re.match(r"^\s*\S[\s\S]*$", self.text):


I believe this regex would pass with just \t or \n

bjester · 2026-02-17T22:39:38Z

ricecooker/classes/nodes.py

+    @questions.setter
+    def questions(self, value):
+        """Setter to allow base Node class initialization (ignored for UnitNode)."""
+        # UnitNode manages questions through test_questions, so ignore base class assignment
+        pass


I presume this is for consistency with ExerciseNode, but they have no shared interface, besides declaring this property.

bjester · 2026-02-17T22:45:01Z

tests/test_curriculum.py

+
+    def test_variants_are_distinct(self):
+        """VARIANT_A and VARIANT_B have different values."""
+        assert VARIANT_A != VARIANT_B


Surely a code review is sufficient for this 😅

bjester · 2026-02-17T22:45:22Z

tests/pipeline/test_extract_metadata.py

+            assert result.content_node_metadata is not None
+            assert result.content_node_metadata["kind"] == content_kinds.DOCUMENT
+        finally:
+            os.unlink(temp_archive.name)


Similar to the other kpub tests

rtibbles · 2026-03-20T22:53:50Z

Have reworked and deduplicated the tests here.

bjester

Overall everything looks good enough for merge, but I think it'd be worthwhile to match exception messages in TestUnitNodeValidation considering the complexity of the validation. And so for that, at least, I'm requesting changes.

bjester · 2026-03-23T21:43:40Z

examples/curriculum_courses/sushichef.py

+        self._temp_dir = tempfile.TemporaryDirectory(prefix="curriculum_kpub_")
+        self.temp_dir = self._temp_dir.name


Not a big deal since this is an example, but I'm assuming self._temp_dir doesn't need to be set on the class? Looks a little weird.

Yeah, not needed at all.

bjester · 2026-03-23T21:48:51Z

ricecooker/classes/curriculum.py

+
+    def validate(self):
+        """Validate the learning objective against the le_utils schema."""
+        return bool(self.text and self.text.strip())


The UUID generation in the constructor should fail if text is None, so is there use for self.text too?

It won't fail if text is an empty or all whitespace string - but I've moved this into init.py to raise earlier.

bjester · 2026-03-23T21:56:19Z

ricecooker/classes/nodes.py

+        if not learning_objectives:
+            raise InvalidNodeException(
+                "Question must have at least one learning objective"
+            )


Seems add_child and add_question both validate this and both call _validate_learning_objectives. Any reason not to add it there?

Nope - made the error message more generic and moved it.

bjester · 2026-03-23T22:06:16Z

ricecooker/classes/nodes.py

+        Args:
+            node: LessonNode to add
+            learning_objectives: List of LearningObjective instances


Seems ricecooker has inconsistent docstring styles...

Yes, they're all over the shop - I think we should just decide what our standard is across our repos and then standardize.

bjester · 2026-03-23T22:13:16Z

tests/test_curriculum.py

+        unit = UnitNode(source_id="unit-1", title="Math Unit")
+        lesson = LessonNode(source_id="lesson-1", title="Lesson 1")
+        lo = LearningObjective("Understand addition")
+        unit.add_child(lesson, learning_objectives=[lo])


The tests here are passing this as a kwarg, which I know is okay, but I think this is considered bad practice?

Yes, updated to just pass positionally.

bjester · 2026-03-23T22:15:58Z

tests/test_curriculum.py

+        unit.add_question(make_question("q5"), VARIANT_B, learning_objectives=[lo1])
+        unit.add_question(make_question("q6"), VARIANT_B, learning_objectives=[lo2])
+
+        with pytest.raises(InvalidNodeException):


These tests look like good coverage, but I think it would provide more certainty that the validation cases are hitting the right spots with some exception message matching for each case.

Added message matching to all to check this.

…osttests.

html5lib sets body.text to None when the body element starts with a child element rather than text. Guard against this the same way as KPUBConversionHandler. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

rtibbles · 2026-04-08T01:53:59Z

Updated!

rtibbles force-pushed the of_course branch from fa7f60c to c49fdb1 Compare January 20, 2026 00:04

rtibbles marked this pull request as draft January 20, 2026 00:05

rtibbles mentioned this pull request Jan 20, 2026

Unit tests (as in, tests for unit folders) learningequality/studio#5646

Merged

bjester reviewed Jan 20, 2026

View reviewed changes

rtibbles force-pushed the of_course branch from c49fdb1 to 5b1a214 Compare February 8, 2026 21:24

bjester reviewed Feb 17, 2026

View reviewed changes

rtibbles force-pushed the of_course branch from 5b1a214 to f7b1ea3 Compare March 1, 2026 01:01

Add support for validating and uploading KPUB files.

461ab12

rtibbles force-pushed the of_course branch from f7b1ea3 to fabf82c Compare March 1, 2026 17:44

rtibbles marked this pull request as ready for review March 20, 2026 22:53

bjester requested changes Mar 23, 2026

View reviewed changes

rtibbles force-pushed the of_course branch from fabf82c to ac47cdc Compare April 7, 2026 20:27

rtibbles and others added 3 commits April 7, 2026 18:15

Add support for courses, units, lessons, learning objectives and prep…

5b80f2c

…osttests.

Add example course script.

7c933ae

Fix body.text None crash in HTML5ConversionHandler.

ddfb95e

html5lib sets body.text to None when the body element starts with a child element rather than text. Guard against this the same way as KPUBConversionHandler. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

rtibbles force-pushed the of_course branch from ac47cdc to ddfb95e Compare April 8, 2026 01:15

		self._temp_dir = tempfile.TemporaryDirectory(prefix="curriculum_kpub_")
		self.temp_dir = self._temp_dir.name

Uh oh!

Conversation

rtibbles commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

References

Reviewer guidance

Uh oh!

bjester left a comment

Choose a reason for hiding this comment

Uh oh!

rtibbles commented Feb 8, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtibbles commented Mar 20, 2026

Uh oh!

bjester left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtibbles commented Apr 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rtibbles commented Jan 20, 2026 •

edited

Loading