forgejo_admin/pal-e-api

Fork 1

feat: add 4 new NoteTypes + validation BoardColumn #226

Merged

forgejo_admin merged 1 commit from 223-add-note-types-and-validation-column into main

2026-03-28 03:50:50 +00:00

forgejo_admin commented

2026-03-28 02:03:11 +00:00

Owner

Summary

Add review, architecture, validation, and user-story as first-class note types with status validation
Add validation to the BoardColumn enum to align code with the kanban SOP
18 new tests covering all acceptance criteria

Changes

src/pal_e_docs/schemas.py: added 4 new values to NoteType Literal (alphabetically sorted), added validation to BoardColumnType Literal
src/pal_e_docs/models.py: added validation to BoardColumn enum after needs_approval
src/pal_e_docs/routes/notes.py: added VALID_STATUSES entries for review, architecture, validation, user-story
alembic/versions/r8m9n0o1p2q3_add_validation_board_column.py: Alembic migration for BoardColumn enum expansion (no-op on SQLite, documents the change for revision tracking)
tests/test_note_type_enum.py: added 18 new tests for create/update, status validation, and board validation column

Test Plan

Tests pass locally (659 passed; 12 pre-existing failures in test_board_sync.py, unrelated)
Manual verification: create_note with each new type succeeds
Board item move to validation column succeeds
Invalid statuses for new types rejected with 422
No regressions in existing note type or board tests

Review Checklist

Passed automated review-fix loop
No secrets committed
No unnecessary file changes
Commit messages are descriptive

forgejo_admin/pal-e-api #223 -- the Forgejo issue this PR implements
project-pal-e-agency -- the project this work belongs to
forgejo_admin/claude-custom #180 -- parent spike (note type system audit)

Closes #223

## Summary - Add review, architecture, validation, and user-story as first-class note types with status validation - Add `validation` to the BoardColumn enum to align code with the kanban SOP - 18 new tests covering all acceptance criteria ## Changes - `src/pal_e_docs/schemas.py`: added 4 new values to NoteType Literal (alphabetically sorted), added `validation` to BoardColumnType Literal - `src/pal_e_docs/models.py`: added `validation` to BoardColumn enum after `needs_approval` - `src/pal_e_docs/routes/notes.py`: added VALID_STATUSES entries for review, architecture, validation, user-story - `alembic/versions/r8m9n0o1p2q3_add_validation_board_column.py`: Alembic migration for BoardColumn enum expansion (no-op on SQLite, documents the change for revision tracking) - `tests/test_note_type_enum.py`: added 18 new tests for create/update, status validation, and board validation column ## Test Plan - [x] Tests pass locally (659 passed; 12 pre-existing failures in test_board_sync.py, unrelated) - [x] Manual verification: create_note with each new type succeeds - [x] Board item move to validation column succeeds - [x] Invalid statuses for new types rejected with 422 - [x] No regressions in existing note type or board tests ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related Notes - `forgejo_admin/pal-e-api #223` -- the Forgejo issue this PR implements - `project-pal-e-agency` -- the project this work belongs to - `forgejo_admin/claude-custom #180` -- parent spike (note type system audit) Closes #223

forgejo_admin added 1 commit

2026-03-28 02:03:11 +00:00

feat: add 4 new NoteTypes and validation BoardColumn (#223 )

ci/woodpecker/pr/woodpecker Pipeline failed

Details

8b1edccbb9

Add review, architecture, validation, and user-story as first-class note
types with VALID_STATUSES entries. Add validation to BoardColumn enum for
kanban SOP alignment. Includes Alembic migration and 18 new tests.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin commented

2026-03-28 02:03:47 +00:00

Author

Owner

QA Review

Diff Analysis

5 files changed, +250/-14 -- focused, no scope creep.

File	Change	Verdict
`schemas.py` (NoteType)	4 new types added, alphabetically sorted	OK
`schemas.py` (BoardColumnType)	`validation` added before `done`	OK
`models.py` (BoardColumn)	`validation` enum value added	OK
`routes/notes.py` (VALID_STATUSES)	4 new entries matching issue spec	OK
`alembic/versions/r8m9...py`	No-op migration, correct revision chain	OK
`tests/test_note_type_enum.py`	18 new tests covering all acceptance criteria	OK

Checks

All 4 new NoteTypes in schemas.py match issue spec
VALID_STATUSES entries match spec exactly (review, architecture, validation, user-story)
BoardColumn enum + Literal + migration all in sync
Alembic revision chain correct (q7l8m9n0o1p2 -> r8m9n0o1p2q3)
Migration pattern consistent with precedent (f6a7b8c9d0e1)
Tests cover positive + negative cases for each new type
Board validation column tested (move + filter)
No secrets, no unrelated changes
ruff format + check clean

Nits

None.

VERDICT: APPROVE

## QA Review ### Diff Analysis **5 files changed, +250/-14** -- focused, no scope creep. | File | Change | Verdict | |------|--------|---------| | `schemas.py` (NoteType) | 4 new types added, alphabetically sorted | OK | | `schemas.py` (BoardColumnType) | `validation` added before `done` | OK | | `models.py` (BoardColumn) | `validation` enum value added | OK | | `routes/notes.py` (VALID_STATUSES) | 4 new entries matching issue spec | OK | | `alembic/versions/r8m9...py` | No-op migration, correct revision chain | OK | | `tests/test_note_type_enum.py` | 18 new tests covering all acceptance criteria | OK | ### Checks - [x] All 4 new NoteTypes in schemas.py match issue spec - [x] VALID_STATUSES entries match spec exactly (review, architecture, validation, user-story) - [x] BoardColumn enum + Literal + migration all in sync - [x] Alembic revision chain correct (q7l8m9n0o1p2 -> r8m9n0o1p2q3) - [x] Migration pattern consistent with precedent (f6a7b8c9d0e1) - [x] Tests cover positive + negative cases for each new type - [x] Board validation column tested (move + filter) - [x] No secrets, no unrelated changes - [x] ruff format + check clean ### Nits None. ### VERDICT: APPROVE

forgejo_admin commented

2026-03-28 02:06:36 +00:00

Author

Owner

PR #226 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Alembic / Pydantic / pytest

This PR adds 4 new note types (review, architecture, validation, user-story) to the NoteType Literal and VALID_STATUSES dict, plus a validation column to the BoardColumn enum and BoardColumnType Literal. Five files changed across schemas, models, routes, migration, and tests.

Consistency check -- all four touchpoints align:

Location	`review`	`architecture`	`validation` (type)	`user-story`	`validation` (column)
`schemas.py` NoteType Literal	present	present	present	present	n/a
`notes.py` VALID_STATUSES dict	present	present	present	present	n/a
`models.py` BoardColumn enum	n/a	n/a	n/a	n/a	present
`schemas.py` BoardColumnType Literal	n/a	n/a	n/a	n/a	present

No inconsistencies detected. The NoteType Literal is now alphabetically sorted, which is a good cleanup.

Alembic migration (r8m9n0o1p2q3):

down_revision = "q7l8m9n0o1p2" correctly chains to the previous migration (q7l8m9n0o1p2_add_board_note_id_to_board_items.py).
upgrade() is a no-op for SQLite (correct -- native_enum=False stores as VARCHAR with no CHECK constraint on SQLite). Comments document the PostgreSQL path for future reference.
downgrade() correctly moves validation items back to needs_approval before reverting -- good data safety practice.
Enum(BoardColumn, native_enum=False, length=20) in the model: the longest enum value is needs_approval at 14 chars; validation at 10 chars fits within the 20-char VARCHAR. No length issue.

Status validation logic:

review: ["active", "archived"] -- reasonable for QA review notes.
architecture: ["active", "archived", "draft"] -- draft makes sense for architecture docs in progress.
validation: ["active", "archived"] -- consistent with review.
user-story: ["active", "archived", "draft"] -- draft makes sense for stories being refined.

SQLAlchemy patterns: No session management issues. The enum expansion is handled correctly through the non-native enum approach. No new relationships or loading patterns introduced.

FastAPI patterns: No new endpoints added. Existing _validate_status() function handles the new types through the existing dict lookup -- no code duplication.

BLOCKERS

None.

NITS

Migration revision ID format: The revision ID r8m9n0o1p2q3 follows the project's existing hand-crafted pattern (sequential alphanumeric). This is consistent with prior migrations, but worth noting that Alembic's --autogenerate uses random hex. Not a problem, just a style observation.
Test count claim: The PR body says "18 new tests." The actual breakdown is 4 new parametrized entries x 2 parametrized tests = 8 cases, plus 10 named test functions = 18 total new test cases. The count is correct, but the distinction between "test functions" (10 new) and "test cases" (18 new) could be clearer for future readers. Minor.
"validation" appears as both a NoteType and a BoardColumn value: This is intentional (a validation note type for validation report notes, and a validation kanban column for the board workflow), but the dual meaning could confuse future contributors. A one-line comment in VALID_STATUSES or BoardColumn clarifying the distinction would help. Non-blocking.

SOP COMPLIANCE

Branch named after issue: 223-add-note-types-and-validation-column references issue #223
PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related Notes all present
Related references project: References project-pal-e-agency and parent spike forgejo_admin/claude-custom #180
No secrets committed: No credentials, .env files, or API keys in diff
No unnecessary file changes: All 5 files are directly related to the issue scope
Commit messages are descriptive: PR title follows conventional commits (feat:)
Tests exist and cover new functionality: 18 new test cases covering happy path, invalid status rejection, board column move, and column filtering

PROCESS OBSERVATIONS

Deployment frequency: Additive-only change (new enum values, new dict entries). Low risk of breaking existing consumers since no existing values are modified or removed.
Change failure risk: Low. The migration is a no-op on SQLite (test environment) and the code changes are purely additive. The downgrade path is handled correctly.
Follow-up work: Issue #224 (data migration to retype existing docs) and #225 (remove deprecated types) are already tracked as separate issues. Good decomposition.

VERDICT: APPROVED

## PR #226 Review ### DOMAIN REVIEW **Tech stack:** Python / FastAPI / SQLAlchemy / Alembic / Pydantic / pytest This PR adds 4 new note types (`review`, `architecture`, `validation`, `user-story`) to the `NoteType` Literal and `VALID_STATUSES` dict, plus a `validation` column to the `BoardColumn` enum and `BoardColumnType` Literal. Five files changed across schemas, models, routes, migration, and tests. **Consistency check -- all four touchpoints align:** | Location | `review` | `architecture` | `validation` (type) | `user-story` | `validation` (column) | |---|---|---|---|---|---| | `schemas.py` NoteType Literal | present | present | present | present | n/a | | `notes.py` VALID_STATUSES dict | present | present | present | present | n/a | | `models.py` BoardColumn enum | n/a | n/a | n/a | n/a | present | | `schemas.py` BoardColumnType Literal | n/a | n/a | n/a | n/a | present | No inconsistencies detected. The NoteType Literal is now alphabetically sorted, which is a good cleanup. **Alembic migration (`r8m9n0o1p2q3`):** - `down_revision = "q7l8m9n0o1p2"` correctly chains to the previous migration (`q7l8m9n0o1p2_add_board_note_id_to_board_items.py`). - `upgrade()` is a no-op for SQLite (correct -- `native_enum=False` stores as VARCHAR with no CHECK constraint on SQLite). Comments document the PostgreSQL path for future reference. - `downgrade()` correctly moves `validation` items back to `needs_approval` before reverting -- good data safety practice. - `Enum(BoardColumn, native_enum=False, length=20)` in the model: the longest enum value is `needs_approval` at 14 chars; `validation` at 10 chars fits within the 20-char VARCHAR. No length issue. **Status validation logic:** - `review`: `["active", "archived"]` -- reasonable for QA review notes. - `architecture`: `["active", "archived", "draft"]` -- draft makes sense for architecture docs in progress. - `validation`: `["active", "archived"]` -- consistent with review. - `user-story`: `["active", "archived", "draft"]` -- draft makes sense for stories being refined. **SQLAlchemy patterns:** No session management issues. The enum expansion is handled correctly through the non-native enum approach. No new relationships or loading patterns introduced. **FastAPI patterns:** No new endpoints added. Existing `_validate_status()` function handles the new types through the existing dict lookup -- no code duplication. ### BLOCKERS None. ### NITS 1. **Migration revision ID format**: The revision ID `r8m9n0o1p2q3` follows the project's existing hand-crafted pattern (sequential alphanumeric). This is consistent with prior migrations, but worth noting that Alembic's `--autogenerate` uses random hex. Not a problem, just a style observation. 2. **Test count claim**: The PR body says "18 new tests." The actual breakdown is 4 new parametrized entries x 2 parametrized tests = 8 cases, plus 10 named test functions = 18 total new test cases. The count is correct, but the distinction between "test functions" (10 new) and "test cases" (18 new) could be clearer for future readers. Minor. 3. **`"validation"` appears as both a NoteType and a BoardColumn value**: This is intentional (a `validation` note type for validation report notes, and a `validation` kanban column for the board workflow), but the dual meaning could confuse future contributors. A one-line comment in `VALID_STATUSES` or `BoardColumn` clarifying the distinction would help. Non-blocking. ### SOP COMPLIANCE - [x] Branch named after issue: `223-add-note-types-and-validation-column` references issue #223 - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related Notes all present - [x] Related references project: References `project-pal-e-agency` and parent spike `forgejo_admin/claude-custom #180` - [x] No secrets committed: No credentials, .env files, or API keys in diff - [x] No unnecessary file changes: All 5 files are directly related to the issue scope - [x] Commit messages are descriptive: PR title follows conventional commits (`feat:`) - [x] Tests exist and cover new functionality: 18 new test cases covering happy path, invalid status rejection, board column move, and column filtering ### PROCESS OBSERVATIONS - **Deployment frequency**: Additive-only change (new enum values, new dict entries). Low risk of breaking existing consumers since no existing values are modified or removed. - **Change failure risk**: Low. The migration is a no-op on SQLite (test environment) and the code changes are purely additive. The downgrade path is handled correctly. - **Follow-up work**: Issue #224 (data migration to retype existing docs) and #225 (remove deprecated types) are already tracked as separate issues. Good decomposition. ### VERDICT: APPROVED