From 2a4219b0e4fec6822601f30da8199496d10ed560 Mon Sep 17 00:00:00 2001 From: Ryan Bundy Date: Thu, 30 Jul 2026 11:41:48 -0400 Subject: [PATCH 1/2] Reconcile displayed roles after the writes, not between them reconcilePrimaryRole reads role_assignment to decide whether a member's displayed role is still one they hold. The callers ran it between the remove and the add that replaced it, so it observed a state that never settled. The simplest case needs no transfer at all. postAccessInfo removed the owner's assignment, reconciled, and only then re-added it -- so re-saving an access page WITHOUT changing the owner cleared that owner's primary_role_id, because at the moment of the read they genuinely held nothing. They kept the role and lost the badge, and nothing in the request explained why. Editing a place's deputies was enough to strip its owner's displayed role. Every mutation now lands first and reconciliation happens once at the end: - syncDeputies collects the members it removed instead of reconciling inside the remove loop, and reconciles them after the add loop. - Given a collector, it defers to that instead, so a caller changing both the owner and the deputies reconciles the whole set once at the very end rather than once per axis. - reconcilePrimaryRoles is that final pass. It deduplicates, because the same member can be touched on more than one axis of one update -- losing a deputy slot while also being the outgoing owner -- and skips the 0 sentinel. - block, hood, colony and place collect the outgoing owner into that set rather than reconciling inline. place.service's deputy-less guard became a conditional instead of an early return. 'jail' and 'cityhall' have an owner role and no deputy role, and returning there would have skipped the owner's reconciliation along with the deputy sync -- reintroducing the same bug for exactly the two places the guard exists for. Verified: 7 new tests, 24 total on this service. The ordering guarantee is asserted directly via mock invocationCallOrder -- the first getPrimaryRoleId read must come after the last addIdToAssignment -- which fails on the previous code, where reconciliation ran inside the remove loop before any add. Plus the collector path, a member who merely changed position not being collected, dedupe, sentinel skipping, and continuing past a member that throws. tsc clean apart from the pre-existing missing 'sharp' module. eslint 0 errors. Full suite compared against a stashed baseline test-name by test-name: identical failures, no regressions, 55 -> 62 passing. Deliberately not done: this fixes ORDERING, not atomicity. The writes are still separate statements, so a concurrent request interleaving between them can still observe a gap. Closing that needs a knex transaction threaded from the service through RoleAssignmentRepository, which today takes no trx and reaches for this.db.knex directly -- a repository-wide signature change well beyond this fix. The ordering bug was the one that fired on a single request with no concurrency at all. --- api/src/services/block/block.service.ts | 9 ++- api/src/services/colony/colony.service.ts | 9 ++- api/src/services/hood/hood.service.ts | 9 ++- api/src/services/place/place.service.ts | 32 +++++---- .../role-assignment.service.spec.ts | 65 +++++++++++++++++++ .../role-assignment.service.ts | 57 ++++++++++++++-- 6 files changed, 159 insertions(+), 22 deletions(-) diff --git a/api/src/services/block/block.service.ts b/api/src/services/block/block.service.ts index b63a1654..6306bc1d 100644 --- a/api/src/services/block/block.service.ts +++ b/api/src/services/block/block.service.ts @@ -84,9 +84,13 @@ export class BlockService { } // Both branches previously removed the old owner identically, so the removal is // hoisted out rather than duplicated. + // Members whose displayed role may need re-checking once every write below has landed. + // Reconciling inline here cleared the owner's badge on a save that did not even change + // the owner: the assignment was removed, read as absent, and only then re-added. + const touched = new Set(); if (oldOwner !== 0) { await this.roleAssignmentRepository.removeIdFromAssignment(blockId, oldOwner, ownerCode); - await this.roleAssignmentService.reconcilePrimaryRole(oldOwner); + touched.add(oldOwner); } if (newOwner !== 0) { await this.roleAssignmentRepository.addIdToAssignment(blockId, newOwner, ownerCode); @@ -97,7 +101,8 @@ export class BlockService { newDeputyIds.push(await this.updateDeputyId(givenDeputy)); } await this.roleAssignmentService - .syncDeputies(blockId, deputyCode, oldDeputyIds, newDeputyIds); + .syncDeputies(blockId, deputyCode, oldDeputyIds, newDeputyIds, touched); + await this.roleAssignmentService.reconcilePrimaryRoles(touched); } public async getMapLocationAndPlaces(blockId: number): Promise { diff --git a/api/src/services/colony/colony.service.ts b/api/src/services/colony/colony.service.ts index b4234ea2..4c7a4699 100644 --- a/api/src/services/colony/colony.service.ts +++ b/api/src/services/colony/colony.service.ts @@ -80,9 +80,13 @@ export class ColonyService { } // Both branches previously removed the old owner identically, so the removal is // hoisted out rather than duplicated. + // Members whose displayed role may need re-checking once every write below has landed. + // Reconciling inline here cleared the owner's badge on a save that did not even change + // the owner: the assignment was removed, read as absent, and only then re-added. + const touched = new Set(); if (oldOwner !== 0) { await this.roleAssignmentRepository.removeIdFromAssignment(colonyId, oldOwner, ownerCode); - await this.roleAssignmentService.reconcilePrimaryRole(oldOwner); + touched.add(oldOwner); } if (newOwner !== 0) { await this.roleAssignmentRepository.addIdToAssignment(colonyId, newOwner, ownerCode); @@ -93,7 +97,8 @@ export class ColonyService { newDeputyIds.push(await this.updateDeputyId(givenDeputy)); } await this.roleAssignmentService - .syncDeputies(colonyId, deputyCode, oldDeputyIds, newDeputyIds); + .syncDeputies(colonyId, deputyCode, oldDeputyIds, newDeputyIds, touched); + await this.roleAssignmentService.reconcilePrimaryRoles(touched); } /** diff --git a/api/src/services/hood/hood.service.ts b/api/src/services/hood/hood.service.ts index 61292347..a3ac8e34 100644 --- a/api/src/services/hood/hood.service.ts +++ b/api/src/services/hood/hood.service.ts @@ -79,9 +79,13 @@ export class HoodService { } // Both branches previously removed the old owner identically, so the removal is // hoisted out rather than duplicated. + // Members whose displayed role may need re-checking once every write below has landed. + // Reconciling inline here cleared the owner's badge on a save that did not even change + // the owner: the assignment was removed, read as absent, and only then re-added. + const touched = new Set(); if (oldOwner !== 0) { await this.roleAssignmentRepository.removeIdFromAssignment(hoodId, oldOwner, ownerCode); - await this.roleAssignmentService.reconcilePrimaryRole(oldOwner); + touched.add(oldOwner); } if (newOwner !== 0) { await this.roleAssignmentRepository.addIdToAssignment(hoodId, newOwner, ownerCode); @@ -92,7 +96,8 @@ export class HoodService { newDeputyIds.push(await this.updateDeputyId(givenDeputy)); } await this.roleAssignmentService - .syncDeputies(hoodId, deputyCode, oldDeputyIds, newDeputyIds); + .syncDeputies(hoodId, deputyCode, oldDeputyIds, newDeputyIds, touched); + await this.roleAssignmentService.reconcilePrimaryRoles(touched); } public async getColony(hoodId: number): Promise { diff --git a/api/src/services/place/place.service.ts b/api/src/services/place/place.service.ts index ef9c60f2..355692d5 100644 --- a/api/src/services/place/place.service.ts +++ b/api/src/services/place/place.service.ts @@ -254,26 +254,34 @@ export class PlaceService { } // Both branches previously removed the old owner identically, so the removal is // hoisted out rather than duplicated. + // Members whose displayed role may need re-checking once every write below has landed. + // Reconciling inline here cleared the owner's badge on a save that did not even change + // the owner: the assignment was removed, read as absent, and only then re-added. + const touched = new Set(); if (oldOwner !== 0) { await this.roleAssignmentRepository.removeIdFromAssignment(placeId, oldOwner, ownerCode); - await this.roleAssignmentService.reconcilePrimaryRole(oldOwner); + touched.add(oldOwner); } if (newOwner !== 0) { await this.roleAssignmentRepository.addIdToAssignment(placeId, newOwner, ownerCode); } // 'jail' and 'cityhall' have an owner role but no deputy role, so findRoleIdsBySlug - // returns deputy: undefined for them. The sync below would then write a role_assignment - // whose role_id is undefined -- a row pointing at no role at all. Skipped wholesale - // rather than guarded per-branch: a place with no deputy role has no deputies to - // reconcile, so there is nothing for the loop to do either way. - if (deputyCode === undefined || deputyCode === null) return; - const oldDeputyIds = data.deputies.map(deputy => deputy.member_id); - const newDeputyIds: number[] = []; - for (const givenDeputy of givenDeputies) { - newDeputyIds.push(await this.updateDeputyId(givenDeputy)); + // returns deputy: undefined for them. The sync would then write a role_assignment whose + // role_id is undefined -- a row pointing at no role at all. Skipped wholesale rather + // than guarded per-branch: a place with no deputy role has no deputies to reconcile. + // + // A conditional rather than an early return, because the outgoing OWNER of such a place + // still needs reconciling and the pass that does it is below. + if (deputyCode !== undefined && deputyCode !== null) { + const oldDeputyIds = data.deputies.map(deputy => deputy.member_id); + const newDeputyIds: number[] = []; + for (const givenDeputy of givenDeputies) { + newDeputyIds.push(await this.updateDeputyId(givenDeputy)); + } + await this.roleAssignmentService + .syncDeputies(placeId, deputyCode, oldDeputyIds, newDeputyIds, touched); } - await this.roleAssignmentService - .syncDeputies(placeId, deputyCode, oldDeputyIds, newDeputyIds); + await this.roleAssignmentService.reconcilePrimaryRoles(touched); } public async updatePlaces(placeinfo: any): Promise { diff --git a/api/src/services/role-assignment/role-assignment.service.spec.ts b/api/src/services/role-assignment/role-assignment.service.spec.ts index e209beab..655576de 100644 --- a/api/src/services/role-assignment/role-assignment.service.spec.ts +++ b/api/src/services/role-assignment/role-assignment.service.spec.ts @@ -172,6 +172,35 @@ describe('RoleAssignmentService', () => { expect(roleAssignmentRepository.addIdToAssignment).not.toHaveBeenCalled(); }); + /** + * The ordering guarantee. reconcilePrimaryRole reads role_assignment to decide whether + * the displayed role is still held, so it must not run until every write has landed -- + * otherwise it observes a state that never settles. + */ + it('reconciles only after every add has landed', async () => { + await service.syncDeputies(PLACE, DEPUTY_ROLE, [A], [C]); + const lastAdd = Math.max( + ...roleAssignmentRepository.addIdToAssignment.mock.invocationCallOrder, + ); + const firstReconcileRead = Math.min( + ...memberRepository.getPrimaryRoleId.mock.invocationCallOrder, + ); + expect(firstReconcileRead).toBeGreaterThan(lastAdd); + }); + + it('defers reconciliation to a collector when given one', async () => { + const touched = new Set(); + await service.syncDeputies(PLACE, DEPUTY_ROLE, [A], [C], touched); + expect([...touched]).toEqual([A]); + expect(memberRepository.getPrimaryRoleId).not.toHaveBeenCalled(); + }); + + it('does not collect a member who merely moved position', async () => { + const touched = new Set(); + await service.syncDeputies(PLACE, DEPUTY_ROLE, [A, B], [B, A], touched); + expect([...touched]).toEqual([]); + }); + /** One failing row must not abandon the rest of the reconciliation. */ it('continues past a failed write', async () => { roleAssignmentRepository.addIdToAssignment @@ -181,4 +210,40 @@ describe('RoleAssignmentService', () => { expect(roleAssignmentRepository.addIdToAssignment).toHaveBeenCalledTimes(2); }); }); + + describe('reconcilePrimaryRoles', () => { + const MEMBER_A = 201; + const MEMBER_B = 202; + + beforeEach(() => { + memberRepository.getPrimaryRoleId.mockResolvedValue(null); + roleAssignmentRepository.getByMemberId.mockResolvedValue([] as any); + }); + + /** The same member can be touched on more than one axis of a single update. */ + it('reconciles each member once even when listed repeatedly', async () => { + await service.reconcilePrimaryRoles([MEMBER_A, MEMBER_A, MEMBER_B, MEMBER_A]); + expect(memberRepository.getPrimaryRoleId).toHaveBeenCalledTimes(2); + }); + + /** 0 is the empty-slot sentinel and is not a member id. */ + it('skips falsy ids', async () => { + await service.reconcilePrimaryRoles([0, MEMBER_A]); + expect(memberRepository.getPrimaryRoleId).toHaveBeenCalledTimes(1); + expect(memberRepository.getPrimaryRoleId).toHaveBeenCalledWith(MEMBER_A); + }); + + it('continues past a member that throws', async () => { + memberRepository.getPrimaryRoleId + .mockRejectedValueOnce(new Error('gone')) + .mockResolvedValue(null); + await service.reconcilePrimaryRoles([MEMBER_A, MEMBER_B]); + expect(memberRepository.getPrimaryRoleId).toHaveBeenCalledTimes(2); + }); + + it('accepts a Set as well as an array', async () => { + await service.reconcilePrimaryRoles(new Set([MEMBER_A, MEMBER_B])); + expect(memberRepository.getPrimaryRoleId).toHaveBeenCalledTimes(2); + }); + }); }); diff --git a/api/src/services/role-assignment/role-assignment.service.ts b/api/src/services/role-assignment/role-assignment.service.ts index a490d295..1aa2526a 100644 --- a/api/src/services/role-assignment/role-assignment.service.ts +++ b/api/src/services/role-assignment/role-assignment.service.ts @@ -24,9 +24,18 @@ export class RoleAssignmentService { /** * Clears member.primary_role_id if it is no longer a role the member holds. * - * Call after any change to a member's role assignments. role_assignment is the - * authority for what a member holds; primary_role_id only records which of those they - * chose to display, so it has to be re-checked whenever assignments change. + * Call after the COMPLETE set of assignment changes has landed, never between a remove + * and the add that replaces it. This method reads role_assignment to decide what is still + * held, so calling it mid-sequence lets it observe a state that never settles. The + * callers used to do exactly that: postAccessInfo removed the owner's assignment, + * reconciled, and only then re-added it -- so re-saving an access page WITHOUT changing + * the owner cleared that owner's displayed role, because at the moment of the read they + * genuinely held nothing. They kept the role and lost the badge, with nothing in the + * request to explain it. + * + * role_assignment is the authority for what a member holds; primary_role_id only records + * which of those they chose to display, so it has to be re-checked whenever assignments + * change. * * Replaces a block that was copy-pasted across the hood, block, colony, place and * admin services. That version compared the *revoked* role against primary_role_id @@ -89,6 +98,13 @@ export class RoleAssignmentService { deputyRoleId: number | null | undefined, oldDeputyIds: number[], newDeputyIds: number[], + /** + * Optional collector for members whose displayed role needs re-checking. Given one, this + * method defers reconciliation instead of doing it, so a caller that is also changing + * the owner can reconcile everything once after ALL of its writes rather than partway + * through. + */ + deferTo?: Set, ): Promise { if (deputyRoleId === undefined || deputyRoleId === null) return; @@ -98,12 +114,13 @@ export class RoleAssignmentService { const oldIds = asIdSet(oldDeputyIds); const newIds = asIdSet(newDeputyIds.slice(0, RoleAssignmentService.DEPUTY_SLOTS)); + const removed: number[] = []; for (const memberId of oldIds) { if (newIds.has(memberId)) continue; try { await this.roleAssignmentRepository .removeIdFromAssignment(placeId, memberId, deputyRoleId); - await this.reconcilePrimaryRole(memberId); + removed.push(memberId); } catch (e) { console.error(e); } @@ -117,6 +134,38 @@ export class RoleAssignmentService { console.error(e); } } + + // Reconciled only once every write above has landed. reconcilePrimaryRole reads + // role_assignment to decide whether the displayed role is still held, so running it + // between the removes and the adds let it observe a state that never settles -- see + // reconcilePrimaryRoles. + // + // When the caller passes a collector, reconciliation is deferred to it instead, so a + // place update that also changes the owner reconciles the whole set exactly once at the + // very end rather than once per axis. + if (deferTo) { + removed.forEach(memberId => deferTo.add(memberId)); + return; + } + await this.reconcilePrimaryRoles(removed); + } + + /** + * Reconciles several members' displayed roles, after all assignment writes are done. + * + * Deduplicated because the same member can be touched on more than one axis of a single + * update -- losing a deputy slot while also being the outgoing owner, say -- and + * reconciling them twice is just a second round trip to reach the same answer. + */ + public async reconcilePrimaryRoles(memberIds: Iterable): Promise { + for (const memberId of new Set(memberIds)) { + if (!memberId) continue; + try { + await this.reconcilePrimaryRole(memberId); + } catch (e) { + console.error(e); + } + } } /** From 702af0b1ea614c97a1ebc8e7523326e385808e44 Mon Sep 17 00:00:00 2001 From: Ryan Bundy Date: Thu, 30 Jul 2026 11:56:09 -0400 Subject: [PATCH 2/2] Move the place access sequence into one shared method CodeRabbit review of #10. The previous commit fixed the reconciliation ordering but left the corrected sequence -- collect touched, remove old owner, add new owner, syncDeputies deferring into the set, reconcile the set -- written out four times, once per place type. As the review put it, that is the same duplication that let the original bug go unfixed in one place while being fixed elsewhere. It was four chances to get an order-sensitive sequence subtly wrong, in code where getting it wrong is invisible: the request succeeds and a member quietly loses their badge. RoleAssignmentService.syncPlaceAccess now owns the order. Callers keep what is genuinely theirs -- which role ids count as owner and deputy there, and resolving submitted usernames to member ids -- and hand over the writes. place.service keeps a local check for whether the place has a deputy role at all, but only to skip pointless username lookups for 'jail' and 'cityhall'. The skip-the-deputy-half decision itself is the shared method's, so those places still get their owner swapped and reconciled. 6 new tests, 30 on this service. The headline one asserts the original bug directly: with the owner UNCHANGED, the first getPrimaryRoleId read must come after the last addIdToAssignment, via mock invocationCallOrder. Also covered: the outgoing owner reconciled after the incoming one is written, no owner writes when the place has none on either side, a deputy-less place still swapping and reconciling its owner, a member who is both outgoing owner and dropped deputy being reconciled exactly once, and both halves applying together. Verified: tsc CLEAN -- the missing 'sharp' module that failed on every branch all session was a genuinely absent dependency, declared in package.json and package-lock.json since the home-image work but never installed into the shared api/node_modules. Installed with --no-save so the tracked lockfile is untouched; every worktree symlinks that directory so all five branches typecheck clean now. Nothing else was hiding behind that error. eslint 0 errors. Suite 62 -> 76 passing, same 2 pre-existing failures. --- api/src/services/block/block.service.ts | 27 +++--- api/src/services/colony/colony.service.ts | 27 +++--- api/src/services/hood/hood.service.ts | 27 +++--- api/src/services/place/place.service.ts | 43 ++++----- .../role-assignment.service.spec.ts | 88 +++++++++++++++++++ .../role-assignment.service.ts | 45 ++++++++++ 6 files changed, 184 insertions(+), 73 deletions(-) diff --git a/api/src/services/block/block.service.ts b/api/src/services/block/block.service.ts index 6306bc1d..c26bfbdc 100644 --- a/api/src/services/block/block.service.ts +++ b/api/src/services/block/block.service.ts @@ -82,27 +82,22 @@ export class BlockService { newOwner = result[0].id; } } - // Both branches previously removed the old owner identically, so the removal is - // hoisted out rather than duplicated. - // Members whose displayed role may need re-checking once every write below has landed. - // Reconciling inline here cleared the owner's badge on a save that did not even change - // the owner: the assignment was removed, read as absent, and only then re-added. - const touched = new Set(); - if (oldOwner !== 0) { - await this.roleAssignmentRepository.removeIdFromAssignment(blockId, oldOwner, ownerCode); - touched.add(oldOwner); - } - if (newOwner !== 0) { - await this.roleAssignmentRepository.addIdToAssignment(blockId, newOwner, ownerCode); - } const oldDeputyIds = data.deputies.map(deputy => deputy.member_id); const newDeputyIds: number[] = []; for (const givenDeputy of givenDeputies) { newDeputyIds.push(await this.updateDeputyId(givenDeputy)); } - await this.roleAssignmentService - .syncDeputies(blockId, deputyCode, oldDeputyIds, newDeputyIds, touched); - await this.roleAssignmentService.reconcilePrimaryRoles(touched); + // Owner swap, deputy set and reconciliation are one sequence whose ORDER matters, so it + // lives in RoleAssignmentService rather than being re-implemented per place type. + await this.roleAssignmentService.syncPlaceAccess({ + placeId: blockId, + ownerRoleId: ownerCode, + deputyRoleId: deputyCode, + oldOwnerId: oldOwner, + newOwnerId: newOwner, + oldDeputyIds, + newDeputyIds, + }); } public async getMapLocationAndPlaces(blockId: number): Promise { diff --git a/api/src/services/colony/colony.service.ts b/api/src/services/colony/colony.service.ts index 4c7a4699..1982b90a 100644 --- a/api/src/services/colony/colony.service.ts +++ b/api/src/services/colony/colony.service.ts @@ -78,27 +78,22 @@ export class ColonyService { newOwner = result[0].id; } } - // Both branches previously removed the old owner identically, so the removal is - // hoisted out rather than duplicated. - // Members whose displayed role may need re-checking once every write below has landed. - // Reconciling inline here cleared the owner's badge on a save that did not even change - // the owner: the assignment was removed, read as absent, and only then re-added. - const touched = new Set(); - if (oldOwner !== 0) { - await this.roleAssignmentRepository.removeIdFromAssignment(colonyId, oldOwner, ownerCode); - touched.add(oldOwner); - } - if (newOwner !== 0) { - await this.roleAssignmentRepository.addIdToAssignment(colonyId, newOwner, ownerCode); - } const oldDeputyIds = data.deputies.map(deputy => deputy.member_id); const newDeputyIds: number[] = []; for (const givenDeputy of givenDeputies) { newDeputyIds.push(await this.updateDeputyId(givenDeputy)); } - await this.roleAssignmentService - .syncDeputies(colonyId, deputyCode, oldDeputyIds, newDeputyIds, touched); - await this.roleAssignmentService.reconcilePrimaryRoles(touched); + // Owner swap, deputy set and reconciliation are one sequence whose ORDER matters, so it + // lives in RoleAssignmentService rather than being re-implemented per place type. + await this.roleAssignmentService.syncPlaceAccess({ + placeId: colonyId, + ownerRoleId: ownerCode, + deputyRoleId: deputyCode, + oldOwnerId: oldOwner, + newOwnerId: newOwner, + oldDeputyIds, + newDeputyIds, + }); } /** diff --git a/api/src/services/hood/hood.service.ts b/api/src/services/hood/hood.service.ts index a3ac8e34..d7f6b0a1 100644 --- a/api/src/services/hood/hood.service.ts +++ b/api/src/services/hood/hood.service.ts @@ -77,27 +77,22 @@ export class HoodService { newOwner = result[0].id; } } - // Both branches previously removed the old owner identically, so the removal is - // hoisted out rather than duplicated. - // Members whose displayed role may need re-checking once every write below has landed. - // Reconciling inline here cleared the owner's badge on a save that did not even change - // the owner: the assignment was removed, read as absent, and only then re-added. - const touched = new Set(); - if (oldOwner !== 0) { - await this.roleAssignmentRepository.removeIdFromAssignment(hoodId, oldOwner, ownerCode); - touched.add(oldOwner); - } - if (newOwner !== 0) { - await this.roleAssignmentRepository.addIdToAssignment(hoodId, newOwner, ownerCode); - } const oldDeputyIds = data.deputies.map(deputy => deputy.member_id); const newDeputyIds: number[] = []; for (const givenDeputy of givenDeputies) { newDeputyIds.push(await this.updateDeputyId(givenDeputy)); } - await this.roleAssignmentService - .syncDeputies(hoodId, deputyCode, oldDeputyIds, newDeputyIds, touched); - await this.roleAssignmentService.reconcilePrimaryRoles(touched); + // Owner swap, deputy set and reconciliation are one sequence whose ORDER matters, so it + // lives in RoleAssignmentService rather than being re-implemented per place type. + await this.roleAssignmentService.syncPlaceAccess({ + placeId: hoodId, + ownerRoleId: ownerCode, + deputyRoleId: deputyCode, + oldOwnerId: oldOwner, + newOwnerId: newOwner, + oldDeputyIds, + newDeputyIds, + }); } public async getColony(hoodId: number): Promise { diff --git a/api/src/services/place/place.service.ts b/api/src/services/place/place.service.ts index 355692d5..ec8174d2 100644 --- a/api/src/services/place/place.service.ts +++ b/api/src/services/place/place.service.ts @@ -252,36 +252,29 @@ export class PlaceService { newOwner = result[0].id; } } - // Both branches previously removed the old owner identically, so the removal is - // hoisted out rather than duplicated. - // Members whose displayed role may need re-checking once every write below has landed. - // Reconciling inline here cleared the owner's badge on a save that did not even change - // the owner: the assignment was removed, read as absent, and only then re-added. - const touched = new Set(); - if (oldOwner !== 0) { - await this.roleAssignmentRepository.removeIdFromAssignment(placeId, oldOwner, ownerCode); - touched.add(oldOwner); - } - if (newOwner !== 0) { - await this.roleAssignmentRepository.addIdToAssignment(placeId, newOwner, ownerCode); - } // 'jail' and 'cityhall' have an owner role but no deputy role, so findRoleIdsBySlug - // returns deputy: undefined for them. The sync would then write a role_assignment whose - // role_id is undefined -- a row pointing at no role at all. Skipped wholesale rather - // than guarded per-branch: a place with no deputy role has no deputies to reconcile. - // - // A conditional rather than an early return, because the outgoing OWNER of such a place - // still needs reconciling and the pass that does it is below. - if (deputyCode !== undefined && deputyCode !== null) { - const oldDeputyIds = data.deputies.map(deputy => deputy.member_id); - const newDeputyIds: number[] = []; + // returns deputy: undefined for them. Resolving deputies for such a place is pointless + // work, and syncPlaceAccess skips the deputy half when no deputy role is given -- but it + // still performs the owner swap and the reconciliation, which those places do need. + const hasDeputyRole = deputyCode !== undefined && deputyCode !== null; + const oldDeputyIds = hasDeputyRole ? data.deputies.map(deputy => deputy.member_id) : []; + const newDeputyIds: number[] = []; + if (hasDeputyRole) { for (const givenDeputy of givenDeputies) { newDeputyIds.push(await this.updateDeputyId(givenDeputy)); } - await this.roleAssignmentService - .syncDeputies(placeId, deputyCode, oldDeputyIds, newDeputyIds, touched); } - await this.roleAssignmentService.reconcilePrimaryRoles(touched); + // Owner swap, deputy set and reconciliation are one sequence whose ORDER matters, so it + // lives in RoleAssignmentService rather than being re-implemented per place type. + await this.roleAssignmentService.syncPlaceAccess({ + placeId, + ownerRoleId: ownerCode, + deputyRoleId: deputyCode, + oldOwnerId: oldOwner, + newOwnerId: newOwner, + oldDeputyIds, + newDeputyIds, + }); } public async updatePlaces(placeinfo: any): Promise { diff --git a/api/src/services/role-assignment/role-assignment.service.spec.ts b/api/src/services/role-assignment/role-assignment.service.spec.ts index 655576de..d19d09cd 100644 --- a/api/src/services/role-assignment/role-assignment.service.spec.ts +++ b/api/src/services/role-assignment/role-assignment.service.spec.ts @@ -246,4 +246,92 @@ describe('RoleAssignmentService', () => { expect(memberRepository.getPrimaryRoleId).toHaveBeenCalledTimes(2); }); }); + + describe('syncPlaceAccess', () => { + const PLACE = 42; + const OWNER_ROLE = 18; + const DEPUTY_ROLE = 20; + const OWNER = 301; + const NEW_OWNER = 302; + const DEPUTY = 303; + + beforeEach(() => { + memberRepository.getPrimaryRoleId.mockResolvedValue(null); + roleAssignmentRepository.getByMemberId.mockResolvedValue([] as any); + }); + + const base = { + placeId: PLACE, + ownerRoleId: OWNER_ROLE, + deputyRoleId: DEPUTY_ROLE, + oldOwnerId: 0, + newOwnerId: 0, + oldDeputyIds: [] as number[], + newDeputyIds: [] as number[], + }; + + /** + * The bug this whole change exists for: re-saving an access page WITHOUT changing the + * owner used to clear that owner's displayed role, because the assignment was removed, + * read as absent, and only then put back. + */ + it('does not reconcile mid-swap when the owner is unchanged', async () => { + await service.syncPlaceAccess({ ...base, oldOwnerId: OWNER, newOwnerId: OWNER }); + const lastWrite = Math.max( + ...roleAssignmentRepository.addIdToAssignment.mock.invocationCallOrder, + ); + const firstRead = Math.min( + ...memberRepository.getPrimaryRoleId.mock.invocationCallOrder, + ); + expect(firstRead).toBeGreaterThan(lastWrite); + }); + + it('reconciles the outgoing owner after the incoming one is written', async () => { + await service.syncPlaceAccess({ ...base, oldOwnerId: OWNER, newOwnerId: NEW_OWNER }); + expect(memberRepository.getPrimaryRoleId).toHaveBeenCalledWith(OWNER); + const lastWrite = Math.max( + ...roleAssignmentRepository.addIdToAssignment.mock.invocationCallOrder, + ); + expect( + Math.min(...memberRepository.getPrimaryRoleId.mock.invocationCallOrder), + ).toBeGreaterThan(lastWrite); + }); + + it('skips the owner writes entirely when the place has no owner either side', async () => { + await service.syncPlaceAccess(base); + expect(roleAssignmentRepository.removeIdFromAssignment).not.toHaveBeenCalled(); + expect(roleAssignmentRepository.addIdToAssignment).not.toHaveBeenCalled(); + }); + + /** 'jail' and 'cityhall': an owner role and no deputy role. */ + it('still swaps and reconciles the owner when there is no deputy role', async () => { + await service.syncPlaceAccess({ + ...base, deputyRoleId: undefined, oldOwnerId: OWNER, newOwnerId: NEW_OWNER, + }); + expect(roleAssignmentRepository.addIdToAssignment) + .toHaveBeenCalledWith(PLACE, NEW_OWNER, OWNER_ROLE); + expect(memberRepository.getPrimaryRoleId).toHaveBeenCalledWith(OWNER); + }); + + /** One member on two axes at once must not be reconciled twice. */ + it('reconciles a member who is both outgoing owner and dropped deputy only once', + async () => { + await service.syncPlaceAccess({ + ...base, oldOwnerId: OWNER, newOwnerId: NEW_OWNER, oldDeputyIds: [OWNER], + }); + const reads = memberRepository.getPrimaryRoleId.mock.calls + .filter(call => call[0] === OWNER); + expect(reads).toHaveLength(1); + }); + + it('applies both the owner swap and the deputy set', async () => { + await service.syncPlaceAccess({ + ...base, oldOwnerId: OWNER, newOwnerId: NEW_OWNER, newDeputyIds: [DEPUTY], + }); + expect(roleAssignmentRepository.addIdToAssignment) + .toHaveBeenCalledWith(PLACE, NEW_OWNER, OWNER_ROLE); + expect(roleAssignmentRepository.addIdToAssignment) + .toHaveBeenCalledWith(PLACE, DEPUTY, DEPUTY_ROLE); + }); + }); }); diff --git a/api/src/services/role-assignment/role-assignment.service.ts b/api/src/services/role-assignment/role-assignment.service.ts index 1aa2526a..4e012e5c 100644 --- a/api/src/services/role-assignment/role-assignment.service.ts +++ b/api/src/services/role-assignment/role-assignment.service.ts @@ -150,6 +150,51 @@ export class RoleAssignmentService { await this.reconcilePrimaryRoles(removed); } + /** + * Applies a place's whole access update: owner swap, deputy set, then reconciliation. + * + * The ordering here is the point. reconcilePrimaryRole must not run until every write has + * landed, and getting that wrong is invisible -- the request succeeds and a member quietly + * loses their displayed role. Keeping the sequence in one place means the block, hood, + * colony and place services cannot each get it subtly differently, which is how the + * original bug survived in four copies while being fixed in none. + * + * Callers still own resolving usernames to ids; this owns the order of the writes. + */ + public async syncPlaceAccess(params: { + placeId: number; + ownerRoleId: number; + /** Absent for places with an owner role and no deputy role ('jail', 'cityhall'). */ + deputyRoleId?: number | null; + /** 0 when the place currently has no owner. */ + oldOwnerId: number; + /** 0 when the update leaves the place without an owner. */ + newOwnerId: number; + oldDeputyIds: number[]; + newDeputyIds: number[]; + }): Promise { + const { + placeId, ownerRoleId, deputyRoleId, oldOwnerId, newOwnerId, oldDeputyIds, newDeputyIds, + } = params; + + // Members whose displayed role may need re-checking once every write below has landed. + const touched = new Set(); + + if (oldOwnerId !== 0) { + await this.roleAssignmentRepository + .removeIdFromAssignment(placeId, oldOwnerId, ownerRoleId); + touched.add(oldOwnerId); + } + if (newOwnerId !== 0) { + await this.roleAssignmentRepository.addIdToAssignment(placeId, newOwnerId, ownerRoleId); + } + + // Defers into `touched` rather than reconciling, so the pass below is the only one. + await this.syncDeputies(placeId, deputyRoleId, oldDeputyIds, newDeputyIds, touched); + + await this.reconcilePrimaryRoles(touched); + } + /** * Reconciles several members' displayed roles, after all assignment writes are done. *