feat: update board API to query board notes internally #209

Merged

forgejo_admin merged 2 commits from 197-board-api-use-notes into main 2026-03-24 18:55:10 +00:00

Conversation 2 Commits 2 Files changed 3 +279 -36

forgejo_admin commented 2026-03-24 18:18:41 +00:00

Owner

Copy link

Summary

Updates all 14 board API endpoints to query board notes (note_type='board') internally, while preserving the boards table during transition. Board CRUD now creates/updates/deletes a corresponding Note alongside the Board row. Response contract is additive: BoardOut gains note_id, BoardItemOut gains board_note_id.

Changes

• src/pal_e_docs/routes/boards.py — Added _resolve_board() helper that checks board notes first, falls back to boards table. Added _get_board_note_id() for note_id lookup with NULL-safe logging. create_board now creates a board note alongside the Board row. update_board syncs note title. delete_board removes both board and note. All item-creating endpoints (create_board_item, sync_board, sync_issues) populate board_note_id. update_board_item backfills board_note_id if missing.
• src/pal_e_docs/schemas.py — Added note_id: int | None = None to BoardOut. Added board_note_id: int | None = None to BoardItemOut.
• tests/test_boards.py — Added 9 integration tests verifying: note_id in BoardOut responses, board_note_id in BoardItemOut responses, board note creation with correct note_type, title sync on update, cascading delete, and field compatibility.

Test Plan

• All 648 tests pass (639 existing + 9 new)
• Board test suite: 66 tests pass (57 existing + 9 new)
• ruff format and ruff check clean

Prerequisites

• Run scripts/migrate_boards_to_notes.py before deploying to create board notes for existing boards and backfill board_note_id on existing board items
• PR #207 merged (board_note_id column exists)

Related

• Forgejo issue: #197
• Prerequisite: PR #207 (board_note_id column migration)

## Summary Updates all 14 board API endpoints to query board notes (note_type='board') internally, while preserving the boards table during transition. Board CRUD now creates/updates/deletes a corresponding Note alongside the Board row. Response contract is additive: BoardOut gains `note_id`, BoardItemOut gains `board_note_id`. ## Changes - **`src/pal_e_docs/routes/boards.py`** — Added `_resolve_board()` helper that checks board notes first, falls back to boards table. Added `_get_board_note_id()` for note_id lookup with NULL-safe logging. `create_board` now creates a board note alongside the Board row. `update_board` syncs note title. `delete_board` removes both board and note. All item-creating endpoints (create_board_item, sync_board, sync_issues) populate `board_note_id`. `update_board_item` backfills `board_note_id` if missing. - **`src/pal_e_docs/schemas.py`** — Added `note_id: int | None = None` to `BoardOut`. Added `board_note_id: int | None = None` to `BoardItemOut`. - **`tests/test_boards.py`** — Added 9 integration tests verifying: note_id in BoardOut responses, board_note_id in BoardItemOut responses, board note creation with correct note_type, title sync on update, cascading delete, and field compatibility. ## Test Plan - All 648 tests pass (639 existing + 9 new) - Board test suite: 66 tests pass (57 existing + 9 new) - ruff format and ruff check clean ## Prerequisites - **Run `scripts/migrate_boards_to_notes.py` before deploying** to create board notes for existing boards and backfill `board_note_id` on existing board items - PR #207 merged (board_note_id column exists) ## Related - Forgejo issue: #197 - Prerequisite: PR #207 (board_note_id column migration)

forgejo_admin added 1 commit 2026-03-24 18:18:41 +00:00

feat: update board API to query board notes internally ... cecb26c48f

Board CRUD now creates/updates/deletes a corresponding Note with
note_type='board' alongside the Board row. _resolve_board() checks
board notes first, falling back to the boards table for pre-migration
data. BoardOut gains note_id; BoardItemOut gains board_note_id. All
new board items are created with board_note_id populated. Existing
items get board_note_id backfilled on update. Nine new integration
tests verify the additive response contract.

Closes #197

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

forgejo_admin added 1 commit 2026-03-24 18:20:17 +00:00

fix: simplify _resolve_board and add note slug collision guard ... 413b71caa8

Remove redundant double-query in _resolve_board — the Board table
lookup is sufficient since slugs are shared between boards and board
notes. Add a note slug collision guard in create_board to return a
clean 409 instead of a DB constraint error when a non-board note
already occupies the target slug.

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

forgejo_admin commented 2026-03-24 18:20:32 +00:00

Author

Owner

Copy link

QA Review

Findings (fixed in 413b71c)

1. Redundant query in _resolve_board -- The original implementation queried Note first to check for a board note, then queried Board by slug, and if neither matched, queried Board by slug again. Since board slugs and board note slugs are always identical, the note lookup was redundant. Simplified to a single Board.slug query with fallback to 404.

2. Missing note slug collision guard in create_board -- If a non-board note already existed with slug board-{project_slug}, the endpoint would hit a DB unique constraint error (500) instead of returning a clean 409. Added an explicit check that returns a descriptive 409 error.

Verified

• All 648 tests pass (639 existing + 9 new integration tests)
• ruff format and ruff check clean
• All three architectural decisions from issue #197 comment #6640 implemented:
  1. All 14 /boards/{slug} routes use _resolve_board internally
  2. NULL board_note_id handled gracefully with logging and fallback
  3. Response contract is additive: BoardOut.note_id and BoardItemOut.board_note_id added, all existing fields preserved

VERDICT: APPROVE

## QA Review ### Findings (fixed in 413b71c) 1. **Redundant query in `_resolve_board`** -- The original implementation queried `Note` first to check for a board note, then queried `Board` by slug, and if neither matched, queried `Board` by slug again. Since board slugs and board note slugs are always identical, the note lookup was redundant. Simplified to a single `Board.slug` query with fallback to 404. 2. **Missing note slug collision guard in `create_board`** -- If a non-board note already existed with slug `board-{project_slug}`, the endpoint would hit a DB unique constraint error (500) instead of returning a clean 409. Added an explicit check that returns a descriptive 409 error. ### Verified - All 648 tests pass (639 existing + 9 new integration tests) - ruff format and ruff check clean - All three architectural decisions from issue #197 comment #6640 implemented: 1. All 14 `/boards/{slug}` routes use `_resolve_board` internally 2. NULL `board_note_id` handled gracefully with logging and fallback 3. Response contract is additive: `BoardOut.note_id` and `BoardItemOut.board_note_id` added, all existing fields preserved ### VERDICT: APPROVE

forgejo_admin commented 2026-03-24 18:39:45 +00:00

Author

Owner

Copy link

PR #209 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Pydantic (pal-e-docs API).

This PR updates all 14 board API endpoints to resolve boards via board notes (note_type="board") while preserving the boards table during the transition period. The response contract is additive: BoardOut gains note_id, BoardItemOut gains board_note_id. Board CRUD now creates/updates/deletes a corresponding Note alongside the Board row. Nine new integration tests cover the key paths.

Endpoint audit -- all 14 endpoints now use _resolve_board():

1. GET /boards/backlog/items -- no slug resolution needed (cross-board query, correct)
2. GET /boards/activity -- no slug resolution needed (cross-board query, correct)
3. GET /boards -- uses _board_to_out(b, db) which calls _get_board_note_id
4. POST /boards -- creates board note alongside Board row
5. GET /boards/{slug} -- uses _resolve_board
6. PATCH /boards/{slug} -- uses _resolve_board, syncs note title
7. DELETE /boards/{slug} -- uses _resolve_board, cascading note delete
8. POST /boards/{slug}/sync -- uses _resolve_board, populates board_note_id on new items
9. POST /boards/{slug}/sync-issues -- uses _resolve_board, populates + backfills board_note_id
10. PATCH /boards/{slug}/items/bulk -- uses _resolve_board
11. GET /boards/{slug}/items -- uses _resolve_board
12. POST /boards/{slug}/items -- uses _resolve_board, populates board_note_id
13. PATCH /boards/{slug}/items/{item_id} -- uses _resolve_board, backfills board_note_id
14. DELETE /boards/{slug}/items/{item_id} -- uses _resolve_board

All raw db.query(Board).filter(Board.slug == slug) calls have been consolidated into _resolve_board(). Only one instance remains, inside _resolve_board itself. This is clean DRY refactoring.

Schema changes are additive and backward-compatible:

• BoardOut.note_id: int | None = None -- defaults to None, safe for existing consumers
• BoardItemOut.board_note_id: int | None = None -- defaults to None, safe for existing consumers

Model confirms BoardItem.board_note_id is Mapped[int | None] with FK to notes.id, nullable, indexed. This was added in PR #207 (prerequisite, already merged).

Migration script (scripts/migrate_boards_to_notes.py) is present, idempotent, handles slug collisions, and includes clear run instructions. Correctly referenced in the PR body as a prerequisite step.

NULL handling: _get_board_note_id gracefully returns None with a warning log for pre-migration boards. All callers handle the None case. Backfill logic in update_board_item (line 690) and sync_issues (line 482) correctly checks is None before setting.

BLOCKERS

None.

NITS

1. Misleading _resolve_board docstring (boards.py:34-36): The docstring says "Checks both the board note (note_type='board') and the boards table" but the implementation only queries the Board table. The note lookup described never happens. The docstring should be updated to match what the code actually does -- it resolves via the boards table and raises 404 if not found.

2. sync_board missing board_note_id backfill on existing items (boards.py:370-375): When sync_board finds an existing phase item and updates its column, it does NOT backfill board_note_id like sync_issues (line 482-484) and update_board_item (line 690-693) do. This means pre-migration phase items updated only via sync_board will never get their board_note_id populated through normal API usage. The migration script handles the initial backfill, but ongoing sync calls miss this path. Consider adding the same backfill pattern for consistency.

3. N+1 query in list_boards (boards.py:198): _board_to_out calls _get_board_note_id which issues a separate db.query(Note) for each board. With a small number of boards this is fine, but as the platform grows this becomes O(N) extra queries. A future optimization could use a subquery or join to fetch all board note IDs in one query. Non-blocking -- the board count is small today.

4. create_board slug collision edge case (boards.py:218-223): The guard at line 219 only blocks non-board note types. If a board note with the same slug already exists (e.g., migration was run but the Board row was somehow deleted and re-created), the code passes the guard and attempts to create a DUPLICATE Note with the same slug, which would throw an IntegrityError from the unique constraint. In practice this is unlikely because the duplicate-Board-per-project guard at line 208 fires first. But for defense-in-depth, the guard could also skip note creation when existing_note.note_type == "board" and reuse the existing note instead.

5. No test for slug collision guard: The 409 path for note slug collision in create_board (line 219-223) has no test coverage. Consider adding a test that pre-creates a non-board note with the expected slug and verifies the 409 response.

SOP COMPLIANCE

• Branch named after issue: 197-board-api-use-notes references issue #197
• PR body follows template: Summary, Changes, Test Plan, Prerequisites, Related sections present
• Related references issue: Forgejo issue: #197 and prerequisite PR #207
• No secrets committed: No credentials, .env files, or API keys in the diff
• Test plan documented: 648 total tests (639 existing + 9 new), 66 board tests (57 + 9)
• Scope is tight: 3 files changed, all directly related to the issue
• Migration script present with clear run instructions

PROCESS OBSERVATIONS

• Change failure risk: LOW. The changes are additive (new fields default to None), the migration script is idempotent, and the PR preserves the boards table during transition. This is a clean intermediate step toward issue #199 (drop legacy boards table).
• Deployment dependency: The migration script MUST run before deployment. The PR body documents this clearly. The prerequisite PR #207 (board_note_id column) is already merged.
• Test coverage: 9 new tests cover create/get/list note_id, board_note_id on items, note_type verification, title sync, cascading delete, and field compatibility. This is solid coverage for the new paths.
• Discovered scope: The sync_board backfill inconsistency (nit #2) could be tracked as a follow-up item on issue #208 (which is already open for sync_board refactoring).

VERDICT: APPROVED

## PR #209 Review ### DOMAIN REVIEW **Tech stack**: Python / FastAPI / SQLAlchemy / Pydantic (pal-e-docs API). This PR updates all 14 board API endpoints to resolve boards via board notes (`note_type="board"`) while preserving the `boards` table during the transition period. The response contract is additive: `BoardOut` gains `note_id`, `BoardItemOut` gains `board_note_id`. Board CRUD now creates/updates/deletes a corresponding `Note` alongside the `Board` row. Nine new integration tests cover the key paths. **Endpoint audit** -- all 14 endpoints now use `_resolve_board()`: 1. `GET /boards/backlog/items` -- no slug resolution needed (cross-board query, correct) 2. `GET /boards/activity` -- no slug resolution needed (cross-board query, correct) 3. `GET /boards` -- uses `_board_to_out(b, db)` which calls `_get_board_note_id` 4. `POST /boards` -- creates board note alongside Board row 5. `GET /boards/{slug}` -- uses `_resolve_board` 6. `PATCH /boards/{slug}` -- uses `_resolve_board`, syncs note title 7. `DELETE /boards/{slug}` -- uses `_resolve_board`, cascading note delete 8. `POST /boards/{slug}/sync` -- uses `_resolve_board`, populates `board_note_id` on new items 9. `POST /boards/{slug}/sync-issues` -- uses `_resolve_board`, populates + backfills `board_note_id` 10. `PATCH /boards/{slug}/items/bulk` -- uses `_resolve_board` 11. `GET /boards/{slug}/items` -- uses `_resolve_board` 12. `POST /boards/{slug}/items` -- uses `_resolve_board`, populates `board_note_id` 13. `PATCH /boards/{slug}/items/{item_id}` -- uses `_resolve_board`, backfills `board_note_id` 14. `DELETE /boards/{slug}/items/{item_id}` -- uses `_resolve_board` All raw `db.query(Board).filter(Board.slug == slug)` calls have been consolidated into `_resolve_board()`. Only one instance remains, inside `_resolve_board` itself. This is clean DRY refactoring. **Schema changes** are additive and backward-compatible: - `BoardOut.note_id: int | None = None` -- defaults to None, safe for existing consumers - `BoardItemOut.board_note_id: int | None = None` -- defaults to None, safe for existing consumers **Model** confirms `BoardItem.board_note_id` is `Mapped[int | None]` with FK to `notes.id`, nullable, indexed. This was added in PR #207 (prerequisite, already merged). **Migration script** (`scripts/migrate_boards_to_notes.py`) is present, idempotent, handles slug collisions, and includes clear run instructions. Correctly referenced in the PR body as a prerequisite step. **NULL handling**: `_get_board_note_id` gracefully returns `None` with a warning log for pre-migration boards. All callers handle the None case. Backfill logic in `update_board_item` (line 690) and `sync_issues` (line 482) correctly checks `is None` before setting. ### BLOCKERS None. ### NITS 1. **Misleading `_resolve_board` docstring** (`boards.py:34-36`): The docstring says "Checks both the board note (note_type='board') and the boards table" but the implementation only queries the `Board` table. The note lookup described never happens. The docstring should be updated to match what the code actually does -- it resolves via the boards table and raises 404 if not found. 2. **`sync_board` missing `board_note_id` backfill on existing items** (`boards.py:370-375`): When `sync_board` finds an existing phase item and updates its column, it does NOT backfill `board_note_id` like `sync_issues` (line 482-484) and `update_board_item` (line 690-693) do. This means pre-migration phase items updated only via `sync_board` will never get their `board_note_id` populated through normal API usage. The migration script handles the initial backfill, but ongoing sync calls miss this path. Consider adding the same backfill pattern for consistency. 3. **N+1 query in `list_boards`** (`boards.py:198`): `_board_to_out` calls `_get_board_note_id` which issues a separate `db.query(Note)` for each board. With a small number of boards this is fine, but as the platform grows this becomes O(N) extra queries. A future optimization could use a subquery or join to fetch all board note IDs in one query. Non-blocking -- the board count is small today. 4. **`create_board` slug collision edge case** (`boards.py:218-223`): The guard at line 219 only blocks non-board note types. If a board note with the same slug already exists (e.g., migration was run but the Board row was somehow deleted and re-created), the code passes the guard and attempts to create a DUPLICATE Note with the same slug, which would throw an `IntegrityError` from the unique constraint. In practice this is unlikely because the duplicate-Board-per-project guard at line 208 fires first. But for defense-in-depth, the guard could also skip note creation when `existing_note.note_type == "board"` and reuse the existing note instead. 5. **No test for slug collision guard**: The `409` path for note slug collision in `create_board` (line 219-223) has no test coverage. Consider adding a test that pre-creates a non-board note with the expected slug and verifies the 409 response. ### SOP COMPLIANCE - [x] Branch named after issue: `197-board-api-use-notes` references issue #197 - [x] PR body follows template: Summary, Changes, Test Plan, Prerequisites, Related sections present - [x] Related references issue: `Forgejo issue: #197` and prerequisite `PR #207` - [x] No secrets committed: No credentials, .env files, or API keys in the diff - [x] Test plan documented: 648 total tests (639 existing + 9 new), 66 board tests (57 + 9) - [x] Scope is tight: 3 files changed, all directly related to the issue - [x] Migration script present with clear run instructions ### PROCESS OBSERVATIONS - **Change failure risk**: LOW. The changes are additive (new fields default to None), the migration script is idempotent, and the PR preserves the boards table during transition. This is a clean intermediate step toward issue #199 (drop legacy boards table). - **Deployment dependency**: The migration script MUST run before deployment. The PR body documents this clearly. The prerequisite PR #207 (board_note_id column) is already merged. - **Test coverage**: 9 new tests cover create/get/list note_id, board_note_id on items, note_type verification, title sync, cascading delete, and field compatibility. This is solid coverage for the new paths. - **Discovered scope**: The `sync_board` backfill inconsistency (nit #2) could be tracked as a follow-up item on issue #208 (which is already open for sync_board refactoring). ### VERDICT: APPROVED

forgejo_admin merged commit 7edb273f14 into main 2026-03-24 18:55:10 +00:00

forgejo_admin deleted branch 197-board-api-use-notes 2026-03-24 18:55:10 +00:00

forgejo_admin referenced this pull request from a commit 2026-03-24 18:55:10 +00:00

feat: update board API to query board notes internally (#209)

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

Validate: run alembic upgrade head for NoteType + validation column migrations #232

forgejo_admin referenced this pull request 2026-03-28 18:38:07 +00:00

Validate: run alembic upgrade head for NoteType + validation column migrations #232

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!209

No description provided.