Lane 2: access rights — wire jobs to places, add the role-grant axis, unify the hierarchy walk - #6
Open
DJAscendance wants to merge 6 commits into
Open
Lane 2: access rights — wire jobs to places, add the role-grant axis, unify the hierarchy walk#6DJAscendance wants to merge 6 commits into
DJAscendance wants to merge 6 commits into
Conversation
member.primary_role_id records which of a member's roles they display; role_assignment is the authority for what they actually hold. Keeping the two consistent was open-coded in 18 places across hood, block, colony, place and admin, and the shared logic was wrong in three ways. 1. It compared the *revoked* role against primary_role_id and nulled on a match. A member holding Block Leader and Neighborhood Deputy who lost Block Leader had their displayed role cleared despite still holding another. Reconciling against the assignments that remain fixes this. 2. The deputy loops were forEach callbacks containing un-awaited promise chains, so the write could land after the request had returned. Converted to for loops that await. 3. admin.fireRole inspected primary_role_id *before* deleting the assignment, deciding against state it was about to change. Now removes, then reconciles. Also hoists the old-owner removal out of the if/else in the four geographic services, which duplicated it identically in both branches, and adds memberRepository.getPrimaryRoleId -- getPrimaryRoleName INNER JOINs role, so it cannot distinguish "no role displayed" from "member not found". Adds five tests, including one covering the multi-role case from (1). No behaviour change intended beyond those three fixes.
roles (05/06/09) and places (02/03/04) were both seeded but nothing joined
them, so role_assignment was empty and no member held a place-scoped office.
That is why access rights looked broken: the table is correctly shaped --
(member_id, role_id, place_id), structurally the CS 4.x rolemember record --
and simply had no rows.
Synthetic fixtures, so permission behaviour is testable now; real officeholders
come later via the admin UI.
Roles resolve BY NAME, never by id. roles_data.json carries only
{name, income_xp, income_cc}, so ids come from auto-increment insert order and
hardcoding them would silently repoint every assignment if that file were ever
reordered. role has a UNIQUE(name) index, so name lookup is stable.
The fixture members cannot be logged into: their password column holds a bcrypt
hash of a random value discarded at authoring time, so no password matches.
Emails use the reserved .invalid TLD. They exist to hold roles, not to be used.
Verified against a throwaway MySQL 5.7: 53 assignments over 12 colonies,
6 hoods and 8 blocks; every role lands on the correct place type; the city-wide
City Guide role carries a null place_id; and re-running leaves counts unchanged
at 12 members / 53 assignments / 12 wallets.
CS 4.x gives every place two independent access axes. CTR had one. Axis 1, identity: an owner slot plus up to eight deputy slots, held in role_assignment and read via getAccessInfoByID. Already present. Axis 2, role grant: any role check-marked to grant write access to EVERY holder. Had no representation at all, so "let every City Guide write here" could not be expressed and place owners had to name eight individuals instead. That is a delegation ceiling, not a cosmetic gap. Adds place_role_access (place_id, role_id) with UNIQUE(place_id, role_id), a repository, and PlaceAccessService.canWrite resolving owner -> deputy -> role grant, in the original's order. The unconfigured default is OPEN, matching the shipped UI: if no nickname and no role is set, all members may write. canWrite therefore refuses a falsy member id outright -- the rule is that all MEMBERS may write, and a visitor carries only the Visitor bit, which satisfies nothing. Tested explicitly. Two deliberate omissions, both documented in the migration: - No capability bitfield (read/change/write/delete). Presence of a row means write, matching a UI that offers one checkbox per role. If capabilities are ever added they must carry the denial bookkeeping the original omitted: 4.1's delete branch recorded grants but never denials, so an explicit denial was indistinguishable from silence and fell through to the hierarchical walk, which could grant it from an ancestor. Reproduce the model, not the bug. - No foreign key on place_id. 04-places.hoods.seed.ts deletes and recreates every hood and block, and an FK would block that exactly as the vote_list FK already does. Orphans are swept by pruneOrphans instead. The hierarchical walk is still absent -- that is the next task. Verified against a throwaway MySQL 5.7 with the full migrate+seed chain: table built with the intended keys; granting City Guide at a block resolves that holder as 'role-grant' while an unrelated member and a visitor are both denied; setGrantedRoles replaces and dedupes; pruneOrphans removes dangling rows. Suite 20 -> 29 passing, same 5 pre-existing DB-dependent failures.
Authority inherits DOWN the place tree: a colony leader holds it over the hoods
and blocks beneath them without appearing in any of those places' owner or deputy
slots. That was already true in CTR, but written three times -- ColonyService,
HoodService and BlockService each open-coded the same logic at depths 1, 2 and 3
with hardcoded role sets, and a fourth place type would have needed a fourth copy.
Replaced by PlaceAccessService.getAncestry + hasGeographicAuthority, which walk
map_location and drive the per-level offices from a table keyed on place.type.
The three canAdmin methods now delegate. Behaviour is preserved: global Admin /
Colony Representative, or the Leader/Deputy pair for a level held at that level's
place.
Two things the shared version gets right that the copies did not:
- Non-recursing place types. The research notes are specific that city, office and
club places do not recurse, so the walk stops at a club rather than letting a
colony leader inherit into it. None of the three copies could express that.
- roleMap is no longer read directly. RoleRepository populates it from an
un-awaited constructor call, so for a window after startup it is `{}`, every
lookup is undefined, and `[undefined, ...].includes(role_id)` is false -- which
quietly DENIES real admins. Added RoleRepository.awaitRoleMap, which joins that
same population instead of racing it. Fails closed, so a correctness bug rather
than a security hole, but a confusing one.
canWrite now also consults inherited authority, reported as reason 'inherited'.
Deliberately NOT tightened: the original consults the hierarchy only when a
request was neither granted nor denied locally, so an explicit local denial stops
the walk. Here inherited authority applies even to a place with an access list the
member is absent from, matching what CTR's existing canAdmin already does.
Tightening it so a block owner could shut out their colony leader is a product
decision, not something a refactor should slip in.
canManageAccess is left alone in all three services: its role sets are
deliberately narrower (Leader but not Deputy), so it is a different question.
21 tests, including authority flowing down but not up, an office being scoped to
its own place, clubs not recursing, and cycle termination. Verified against a
throwaway MySQL 5.7 with real seeded geography: block 36 -> hood 35 -> colony 23
resolved from map_location; colony and hood leaders both reach the block; the
block leader does not reach the colony. Suite 29 -> 41 passing, same 5
pre-existing DB-dependent failures.
Three of the four findings from a local CodeRabbit pass. The fourth is a pre-existing bug in four services and is left for a decision rather than folded in here; see below. The fixture cleanup no longer deletes real accounts. FIXTURE_PREFIX is 'fixture_', and '_' is a single-character wildcard in LIKE, so `LIKE 'fixture_%'` also matched usernames like 'fixtures' or 'fixtureBob'. That query feeds a DELETE that takes the member row, their role_assignment rows and their wallet, so an over-match destroys a real account. The prefix is now escaped with an explicit ESCAPE clause. canManageAccess awaits roleMap in all three place services. It compares assignment.role_id against roleRepository.roleMap.Admin and friends, and roleMap is populated by an un-awaited constructor call -- so for a window after startup every lookup is undefined, `[undefined].includes(role_id)` is false, and a legitimate admin is quietly denied. canAdmin already got this fix by moving to placeAccessService; canManageAccess kept reading the map directly. Fixed with awaitRoleMap rather than by delegating to placeAccessService, because manage-access is deliberately narrower than canAdmin (Leader, not Deputy) and delegating would widen it. updatePlaceAccess no longer writes a deputy assignment for places that have no deputy role. findRoleIdsBySlug declared `deputy: number` but returns nothing for 'jail' and 'cityhall', which have an owner role and no deputy. The deputy sync would then call addIdToAssignment with an undefined role code, creating a role_assignment row pointing at no role. The sync is now skipped wholesale for those slugs -- a place with no deputy role has nothing to reconcile -- and the signature says `deputy?: number` so the next caller is told the truth by the compiler rather than by a bad row. Verified: tsc reports only the pre-existing missing 'sharp' module. eslint 0 errors. Suite compared against a stashed baseline suite-for- suite, not by count: identical, 41 passing and the same five failures, all MySQL connection errors needing a live database. Deliberately not done -- the fourth finding. The deputy reconciliation loop pairs old and new deputies BY INDEX, so it is order-sensitive: given old [A, B] and new [B, A], index 0 removes A and adds B, then index 1 removes B -- which just got added and is meant to stay -- and adds A. B loses the role and, worse, takes a reconcilePrimaryRole call while still a deputy, which is precisely the primary-role clearing this PR exists to fix. Set comparison is the correct model. Not fixed here because: it predates this PR (origin/master has the same pairing in a forEach), it is duplicated verbatim in block, hood, colony AND place, so fixing it properly means one shared helper plus tests rather than four edits, and it changes role_assignment write behaviour, which deserves its own reviewable commit rather than riding along in a review-response.
|
Warning Review limit reached
Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds place-scoped role access storage and repositories, a centralized authorization service with geographic inheritance, deterministic role-assignment fixtures, primary-role reconciliation, and integration across administrative, colony, hood, block, and place services. ChangesPlace access authorization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PlaceAccessService
participant RoleAssignmentRepository
participant PlaceRoleAccessRepository
participant MapLocationRepository
Client->>PlaceAccessService: canWrite(placeId, memberId, ownerCode, deputyCode)
PlaceAccessService->>RoleAssignmentRepository: Resolve owner and deputy access
PlaceAccessService->>PlaceRoleAccessRepository: Check granted roles
PlaceAccessService->>MapLocationRepository: Walk place ancestry
PlaceAccessService-->>Client: Return allowed and reason
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
api/src/services/place-access/place-access.service.ts (1)
221-234: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOptional: fold the two
place_role_accessreads into one.
memberHasGrantedRoleandgetRoleIdsByPlaceare separate round trips on the same table for the same place. Fetching the granted role ids once and intersecting with the member's assignments (already loaded insidehasGeographicAuthority) would cut a query on the deny path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/src/services/place-access/place-access.service.ts` around lines 221 - 234, Optionally refactor the access check around memberHasGrantedRole, hasGeographicAuthority, and getRoleIdsByPlace to fetch the place’s granted role IDs once, then reuse them to determine whether the member has a matching granted role and whether access is unconfigured. Preserve the existing allow/deny reasons and geographic-authority behavior while eliminating the duplicate place_role_access query.api/src/services/block/block.service.ts (1)
92-111: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winSwallowing per-deputy errors leaves access rights half-applied and reports success.
catch { console.log(e) }inside the loop means a failed removal still proceeds to the next index andpostAccessInforesolves normally, so the caller sees a successful save while some deputy slots were not updated. Consider collecting failures and rethrowing after the loop, or letting the error propagate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/src/services/block/block.service.ts` around lines 92 - 111, Update the per-deputy processing loop in the block service so errors from role removal, reconciliation, or assignment are not swallowed by the catch around the loop body. Let failures propagate immediately, or collect them and rethrow after all deputies are processed, ensuring postAccessInfo reports failure when any deputy update is incomplete.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/db/seed/11-role-assignments.seed.ts`:
- Around line 124-139: Move the colonies, hoods, and blocks queries plus the
no-colonies validation in the seed entry flow before removePreviousFixtures and
createFixtureMembers. Ensure prerequisite failure occurs before any fixture
deletion or recreation, while preserving the existing deterministic ordering and
limits.
In `@api/src/repositories/role/role.repository.ts`:
- Around line 10-34: Handle rejection from the constructor-started
populateRoleMap promise so startup failures do not become unhandled rejections.
Update awaitRoleMap to clear roleMapReady when population fails, then rethrow
the error so the current caller observes it and a later call can retry; preserve
successful memoization and shared in-flight behavior.
In `@api/src/services/block/block.service.ts`:
- Around line 77-91: Ensure role codes are resolved only after the role map is
initialized: in api/src/services/block/block.service.ts lines 77-91,
api/src/services/colony/colony.service.ts lines 73-81, and
api/src/services/hood/hood.service.ts lines 72-80, await
roleRepository.awaitRoleMap() at the start of postAccessInfo before resolving
ownerCode or deputyCode; in api/src/services/place/place.service.ts lines
304-309, await awaitRoleMap() inside findRoleIdsBySlug so all callers receive
initialized role IDs.
In `@api/src/services/place-access/place-access.service.ts`:
- Around line 204-213: Update canWrite to handle a missing or null deputyCode
before invoking roleAssignmentRepository.getAccessInfoByID, ensuring deputy
access is skipped and represented as an empty deputies result rather than
passing undefined into the repository query. Preserve the existing access
behavior when deputyCode is present.
In `@api/src/services/role-assignment/role-assignment.service.ts`:
- Around line 41-50: Update the assignment transfer flow in the block service so
reconcilePrimaryRole runs only after the replacement assignment has been
inserted, preferably within the same transaction as the removal and insertion.
Preserve the final primary-role state when a member moves the same role to a new
place, and add a regression test covering that transfer scenario.
---
Nitpick comments:
In `@api/src/services/block/block.service.ts`:
- Around line 92-111: Update the per-deputy processing loop in the block service
so errors from role removal, reconciliation, or assignment are not swallowed by
the catch around the loop body. Let failures propagate immediately, or collect
them and rethrow after all deputies are processed, ensuring postAccessInfo
reports failure when any deputy update is incomplete.
In `@api/src/services/place-access/place-access.service.ts`:
- Around line 221-234: Optionally refactor the access check around
memberHasGrantedRole, hasGeographicAuthority, and getRoleIdsByPlace to fetch the
place’s granted role IDs once, then reuse them to determine whether the member
has a matching granted role and whether access is unconfigured. Preserve the
existing allow/deny reasons and geographic-authority behavior while eliminating
the duplicate place_role_access query.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 36b0c308-2ca7-46fa-b500-e630b621630c
📒 Files selected for processing (19)
api/db/migrations/20260730130000_create_place_role_access.tsapi/db/seed/11-role-assignments.seed.tsapi/src/db/db.class.tsapi/src/repositories/index.tsapi/src/repositories/member/member.repository.tsapi/src/repositories/place-role-access/place-role-access.repository.tsapi/src/repositories/role/role.repository.tsapi/src/services/admin/admin.services.tsapi/src/services/block/block.service.tsapi/src/services/colony/colony.service.tsapi/src/services/hood/hood.service.tsapi/src/services/index.tsapi/src/services/place-access/place-access.service.spec.tsapi/src/services/place-access/place-access.service.tsapi/src/services/place/place.service.tsapi/src/services/role-assignment/role-assignment.service.spec.tsapi/src/services/role-assignment/role-assignment.service.tsapi/src/types/models/index.tsapi/src/types/models/place-role-access.model.ts
Comment on lines
+41
to
+50
| public async reconcilePrimaryRole(memberId: number): Promise<void> { | ||
| if (!memberId) return; | ||
| const current = await this.memberRepository.getPrimaryRoleId(memberId); | ||
| if (current === null || current === undefined) return; | ||
| const assignments = await this.roleAssignmentRepository.getByMemberId(memberId); | ||
| const stillHeld = assignments | ||
| .some(assignment => Number(assignment.role_id) === Number(current)); | ||
| if (!stillHeld) { | ||
| await this.memberRepository.update(memberId, { primary_role_id: null }); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make reconciliation atomic with the full assignment update.
The shown api/src/services/block/block.service.ts flow removes an assignment and calls this method before inserting its replacement. If the member keeps the same role at a new place, Lines 45-49 observe the temporary gap and clear primary_role_id even though the final state still holds that role. Reconcile only after the complete mutation set, preferably in the same transaction; add a transfer regression test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/src/services/role-assignment/role-assignment.service.ts` around lines 41
- 50, Update the assignment transfer flow in the block service so
reconcilePrimaryRole runs only after the replacement assignment has been
inserted, preferably within the same transaction as the removal and insertion.
Preserve the final primary-role state when a member moves the same role to a new
place, and add a regression test covering that transfer scenario.
Four of five findings from the CodeRabbit App review of #6. The fifth is a heavy lift that needs its own change; see below. awaitRoleMap no longer poisons itself. The constructor's population promise had no .catch(), so a transient database error at startup was an unobserved rejection -- a warning normally, fatal under --unhandled-rejections=throw. Worse, the rejected promise stayed memoized in roleMapReady, so every later awaitRoleMap re-awaited the same rejection and the process could not recover without a restart. That turned the canManageAccess fix from the previous commit into a liability: those call sites would throw rather than fail closed. Population is now started through a helper that clears the memo on failure so the next caller retries, with an identity check so a late-settling older attempt cannot clear a newer one's memo. The eager rejection is observed and discarded. Kept deliberately: awaitRoleMap still rejects rather than returning a half-empty map. Returning {} would put callers back on the silent-denial path this method exists to close, telling a real admin "no" instead of "could not determine". The rejection reaches the controllers' existing try/catch, and the cleared memo means the next request retries. Six role-code reads were awaiting a number. `await roleMap.BlockDeputy` awaits an already-resolved value and waits for nothing, so it read the unpopulated map exactly as a bare access would -- the await was pure decoration. getAccessInfoByUsername and postAccessInfo in all three place services now await the map itself. place.service is fixed at findRoleIdsBySlug, which is the single point where every slug's codes are resolved, so all four of its callers are covered by one await. getAccessInfoByID no longer throws for places with no deputy role. 'jail' (Security Chief) and 'cityhall' (City Council) have an owner role and no deputy, and `.where('role_id', undefined)` makes knex throw "Undefined binding(s) detected", taking down the owner lookup along with it. The deputy query is skipped instead. Guarded in the repository because every caller had the same exposure -- and because the guard added for this in the previous commit sat AFTER the getAccessInfoByID call in place.service, so it could never have been reached. The seed validates before it destroys. removePreviousFixtures and createFixtureMembers ran before the place queries, so seeding a database with no colonies deleted the existing fixtures and only then threw, leaving it emptier than a failed seed found it. Reads and validation now come first, so a failed precondition is a no-op. Verified: tsc reports only the pre-existing missing 'sharp' module. eslint 0 errors on the eight changed files. Suite compared against a stashed baseline test-name by test-name: no regressions, and three tests went from failing to passing -- MemberService > createMemberAndLogin > {should not store the provided member password in clear text, should return a session token for the new member, should tell the database to create a member with the provided name and email}. Those were being killed by the uncaught constructor rejection, which is independent evidence the first finding was real and reached past authorization. Totals 41 -> 44 passing, 5 -> 2 failing. Deliberately not done -- the fifth finding. reconcilePrimaryRole observes a temporary gap: the callers remove an assignment, reconcile, then insert the replacement, so a member who keeps a role at a DIFFERENT place has primary_role_id cleared even though the final state still holds it. The fix is to reconcile after the complete mutation set inside one transaction, which changes the shape of every caller and wants a transfer regression test. It also affects syncDeputies on #9, which removes and adds in two passes. Doing it here would mix a transaction boundary change into a review-response commit.
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
1 participant
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.
Lane 2 of the CS 4.1 → CTR access-rights work. Targets this fork's
master, deliberately notCybertownRevival/ctr— parking it here until the whole area is settled.Reference: the CS 4.1 research notes, "Access rights" (two axes, capability bits, resolution order, hierarchical inheritance).
The headline
role_assignmentwas correctly shaped and completely empty.Roles are seeded (05/06/09 — 74 + 76 rows with real income/XP). Places are seeded (02/03/04 — 21 places, 10 colonies, 98 hoods+blocks, original 16-hex ids preserved in
place.slug). Nothing joined them. Not one seed file referencedrole_assignment, so no member held a place-scoped office — which is why "jobs aren't wired to their places" and access rights looked broken.The table didn't need redesigning.
(member_id, role_id, place_id)is already structurally the CS 4.xrolememberrecord.Four commits
20e04dc— onereconcilePrimaryRole, replacing 18 copy-pasted blocksmember.primary_role_idrecords which of a member's roles they display;role_assignmentis the authority for what they hold. Keeping those consistent was open-coded in 18 places across hood, block, colony, place and admin — and the shared logic was wrong three ways:primary_role_idand nulled on a match. A member holding Block Leader and Neighborhood Deputy who lost Block Leader had their displayed role wiped despite still holding one. Now reconciles against the assignments that remain.forEachcallbacks containing un-awaited.then()chains, so the write could land after the request returned. Nowforloops that await.admin.fireRolehad inverted ordering — it inspectedprimary_role_idbefore deleting the assignment, deciding against state it was about to change.Net −21 lines.
5fc6caa— seedrole_assignment53 assignments across 12 colonies, 6 hoods, 8 blocks, plus 12 fixture members.
roles_data.jsoncarries only{name, income_xp, income_cc}, so ids come from auto-increment insert order — hardcoding them would silently repoint every assignment if that file were reordered.rolehasUNIQUE(name), so name lookup is stable..invalidTLD. They hold roles; they are not usable accounts.04-places.hoods.seed.tsalready deletes everymap_location).e514c60— the role-grant axis (place_role_access)CS 4.x gives every place two independent axes. CTR had one.
role_assignmentWithout axis 2, "let every City Guide write here" was inexpressible and owners had to name eight individuals. That's a delegation ceiling, not a cosmetic gap.
Adds
place_role_access (place_id, role_id)andPlaceAccessService.canWrite, resolving owner → deputy → role grant in the original's order.The default is OPEN, faithfully. The shipped UI's rule is that if no nickname and no role is set, all members may write. So
canWriterefuses a falsy member id outright — a visitor carries only the Visitor bit and satisfies nothing. Tested explicitly, because getting it wrong hands visitors write access to every unconfigured place.Two deliberate omissions, documented in the migration:
place_id. The hoods seed deletes and recreates every hood and block; an FK would break it exactly as thevote_listFK already does.pruneOrphans()sweeps instead.74a6481— one hierarchical walkAuthority inherits down the tree. That already worked, but was written three times — Colony/Hood/Block services each open-coded the same logic at depths 1/2/3 with hardcoded role sets. A fourth place type would have needed a fourth copy.
Now
getAncestry+hasGeographicAuthoritywalkmap_location, with per-level offices in a table keyed onplace.type. The threecanAdminmethods delegate. Behaviour preserved.Two things the copies could not do:
roleMapis no longer read directly.RoleRepositorypopulates it from an un-awaited constructor call, so for a window after startup it is{}, every lookup isundefined, and[undefined, …].includes(role_id)is false — which silently denies real admins. AddedawaitRoleMap(), which joins that population instead of racing it. Fails closed, so a correctness bug rather than a security hole, but a baffling one.Deliberately not done
canAdminalready does. Making a block owner able to shut out their colony leader is a product decision, flagged in-code at the spot where it would go.canManageAccessuntouched in all three services. Its role sets are deliberately narrower (Leader but not Deputy), so collapsing it would have quietly widened who can edit access rights.Verification
Every commit typechecks and lints clean (0 errors). Suite across the lane:
The 5 failures are pre-existing and unrelated — they need a live MySQL and fail identically on the merge base.
Also run against a throwaway MySQL 5.7 through the full migrate+seed chain, not just unit tests:
Two pre-existing bugs found, not fixed here
1.
migrate:latestcannot run on a fresh database.20260309032638_add_voting_tables.tsinserts a data row inside a migration — "Mayor Election 2026" withplace_idhardcoded to1— but places come from seeds, which run after migrations:Worse, the migration has no
hasTableguard, so the failed run leaves the threevote_*tables behind and the retry dies withER_TABLE_EXISTS_ERROR— hiding the real cause. The row also breaks03-places.colonies.seed.ts, which can't delete colonies while it references one. Anyone setting up CTR fresh hits this. Filed separately; the fix is to move the sample vote into a seed that resolves its place by slug.2. Place names carry unstripped HTML.
Edge Of<BR>Darknessabove is real seeded data — acleanBlockName()artifact. Relevant to making names editable.Summary by CodeRabbit
New Features
Bug Fixes
Tests