Skip to content

Commit 5cb5b93

Browse files
stevenrombautsstevenrombauts
committed
Add tests for PackageController::transferPackageAction() 
1 parent e679bfb commit 5cb5b93

File tree

6 files changed

+172
-87
lines changed

6 files changed

+172
-87
lines changed

‎src/Command/TransferOwnershipCommand.php‎

Lines changed: 3 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
use App\Entity\AuditRecord;
1616
use App\Entity\Package;
1717
use App\Entity\User;
18+
use App\Model\PackageManager;
1819
use App\Util\DoctrineTrait;
1920
use Composer\Console\Input\InputOption;
2021
use Doctrine\Persistence\ManagerRegistry;
@@ -30,6 +31,7 @@ class TransferOwnershipCommand extends Command
3031

3132
public function __construct(
3233
private readonly ManagerRegistry $doctrine,
34+
private readonly PackageManager $packageManager,
3335
)
3436
{
3537
parent::__construct();
@@ -165,24 +167,8 @@ private function outputPackageTable(OutputInterface $output, array $packages, ar
165167
*/
166168
private function transferOwnership(array $packages, array $maintainers): void
167169
{
168-
$normalizedMaintainers = array_values(array_map(fn (User $user) => $user->getId(), $maintainers));
169-
sort($normalizedMaintainers, SORT_NUMERIC);
170-
171170
foreach ($packages as $package) {
172-
$oldMaintainers = $package->getMaintainers()->toArray();
173-
174-
$normalizedOldMaintainers = array_values(array_map(fn (User $user) => $user->getId(), $oldMaintainers));
175-
sort($normalizedOldMaintainers, SORT_NUMERIC);
176-
if ($normalizedMaintainers === $normalizedOldMaintainers) {
177-
continue;
178-
}
179-
180-
$package->getMaintainers()->clear();
181-
foreach ($maintainers as $maintainer) {
182-
$package->addMaintainer($maintainer);
183-
}
184-
185-
$this->doctrine->getManager()->persist(AuditRecord::packageTransferred($package, null, $oldMaintainers, array_values($maintainers)));
171+
$this->packageManager->transferPackage($package, $maintainers);
186172
}
187173

188174
$this->doctrine->getManager()->flush();

‎src/Controller/PackageController.php‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -995,17 +995,14 @@ public function transferPackageAction(Request $req, #[MapEntity] Package $packag
995995
{
996996
$this->denyAccessUnlessGranted(PackageActions::TransferPackage->value, $package);
997997

998-
$oldMaintainers = $package->getMaintainers()->toArray();
999-
1000998
$form = $this->createTransferPackageForm($package);
1001999
$form->handleRequest($req);
10021000

10031001
if ($form->isSubmitted() && $form->isValid()) {
10041002
try {
1005-
$em = $this->getEM();
10061003
$newMaintainers = $form->getData()->getMaintainers()->toArray();
1007-
$result = $this->packageManager->transferPackage($package, $oldMaintainers, $newMaintainers);
1008-
$em->flush();
1004+
$result = $this->packageManager->transferPackage($package, $newMaintainers);
1005+
$this->getEM()->flush();
10091006

10101007
if ($result) {
10111008
$usernames = array_map(fn (User $user) => $user->getUsername(), $newMaintainers);
@@ -1671,7 +1668,7 @@ private function createRemoveMaintainerForm(Package $package): FormInterface
16711668
private function createTransferPackageForm(Package $package): FormInterface
16721669
{
16731670
$transferRequest = new TransferPackageRequest();
1674-
$transferRequest->setMaintainers($package->getMaintainers());
1671+
$transferRequest->setMaintainers(clone $package->getMaintainers());
16751672

16761673
return $this->createForm(TransferPackageRequestType::class, $transferRequest);
16771674
}

‎src/Model/PackageManager.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -245,8 +245,9 @@ public function notifyNewMaintainer(User $user, Package $package): bool
245245
* @param User[] $oldMaintainers
246246
* @param User[] $newMaintainers
247247
*/
248-
public function transferPackage(Package $package, array $oldMaintainers, array $newMaintainers): bool
248+
public function transferPackage(Package $package, array $newMaintainers): bool
249249
{
250+
$oldMaintainers = $package->getMaintainers()->toArray();
250251
$normalizedOldMaintainers = array_values(array_map(fn (User $user) => $user->getId(), $oldMaintainers));
251252
sort($normalizedOldMaintainers, SORT_NUMERIC);
252253

‎tests/Command/TransferOwnershipCommandTest.php‎

Lines changed: 2 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -12,14 +12,12 @@
1212

1313
namespace App\Tests\Command;
1414

15-
use App\Audit\AuditRecordType;
1615
use App\Command\TransferOwnershipCommand;
17-
use App\Entity\AuditRecord;
1816
use App\Entity\Package;
1917
use App\Entity\User;
18+
use App\Model\PackageManager;
2019
use App\Tests\IntegrationTestCase;
2120
use Doctrine\Persistence\ManagerRegistry;
22-
use PHPUnit\Framework\Attributes\TestWith;
2321
use Symfony\Component\Console\Command\Command;
2422
use Symfony\Component\Console\Tester\CommandTester;
2523

@@ -45,7 +43,7 @@ protected function setUp(): void
4543
$this->package3 = self::createPackage('vendor2/package1', 'https://github.com/vendor2/package1',maintainers: [$john]);
4644
$this->store($this->package1, $this->package2, $this->package3);
4745

48-
$command = new TransferOwnershipCommand(self::getContainer()->get(ManagerRegistry::class));
46+
$command = new TransferOwnershipCommand(self::getContainer()->get(ManagerRegistry::class), self::getContainer()->get(PackageManager::class));
4947
$this->commandTester = new CommandTester($command);
5048
}
5149

@@ -73,9 +71,6 @@ public function testExecuteSuccessForVendor(): void
7371
$this->assertEqualsCanonicalizing(['alice', 'bob'], array_map($callable, $package1->getMaintainers()->toArray()));
7472
$this->assertEqualsCanonicalizing(['alice', 'bob'], array_map($callable, $package2->getMaintainers()->toArray()));
7573
$this->assertEqualsCanonicalizing(['john'], array_map($callable, $package3->getMaintainers()->toArray()), 'vendor2 packages should not be changed');
76-
77-
$this->assertAuditLogWasCreated($package1, ['john', 'alice'], ['alice', 'bob']);
78-
$this->assertAuditLogWasCreated($package2, ['john', 'bob'], ['alice', 'bob']);
7974
}
8075

8176
public function testExecuteSuccessForPackage(): void
@@ -99,8 +94,6 @@ public function testExecuteSuccessForPackage(): void
9994
$callable = fn (User $user) => $user->getUsernameCanonical();
10095
$this->assertEqualsCanonicalizing(['bob', 'john'], array_map($callable, $package2->getMaintainers()->toArray()), 'vendor1 packages should not be changed');
10196
$this->assertEqualsCanonicalizing(['alice', 'john'], array_map($callable, $package3->getMaintainers()->toArray()));
102-
103-
$this->assertAuditLogWasCreated($package3, ['john'], ['alice', 'john']);
10497
}
10598

10699
public function testExecuteWithDryRunDoesNothing(): void
@@ -126,33 +119,6 @@ public function testExecuteWithDryRunDoesNothing(): void
126119
$this->assertEqualsCanonicalizing(['john', 'alice'], array_map($callable, $package1->getMaintainers()->toArray()));
127120
}
128121

129-
public function testExecuteIgnoresIdenticalMaintainers(): void
130-
{
131-
$this->commandTester->execute([
132-
'vendorOrPackage' => 'vendor1',
133-
'maintainers' => ['alice', 'john'],
134-
]);
135-
136-
$this->commandTester->assertCommandIsSuccessful();
137-
138-
$em = self::getEM();
139-
$em->clear();
140-
141-
$package1 = $em->find(Package::class, $this->package1->getId());
142-
$package2 = $em->find(Package::class, $this->package2->getId());
143-
144-
$this->assertNotNull($package1);
145-
$this->assertNotNull($package2);
146-
147-
$callable = fn (User $user) => $user->getUsernameCanonical();
148-
$this->assertEqualsCanonicalizing(['alice', 'john'], array_map($callable, $package1->getMaintainers()->toArray()));
149-
$this->assertEqualsCanonicalizing(['alice', 'john'], array_map($callable, $package2->getMaintainers()->toArray()));
150-
151-
$record = $this->retrieveAuditRecordForPackage($package1);
152-
$this->assertNull($record, 'No audit log should be created if package maintainers are identical');
153-
$this->assertAuditLogWasCreated($package2, ['john', 'bob'], ['alice', 'john']);
154-
}
155-
156122
public function testExecuteFailsWithUnknownMaintainers(): void
157123
{
158124
$this->commandTester->execute([
@@ -178,29 +144,4 @@ public function testExecuteFailsIfNoVendorPackagesFound(): void
178144
$output = $this->commandTester->getDisplay();
179145
$this->assertStringContainsString('No packages found for foobar', $output);
180146
}
181-
182-
/**
183-
* @param string[] $oldMaintainers
184-
* @param string[] $newMaintainers
185-
*/
186-
private function assertAuditLogWasCreated(Package $package, array $oldMaintainers, array $newMaintainers): void
187-
{
188-
$record = $this->retrieveAuditRecordForPackage($package);
189-
$this->assertNotNull($record);
190-
$this->assertSame('admin', $record->attributes['actor']);
191-
$this->assertSame($package->getId(), $record->packageId);
192-
193-
$callable = fn (array $user) => $user['username'];
194-
$this->assertEqualsCanonicalizing($oldMaintainers, array_map($callable, $record->attributes['previous_maintainers']));
195-
$this->assertEqualsCanonicalizing($newMaintainers, array_map($callable, $record->attributes['current_maintainers']));
196-
}
197-
198-
private function retrieveAuditRecordForPackage(Package $package): ?AuditRecord
199-
{
200-
return $this->getEM()->getRepository(AuditRecord::class)->findOneBy([
201-
'type' => AuditRecordType::PackageTransferred->value,
202-
'packageId' => $package->getId(),
203-
'actorId' => null,
204-
]);
205-
}
206147
}

‎tests/Controller/PackageControllerTest.php‎

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
use App\Entity\Package;
1717
use App\Entity\User;
1818
use App\Tests\IntegrationTestCase;
19+
use PHPUnit\Framework\Attributes\TestWith;
1920

2021
class PackageControllerTest extends IntegrationTestCase
2122
{
@@ -144,4 +145,82 @@ public function testRemoveMaintainer(): void
144145

145146
$this->assertNotNull($auditRecord);
146147
}
148+
149+
public function testTransferPackage(): void
150+
{
151+
$john = self::createUser('john', 'john@example.org');
152+
$alice = self::createUser('alice', 'alice@example.org');
153+
$bob = self::createUser('bob', 'bob@example.org');
154+
$package = self::createPackage('test/pkg', 'https://example.com/test/pkg', maintainers: [$john, $alice]);
155+
156+
$this->store($john, $alice, $bob, $package);
157+
158+
$this->client->loginUser($john);
159+
160+
$this->assertTrue($package->isMaintainer($john));
161+
$this->assertTrue($package->isMaintainer($alice));
162+
$this->assertFalse($package->isMaintainer($bob));
163+
164+
$crawler = $this->client->request('GET', '/packages/test/pkg');
165+
166+
$form = $crawler->filter('[name="transfer_package_form"]')->form();
167+
$form->setValues([
168+
'transfer_package_form[maintainers][0]' => 'alice',
169+
'transfer_package_form[maintainers][1]' => 'bob@example.org',
170+
]);
171+
172+
$this->client->submit($form);
173+
174+
$this->assertResponseRedirects('/packages/test/pkg');
175+
$this->client->followRedirect();
176+
$this->assertResponseIsSuccessful();
177+
178+
$em = self::getEM();
179+
$em->clear();
180+
181+
$package = $em->getRepository(Package::class)->find($package->getId());
182+
$this->assertNotNull($package);
183+
184+
$maintainerIds = array_map(fn (User $user) => $user->getId(), $package->getMaintainers()->toArray());
185+
$this->assertContains($alice->getId(), $maintainerIds);
186+
$this->assertContains($bob->getId(), $maintainerIds);
187+
$this->assertNotContains($john->getId(), $maintainerIds);
188+
189+
$auditRecord = $em->getRepository(\App\Entity\AuditRecord::class)->findOneBy([
190+
'type' => AuditRecordType::PackageTransferred->value,
191+
'packageId' => $package->getId(),
192+
]);
193+
194+
$this->assertNotNull($auditRecord, 'Audit record not found');
195+
}
196+
197+
#[TestWith(['does_not_exist', 'value is not a valid username or email'])]
198+
#[TestWith([null, 'at least one maintainer must be specified'])]
199+
public function testTransferPackageReturnsValidationError(?string $value, string $message): void
200+
{
201+
$alice = self::createUser('alice', 'alice@example.org');
202+
$package = self::createPackage('test/pkg', 'https://example.com/test/pkg', maintainers: [$alice]);
203+
204+
$this->store($alice, $package);
205+
206+
$this->client->loginUser($alice);
207+
208+
$crawler = $this->client->request('GET', '/packages/test/pkg');
209+
210+
$form = $crawler->filter('[name="transfer_package_form"]')->form();
211+
$form->setValues([
212+
'transfer_package_form[maintainers][0]' => $value,
213+
]);
214+
215+
$this->client->submit($form);
216+
217+
$this->assertResponseRedirects('/packages/test/pkg');
218+
$crawler = $this->client->followRedirect();
219+
$this->assertResponseIsSuccessful();
220+
221+
$elements = $crawler->filter('.flash-container .alert-error');
222+
$this->assertCount(1, $elements);
223+
$text = $elements->text();
224+
$this->assertStringContainsStringIgnoringCase($message, $text);
225+
}
147226
}

‎tests/Model/PackageManagerTest.php‎

Lines changed: 83 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,24 @@
1212

1313
namespace App\Tests\Model;
1414

15+
use App\Audit\AuditRecordType;
16+
use App\Entity\AuditRecord;
1517
use App\Entity\Package;
16-
use PHPUnit\Framework\TestCase;
18+
use App\Entity\User;
19+
use App\Model\PackageManager;
20+
use App\Tests\IntegrationTestCase;
1721

18-
class PackageManagerTest extends TestCase
22+
class PackageManagerTest extends IntegrationTestCase
1923
{
24+
private PackageManager $packageManager;
25+
26+
protected function setUp(): void
27+
{
28+
parent::setUp();
29+
30+
$this->packageManager = self::getContainer()->get(PackageManager::class);
31+
}
32+
2033
public function testNotifyFailure(): void
2134
{
2235
$this->markTestSkipped('Do it!');
@@ -46,4 +59,72 @@ public function testNotifyFailure(): void
4659
$client->request('POST', '/api/github?username=test&apiToken=token', ['payload' => $payload]);
4760
$this->assertEquals(202, $client->getResponse()->getStatusCode());
4861
}
62+
63+
public function testTransferPackageReplacesAllMaintainers(): void
64+
{
65+
$alice = self::createUser('alice', 'alice@example.org');
66+
$bob = self::createUser('bob', 'bob@example.org');
67+
$john = self::createUser('john', 'john@example.org');
68+
$this->store($alice, $bob, $john);
69+
70+
$package = self::createPackage('vendor/package', 'https://github.com/vendor/package', maintainers: [$john, $alice]);
71+
$this->store($package);
72+
73+
$result = $this->packageManager->transferPackage($package, [$bob, $alice]);
74+
75+
$em = self::getEM();
76+
$em->flush();
77+
$em->clear();
78+
79+
$this->assertTrue($result);
80+
81+
$callable = fn (User $user) => $user->getUsernameCanonical();
82+
$this->assertEqualsCanonicalizing(['alice', 'bob'], array_map($callable, $package->getMaintainers()->toArray()));
83+
$this->assertAuditLogWasCreated($package, ['john', 'alice'], ['bob', 'alice']);
84+
}
85+
86+
public function testTransferPackageWithSameMaintainersDoesNothing(): void
87+
{
88+
$alice = self::createUser('alice', 'alice@example.org');
89+
$bob = self::createUser('bob', 'bob@example.org');
90+
$this->store($alice, $bob);
91+
92+
$package = self::createPackage('vendor/package', 'https://github.com/vendor/package', maintainers: [$bob, $alice]);
93+
$this->store($package);
94+
95+
$result = $this->packageManager->transferPackage($package, [$alice, $bob]);
96+
97+
$em = self::getEM();
98+
$em->flush();
99+
$em->clear();
100+
101+
$this->assertFalse($result);
102+
103+
$record = $em->getRepository(AuditRecord::class)->findOneBy([
104+
'type' => AuditRecordType::PackageTransferred->value,
105+
'packageId' => $package->getId(),
106+
]);
107+
108+
$this->assertNull($record, 'No audit record should be created when maintainers are the same');
109+
}
110+
111+
/**
112+
* @param string[] $oldMaintainers
113+
* @param string[] $newMaintainers
114+
*/
115+
private function assertAuditLogWasCreated(Package $package, array $oldMaintainers, array $newMaintainers): void
116+
{
117+
$record = self::getEM()->getRepository(AuditRecord::class)->findOneBy([
118+
'type' => AuditRecordType::PackageTransferred->value,
119+
'packageId' => $package->getId(),
120+
'actorId' => null,
121+
]);
122+
123+
$this->assertNotNull($record, 'Audit record should be created for package transfer');
124+
$this->assertSame($package->getId(), $record->packageId);
125+
126+
$callable = fn (array $user) => $user['username'];
127+
$this->assertEqualsCanonicalizing($oldMaintainers, array_map($callable, $record->attributes['previous_maintainers']));
128+
$this->assertEqualsCanonicalizing($newMaintainers, array_map($callable, $record->attributes['current_maintainers']));
129+
}
49130
}

0 commit comments

Comments
 (0)