Reconcile displayed roles after the writes, not between them

api/src/services/block/block.service.ts

@@ -82,22 +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.
-    if (oldOwner !== 0) {
-      await this.roleAssignmentRepository.removeIdFromAssignment(blockId, oldOwner, ownerCode);
-      await this.roleAssignmentService.reconcilePrimaryRole(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);
+    // 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<any> {

api/src/services/colony/colony.service.ts

@@ -78,22 +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.
-    if (oldOwner !== 0) {
-      await this.roleAssignmentRepository.removeIdFromAssignment(colonyId, oldOwner, ownerCode);
-      await this.roleAssignmentService.reconcilePrimaryRole(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);
+    // 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,
+    });
   }
 
   /**

api/src/services/hood/hood.service.ts

@@ -77,22 +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.
-    if (oldOwner !== 0) {
-      await this.roleAssignmentRepository.removeIdFromAssignment(hoodId, oldOwner, ownerCode);
-      await this.roleAssignmentService.reconcilePrimaryRole(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);
+    // 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<Place> {

api/src/services/place/place.service.ts

@@ -252,28 +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.
-    if (oldOwner !== 0) {
-      await this.roleAssignmentRepository.removeIdFromAssignment(placeId, oldOwner, ownerCode);
-      await this.roleAssignmentService.reconcilePrimaryRole(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);
+    // 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[] = [];
-    for (const givenDeputy of givenDeputies) {
-      newDeputyIds.push(await this.updateDeputyId(givenDeputy));
+    if (hasDeputyRole) {
+      for (const givenDeputy of givenDeputies) {
+        newDeputyIds.push(await this.updateDeputyId(givenDeputy));
+      }
     }
-    await this.roleAssignmentService
-      .syncDeputies(placeId, deputyCode, oldDeputyIds, newDeputyIds);
+    // 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<void> {

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<number>();
+      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<number>();
+      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,128 @@ 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);
+    });
+  });
+
+  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);
+    });
+  });
 });