From 2807b9ce2fedd0150c0c68c4ca5117d6f10f75c8 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 22 Dec 2024 18:24:56 +0100 Subject: [PATCH] Fix ImportedLinksProcessorTest --- .../ShortUrl/DeleteShortUrlCommandTest.php | 2 +- .../Command/Visit/LocateVisitsCommandTest.php | 2 +- .../RedirectRule/RedirectRuleHandlerTest.php | 2 +- .../Core/src/Importer/ShortUrlImporting.php | 5 ++ .../RedirectRule/Entity/RedirectCondition.php | 2 +- .../Geolocation/GeolocationDbUpdaterTest.php | 2 +- .../Importer/ImportedLinksProcessorTest.php | 49 +++++++++++++++++-- .../Entity/RedirectConditionTest.php | 20 ++++++++ ...ersistenceShortUrlRelationResolverTest.php | 6 +-- .../Core/test/ShortUrl/UrlShortenerTest.php | 2 +- .../Rest/test/Service/ApiKeyServiceTest.php | 4 +- 11 files changed, 80 insertions(+), 16 deletions(-) diff --git a/module/CLI/test/Command/ShortUrl/DeleteShortUrlCommandTest.php b/module/CLI/test/Command/ShortUrl/DeleteShortUrlCommandTest.php index 0402dc8c1..6ffec859a 100644 --- a/module/CLI/test/Command/ShortUrl/DeleteShortUrlCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/DeleteShortUrlCommandTest.php @@ -74,7 +74,7 @@ public function deleteIsRetriedWhenThresholdIsReachedAndQuestionIsAccepted( $identifier = ShortUrlIdentifier::fromShortCodeAndDomain($shortCode); $this->service->expects($this->exactly($expectedDeleteCalls))->method('deleteByShortCode')->with( $identifier, - $this->isType('bool'), + $this->isBool(), )->willReturnCallback(function ($_, bool $ignoreThreshold) use ($shortCode): void { if (!$ignoreThreshold) { throw Exception\DeleteShortUrlException::fromVisitsThreshold( diff --git a/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php b/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php index 0f24a6034..9b35324aa 100644 --- a/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php +++ b/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php @@ -47,7 +47,7 @@ protected function setUp(): void $locker = $this->createMock(Lock\LockFactory::class); $this->lock = $this->createMock(Lock\SharedLockInterface::class); - $locker->method('createLock')->with($this->isType('string'), 600.0, false)->willReturn($this->lock); + $locker->method('createLock')->with($this->isString(), 600.0, false)->willReturn($this->lock); $command = new LocateVisitsCommand($this->visitService, $this->visitToLocation, $locker); diff --git a/module/CLI/test/RedirectRule/RedirectRuleHandlerTest.php b/module/CLI/test/RedirectRule/RedirectRuleHandlerTest.php index eb78da613..a78110607 100644 --- a/module/CLI/test/RedirectRule/RedirectRuleHandlerTest.php +++ b/module/CLI/test/RedirectRule/RedirectRuleHandlerTest.php @@ -77,7 +77,7 @@ public function rulesAreDisplayedWhenRulesListIsEmpty( $this->io->expects($this->once())->method('choice')->willReturn($action->value); $this->io->expects($this->never())->method('newLine'); $this->io->expects($this->never())->method('text'); - $this->io->expects($this->once())->method('table')->with($this->isType('array'), [ + $this->io->expects($this->once())->method('table')->with($this->isArray(), [ ['1', $comment($this->cond1->toHumanFriendly()), 'https://example.com/one'], [ '2', diff --git a/module/Core/src/Importer/ShortUrlImporting.php b/module/Core/src/Importer/ShortUrlImporting.php index cc534fc23..aeb24ccea 100644 --- a/module/Core/src/Importer/ShortUrlImporting.php +++ b/module/Core/src/Importer/ShortUrlImporting.php @@ -14,6 +14,7 @@ use Shlinkio\Shlink\Importer\Model\ImportedShlinkRedirectRule; use Shlinkio\Shlink\Importer\Model\ImportedShlinkVisit; +use function count; use function Shlinkio\Shlink\Core\ArrayUtils\map; use function Shlinkio\Shlink\Core\normalizeDate; use function sprintf; @@ -69,6 +70,10 @@ public function importRedirectRules( EntityManagerInterface $em, ShortUrlRedirectRuleServiceInterface $redirectRuleService, ): void { + if ($this->isNew && count($rules) === 0) { + return; + } + $shortUrl = $this->resolveShortUrl($em); $redirectRules = map( $rules, diff --git a/module/Core/src/RedirectRule/Entity/RedirectCondition.php b/module/Core/src/RedirectRule/Entity/RedirectCondition.php index 602d07b7f..255cb19ea 100644 --- a/module/Core/src/RedirectRule/Entity/RedirectCondition.php +++ b/module/Core/src/RedirectRule/Entity/RedirectCondition.php @@ -24,7 +24,7 @@ class RedirectCondition extends AbstractEntity implements JsonSerializable { private function __construct( - private readonly RedirectConditionType $type, + public readonly RedirectConditionType $type, private readonly string $matchValue, private readonly string|null $matchKey = null, ) { diff --git a/module/Core/test/Geolocation/GeolocationDbUpdaterTest.php b/module/Core/test/Geolocation/GeolocationDbUpdaterTest.php index c29830307..8c9f5a1bd 100644 --- a/module/Core/test/Geolocation/GeolocationDbUpdaterTest.php +++ b/module/Core/test/Geolocation/GeolocationDbUpdaterTest.php @@ -291,7 +291,7 @@ public static function provideTrackingOptions(): iterable private function geolocationDbUpdater(TrackingOptions|null $options = null): GeolocationDbUpdater { $locker = $this->createMock(Lock\LockFactory::class); - $locker->method('createLock')->with($this->isType('string'))->willReturn($this->lock); + $locker->method('createLock')->with($this->isString())->willReturn($this->lock); return new GeolocationDbUpdater($this->dbUpdater, $locker, $options ?? new TrackingOptions(), $this->em, 3); } diff --git a/module/Core/test/Importer/ImportedLinksProcessorTest.php b/module/Core/test/Importer/ImportedLinksProcessorTest.php index 8fb63e683..e8d800a1e 100644 --- a/module/Core/test/Importer/ImportedLinksProcessorTest.php +++ b/module/Core/test/Importer/ImportedLinksProcessorTest.php @@ -14,6 +14,9 @@ use PHPUnit\Framework\TestCase; use RuntimeException; use Shlinkio\Shlink\Core\Importer\ImportedLinksProcessor; +use Shlinkio\Shlink\Core\Model\DeviceType; +use Shlinkio\Shlink\Core\RedirectRule\Entity\ShortUrlRedirectRule; +use Shlinkio\Shlink\Core\RedirectRule\Model\RedirectConditionType; use Shlinkio\Shlink\Core\RedirectRule\ShortUrlRedirectRuleServiceInterface; use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortCodeUniquenessHelperInterface; @@ -24,6 +27,8 @@ use Shlinkio\Shlink\Core\Visit\Model\Visitor; use Shlinkio\Shlink\Core\Visit\Repository\VisitRepository; use Shlinkio\Shlink\Importer\Model\ImportedShlinkOrphanVisit; +use Shlinkio\Shlink\Importer\Model\ImportedShlinkRedirectCondition; +use Shlinkio\Shlink\Importer\Model\ImportedShlinkRedirectRule; use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl; use Shlinkio\Shlink\Importer\Model\ImportedShlinkVisit; use Shlinkio\Shlink\Importer\Model\ImportResult; @@ -71,10 +76,31 @@ protected function setUp(): void #[Test] public function newUrlsWithNoErrorsAreAllPersisted(): void { + $now = Chronos::now(); $urls = [ - new ImportedShlinkUrl(ImportSource::BITLY, 'https://foo', [], Chronos::now(), null, 'foo', null), - new ImportedShlinkUrl(ImportSource::BITLY, 'https://bar', [], Chronos::now(), null, 'bar', 'foo'), - new ImportedShlinkUrl(ImportSource::BITLY, 'https://baz', [], Chronos::now(), null, 'baz', null), + new ImportedShlinkUrl(ImportSource::BITLY, 'https://foo', [], $now, null, 'foo', null), + new ImportedShlinkUrl(ImportSource::BITLY, 'https://bar', [], $now, null, 'bar', 'foo'), + new ImportedShlinkUrl(ImportSource::BITLY, 'https://baz', [], $now, null, 'baz', null, redirectRules: [ + new ImportedShlinkRedirectRule( + longUrl: 'https://example.com/android', + conditions: [ + new ImportedShlinkRedirectCondition( + RedirectConditionType::DEVICE->value, + DeviceType::ANDROID->value, + ), + ], + ), + new ImportedShlinkRedirectRule( + longUrl: 'https://example.com/spain', + conditions: [ + new ImportedShlinkRedirectCondition( + RedirectConditionType::GEOLOCATION_COUNTRY_CODE->value, + 'ES', + ), + new ImportedShlinkRedirectCondition(RedirectConditionType::LANGUAGE->value, 'es-ES'), + ], + ), + ]), ]; $expectedCalls = count($urls); @@ -86,7 +112,19 @@ public function newUrlsWithNoErrorsAreAllPersisted(): void $this->em->expects($this->exactly($expectedCalls))->method('persist')->with( $this->isInstanceOf(ShortUrl::class), ); - $this->io->expects($this->exactly($expectedCalls))->method('text')->with($this->isType('string')); + $this->io->expects($this->exactly($expectedCalls))->method('text')->with($this->isString()); + $this->redirectRuleService->expects($this->once())->method('saveRulesForShortUrl')->with( + $this->isInstanceOf(ShortUrl::class), + $this->callback(function (array $rules): bool { + Assert::assertCount(2, $rules); + Assert::assertInstanceOf(ShortUrlRedirectRule::class, $rules[0]); + Assert::assertInstanceOf(ShortUrlRedirectRule::class, $rules[1]); + Assert::assertCount(1, $rules[0]->mapConditions(fn ($c) => $c)); + Assert::assertCount(2, $rules[1]->mapConditions(fn ($c) => $c)); + + return true; + }), + ); $this->processor->process($this->io, ImportResult::withShortUrls($urls), $this->buildParams()); } @@ -243,7 +281,8 @@ public function visitsArePersistedWithProperShortUrl(ShortUrl $originalShortUrl, if (!$originalShortUrl->getId()) { $this->em->expects($this->never())->method('find'); } else { - $this->em->expects($this->exactly(2))->method('find')->willReturn($foundShortUrl); + // 3 times: Initial short URL checking, before creating redirect rules, before creating visits + $this->em->expects($this->exactly(3))->method('find')->willReturn($foundShortUrl); } $this->em->expects($this->once())->method('persist')->willReturnCallback( static fn (Visit $visit) => Assert::assertSame( diff --git a/module/Core/test/RedirectRule/Entity/RedirectConditionTest.php b/module/Core/test/RedirectRule/Entity/RedirectConditionTest.php index 5a4a2e2be..bec6d2635 100644 --- a/module/Core/test/RedirectRule/Entity/RedirectConditionTest.php +++ b/module/Core/test/RedirectRule/Entity/RedirectConditionTest.php @@ -9,6 +9,8 @@ use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\Core\Model\DeviceType; use Shlinkio\Shlink\Core\RedirectRule\Entity\RedirectCondition; +use Shlinkio\Shlink\Core\RedirectRule\Model\RedirectConditionType; +use Shlinkio\Shlink\Importer\Model\ImportedShlinkRedirectCondition; use Shlinkio\Shlink\IpGeolocation\Model\Location; use const Shlinkio\Shlink\IP_ADDRESS_REQUEST_ATTRIBUTE; @@ -133,4 +135,22 @@ public static function provideVisitsWithCity(): iterable yield 'matching location' => [new Location(city: 'Madrid'), 'Madrid', true]; yield 'matching case-insensitive' => [new Location(city: 'Los Angeles'), 'los angeles', true]; } + + #[Test] + #[TestWith(['invalid', null])] + #[TestWith([RedirectConditionType::DEVICE->value, RedirectConditionType::DEVICE])] + #[TestWith([RedirectConditionType::LANGUAGE->value, RedirectConditionType::LANGUAGE])] + #[TestWith([RedirectConditionType::QUERY_PARAM->value, RedirectConditionType::QUERY_PARAM])] + #[TestWith([RedirectConditionType::IP_ADDRESS->value, RedirectConditionType::IP_ADDRESS])] + #[TestWith( + [RedirectConditionType::GEOLOCATION_COUNTRY_CODE->value, RedirectConditionType::GEOLOCATION_COUNTRY_CODE], + )] + #[TestWith([RedirectConditionType::GEOLOCATION_CITY_NAME->value, RedirectConditionType::GEOLOCATION_CITY_NAME])] + public function canBeCreatedFromImport(string $type, RedirectConditionType|null $expectedType): void + { + $condition = RedirectCondition::fromImport( + new ImportedShlinkRedirectCondition($type, DeviceType::ANDROID->value, ''), + ); + self::assertEquals($expectedType, $condition?->type); + } } diff --git a/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php b/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php index 934d85118..31f42bc76 100644 --- a/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php +++ b/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php @@ -78,7 +78,7 @@ public function findsAndPersistsTagsWrappedIntoCollection(array $tags, array $ex $tagRepo = $this->createMock(TagRepository::class); $tagRepo->expects($this->exactly($expectedLookedOutTags))->method('findOneBy')->with( - $this->isType('array'), + $this->isArray(), )->willReturnCallback(function (array $criteria): Tag|null { ['name' => $name] = $criteria; return $name === 'foo' ? new Tag($name) : null; @@ -115,7 +115,7 @@ public function returnsEmptyCollectionWhenProvidingEmptyListOfTags(): void public function newDomainsAreMemoizedUntilStateIsCleared(): void { $repo = $this->createMock(DomainRepository::class); - $repo->expects($this->exactly(3))->method('findOneBy')->with($this->isType('array'))->willReturn(null); + $repo->expects($this->exactly(3))->method('findOneBy')->with($this->isArray())->willReturn(null); $this->em->method('getRepository')->with(Domain::class)->willReturn($repo); $authority = 'foo.com'; @@ -134,7 +134,7 @@ public function newDomainsAreMemoizedUntilStateIsCleared(): void public function newTagsAreMemoizedUntilStateIsCleared(): void { $tagRepo = $this->createMock(TagRepository::class); - $tagRepo->expects($this->exactly(6))->method('findOneBy')->with($this->isType('array'))->willReturn(null); + $tagRepo->expects($this->exactly(6))->method('findOneBy')->with($this->isArray())->willReturn(null); $this->em->method('getRepository')->with(Tag::class)->willReturn($tagRepo); $tags = ['foo', 'bar']; diff --git a/module/Core/test/ShortUrl/UrlShortenerTest.php b/module/Core/test/ShortUrl/UrlShortenerTest.php index a6cacc469..fb191e955 100644 --- a/module/Core/test/ShortUrl/UrlShortenerTest.php +++ b/module/Core/test/ShortUrl/UrlShortenerTest.php @@ -38,7 +38,7 @@ protected function setUp(): void // FIXME Should use the interface, but it doe snot define wrapInTransaction explicitly $this->em = $this->createMock(EntityManager::class); $this->em->method('persist')->willReturnCallback(fn (ShortUrl $shortUrl) => $shortUrl->setId('10')); - $this->em->method('wrapInTransaction')->with($this->isType('callable'))->willReturnCallback( + $this->em->method('wrapInTransaction')->with($this->isCallable())->willReturnCallback( fn (callable $callback) => $callback(), ); diff --git a/module/Rest/test/Service/ApiKeyServiceTest.php b/module/Rest/test/Service/ApiKeyServiceTest.php index 81bec4ea8..a5b6cecbe 100644 --- a/module/Rest/test/Service/ApiKeyServiceTest.php +++ b/module/Rest/test/Service/ApiKeyServiceTest.php @@ -45,7 +45,7 @@ protected function setUp(): void public function apiKeyIsProperlyCreated(Chronos|null $date, string|null $name, array $roles): void { $this->repo->expects($this->once())->method('nameExists')->with( - ! empty($name) ? $name : $this->isType('string'), + ! empty($name) ? $name : $this->isString(), )->willReturn(false); $this->em->expects($this->once())->method('persist')->with($this->isInstanceOf(ApiKey::class)); @@ -83,7 +83,7 @@ public function autoGeneratedNameIsRegeneratedIfAlreadyExists(): void { $callCount = 0; $this->repo->expects($this->exactly(3))->method('nameExists')->with( - $this->isType('string'), + $this->isString(), )->willReturnCallback(function () use (&$callCount): bool { $callCount++; return $callCount < 3;