fix/sites-polish
needs attentionviewing older commitc5ea5a5 · incrementalPR #290reviewed 2026-07-09 04:53 UTC1H · 2M · 3L · 1I- Purpose
- Polish the sites 1:N feature (PR #279 follow-up): fix assignee visibility loss when an RPU is moved to a new site, and replace the full-width Agrupar switch with a compact popover control.
- Goal
- Ensure site assignees are copied when a new site is created via the split/move flow, and that the copy source lookup is RLS-protected against cross-org probing.
- Sub-goals
- SG-1: Replace full-width 'Agrupar por sitio' Switch with a compact GroupControl popover (alongside Ordenar/Filtrar)
- SG-2: Implement assignee-copy on site creation — handler org-gates source, shell copies atomically via INSERT..SELECT ON CONFLICT DO NOTHING
- SG-3: RLS-wrap the source site lookup so a foreign-org public ID is invisible before the explicit org-check (this commit)
- What
- This commit moves `createRLSDb` before the assignee-copy source lookup in `createSiteHandler`, wrapping `findByPublicId` in an `rlsDb.transaction()`. It also adds 166 lines of integration tests for `copyAllToSite` covering N-assignee copy, idempotency, and empty-source no-op.
- Why
- Previous review (bf60412e) flagged TOCTOU: the source site was resolved with raw `database` (no RLS), so a foreign-org site could be resolved before the org-gate check. Integration tests were also missing for the copy mechanics.
- Areas
- apps/platform (UI + handler)+102−25domains/core (shell + tests)+209−3packages/api (schemas + types)+7−1
- Blast
- 11 files, +321/-28 lines across platform UI, handler, domain shell, and API schemas. Incremental diff: 2 files (+168/-4).
Findings · 7
correctness1
Narrow two-transaction window between source lookup and copy
apps/platform/src/api/handlers/sites.handler.ts:76
The source-site lookup now runs in `rlsDb.transaction()` (correct) but this transaction commits before `createSiteShell` opens its own transaction for `copyAllToSite`. If the source site is deleted between those two transactions, `copyAllToSite` silently returns 0 rows — indistinguishable from an empty-source no-op. Not a security issue; worst outcome is a silent empty copy. A comment noting the two-transaction split prevents future confusion. Long-term: passing the resolved publicId into the shell so it resolves-and-copies atomically would close this.
conventions1
RLS ordering is now correct — source lookup runs inside RLS transaction
apps/platform/src/api/handlers/sites.handler.ts:67
Moving `createRLSDb` before the first DB read is the correct pattern per rls-checklist.md (SET LOCAL binds per-transaction). The previous placement left `findByPublicId` executing against bare `database` with no RLS context. Fix is conformant.
tests5
Handler-level cross-org gate still not exercised
domains/core/src/site-assignee/__tests__/site-assignee.copy.integration.test.ts:1
The previous review flagged the org-gate check as untested. All three new cases operate within a single org, so the path where an actor supplies a `copyAssigneesFromSitePublicId` belonging to a different org is never exercised. The RLS wrap is now the primary enforcement, but the explicit `source.orgId !== organization.id` defense-in-depth check in the handler has no test. Add a handler-level or cross-org shell test: create a second org, create a source site in it, attempt copy with an actor scoped to org A, assert `SITE_NOT_FOUND`.
Test actor uses a phantom UUID as `creatorProfileId`
domains/core/src/site-assignee/__tests__/site-assignee.copy.integration.test.ts:27
`testActor.actorId = '00000000-0000-0000-0000-000000000003'` is passed as `creatorProfileId` to `organizationQueries.insert`, but no profile with that UUID is ever inserted. Currently harmless because the `organizations` table has no FK on `creator_profile_id`. If the schema tightens or the field is mapped, this test will fail with a FK violation rather than a descriptive error. Fix: set `creatorProfileId` to `profileAId` (inserted immediately after), or insert a dedicated actor profile first.
Idempotency test calls query directly, bypassing the shell under test
domains/core/src/site-assignee/__tests__/site-assignee.copy.integration.test.ts:148
The idempotency case calls `siteAssigneeQueries.copyAllToSite(database, ...)` directly rather than a second `createSiteShell` invocation. This proves the query's ON CONFLICT behaviour in isolation but does not verify that the shell itself is idempotent end-to-end (e.g. if the shell had additional side-effects on re-entry). Either rename the test to clarify it probes the query layer, or replace the direct query call with a second `mkSite(name, sourceSiteId)` targeting the same site.
No test for non-existent `copyAssigneesFromSiteId`
domains/core/src/site-assignee/__tests__/site-assignee.copy.integration.test.ts:1
None of the three cases supply a `copyAssigneesFromSiteId` that doesn't exist in the database. If the shell silently no-ops on a missing source (returns success with 0 rows), that silent data loss is undetected. Add a test that passes a random non-existent UUID and asserts the expected error tag (e.g. `SITE_NOT_FOUND`) or confirms the zero-row behavior is intentional.
`afterAll` swallows cleanup errors silently
domains/core/src/site-assignee/__tests__/site-assignee.copy.integration.test.ts:110
Multiple `try { ... } catch {}` blocks silence all cleanup errors. If cascade ordering is wrong or a constraint blocks a delete, rows survive across test runs and future runs see stale data without any CI signal. At minimum, log on cleanup failure: `catch (e) { console.warn('cleanup:', e.message) }`. Also verify the delete cascade order matches FK constraints.