feat(territories): role-gated attribution per territory kind - #365
Merged
Conversation
Frees the TerritoryKind name for the entity table that will own per-kind configuration and, later, user-created kinds. Enum values and their mapped strings are unchanged, so Territory.type keeps its data — the migration is a Postgres type rename, not a data migration.
Each territory kind now declares which roles a publisher must hold to be attributed a territory of that kind. Closes the last open piece of the role-based permission epic besides the direct-grant cutover. Introduces the TerritoryKind entity rather than keying the join table on the enum, so the configuration survives kinds becoming user-created: that table grows a name/slug and drops the enum column, and the role links are untouched. Territory.type deliberately stays on TerritoryKindKey for now. The gate lives in the createAttribution / updateAttribution delegators, not in the aggregate. Those two are the human-initiated path, so posting straight at the route cannot slip past it, while campaign-lifecycle.workflow — which calls the aggregate directly — stays exempt: it re-attributes a pairing that already existed and swallows ConflictError, so gating it would silently strip a publisher's territory when a kind's roles change. An unchanged publisher on an edit also passes, so tightening a kind never locks an existing attribution. Refs #147
Review of #365 surfaced a fail-open path: kindKey was typed `string`, so a mistyped key found no kind, getKindAllowedRoleIds returned [] — which means "unrestricted" — and the gate silently passed. Every call site already holds a TerritoryKindKey (Territory.type, or Object.values in the settings action), so narrowing costs nothing now and the compiler rejects the typo instead. Widen it back to string when congregations can define their own kinds, not before. Also: - Fix an inaccurate comment: BUILT_IN_TERRITORY_KIND_KEYS holds the enum member names, not the @Map strings, and it contradicted the migration that says so. - Guard the hand-spelled kind-roles-* settings fields against drifting from the enum; without it, adding a kind silently drops its roles on every save. - Cover the unchanged-publisher edit path end-to-end, not just at unit level. - Drop two tests that asserted on delegation instead of observable outcomes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #147 — phase 5 of the role-based permission epic (#142).
Each territory kind now declares which roles a publisher must hold to be attributed a territory of that kind. The attribution pickers filter to matching publishers, and the server rejects a non-matching one posted directly at the route.
Two commits
chore(territories): rename TerritoryKind enum to TerritoryKindKey— mechanical, no behaviour change. Frees the name for the entity table. Enum values and their@mapstrings are unchanged, soTerritory.typekeeps its data and the migration is a Postgres type rename.feat(territories): role-gated attribution per territory kind— the feature.Design notes
The
TerritoryKindentity, not an enum-keyed join. #147 speccedTerritoryKindRole(kind, congregationId, roleId). Since kinds will eventually be user-created, a Prisma enum can't hold them, so this introduces the real table and hangs the role links off it: when custom kinds land, that table grows a name/slug and drops the enum column, and the join rows are untouched.Territory.typedeliberately stays onTerritoryKindKey— pointing it at the FK is a later change.The gate lives in the
createAttribution/updateAttributiondelegators, not in the aggregate. Those two are called only by the attribution routes, so a direct POST cannot slip past — whilecampaign-lifecycle.workflow, which callsattributionAggregate.assigndirectly, stays exempt by construction. That sweep re-attributes a pairing that already existed and swallowsConflictError, so gating it would silently strip a publisher's territory when a kind's roles change. An integration test guards that exemption.An unchanged publisher on an edit passes. Otherwise tightening a kind's roles would make every existing attribution unsavable. The edit picker keeps the current publisher listed for the same reason.
What landed
TerritoryKind+TerritoryKindAllowedRolewith RLS policies and a compound FK so a kind can never be linked across tenants. The migration seeds the five built-ins for existing congregations;seedBuiltInTerritoryKindscovers new ones, injected intoseedCongregationDefaultsthe wayseedTemplatesalready was.territory-kinds.server.ts,territory-kinds.queries.ts,attributable-publishers.queries.ts,attribution-eligibility.policy.ts. Eligibility resolves throughfindMembersWithAnyRole, the canonical helper that unions identity roles onMemberRoleAssignmentwith the account's custom roles.RolePicker; the Phone activation switch moved into its own row. ExtractingDurationInputalongside it leftsettings.tsxat 314 lines, under the 351 it was grandfathered at.IMPORT_TOTAL_STEPS41 → 43, caught by its own guard test.AuditAction.TerritoryKindAllowedRolesChanged, emitted with the added/removed diff.Verification
3153 unit tests, 334 integration tests, lint, typecheck, and all six architecture guards pass. 11 new integration tests cover persistence, RLS isolation across congregations, cascade when a role is deleted, seeding, and the campaign-sweep exemption.
Browser QA against the demo congregation: the picker dropped 45 publishers to the 5 elders on a restricted Phone territory; an unrestricted kind still listed everyone; a direct POST with a non-elder returned 409 with the field error and wrote nothing; an unchanged non-elder saved fine; changing to another non-elder was rejected.
Out of scope
phone-territory-active, read in 16 places); moving it ontoTerritoryKind.activeis a follow-up that has to rewire those call sites.Territory.typeat the new FK, and user-created kinds.Worth a follow-up
The settings page renders the full role chip list once per kind. On a congregation with ~20 roles that is a tall wall of chips — a collapsed-by-default row would read better.