diff --git a/.plans/open/issue-204-duplicate-sync-methods.md b/.plans/open/issue-204-duplicate-sync-methods.md index 5396b4d..2e6ba97 100644 --- a/.plans/open/issue-204-duplicate-sync-methods.md +++ b/.plans/open/issue-204-duplicate-sync-methods.md @@ -46,32 +46,20 @@ private function syncAddresses(int $memberId, array $incoming): void { ## Solution -Create a generic sync utility: - -```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. +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. ## 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) -- [ ] Document the generic sync algorithm +- [ ] Ensure identical behavior is preserved (all existing tests must pass) +- [ ] 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 - refactoring - backend -- priority:medium \ No newline at end of file +- priority:medium diff --git a/lib/Service/MemberService.php b/lib/Service/MemberService.php index 49574b3..165a25b 100644 --- a/lib/Service/MemberService.php +++ b/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 $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); - $existingById = []; - foreach ($existing as $addr) { - $existingById[$addr->getId()] = $addr; - } - - $keptIds = []; - foreach ($incoming as $item) { - $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); - } - } + $this->syncSubEntities( + $memberId, + $incoming, + fn() => $this->addressMapper->findByMemberId($memberId), + fn($e) => $e->getId(), + fn($id, $data) => $this->updateAddress($id, $data), + fn($mid, $data) => $this->addAddress($mid, $data), + fn($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); - $existingById = []; - foreach ($existing as $p) { - $existingById[$p->getId()] = $p; - } - - $keptIds = []; - foreach ($incoming as $item) { - $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); - } - } + $this->syncSubEntities( + $memberId, + $incoming, + fn() => $this->phoneMapper->findByMemberId($memberId), + fn($e) => $e->getId(), + fn($id, $data) => $this->updatePhone($id, $data), + fn($mid, $data) => $this->addPhone($mid, $data), + fn($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); - $existingById = []; - foreach ($existing as $e) { - $existingById[$e->getId()] = $e; - } - - $keptIds = []; - foreach ($incoming as $item) { - $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); - } - } + $this->syncSubEntities( + $memberId, + $incoming, + fn() => $this->emailMapper->findByMemberId($memberId), + fn($e) => $e->getId(), + fn($id, $data) => $this->updateEmail($id, $data), + fn($mid, $data) => $this->addEmail($mid, $data), + fn($id) => $this->deleteEmail($id) + ); } // ── Delete (soft) ────────────────────────────────────────────────