diff --git a/composer.json b/composer.json index 0c088a77..87275e92 100644 --- a/composer.json +++ b/composer.json @@ -124,6 +124,7 @@ ], "test:unit": "@php vendor/bin/phpunit --order-by=random --colors=always --coverage-php build/coverage-unit.cov --testdox", "test:unit:ci": "@test:unit --coverage-xml=build/coverage-unit/coverage-xml --log-junit=build/coverage-unit/junit.xml", + "test:unit:pretty": "@php vendor/bin/phpunit --order-by=random --colors=always --coverage-html build/coverage-unit-html", "test:db": "@parallel test:db:sqlite:ci test:db:mysql test:db:maria test:db:postgres test:db:ms", "test:db:sqlite": "APP_ENV=test php vendor/bin/phpunit --order-by=random --colors=always --testdox -c phpunit-db.xml", "test:db:sqlite:ci": "@test:db:sqlite --coverage-php build/coverage-db.cov --coverage-xml=build/coverage-db/coverage-xml --log-junit=build/coverage-db/junit.xml", @@ -132,7 +133,6 @@ "test:db:postgres": "DB_DRIVER=postgres composer test:db:sqlite", "test:db:ms": "DB_DRIVER=mssql composer test:db:sqlite", "test:api": "bin/test/run-api-tests.sh", - "test:unit:pretty": "@php vendor/bin/phpunit --order-by=random --colors=always --coverage-html build/coverage-unit-html", "infect:ci:base": "infection --threads=4 --log-verbosity=default --only-covered --skip-initial-tests", "infect:ci:unit": "@infect:ci:base --coverage=build/coverage-unit --min-msi=80", "infect:ci:db": "@infect:ci:base --coverage=build/coverage-db --min-msi=95 --configuration=infection-db.json", diff --git a/module/Core/src/Entity/Visit.php b/module/Core/src/Entity/Visit.php index fd1b830b..98d1a4c5 100644 --- a/module/Core/src/Entity/Visit.php +++ b/module/Core/src/Entity/Visit.php @@ -150,6 +150,15 @@ class Visit extends AbstractEntity implements JsonSerializable return $this->type; } + /** + * Needed only for ArrayCollections to be able to apply criteria filtering + * @internal + */ + public function getType(): string + { + return $this->type(); + } + public function jsonSerialize(): array { return [ diff --git a/module/Core/src/Importer/ImportedLinksProcessor.php b/module/Core/src/Importer/ImportedLinksProcessor.php index 4091906d..25393055 100644 --- a/module/Core/src/Importer/ImportedLinksProcessor.php +++ b/module/Core/src/Importer/ImportedLinksProcessor.php @@ -49,9 +49,7 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface /** @var ImportedShlinkUrl $importedUrl */ foreach ($iterable as $importedUrl) { - $longUrl = $importedUrl->longUrl(); - - $generateNewIfDuplicated = static function () use ($io, $importedUrl): bool { + $skipOnShortCodeConflict = static function () use ($io, $importedUrl): bool { $action = $io->choice(sprintf( 'Failed to import URL "%s" because its short-code "%s" is already in use. Do you want to generate ' . 'a new one or skip it?', @@ -59,10 +57,11 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface $importedUrl->shortCode(), ), ['Generate new short-code', 'Skip'], 1); - return $action !== 'Skip'; + return $action === 'Skip'; }; - [$shortUrl, $isNew] = $this->getOrCreateShortUrl($importedUrl, $importShortCodes, $generateNewIfDuplicated); + [$shortUrl, $isNew] = $this->getOrCreateShortUrl($importedUrl, $importShortCodes, $skipOnShortCodeConflict); + $longUrl = $importedUrl->longUrl(); if ($shortUrl === null) { $io->text(sprintf('%s: Error', $longUrl)); continue; @@ -93,15 +92,15 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface private function getOrCreateShortUrl( ImportedShlinkUrl $importedUrl, bool $importShortCodes, - callable $generateNewIfDuplicated + callable $skipOnShortCodeConflict ): array { - $existingShortUrl = $this->shortUrlRepo->findOneByImportedUrl($importedUrl); - if ($existingShortUrl !== null) { - return [$existingShortUrl, false]; + $alreadyImportedShortUrl = $this->shortUrlRepo->findOneByImportedUrl($importedUrl); + if ($alreadyImportedShortUrl !== null) { + return [$alreadyImportedShortUrl, false]; } $shortUrl = ShortUrl::fromImport($importedUrl, $importShortCodes, $this->relationResolver); - if (! $this->handleShortCodeUniqueness($shortUrl, $importShortCodes, $generateNewIfDuplicated)) { + if (! $this->handleShortCodeUniqueness($shortUrl, $importShortCodes, $skipOnShortCodeConflict)) { return [null, false]; } @@ -112,13 +111,13 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface private function handleShortCodeUniqueness( ShortUrl $shortUrl, bool $importShortCodes, - callable $generateNewIfDuplicated + callable $skipOnShortCodeConflict ): bool { if ($this->shortCodeHelper->ensureShortCodeUniqueness($shortUrl, $importShortCodes)) { return true; } - if (! $generateNewIfDuplicated()) { + if ($skipOnShortCodeConflict()) { return false; } diff --git a/module/Core/test/Importer/ImportedLinksProcessorTest.php b/module/Core/test/Importer/ImportedLinksProcessorTest.php index df650e18..170e21b7 100644 --- a/module/Core/test/Importer/ImportedLinksProcessorTest.php +++ b/module/Core/test/Importer/ImportedLinksProcessorTest.php @@ -5,18 +5,21 @@ declare(strict_types=1); namespace ShlinkioTest\Shlink\Core\Importer; use Cake\Chronos\Chronos; +use Doctrine\Common\Collections\ArrayCollection; use Doctrine\ORM\EntityManagerInterface; use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\PhpUnit\ProphecyTrait; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Core\Entity\ShortUrl; +use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Importer\ImportedLinksProcessor; use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortCodeHelperInterface; use Shlinkio\Shlink\Core\ShortUrl\Resolver\SimpleShortUrlRelationResolver; use Shlinkio\Shlink\Core\Util\DoctrineBatchHelperInterface; use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl; +use Shlinkio\Shlink\Importer\Model\ImportedShlinkVisit; use Symfony\Component\Console\Style\StyleInterface; use function count; @@ -86,7 +89,6 @@ class ImportedLinksProcessorTest extends TestCase new ImportedShlinkUrl('', 'baz2', [], Chronos::now(), null, 'baz2', null), new ImportedShlinkUrl('', 'baz3', [], Chronos::now(), null, 'baz3', null), ]; - $contains = fn (string $needle) => fn (string $text) => str_contains($text, $needle); $importedUrlExists = $this->repo->findOneByImportedUrl(Argument::cetera())->will( function (array $args): ?ShortUrl { @@ -104,8 +106,8 @@ class ImportedLinksProcessorTest extends TestCase $importedUrlExists->shouldHaveBeenCalledTimes(count($urls)); $ensureUniqueness->shouldHaveBeenCalledTimes(2); $persist->shouldHaveBeenCalledTimes(2); - $this->io->text(Argument::that($contains('Skipped')))->shouldHaveBeenCalledTimes(3); - $this->io->text(Argument::that($contains('Imported')))->shouldHaveBeenCalledTimes(2); + $this->io->text(Argument::containingString('Skipped'))->shouldHaveBeenCalledTimes(3); + $this->io->text(Argument::containingString('Imported'))->shouldHaveBeenCalledTimes(2); } /** @test */ @@ -118,7 +120,6 @@ class ImportedLinksProcessorTest extends TestCase new ImportedShlinkUrl('', 'baz2', [], Chronos::now(), null, 'baz2', null), new ImportedShlinkUrl('', 'baz3', [], Chronos::now(), null, 'baz3', 'bar'), ]; - $contains = fn (string $needle) => fn (string $text) => str_contains($text, $needle); $importedUrlExists = $this->repo->findOneByImportedUrl(Argument::cetera())->willReturn(null); $failingEnsureUniqueness = $this->shortCodeHelper->ensureShortCodeUniqueness( @@ -144,7 +145,70 @@ class ImportedLinksProcessorTest extends TestCase $successEnsureUniqueness->shouldHaveBeenCalledTimes(2); $choice->shouldHaveBeenCalledTimes(5); $persist->shouldHaveBeenCalledTimes(2); - $this->io->text(Argument::that($contains('Skipped')))->shouldHaveBeenCalledTimes(3); - $this->io->text(Argument::that($contains('Imported')))->shouldHaveBeenCalledTimes(2); + $this->io->text(Argument::containingString('Error'))->shouldHaveBeenCalledTimes(3); + $this->io->text(Argument::containingString('Imported'))->shouldHaveBeenCalledTimes(2); + } + + /** + * @test + * @dataProvider provideUrlsWithVisits + */ + public function properAmountOfVisitsIsImported( + ImportedShlinkUrl $importedUrl, + string $expectedOutput, + int $amountOfPersistedVisits, + ?ShortUrl $foundShortUrl + ): void { + $findExisting = $this->repo->findOneByImportedUrl(Argument::cetera())->willReturn($foundShortUrl); + $ensureUniqueness = $this->shortCodeHelper->ensureShortCodeUniqueness(Argument::cetera())->willReturn(true); + $persistUrl = $this->em->persist(Argument::type(ShortUrl::class)); + $persistVisits = $this->em->persist(Argument::type(Visit::class)); + + $this->processor->process($this->io->reveal(), [$importedUrl], ['import_short_codes' => true]); + + $findExisting->shouldHaveBeenCalledOnce(); + $ensureUniqueness->shouldHaveBeenCalledTimes($foundShortUrl === null ? 1 : 0); + $persistUrl->shouldHaveBeenCalledTimes($foundShortUrl === null ? 1 : 0); + $persistVisits->shouldHaveBeenCalledTimes($amountOfPersistedVisits); + $this->io->text(Argument::containingString($expectedOutput))->shouldHaveBeenCalledOnce(); + } + + public function provideUrlsWithVisits(): iterable + { + $now = Chronos::now(); + $createImportedUrl = fn (array $visits) => new ImportedShlinkUrl('', 's', [], $now, null, 's', null, $visits); + + yield 'new short URL' => [$createImportedUrl([ + new ImportedShlinkVisit('', '', $now, null), + new ImportedShlinkVisit('', '', $now, null), + new ImportedShlinkVisit('', '', $now, null), + new ImportedShlinkVisit('', '', $now, null), + new ImportedShlinkVisit('', '', $now, null), + ]), 'Imported with 5 visits', 5, null]; + yield 'existing short URL without previous imported visits' => [ + $createImportedUrl([ + new ImportedShlinkVisit('', '', $now, null), + new ImportedShlinkVisit('', '', $now, null), + new ImportedShlinkVisit('', '', $now->addDays(3), null), + new ImportedShlinkVisit('', '', $now->addDays(3), null), + ]), + 'Skipped. Imported 4 visits', + 4, + ShortUrl::createEmpty(), + ]; + yield 'existing short URL with previous imported visits' => [ + $createImportedUrl([ + new ImportedShlinkVisit('', '', $now, null), + new ImportedShlinkVisit('', '', $now, null), + new ImportedShlinkVisit('', '', $now, null), + new ImportedShlinkVisit('', '', $now->addDays(3), null), + new ImportedShlinkVisit('', '', $now->addDays(3), null), + ]), + 'Skipped. Imported 2 visits', + 2, + ShortUrl::createEmpty()->setVisits(new ArrayCollection([ + Visit::fromImport(ShortUrl::createEmpty(), new ImportedShlinkVisit('', '', $now, null)), + ])), + ]; } }