pal-e-docs

Review: Add visibility column to notes and projects tables

review-1388-2026-07-03 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Re-review after previous NEEDS_REVISION (wrong repo). The issue body has been updated to correctly identify both repos and separate file targets. One remaining traceability gap: missing arch note.

Template Completeness

  • [x] Type -- Feature
  • [x] Lineage -- Related to paldocs #23 (auth spike)
  • [x] Repo -- ldraney/pal-e-api (Alembic migration) + ldraney/paldocs (ci_schema.sql)
  • [x] User Story
  • [x] Context -- References auth spike decision doc
  • [x] File Targets -- 3 in pal-e-api, 1 in paldocs (cross-repo, separate PR)
  • [x] Feature Flag -- none (correct, internal schema change)
  • [x] Acceptance Criteria -- 6 items
  • [x] Test Expectations -- unit tests + pytest command
  • [x] Constraints -- 4 constraints documented
  • [x] Checklist
  • [x] Related -- project + dependency chain

Traceability

  • [x] story:auth label present
  • [x] story note verified -- story-paldocs-auth exists in pal-e-docs and is listed in project-paldocs user-stories section
  • [x] arch:rails-db label present
  • [ ] arch note MISSING -- search for "arch-rails-db" returned empty. [SCOPE] Create architecture note arch-rails-db for the shared database layer (Alembic + Rails schema ownership)
  • [x] Forgejo issue -- ldraney/paldocs#27, state: open

File Targets

  • [x] src/pal_e_docs/models.py -- verified: Note.is_public at line 119, Project.is_public at line 66. No visibility field yet (correct, to be added).
  • [x] src/pal_e_docs/schemas.py -- verified: is_public in ProjectCreate, ProjectUpdate, ProjectOut, NoteCreate, NoteUpdate, NoteOut, NoteSummary, NoteSearchResult. No visibility field yet.
  • [x] alembic/versions/ -- directory exists with 4 existing migrations. New migration file will be created here.
  • [x] db/ci_schema.sql (paldocs) -- verified: is_public BOOLEAN at lines 11 and 24 for projects and notes tables respectively. No visibility column yet.

Repo Placement

FIXED (was the primary issue in previous review). Issue is filed under ldraney/paldocs but the issue body now explicitly states: "Repo: ldraney/pal-e-api (Alembic migration) + ldraney/paldocs (ci_schema.sql update)". File targets are separated by repo with cross-repo work noted as "separate PR". Filing under paldocs is acceptable since this is a paldocs auth feature tracked on board-paldocs.
Note: The arch:rails-db label was previously flagged as misleading (work is Alembic, not Rails). However, this label is an established convention on board-paldocs for shared database schema work -- used by 3 completed items including schema ownership transfer (#1559). The issue body is the agent spec, not the label, and the body is now explicit about Alembic.

Dependencies

Correctly documented in issue body:
  • Depends on: nothing (wave 1 -- confirmed, no blocking items on the board)
  • Blocks: paldocs #29 (Role-based visibility filtering in controllers) -- board item #1390, confirmed in backlog with sprint:3 label
  • Related: paldocs #28 (Rails OIDC) -- same sprint:3/story:auth, independent track (no dependency)

Acceptance Criteria

6 criteria, all agent-verifiable:
  • AC 1-2: Column existence -- verifiable via migration + introspection
  • AC 3: Backfill logic -- verifiable via unit test (is_public mapping)
  • AC 4-5: Model/schema updates -- verifiable by inspection and test run
  • AC 6: Existing tests pass -- verifiable via PALDOCS_DATABASE_PATH=:memory: pytest (correct command for pal-e-docs)
Test command is valid and matches the project's test convention.

Blast Radius

  • is_public is referenced in 20+ locations across 5 route modules (notes.py, projects.py, boards.py, blocks.py, links.py)
  • 67 test files reference is_public
  • paldocs Rails app has NO is_public references in app/ (clean -- Rails reads from shared DB but hasn't built visibility filtering yet)
  • Ticket correctly constrains scope: "Do not drop is_public yet" and "Files the agent should NOT touch: src/pal_e_docs/routes/"
  • Route-level changes are deferred to paldocs #29 (visibility filtering) -- good decomposition

Decomposition Assessment

3 file targets in pal-e-api + 1 in paldocs (cross-repo, separate PR). 6 acceptance criteria (borderline at >5 threshold). Estimated agent time: ~5 minutes. All AC are part of one atomic migration -- decomposing would be counterproductive. Cross-repo work is a trivial ci_schema.sql update. No decomposition needed.

Recommendation

  • [SCOPE] Create architecture note arch-rails-db documenting the shared database layer: Alembic as schema owner, Rails as consumer, CNPG cluster, migration conventions. This note should cover the dual-ownership model established by schema ownership transfer (#1559).
All other previous NEEDS_REVISION findings (wrong repo, missing repo clarity) have been resolved. The ticket is otherwise well-scoped and actionable.