feat: add note_type, status, parent_note_id, position columns (#60) #61

Merged

forgejo_admin merged 2 commits from 60-feat-add-note-type-status-parent-note-id into main 2026-03-02 08:33:12 +00:00

Conversation 4 Commits 2 Files changed 5 +1096 -5

forgejo_admin commented 2026-03-02 06:34:34 +00:00

Owner

Copy link

Summary

Implements Phase 2 of the Note Decomposition plan (plan-2026-03-01-note-decomposition). Adds four new columns to the notes table that enable parent-child note relationships, structured note typing, and per-type status tracking. This is the schema foundation for decomposing monolithic plan notes into composable, individually-queryable phase records.

Changes

Alembic Migration (d4e5f6a7b8c9)

• note_type — VARCHAR, nullable
• status — VARCHAR, nullable
• parent_note_id — INTEGER, nullable, FK to notes.id, ondelete=RESTRICT
• position — INTEGER, nullable
• Composite index on (parent_note_id, position)
• No data backfill — all columns nullable, existing notes stay NULL

SQLAlchemy Model (models.py)

• Four new columns on Note
• Self-referential relationship: parent / children with back_populates
• children ordered by Note.position, with passive_deletes=True to let DB enforce RESTRICT
• parent_slug property for Pydantic serialization

Pydantic Schemas (schemas.py)

• NoteType Literal with 11 values: plan, phase, sop, convention, issue, todo, template, project-page, skill, agent, doc
• NoteCreate and NoteUpdate gain note_type, status, parent_slug, position
• NoteOut and NoteSummary gain same fields in response

Route Validation (routes/notes.py)

• VALID_STATUSES dict — maps each note_type to its valid statuses (~30 lines)
• ALLOWED_PARENT_TYPES dict — {"phase": ["plan"]} (extensible)
• _validate_status() — rejects status without note_type, rejects invalid status for type
• _resolve_parent() — resolves parent_slug to parent_note_id, validates parent-type map, self-parent check
• list_notes — new query params: note_type, status, parent_slug
• create_note — accepts and validates all new fields
• update_note — same validation, supports status-only updates without rewriting html_content

Tests (test_note_decomposition.py)

• 30 new tests covering:
  • Valid/invalid note_type (Pydantic Literal enforcement)
  • Valid/invalid status per note_type
  • Status without note_type rejected (422)
  • Phase with plan parent succeeds
  • Phase with non-plan parent rejected (422)
  • Non-phase with parent rejected (422)
  • Parent not found (404)
  • Self-parent check on update (422)
  • Status-only update (no new revision created)
  • list_notes filtering by note_type, status, parent_slug
  • Combined filters
  • Position ordering
  • Response shape (NoteOut and NoteSummary include new fields)
  • RESTRICT delete behavior (parent with children cannot be deleted)
  • Backward compatibility (notes without decomposition fields work as before)

Test Plan

• All 30 new decomposition tests pass
• All 199 existing tests pass unchanged (including auth, browse, sanitize, autolink, wrap_tables, mobile responsive)
• ruff check passes
• ruff format --check passes
• QA agent review

Review Checklist

• Migration is safe: all columns nullable, no data backfill, no breaking changes
• RESTRICT FK prevents accidental deletion of parent notes with children
• Validation covers all edge cases from the plan spec
• Backward compatible: existing notes and API calls work unchanged
• Index on (parent_note_id, position) matches the core composition query pattern

Related Notes

• plan-2026-03-01-note-decomposition — Phase 2
• note-conventions — the spec defining note_type enum and status-per-type
• doc-pal-e-docs-schema — ERD reference
• Forgejo issue #60

## Summary Implements Phase 2 of the Note Decomposition plan (`plan-2026-03-01-note-decomposition`). Adds four new columns to the `notes` table that enable parent-child note relationships, structured note typing, and per-type status tracking. This is the schema foundation for decomposing monolithic plan notes into composable, individually-queryable phase records. ## Changes ### Alembic Migration (`d4e5f6a7b8c9`) - `note_type` — VARCHAR, nullable - `status` — VARCHAR, nullable - `parent_note_id` — INTEGER, nullable, FK to `notes.id`, ondelete=RESTRICT - `position` — INTEGER, nullable - Composite index on `(parent_note_id, position)` - No data backfill — all columns nullable, existing notes stay NULL ### SQLAlchemy Model (`models.py`) - Four new columns on `Note` - Self-referential relationship: `parent` / `children` with `back_populates` - `children` ordered by `Note.position`, with `passive_deletes=True` to let DB enforce RESTRICT - `parent_slug` property for Pydantic serialization ### Pydantic Schemas (`schemas.py`) - `NoteType` Literal with 11 values: plan, phase, sop, convention, issue, todo, template, project-page, skill, agent, doc - `NoteCreate` and `NoteUpdate` gain `note_type`, `status`, `parent_slug`, `position` - `NoteOut` and `NoteSummary` gain same fields in response ### Route Validation (`routes/notes.py`) - `VALID_STATUSES` dict — maps each note_type to its valid statuses (~30 lines) - `ALLOWED_PARENT_TYPES` dict — `{"phase": ["plan"]}` (extensible) - `_validate_status()` — rejects status without note_type, rejects invalid status for type - `_resolve_parent()` — resolves parent_slug to parent_note_id, validates parent-type map, self-parent check - `list_notes` — new query params: `note_type`, `status`, `parent_slug` - `create_note` — accepts and validates all new fields - `update_note` — same validation, supports status-only updates without rewriting html_content ### Tests (`test_note_decomposition.py`) - 30 new tests covering: - Valid/invalid note_type (Pydantic Literal enforcement) - Valid/invalid status per note_type - Status without note_type rejected (422) - Phase with plan parent succeeds - Phase with non-plan parent rejected (422) - Non-phase with parent rejected (422) - Parent not found (404) - Self-parent check on update (422) - Status-only update (no new revision created) - list_notes filtering by note_type, status, parent_slug - Combined filters - Position ordering - Response shape (NoteOut and NoteSummary include new fields) - RESTRICT delete behavior (parent with children cannot be deleted) - Backward compatibility (notes without decomposition fields work as before) ## Test Plan - [x] All 30 new decomposition tests pass - [x] All 199 existing tests pass unchanged (including auth, browse, sanitize, autolink, wrap_tables, mobile responsive) - [x] `ruff check` passes - [x] `ruff format --check` passes - [ ] QA agent review ## Review Checklist - [ ] Migration is safe: all columns nullable, no data backfill, no breaking changes - [ ] RESTRICT FK prevents accidental deletion of parent notes with children - [ ] Validation covers all edge cases from the plan spec - [ ] Backward compatible: existing notes and API calls work unchanged - [ ] Index on (parent_note_id, position) matches the core composition query pattern ## Related Notes - `plan-2026-03-01-note-decomposition` — Phase 2 - `note-conventions` — the spec defining note_type enum and status-per-type - `doc-pal-e-docs-schema` — ERD reference - Forgejo issue #60

forgejo_admin added 1 commit 2026-03-02 06:34:34 +00:00

feat: add note_type, status, parent_note_id, position columns to notes (#60) ... f6e12957f2

Add four new columns to the notes table for note decomposition:
- note_type: VARCHAR, validated via Pydantic Literal (11 values)
- status: VARCHAR, validated per note_type via VALID_STATUSES dict
- parent_note_id: FK to notes.id with ondelete=RESTRICT for parent-child relationships
- position: INTEGER for ordering children within a parent

Includes Alembic migration, SQLAlchemy model update with self-referential
parent/children relationships, Pydantic schema updates (NoteCreate, NoteUpdate,
NoteOut, NoteSummary), route validation (status-per-type, parent-type map,
self-parent check), and list_notes filtering by note_type/status/parent_slug.

30 new tests covering all validation paths. All 199 existing tests pass unchanged.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

forgejo_admin commented 2026-03-02 06:35:32 +00:00

Author

Owner

Copy link

Self-Review (Dev Agent)

Reviewed the full diff. No blocking issues found.

Checklist

• Migration: all 4 columns nullable, no data backfill, composite index, FK with RESTRICT, downgrade reverses correctly
• Model: passive_deletes=True on children relationship ensures DB-level RESTRICT enforcement (not ORM cascade)
• Model: parent_slug property resolves correctly for Pydantic from_attributes serialization
• Schemas: NoteType Literal with 11 values, all 4 fields on Create/Update/Out/Summary
• Routes: _validate_status covers status-without-type, invalid-status-for-type, unknown-type
• Routes: _resolve_parent covers type-not-in-map, parent-not-found, self-parent, parent-type-mismatch
• Routes: effective_note_type in update handles both new-type and existing-type cases
• Routes: list_notes joinedload(Note.parent) for parent_slug resolution + 3 new filters
• Tests: 30 tests covering all validation paths, backward compatibility, RESTRICT behavior
• All 199 tests pass, ruff check clean, ruff format clean

Notes

• The list_notes query adds joinedload(Note.parent) alongside subqueryload(Note.tags). Verified no duplicate rows when combined with project join filter.
• The existing SAWarning about circular FK between projects/notes during teardown is pre-existing (conftest handles it with PRAGMA workaround). Not introduced by this PR.

Ready for QA agent review.

## Self-Review (Dev Agent) Reviewed the full diff. No blocking issues found. ### Checklist - [x] Migration: all 4 columns nullable, no data backfill, composite index, FK with RESTRICT, downgrade reverses correctly - [x] Model: `passive_deletes=True` on children relationship ensures DB-level RESTRICT enforcement (not ORM cascade) - [x] Model: `parent_slug` property resolves correctly for Pydantic `from_attributes` serialization - [x] Schemas: `NoteType` Literal with 11 values, all 4 fields on Create/Update/Out/Summary - [x] Routes: `_validate_status` covers status-without-type, invalid-status-for-type, unknown-type - [x] Routes: `_resolve_parent` covers type-not-in-map, parent-not-found, self-parent, parent-type-mismatch - [x] Routes: `effective_note_type` in update handles both new-type and existing-type cases - [x] Routes: `list_notes` joinedload(Note.parent) for parent_slug resolution + 3 new filters - [x] Tests: 30 tests covering all validation paths, backward compatibility, RESTRICT behavior - [x] All 199 tests pass, ruff check clean, ruff format clean ### Notes - The `list_notes` query adds `joinedload(Note.parent)` alongside `subqueryload(Note.tags)`. Verified no duplicate rows when combined with project join filter. - The existing SAWarning about circular FK between projects/notes during teardown is pre-existing (conftest handles it with PRAGMA workaround). Not introduced by this PR. Ready for QA agent review.

forgejo_admin commented 2026-03-02 08:19:41 +00:00

Author

Owner

Copy link

PR #61 Review

BLOCKERS

None found. The implementation is correct and complete against the spec.

NITS

1. create_note missing eager load of parent relationship (src/pal_e_docs/routes/notes.py, line 222)

   • After db.refresh(note), the parent relationship is resolved via lazy load when Pydantic accesses note.parent_slug. This works within the active session, but both get_note (line 171) and update_note (line 232) explicitly use joinedload(Note.parent).
   • For consistency and robustness (avoiding a potential DetachedInstanceError if session handling ever changes), consider re-querying the note with joinedload(Note.parent) before returning, or adding a line to explicitly access note.parent after refresh to trigger the lazy load.
   • Not blocking because tests prove it works today.

2. Inconsistent 404 vs empty result on invalid filter values (src/pal_e_docs/routes/notes.py, lines 152-162)

   • list_notes returns 404 when parent_slug references a nonexistent note, but silently returns empty results when note_type or status don't match anything. This is a reasonable design choice (parent is a FK lookup, the others are string filters), but worth documenting or being aware of.
   • Not blocking.

3. list_notes does not order by position when filtering by parent_slug (src/pal_e_docs/routes/notes.py, line 164)

   • When parent_slug is specified, callers likely expect children returned in position order. Currently they're returned in updated_at desc order. The test at line 526 acknowledges this explicitly ("list_notes orders by updated_at desc, not position -- that's expected").
   • This is acceptable for Phase 2 since clients can sort client-side, but consider adding order_by(Note.position) when parent_slug is set as a follow-up improvement.
   • Not blocking.

4. No API mechanism to unset parent_slug

   • NoteUpdate.parent_slug is str | None where None means "don't change". There's no way to remove an existing parent (set parent_note_id back to NULL). This is a known limitation of the current update API pattern (same issue exists for other nullable fields like project_slug). Not blocking for Phase 2 -- could be addressed in a follow-up.

5. Test coverage gap: only 3 of 11 note_types have status validation tests

   • Tests validate statuses for plan, phase, and doc explicitly. The other 8 types (sop, convention, issue, todo, template, project-page, skill, agent) rely on the same VALID_STATUSES dict lookup, so the code path is identical. Not blocking because the validation mechanism is tested; the parametric coverage is nice-to-have.

VERIFICATION CHECKLIST

• Migration chain correct: c3d4e5f6a7b8 -> d4e5f6a7b8c9
• All 4 columns nullable, no data backfill -- safe for existing data
• parent_note_id FK ondelete=RESTRICT -- matches spec
• Composite index on (parent_note_id, position) -- matches spec
• Downgrade reverses all operations in correct order
• SQLAlchemy model: 4 new columns, self-referential relationship, parent_slug property
• NoteType Literal has all 11 values matching note-conventions spec exactly
• VALID_STATUSES dict matches note-conventions spec for all 11 types
• ALLOWED_PARENT_TYPES: {"phase": ["plan"]} -- matches spec
• _validate_status: rejects status-without-type (422), rejects invalid status for type (422)
• _resolve_parent: validates parent-type map, self-parent check (422), parent-not-found (404)
• NoteCreate, NoteUpdate, NoteOut, NoteSummary all updated with 4 new fields
• list_notes accepts note_type, status, parent_slug query params
• create_note and update_note accept and validate all new fields
• update_note correctly uses effective_note_type (body value or existing) for validation
• Status-only update does not create new revision -- tested and verified
• RESTRICT delete behavior tested (parent with children returns 409)
• Backward compatibility tested (notes without new fields work as before)
• 30 new tests covering all major validation paths
• No injection vectors (query params are typed, FK lookups by slug not raw SQL)
• No secrets or credentials committed

SOP COMPLIANCE

• Branch named after issue (60-feat-add-note-type-status-parent-note-id references issue #60)
• PR body follows template-pr-body (## Summary, ## Changes, ## Test Plan, ## Review Checklist, ## Related Notes)
• Related Notes references the plan slug (plan-2026-03-01-note-decomposition)
• Tests exist (30 new tests in test_note_decomposition.py)
• No secrets, .env files, or credentials committed
• No unnecessary file changes (5 files: migration, model, schemas, routes, tests -- all in scope)
• Commit messages are descriptive
• Scope matches issue #60 acceptance criteria exactly

VERDICT: APPROVED

Clean implementation. All 12 acceptance criteria from the issue are met. The VALID_STATUSES dict and NoteType Literal match the note-conventions spec exactly. Validation logic is thorough with proper error codes. Tests cover all critical paths. Migration is SQLite-safe. Fully backward compatible.

The 5 nits above are all non-blocking improvements that could be addressed in follow-up work.

## PR #61 Review ### BLOCKERS None found. The implementation is correct and complete against the spec. ### NITS 1. **`create_note` missing eager load of `parent` relationship** (`src/pal_e_docs/routes/notes.py`, line 222) - After `db.refresh(note)`, the `parent` relationship is resolved via lazy load when Pydantic accesses `note.parent_slug`. This works within the active session, but both `get_note` (line 171) and `update_note` (line 232) explicitly use `joinedload(Note.parent)`. - For consistency and robustness (avoiding a potential `DetachedInstanceError` if session handling ever changes), consider re-querying the note with `joinedload(Note.parent)` before returning, or adding a line to explicitly access `note.parent` after refresh to trigger the lazy load. - Not blocking because tests prove it works today. 2. **Inconsistent 404 vs empty result on invalid filter values** (`src/pal_e_docs/routes/notes.py`, lines 152-162) - `list_notes` returns 404 when `parent_slug` references a nonexistent note, but silently returns empty results when `note_type` or `status` don't match anything. This is a reasonable design choice (parent is a FK lookup, the others are string filters), but worth documenting or being aware of. - Not blocking. 3. **`list_notes` does not order by `position` when filtering by `parent_slug`** (`src/pal_e_docs/routes/notes.py`, line 164) - When `parent_slug` is specified, callers likely expect children returned in position order. Currently they're returned in `updated_at desc` order. The test at line 526 acknowledges this explicitly ("list_notes orders by updated_at desc, not position -- that's expected"). - This is acceptable for Phase 2 since clients can sort client-side, but consider adding `order_by(Note.position)` when `parent_slug` is set as a follow-up improvement. - Not blocking. 4. **No API mechanism to unset `parent_slug`** - `NoteUpdate.parent_slug` is `str | None` where `None` means "don't change". There's no way to remove an existing parent (set `parent_note_id` back to NULL). This is a known limitation of the current update API pattern (same issue exists for other nullable fields like `project_slug`). Not blocking for Phase 2 -- could be addressed in a follow-up. 5. **Test coverage gap: only 3 of 11 note_types have status validation tests** - Tests validate statuses for `plan`, `phase`, and `doc` explicitly. The other 8 types (sop, convention, issue, todo, template, project-page, skill, agent) rely on the same `VALID_STATUSES` dict lookup, so the code path is identical. Not blocking because the validation mechanism is tested; the parametric coverage is nice-to-have. ### VERIFICATION CHECKLIST - [x] Migration chain correct: `c3d4e5f6a7b8` -> `d4e5f6a7b8c9` - [x] All 4 columns nullable, no data backfill -- safe for existing data - [x] `parent_note_id` FK ondelete=RESTRICT -- matches spec - [x] Composite index on `(parent_note_id, position)` -- matches spec - [x] Downgrade reverses all operations in correct order - [x] SQLAlchemy model: 4 new columns, self-referential relationship, `parent_slug` property - [x] `NoteType` Literal has all 11 values matching `note-conventions` spec exactly - [x] `VALID_STATUSES` dict matches `note-conventions` spec for all 11 types - [x] `ALLOWED_PARENT_TYPES`: `{"phase": ["plan"]}` -- matches spec - [x] `_validate_status`: rejects status-without-type (422), rejects invalid status for type (422) - [x] `_resolve_parent`: validates parent-type map, self-parent check (422), parent-not-found (404) - [x] `NoteCreate`, `NoteUpdate`, `NoteOut`, `NoteSummary` all updated with 4 new fields - [x] `list_notes` accepts `note_type`, `status`, `parent_slug` query params - [x] `create_note` and `update_note` accept and validate all new fields - [x] `update_note` correctly uses `effective_note_type` (body value or existing) for validation - [x] Status-only update does not create new revision -- tested and verified - [x] RESTRICT delete behavior tested (parent with children returns 409) - [x] Backward compatibility tested (notes without new fields work as before) - [x] 30 new tests covering all major validation paths - [x] No injection vectors (query params are typed, FK lookups by slug not raw SQL) - [x] No secrets or credentials committed ### SOP COMPLIANCE - [x] Branch named after issue (`60-feat-add-note-type-status-parent-note-id` references issue #60) - [x] PR body follows `template-pr-body` (## Summary, ## Changes, ## Test Plan, ## Review Checklist, ## Related Notes) - [x] Related Notes references the plan slug (`plan-2026-03-01-note-decomposition`) - [x] Tests exist (30 new tests in `test_note_decomposition.py`) - [x] No secrets, .env files, or credentials committed - [x] No unnecessary file changes (5 files: migration, model, schemas, routes, tests -- all in scope) - [x] Commit messages are descriptive - [x] Scope matches issue #60 acceptance criteria exactly ### VERDICT: APPROVED Clean implementation. All 12 acceptance criteria from the issue are met. The VALID_STATUSES dict and NoteType Literal match the `note-conventions` spec exactly. Validation logic is thorough with proper error codes. Tests cover all critical paths. Migration is SQLite-safe. Fully backward compatible. The 5 nits above are all non-blocking improvements that could be addressed in follow-up work.

forgejo_admin referenced this pull request from a commit 2026-03-02 08:24:28 +00:00

fix: address 5 QA nits from PR #61 review

forgejo_admin added 1 commit 2026-03-02 08:24:28 +00:00

fix: address 5 QA nits from PR #61 review ... 0d42be6034

1. Eager load parent relationship in create_note and update_note responses
   so parent_slug is always populated (re-query with joinedload instead of
   db.refresh)
2. Return empty list (not 404) when list_notes parent_slug filter references
   a nonexistent slug -- consistent with how tags and other filters work
3. Order by position ASC (nulls last) when list_notes filters by parent_slug,
   so children come back in their defined order
4. Allow unsetting parent via empty string: parent_slug="" sets
   parent_note_id=NULL, enabling removal from parent-child relationships
5. Add parametric tests covering all 11 note_types x their valid statuses
   (30 accepted combos) and one invalid status per type (11 rejected combos)

Test count: 71 decomposition tests, 240 total. All pass.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

forgejo_admin commented 2026-03-02 08:24:36 +00:00

Author

Owner

Copy link

QA Nit Fixes (Round 1)

All 5 non-blocking nits from QA review have been addressed in commit 0d42be6:

Nit	Fix	Test
1. Eager load parent in create/update response	Re-query with joinedload(Note.parent) instead of db.refresh() in both create_note and update_note	test_create_note_response_includes_parent_slug, test_update_note_response_includes_parent_slug
2. 404 vs empty for nonexistent parent_slug	Return [] instead of raising 404, consistent with tags/project filters	test_list_notes_filter_parent_slug_not_found_returns_empty
3. Position ordering for parent_slug filter	.order_by(Note.position.asc().nullslast()) when parent_slug is active	test_children_position_ordering (now asserts strict [1, 2, 3] order)
4. Unset parent via empty string	parent_slug="" sets parent_note_id=None, skips validation	test_unset_parent_via_empty_string
5. Parametric status tests	@pytest.mark.parametrize with 30 valid pairs + 11 invalid pairs across all 11 note_types	test_valid_status_accepted_per_type[*], test_invalid_status_rejected_per_type[*]

Results: 71 decomposition tests, 240 total tests. All pass. Ruff clean.

## QA Nit Fixes (Round 1) All 5 non-blocking nits from QA review have been addressed in commit `0d42be6`: | Nit | Fix | Test | |-----|-----|------| | 1. Eager load parent in create/update response | Re-query with `joinedload(Note.parent)` instead of `db.refresh()` in both `create_note` and `update_note` | `test_create_note_response_includes_parent_slug`, `test_update_note_response_includes_parent_slug` | | 2. 404 vs empty for nonexistent parent_slug | Return `[]` instead of raising 404, consistent with tags/project filters | `test_list_notes_filter_parent_slug_not_found_returns_empty` | | 3. Position ordering for parent_slug filter | `.order_by(Note.position.asc().nullslast())` when parent_slug is active | `test_children_position_ordering` (now asserts strict `[1, 2, 3]` order) | | 4. Unset parent via empty string | `parent_slug=""` sets `parent_note_id=None`, skips validation | `test_unset_parent_via_empty_string` | | 5. Parametric status tests | `@pytest.mark.parametrize` with 30 valid pairs + 11 invalid pairs across all 11 note_types | `test_valid_status_accepted_per_type[*]`, `test_invalid_status_rejected_per_type[*]` | **Results:** 71 decomposition tests, 240 total tests. All pass. Ruff clean.

forgejo_admin commented 2026-03-02 08:31:32 +00:00

Author

Owner

Copy link

PR #61 Review (Round 2)

BLOCKERS

None.

NITS

1. Changing note_type does not re-validate existing status (data integrity gap)

In update_note (routes/notes.py lines 270-280), if a caller sends {"note_type": "issue"} on a note that currently has note_type=plan, status=active, the note_type is updated to issue but the existing status=active is NOT re-validated against the new type. Since active is not a valid status for issue, this leaves the database in an invalid state.

The status validation only fires when body.status is not None:

if body.status is not None:
    _validate_status(effective_note_type, body.status)
    note.status = body.status

This is consistent with partial-update semantics (omitting a field means "don't touch it"), so I am not calling it a blocker. But it is a gap worth noting. A future improvement could either (a) re-validate existing status when note_type changes, or (b) clear status to None when note_type changes.

Non-blocking because:

• Changing note_type on an existing note is an unusual operation
• The spec says "reject invalid combinations" which applies to submitted input, not existing data
• Consistent with how other fields work (partial update semantics)

ROUND 1 NIT VERIFICATION

All 5 nits from the round 1 review are confirmed fixed:

• Nit 1: Eager load parent in create_note and update_note responses -- Both endpoints re-query with joinedload(Note.parent) after commit (lines 226-231 and 311-317). Tests at lines 705 and 726 verify parent_slug is populated in the response.
• Nit 2: Return empty list for nonexistent parent_slug in list_notes -- Returns [] when parent not found (line 160-162). Test at line 435 verifies.
• Nit 3: Order by position (asc, nulls last) when filtering by parent_slug -- q.order_by(Note.position.asc().nullslast()) at line 167. Test at line 487 creates children out of order and verifies sorted return.
• Nit 4: Unset parent via parent_slug="" on update -- Empty string check at line 284-286 sets parent_note_id = None. Test at line 670 verifies.
• Nit 5: Parametric status tests covering all 11 types -- _valid_pairs (27 combos) and _invalid_pairs (11 combos) at lines 754-819, using @pytest.mark.parametrize.

CORRECTNESS

• Migration: 4 nullable columns, FK with RESTRICT, composite index on (parent_note_id, position). Downgrade drops in correct order. Chain correct (c3d4e5f6a7b8 -> d4e5f6a7b8c9). SQLite-compatible (follows same pattern as existing c3d4e5f6a7b8 migration which is deployed).
• Model: Self-referential relationship with remote_side=[id], foreign_keys=[parent_note_id], back_populates. Children ordered by Note.position with passive_deletes=True. parent_slug computed property.
• Schemas: NoteType Literal with all 11 values matching note-conventions spec. NoteCreate, NoteUpdate, NoteOut, NoteSummary all include the 4 new fields.
• VALID_STATUSES dict: Matches note-conventions spec exactly for all 11 types.
• ALLOWED_PARENT_TYPES: {"phase": ["plan"]} matches spec.
• Validation: _validate_status rejects status without note_type, rejects invalid status for type. _resolve_parent validates parent-type map, self-parent check, parent existence.
• list_notes: Filters by note_type, status, parent_slug. Returns empty list for nonexistent parent. Orders by position when filtering by parent.
• create_note/update_note: Accept new fields, validate, resolve parent_slug to parent_note_id.
• Backward compatibility: All fields nullable. Existing notes with NULL values work as before.
• RESTRICT enforcement: test_delete_parent_with_children_fails confirms 409. conftest.py and database.py both enable PRAGMA foreign_keys=ON.

SECURITY

• New query params (note_type, status, parent_slug) are used via SQLAlchemy ORM filters, which are parameterized. No injection risk.
• No secrets, .env files, or credentials in the diff.

TEST COVERAGE

35 test functions in test_note_decomposition.py, expanding to ~71 test cases with parametrization:

• note_type validation (valid, invalid, all 11 types, omitted)
• status validation (valid, invalid for type, status without note_type, phase statuses, doc draft)
• Parent-child: create phase with plan parent, non-plan parent rejected, non-phase with parent rejected, parent not found, parent without note_type rejected
• Self-parent check on update
• Update: status-only, change note_type, invalid status for existing type, set parent
• list_notes: filter by note_type, status, parent_slug, combined filters, nonexistent parent returns empty
• Position ordering
• Response shape: NoteOut and NoteSummary include decomposition fields
• Delete RESTRICT: parent with children fails, child-then-parent succeeds
• Backward compatibility: old-style notes still work
• Unset parent via empty string
• Eager load parent_slug in create and update responses
• Parametric: all 27 valid (type, status) pairs accepted, all 11 invalid pairs rejected

All existing tests (test_crud.py etc.) should pass unchanged since all new columns are nullable.

SOP COMPLIANCE

• Branch named after issue (60-feat-add-note-type-status-parent-note-id)
• PR body follows template-pr-body (Summary, Changes, Test Plan, Review Checklist, Related Notes)
• Related Notes references plan slug (plan-2026-03-01-note-decomposition)
• Tests exist and cover all validation paths
• No secrets, .env files, or credentials committed
• No unnecessary file changes (5 files, all in scope)
• Commit messages are descriptive

VERDICT: APPROVED

Clean implementation that matches the spec exactly. All round 1 nits fixed. One non-blocking data integrity nit noted for future consideration (note_type change without status re-validation). No blockers.

## PR #61 Review (Round 2) ### BLOCKERS None. ### NITS **1. Changing `note_type` does not re-validate existing `status` (data integrity gap)** In `update_note` (routes/notes.py lines 270-280), if a caller sends `{"note_type": "issue"}` on a note that currently has `note_type=plan, status=active`, the note_type is updated to `issue` but the existing `status=active` is NOT re-validated against the new type. Since `active` is not a valid status for `issue`, this leaves the database in an invalid state. The status validation only fires when `body.status is not None`: ```python if body.status is not None: _validate_status(effective_note_type, body.status) note.status = body.status ``` This is consistent with partial-update semantics (omitting a field means "don't touch it"), so I am not calling it a blocker. But it is a gap worth noting. A future improvement could either (a) re-validate existing status when note_type changes, or (b) clear status to None when note_type changes. Non-blocking because: - Changing note_type on an existing note is an unusual operation - The spec says "reject invalid combinations" which applies to submitted input, not existing data - Consistent with how other fields work (partial update semantics) ### ROUND 1 NIT VERIFICATION All 5 nits from the round 1 review are confirmed fixed: - [x] **Nit 1: Eager load parent in create_note and update_note responses** -- Both endpoints re-query with `joinedload(Note.parent)` after commit (lines 226-231 and 311-317). Tests at lines 705 and 726 verify `parent_slug` is populated in the response. - [x] **Nit 2: Return empty list for nonexistent parent_slug in list_notes** -- Returns `[]` when parent not found (line 160-162). Test at line 435 verifies. - [x] **Nit 3: Order by position (asc, nulls last) when filtering by parent_slug** -- `q.order_by(Note.position.asc().nullslast())` at line 167. Test at line 487 creates children out of order and verifies sorted return. - [x] **Nit 4: Unset parent via `parent_slug=""` on update** -- Empty string check at line 284-286 sets `parent_note_id = None`. Test at line 670 verifies. - [x] **Nit 5: Parametric status tests covering all 11 types** -- `_valid_pairs` (27 combos) and `_invalid_pairs` (11 combos) at lines 754-819, using `@pytest.mark.parametrize`. ### CORRECTNESS - **Migration:** 4 nullable columns, FK with RESTRICT, composite index on `(parent_note_id, position)`. Downgrade drops in correct order. Chain correct (`c3d4e5f6a7b8` -> `d4e5f6a7b8c9`). SQLite-compatible (follows same pattern as existing `c3d4e5f6a7b8` migration which is deployed). - **Model:** Self-referential relationship with `remote_side=[id]`, `foreign_keys=[parent_note_id]`, `back_populates`. Children ordered by `Note.position` with `passive_deletes=True`. `parent_slug` computed property. - **Schemas:** `NoteType` Literal with all 11 values matching note-conventions spec. `NoteCreate`, `NoteUpdate`, `NoteOut`, `NoteSummary` all include the 4 new fields. - **VALID_STATUSES dict:** Matches note-conventions spec exactly for all 11 types. - **ALLOWED_PARENT_TYPES:** `{"phase": ["plan"]}` matches spec. - **Validation:** `_validate_status` rejects status without note_type, rejects invalid status for type. `_resolve_parent` validates parent-type map, self-parent check, parent existence. - **list_notes:** Filters by `note_type`, `status`, `parent_slug`. Returns empty list for nonexistent parent. Orders by position when filtering by parent. - **create_note/update_note:** Accept new fields, validate, resolve `parent_slug` to `parent_note_id`. - **Backward compatibility:** All fields nullable. Existing notes with NULL values work as before. - **RESTRICT enforcement:** `test_delete_parent_with_children_fails` confirms 409. `conftest.py` and `database.py` both enable `PRAGMA foreign_keys=ON`. ### SECURITY - New query params (`note_type`, `status`, `parent_slug`) are used via SQLAlchemy ORM filters, which are parameterized. No injection risk. - No secrets, .env files, or credentials in the diff. ### TEST COVERAGE 35 test functions in `test_note_decomposition.py`, expanding to ~71 test cases with parametrization: - note_type validation (valid, invalid, all 11 types, omitted) - status validation (valid, invalid for type, status without note_type, phase statuses, doc draft) - Parent-child: create phase with plan parent, non-plan parent rejected, non-phase with parent rejected, parent not found, parent without note_type rejected - Self-parent check on update - Update: status-only, change note_type, invalid status for existing type, set parent - list_notes: filter by note_type, status, parent_slug, combined filters, nonexistent parent returns empty - Position ordering - Response shape: NoteOut and NoteSummary include decomposition fields - Delete RESTRICT: parent with children fails, child-then-parent succeeds - Backward compatibility: old-style notes still work - Unset parent via empty string - Eager load parent_slug in create and update responses - Parametric: all 27 valid (type, status) pairs accepted, all 11 invalid pairs rejected All existing tests (test_crud.py etc.) should pass unchanged since all new columns are nullable. ### SOP COMPLIANCE - [x] Branch named after issue (`60-feat-add-note-type-status-parent-note-id`) - [x] PR body follows `template-pr-body` (Summary, Changes, Test Plan, Review Checklist, Related Notes) - [x] Related Notes references plan slug (`plan-2026-03-01-note-decomposition`) - [x] Tests exist and cover all validation paths - [x] No secrets, .env files, or credentials committed - [x] No unnecessary file changes (5 files, all in scope) - [x] Commit messages are descriptive ### VERDICT: APPROVED Clean implementation that matches the spec exactly. All round 1 nits fixed. One non-blocking data integrity nit noted for future consideration (note_type change without status re-validation). No blockers.

forgejo_admin merged commit ac72e52bc6 into main 2026-03-02 08:33:12 +00:00

forgejo_admin deleted branch 60-feat-add-note-type-status-parent-note-id 2026-03-02 08:33:12 +00:00

forgejo_admin referenced this pull request from a commit 2026-03-02 08:33:13 +00:00

feat: add note_type, status, parent_note_id, position columns (#60) (#61)

forgejo_admin referenced this pull request 2026-03-02 17:02:30 +00:00

feat: composition rendering — parent notes render child notes inline #62

forgejo_admin referenced this pull request 2026-03-28 16:13:35 +00:00

Fix 12 failing board_sync tests blocking CI deployment #233

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

domain:backend

  

domain:devops

  

domain:frontend

  

status:approved

QA passed, awaiting merge approval

  

status:in-progress

Dev agent is actively working

  

status:needs-fix

QA found issues, back to dev

  

status:qa

PR submitted, awaiting QA review

  

type:bug

Bug fix

  

type:devops

Infrastructure/CI/config work

  

type:feature

New functionality

No labels

domain:backend

domain:devops

domain:frontend

status:approved

status:in-progress

status:needs-fix

status:qa

type:bug

type:devops

type:feature

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

forgejo_admin/pal-e-api!61

No description provided.