forgejo_admin/pal-e-api

Fork 1

feat: propagate phase note title drift in sync_board #238

Merged

forgejo_admin merged 1 commit from 192-sync-board-title-drift into main

2026-03-28 18:48:21 +00:00

forgejo_admin commented

2026-03-28 18:44:21 +00:00

Owner

Summary

sync_board now detects when a phase board item's title has drifted from its linked note title and propagates the update. Follows the same drift-detection pattern already used by sync_issues. Non-phase items with manually set titles are not touched.

Changes

src/pal_e_docs/routes/boards.py: Added title drift detection to the existing phase item reconciliation loop in sync_board. When an existing phase board item's title differs from its linked note's title, the title is updated and the item counts toward updated. A changed flag (matching the sync_issues pattern) prevents unnecessary writes and correctly separates skipped vs updated.
tests/test_board_sync.py: Added 3 tests: (1) stale phase title is detected and updated, returning updated=1; (2) current phase title is skipped, returning skipped=3; (3) non-phase items with manual titles are not overwritten by sync.

Test Plan

Tests pass locally (pytest tests/test_board_sync.py -v -- 19 passed)
Full suite passes (pytest tests/ -v -- 685 passed)
ruff check clean
No regressions in existing board sync or column reconciliation behavior

Review Checklist

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

Closes #192
project-pal-e-docs -- the project this work belongs to

## Summary `sync_board` now detects when a phase board item's title has drifted from its linked note title and propagates the update. Follows the same drift-detection pattern already used by `sync_issues`. Non-phase items with manually set titles are not touched. ## Changes - `src/pal_e_docs/routes/boards.py`: Added title drift detection to the existing phase item reconciliation loop in `sync_board`. When an existing phase board item's title differs from its linked note's title, the title is updated and the item counts toward `updated`. A `changed` flag (matching the `sync_issues` pattern) prevents unnecessary writes and correctly separates `skipped` vs `updated`. - `tests/test_board_sync.py`: Added 3 tests: (1) stale phase title is detected and updated, returning updated=1; (2) current phase title is skipped, returning skipped=3; (3) non-phase items with manual titles are not overwritten by sync. ## Test Plan - [x] Tests pass locally (`pytest tests/test_board_sync.py -v` -- 19 passed) - [x] Full suite passes (`pytest tests/ -v` -- 685 passed) - [x] `ruff check` clean - [x] No regressions in existing board sync or column reconciliation behavior ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related Notes - Closes #192 - `project-pal-e-docs` -- the project this work belongs to

forgejo_admin added 1 commit

2026-03-28 18:44:21 +00:00

feat: propagate phase note title drift in sync_board 87832ec22f

sync_board now detects when a phase board item's title differs from its
linked note title and updates it, following the same drift pattern used
by sync_issues. Title-only changes increment the updated count; items
with current titles are skipped. Non-phase items are not touched.

Closes #192

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

forgejo_admin commented

2026-03-28 18:45:19 +00:00

Author

Owner

QA Review -- PR #238

Diff Analysis

Production code (src/pal_e_docs/routes/boards.py):

Adds a changed flag to the existing phase reconciliation block, matching the exact pattern used by sync_issues (lines 458-478).
Title drift check (existing.title != phase.title) is correct -- string comparison catches any title change.
Only phase items inside the for phase in child_phases loop are affected. Non-phase items (plan, issue, todo) are never entered into this code path, satisfying acceptance criterion 4.
The changed flag correctly gates updated vs skipped counters -- title-only drift increments updated, no-change increments skipped.
No unnecessary writes: if title already matches, no assignment occurs.

Tests (tests/test_board_sync.py):

test_sync_updates_stale_phase_title: Renames a phase note via PUT, re-syncs, asserts updated=1/skipped=2/created=0, then verifies the board item title matches the new note title. Covers the primary user story.
test_sync_skips_phase_with_current_title: Double-sync with no changes, asserts all 3 skipped. Confirms idempotency still holds with the new code path.
test_sync_does_not_overwrite_non_phase_titles: Manually renames a plan board item, syncs, verifies the plan item keeps its custom title. Confirms sync_board's phase-only scope.

Checklist

Matches sync_issues drift pattern (issue requirement)
All 4 acceptance criteria covered
All 3 test expectations met
685/685 tests pass (full suite)
ruff clean
No unrelated changes (2 files, 60 additions, 0 deletions)
No secrets
Commit message is descriptive with Closes #192

Nits

None.

VERDICT: APPROVED

## QA Review -- PR #238 ### Diff Analysis **Production code** (`src/pal_e_docs/routes/boards.py`): - Adds a `changed` flag to the existing phase reconciliation block, matching the exact pattern used by `sync_issues` (lines 458-478). - Title drift check (`existing.title != phase.title`) is correct -- string comparison catches any title change. - Only phase items inside the `for phase in child_phases` loop are affected. Non-phase items (plan, issue, todo) are never entered into this code path, satisfying acceptance criterion 4. - The `changed` flag correctly gates `updated` vs `skipped` counters -- title-only drift increments `updated`, no-change increments `skipped`. - No unnecessary writes: if title already matches, no assignment occurs. **Tests** (`tests/test_board_sync.py`): - `test_sync_updates_stale_phase_title`: Renames a phase note via PUT, re-syncs, asserts updated=1/skipped=2/created=0, then verifies the board item title matches the new note title. Covers the primary user story. - `test_sync_skips_phase_with_current_title`: Double-sync with no changes, asserts all 3 skipped. Confirms idempotency still holds with the new code path. - `test_sync_does_not_overwrite_non_phase_titles`: Manually renames a plan board item, syncs, verifies the plan item keeps its custom title. Confirms sync_board's phase-only scope. ### Checklist - [x] Matches `sync_issues` drift pattern (issue requirement) - [x] All 4 acceptance criteria covered - [x] All 3 test expectations met - [x] 685/685 tests pass (full suite) - [x] ruff clean - [x] No unrelated changes (2 files, 60 additions, 0 deletions) - [x] No secrets - [x] Commit message is descriptive with Closes #192 ### Nits None. **VERDICT: APPROVED**

forgejo_admin commented

2026-03-28 18:47:08 +00:00

Author

Owner

PR #238 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / pytest

Pattern alignment verified. The changed flag pattern in sync_board (lines 363-374 of boards.py) now mirrors the identical pattern in sync_issues (lines 466-488). Both use:

changed = False at loop top
Conditional field updates each set changed = True
if changed: updated += 1 / else: skipped += 1

This is clean, consistent, and the right structural choice.

Logic correctness. The diff adds exactly 7 net lines of logic (introducing changed flag, title comparison, and restructuring the updated/skipped branching). The behavior is correct:

Column drift alone -> updated
Title drift alone -> updated
Both column + title drift -> single updated (not double-counted)
Neither drift -> skipped
Non-phase items (plan, issue) are outside the phase loop entirely, so untouched -- confirmed by reading the fixture setup (plan item created at line 89-94 of test file) and the test at line 262.

No regressions. The restructuring of the if existing: block preserves the original column-update behavior exactly while adding the new title-update path. The else (create) branch is not touched.

BLOCKERS

None.

New functionality has test coverage: 3 tests covering the happy path (stale title updated), the skip path (current title skipped), and the boundary (non-phase items untouched).
No unvalidated user input -- this code reads from the database (phase.title) and writes to the database (existing.title). No external user input enters the drift detection path.
No secrets or credentials.
No DRY violations -- the pattern was intentionally copied from sync_issues as a parallel structure, not duplicated auth/security logic.

NITS

Docstring update (boards.py:325-330). The sync_board docstring mentions syncing "phase notes from plan notes" and describes column mapping, but does not mention the new title propagation behavior. A one-line addition like "Also propagates title drift from phase notes to their board items." would keep the docstring accurate.
Test docstring on test_sync_skips_phase_with_current_title. This test (line 250) is functionally identical to what the existing test_sync_column_idempotent test (if one exists) would do -- it verifies a double-sync with no changes yields all skips. The docstring is clear, but worth noting this also implicitly tests column idempotency, not just title idempotency.

SOP COMPLIANCE

Branch named after issue: 192-sync-board-title-drift references issue #192
PR body has Summary, Changes, Test Plan, Related sections
Related references project slug (project-pal-e-docs)
PR body references Closes #192
Tests exist: 3 new tests (19 total in test_board_sync.py, 685 full suite)
No secrets committed
No unnecessary file changes -- exactly 2 files modified, both directly relevant
Commit messages are descriptive

PROCESS OBSERVATIONS

Scope discipline: 60 additions, 0 deletions, 2 files. This is a textbook-sized PR for a single feature. Change failure risk is minimal.
Pattern reuse: The changed flag pattern from sync_issues was correctly identified and replicated, reducing cognitive load for future readers.
Test quality: The three tests cover the critical paths -- drift detected, no drift, and boundary (non-phase items). The boundary test is particularly valuable since it guards against future regressions where someone might accidentally expand drift detection to all item types.

VERDICT: APPROVED

## PR #238 Review ### DOMAIN REVIEW **Tech stack:** Python / FastAPI / SQLAlchemy / pytest **Pattern alignment verified.** The `changed` flag pattern in `sync_board` (lines 363-374 of `boards.py`) now mirrors the identical pattern in `sync_issues` (lines 466-488). Both use: 1. `changed = False` at loop top 2. Conditional field updates each set `changed = True` 3. `if changed: updated += 1` / `else: skipped += 1` This is clean, consistent, and the right structural choice. **Logic correctness.** The diff adds exactly 7 net lines of logic (introducing `changed` flag, title comparison, and restructuring the `updated`/`skipped` branching). The behavior is correct: - Column drift alone -> `updated` - Title drift alone -> `updated` - Both column + title drift -> single `updated` (not double-counted) - Neither drift -> `skipped` - Non-phase items (plan, issue) are outside the phase loop entirely, so untouched -- confirmed by reading the fixture setup (plan item created at line 89-94 of test file) and the test at line 262. **No regressions.** The restructuring of the `if existing:` block preserves the original column-update behavior exactly while adding the new title-update path. The `else` (create) branch is not touched. ### BLOCKERS None. - New functionality has test coverage: 3 tests covering the happy path (stale title updated), the skip path (current title skipped), and the boundary (non-phase items untouched). - No unvalidated user input -- this code reads from the database (phase.title) and writes to the database (existing.title). No external user input enters the drift detection path. - No secrets or credentials. - No DRY violations -- the pattern was intentionally copied from `sync_issues` as a parallel structure, not duplicated auth/security logic. ### NITS 1. **Docstring update (boards.py:325-330).** The `sync_board` docstring mentions syncing "phase notes from plan notes" and describes column mapping, but does not mention the new title propagation behavior. A one-line addition like "Also propagates title drift from phase notes to their board items." would keep the docstring accurate. 2. **Test docstring on `test_sync_skips_phase_with_current_title`.** This test (line 250) is functionally identical to what the *existing* `test_sync_column_idempotent` test (if one exists) would do -- it verifies a double-sync with no changes yields all skips. The docstring is clear, but worth noting this also implicitly tests column idempotency, not just title idempotency. ### SOP COMPLIANCE - [x] Branch named after issue: `192-sync-board-title-drift` references issue #192 - [x] PR body has Summary, Changes, Test Plan, Related sections - [x] Related references project slug (`project-pal-e-docs`) - [x] PR body references `Closes #192` - [x] Tests exist: 3 new tests (19 total in test_board_sync.py, 685 full suite) - [x] No secrets committed - [x] No unnecessary file changes -- exactly 2 files modified, both directly relevant - [x] Commit messages are descriptive ### PROCESS OBSERVATIONS - **Scope discipline:** 60 additions, 0 deletions, 2 files. This is a textbook-sized PR for a single feature. Change failure risk is minimal. - **Pattern reuse:** The `changed` flag pattern from `sync_issues` was correctly identified and replicated, reducing cognitive load for future readers. - **Test quality:** The three tests cover the critical paths -- drift detected, no drift, and boundary (non-phase items). The boundary test is particularly valuable since it guards against future regressions where someone might accidentally expand drift detection to all item types. ### VERDICT: APPROVED