pal-e-docs

Review: Add visibility column to notes and projects tables (re-review)

review-1388-2026-07-03-v2 Review

review ready
Project

Verdict: APPROVED

Re-review of board item #1388 (ldraney/paldocs#27). Previous review (review-1388-2026-07-03) returned NEEDS_REFINEMENT due to missing arch-rails-db backing note. That note has since been created with full content. All previous findings resolved.

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 -- story-paldocs-auth ("role-based access control via Keycloak")
  • [x] story note verified -- found in project-paldocs user-stories section
  • [x] arch:rails-db label -- shared database architecture component
  • [x] arch note verified -- arch-rails-db exists: "Database: paldocs (Shared CNPG, Schema Ownership, Dual Migrations)". Full content covers diagram, components table, key decisions on schema ownership, dual-migration rules, and cross-cutting deploy ordering. Active status, paldocs project.
  • [x] Forgejo issue -- ldraney/paldocs#27, state: open

File Targets

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

Repo Placement

OK. Issue is filed under ldraney/paldocs but the issue body explicitly states both repos. Primary work (Alembic migration, models, schemas) is in ldraney/pal-e-api. Cross-repo work (ci_schema.sql) is in ldraney/paldocs as a separate PR. Filing under paldocs is acceptable -- this is a paldocs auth feature tracked on board-paldocs. The arch:rails-db label is an established convention for shared database schema work on this board.

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
Test command is valid and matches the project's test convention.

Blast Radius

  • is_public is referenced in 28 locations across route modules in pal-e-api
  • paldocs Rails app has 0 is_public references in app/ (clean)
  • 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). However, all AC are part of one atomic migration -- column additions, backfill, and model/schema updates are tightly coupled. Decomposing would create coordination overhead without benefit. Cross-repo ci_schema.sql update is trivial. No decomposition needed.

Recommendation

No action needed. All previous NEEDS_REFINEMENT findings have been resolved. The arch-rails-db note now exists with comprehensive content covering the shared CNPG cluster, schema ownership model, and dual-migration conventions. The ticket is well-scoped, fully traceable, and ready for agent execution.