fix: align board sync test fixtures with project-based phase discovery

forgejo_admin commented

2026-03-28 16:17:16 +00:00

Owner

Summary

The sync_board refactor (PR #210) changed phase discovery from board-item-based to project-note-based (Note.project_id == board.project_id), but test fixtures created plan notes without project_slug, leaving them with project_id=NULL and invisible to sync. This caused all 12 sync-dependent tests to fail.

Changes

tests/test_board_sync.py: Added project_slug parameter to _create_note helper so notes can be associated with projects
tests/test_board_sync.py: Pass project_slug="proj-sync" when creating plan note in TestBoardSync._setup
tests/test_board_sync.py: Pass project_slug="proj-hook" when creating plan note in TestUpdateNotePhaseHook._setup

Test Plan

All 16 tests in test_board_sync.py pass (12 previously failing, 4 already passing)
Full suite: 682 passed, 0 failed, no regressions
ruff format + ruff check clean

Review Checklist

No secrets committed
No unnecessary file changes
Commit messages are descriptive
Single file changed, minimal diff

Closes #233
pal-e-api — the project this work belongs to

## Summary The sync_board refactor (PR #210) changed phase discovery from board-item-based to project-note-based (Note.project_id == board.project_id), but test fixtures created plan notes without project_slug, leaving them with project_id=NULL and invisible to sync. This caused all 12 sync-dependent tests to fail. ## Changes - `tests/test_board_sync.py`: Added `project_slug` parameter to `_create_note` helper so notes can be associated with projects - `tests/test_board_sync.py`: Pass `project_slug="proj-sync"` when creating plan note in `TestBoardSync._setup` - `tests/test_board_sync.py`: Pass `project_slug="proj-hook"` when creating plan note in `TestUpdateNotePhaseHook._setup` ## Test Plan - [x] All 16 tests in `test_board_sync.py` pass (12 previously failing, 4 already passing) - [x] Full suite: 682 passed, 0 failed, no regressions - [x] ruff format + ruff check clean ## Review Checklist - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive - [x] Single file changed, minimal diff ## Related Notes - Closes #233 - `pal-e-api` — the project this work belongs to

forgejo_admin added 1 commit

2026-03-28 16:17:16 +00:00

fix: align board sync test fixtures with project-based phase discovery

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

bd011db83b

sync_board discovers plan notes via project_id, but test fixtures
created plan notes without project_slug so they had project_id=NULL
and were invisible to sync. Pass project_slug to plan note creation
in both test classes so sync_board finds them.

Fixes #233

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

forgejo_admin commented

2026-03-28 16:22:32 +00:00

Author

Owner

PR #234 Review

DOMAIN REVIEW

Stack: Python / FastAPI / SQLAlchemy / pytest

Root cause analysis verified. PR #210 changed sync_board to discover plan notes via Note.project_id == board.project_id (line 336 of routes/boards.py). The old test fixtures created plan notes without project_slug, so project_id was NULL. The query Note.project_id == board.project_id would never match these orphaned plan notes, causing sync to return 0 phases across all dependent tests.

Fix correctness verified. The NoteCreate Pydantic schema (schemas.py:108) accepts project_slug: str | None = None. The create_note endpoint (routes/notes.py:721-726) resolves project_slug to project_id via a Project lookup. Adding project_slug to the test fixture helper and passing it in both _setup methods correctly wires plan notes to their projects so sync_board discovers them.

12-test failure count verified. I manually traced which tests depend on sync discovering phases:

TestBoardSync: 8 of 10 tests affected (excluding test_sync_board_not_found which tests 404, and test_sync_no_plan_items which creates its own empty project)
TestUpdateNotePhaseHook: 4 of 6 tests affected (excluding test_update_phase_without_board_item_is_noop which tests the no-board-item path, and test_update_non_phase_note_does_not_touch_board which only checks the plan board item added directly via POST)
Total: 8 + 4 = 12. Matches the claim.

No side effects. The _create_note helper's new project_slug parameter defaults to None, so all existing callers that do not pass it are unaffected. The two _setup methods are the only call sites that changed. Phase note creation (child notes under the plan) correctly does NOT pass project_slug -- phases inherit their project association through parent_note_id, not through direct project_id assignment, and sync discovers them via Note.parent_note_id == plan_note.id.

BLOCKERS

None.

NITS

None. The diff is minimal, correctly formatted, and the comment on line 54 (# must belong to project so sync_board discovers it) documents the "why" clearly.

SOP COMPLIANCE

Branch named after issue (233-fix-board-sync-test-fixtures references #233)
PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related)
Related references Closes #233
No secrets committed
No unnecessary file changes (single file, +14/-4)
Commit messages are descriptive
ruff format + ruff check reported clean per Test Plan

PROCESS OBSERVATIONS

Change failure risk: LOW. Test-only change with zero production code modifications. The fix restores the test/implementation contract that PR #210 broke.
DORA impact: Unblocks CI deployment pipeline. 12 failing tests are a hard blocker for deployment frequency. This is a high-priority fix.
Regression risk: NONE. Default parameter value preserves backward compatibility with all other _create_note callers in the test suite.

VERDICT: APPROVED

## PR #234 Review ### DOMAIN REVIEW **Stack:** Python / FastAPI / SQLAlchemy / pytest **Root cause analysis verified.** PR #210 changed `sync_board` to discover plan notes via `Note.project_id == board.project_id` (line 336 of `routes/boards.py`). The old test fixtures created plan notes without `project_slug`, so `project_id` was NULL. The query `Note.project_id == board.project_id` would never match these orphaned plan notes, causing sync to return 0 phases across all dependent tests. **Fix correctness verified.** The `NoteCreate` Pydantic schema (`schemas.py:108`) accepts `project_slug: str | None = None`. The `create_note` endpoint (`routes/notes.py:721-726`) resolves `project_slug` to `project_id` via a Project lookup. Adding `project_slug` to the test fixture helper and passing it in both `_setup` methods correctly wires plan notes to their projects so `sync_board` discovers them. **12-test failure count verified.** I manually traced which tests depend on sync discovering phases: - **TestBoardSync:** 8 of 10 tests affected (excluding `test_sync_board_not_found` which tests 404, and `test_sync_no_plan_items` which creates its own empty project) - **TestUpdateNotePhaseHook:** 4 of 6 tests affected (excluding `test_update_phase_without_board_item_is_noop` which tests the no-board-item path, and `test_update_non_phase_note_does_not_touch_board` which only checks the plan board item added directly via POST) - Total: 8 + 4 = 12. Matches the claim. **No side effects.** The `_create_note` helper's new `project_slug` parameter defaults to `None`, so all existing callers that do not pass it are unaffected. The two `_setup` methods are the only call sites that changed. Phase note creation (child notes under the plan) correctly does NOT pass `project_slug` -- phases inherit their project association through parent_note_id, not through direct project_id assignment, and sync discovers them via `Note.parent_note_id == plan_note.id`. ### BLOCKERS None. ### NITS None. The diff is minimal, correctly formatted, and the comment on line 54 (`# must belong to project so sync_board discovers it`) documents the "why" clearly. ### SOP COMPLIANCE - [x] Branch named after issue (`233-fix-board-sync-test-fixtures` references #233) - [x] PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related) - [x] Related references `Closes #233` - [x] No secrets committed - [x] No unnecessary file changes (single file, +14/-4) - [x] Commit messages are descriptive - [x] ruff format + ruff check reported clean per Test Plan ### PROCESS OBSERVATIONS - **Change failure risk: LOW.** Test-only change with zero production code modifications. The fix restores the test/implementation contract that PR #210 broke. - **DORA impact:** Unblocks CI deployment pipeline. 12 failing tests are a hard blocker for deployment frequency. This is a high-priority fix. - **Regression risk: NONE.** Default parameter value preserves backward compatibility with all other `_create_note` callers in the test suite. ### VERDICT: APPROVED