Cleanup PackageManager::transferPackage() and related tests

Author: stevenrombauts

src/Command/TransferOwnershipCommand.php

@@ -15,6 +15,7 @@
 use App\Entity\AuditRecord;
 use App\Entity\Package;
 use App\Entity\User;
+use App\Model\PackageManager;
 use App\Util\DoctrineTrait;
 use Composer\Console\Input\InputOption;
 use Doctrine\Persistence\ManagerRegistry;
@@ -30,6 +31,7 @@ class TransferOwnershipCommand extends Command
 
     public function __construct(
         private readonly ManagerRegistry $doctrine,
+        private readonly PackageManager $packageManager,
     )
     {
         parent::__construct();
@@ -165,24 +167,8 @@ private function outputPackageTable(OutputInterface $output, array $packages, ar
      */
     private function transferOwnership(array $packages, array $maintainers): void
     {
-        $normalizedMaintainers = array_values(array_map(fn (User $user) => $user->getId(), $maintainers));
-        sort($normalizedMaintainers, SORT_NUMERIC);
-
         foreach ($packages as $package) {
-            $oldMaintainers = $package->getMaintainers()->toArray();
-
-            $normalizedOldMaintainers = array_values(array_map(fn (User $user) => $user->getId(), $oldMaintainers));
-            sort($normalizedOldMaintainers, SORT_NUMERIC);
-            if ($normalizedMaintainers === $normalizedOldMaintainers) {
-                continue;
-            }
-
-            $package->getMaintainers()->clear();
-            foreach ($maintainers as $maintainer) {
-                $package->addMaintainer($maintainer);
-            }
-
-            $this->doctrine->getManager()->persist(AuditRecord::packageTransferred($package, null, $oldMaintainers, array_values($maintainers)));
+            $this->packageManager->transferPackage($package, $maintainers);
         }
 
         $this->doctrine->getManager()->flush();

src/Controller/PackageController.php

@@ -995,15 +995,13 @@ public function transferPackageAction(Request $req, #[MapEntity] Package $packag
     {
         $this->denyAccessUnlessGranted(PackageActions::TransferPackage->value, $package);
 
-        $oldMaintainers = $package->getMaintainers()->toArray();
-
         $form = $this->createTransferPackageForm($package);
         $form->handleRequest($req);
 
         if ($form->isSubmitted() && $form->isValid()) {
             try {
                 $newMaintainers = $form->getData()->getMaintainers()->toArray();
-                $result = $this->packageManager->transferPackage($package, $oldMaintainers, $newMaintainers);
+                $result = $this->packageManager->transferPackage($package, $newMaintainers);
                 $this->getEM()->flush();
 
                 if ($result) {

src/Model/PackageManager.php

@@ -245,8 +245,9 @@ public function notifyNewMaintainer(User $user, Package $package): bool
      * @param User[] $oldMaintainers
      * @param User[] $newMaintainers
      */
-    public function transferPackage(Package $package, array $oldMaintainers, array $newMaintainers): bool
+    public function transferPackage(Package $package, array $newMaintainers): bool
     {
+        $oldMaintainers = $package->getMaintainers()->toArray();
         $normalizedOldMaintainers = array_values(array_map(fn (User $user) => $user->getId(), $oldMaintainers));
         sort($normalizedOldMaintainers, SORT_NUMERIC);
 

tests/Command/TransferOwnershipCommandTest.php

@@ -12,14 +12,12 @@
 
 namespace App\Tests\Command;
 
-use App\Audit\AuditRecordType;
 use App\Command\TransferOwnershipCommand;
-use App\Entity\AuditRecord;
 use App\Entity\Package;
 use App\Entity\User;
+use App\Model\PackageManager;
 use App\Tests\IntegrationTestCase;
 use Doctrine\Persistence\ManagerRegistry;
-use PHPUnit\Framework\Attributes\TestWith;
 use Symfony\Component\Console\Command\Command;
 use Symfony\Component\Console\Tester\CommandTester;
 
@@ -45,7 +43,7 @@ protected function setUp(): void
         $this->package3 = self::createPackage('vendor2/package1', 'https://github.com/vendor2/package1',maintainers: [$john]);
         $this->store($this->package1, $this->package2, $this->package3);
 
-        $command = new TransferOwnershipCommand(self::getContainer()->get(ManagerRegistry::class));
+        $command = new TransferOwnershipCommand(self::getContainer()->get(ManagerRegistry::class), self::getContainer()->get(PackageManager::class));
         $this->commandTester = new CommandTester($command);
     }
 
@@ -73,9 +71,6 @@ public function testExecuteSuccessForVendor(): void
         $this->assertEqualsCanonicalizing(['alice', 'bob'], array_map($callable, $package1->getMaintainers()->toArray()));
         $this->assertEqualsCanonicalizing(['alice', 'bob'], array_map($callable, $package2->getMaintainers()->toArray()));
         $this->assertEqualsCanonicalizing(['john'], array_map($callable, $package3->getMaintainers()->toArray()), 'vendor2 packages should not be changed');
-
-        $this->assertAuditLogWasCreated($package1, ['john', 'alice'], ['alice', 'bob']);
-        $this->assertAuditLogWasCreated($package2, ['john', 'bob'], ['alice', 'bob']);
     }
 
     public function testExecuteSuccessForPackage(): void
@@ -99,8 +94,6 @@ public function testExecuteSuccessForPackage(): void
         $callable = fn (User $user) => $user->getUsernameCanonical();
         $this->assertEqualsCanonicalizing(['bob', 'john'], array_map($callable, $package2->getMaintainers()->toArray()), 'vendor1 packages should not be changed');
         $this->assertEqualsCanonicalizing(['alice', 'john'], array_map($callable, $package3->getMaintainers()->toArray()));
-
-        $this->assertAuditLogWasCreated($package3, ['john'], ['alice', 'john']);
     }
 
     public function testExecuteWithDryRunDoesNothing(): void
@@ -152,25 +145,4 @@ public function testExecuteFailsIfNoVendorPackagesFound(): void
         $output = $this->commandTester->getDisplay();
         $this->assertStringContainsString('No packages found for foobar', $output);
     }
-
-    /**
-     * @param string[] $oldMaintainers
-     * @param string[] $newMaintainers
-     */
-    private function assertAuditLogWasCreated(Package $package, array $oldMaintainers, array $newMaintainers): void
-    {
-        $record = $this->getEM()->getRepository(AuditRecord::class)->findOneBy([
-            'type' => AuditRecordType::PackageTransferred->value,
-            'packageId' => $package->getId(),
-            'actorId' => null,
-        ]);
-
-        $this->assertNotNull($record);
-        $this->assertSame('admin', $record->attributes['actor']);
-        $this->assertSame($package->getId(), $record->packageId);
-
-        $callable = fn (array $user) => $user['username'];
-        $this->assertEqualsCanonicalizing($oldMaintainers, array_map($callable, $record->attributes['previous_maintainers']));
-        $this->assertEqualsCanonicalizing($newMaintainers, array_map($callable, $record->attributes['current_maintainers']));
-    }
 }

tests/Model/PackageManagerTest.php

@@ -12,11 +12,24 @@
 
 namespace App\Tests\Model;
 
+use App\Audit\AuditRecordType;
+use App\Entity\AuditRecord;
 use App\Entity\Package;
-use PHPUnit\Framework\TestCase;
+use App\Entity\User;
+use App\Model\PackageManager;
+use App\Tests\IntegrationTestCase;
 
-class PackageManagerTest extends TestCase
+class PackageManagerTest extends IntegrationTestCase
 {
+    private PackageManager $packageManager;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $this->packageManager = self::getContainer()->get(PackageManager::class);
+    }
+
     public function testNotifyFailure(): void
     {
         $this->markTestSkipped('Do it!');
@@ -46,4 +59,72 @@ public function testNotifyFailure(): void
         $client->request('POST', '/api/github?username=test&apiToken=token', ['payload' => $payload]);
         $this->assertEquals(202, $client->getResponse()->getStatusCode());
     }
+
+    public function testTransferPackageReplacesAllMaintainers(): void
+    {
+        $alice = self::createUser('alice', '[email protected]');
+        $bob = self::createUser('bob', '[email protected]');
+        $john = self::createUser('john', '[email protected]');
+        $this->store($alice, $bob, $john);
+
+        $package = self::createPackage('vendor/package', 'https://github.com/vendor/package', maintainers: [$john, $alice]);
+        $this->store($package);
+
+        $result = $this->packageManager->transferPackage($package, [$bob, $alice]);
+
+        $em = self::getEM();
+        $em->flush();
+        $em->clear();
+
+        $this->assertTrue($result);
+
+        $callable = fn (User $user) => $user->getUsernameCanonical();
+        $this->assertEqualsCanonicalizing(['alice', 'bob'], array_map($callable, $package->getMaintainers()->toArray()));
+        $this->assertAuditLogWasCreated($package, ['john', 'alice'], ['bob', 'alice']);
+    }
+
+    public function testTransferPackageWithSameMaintainersDoesNothing(): void
+    {
+        $alice = self::createUser('alice', '[email protected]');
+        $bob = self::createUser('bob', '[email protected]');
+        $this->store($alice, $bob);
+
+        $package = self::createPackage('vendor/package', 'https://github.com/vendor/package', maintainers: [$bob, $alice]);
+        $this->store($package);
+
+        $result = $this->packageManager->transferPackage($package, [$alice, $bob]);
+
+        $em = self::getEM();
+        $em->flush();
+        $em->clear();
+
+        $this->assertFalse($result);
+
+        $record = $em->getRepository(AuditRecord::class)->findOneBy([
+            'type' => AuditRecordType::PackageTransferred->value,
+            'packageId' => $package->getId(),
+        ]);
+
+        $this->assertNull($record, 'No audit record should be created when maintainers are the same');
+    }
+
+    /**
+     * @param string[] $oldMaintainers
+     * @param string[] $newMaintainers
+     */
+    private function assertAuditLogWasCreated(Package $package, array $oldMaintainers, array $newMaintainers): void
+    {
+        $record = self::getEM()->getRepository(AuditRecord::class)->findOneBy([
+            'type' => AuditRecordType::PackageTransferred->value,
+            'packageId' => $package->getId(),
+            'actorId' => null,
+        ]);
+
+        $this->assertNotNull($record, 'Audit record should be created for package transfer');
+        $this->assertSame($package->getId(), $record->packageId);
+
+        $callable = fn (array $user) => $user['username'];
+        $this->assertEqualsCanonicalizing($oldMaintainers, array_map($callable, $record->attributes['previous_maintainers']));
+        $this->assertEqualsCanonicalizing($newMaintainers, array_map($callable, $record->attributes['current_maintainers']));
+    }
 }