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..e209beab 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,104 @@ 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); + }); + + // 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(byValue); + const added = () => + 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 + * 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..a490d295 100644 --- a/api/src/services/role-assignment/role-assignment.service.ts +++ b/api/src/services/role-assignment/role-assignment.service.ts @@ -51,8 +51,77 @@ 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, + // 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 { + 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.error(e); + } + } + + for (const memberId of newIds) { + if (oldIds.has(memberId)) continue; + try { + await this.roleAssignmentRepository.addIdToAssignment(placeId, memberId, deputyRoleId); + } catch (e) { + console.error(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 {