From 39fce380805ce9c924035a0f970d9872d279c3b5 Mon Sep 17 00:00:00 2001 From: Ryan Bundy Date: Thu, 30 Jul 2026 10:25:32 -0400 Subject: [PATCH 1/2] Reconcile deputies by membership instead of by slot position The deputy sync paired the old and new deputy lists BY INDEX, so it was order-sensitive. Given old [A, B] and new [B, A]: index 0 saw A != B, so it removed A and added B; index 1 then saw B != A, so it removed B -- which had just been added and was meant to stay -- and added A back. Net effect, B silently lost the deputy role. Worse, B took a reconcilePrimaryRole call while still a deputy, which is exactly the spurious primary-role clearing that reconcilePrimaryRole was added to prevent. Reordering the slots on the access-rights page was enough to trigger it; no field had to change. A deputy assignment means membership, not position, so the comparison is now between sets. A member in both lists is left alone, which also stops needless remove-then-re-add churn on rows that were never changing. The loop was duplicated verbatim in block, hood, colony and place, so this replaces four copies with one helper on RoleAssignmentService -- next to reconcilePrimaryRole, which already documents being extracted from the same four services plus admin. Each call site keeps only the part that is genuinely its own: which role id counts as deputy there. Behaviour deliberately preserved rather than tidied: - The eight-slot cap is kept, as RoleAssignmentService.DEPUTY_SLOTS, applied to incoming ids only. The fixed [0,0,0,0,0,0,0,0] arrays imposed it implicitly and dropping it would quietly widen how many deputies a place can be given, which is not a review-fix decision. - Old ids are NOT capped, so any deputy beyond the eighth still gets cleaned up if one somehow exists. - 0 remains the empty-slot sentinel and is filtered on both sides. - Failures stay caught per member, so one bad row does not abandon the rest of the reconciliation, as before. - The undefined-deputy-role guard added for 'jail' and 'cityhall' now lives in the helper too, so all four call sites get it rather than just place. Verified: 11 new tests covering reorder, add, remove, swap, the 0 sentinel, duplicate ids, the slot cap, a place with no deputy role, and continuing past a failed write. tsc reports only the pre-existing missing 'sharp' module. eslint 0 errors. Suite compared against the 6e3e131 baseline suite-for-suite rather than by count: the same five suites fail, all MySQL connection errors needing a live database, and passing tests go 41 -> 52. The reorder case is the one that would have failed before this change; it is written as a test of the helper, so it documents the fixed behaviour rather than reproducing the old loop. --- api/src/services/block/block.service.ts | 33 ++----- api/src/services/colony/colony.service.ts | 33 ++----- api/src/services/hood/hood.service.ts | 33 ++----- api/src/services/place/place.service.ts | 33 ++----- .../role-assignment.service.spec.ts | 95 +++++++++++++++++++ .../role-assignment.service.ts | 70 +++++++++++++- 6 files changed, 187 insertions(+), 110 deletions(-) diff --git a/api/src/services/block/block.service.ts b/api/src/services/block/block.service.ts index d6ec9200..b63a1654 100644 --- a/api/src/services/block/block.service.ts +++ b/api/src/services/block/block.service.ts @@ -68,8 +68,6 @@ export class BlockService { const ownerCode = roleMap.BlockLeader; let oldOwner = null; let newOwner = 0; - const oldDeputies = [0,0,0,0,0,0,0,0]; - const newDeputies = [0,0,0,0,0,0,0,0]; const data = await this .roleAssignmentRepository .getAccessInfoByID(blockId, ownerCode, deputyCode); @@ -93,32 +91,13 @@ export class BlockService { if (newOwner !== 0) { await this.roleAssignmentRepository.addIdToAssignment(blockId, newOwner, ownerCode); } - data.deputies.forEach((deputies, index) => { - oldDeputies[index] = deputies.member_id; - }); - for (let i = 0; i < givenDeputies.length; i++) { - newDeputies[i] = await this.updateDeputyId(givenDeputies[i]); - } - // Was a forEach containing un-awaited promise chains, so the primary-role write - // could land after the request had already returned. A for loop lets these await. - for (let index = 0; index < oldDeputies.length; index++) { - const oldDeputy = oldDeputies[index]; - const newDeputy = newDeputies[index]; - if (oldDeputy === newDeputy) continue; - try { - if (oldDeputy !== 0) { - await this.roleAssignmentRepository - .removeIdFromAssignment(blockId, oldDeputy, deputyCode); - await this.roleAssignmentService.reconcilePrimaryRole(oldDeputy); - } - if (newDeputy !== 0) { - await this.roleAssignmentRepository - .addIdToAssignment(blockId, newDeputy, deputyCode); - } - } catch (e) { - console.log(e); - } + 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); } 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 44e9f831..b4234ea2 100644 --- a/api/src/services/colony/colony.service.ts +++ b/api/src/services/colony/colony.service.ts @@ -64,8 +64,6 @@ export class ColonyService { const ownerCode = roleMap.ColonyLeader; let oldOwner = null; let newOwner = 0; - const oldDeputies = [0, 0, 0, 0, 0, 0, 0, 0]; - const newDeputies = [0, 0, 0, 0, 0, 0, 0, 0]; const data = await this .roleAssignmentRepository .getAccessInfoByID(colonyId, ownerCode, deputyCode); @@ -89,32 +87,13 @@ export class ColonyService { if (newOwner !== 0) { await this.roleAssignmentRepository.addIdToAssignment(colonyId, newOwner, ownerCode); } - data.deputies.forEach((deputies, index) => { - oldDeputies[index] = deputies.member_id; - }); - for (let i = 0; i < givenDeputies.length; i++) { - newDeputies[i] = await this.updateDeputyId(givenDeputies[i]); - } - // Was a forEach containing un-awaited promise chains, so the primary-role write - // could land after the request had already returned. A for loop lets these await. - for (let index = 0; index < oldDeputies.length; index++) { - const oldDeputy = oldDeputies[index]; - const newDeputy = newDeputies[index]; - if (oldDeputy === newDeputy) continue; - try { - if (oldDeputy !== 0) { - await this.roleAssignmentRepository - .removeIdFromAssignment(colonyId, oldDeputy, deputyCode); - await this.roleAssignmentService.reconcilePrimaryRole(oldDeputy); - } - if (newDeputy !== 0) { - await this.roleAssignmentRepository - .addIdToAssignment(colonyId, newDeputy, deputyCode); - } - } catch (e) { - console.log(e); - } + 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); } /** diff --git a/api/src/services/hood/hood.service.ts b/api/src/services/hood/hood.service.ts index 06c5a22e..61292347 100644 --- a/api/src/services/hood/hood.service.ts +++ b/api/src/services/hood/hood.service.ts @@ -63,8 +63,6 @@ export class HoodService { const ownerCode = roleMap.NeighborhoodLeader; let oldOwner = null; let newOwner = 0; - const oldDeputies = [0,0,0,0,0,0,0,0]; - const newDeputies = [0,0,0,0,0,0,0,0]; const data = await this .roleAssignmentRepository .getAccessInfoByID(hoodId, ownerCode, deputyCode); @@ -88,32 +86,13 @@ export class HoodService { if (newOwner !== 0) { await this.roleAssignmentRepository.addIdToAssignment(hoodId, newOwner, ownerCode); } - data.deputies.forEach((deputies, index) => { - oldDeputies[index] = deputies.member_id; - }); - for (let i = 0; i < givenDeputies.length; i++) { - newDeputies[i] = await this.updateDeputyId(givenDeputies[i]); - } - // Was a forEach containing un-awaited promise chains, so the primary-role write - // could land after the request had already returned. A for loop lets these await. - for (let index = 0; index < oldDeputies.length; index++) { - const oldDeputy = oldDeputies[index]; - const newDeputy = newDeputies[index]; - if (oldDeputy === newDeputy) continue; - try { - if (oldDeputy !== 0) { - await this.roleAssignmentRepository - .removeIdFromAssignment(hoodId, oldDeputy, deputyCode); - await this.roleAssignmentService.reconcilePrimaryRole(oldDeputy); - } - if (newDeputy !== 0) { - await this.roleAssignmentRepository - .addIdToAssignment(hoodId, newDeputy, deputyCode); - } - } catch (e) { - console.log(e); - } + 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); } 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 a0a83a85..ef9c60f2 100644 --- a/api/src/services/place/place.service.ts +++ b/api/src/services/place/place.service.ts @@ -238,8 +238,6 @@ export class PlaceService { const ownerCode = placeRoleId.owner; let oldOwner = null; let newOwner = 0; - const oldDeputies = [0, 0, 0, 0, 0, 0, 0, 0]; - const newDeputies = [0, 0, 0, 0, 0, 0, 0, 0]; const data = await this .roleAssignmentRepository .getAccessInfoByID(placeId, ownerCode, deputyCode); @@ -269,32 +267,13 @@ export class PlaceService { // 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; - data.deputies.forEach((deputies, index) => { - oldDeputies[index] = deputies.member_id; - }); - for (let i = 0; i < givenDeputies.length; i++) { - newDeputies[i] = await this.updateDeputyId(givenDeputies[i]); - } - // Was a forEach containing un-awaited promise chains, so the primary-role write - // could land after the request had already returned. A for loop lets these await. - for (let index = 0; index < oldDeputies.length; index++) { - const oldDeputy = oldDeputies[index]; - const newDeputy = newDeputies[index]; - if (oldDeputy === newDeputy) continue; - try { - if (oldDeputy !== 0) { - await this.roleAssignmentRepository - .removeIdFromAssignment(placeId, oldDeputy, deputyCode); - await this.roleAssignmentService.reconcilePrimaryRole(oldDeputy); - } - if (newDeputy !== 0) { - await this.roleAssignmentRepository - .addIdToAssignment(placeId, newDeputy, deputyCode); - } - } catch (e) { - console.log(e); - } + 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); } 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 ac596032..294d7bbb 100644 --- a/api/src/services/role-assignment/role-assignment.service.spec.ts +++ b/api/src/services/role-assignment/role-assignment.service.spec.ts @@ -81,4 +81,99 @@ describe('RoleAssignmentService', () => { }); }); }); + + describe('syncDeputies', () => { + const PLACE = 42; + const DEPUTY_ROLE = 20; + const A = 101; + const B = 102; + const C = 103; + + /** Nobody holds a primary role by default, so reconcilePrimaryRole is a no-op. */ + beforeEach(() => { + memberRepository.getPrimaryRoleId.mockResolvedValue(null); + roleAssignmentRepository.getByMemberId.mockResolvedValue([] as any); + }); + + const removed = () => + roleAssignmentRepository.removeIdFromAssignment.mock.calls.map(call => call[1]).sort(); + const added = () => + roleAssignmentRepository.addIdToAssignment.mock.calls.map(call => call[1]).sort(); + + /** + * The bug this method exists to fix. The old index-paired loop saw A != B at index 0 and + * so removed A and added B, then saw B != A at index 1 and removed B -- which it had just + * added and which was meant to stay. Membership is unchanged here, so nothing should move. + */ + it('does nothing when the same members are reordered', async () => { + await service.syncDeputies(PLACE, DEPUTY_ROLE, [A, B], [B, A]); + expect(roleAssignmentRepository.removeIdFromAssignment).not.toHaveBeenCalled(); + expect(roleAssignmentRepository.addIdToAssignment).not.toHaveBeenCalled(); + }); + + /** A reordered member must not have their displayed role reconciled away either. */ + it('does not reconcile the primary role of a member who stays a deputy', async () => { + await service.syncDeputies(PLACE, DEPUTY_ROLE, [A, B], [B, A]); + expect(memberRepository.getPrimaryRoleId).not.toHaveBeenCalled(); + }); + + it('removes only members who are no longer deputies', async () => { + await service.syncDeputies(PLACE, DEPUTY_ROLE, [A, B], [A]); + expect(removed()).toEqual([B]); + expect(added()).toEqual([]); + }); + + it('adds only members who were not deputies before', async () => { + await service.syncDeputies(PLACE, DEPUTY_ROLE, [A], [A, C]); + expect(added()).toEqual([C]); + expect(removed()).toEqual([]); + }); + + it('handles a straight swap', async () => { + await service.syncDeputies(PLACE, DEPUTY_ROLE, [A], [C]); + expect(removed()).toEqual([A]); + expect(added()).toEqual([C]); + }); + + it('reconciles the primary role of a removed deputy', async () => { + await service.syncDeputies(PLACE, DEPUTY_ROLE, [A], []); + expect(memberRepository.getPrimaryRoleId).toHaveBeenCalledWith(A); + }); + + /** 0 is the empty-slot sentinel the fixed eight-element arrays were filled with. */ + it('ignores the 0 sentinel on both sides', async () => { + await service.syncDeputies(PLACE, DEPUTY_ROLE, [A, 0, 0], [A, 0, 0, 0]); + expect(roleAssignmentRepository.removeIdFromAssignment).not.toHaveBeenCalled(); + expect(roleAssignmentRepository.addIdToAssignment).not.toHaveBeenCalled(); + }); + + /** The same person submitted twice is still one deputy, so one write. */ + it('deduplicates repeated ids', async () => { + await service.syncDeputies(PLACE, DEPUTY_ROLE, [], [C, C]); + expect(added()).toEqual([C]); + }); + + it('caps incoming deputies at the slot count', async () => { + const nine = [1, 2, 3, 4, 5, 6, 7, 8, 9]; + await service.syncDeputies(PLACE, DEPUTY_ROLE, [], nine); + expect(added()).toHaveLength(RoleAssignmentService.DEPUTY_SLOTS); + expect(added()).not.toContain(9); + }); + + /** 'jail' and 'cityhall' have an owner role and no deputy role. */ + it('does nothing when the place has no deputy role', async () => { + await service.syncDeputies(PLACE, undefined, [A], [C]); + expect(roleAssignmentRepository.removeIdFromAssignment).not.toHaveBeenCalled(); + expect(roleAssignmentRepository.addIdToAssignment).not.toHaveBeenCalled(); + }); + + /** One failing row must not abandon the rest of the reconciliation. */ + it('continues past a failed write', async () => { + roleAssignmentRepository.addIdToAssignment + .mockRejectedValueOnce(new Error('duplicate')) + .mockResolvedValue(undefined as any); + await service.syncDeputies(PLACE, DEPUTY_ROLE, [], [B, C]); + expect(roleAssignmentRepository.addIdToAssignment).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 6ac5f1d4..7ca8bf44 100644 --- a/api/src/services/role-assignment/role-assignment.service.ts +++ b/api/src/services/role-assignment/role-assignment.service.ts @@ -51,8 +51,74 @@ export class RoleAssignmentService { } /** - * Grabs all payments due to users from database 50 at a time and - * places them in response array sorts respone into highest cc payout + * How many deputy slots a place has. + * + * Comes from the `[0,0,0,0,0,0,0,0]` arrays that block, hood, colony and place each + * declared. Kept as an explicit cap on incoming deputies so this change does not quietly + * widen how many deputies a place can be given. + */ + public static readonly DEPUTY_SLOTS = 8; + + /** + * Brings a place's deputy assignments from one set of members to another. + * + * Replaces a loop that was duplicated verbatim in the block, hood, colony and place + * services, and which paired old against new BY INDEX -- so it was order-sensitive. + * Given old [A, B] and new [B, A], index 0 saw A != B and so removed A and added B; + * index 1 then saw B != A and removed B, which had just been added and was meant to + * stay. B lost the role, and took a reconcilePrimaryRole call while still a deputy -- + * exactly the spurious primary-role clearing that reconcilePrimaryRole exists to + * prevent. Reordering the slots in the UI was enough to trigger it. + * + * Membership, not position, is what a deputy assignment means, so the comparison is + * between sets: a member in both is left alone, which also means no needless + * remove/re-add churn on their row. + * + * Old ids are not capped, so deputies beyond the slot count still get cleaned up if + * they somehow exist; new ids are capped, matching the fixed arrays this replaces. + * 0 is the "empty slot" sentinel throughout this codebase and is not a member id. + * + * Failures are caught per member so one bad row does not abandon the rest of the + * reconciliation, which is what the loops it replaces did. + */ + public async syncDeputies( + placeId: number, + deputyRoleId: number, + oldDeputyIds: number[], + newDeputyIds: number[], + ): Promise { + if (deputyRoleId === undefined || deputyRoleId === null) return; + + const asIdSet = (ids: number[]): Set => + new Set(ids.map(Number).filter(id => Number.isInteger(id) && id !== 0)); + + const oldIds = asIdSet(oldDeputyIds); + const newIds = asIdSet(newDeputyIds.slice(0, RoleAssignmentService.DEPUTY_SLOTS)); + + for (const memberId of oldIds) { + if (newIds.has(memberId)) continue; + try { + await this.roleAssignmentRepository + .removeIdFromAssignment(placeId, memberId, deputyRoleId); + await this.reconcilePrimaryRole(memberId); + } catch (e) { + console.log(e); + } + } + + for (const memberId of newIds) { + if (oldIds.has(memberId)) continue; + try { + await this.roleAssignmentRepository.addIdToAssignment(placeId, memberId, deputyRoleId); + } catch (e) { + console.log(e); + } + } + } + + /** + * Grabs all payments due to users from database 50 at a time and + * places them in response array sorts respone into highest cc payout * then drops all other payouts to the same user */ public async getMembersDueRoleCredit(limit: number): Promise { From d1f6d02510f8ba4b014f6ea4d137dc2df820653f Mon Sep 17 00:00:00 2001 From: Ryan Bundy Date: Thu, 30 Jul 2026 11:05:23 -0400 Subject: [PATCH 2/2] Address Copilot review on deputy reconciliation Three findings, all in code added by this PR. syncDeputies' deputyRoleId is now typed `number | null | undefined`. The method already guarded for undefined and the specs already passed it, but the signature said `number` -- so the guard looked like dead defensive code and a call site passing the genuinely-optional deputy code would have been a type error under stricter settings. 'jail' and 'cityhall' have an owner role and no deputy role, so undefined is a real input, not a hypothetical. Caught errors go to console.error rather than console.log. These are failed role_assignment writes; logging them on stdout alongside ordinary output is how they get missed. The surrounding code uses console.log and this commit does not go change all of it, but new error paths should not add to that. The spec's sort helpers take a numeric comparator. `.sort()` with no comparator is lexicographic, so it orders 10 before 2. The current ids (101-103, and 1-8 in the slot-cap test) happen to sort the same either way, so the tests passed -- which is exactly why it was worth fixing before someone changes an id and gets a confusing failure. Verified: tsc clean apart from the pre-existing missing 'sharp' module. eslint 0 errors. 17/17 syncDeputies specs, 55 passing overall with the same 2 pre-existing failures. --- .../role-assignment/role-assignment.service.spec.ts | 9 +++++++-- .../services/role-assignment/role-assignment.service.ts | 9 ++++++--- 2 files changed, 13 insertions(+), 5 deletions(-) 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 294d7bbb..e209beab 100644 --- a/api/src/services/role-assignment/role-assignment.service.spec.ts +++ b/api/src/services/role-assignment/role-assignment.service.spec.ts @@ -95,10 +95,15 @@ describe('RoleAssignmentService', () => { roleAssignmentRepository.getByMemberId.mockResolvedValue([] as any); }); + // Numeric comparator: a bare .sort() is lexicographic, so it would put 10 before 2 and + // these assertions would start failing on ids that happen to differ in digit count. + const byValue = (a: number, b: number) => a - b; const removed = () => - roleAssignmentRepository.removeIdFromAssignment.mock.calls.map(call => call[1]).sort(); + roleAssignmentRepository.removeIdFromAssignment.mock.calls + .map(call => call[1]).sort(byValue); const added = () => - roleAssignmentRepository.addIdToAssignment.mock.calls.map(call => call[1]).sort(); + roleAssignmentRepository.addIdToAssignment.mock.calls + .map(call => call[1]).sort(byValue); /** * The bug this method exists to fix. The old index-paired loop saw A != B at index 0 and diff --git a/api/src/services/role-assignment/role-assignment.service.ts b/api/src/services/role-assignment/role-assignment.service.ts index 7ca8bf44..a490d295 100644 --- a/api/src/services/role-assignment/role-assignment.service.ts +++ b/api/src/services/role-assignment/role-assignment.service.ts @@ -83,7 +83,10 @@ export class RoleAssignmentService { */ public async syncDeputies( placeId: number, - deputyRoleId: number, + // Optional because 'jail' and 'cityhall' have an owner role and no deputy role, so + // findRoleIdsBySlug genuinely returns nothing for them. The guard below relied on that + // while the signature denied it. + deputyRoleId: number | null | undefined, oldDeputyIds: number[], newDeputyIds: number[], ): Promise { @@ -102,7 +105,7 @@ export class RoleAssignmentService { .removeIdFromAssignment(placeId, memberId, deputyRoleId); await this.reconcilePrimaryRole(memberId); } catch (e) { - console.log(e); + console.error(e); } } @@ -111,7 +114,7 @@ export class RoleAssignmentService { try { await this.roleAssignmentRepository.addIdToAssignment(placeId, memberId, deputyRoleId); } catch (e) { - console.log(e); + console.error(e); } } }