Merge pull request 'refactor(MemberService): extract generic syncSubEntities() method' (#204) from fix/duplicate-sync-methods into main

.plans/done/issue-204-duplicate-sync-methods.md

@@ -46,29 +46,17 @@ private function syncAddresses(int $memberId, array $incoming): void {
 
 ## Solution
 
-Create a generic sync utility:
+Extract the common sync logic into a single private generic method that accepts callables for the mapper-specific operations. Each of the three sync methods becomes a thin 10-line wrapper calling the generic method.
-
-```php
-private function syncSubEntities(
-    int $memberId,
-    array $incoming,
-    callable $findByMemberId,
-    callable $add,
-    callable $update,
-    callable $delete
-): void {
-    // Generic sync logic
-}
-```
-
-Or create a `SubEntitySyncService` for reusability across entities.
 
 ## Tasks
 
-- [ ] Extract common sync logic into a generic method or service
+- [ ] Extract common sync logic into `syncSubEntities()` private generic method
 - [ ] Refactor `syncAddresses()`, `syncPhones()`, `syncEmails()` to use the new pattern
-- [ ] Ensure identical behavior is preserved (test thoroughly)
+- [ ] Ensure identical behavior is preserved (all existing tests must pass)
-- [ ] Document the generic sync algorithm
+- [ ] Run `make test` — all must pass
+- [ ] Commit, push, create PR via tea, merge it
+- [ ] Move plan file from `.plans/open/` to `.plans/done/`
+- [ ] Check out main again
 
 ## Labels
 

lib/Service/MemberService.php

@@ -555,6 +555,51 @@ class MemberService {
         return $this->find($id);
     }
 
+    /**
+     * Generic sub-entity sync: update existing, create new, delete removed.
+     *
+     * @param int $memberId
+     * @param array[] $incoming Payload arrays (each may contain an 'id' key)
+     * @param callable(): list<object> $fetchExisting Fetch current DB entities for this member
+     * @param callable(object): int $getId Extract entity ID
+     * @param callable(int, array): void $updateEntity Update an existing entity by ID
+     * @param callable(int, array): void $createEntity Create a new entity for a member
+     * @param callable(int): void $deleteEntity Delete an entity by ID
+     * @throws Exception
+     */
+    private function syncSubEntities(
+        int $memberId,
+        array $incoming,
+        callable $fetchExisting,
+        callable $getId,
+        callable $updateEntity,
+        callable $createEntity,
+        callable $deleteEntity
+    ): void {
+        $existing = $fetchExisting();
+        $existingById = [];
+        foreach ($existing as $entity) {
+            $existingById[$getId($entity)] = $entity;
+        }
+
+        $keptIds = [];
+        foreach ($incoming as $item) {
+            $id = isset($item['id']) ? (int)$item['id'] : 0;
+            if ($id > 0 && isset($existingById[$id])) {
+                $updateEntity($id, $item);
+                $keptIds[$id] = true;
+            } else {
+                $createEntity($memberId, $item);
+            }
+        }
+
+        foreach (array_keys($existingById) as $id) {
+            if (!isset($keptIds[$id])) {
+                $deleteEntity($id);
+            }
+        }
+    }
+
     /**
      * Sync a member's addresses against an incoming array.
      * Rules:
@@ -566,28 +611,15 @@ class MemberService {
      * @throws Exception
      */
     private function syncAddresses(int $memberId, array $incoming): void {
-        $existing = $this->addressMapper->findByMemberId($memberId);
+        $this->syncSubEntities(
-        $existingById = [];
+            $memberId,
-        foreach ($existing as $addr) {
+            $incoming,
-            $existingById[$addr->getId()] = $addr;
+            fn() => $this->addressMapper->findByMemberId($memberId),
-        }
+            fn($e) => $e->getId(),
-
+            fn($id, $data) => $this->updateAddress($id, $data),
-        $keptIds = [];
+            fn($mid, $data) => $this->addAddress($mid, $data),
-        foreach ($incoming as $item) {
+            fn($id) => $this->deleteAddress($id)
-            $id = isset($item['id']) ? (int)$item['id'] : 0;
+        );
-            if ($id > 0 && isset($existingById[$id])) {
-                $this->updateAddress($id, $item);
-                $keptIds[$id] = true;
-            } else {
-                $this->addAddress($memberId, $item);
-            }
-        }
-
-        foreach ($existingById as $id => $addr) {
-            if (!isset($keptIds[$id])) {
-                $this->deleteAddress($id);
-            }
-        }
     }
 
     /**
@@ -597,28 +629,15 @@ class MemberService {
      * @throws Exception
      */
     private function syncPhones(int $memberId, array $incoming): void {
-        $existing = $this->phoneMapper->findByMemberId($memberId);
+        $this->syncSubEntities(
-        $existingById = [];
+            $memberId,
-        foreach ($existing as $p) {
+            $incoming,
-            $existingById[$p->getId()] = $p;
+            fn() => $this->phoneMapper->findByMemberId($memberId),
-        }
+            fn($e) => $e->getId(),
-
+            fn($id, $data) => $this->updatePhone($id, $data),
-        $keptIds = [];
+            fn($mid, $data) => $this->addPhone($mid, $data),
-        foreach ($incoming as $item) {
+            fn($id) => $this->deletePhone($id)
-            $id = isset($item['id']) ? (int)$item['id'] : 0;
+        );
-            if ($id > 0 && isset($existingById[$id])) {
-                $this->updatePhone($id, $item);
-                $keptIds[$id] = true;
-            } else {
-                $this->addPhone($memberId, $item);
-            }
-        }
-
-        foreach ($existingById as $id => $p) {
-            if (!isset($keptIds[$id])) {
-                $this->deletePhone($id);
-            }
-        }
     }
 
     /**
@@ -628,28 +647,15 @@ class MemberService {
      * @throws Exception
      */
     private function syncEmails(int $memberId, array $incoming): void {
-        $existing = $this->emailMapper->findByMemberId($memberId);
+        $this->syncSubEntities(
-        $existingById = [];
+            $memberId,
-        foreach ($existing as $e) {
+            $incoming,
-            $existingById[$e->getId()] = $e;
+            fn() => $this->emailMapper->findByMemberId($memberId),
-        }
+            fn($e) => $e->getId(),
-
+            fn($id, $data) => $this->updateEmail($id, $data),
-        $keptIds = [];
+            fn($mid, $data) => $this->addEmail($mid, $data),
-        foreach ($incoming as $item) {
+            fn($id) => $this->deleteEmail($id)
-            $id = isset($item['id']) ? (int)$item['id'] : 0;
+        );
-            if ($id > 0 && isset($existingById[$id])) {
-                $this->updateEmail($id, $item);
-                $keptIds[$id] = true;
-            } else {
-                $this->addEmail($memberId, $item);
-            }
-        }
-
-        foreach ($existingById as $id => $e) {
-            if (!isset($keptIds[$id])) {
-                $this->deleteEmail($id);
-            }
-        }
     }
 
     // ── Delete (soft) ────────────────────────────────────────────────