Courses, Units, Lessons, and KPUB#653
Courses, Units, Lessons, and KPUB#653rtibbles wants to merge 4 commits intolearningequality:mainfrom
Conversation
bjester
left a comment
bjester
left a comment
There was a problem hiding this comment.
Needs to update the le_utils version
|
Updated! |
tests/pipeline/test_convert.py
Outdated
| """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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
ricecooker/classes/curriculum.py
Outdated
| """ | ||
| # 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): |
There was a problem hiding this comment.
I believe this regex would pass with just \t or \n
ricecooker/classes/nodes.py
Outdated
| @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 |
There was a problem hiding this comment.
I presume this is for consistency with ExerciseNode, but they have no shared interface, besides declaring this property.
tests/test_curriculum.py
Outdated
|
|
||
| def test_variants_are_distinct(self): | ||
| """VARIANT_A and VARIANT_B have different values.""" | ||
| assert VARIANT_A != VARIANT_B |
There was a problem hiding this comment.
Surely a code review is sufficient for this 😅
| assert result.content_node_metadata is not None | ||
| assert result.content_node_metadata["kind"] == content_kinds.DOCUMENT | ||
| finally: | ||
| os.unlink(temp_archive.name) |
There was a problem hiding this comment.
Similar to the other kpub tests
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>
2 participants
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