refactor: sync_board discovers plans from project notes, not board items #210

Merged

forgejo_admin merged 1 commit from 208-sync-board-refactor into main

2026-03-24 20:38:23 +00:00

forgejo_admin commented

2026-03-24 20:28:07 +00:00

Owner

Summary

Refactors POST /boards/{slug}/sync to discover plan notes directly from the project (WHERE note_type='plan' AND project_id=board.project_id) instead of from plan-type board items. This unblocks #203 (removing plan items from boards) without breaking phase sync.

Changes

src/pal_e_docs/routes/boards.py -- Replaced the plan board item query with a direct Note query filtered by project_id and note_type='plan'. Removed the intermediate plan_item -> note_slug -> Note lookup chain. The phase discovery and board item create/update logic is unchanged.
tests/test_boards.py -- Added TestBoardSync class with 5 tests covering: sync without plan items on board, sync with plan items still present (backwards compat), column update on status change, skip unchanged phases, and empty project with no plans.

Test Plan

pytest tests/test_boards.py -v -- 71 tests pass (66 existing + 5 new)
Key new test: test_sync_without_plan_items_on_board -- creates plan/phase notes on a project, runs sync with zero plan board items, verifies phases are still created
Key new test: test_sync_with_plan_items_still_on_board -- backwards compatibility, sync works identically when plan items exist

Review Checklist

Only boards.py sync logic modified -- no CRUD or sync-issues changes
Backwards compatible -- works with or without plan items on board
All 71 tests pass (66 existing + 5 new)
ruff format + ruff check clean
No unrelated changes

Closes #208
Blocks: #203 (remove plan items from boards)

## Summary Refactors `POST /boards/{slug}/sync` to discover plan notes directly from the project (`WHERE note_type='plan' AND project_id=board.project_id`) instead of from plan-type board items. This unblocks #203 (removing plan items from boards) without breaking phase sync. ## Changes - `src/pal_e_docs/routes/boards.py` -- Replaced the plan board item query with a direct Note query filtered by `project_id` and `note_type='plan'`. Removed the intermediate `plan_item -> note_slug -> Note` lookup chain. The phase discovery and board item create/update logic is unchanged. - `tests/test_boards.py` -- Added `TestBoardSync` class with 5 tests covering: sync without plan items on board, sync with plan items still present (backwards compat), column update on status change, skip unchanged phases, and empty project with no plans. ## Test Plan - `pytest tests/test_boards.py -v` -- 71 tests pass (66 existing + 5 new) - Key new test: `test_sync_without_plan_items_on_board` -- creates plan/phase notes on a project, runs sync with zero plan board items, verifies phases are still created - Key new test: `test_sync_with_plan_items_still_on_board` -- backwards compatibility, sync works identically when plan items exist ## Review Checklist - [x] Only `boards.py` sync logic modified -- no CRUD or sync-issues changes - [x] Backwards compatible -- works with or without plan items on board - [x] All 71 tests pass (66 existing + 5 new) - [x] ruff format + ruff check clean - [x] No unrelated changes ## Related - Closes #208 - Blocks: #203 (remove plan items from boards)

forgejo_admin added 1 commit

2026-03-24 20:28:07 +00:00

refactor: sync_board discovers plans from project notes, not board items

ci/woodpecker/pr/woodpecker Pipeline failed

Details

6ff72745fb

Query plan notes directly from the project (note_type='plan',
project_id=board.project_id) instead of from plan board items. This
unblocks removing plan items from boards (#203) without breaking
phase sync.

Closes #208

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

forgejo_admin commented

2026-03-24 20:28:42 +00:00

Author

Owner

QA Review

Scope: 2 files changed, +153/-16 lines. Only sync_board logic modified -- CRUD and sync-issues untouched.

Code Quality

Query refactor is correct. The old path (BoardItem -> note_slug -> Note -> children) is replaced with a single direct query (Note WHERE project_id AND note_type='plan'). The phase discovery loop downstream is unchanged.
No dead imports. BoardItem and BoardItemType are still used elsewhere in the file.
Backwards compatible. The new query finds plans regardless of whether they have board items. Existing boards with plan items will continue to work -- sync just ignores those board items now.

Test Coverage

5 new tests covering all acceptance criteria from #208
test_sync_updates_column_on_status_change correctly uses direct DB manipulation to bypass the note update handler's auto-sync, isolating the sync endpoint's own column update logic
Edge cases covered: empty project, idempotent re-sync

Nits

None.

VERDICT: APPROVE

## QA Review **Scope:** 2 files changed, +153/-16 lines. Only `sync_board` logic modified -- CRUD and sync-issues untouched. ### Code Quality - **Query refactor is correct.** The old path (`BoardItem -> note_slug -> Note -> children`) is replaced with a single direct query (`Note WHERE project_id AND note_type='plan'`). The phase discovery loop downstream is unchanged. - **No dead imports.** `BoardItem` and `BoardItemType` are still used elsewhere in the file. - **Backwards compatible.** The new query finds plans regardless of whether they have board items. Existing boards with plan items will continue to work -- sync just ignores those board items now. ### Test Coverage - 5 new tests covering all acceptance criteria from #208 - `test_sync_updates_column_on_status_change` correctly uses direct DB manipulation to bypass the note update handler's auto-sync, isolating the sync endpoint's own column update logic - Edge cases covered: empty project, idempotent re-sync ### Nits None. **VERDICT: APPROVE**

forgejo_admin commented

2026-03-24 20:30:36 +00:00

Author

Owner

PR #210 Review

DOMAIN REVIEW

Stack: Python / FastAPI / SQLAlchemy (pal-e-docs API)

Correctness of the refactor

The core change is clean and minimal. The old pattern was:

Query BoardItem WHERE item_type='plan' AND board_id=board.id
For each plan item, resolve note_slug -> Note
Skip if no note_slug or no matching Note
Then query child phases from Note.parent_note_id

The new pattern:

Query Note WHERE project_id=board.project_id AND note_type='plan'
For each plan note, query child phases from Note.parent_note_id

This is a strictly better approach:

Eliminates the intermediate board item lookup (2 fewer queries per plan)
No null guards needed (plan_item.note_slug check, plan_note null check)
Plans no longer need to exist as board items for sync to work -- this directly unblocks #203

Query correctness: The filter Note.project_id == board.project_id correctly scopes to the same project. The Note.note_type == "plan" filter is a string comparison against a nullable string column -- this matches the model definition at /home/ldraney/pal-e-docs/src/pal_e_docs/models.py:101 where note_type is String, nullable=True.

Dead code removal: Confirmed zero remaining references to plan_items or plan_item in boards.py. The old query pattern is fully removed. No orphaned imports -- BoardItem, BoardItemType, Note, Board, BoardColumn, Project are all still used elsewhere in the file.

Backwards compatibility: The sync logic downstream (phase discovery, board item create/update, column mapping) is unchanged. The only change is how plan notes are discovered, not what happens with them.

SQLAlchemy patterns: The .filter().all() pattern is correct. No session leaks -- the existing db.commit() at the end of the function handles the transaction.

PEP compliance: Docstrings updated to reflect the new behavior. Type hints maintained. No style issues.

BLOCKERS

None.

NITS

Line 336 parenthesization: The query has an extra level of parentheses around the entire expression:
```
plan_notes = (
    db.query(Note).filter(Note.project_id == board.project_id, Note.note_type == "plan").all()
)
```
The .all() is inside the outer parens, which works but is slightly inconsistent with the rest of the file where .all() is typically on its own continuation line after the filter chain. This is cosmetic and non-blocking.
Test: test_sync_updates_column_on_status_change uses TestingSessionLocal directly (line 1275). This creates a separate session to modify the DB directly, which is a valid test technique for simulating out-of-band changes. The try/finally with session.close() is correct. However, the import of Note from pal_e_docs.models at line 1267 is inside the method body -- this is fine for test isolation but could be at the top of the test file. Not blocking since test files commonly do this.
No test for multiple plans in one project: The test suite covers 0 plans and 1 plan scenarios, but not a project with 2+ plan notes each having phase children. This would verify that the new query correctly discovers all plans. Low risk since the .all() on the query naturally handles multiple results, but it would be a good addition for completeness.

SOP COMPLIANCE

Branch named after issue: 208-sync-board-refactor (references issue #208)
PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present
Related references issue: Closes #208, and notes it blocks #203
No secrets committed
No unrelated file changes: exactly 2 files changed (boards.py, test_boards.py), both directly relevant
Tests exist: 5 new tests covering the key scenarios
Related references plan slug: No plan slug referenced -- this appears to be a standalone refactor issue rather than plan-driven work, which is acceptable per kanban workflow

PROCESS OBSERVATIONS

Change failure risk: Low. The refactor is a query source change, not a logic change. The downstream behavior (phase item create/update/skip) is identical. The 5 new tests provide a solid safety net.
Deployment frequency impact: Positive. This unblocks #203 (removing plan items from boards), which is a simplification that reduces board complexity.
Test coverage: 5 new tests cover: no-plan-items sync, with-plan-items backward compat, column update on status change, idempotent re-sync (skip unchanged), empty project edge case. This is thorough coverage for the change.
153 additions / 16 deletions: The ratio is appropriate -- most additions are test code (148 lines of tests vs 5 net lines of production code change).

VERDICT: APPROVED

Clean, minimal refactor that correctly changes the plan discovery strategy from board-item-based to project-note-based. No blockers. Test coverage is thorough. The change is backwards compatible and directly enables the next phase of work (#203).

## PR #210 Review ### DOMAIN REVIEW **Stack**: Python / FastAPI / SQLAlchemy (pal-e-docs API) **Correctness of the refactor** The core change is clean and minimal. The old pattern was: 1. Query `BoardItem` WHERE `item_type='plan'` AND `board_id=board.id` 2. For each plan item, resolve `note_slug` -> `Note` 3. Skip if no `note_slug` or no matching `Note` 4. Then query child phases from `Note.parent_note_id` The new pattern: 1. Query `Note` WHERE `project_id=board.project_id` AND `note_type='plan'` 2. For each plan note, query child phases from `Note.parent_note_id` This is a strictly better approach: - Eliminates the intermediate board item lookup (2 fewer queries per plan) - No null guards needed (`plan_item.note_slug` check, `plan_note` null check) - Plans no longer need to exist as board items for sync to work -- this directly unblocks #203 **Query correctness**: The filter `Note.project_id == board.project_id` correctly scopes to the same project. The `Note.note_type == "plan"` filter is a string comparison against a nullable string column -- this matches the model definition at `/home/ldraney/pal-e-docs/src/pal_e_docs/models.py:101` where `note_type` is `String, nullable=True`. **Dead code removal**: Confirmed zero remaining references to `plan_items` or `plan_item` in `boards.py`. The old query pattern is fully removed. No orphaned imports -- `BoardItem`, `BoardItemType`, `Note`, `Board`, `BoardColumn`, `Project` are all still used elsewhere in the file. **Backwards compatibility**: The sync logic downstream (phase discovery, board item create/update, column mapping) is unchanged. The only change is *how* plan notes are discovered, not *what happens* with them. **SQLAlchemy patterns**: The `.filter().all()` pattern is correct. No session leaks -- the existing `db.commit()` at the end of the function handles the transaction. **PEP compliance**: Docstrings updated to reflect the new behavior. Type hints maintained. No style issues. ### BLOCKERS None. ### NITS 1. **Line 336 parenthesization**: The query has an extra level of parentheses around the entire expression: ```python plan_notes = ( db.query(Note).filter(Note.project_id == board.project_id, Note.note_type == "plan").all() ) ``` The `.all()` is inside the outer parens, which works but is slightly inconsistent with the rest of the file where `.all()` is typically on its own continuation line after the filter chain. This is cosmetic and non-blocking. 2. **Test: `test_sync_updates_column_on_status_change` uses `TestingSessionLocal` directly** (line 1275). This creates a separate session to modify the DB directly, which is a valid test technique for simulating out-of-band changes. The `try/finally` with `session.close()` is correct. However, the import of `Note` from `pal_e_docs.models` at line 1267 is inside the method body -- this is fine for test isolation but could be at the top of the test file. Not blocking since test files commonly do this. 3. **No test for multiple plans in one project**: The test suite covers 0 plans and 1 plan scenarios, but not a project with 2+ plan notes each having phase children. This would verify that the new query correctly discovers all plans. Low risk since the `.all()` on the query naturally handles multiple results, but it would be a good addition for completeness. ### SOP COMPLIANCE - [x] Branch named after issue: `208-sync-board-refactor` (references issue #208) - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present - [x] Related references issue: `Closes #208`, and notes it blocks #203 - [x] No secrets committed - [x] No unrelated file changes: exactly 2 files changed (`boards.py`, `test_boards.py`), both directly relevant - [x] Tests exist: 5 new tests covering the key scenarios - [ ] Related references plan slug: No plan slug referenced -- this appears to be a standalone refactor issue rather than plan-driven work, which is acceptable per kanban workflow ### PROCESS OBSERVATIONS - **Change failure risk**: Low. The refactor is a query source change, not a logic change. The downstream behavior (phase item create/update/skip) is identical. The 5 new tests provide a solid safety net. - **Deployment frequency impact**: Positive. This unblocks #203 (removing plan items from boards), which is a simplification that reduces board complexity. - **Test coverage**: 5 new tests cover: no-plan-items sync, with-plan-items backward compat, column update on status change, idempotent re-sync (skip unchanged), empty project edge case. This is thorough coverage for the change. - **153 additions / 16 deletions**: The ratio is appropriate -- most additions are test code (148 lines of tests vs 5 net lines of production code change). ### VERDICT: APPROVED Clean, minimal refactor that correctly changes the plan discovery strategy from board-item-based to project-note-based. No blockers. Test coverage is thorough. The change is backwards compatible and directly enables the next phase of work (#203).