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

Factor out the duplicated sync logic from syncAddresses(), syncPhones(),
and syncEmails() into a single generic syncSubEntities() method that
accepts callables for fetch/update/create/delete operations. Each
specialized sync method is now a thin ~10-line wrapper.

(Closes #204)

.plans/done/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
+- priority:medium

lib/Service/MemberService.php

@@ -19,6 +19,7 @@ use OCA\Mitgliederverwaltung\Validation\PhoneValidator;
 use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Db\MultipleObjectsReturnedException;
 use OCP\DB\Exception;
+use OCP\IDBConnection;
 use Psr\Log\LoggerInterface;
 
 /**
@@ -37,6 +38,7 @@ class MemberService {
     private AuditService $auditService;
     private LoggerInterface $logger;
     private ?EncryptionService $encryptionService;
+    private IDBConnection $db;
 
     public function __construct(
         MemberMapper $memberMapper,
@@ -47,7 +49,8 @@ class MemberService {
         StufeHistoryMapper $stufeHistoryMapper,
         AuditService $auditService,
         LoggerInterface $logger,
-        ?EncryptionService $encryptionService = null
+        ?EncryptionService $encryptionService,
+        IDBConnection $db
     ) {
         $this->memberMapper = $memberMapper;
         $this->addressMapper = $addressMapper;
@@ -58,6 +61,7 @@ class MemberService {
         $this->auditService = $auditService;
         $this->logger = $logger;
         $this->encryptionService = $encryptionService;
+        $this->db = $db;
     }
 
     /**
@@ -153,12 +157,20 @@ class MemberService {
         $member->setCreatedAt($now);
         $member->setUpdatedAt($now);
 
-        /** @var Member $member */
-        $member = $this->memberMapper->insert($member);
+        $this->db->beginTransaction();
+        try {
+            /** @var Member $member */
+            $member = $this->memberMapper->insert($member);
 
-        $savedAddresses = $this->saveAddresses($member->getId(), $addresses);
-        $savedPhones = $this->savePhones($member->getId(), $phones);
-        $savedEmails = $this->saveEmails($member->getId(), $emails);
+            $savedAddresses = $this->saveAddresses($member->getId(), $addresses);
+            $savedPhones = $this->savePhones($member->getId(), $phones);
+            $savedEmails = $this->saveEmails($member->getId(), $emails);
+
+            $this->db->commit();
+        } catch (\Exception $e) {
+            $this->db->rollback();
+            throw $e;
+        }
 
         $this->auditService->logCreate($member->jsonSerialize(), 'member', $member->getId());
 
@@ -515,19 +527,27 @@ class MemberService {
 
         $member->setUpdatedAt((new DateTime())->format('Y-m-d H:i:s'));
 
-        /** @var Member $member */
-        $member = $this->memberMapper->update($member);
+        $this->db->beginTransaction();
+        try {
+            /** @var Member $member */
+            $member = $this->memberMapper->update($member);
 
-        // Sync sub-entities if the caller included them in the payload.
-        // Absent keys (undefined) leave existing sub-entities untouched.
-        if (array_key_exists('addresses', $rawData) && is_array($rawData['addresses'])) {
-            $this->syncAddresses($id, $rawData['addresses']);
-        }
-        if (array_key_exists('phones', $rawData) && is_array($rawData['phones'])) {
-            $this->syncPhones($id, $rawData['phones']);
-        }
-        if (array_key_exists('emails', $rawData) && is_array($rawData['emails'])) {
-            $this->syncEmails($id, $rawData['emails']);
+            // Sync sub-entities if the caller included them in the payload.
+            // Absent keys (undefined) leave existing sub-entities untouched.
+            if (array_key_exists('addresses', $rawData) && is_array($rawData['addresses'])) {
+                $this->syncAddresses($id, $rawData['addresses']);
+            }
+            if (array_key_exists('phones', $rawData) && is_array($rawData['phones'])) {
+                $this->syncPhones($id, $rawData['phones']);
+            }
+            if (array_key_exists('emails', $rawData) && is_array($rawData['emails'])) {
+                $this->syncEmails($id, $rawData['emails']);
+            }
+
+            $this->db->commit();
+        } catch (\Exception $e) {
+            $this->db->rollback();
+            throw $e;
         }
 
         $this->auditService->logUpdate($oldData, $member->jsonSerialize(), 'member', $id);
@@ -535,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:
@@ -546,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)
+        );
     }
 
     /**
@@ -577,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)
+        );
     }
 
     /**
@@ -608,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) ────────────────────────────────────────────────
@@ -685,12 +711,20 @@ class MemberService {
         $member->setCalendarEventUri(null);
         $member->setContactVcardUri(null);
 
-        $this->memberMapper->update($member);
+        $this->db->beginTransaction();
+        try {
+            $this->memberMapper->update($member);
 
-        // Hard-delete sub-entities (addresses, phones, emails)
-        $this->addressMapper->deleteByMemberId($id);
-        $this->phoneMapper->deleteByMemberId($id);
-        $this->emailMapper->deleteByMemberId($id);
+            // Hard-delete sub-entities (addresses, phones, emails)
+            $this->addressMapper->deleteByMemberId($id);
+            $this->phoneMapper->deleteByMemberId($id);
+            $this->emailMapper->deleteByMemberId($id);
+
+            $this->db->commit();
+        } catch (\Exception $e) {
+            $this->db->rollback();
+            throw $e;
+        }
 
         // Audit log the soft-delete
         $this->auditService->logSoftDelete('member', $id);

tests/Unit/MemberServiceTest.php

@@ -20,6 +20,7 @@ use OCA\Mitgliederverwaltung\Service\EncryptionService;
 use OCA\Mitgliederverwaltung\Service\MemberService;
 use OCA\Mitgliederverwaltung\Service\ValidationException;
 use OCP\AppFramework\Db\DoesNotExistException;
+use OCP\IDBConnection;
 use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
 use Psr\Log\LoggerInterface;
@@ -35,6 +36,7 @@ class MemberServiceTest extends TestCase {
     private StufeHistoryMapper&MockObject $stufeHistoryMapper;
     private AuditService&MockObject $auditService;
     private LoggerInterface&MockObject $logger;
+    private IDBConnection&MockObject $db;
 
     protected function setUp(): void {
         parent::setUp();
@@ -59,6 +61,7 @@ class MemberServiceTest extends TestCase {
         $this->stufeHistoryMapper = $this->createMock(StufeHistoryMapper::class);
         $this->auditService = $this->createMock(AuditService::class);
         $this->logger = $this->createMock(LoggerInterface::class);
+        $this->db = $this->createMock(IDBConnection::class);
 
         $this->service = new MemberService(
             $this->memberMapper,
@@ -68,7 +71,9 @@ class MemberServiceTest extends TestCase {
             $this->familyMapper,
             $this->stufeHistoryMapper,
             $this->auditService,
-            $this->logger
+            $this->logger,
+            null,
+            $this->db
         );
     }
 
@@ -706,7 +711,8 @@ class MemberServiceTest extends TestCase {
             $this->stufeHistoryMapper,
             $this->auditService,
             $this->logger,
-            $encryption
+            $encryption,
+            $this->db
         );
 
         $map = $service->revealAllAllergies('alice');
@@ -744,7 +750,8 @@ class MemberServiceTest extends TestCase {
             $this->stufeHistoryMapper,
             $this->auditService,
             $this->logger,
-            $encryption
+            $encryption,
+            $this->db
         );
 
         $service->revealAllAllergies('alice');
@@ -1573,4 +1580,169 @@ class MemberServiceTest extends TestCase {
         $this->assertArrayHasKey('phones', $result[0]);
         $this->assertArrayHasKey('emails', $result[0]);
     }
+
+    // ── Database Transactions ───────────────────────────────────────
+
+    public function testCreateCommitsTransactionOnSuccess(): void {
+        $data = $this->getValidMemberData();
+
+        $this->memberMapper->method('search')->willReturn([]);
+        $this->memberMapper->method('insert')
+            ->willReturnCallback(function (Member $m) {
+                $m->setId(1);
+                return $m;
+            });
+
+        $this->db->expects($this->once())->method('beginTransaction');
+        $this->db->expects($this->once())->method('commit');
+        $this->db->expects($this->never())->method('rollback');
+
+        $this->service->create($data);
+    }
+
+    public function testCreateRollbacksTransactionWhenSubEntityInsertFails(): void {
+        $data = $this->getValidMemberData();
+        $phones = [['numberE164' => '+4917612345678']];
+
+        $this->memberMapper->method('search')->willReturn([]);
+        $this->memberMapper->method('insert')
+            ->willReturnCallback(function (Member $m) {
+                $m->setId(1);
+                return $m;
+            });
+        $this->phoneMapper->method('insert')->willThrowException(new \RuntimeException('DB error'));
+
+        $this->db->expects($this->once())->method('beginTransaction');
+        $this->db->expects($this->once())->method('rollback');
+        $this->db->expects($this->never())->method('commit');
+
+        $this->expectException(\RuntimeException::class);
+        $this->expectExceptionMessage('DB error');
+
+        $this->service->create($data, [], $phones, []);
+    }
+
+    public function testCreateRollbacksTransactionWhenAddressInsertFails(): void {
+        $data = $this->getValidMemberData();
+        $addresses = [['strasse' => 'Hauptstr. 1', 'plz' => '12345', 'ort' => 'Berlin']];
+
+        $this->memberMapper->method('search')->willReturn([]);
+        $this->memberMapper->method('insert')
+            ->willReturnCallback(function (Member $m) {
+                $m->setId(1);
+                return $m;
+            });
+        $this->addressMapper->method('insert')->willThrowException(new \RuntimeException('Constraint violation'));
+
+        $this->db->expects($this->once())->method('beginTransaction');
+        $this->db->expects($this->once())->method('rollback');
+        $this->db->expects($this->never())->method('commit');
+
+        $this->expectException(\RuntimeException::class);
+
+        $this->service->create($data, $addresses, [], []);
+    }
+
+    public function testSoftDeleteCommitsTransactionOnSuccess(): void {
+        $member = $this->createMember(1);
+
+        $this->memberMapper->method('findById')->with(1)->willReturn($member);
+        $this->memberMapper->method('update')->willReturnCallback(fn(Member $m) => $m);
+
+        $this->db->expects($this->once())->method('beginTransaction');
+        $this->db->expects($this->once())->method('commit');
+        $this->db->expects($this->never())->method('rollback');
+
+        $this->service->softDelete(1);
+    }
+
+    public function testSoftDeleteRollbacksTransactionWhenDeleteFails(): void {
+        $member = $this->createMember(1);
+
+        $this->memberMapper->method('findById')->with(1)->willReturn($member);
+        $this->memberMapper->method('update')->willReturnCallback(fn(Member $m) => $m);
+        $this->phoneMapper->method('deleteByMemberId')->willThrowException(new \RuntimeException('DB error'));
+
+        $this->db->expects($this->once())->method('beginTransaction');
+        $this->db->expects($this->once())->method('rollback');
+        $this->db->expects($this->never())->method('commit');
+
+        $this->expectException(\RuntimeException::class);
+
+        $this->service->softDelete(1);
+    }
+
+    public function testUpdateCommitsTransactionOnSuccess(): void {
+        $member = $this->createMember(1);
+
+        $this->memberMapper->method('findById')->with(1)->willReturn($member);
+        $this->memberMapper->method('update')->willReturnCallback(fn(Member $m) => $m);
+        $this->addressMapper->method('findByMemberId')->willReturn([]);
+        $this->phoneMapper->method('findByMemberId')->willReturn([]);
+        $this->emailMapper->method('findByMemberId')->willReturn([]);
+
+        $this->db->expects($this->once())->method('beginTransaction');
+        $this->db->expects($this->once())->method('commit');
+        $this->db->expects($this->never())->method('rollback');
+
+        $this->service->update(1, ['notizen' => 'Test']);
+    }
+
+    public function testUpdateRollbacksTransactionWhenSubEntitySyncFails(): void {
+        $member = $this->createMember(1);
+
+        $existing = new Address();
+        $existing->setId(42);
+        $existing->setMemberId(1);
+        $existing->setStrasse('Alt');
+        $existing->setPlz('00000');
+        $existing->setOrt('Alt');
+        $existing->setLand('Deutschland');
+        $existing->setIsPrimary(false);
+
+        $this->memberMapper->method('findById')->with(1)->willReturn($member);
+        $this->memberMapper->method('update')->willReturnCallback(fn(Member $m) => $m);
+        $this->addressMapper->method('findByMemberId')->willReturn([$existing]);
+        $this->phoneMapper->method('findByMemberId')->willReturn([]);
+        $this->emailMapper->method('findByMemberId')->willReturn([]);
+
+        // find() is called by syncAddresses -> updateAddress
+        $this->addressMapper->method('find')->with(42)->willReturn($existing);
+        $this->addressMapper->method('update')->willThrowException(new \RuntimeException('DB error'));
+
+        $this->db->expects($this->once())->method('beginTransaction');
+        $this->db->expects($this->once())->method('rollback');
+        $this->db->expects($this->never())->method('commit');
+
+        $this->expectException(\RuntimeException::class);
+
+        $this->service->update(1, [
+            'addresses' => [
+                ['id' => 42, 'strasse' => 'Neu', 'plz' => '12345', 'ort' => 'Neu'],
+            ],
+        ]);
+    }
+
+    public function testUpdateRollbacksTransactionWhenPhoneSyncFails(): void {
+        $member = $this->createMember(1);
+
+        $this->memberMapper->method('findById')->with(1)->willReturn($member);
+        $this->memberMapper->method('update')->willReturnCallback(fn(Member $m) => $m);
+        $this->addressMapper->method('findByMemberId')->willReturn([]);
+        $this->phoneMapper->method('findByMemberId')->willReturn([]);
+        $this->emailMapper->method('findByMemberId')->willReturn([]);
+
+        // syncPhones will try to insert a new phone
+        $this->phoneMapper->method('insert')->willThrowException(new \RuntimeException('DB error'));
+
+        $this->db->expects($this->once())->method('beginTransaction');
+        $this->db->expects($this->once())->method('rollback');
+        $this->db->expects($this->never())->method('commit');
+
+        $this->expectException(\RuntimeException::class);
+
+        $this->service->update(1, [
+            'phones' => [['numberE164' => '+4917612345678']],
+        ]);
+    }
 }

tests/Unit/MemberValidationTest.php

@@ -15,6 +15,7 @@ use OCA\Mitgliederverwaltung\Service\AuditService;
 use OCA\Mitgliederverwaltung\Service\DuplicateMemberException;
 use OCA\Mitgliederverwaltung\Service\MemberService;
 use OCA\Mitgliederverwaltung\Service\ValidationException;
+use OCP\IDBConnection;
 use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
 use Psr\Log\LoggerInterface;
@@ -36,6 +37,7 @@ class MemberValidationTest extends TestCase {
     private StufeHistoryMapper&MockObject $stufeHistoryMapper;
     private AuditService&MockObject $auditService;
     private LoggerInterface&MockObject $logger;
+    private IDBConnection&MockObject $db;
 
     protected function setUp(): void {
         parent::setUp();
@@ -48,6 +50,7 @@ class MemberValidationTest extends TestCase {
         $this->stufeHistoryMapper = $this->createMock(StufeHistoryMapper::class);
         $this->auditService = $this->createMock(AuditService::class);
         $this->logger = $this->createMock(LoggerInterface::class);
+        $this->db = $this->createMock(IDBConnection::class);
 
         $this->service = new MemberService(
             $this->memberMapper,
@@ -57,7 +60,9 @@ class MemberValidationTest extends TestCase {
             $this->familyMapper,
             $this->stufeHistoryMapper,
             $this->auditService,
-            $this->logger
+            $this->logger,
+            null,
+            $this->db
         );
 
         // Default: no duplicates found
@@ -256,7 +261,9 @@ class MemberValidationTest extends TestCase {
             $this->familyMapper,
             $this->stufeHistoryMapper,
             $this->auditService,
-            $this->logger
+            $this->logger,
+            null,
+            $this->db
         );
 
         $this->expectException(DuplicateMemberException::class);
@@ -292,7 +299,9 @@ class MemberValidationTest extends TestCase {
             $this->familyMapper,
             $this->stufeHistoryMapper,
             $this->auditService,
-            $this->logger
+            $this->logger,
+            null,
+            $this->db
         );
 
         $this->expectException(DuplicateMemberException::class);
@@ -338,7 +347,9 @@ class MemberValidationTest extends TestCase {
             $this->familyMapper,
             $this->stufeHistoryMapper,
             $this->auditService,
-            $this->logger
+            $this->logger,
+            null,
+            $this->db
         );
 
         $this->addressMapper->method('findByMemberId')->willReturn([]);
@@ -377,7 +388,9 @@ class MemberValidationTest extends TestCase {
             $this->familyMapper,
             $this->stufeHistoryMapper,
             $this->auditService,
-            $this->logger
+            $this->logger,
+            null,
+            $this->db
         );
 
         $this->expectException(DuplicateMemberException::class);