diff --git a/CHANGELOG.md b/CHANGELOG.md index 89bb1343..255eb51f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,12 @@ accepted/draft snapshots. Add a configurable Learning in Public link editor that preserves autosave, supports an optional blank state, and falls back to a plain textarea without JavaScript. +## 0.5.13 + +- #306: derive recursive mixed course hierarchy from repository directories and import structured + YAML homework units with Markdown prose companions. Reparenting keeps Unit, Homework, Question, + Answer and Submission identities; course-tree scoring answers stay out of learner projections. + ## 0.5.11 - #301: add a six-state learner homework descriptor and a read-only helper for rendering the same diff --git a/community_base/__init__.py b/community_base/__init__.py index e1e093c0..ce25754b 100644 --- a/community_base/__init__.py +++ b/community_base/__init__.py @@ -1 +1 @@ -__version__ = "0.5.12" +__version__ = "0.5.13" diff --git a/community_base/content_sync/FORMAT.md b/community_base/content_sync/FORMAT.md index 477e27ba..65a7b4bc 100644 --- a/community_base/content_sync/FORMAT.md +++ b/community_base/content_sync/FORMAT.md @@ -158,10 +158,11 @@ provenance set on those two models. - A tree is expressed by directories and by nothing else. `parent:` keys do not exist. - In a docs collection: a directory is a node and must contain `index.md`; leaves are `NN-slug.md` files; maximum depth four below the collection root. -- In a course: a module is a directory holding `module.yaml`; a submodule is a directory holding - `module.yaml` inside a module directory; maximum two module levels - (`curriculum.source.validate_module_tree`); a module directory holds either submodule directories - or unit files, never both, apart from `README.md` and asset directories. +- In a course: a module is a directory holding `module.yaml`. Module directories may nest to any + depth and may contain direct units beside child module directories. Direct units and child + modules share one sibling order; every sibling must have a unique order, from its numeric name + prefix or an explicit `sort_order`. Missing or duplicate sibling orders are errors. `README.md`, + `homework.md` companions and asset directories are supporting files, not siblings. - A wiki collection is flat: one directory of `slug.md` files; subdirectories are an error, apart from asset directories. - Cohorts are not part of the tree; they are placements (section 3.8, course). @@ -352,8 +353,9 @@ Layout, with `` the collection path (`.` for a single-course repository, /NN-/NN-.md /NN-/images/... assets /NN-/code/... never synced, referenced by unit `code` -/NN-/NN-/module.yaml optional second level -/NN-/NN-/NN-.md +/NN-/NN-/module.yaml recursive module directory +/NN-/NN-homework/homework.yaml structured homework unit +/NN-/NN-homework/homework.md homework page prose, required /cohorts//cohort.yaml /cohorts//README.md cohort notes or archive notice, optional /cohorts//homework//homework.yaml @@ -403,6 +405,12 @@ under `extra` and stay DTC-read. `cohorts`, `current_cohort`, `urls`, `schema_ve syllabus. Set it on the first top-level module in a section. It is a presentation label; it does not affect module ordering, access or progress. +The directory tree is the module hierarchy at every depth. A module may mix child module +directories, ordinary Markdown units and structured homework unit directories. Those direct units +and child modules occupy one ordered sibling sequence. Their numeric directory/file prefixes are +orders unless `sort_order` is written explicitly; each parent's sibling orders must be present and +unique. + No `units` list, no `schema_version`, no `bonus`, no `ignore`. The overview is `README.md`. Unit document `NN-.md` @@ -420,6 +428,33 @@ The body is the lesson. A `kind: homework` unit body is the instructions page; t assignment is the cohort's `homework.yaml`. `is_homework`, `is_preview`, `access`, `prev_url` and `next_url` do not exist. +Structured course-tree homework unit directory `NN-/homework.yaml` and `homework.md` + +The directory is one unit in its parent module's sibling order. `homework.yaml` uses the normal +unit core keys (`content_id`, `title`, and `slug`; order comes from the numeric directory prefix or +an explicit `sort_order`) plus these fields: + +| Key | Type | Required | Default | +|---|---|---|---| +| `due_at` | ISO datetime with offset | yes | none | +| `form` | mapping of form flags and `learning_in_public_cap` | no | field defaults | +| `final_fields` | list of `{key, label, type, required}` | no | `[]` | +| `questions` | ordered question list | yes | none | + +Question `type` is `multiple_choice`, `checkboxes`, `free_form` or `free_form_long`. Every +question has a UUID `content_id`, stable authored `id` slug, `prompt`, and optional `points` +(default `1`) and `step_label`. Choice questions carry ordered `{id, label}` `options`; free-form +questions carry an `answer_type` (`any`, `float`, `integer`, `exact_string` or `contains_string`). +Course-tree homework alone accepts `correct` in the existing scoring format (one-based option +indices for choice questions). The importer carries it into the existing scoring field and +learner-facing curriculum projections do not include it. Answer sealing follows when the shared +keyring is provisioned. + +The required `homework.md` companion is the unit's prose body and is stored as the unit's homework +content; it is not a cohort assignment instruction document. This convention does not change +cohort manifests below: their answers remain encrypted envelopes and plaintext `correct` remains +invalid there. + `cohorts//cohort.yaml`. The identifier is the directory name and is not repeated inside the file. It follows the slug pattern (`2026`, `self-paced`, `4`). diff --git a/community_base/content_sync/check.py b/community_base/content_sync/check.py index 26fea3dc..f4d04b1c 100644 --- a/community_base/content_sync/check.py +++ b/community_base/content_sync/check.py @@ -132,6 +132,7 @@ def main(argv: list[str] | None = None) -> int: def _check_dialect(item: ParsedDocument, headings: list[tuple[int, str, str]], diagnostics) -> None: title = item.data.get("title") + body_source = item.raw.body_path or item.raw.path for number, line in _code_free_lines(item.body): located = item.body_line + number for pattern, rule, message, severity in _DIALECT_RULES: @@ -140,7 +141,7 @@ def _check_dialect(item: ParsedDocument, headings: list[tuple[int, str, str]], d continue diagnostics.append( Diagnostic( - item.raw.path, + body_source, "/body", rule, f"{message}: {match.group(0).strip()[:60]}", @@ -154,7 +155,7 @@ def _check_dialect(item: ParsedDocument, headings: list[tuple[int, str, str]], d for message in _check_embed(line): diagnostics.append( Diagnostic( - item.raw.path, + body_source, "/body", "4.1", message, @@ -166,7 +167,7 @@ def _check_dialect(item: ParsedDocument, headings: list[tuple[int, str, str]], d if level == 1 and text.strip().lower() == title.strip().lower(): diagnostics.append( Diagnostic( - item.raw.path, + body_source, "/body", "4.1", "the body repeats the title as a leading H1; the renderer strips it", diff --git a/community_base/content_sync/documents.py b/community_base/content_sync/documents.py index b2f30d4a..2f52cbba 100644 --- a/community_base/content_sync/documents.py +++ b/community_base/content_sync/documents.py @@ -180,6 +180,7 @@ def record(self) -> dict[str, Any]: "kind": self.kind, "part": self.part.name, "source_path": self.raw.path, + "body_source_path": self.raw.body_path or self.raw.path, "parent": self.raw.parent, "path": self.path, "slug": self.slug, @@ -531,9 +532,13 @@ def _node_at(tree: DirNode, path: str) -> DirNode | None: def _read_collection( - repository: Repository, collection: Collection, diagnostics: list[Diagnostic] + repository: Repository, + collection: Collection, + diagnostics: list[Diagnostic], + *, + collection_root: DirNode | None = None, ) -> list[ParsedDocument]: - node = _node_at(repository.tree, collection.path) + node = collection_root or _node_at(repository.tree, collection.path) if node is None and (repository.root / collection.path).is_dir(): # A collection root the repository holds but whose every file `ignore` # hides, or which is empty, is an empty collection and not a missing @@ -588,6 +593,12 @@ def _read_item( diagnostics.append(locate(raw.path, problem)) if problems: return None + if raw.body_path: + body, body_line, body_problems = _load_companion_markdown(repository, raw.body_path) + for problem in body_problems: + diagnostics.append(locate(raw.body_path, problem)) + if body_problems: + return None if isinstance(content, Mapping): data = dict(content) for problem in check_item_keys(content, part): @@ -616,7 +627,7 @@ def _read_item( sort_order=_item_sort_order(raw, data), required_level=0, path=slug, - is_document=_is_document(raw.path), + is_document=_is_document(raw.path) or raw.body_path is not None, ) @@ -846,6 +857,18 @@ def _load_document(repository: Repository, rel: str) -> tuple[Any, str, int, lis return data, body, closing + 1, [] +def _load_companion_markdown(repository: Repository, rel: str) -> tuple[str, int, list[Problem]]: + """Read prose-only Markdown paired with a structured manifest item.""" + + try: + text = repository.read_text(rel) + except UnicodeDecodeError: + return "", 0, [Problem(WHOLE_FILE, "3.2", "must be UTF-8")] + if "\r\n" in text: + return "", 0, [Problem(WHOLE_FILE, "3.2", "must use LF line endings")] + return text, 0, [] + + def one_line(error: Exception) -> str: """An exception message on one line, for a diagnostic.""" diff --git a/community_base/content_sync/kinds/base.py b/community_base/content_sync/kinds/base.py index 2440dacd..21b39ba2 100644 --- a/community_base/content_sync/kinds/base.py +++ b/community_base/content_sync/kinds/base.py @@ -97,6 +97,11 @@ class RawItem: name: str parent: str | None = None contributes_slug: bool = True + # Composite items may keep their machine-readable fields in one file and + # their Markdown prose in a sibling companion. The layout still emits one + # item, keyed by ``path``; this path is the body source for rendering and + # relative references. + body_path: str | None = None @dataclass(frozen=True, slots=True) diff --git a/community_base/content_sync/kinds/course.py b/community_base/content_sync/kinds/course.py index 5937e84f..7396c6c2 100644 --- a/community_base/content_sync/kinds/course.py +++ b/community_base/content_sync/kinds/course.py @@ -33,6 +33,21 @@ "docs_url": KeySpec("url"), "faq_url": KeySpec("url"), "hashtag": KeySpec("hashtag"), + # Kept source-relative so the curriculum parser can resolve the + # authored course hierarchy rather than guessing from a title or + # site-owned route. + "projects": KeySpec( + "object_list", + item_keys={ + "slug": KeySpec("slug", required=True), + "title": KeySpec("string", required=True, max_length=200), + "module_path": KeySpec("string", required=True), + "cohort_key": KeySpec("slug"), + "submission_due_at": KeySpec("datetime", required=True), + "review_due_at": KeySpec("datetime", required=True), + "peer_review_count": KeySpec("integer"), + }, + ), "testimonials": KeySpec( "object_list", item_keys={ @@ -52,6 +67,9 @@ keys={ "syllabus_section": KeySpec("string", max_length=255), "is_bonus": KeySpec("boolean", default=False), + # Existing course sources call the same generic module flag `bonus`. + # The graph and persistence API use the clearer `is_bonus` name. + "bonus": KeySpec("boolean"), "available_after_days": KeySpec("integer"), }, ) @@ -71,6 +89,7 @@ ), "session_position": KeySpec("integer"), "is_bonus": KeySpec("boolean", default=False), + "available_after_days": KeySpec("integer"), "code": KeySpec( "object_list", item_keys={ @@ -81,6 +100,56 @@ }, ) +HOMEWORK_UNIT = PartSpec( + name="homework_unit", + shape=SHAPE_MANIFEST, + keys={ + "due_at": KeySpec("datetime", required=True), + "is_bonus": KeySpec("boolean", default=False), + "available_after_days": KeySpec("integer"), + "form": KeySpec( + "mapping", + item_keys={ + **{name: KeySpec("boolean") for name in FORM_FLAGS}, + "learning_in_public_cap": KeySpec("integer"), + }, + ), + "final_fields": KeySpec( + "object_list", + item_keys={ + "key": KeySpec("slug", required=True), + "label": KeySpec("string", required=True), + "type": KeySpec("choice", choices=("text", "url", "textarea")), + "required": KeySpec("boolean", default=False), + }, + ), + "questions": KeySpec( + "object_list", + required=True, + item_keys={ + "content_id": KeySpec("uuid", required=True), + "id": KeySpec("slug", required=True), + "type": KeySpec("choice", choices=QUESTION_TYPES, required=True), + "prompt": KeySpec("markdown", required=True), + "points": KeySpec("integer", default=1), + "step_label": KeySpec("string"), + "options": KeySpec( + "object_list", + item_keys={ + "id": KeySpec("slug", required=True), + "label": KeySpec("string", required=True), + }, + ), + "answer_type": KeySpec("choice", choices=ANSWER_TYPES), + # The source course repository still owns historical scalar + # answers. This key exists only for course-tree homework units; + # cohort homework manifests continue to require envelopes. + "correct": KeySpec("string"), + }, + ), + }, +) + COHORT = PartSpec( name="cohort", shape=SHAPE_MANIFEST, @@ -159,6 +228,7 @@ "course": COURSE, "module": MODULE, "unit": UNIT, + "homework_unit": HOMEWORK_UNIT, "cohort": COHORT, "homework": HOMEWORK, }, diff --git a/community_base/content_sync/kinds/layouts.py b/community_base/content_sync/kinds/layouts.py index 2c9c6850..bd15790c 100644 --- a/community_base/content_sync/kinds/layouts.py +++ b/community_base/content_sync/kinds/layouts.py @@ -225,7 +225,7 @@ class CourseLayout(Layout): named. """ - def __init__(self, max_module_levels: int = 2) -> None: + def __init__(self, max_module_levels: int | None = None) -> None: self.max_module_levels = max_module_levels def walk(self, root: DirNode) -> Found: @@ -255,16 +255,44 @@ def walk(self, root: DirNode) -> Found: continue if name == CODE_DIR or is_asset_dir(child): continue + if child.has(HOMEWORK_MANIFEST) and not child.has(MODULE_MANIFEST): + problems.append( + ( + child.path, + Problem( + "", + "3.5", + "a homework unit must be inside a module directory", + ), + ) + ) + continue + if not child.has(MODULE_MANIFEST) and not child.has(HOMEWORK_MANIFEST): + # Directories without course manifests are supporting source + # material (for example scripts, solutions or assets), not + # curriculum nodes. A module is declared by module.yaml. + continue self._walk_module(child, root.path, course_path, 1, items, problems) return items, problems def _walk_module(self, node, container, parent, level, items, problems) -> None: + if node.has(MODULE_MANIFEST) and node.has(HOMEWORK_MANIFEST): + problems.append( + ( + node.path, + Problem("", "3.5", "module.yaml and homework.yaml cannot share a directory"), + ) + ) + return if not node.has(MODULE_MANIFEST): problems.append( - (node.path, Problem("", "3.5", f"a module directory needs {MODULE_MANIFEST}")) + ( + node.path, + Problem("", "3.5", f"a curriculum directory needs {MODULE_MANIFEST}"), + ) ) return - if level > self.max_module_levels: + if self.max_module_levels is not None and level > self.max_module_levels: problems.append( ( node.path, @@ -284,23 +312,25 @@ def _walk_module(self, node, container, parent, level, items, problems) -> None: parent=parent, ) ) - units = [name for name in node.files if name.endswith(".md") and name != README] - submodules = [ - child - for child in node.dirs - if _base_name(child.path) != CODE_DIR and not is_asset_dir(child) - ] - if units and submodules: + if node.has(HOMEWORK_MANIFEST): problems.append( ( - node.path, + node.joined(HOMEWORK_MANIFEST), Problem( "", "3.5", - "a module directory holds either submodule directories or unit files", + "a homework unit is a sibling directory with homework.yaml and homework.md", ), ) ) + units = [name for name in node.files if name.endswith(".md") and name != README] + submodules = [ + child + for child in node.dirs + if _base_name(child.path) != CODE_DIR + and not is_asset_dir(child) + and (child.has(MODULE_MANIFEST) or child.has(HOMEWORK_MANIFEST)) + ] for name in units: items.append( RawItem( @@ -312,8 +342,66 @@ def _walk_module(self, node, container, parent, level, items, problems) -> None: ) ) for child in submodules: + if child.has(MODULE_MANIFEST) and child.has(HOMEWORK_MANIFEST): + problems.append( + ( + child.path, + Problem( + "", "3.5", "module.yaml and homework.yaml cannot share a directory" + ), + ) + ) + continue + if child.has(HOMEWORK_MANIFEST): + self._walk_homework_unit(child, node.path, module_path, items, problems) + continue self._walk_module(child, node.path, module_path, level + 1, items, problems) + def _walk_homework_unit(self, node, container, parent, items, problems) -> None: + """Recognize one YAML-backed homework unit and its prose companion.""" + + companion = node.has("homework.md") + other_markdown = [ + name for name in node.files if name.endswith(".md") and name != "homework.md" + ] + extra_content_dirs = [ + child + for child in node.dirs + if _base_name(child.path) != CODE_DIR and not is_asset_dir(child) + ] + if not companion: + problems.append( + ( + node.path, + Problem("", "3.5", "a homework unit needs exactly one homework.md companion"), + ) + ) + if other_markdown or extra_content_dirs: + offenders = [node.joined(name) for name in other_markdown] + [ + child.path for child in extra_content_dirs + ] + problems.append( + ( + offenders[0], + Problem( + "", + "3.5", + "a homework unit has one homework.md companion and no nested content items", + ), + ) + ) + if companion and not other_markdown and not extra_content_dirs: + items.append( + RawItem( + part="homework_unit", + path=node.joined(HOMEWORK_MANIFEST), + container=container, + name=_base_name(node.path), + parent=parent, + body_path=node.joined("homework.md"), + ) + ) + def _walk_cohorts(self, node, items, problems) -> None: for name in node.files: if name != README and not is_asset_name(name): diff --git a/community_base/content_sync/resolution.py b/community_base/content_sync/resolution.py index 18bf0c09..f0f1b80d 100644 --- a/community_base/content_sync/resolution.py +++ b/community_base/content_sync/resolution.py @@ -393,7 +393,7 @@ def replace(match: re.Match[str]) -> str: line = self._line(item, destination) if _is_external(destination): for problem in check_asset_reference(destination, "/body", line): - self.diagnostics.append(locate(item.raw.path, problem)) + self.diagnostics.append(locate(item.raw.body_path or item.raw.path, problem)) return tag asset = self._asset(item, destination, "/body", line) if asset is None: @@ -517,7 +517,7 @@ def _relative( target, _, fragment = destination.partition("#") if not target: return None - resolved = _resolve_relative(item.raw.path, target) + resolved = _resolve_relative(item.raw.body_path or item.raw.path, target) if resolved is None: self._report( item, "/body", "3.6", f"reference leaves the repository: {destination}", line @@ -596,7 +596,7 @@ def _repository_path(self, item: ParsedDocument, destination: str) -> str | None target = target.split("?")[0] if not target: return None - resolved = _resolve_relative(item.raw.path, target) + resolved = _resolve_relative(item.raw.body_path or item.raw.path, target) if not resolved: return None return resolved if (self.repository.root / resolved).exists() else None @@ -626,13 +626,15 @@ def _asset( ) -> ResolvedAsset | None: problems = check_asset_reference(reference, pointer, line) if problems: + source_path = item.raw.body_path if pointer == "/body" else item.raw.path for problem in problems: - self.diagnostics.append(locate(item.raw.path, problem)) + self.diagnostics.append(locate(source_path or item.raw.path, problem)) return None if reference.startswith("https://"): return None target = reference.split("#")[0].split("?")[0] - resolved = _resolve_relative(item.raw.path, target) + source_path = item.raw.body_path if pointer == "/body" else item.raw.path + resolved = _resolve_relative(source_path or item.raw.path, target) if resolved is None: self._report(item, pointer, "3.6", f"asset leaves the repository: {target}", line) return None @@ -716,7 +718,14 @@ def _report( severity: str = SEVERITY_ERROR, ) -> None: self.diagnostics.append( - Diagnostic(item.raw.path, pointer, rule, message, severity=severity, line=line) + Diagnostic( + item.raw.body_path if pointer == "/body" and item.raw.body_path else item.raw.path, + pointer, + rule, + message, + severity=severity, + line=line, + ) ) def _line(self, item: ParsedDocument, destination: str) -> int | None: diff --git a/community_base/coursework/importing.py b/community_base/coursework/importing.py index 9cf3a6ae..ebf10144 100644 --- a/community_base/coursework/importing.py +++ b/community_base/coursework/importing.py @@ -40,6 +40,7 @@ write_values, ) from community_base.curriculum.models import Cohort, Course, Module, Unit +from community_base.curriculum.source import ModuleGraph, UnitGraph QUESTION_TYPES = { "multiple_choice": QuestionTypes.MULTIPLE_CHOICE.value, @@ -92,6 +93,133 @@ def apply_homework_graphs( return counts +def apply_course_tree_homework_units( + course: Course, module_graphs: Iterable[ModuleGraph], *, commit: str, checkout +) -> dict: + """Apply structured course-tree homework data to assignments already bound to its units. + + The course tree owns question/form content, while each cohort still owns its + assignment row and submissions. This adapter updates only assignments that + already point at the persisted homework unit; it never creates an unbound + assignment or removes questions (which would cascade-delete learner answers). + Cohort manifests continue to use :func:`apply_homework_graphs` and its + envelope-only answer contract. + """ + + counts = {"created": 0, "updated": 0, "unchanged": 0, "deleted": 0} + with transaction.atomic(): + for unit_graph in _homework_units(module_graphs): + unit = _persisted_unit(course, unit_graph) + if unit is None: + continue + module = _top_level_module(unit.module) + assignments = Homework.objects.filter(cohort__course=course, unit=unit).select_related( + "cohort" + ) + for homework in assignments: + values = { + "module": module, + "due_date": unit_graph.homework_unit.due_at, + **_course_tree_form_values(unit_graph.homework_unit.form), + "final_fields": [ + { + "key": field.key, + "label": field.label, + "type": field.type, + "required": field.required, + } + for field in unit_graph.homework_unit.final_fields + ], + # The cohort manifest, when present, owns the assignment's + # identity/provenance; tree data updates its bound content. + **provenance( + homework.source_path, + homework.source_commit_sha, + homework.source_checksum, + ), + } + counts[write_values(homework, values)] += 1 + for question_graph in unit_graph.homework_unit.questions: + question = _course_tree_question(homework, question_graph) + values = _course_tree_question_values( + question_graph, unit_graph, commit, checkout + ) + counts[write_values(question, values)] += 1 + return counts + + +def _homework_units(module_graphs: Iterable[ModuleGraph]): + for module_graph in module_graphs: + for item in module_graph.items: + if isinstance(item, UnitGraph): + if item.homework_unit is not None: + yield item + elif isinstance(item, ModuleGraph): + yield from _homework_units((item,)) + + +def _persisted_unit(course: Course, graph: UnitGraph) -> Unit | None: + units = Unit.objects.filter(module__course=course) + if graph.content_id: + unit = units.filter(source_content_id=graph.content_id).first() + if unit is not None: + return unit + return units.filter(source_path=graph.source_path).first() + + +def _top_level_module(module: Module) -> Module: + while module.parent_id is not None: + module = module.parent + return module + + +def _course_tree_form_values(form) -> dict: + values = {} + for name, field in FORM_FIELDS.items(): + declared = getattr(form, name) + if declared is None: + declared = Homework._meta.get_field(field).default + values[field] = declared + return values + + +def _course_tree_question(homework: Homework, graph) -> Question: + # The authored slug is the durable identity across the source migration; + # UUIDs are also carried for shared source provenance and new rows. + question = Question.objects.filter( + homework=homework, source_question_id=graph.stable_id + ).first() + if question is None: + question = Question.objects.filter( + homework=homework, source_content_id=graph.content_id + ).first() + if question is None: + question = Question(homework=homework) + question.source_content_id = graph.content_id + return question + + +def _course_tree_question_values(graph, unit_graph: UnitGraph, commit, checkout) -> dict: + return { + "source_content_id": graph.content_id, + "source_question_id": graph.stable_id, + "text": graph.prompt, + "step_label": graph.step_label, + "question_type": QUESTION_TYPES[graph.type], + "answer_type": ANSWER_TYPES[graph.answer_type], + "possible_answers": "\n".join(option.label for option in graph.options) or None, + "source_option_ids": [option.id for option in graph.options] or None, + "correct_answer": graph.correct, + "answer_envelope": None, + "scores_for_correct_answer": graph.points, + **provenance( + unit_graph.source_path, + commit, + file_checksum(checkout, unit_graph.source_path), + ), + } + + def _apply(course: Course, graph: HomeworkGraph, commit, checkout, counts: dict) -> Homework: cohort = Cohort.objects.filter(course=course, slug=graph.cohort_slug).first() if cohort is None: # pragma: no cover -- the parser applied the cohort first diff --git a/community_base/coursework/migrations/0004_homework_final_fields_question_step_label.py b/community_base/coursework/migrations/0004_homework_final_fields_question_step_label.py new file mode 100644 index 00000000..845f8e45 --- /dev/null +++ b/community_base/coursework/migrations/0004_homework_final_fields_question_step_label.py @@ -0,0 +1,23 @@ +# Generated by Django 6.0.8 on 2026-09-26 20:20 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('cb_coursework', '0003_homework_module_homework_unit'), + ] + + operations = [ + migrations.AddField( + model_name='homework', + name='final_fields', + field=models.JSONField(blank=True, default=list), + ), + migrations.AddField( + model_name='question', + name='step_label', + field=models.TextField(blank=True, default=''), + ), + ] diff --git a/community_base/coursework/models.py b/community_base/coursework/models.py index 3ab3bc9e..c27f93cc 100644 --- a/community_base/coursework/models.py +++ b/community_base/coursework/models.py @@ -66,6 +66,7 @@ class Homework(SourceProvenanceMixin, models.Model): due_date = models.DateTimeField() learning_in_public_cap = models.IntegerField(default=7) + final_fields = models.JSONField(default=list, blank=True) homework_url_field = models.BooleanField(default=True) time_spent_lectures_field = models.BooleanField(default=True) @@ -122,6 +123,7 @@ class AnswerTypes(Enum): class Question(SourceProvenanceMixin, models.Model): homework = models.ForeignKey(Homework, on_delete=models.CASCADE, related_name="questions") text = models.TextField() + step_label = models.TextField(blank=True, default="") question_type = models.CharField(max_length=2, choices=QUESTION_TYPES) answer_type = models.CharField( # noqa: DJ001 -- null means the answer type is unset. max_length=3, choices=ANSWER_TYPES, blank=True, null=True diff --git a/community_base/curriculum/README.md b/community_base/curriculum/README.md index 94b49f36..0de12b6f 100644 --- a/community_base/curriculum/README.md +++ b/community_base/curriculum/README.md @@ -28,7 +28,7 @@ uv run python manage.py migrate |---|---| | `Course` | Reusable course; tags, testimonials, links, access levels, provenance. | | `Cohort` | One delivery of a course: `mode="cohort"` (dated) or `mode="self_paced"` (one per course). | -| `Module` | Ordered module, owned by the course (shared across every cohort). `parent` makes it a submodule of another module -- maximum two module levels. A module holds either child modules or direct units, never both. `is_bonus` and `available_after_days` (a drip offset for a top-level module, cascading to its units unless they override it) round it out. | +| `Module` | Ordered module, owned by the course (shared across every cohort). `parent` makes it a submodule; physical directories define an arbitrarily deep tree. Direct units and child modules may be mixed and share one unique sibling order. `is_bonus` and `available_after_days` (a drip offset cascading through descendants unless overridden) round it out. | | `Unit` | Lesson, owned by its module. `kind` is `lesson` (default), `homework`, `event` or `checklist_item`; `event` units carry `session_position` (1-indexed, not a foreign key -- the site resolves the real event per viewer, against the viewer's own cohort, at render time) instead of embedding cohort-specific data in shared curriculum. `is_bonus` excludes a unit from the progress denominator while it is still tracked and displayed. | | `CohortModule` | Optional per-cohort placement of a top-level module: `cohort`, `module`, `sort_order`. A cohort with no placements shows the course's full module tree in module order -- the common case, requiring zero extra rows. A cohort with placements shows exactly that curated subset and order instead, for courses whose cohorts genuinely differ (two cohorts of the same course each placing a different module that represents an alternative treatment of one topic, for example). | | `Enrollment` | User-cohort enrollment with soft-delete history. | @@ -40,13 +40,10 @@ uv run python manage.py migrate `.syllabus_modules` (placements, or the course's default tree); `Cohort.effective_modules()` computes the same thing for one cohort. Progress (`Course.total_units()`, `Course.completed_units()`) and the depth-first reading order -(`services.get_all_units_ordered()`, reused by `get_next_unit`/`get_prev_unit`) walk this same -shape: for each top-level module, in `sort_order`, either its own units (a module with no -children) or each child module's units in order (a module with children) -- never both, since a -module never mixes children and direct units. `is_bonus` (on the unit, its module, or that -module's parent module) excludes a unit from the progress denominator; `kind="event"` units -still count; `kind="checklist_item"` units never count, whether or not they are marked -`is_bonus`. +(`services.get_all_units_ordered()`, reused by `get_next_unit`/`get_prev_unit`) walk the recursive +module tree and each module's mixed sibling sequence. `is_bonus` (on the unit or any ancestor +module) excludes a unit from the progress denominator; `kind="event"` units still count; +`kind="checklist_item"` units never count, whether or not they are marked `is_bonus`. ## Pre-work checklists @@ -101,7 +98,9 @@ between them (issue C7.10 names both files). /NN-/module.yaml /NN-/README.md module overview, optional /NN-/NN-.md -/NN-/NN-/module.yaml optional second module level +/NN-/NN-child-module/module.yaml recursive module directory +/NN-/NN-homework/homework.yaml structured homework unit +/NN-/NN-homework/homework.md required unit prose companion /cohorts//cohort.yaml /cohorts//README.md cohort notice, optional /cohorts//homework//homework.yaml @@ -124,8 +123,8 @@ diagnostic, not by a second rule written in the parser. | Graph | From | |---|---| | `CourseGraph` | `course.yaml` plus the core keys; `image` becomes `cover_image_url`, `repository_url` becomes `github_repo_url`, `status` drives `visible`. | -| `ModuleGraph` | one `module.yaml` per module directory, its `README.md` as `overview`, `sort_order` from the `NN-` prefix, recursive through `children` to at most two module levels. | -| `UnitGraph` | one `NN-.md` per unit: `kind`, `video_url`, `timestamps`, `session_position`, `is_bonus`, `code`, with the markdown body unrendered. | +| `ModuleGraph` | one `module.yaml` per module directory, its `README.md` as `overview`, with mixed `items` containing direct units and child modules in source order at any depth. | +| `UnitGraph` | one `NN-.md` or `NN-/homework.yaml` plus `homework.md`; the prose remains unrendered and is stored on the matching Unit field. YAML homework carries due date, form/final fields and ordered questions. | | `CohortGraph` | one `cohorts//cohort.yaml` per cohort; `delivery` becomes `mode`, `modules` becomes `module_refs`, `homework` becomes `homework_bindings`. | Cohort placement follows the contract `CohortModule` already has: `module_refs is None` means @@ -151,9 +150,33 @@ Three parser rulings, where section 3.8 is silent: One importer applies the graph: source-managed rows are created, updated or removed to match the repository, a course that vanishes is soft-deleted to `draft`, and every import records a -`CurriculumImportRun`. Re-importing unchanged content is a no-op. `source.validate_module_tree` -rejects a mixed module (children and direct units) or a tree deeper than two module levels, -naming the offending directory. +`CurriculumImportRun`. Re-importing unchanged content is a no-op. Each module's direct units and +child modules use one order sequence; missing or repeated sibling orders fail before import. +Moving a source unit between modules matches it course-wide by `content_id` and reparents its +existing row, preserving progress and coursework foreign keys. + +### Tree parser and projection API + +`parse_course_tree(source, path=".", ignore=())` parses only a course's physical module/unit tree. +It can read a legacy repository root marked by `course.yaml`; site adapters may use `ignore` for +their existing repository-owned exclusions. The returned `CourseTreeGraph.modules` contains +recursive `ModuleGraph.items` tuples, each an ordered union of `ModuleGraph` and `UnitGraph`. +Project references resolve their source-relative `module_path` to a stable module content ID. + +For an existing shared `Course`, `apply_curriculum_tree(course, tree, *, commit, checkout)` upserts +only module and unit rows. It also updates package Coursework assignments already linked to a YAML +homework Unit by default; `sync_homework=False` leaves coursework to the host adapter. This does +not create cohort assignments. It maps the YAML question's stable `id` before its content UUID so +an identity migration updates a `Question` in place and preserves `Answer` and `Submission` +references. Course-tree `correct` values go to `Question.correct_answer`; cohort homework manifests +remain envelope-only. Correct answers are not part of learner curriculum projections. + +`services.get_curriculum_tree(course, modules=None)` returns an ordered tuple of frozen +`ModuleProjection` and `UnitProjection` values. A module projection exposes its ORM row, recursive +mixed-order `items`, source `path`, `depth`/`level`, direct and descendant unit counts, and +descendant module count. `.all_units` returns descendant units in reading order. A unit projection +exposes the ORM unit, full path, module path and depth. Neither projection includes coursework +questions or scoring answers. Run imports with the content sync command: diff --git a/community_base/curriculum/content_sync_parsers.py b/community_base/curriculum/content_sync_parsers.py index 3c01edba..6b8ca155 100644 --- a/community_base/curriculum/content_sync_parsers.py +++ b/community_base/curriculum/content_sync_parsers.py @@ -80,6 +80,8 @@ def upsert(self, item, source, media): course, counts = apply_curriculum_graph(parsed, source, self._checkout) for action, count in self._apply_homework(item, parsed, course).items(): counts[action] = counts.get(action, 0) + count + for action, count in self._apply_course_tree_homework(parsed, course).items(): + counts[action] = counts.get(action, 0) + count if counts["created"]: action = "created" elif counts["updated"]: @@ -170,6 +172,25 @@ def _apply_homework(self, item, parsed, course) -> dict: checkout=self._checkout, ) + def _apply_course_tree_homework(self, parsed, course) -> dict: + """Update already-bound assignments from structured homework units. + + This is a separate source boundary from cohort homework manifests: + course-tree questions may carry the legacy `correct` field, while the + cohort manifest importer remains envelope-only. + """ + + if not apps.is_installed("community_base.coursework"): + return {} + from community_base.coursework.importing import apply_course_tree_homework_units + + return apply_course_tree_homework_units( + course, + parsed.course.modules, + commit=graph_commit(parsed), + checkout=self._checkout, + ) + def _parse(self, item): return parse_course( self._result, diff --git a/community_base/curriculum/importing.py b/community_base/curriculum/importing.py index 1492eaaf..b3241294 100644 --- a/community_base/curriculum/importing.py +++ b/community_base/curriculum/importing.py @@ -15,6 +15,7 @@ import hashlib import re +from django.apps import apps from django.db import transaction from django.utils import timezone @@ -27,7 +28,14 @@ Module, Unit, ) -from community_base.curriculum.source import CurriculumParseError, InstructorGraph, ParsedCurriculum +from community_base.curriculum.source import ( + CourseTreeGraph, + CurriculumParseError, + InstructorGraph, + ModuleGraph, + ParsedCurriculum, + UnitGraph, +) ACTION_CREATED = "created" ACTION_UPDATED = "updated" @@ -116,6 +124,61 @@ def apply_curriculum_graph(parsed: ParsedCurriculum, source, checkout) -> tuple[ return course, counts +def apply_curriculum_tree( + course: Course, + tree: CourseTreeGraph, + *, + commit: str, + checkout, + sync_homework: bool = True, +) -> dict: + """Upsert the tree for an already-owned shared course. + + This is the adapter seam for a site that still owns its course header, + cohorts, access and routes. Source unit IDs are looked up across the whole + course before a row is reparented, so a physical move retains the Unit + primary key and every learner/coursework foreign key to it. When the + coursework app is installed, structured homework data updates existing + assignments bound to those units unless ``sync_homework`` is disabled. + """ + + counts = {"created": 0, "updated": 0, "unchanged": 0, "deleted": 0} + seen_modules: set[str] = set() + seen_units: set[str] = set() + with transaction.atomic(): + _apply_module_tree( + course, + tree.modules, + parent=None, + commit=commit, + checkout=checkout, + counts=counts, + seen=seen_modules, + seen_units=seen_units, + top_level_by_ref={}, + ) + if sync_homework and apps.is_installed("community_base.coursework"): + from community_base.coursework.importing import apply_course_tree_homework_units + + homework_counts = apply_course_tree_homework_units( + course, + tree.modules, + commit=commit, + checkout=checkout, + ) + for action, count in homework_counts.items(): + counts[action] += count + counts["deleted"] += delete_stale( + Unit.objects.filter(module__course=course).exclude(source_content_id__isnull=True), + seen_units, + ) + counts["deleted"] += delete_stale( + Module.objects.filter(course=course).exclude(source_content_id__isnull=True), + seen_modules, + ) + return counts + + def _manifest_checksum(parsed) -> str: canonical = repr(sorted(_canonical(_graph_summary(parsed.course)).items())) return hashlib.sha256(canonical.encode("utf-8")).hexdigest() @@ -161,6 +224,7 @@ def _apply(parsed, source, checkout, commit) -> tuple[Course, dict]: # resolve their placements by the same identifier the source graph uses # (content_id, or slug when the source has none). seen_module_ids: set[str] = set() + seen_unit_ids: set[str] = set() top_level_by_ref: dict[str, Module] = {} _apply_module_tree( course, @@ -170,8 +234,13 @@ def _apply(parsed, source, checkout, commit) -> tuple[Course, dict]: checkout=checkout, counts=counts, seen=seen_module_ids, + seen_units=seen_unit_ids, top_level_by_ref=top_level_by_ref, ) + counts["deleted"] += delete_stale( + Unit.objects.filter(module__course=course).exclude(source_content_id__isnull=True), + seen_unit_ids, + ) counts["deleted"] += delete_stale( Module.objects.filter(course=course).exclude(source_content_id__isnull=True), seen_module_ids, @@ -199,38 +268,36 @@ def _apply_module_tree( checkout, counts: dict, seen: set, + seen_units: set, top_level_by_ref: dict, depth: int = 0, ) -> None: - for position, module_graph in enumerate(module_graphs): + for module_graph in module_graphs: module = _module(course, parent, module_graph) seen.add(module_graph.content_id) - values = _module_values(module_graph, position, commit, checkout) + values = _module_values(module_graph, parent, commit, checkout) counts[write_values(module, values)] += 1 if depth == 0: top_level_by_ref[module_graph.content_id or module_graph.slug] = module - seen_unit_ids: set = set() - for unit_graph in module_graph.units: - unit = _unit(module, unit_graph) - seen_unit_ids.add(unit_graph.content_id) - counts[write_values(unit, _unit_values(unit_graph, commit, checkout))] += 1 - counts["deleted"] += delete_stale( - Unit.objects.filter(module=module).exclude(source_content_id__isnull=True), - seen_unit_ids, - ) - - _apply_module_tree( - course, - module_graph.children, - parent=module, - commit=commit, - checkout=checkout, - counts=counts, - seen=seen, - top_level_by_ref=top_level_by_ref, - depth=depth + 1, - ) + for item in module_graph.items: + if isinstance(item, UnitGraph): + unit = _unit(course, module, item) + seen_units.add(item.content_id) + counts[write_values(unit, _unit_values(item, module, commit, checkout))] += 1 + elif isinstance(item, ModuleGraph): + _apply_module_tree( + course, + (item,), + parent=module, + commit=commit, + checkout=checkout, + counts=counts, + seen=seen, + seen_units=seen_units, + top_level_by_ref=top_level_by_ref, + depth=depth + 1, + ) def _apply_placements(cohort: Cohort, cohort_graph, top_level_by_ref: dict) -> int: @@ -298,21 +365,23 @@ def _module(course: Course, parent: Module | None, graph) -> Module: if module is None: module = Module.objects.filter(course=course, parent=parent, slug=graph.slug).first() if module is None: - module = Module(course=course, slug=graph.slug) - module.parent = parent - module.source_content_id = graph.content_id + module = Module(course=course) return module -def _unit(module: Module, graph) -> Unit: +def _unit(course: Course, module: Module, graph: UnitGraph) -> Unit: unit = None if graph.content_id: - unit = Unit.objects.filter(module=module, source_content_id=graph.content_id).first() + # Content IDs belong to the course tree, not to a module. Look up across + # the whole course so moving a physical source file reuses its row and + # preserves learner progress and coursework foreign keys. + unit = Unit.objects.filter( + module__course=course, source_content_id=graph.content_id + ).first() if unit is None: unit = Unit.objects.filter(module=module, slug=graph.slug).first() if unit is None: - unit = Unit(module=module, slug=graph.slug) - unit.source_content_id = graph.content_id + unit = Unit() return unit @@ -349,11 +418,13 @@ def _cohort_values(graph, commit, checkout) -> dict: } -def _module_values(graph, position, commit, checkout) -> dict: - sort_order = graph.sort_order or position +def _module_values(graph, parent, commit, checkout) -> dict: return { + "parent": parent, + "slug": graph.slug, + "source_content_id": graph.content_id, "title": graph.title, - "sort_order": sort_order, + "sort_order": graph.sort_order, "syllabus_section": graph.syllabus_section, "overview": graph.overview, "is_bonus": graph.is_bonus, @@ -362,8 +433,11 @@ def _module_values(graph, position, commit, checkout) -> dict: } -def _unit_values(graph, commit, checkout) -> dict: +def _unit_values(graph, module, commit, checkout) -> dict: return { + "module": module, + "slug": graph.slug, + "source_content_id": graph.content_id, "title": graph.title, "sort_order": graph.sort_order, "kind": graph.kind, @@ -375,7 +449,9 @@ def _unit_values(graph, commit, checkout) -> dict: "timestamps": list(graph.timestamps), "is_preview": graph.is_preview, "required_level": graph.required_level, - "content_hash": _body_hash(graph), + "content_hash": ( + graph.content_hash if graph.content_hash is not None else _body_hash(graph) + ), **provenance(graph.source_path, commit, file_checksum(checkout, graph.source_path)), } @@ -403,13 +479,13 @@ def write_values(instance, values) -> str: was_new = instance.pk is None content_keys = [key for key in values if key not in _PROVENANCE_KEYS] if not was_new: - changed = any( - _canonical(getattr(instance, key)) != _canonical(values[key]) for key in content_keys - ) + changed = any(not _same_field_value(instance, key, values[key]) for key in content_keys) if not changed: + for key, value in values.items(): + setattr(instance, key, value) for key in _PROVENANCE_KEYS: setattr(instance, key, values[key]) - fields = [*_PROVENANCE_KEYS, "source_content_id"] + fields = list(dict.fromkeys([*values, *_PROVENANCE_KEYS])) instance.save(update_fields=fields) return ACTION_UNCHANGED for key, value in values.items(): @@ -421,6 +497,17 @@ def write_values(instance, values) -> str: _PROVENANCE_KEYS = ("source_path", "source_commit_sha", "source_checksum") +def _same_field_value(instance, key, value) -> bool: + if key == "source_content_id": + old = getattr(instance, key) + return (str(old) if old is not None else None) == ( + str(value) if value is not None else None + ) + if key in {"module", "parent"}: + return getattr(instance, f"{key}_id") == getattr(value, "pk", None) + return _canonical(getattr(instance, key)) == _canonical(value) + + def delete_stale(queryset, seen_ids: set) -> int: seen = {str(value) for value in seen_ids if value is not None} stale = [ diff --git a/community_base/curriculum/migrations/0005_alter_module_parent.py b/community_base/curriculum/migrations/0005_alter_module_parent.py new file mode 100644 index 00000000..e0746f28 --- /dev/null +++ b/community_base/curriculum/migrations/0005_alter_module_parent.py @@ -0,0 +1,19 @@ +# Generated by Django 6.0.8 on 2026-09-26 20:30 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('cb_curriculum', '0004_module_syllabus_section'), + ] + + operations = [ + migrations.AlterField( + model_name='module', + name='parent', + field=models.ForeignKey(blank=True, help_text='Set to make this module a child of another module.', null=True, on_delete=django.db.models.deletion.CASCADE, related_name='children', to='cb_curriculum.module'), + ), + ] diff --git a/community_base/curriculum/models.py b/community_base/curriculum/models.py index dae5e7fe..af2ff2d7 100644 --- a/community_base/curriculum/models.py +++ b/community_base/curriculum/models.py @@ -167,24 +167,35 @@ def primary_instructor(self): def _countable_units(self): """Units that count toward the progress denominator. - Every unit counts, including ``kind=event`` units, except a unit (or its module, or - that module's parent module) marked ``is_bonus`` -- tracked and displayed, but + Every unit counts, including ``kind=event`` units, except a unit or any + ancestor module marked ``is_bonus`` -- tracked and displayed, but excluded from the denominator (owner decision, community-base#252). ``kind=checklist_item`` units are excluded outright: a pre-work checklist is a separate readiness track, not lesson/homework/event course progress, regardless of whether an individual item is marked required (``is_bonus=False``) or optional (``is_bonus=True``). """ - return ( + module_rows = list( + Module.objects.filter(course=self).values_list("pk", "parent_id", "is_bonus") + ) + bonus_module_ids = {pk for pk, _parent_id, is_bonus in module_rows if is_bonus} + while True: + inherited = { + pk for pk, parent_id, _is_bonus in module_rows if parent_id in bonus_module_ids + } + added = inherited - bonus_module_ids + if not added: + break + bonus_module_ids.update(added) + return list( Unit.objects.filter(module__course=self) .exclude(kind=UNIT_KIND_CHECKLIST_ITEM) .exclude(is_bonus=True) - .exclude(module__is_bonus=True) - .exclude(module__parent__is_bonus=True) + .exclude(module_id__in=bonus_module_ids) ) def total_units(self): - return self._countable_units().count() + return len(self._countable_units()) def completed_units(self, user): if user is None or not user.is_authenticated: @@ -200,8 +211,8 @@ def get_syllabus(self): A cohort's effective modules are its :class:`CohortModule` placements when it has any, otherwise the course's full top-level module tree (see - :meth:`Cohort.effective_modules`) -- computed here in two queries total rather than - one query per cohort. + :meth:`Cohort.effective_modules`). The shared persisted tree projection is built + once and sliced to each cohort's selected top-level modules. """ cohorts = list(self.cohorts.order_by("start_date", "pk")) @@ -221,8 +232,15 @@ def get_syllabus(self): ) for placement in placements: placements_by_cohort.setdefault(placement.cohort_id, []).append(placement.module) + from community_base.curriculum.services import get_curriculum_tree + + full_tree = get_curriculum_tree(self) + tree_by_module_id = {node.module.pk: node for node in full_tree} for cohort in cohorts: cohort.syllabus_modules = placements_by_cohort.get(cohort.pk) or default_modules + cohort.syllabus_tree = tuple( + tree_by_module_id[module.pk] for module in cohort.syllabus_modules + ) return cohorts def get_next_unit_for(self, user): @@ -331,10 +349,8 @@ def effective_modules(self): class Module(SourceProvenanceMixin, models.Model): """An ordered module of a course. A submodule is a module with ``parent`` set. - A module holds either child modules or direct units, never both (enforced in - :meth:`clean`, not a database constraint, because the rule spans two related - tables -- ``children`` and ``units`` -- which a ``CheckConstraint`` cannot express). - Nesting is capped at two module levels: a submodule cannot itself have children. + A module's direct units and child modules share the same physical sibling + order, so a node may hold both and modules may nest to any repository depth. """ course = models.ForeignKey(Course, on_delete=models.CASCADE, related_name="modules") @@ -344,7 +360,7 @@ class Module(SourceProvenanceMixin, models.Model): blank=True, on_delete=models.CASCADE, related_name="children", - help_text="Set to make this module a submodule of another. Maximum two levels.", + help_text="Set to make this module a child of another module.", ) slug = models.SlugField(max_length=300, default="") title = models.CharField(max_length=300) @@ -407,33 +423,29 @@ def clean(self): if self.pk is not None and self.parent_id == self.pk: errors["parent"] = "A module cannot be its own parent." elif self.parent is not None: - if self.parent.parent_id is not None: - errors["parent"] = "A submodule cannot itself have children (max two levels)." if self.course_id and self.parent.course_id != self.course_id: errors["parent"] = "A parent module must belong to the same course." - if self.parent.units.exists(): - errors["parent"] = ( - f"Module {self.parent.title!r} already has direct units; " - "it cannot also have child modules." - ) - if self.pk is not None: - has_children = self.children.exists() - has_units = self.units.exists() - if has_children and has_units: - errors["parent"] = ( - f"Module {self.title!r} has both child modules and direct units; " - "it must have only one." - ) + ancestor = self.parent + seen_ancestors = set() + while ancestor is not None and ancestor.pk not in seen_ancestors: + if self.pk is not None and ancestor.pk == self.pk: + errors["parent"] = "A module cannot be nested beneath itself." + break + seen_ancestors.add(ancestor.pk) + ancestor = ancestor.parent if errors: raise ValidationError(errors) @property def effective_is_bonus(self) -> bool: - """Bonus cascades from an ancestor: a bonus week's submodules are bonus too.""" + """Bonus cascades from any ancestor, however deep the module is.""" - if self.is_bonus: - return True - return bool(self.parent_id and self.parent.is_bonus) + module = self + while module is not None: + if module.is_bonus: + return True + module = module.parent if module.parent_id else None + return False class CohortModule(models.Model): @@ -584,18 +596,6 @@ def set_rendered_html(self, rendered_html: str) -> None: self.body_html_source = BODY_HTML_SITE self.body_html = rendered_html - def clean(self): - super().clean() - if self.module_id and self.module.children.exists(): - raise ValidationError( - { - "module": ( - f"Module {self.module.title!r} has child modules; " - "it cannot also have direct units." - ) - } - ) - @property def course(self): return self.module.course @@ -621,15 +621,15 @@ def effective_is_bonus(self) -> bool: @property def effective_available_after_days(self): - """Resolve the drip offset: unit override, its module's, then its parent module's.""" + """Resolve the nearest drip offset: unit override then module ancestors.""" if self.available_after_days is not None: return self.available_after_days module = self.module - if module.available_after_days is not None: - return module.available_after_days - if module.parent_id: - return module.parent.available_after_days + while module is not None: + if module.available_after_days is not None: + return module.available_after_days + module = module.parent if module.parent_id else None return None diff --git a/community_base/curriculum/parsers.py b/community_base/curriculum/parsers.py index d9e2eeb9..cdb51696 100644 --- a/community_base/curriculum/parsers.py +++ b/community_base/curriculum/parsers.py @@ -36,7 +36,9 @@ from __future__ import annotations import datetime as dt +import hashlib from collections.abc import Iterable, Mapping +from pathlib import Path, PurePosixPath from typing import Any from community_base.content_sync.documents import ( @@ -44,9 +46,14 @@ Diagnostic, ParsedDocument, ReadResult, + _check_identity, + _node_at, + _read_collection, + _read_repository, read_repository, ) -from community_base.content_sync.kinds.base import resolve_level +from community_base.content_sync.kinds import DirNode, get_kind +from community_base.content_sync.kinds.base import resolve_level, split_order_prefix from community_base.curriculum.code_annotations import ( CodeAnnotationError, validate_annotated_body, @@ -57,16 +64,23 @@ MODE_SELF_PACED, CohortGraph, CourseGraph, + CourseTreeGraph, CurriculumParseError, + HomeworkFinalFieldGraph, + HomeworkFormGraph, + HomeworkOptionGraph, + HomeworkQuestionGraph, + HomeworkUnitGraph, InstructorGraph, ModuleGraph, ParsedCurriculum, + ProjectGraph, UnitGraph, validate_module_tree, ) #: One parser, one version. It names the format, not a site. -PARSER_VERSION = "course-format-1" +PARSER_VERSION = "course-format-2" #: The `content.yaml` version this parser reads (section 3.1). SCHEMA_VERSION = 1 @@ -110,6 +124,131 @@ def parse_course_repository(source: Any, *, path: str | None = None) -> ParsedCu return parse_course(result, collections[0], commit_sha=getattr(source, "commit_sha", None)) +def parse_course_tree( + source: Any, + *, + path: str = ".", + ignore: Iterable[str] = (), + project_specs: Iterable[Mapping[str, Any]] = (), +) -> CourseTreeGraph: + """Parse one physical module/unit tree without taking ownership of its header. + + This is the adapter seam for existing course repositories whose root + ``course.yaml`` also contains site-owned policy. The file must exist as the + course root marker, but its contents and any ``cohorts/`` directory are + deliberately outside this call. All module and unit sources remain under + the normal course schemas and validators. ``source`` may be an immutable + package checkout, a read-only checkout view exposing ``checkout`` and + ``relative``, or a directory; ``path`` is relative to that repository. + + ``project_specs`` lets an adapter pass project fields it owns through the + generic source-relative module resolver without passing unrelated header + metadata into the package parser. + + ``ignore`` is an explicit adapter boundary for legacy source headers that + owned ignore globs before ``content.yaml``. Patterns are relative to the + selected course root and hide files before any module/unit parsing. + """ + + result, collection, course_manifest_path = _read_course_tree(source, path, ignore) + by_parent: dict[str | None, list[ParsedDocument]] = {} + for document in result.documents: + by_parent.setdefault(document.raw.parent, []).append(document) + modules = _module_graphs( + result, + by_parent, + parent=course_manifest_path, + inherited=None, + ) + validate_module_tree(modules, where=collection.path or ".") + projects = _project_graphs(project_specs, modules, course_manifest_path) + return CourseTreeGraph( + parser_version=PARSER_VERSION, + schema_version=SCHEMA_VERSION, + source_path=course_manifest_path, + modules=modules, + projects=projects, + ) + + +def _read_course_tree( + source: Any, path: str, ignore: Iterable[str] +) -> tuple[ReadResult, Collection, str]: + """Read the course layout under ``path`` without a repository manifest.""" + + if isinstance(source, str | Path): + checkout = None + root = Path(source) + else: + checkout = getattr(source, "checkout", source) + root_value = getattr(checkout, "root", None) + if root_value is None: + raise CurriculumParseError("course tree source must expose a checkout root") + root = Path(root_value) + if not root.is_dir(): + raise CurriculumParseError(f"{root}: course tree source is not a directory") + collection_path = _course_tree_relative_path(source, path) + content_checkout = checkout if checkout is not None and hasattr(checkout, "read_text") else None + patterns = tuple( + f"{collection_path}/{pattern.lstrip('/')}" if collection_path else pattern + for pattern in ignore + ) + repository = _read_repository(root, patterns, content_checkout) + node = _node_at(repository.tree, collection_path) + if node is None: + raise CurriculumParseError( + f"{collection_path or '.'}: no course tree directory at that source-relative path" + ) + course_manifest_path = f"{collection_path}/course.yaml" if collection_path else "course.yaml" + if "course.yaml" not in node.files: + raise CurriculumParseError(f"{collection_path or '.'}: a course tree needs course.yaml") + + # The tree-only seam intentionally excludes cohort-owned metadata and + # bindings. It reuses the registered layout and all module/unit parsers. + tree_root = DirNode( + path=node.path, + files=node.files, + dirs=tuple(child for child in node.dirs if child.path.rsplit("/", 1)[-1] != "cohorts"), + ) + collection = Collection(kind=get_kind(COURSE_KIND), path=collection_path, index=0) + diagnostics: list[Diagnostic] = [] + documents = _read_collection( + repository, + collection, + diagnostics, + collection_root=tree_root, + ) + # `course.yaml` is just the physical root marker at this boundary. Its + # fields may be a legacy site's schema, so neither its parse diagnostics + # nor its derived record participate in the generic tree graph. + documents = [item for item in documents if item.raw.path != course_manifest_path] + diagnostics = [item for item in diagnostics if item.path != course_manifest_path] + _check_identity(documents, diagnostics) + errors = [item for item in diagnostics if item.severity == "error"] + if errors: + raise CurriculumParseError("\n".join(item.render() for item in errors)) + result = ReadResult( + root=root, + documents=tuple(documents), + diagnostics=tuple(sorted(diagnostics, key=lambda item: item.sort_key)), + repository=repository, + ) + return result, collection, course_manifest_path + + +def _course_tree_relative_path(source: Any, path: str) -> str: + """Normalize a checkout-relative path and honor read-only checkout views.""" + + if path in ("", "."): + return "" + if hasattr(source, "relative"): + return source.relative(path) + candidate = PurePosixPath(str(path).replace("\\", "/")) + if candidate.is_absolute() or any(part in {"", ".", ".."} for part in candidate.parts): + raise CurriculumParseError(f"invalid course tree path {path!r}") + return candidate.as_posix() + + def parse_course( result: ReadResult, collection: Collection, *, commit_sha: str | None = None ) -> ParsedCurriculum: @@ -178,6 +317,7 @@ def _course_graph( status = values.get("status") or PUBLISHED title = document.title cohorts = _cohort_graphs(documents, modules, course_title=title) + projects = _project_graphs(values.get("projects") or (), modules, document.raw.path) return CourseGraph( content_id=document.content_id, slug=document.slug, @@ -197,6 +337,7 @@ def _course_graph( hashtag=values.get("hashtag") or "", visible=status == PUBLISHED, instructors=_instructors(result, values.get("instructors") or ()), + projects=projects, modules=modules, cohorts=cohorts, ) @@ -244,8 +385,20 @@ def _module_graph( level = resolve_level(document.data.get("required_level")) if level is None: level = inherited - units = [item for item in by_parent.get(document.raw.path, ()) if item.part.name == "unit"] - units.sort(key=lambda item: item.sort_key) + siblings = [ + item + for item in by_parent.get(document.raw.path, ()) + if item.part.name in {"module", "unit", "homework_unit"} + ] + siblings.sort(key=lambda item: item.sort_key) + items = [] + for sibling in siblings: + if sibling.part.name == "module": + items.append(_module_graph(result, by_parent, sibling, level)) + elif sibling.part.name == "homework_unit": + items.append(_homework_unit_graph(sibling, level)) + else: + items.append(_unit_graph(sibling, level)) return ModuleGraph( content_id=document.content_id, slug=document.slug, @@ -254,13 +407,23 @@ def _module_graph( overview=_overview(result, document), syllabus_section=values.get("syllabus_section") or "", sort_order=document.sort_order, - is_bonus=bool(values.get("is_bonus")), + is_bonus=_module_bonus(document), available_after_days=values.get("available_after_days"), - units=tuple(_unit_graph(item, level) for item in units), - children=_module_graphs(result, by_parent, parent=document.raw.path, inherited=level), + has_order=_has_order(document), + items=tuple(items), ) +def _module_bonus(document: ParsedDocument) -> bool: + """Map the source's ``bonus`` spelling onto the shared ``is_bonus`` flag.""" + + if "is_bonus" in document.data and "bonus" in document.data: + raise CurriculumParseError( + f"{document.raw.path}: declare either is_bonus or bonus, not both" + ) + return bool(document.values.get("is_bonus") or document.values.get("bonus")) + + def _overview(result: ReadResult, document: ParsedDocument) -> str: """A module's `README.md`, the one place section 3.2 reads one for a course.""" @@ -282,6 +445,7 @@ def _unit_graph(document: ParsedDocument, inherited: int | None) -> UnitGraph: title=document.title, source_path=document.raw.path, body=body, + content_hash=_unit_content_hash(body), video_url=values.get("video_url") or "", timestamps=tuple(values.get("timestamps") or ()), required_level=_declared_level(document, inherited), @@ -289,9 +453,159 @@ def _unit_graph(document: ParsedDocument, inherited: int | None) -> UnitGraph: kind=values.get("kind") or "lesson", session_position=values.get("session_position"), is_bonus=bool(values.get("is_bonus")), + available_after_days=values.get("available_after_days"), + body_source_path=document.raw.body_path, + has_order=_has_order(document), + ) + + +def _homework_unit_graph(document: ParsedDocument, inherited: int | None) -> UnitGraph: + values = document.values + homework = HomeworkUnitGraph( + due_at=_datetime(values.get("due_at"), document.raw.path, "/due_at"), + form=_homework_form(values.get("form") or {}), + final_fields=tuple( + HomeworkFinalFieldGraph( + key=entry["key"], + label=entry["label"], + type=entry.get("type") or "text", + required=bool(entry.get("required")), + ) + for entry in values.get("final_fields") or () + ), + questions=tuple( + HomeworkQuestionGraph( + content_id=entry["content_id"], + stable_id=entry["id"], + type=entry["type"], + prompt=entry["prompt"], + points=entry.get("points", 1), + options=tuple( + HomeworkOptionGraph(id=option["id"], label=option["label"]) + for option in entry.get("options") or () + ), + answer_type=entry.get("answer_type"), + step_label=entry.get("step_label") or "", + correct=entry.get("correct"), + ) + for entry in values.get("questions") or () + ), + ) + return UnitGraph( + content_id=document.content_id, + slug=document.slug, + title=document.title, + source_path=document.raw.path, + homework=document.body, + content_hash=_unit_content_hash(document.body), + required_level=_declared_level(document, inherited), + sort_order=document.sort_order, + kind="homework", + is_bonus=bool(values.get("is_bonus")), + available_after_days=values.get("available_after_days"), + body_source_path=document.raw.body_path, + has_order=_has_order(document), + homework_unit=homework, ) +def _unit_content_hash(body: str) -> str: + return hashlib.md5(body.encode("utf-8")).hexdigest() if body else "" + + +def _homework_form(values: Mapping[str, Any]) -> HomeworkFormGraph: + return HomeworkFormGraph( + homework_url=values.get("homework_url"), + time_spent_lectures=values.get("time_spent_lectures"), + time_spent_homework=values.get("time_spent_homework"), + faq_contribution=values.get("faq_contribution"), + learning_in_public_cap=values.get("learning_in_public_cap"), + ) + + +def _has_order(document: ParsedDocument) -> bool: + declared = document.data.get("sort_order") + if isinstance(declared, int) and not isinstance(declared, bool): + return True + prefix, _ = split_order_prefix(document.raw.name) + return prefix is not None + + +def _datetime(value: Any, path: str, pointer: str) -> dt.datetime: + if isinstance(value, dt.datetime): + parsed = value + elif isinstance(value, str): + try: + parsed = dt.datetime.fromisoformat(value) + except ValueError: + raise CurriculumParseError(f"{path}:{pointer}: expected an ISO datetime") from None + else: + raise CurriculumParseError(f"{path}:{pointer}: expected an ISO datetime") + if parsed.tzinfo is None or parsed.utcoffset() is None: + raise CurriculumParseError(f"{path}:{pointer}: datetime needs a UTC offset") + return parsed + + +def _project_graphs( + entries: Iterable[Mapping[str, Any]], modules, path: str +) -> tuple[ProjectGraph, ...]: + by_path: dict[str, list[ModuleGraph]] = {} + + def walk(siblings, prefix=()): + for module in siblings: + module_path = (*prefix, module.slug) + by_path.setdefault("/".join(module_path), []).append(module) + walk(module.children, module_path) + + walk(modules) + found: list[ProjectGraph] = [] + seen: set[str] = set() + for index, entry in enumerate(entries): + project_slug = entry["slug"] + pointer = f"{path}:/projects/{index}" + if project_slug in seen: + raise CurriculumParseError(f"{pointer}/slug: duplicate project slug {project_slug!r}") + seen.add(project_slug) + module_path = entry["module_path"] + components = module_path.split("/") + if any(not component for component in components): + raise CurriculumParseError( + f"{pointer}/module_path: invalid module path {module_path!r}" + ) + matches = by_path.get(module_path, []) + if not matches: + raise CurriculumParseError( + f"{pointer}/module_path: no module at source path {module_path!r}" + ) + if len(matches) != 1: + raise CurriculumParseError( + f"{pointer}/module_path: ambiguous module path {module_path!r}" + ) + submission_due_at = _datetime( + entry["submission_due_at"], path, f"/projects/{index}/submission_due_at" + ) + review_due_at = _datetime(entry["review_due_at"], path, f"/projects/{index}/review_due_at") + if review_due_at <= submission_due_at: + raise CurriculumParseError(f"{pointer}/review_due_at: must follow submission_due_at") + peer_review_count = entry.get("peer_review_count") + if peer_review_count is not None and not 1 <= peer_review_count <= 10: + raise CurriculumParseError(f"{pointer}/peer_review_count: must be between 1 and 10") + module = matches[0] + found.append( + ProjectGraph( + slug=project_slug, + title=entry["title"], + module_path=module_path, + module_content_id=module.content_id, + submission_due_at=submission_due_at, + review_due_at=review_due_at, + cohort_key=entry.get("cohort_key"), + peer_review_count=peer_review_count, + ) + ) + return tuple(found) + + def _declared_level(document: ParsedDocument, inherited: int | None) -> int | None: """The unit's own level, else the one its module ancestors declared. diff --git a/community_base/curriculum/services.py b/community_base/curriculum/services.py index e051db04..927ca72c 100644 --- a/community_base/curriculum/services.py +++ b/community_base/curriculum/services.py @@ -1,9 +1,10 @@ """Domain services for enrollment, progress and cohort drip scheduling.""" +from __future__ import annotations + import datetime from dataclasses import dataclass -from django.db.models import Prefetch from django.utils import timezone from community_base.curriculum.models import ( @@ -125,6 +126,136 @@ def completed_unit_ids(user, units) -> set[int]: ) +@dataclass(frozen=True, slots=True) +class UnitProjection: + """A persisted unit at its source-derived position in the physical tree.""" + + unit: Unit + path: str + module_path: str + depth: int + kind: str = "unit" + + +@dataclass(frozen=True, slots=True) +class ModuleProjection: + """A persisted module with mixed, ordered module/unit child projections. + + ``descendant_unit_count`` includes direct units as well as units below child + modules. ``descendant_module_count`` counts child modules, not this module. + Child module items are recursively projected ModuleProjection objects. + """ + + module: Module + items: tuple[ModuleProjection | UnitProjection, ...] + path: str + depth: int + direct_unit_count: int + descendant_unit_count: int + descendant_module_count: int + kind: str = "module" + + @property + def level(self) -> int: + """The human-facing level, with top-level modules at level one.""" + + return self.depth + 1 + + @property + def all_units(self) -> tuple[UnitProjection, ...]: + """Return every unit below this module in reading order.""" + + return tuple( + nested + for item in self.items + for nested in (item.all_units if isinstance(item, ModuleProjection) else (item,)) + ) + + +def get_curriculum_tree( + course: Course, + modules=None, +) -> tuple[ModuleProjection, ...]: + """Project a course's physical module tree with every mixed level preserved. + + Pass ``modules`` to project a cohort's already ordered top-level selection; + child modules and direct units remain in the shared source order. This + projection contains learner-facing curriculum fields only and never carries + homework scoring metadata. + """ + + all_modules = list(Module.objects.filter(course=course).order_by("sort_order", "pk")) + all_units = list( + Unit.objects.filter(module__course=course) + .select_related("module") + .order_by("sort_order", "pk") + ) + modules_by_parent: dict[int | None, list[Module]] = {} + modules_by_id: dict[int, Module] = {} + for module in all_modules: + modules_by_parent.setdefault(module.parent_id, []).append(module) + modules_by_id[module.pk] = module + units_by_module: dict[int, list[Unit]] = {} + for unit in all_units: + units_by_module.setdefault(unit.module_id, []).append(unit) + + def item_key(item): + # Unit and Module tables have independent primary-key sequences. The + # type tag closes the rare tie while normal source siblings use one + # unique sort_order across both types. + return (item.sort_order, item.pk, 0 if isinstance(item, Unit) else 1) + + def project(module: Module, parent_path: str, depth: int) -> ModuleProjection: + path = f"{parent_path}/{module.slug}" if parent_path else module.slug + direct_units = units_by_module.get(module.pk, []) + child_modules = modules_by_parent.get(module.pk, []) + projected_children = {child.pk: project(child, path, depth + 1) for child in child_modules} + physical_items = [*direct_units, *child_modules] + physical_items.sort(key=item_key) + items: list[ModuleProjection | UnitProjection] = [] + descendant_unit_count = 0 + descendant_module_count = len(child_modules) + for item in physical_items: + if isinstance(item, Unit): + items.append( + UnitProjection( + unit=item, + path=f"{path}/{item.slug}", + module_path=path, + depth=depth, + ) + ) + descendant_unit_count += 1 + else: + child = projected_children[item.pk] + items.append(child) + descendant_unit_count += child.descendant_unit_count + descendant_module_count += child.descendant_module_count + return ModuleProjection( + module=module, + items=tuple(items), + path=path, + depth=depth, + direct_unit_count=len(direct_units), + descendant_unit_count=descendant_unit_count, + descendant_module_count=descendant_module_count, + ) + + selected_modules = list(modules) if modules is not None else modules_by_parent.get(None, []) + + def parent_context(module: Module) -> tuple[str, int]: + ancestors = [] + parent_id = module.parent_id + while parent_id is not None: + parent = modules_by_id[parent_id] + ancestors.append(parent.slug) + parent_id = parent.parent_id + ancestors.reverse() + return "/".join(ancestors), len(ancestors) + + return tuple(project(module, *parent_context(module)) for module in selected_modules) + + @dataclass(frozen=True) class DripDecision: """Drip-schedule decision after tier/unit access has been granted.""" @@ -164,39 +295,9 @@ def decide_unit_drip( def get_all_units_ordered(course: Course) -> list[Unit]: - """Return every unit of the course in depth-first reading order. - - For each top-level module, in ``sort_order``: if it has submodules, each submodule's - units in order; otherwise the module's own units directly -- a module holds either - children or units, never both (community-base#252). ``id`` is an explicit tiebreaker - after ``sort_order``, which is not unique. - """ + """Return every unit in depth-first, mixed sibling source order.""" - top_modules = list( - Module.objects.filter(course=course, parent__isnull=True) - .prefetch_related( - Prefetch("children", queryset=Module.objects.order_by("sort_order", "pk")), - Prefetch("units", queryset=Unit.objects.order_by("sort_order", "pk")), - ) - .order_by("sort_order", "pk") - ) - child_ids = [child.pk for module in top_modules for child in module.children.all()] - units_by_module: dict[int, list[Unit]] = {} - if child_ids: - for unit in Unit.objects.filter(module_id__in=child_ids).order_by( - "module_id", "sort_order", "pk" - ): - units_by_module.setdefault(unit.module_id, []).append(unit) - - ordered: list[Unit] = [] - for module in top_modules: - children = list(module.children.all()) - if children: - for child in children: - ordered.extend(units_by_module.get(child.pk, [])) - else: - ordered.extend(module.units.all()) - return ordered + return [item.unit for module in get_curriculum_tree(course) for item in module.all_units] def get_next_unit(course: Course, current_unit: Unit): diff --git a/community_base/curriculum/source.py b/community_base/curriculum/source.py index 00959129..362684d3 100644 --- a/community_base/curriculum/source.py +++ b/community_base/curriculum/source.py @@ -9,7 +9,7 @@ from collections.abc import Mapping from dataclasses import dataclass, field -from datetime import date +from datetime import date, datetime from typing import Any MODE_COHORT = "cohort" @@ -49,11 +49,60 @@ class UnitGraph: kind: str = "lesson" session_position: int | None = None is_bonus: bool = False + available_after_days: int | None = None + body_source_path: str | None = None + content_hash: str | None = None + has_order: bool = False + homework_unit: HomeworkUnitGraph | None = None + + +@dataclass(frozen=True, slots=True) +class HomeworkOptionGraph: + id: str + label: str + + +@dataclass(frozen=True, slots=True) +class HomeworkQuestionGraph: + content_id: str + stable_id: str + type: str + prompt: str + points: int + options: tuple[HomeworkOptionGraph, ...] = field(default=()) + answer_type: str | None = None + step_label: str = "" + correct: str | None = None + + +@dataclass(frozen=True, slots=True) +class HomeworkFormGraph: + homework_url: bool | None = None + time_spent_lectures: bool | None = None + time_spent_homework: bool | None = None + faq_contribution: bool | None = None + learning_in_public_cap: int | None = None + + +@dataclass(frozen=True, slots=True) +class HomeworkFinalFieldGraph: + key: str + label: str + type: str = "text" + required: bool = False + + +@dataclass(frozen=True, slots=True) +class HomeworkUnitGraph: + due_at: datetime + form: HomeworkFormGraph = field(default_factory=HomeworkFormGraph) + questions: tuple[HomeworkQuestionGraph, ...] = field(default=()) + final_fields: tuple[HomeworkFinalFieldGraph, ...] = field(default=()) @dataclass(frozen=True, slots=True) class ModuleGraph: - """One module, course-owned. Either ``children`` or ``units`` is non-empty, never both.""" + """One physical module and its ordered, mixed child sequence.""" content_id: str | None slug: str @@ -64,8 +113,48 @@ class ModuleGraph: sort_order: int = 0 is_bonus: bool = False available_after_days: int | None = None - units: tuple[UnitGraph, ...] = field(default=()) - children: tuple[ModuleGraph, ...] = field(default=()) + has_order: bool = False + items: tuple[ModuleGraph | UnitGraph, ...] = field(default=()) + + @property + def units(self) -> tuple[UnitGraph, ...]: + """Direct units in sibling order, retained for older read-only consumers.""" + + return tuple(item for item in self.items if isinstance(item, UnitGraph)) + + @property + def children(self) -> tuple[ModuleGraph, ...]: + """Child modules in sibling order, retained for older read-only consumers.""" + + return tuple(item for item in self.items if isinstance(item, ModuleGraph)) + + +@dataclass(frozen=True, slots=True) +class ProjectGraph: + slug: str + title: str + module_path: str + module_content_id: str | None + submission_due_at: datetime + review_due_at: datetime + cohort_key: str | None = None + peer_review_count: int | None = None + + +@dataclass(frozen=True, slots=True) +class CourseTreeGraph: + """Physical course content without course-, cohort- or site-owned metadata. + + Site adapters for legacy course repositories use this graph to parse the + checked-in module/unit tree while retaining ownership of their existing + course headers and cohort policy. + """ + + parser_version: str + schema_version: int + source_path: str + modules: tuple[ModuleGraph, ...] = field(default=()) + projects: tuple[ProjectGraph, ...] = field(default=()) @dataclass(frozen=True, slots=True) @@ -115,9 +204,9 @@ class CourseGraph: hashtag: str = "" visible: bool = True instructors: tuple[InstructorGraph, ...] = field(default=()) - # The one module tree, owned by the course. Top-level modules in display - # order; each may carry ``children`` (submodules, max depth two) or - # ``units`` directly, never both. + projects: tuple[ProjectGraph, ...] = field(default=()) + # The one module tree, owned by the course. Top-level modules and every + # module's `items` preserve the physical mixed sibling order. modules: tuple[ModuleGraph, ...] = field(default=()) cohorts: tuple[CohortGraph, ...] = field(default=()) @@ -134,34 +223,100 @@ class CurriculumParseError(ValueError): """A repository layout does not satisfy the curriculum source contract.""" -def validate_module_tree(modules: tuple[ModuleGraph, ...], *, where: str, depth: int = 1) -> None: +def validate_module_tree(modules: tuple[ModuleGraph, ...], *, where: str) -> None: """Validate a course's module tree once, for the one course parser. - Enforces: a module has either ``children`` or ``units``, never both, naming the - offending directory (``where``); nesting does not exceed two module levels; sibling - module slugs (and sibling unit slugs) are unique within their own parent, not globally - -- so two submodules under different parents may each contain a unit slugged the same. + Enforces unique sibling slugs and orders across the mixed module/unit sequence, + and explicit ordering for every sibling at every physical level. """ seen_module_slugs: set[str] = set() + seen_module_orders: set[int] = set() for module in modules: if module.slug in seen_module_slugs: raise CurriculumParseError(f"{where}: duplicate module slug {module.slug!r}") seen_module_slugs.add(module.slug) module_where = f"{module.source_path or where}" - if module.children and module.units: + if not module.has_order: + raise CurriculumParseError( + f"{module_where}: missing sibling order; declare sort_order or use an NN- prefix" + ) + if module.sort_order in seen_module_orders: raise CurriculumParseError( - f"{module_where}: has both child modules and direct units; " - "a module must have only one" + f"{module_where}: duplicate sibling order {module.sort_order}" ) - if module.children: - if depth >= 2: + seen_module_orders.add(module.sort_order) + seen_sibling_slugs: set[str] = set() + seen_orders: set[int] = set() + for sibling in module.items: + sibling_where = sibling.source_path or module_where + if sibling.slug in seen_sibling_slugs: + raise CurriculumParseError( + f"{sibling_where}: duplicate sibling slug {sibling.slug!r}" + ) + seen_sibling_slugs.add(sibling.slug) + if not sibling.has_order: + raise CurriculumParseError( + f"{sibling_where}: missing sibling order; declare sort_order or use an " + "NN- prefix" + ) + if sibling.sort_order in seen_orders: + raise CurriculumParseError( + f"{sibling_where}: duplicate sibling order {sibling.sort_order}" + ) + seen_orders.add(sibling.sort_order) + if isinstance(sibling, UnitGraph) and sibling.homework_unit: + validate_homework_unit(sibling.homework_unit, where=sibling_where) + children = module.children + if children: + validate_module_tree(children, where=module_where) + + +def validate_homework_unit(homework: HomeworkUnitGraph, *, where: str) -> None: + """Validate question identities, choice shapes and legacy scoring values.""" + + seen_question_ids: set[str] = set() + seen_content_ids: set[str] = set() + for index, question in enumerate(homework.questions): + pointer = f"{where}:/questions/{index}" + if question.stable_id in seen_question_ids: + raise CurriculumParseError( + f"{pointer}/id: duplicate question id {question.stable_id!r}" + ) + if question.content_id in seen_content_ids: + raise CurriculumParseError( + f"{pointer}/content_id: duplicate question content_id {question.content_id!r}" + ) + seen_question_ids.add(question.stable_id) + seen_content_ids.add(question.content_id) + option_ids = [option.id for option in question.options] + if len(option_ids) != len(set(option_ids)): + raise CurriculumParseError(f"{pointer}/options: duplicate option id") + choice = question.type in {"multiple_choice", "checkboxes"} + if choice and not question.options: + raise CurriculumParseError(f"{pointer}/options: choice questions need ordered options") + if not choice and question.options: + raise CurriculumParseError( + f"{pointer}/options: free-form questions do not take options" + ) + if question.correct is not None and choice: + try: + indexes = [int(item.strip()) for item in question.correct.split(",")] + except (TypeError, ValueError): + raise CurriculumParseError( + f"{pointer}/correct: choice answers use 1-based option indexes" + ) from None + if ( + not indexes + or any( + index_value < 1 or index_value > len(question.options) + for index_value in indexes + ) + or len(set(indexes)) != len(indexes) + or (question.type == "multiple_choice" and len(indexes) != 1) + ): raise CurriculumParseError( - f"{module_where}: exceeds the maximum module depth of two levels" + f"{pointer}/correct: answer index must identify an authored option" ) - validate_module_tree(module.children, where=module_where, depth=depth + 1) - seen_unit_slugs: set[str] = set() - for unit in module.units: - if unit.slug in seen_unit_slugs: - raise CurriculumParseError(f"{module_where}: duplicate unit slug {unit.slug!r}") - seen_unit_slugs.add(unit.slug) + if question.points < 0: + raise CurriculumParseError(f"{pointer}/points: must be zero or a positive integer") diff --git a/community_base/curriculum/templates/curriculum/_syllabus_item.html b/community_base/curriculum/templates/curriculum/_syllabus_item.html new file mode 100644 index 00000000..81cab60b --- /dev/null +++ b/community_base/curriculum/templates/curriculum/_syllabus_item.html @@ -0,0 +1,32 @@ +
  • + {% if item.kind == "module" %} +

    + {% if item.depth %} + {{ item.module.title }} + {% else %} + {{ item.module.title }} + {% endif %} +

    + {% if item.items %} +
      + {% for child in item.items %} + {% include "curriculum/_syllabus_item.html" with item=child course=course cohort=cohort has_access=has_access completed_unit_ids=completed_unit_ids %} + {% endfor %} +
    + {% endif %} + {% else %} + {% with unit=item.unit %} + {% if has_access or unit.is_preview %} + {% if item.depth %} + {{ unit.title }} + {% else %} + {{ unit.title }} + {% endif %} + {% else %} + {{ unit.title }} + {% endif %} + {% if unit.is_preview %}Preview{% endif %} + {% if unit.pk in completed_unit_ids %}Completed{% endif %} + {% endwith %} + {% endif %} +
    • diff --git a/community_base/curriculum/templates/curriculum/course_detail.html b/community_base/curriculum/templates/curriculum/course_detail.html index fee8597c..59beee2b 100644 --- a/community_base/curriculum/templates/curriculum/course_detail.html +++ b/community_base/curriculum/templates/curriculum/course_detail.html @@ -23,7 +23,13 @@

    {{ course.title }}

    {% csrf_token %} - {% if next_unit and default_cohort %}Continue{% endif %} + {% if next_unit and default_cohort %} + {% if next_unit.module_depth %} + Continue + {% else %} + Continue + {% endif %} + {% endif %} {% elif has_access %}
    {% csrf_token %} @@ -55,19 +61,9 @@

    {{ cohort.title }}

    {% elif cohort.registration_url %} Register {% endif %} - {% for module in cohort.syllabus_modules %} -

    {{ module.title }}

    + {% for module in cohort.syllabus_tree %}
      - {% for unit in module.units.all %} -
    • - {% if has_access or unit.is_preview %} - {{ unit.title }} - {% else %} - {{ unit.title }} - {% endif %} - {% if unit.is_preview %}Preview{% endif %} -
      • - {% endfor %} + {% include "curriculum/_syllabus_item.html" with item=module course=course cohort=cohort has_access=has_access %}
    {% endfor %} diff --git a/community_base/curriculum/templates/curriculum/module_overview.html b/community_base/curriculum/templates/curriculum/module_overview.html index 2fdd3966..2d888acf 100644 --- a/community_base/curriculum/templates/curriculum/module_overview.html +++ b/community_base/curriculum/templates/curriculum/module_overview.html @@ -10,15 +10,8 @@

    {{ module.title }}

    {% if module.overview_html %}
    {{ module.overview_html|safe }}
    {% endif %}
      - {% for unit in units %} -
    1. - {% if has_access or unit.is_preview %} - {{ unit.title }} - {% else %} - {{ unit.title }} - {% endif %} - {% if unit.pk in completed_unit_ids %}Completed{% endif %} -
      2. + {% for item in module_projection.items %} + {% include "curriculum/_syllabus_item.html" with item=item course=course cohort=cohort has_access=has_access completed_unit_ids=completed_unit_ids %} {% empty %}
    2. No lessons in this module yet.
      3. {% endfor %} diff --git a/community_base/curriculum/templates/curriculum/unit_detail.html b/community_base/curriculum/templates/curriculum/unit_detail.html index 42c811e9..ea24d499 100644 --- a/community_base/curriculum/templates/curriculum/unit_detail.html +++ b/community_base/curriculum/templates/curriculum/unit_detail.html @@ -6,7 +6,11 @@

      {{ course.title }} · - {{ module.title }} + {% if module_depth %} + {{ module.title }} + {% else %} + {{ module.title }} + {% endif %}

      {{ unit.title }}

      @@ -57,15 +61,28 @@

      {{ homework.title }}

      {% endif %}