Reconcile deputies by membership instead of by slot position

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<any> {

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);
   }
 
   /**

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<Place> {

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<void> {

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);
+    });
+  });
 });

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<void> {
+    if (deputyRoleId === undefined || deputyRoleId === null) return;
+
+    const asIdSet = (ids: number[]): Set<number> =>
+      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<any[]> {