From 279bd12a2da5f51e6fd998553a836a66319b8ed0 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 1 Feb 2020 17:34:16 +0100 Subject: [PATCH] Ensured domain can be passed when fetching visits for a short URL --- .../src/Command/ShortUrl/GetVisitsCommand.php | 5 +-- .../Command/ShortUrl/GetVisitsCommandTest.php | 16 +++++---- module/Core/src/Model/ShortUrlIdentifier.php | 4 +-- .../Adapter/VisitsPaginatorAdapter.php | 21 +++++++---- .../Core/src/Repository/VisitRepository.php | 23 +++++++++--- .../Repository/VisitRepositoryInterface.php | 7 +++- module/Core/src/Service/VisitsTracker.php | 11 +++--- .../src/Service/VisitsTrackerInterface.php | 3 +- .../Repository/VisitRepositoryTest.php | 12 +++---- .../Core/test/Service/VisitsTrackerTest.php | 36 +++++++++++++------ .../Rest/src/Action/Visit/GetVisitsAction.php | 5 +-- .../test/Action/Visit/GetVisitsActionTest.php | 5 +-- 12 files changed, 100 insertions(+), 48 deletions(-) diff --git a/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php b/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php index 363ff3aa..1fcee476 100644 --- a/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php +++ b/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php @@ -9,6 +9,7 @@ use Shlinkio\Shlink\CLI\Util\ExitCodes; use Shlinkio\Shlink\CLI\Util\ShlinkTable; use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\Visit; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\VisitsParams; use Shlinkio\Shlink\Core\Service\VisitsTrackerInterface; use Symfony\Component\Console\Input\InputArgument; @@ -65,11 +66,11 @@ class GetVisitsCommand extends AbstractWithDateRangeCommand protected function execute(InputInterface $input, OutputInterface $output): ?int { - $shortCode = $input->getArgument('shortCode'); + $identifier = ShortUrlIdentifier::fromCli($input); $startDate = $this->getDateOption($input, $output, 'startDate'); $endDate = $this->getDateOption($input, $output, 'endDate'); - $paginator = $this->visitsTracker->info($shortCode, new VisitsParams(new DateRange($startDate, $endDate))); + $paginator = $this->visitsTracker->info($identifier, new VisitsParams(new DateRange($startDate, $endDate))); $rows = map($paginator->getCurrentItems(), function (Visit $visit) { $rowData = $visit->jsonSerialize(); diff --git a/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php b/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php index 53ba114e..a725240e 100644 --- a/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php @@ -15,6 +15,7 @@ use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Entity\VisitLocation; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Model\VisitsParams; use Shlinkio\Shlink\Core\Service\VisitsTrackerInterface; @@ -42,9 +43,12 @@ class GetVisitsCommandTest extends TestCase public function noDateFlagsTriesToListWithoutDateRange(): void { $shortCode = 'abc123'; - $this->visitsTracker->info($shortCode, new VisitsParams(new DateRange(null, null)))->willReturn( - new Paginator(new ArrayAdapter([])), - )->shouldBeCalledOnce(); + $this->visitsTracker->info( + new ShortUrlIdentifier($shortCode), + new VisitsParams(new DateRange(null, null)), + ) + ->willReturn(new Paginator(new ArrayAdapter([]))) + ->shouldBeCalledOnce(); $this->commandTester->execute(['shortCode' => $shortCode]); } @@ -56,7 +60,7 @@ class GetVisitsCommandTest extends TestCase $startDate = '2016-01-01'; $endDate = '2016-02-01'; $this->visitsTracker->info( - $shortCode, + new ShortUrlIdentifier($shortCode), new VisitsParams(new DateRange(Chronos::parse($startDate), Chronos::parse($endDate))), ) ->willReturn(new Paginator(new ArrayAdapter([]))) @@ -74,7 +78,7 @@ class GetVisitsCommandTest extends TestCase { $shortCode = 'abc123'; $startDate = 'foo'; - $info = $this->visitsTracker->info($shortCode, new VisitsParams(new DateRange())) + $info = $this->visitsTracker->info(new ShortUrlIdentifier($shortCode), new VisitsParams(new DateRange())) ->willReturn(new Paginator(new ArrayAdapter([]))); $this->commandTester->execute([ @@ -94,7 +98,7 @@ class GetVisitsCommandTest extends TestCase public function outputIsProperlyGenerated(): void { $shortCode = 'abc123'; - $this->visitsTracker->info($shortCode, Argument::any())->willReturn( + $this->visitsTracker->info(new ShortUrlIdentifier($shortCode), Argument::any())->willReturn( new Paginator(new ArrayAdapter([ (new Visit(new ShortUrl(''), new Visitor('bar', 'foo', '')))->locate( new VisitLocation(new Location('', 'Spain', '', '', 0, 0, '')), diff --git a/module/Core/src/Model/ShortUrlIdentifier.php b/module/Core/src/Model/ShortUrlIdentifier.php index 820bcf03..4a74ba07 100644 --- a/module/Core/src/Model/ShortUrlIdentifier.php +++ b/module/Core/src/Model/ShortUrlIdentifier.php @@ -36,8 +36,8 @@ final class ShortUrlIdentifier public static function fromCli(InputInterface $input): self { - $shortCode = $input->getArgument('shortCode'); - $domain = $input->getOption('domain'); + $shortCode = $input->getArguments()['shortCode'] ?? ''; + $domain = $input->getOptions()['domain'] ?? null; return new self($shortCode, $domain); } diff --git a/module/Core/src/Paginator/Adapter/VisitsPaginatorAdapter.php b/module/Core/src/Paginator/Adapter/VisitsPaginatorAdapter.php index 19baff73..247ea93e 100644 --- a/module/Core/src/Paginator/Adapter/VisitsPaginatorAdapter.php +++ b/module/Core/src/Paginator/Adapter/VisitsPaginatorAdapter.php @@ -5,26 +5,31 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Paginator\Adapter; use Laminas\Paginator\Adapter\AdapterInterface; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\VisitsParams; use Shlinkio\Shlink\Core\Repository\VisitRepositoryInterface; class VisitsPaginatorAdapter implements AdapterInterface { private VisitRepositoryInterface $visitRepository; - private string $shortCode; + private ShortUrlIdentifier $identifier; private VisitsParams $params; - public function __construct(VisitRepositoryInterface $visitRepository, string $shortCode, VisitsParams $params) - { + public function __construct( + VisitRepositoryInterface $visitRepository, + ShortUrlIdentifier $identifier, + VisitsParams $params + ) { $this->visitRepository = $visitRepository; - $this->shortCode = $shortCode; $this->params = $params; + $this->identifier = $identifier; } public function getItems($offset, $itemCountPerPage): array // phpcs:ignore { return $this->visitRepository->findVisitsByShortCode( - $this->shortCode, + $this->identifier->shortCode(), + $this->identifier->domain(), $this->params->getDateRange(), $itemCountPerPage, $offset, @@ -33,6 +38,10 @@ class VisitsPaginatorAdapter implements AdapterInterface public function count(): int { - return $this->visitRepository->countVisitsByShortCode($this->shortCode, $this->params->getDateRange()); + return $this->visitRepository->countVisitsByShortCode( + $this->identifier->shortCode(), + $this->identifier->domain(), + $this->params->getDateRange(), + ); } } diff --git a/module/Core/src/Repository/VisitRepository.php b/module/Core/src/Repository/VisitRepository.php index 0e48e0a1..454323ef 100644 --- a/module/Core/src/Repository/VisitRepository.php +++ b/module/Core/src/Repository/VisitRepository.php @@ -46,11 +46,12 @@ DQL; */ public function findVisitsByShortCode( string $shortCode, + ?string $domain = null, ?DateRange $dateRange = null, ?int $limit = null, ?int $offset = null ): array { - $qb = $this->createVisitsByShortCodeQueryBuilder($shortCode, $dateRange); + $qb = $this->createVisitsByShortCodeQueryBuilder($shortCode, $domain, $dateRange); $qb->select('v') ->orderBy('v.date', 'DESC'); @@ -64,22 +65,34 @@ DQL; return $qb->getQuery()->getResult(); } - public function countVisitsByShortCode(string $shortCode, ?DateRange $dateRange = null): int + public function countVisitsByShortCode(string $shortCode, ?string $domain = null, ?DateRange $dateRange = null): int { - $qb = $this->createVisitsByShortCodeQueryBuilder($shortCode, $dateRange); + $qb = $this->createVisitsByShortCodeQueryBuilder($shortCode, $domain, $dateRange); $qb->select('COUNT(DISTINCT v.id)'); return (int) $qb->getQuery()->getSingleScalarResult(); } - private function createVisitsByShortCodeQueryBuilder(string $shortCode, ?DateRange $dateRange = null): QueryBuilder - { + private function createVisitsByShortCodeQueryBuilder( + string $shortCode, + ?string $domain, + ?DateRange $dateRange + ): QueryBuilder { $qb = $this->getEntityManager()->createQueryBuilder(); $qb->from(Visit::class, 'v') ->join('v.shortUrl', 'su') ->where($qb->expr()->eq('su.shortCode', ':shortCode')) ->setParameter('shortCode', $shortCode); + // Apply domain filtering + if ($domain !== null) { + $qb->join('su.domain', 'd') + ->andWhere($qb->expr()->eq('d.authority', ':domain')) + ->setParameter('domain', $domain); + } else { + $qb->andWhere($qb->expr()->isNull('su.domain')); + } + // Apply date range filtering if ($dateRange !== null && $dateRange->getStartDate() !== null) { $qb->andWhere($qb->expr()->gte('v.date', ':startDate')) diff --git a/module/Core/src/Repository/VisitRepositoryInterface.php b/module/Core/src/Repository/VisitRepositoryInterface.php index e70c989e..0d0b66d0 100644 --- a/module/Core/src/Repository/VisitRepositoryInterface.php +++ b/module/Core/src/Repository/VisitRepositoryInterface.php @@ -28,10 +28,15 @@ interface VisitRepositoryInterface extends ObjectRepository */ public function findVisitsByShortCode( string $shortCode, + ?string $domain = null, ?DateRange $dateRange = null, ?int $limit = null, ?int $offset = null ): array; - public function countVisitsByShortCode(string $shortCode, ?DateRange $dateRange = null): int; + public function countVisitsByShortCode( + string $shortCode, + ?string $domain = null, + ?DateRange $dateRange = null + ): int; } diff --git a/module/Core/src/Service/VisitsTracker.php b/module/Core/src/Service/VisitsTracker.php index ca1a65f8..54abe319 100644 --- a/module/Core/src/Service/VisitsTracker.php +++ b/module/Core/src/Service/VisitsTracker.php @@ -15,6 +15,7 @@ use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Model\VisitsParams; use Shlinkio\Shlink\Core\Paginator\Adapter\VisitsPaginatorAdapter; +use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; use Shlinkio\Shlink\Core\Repository\VisitRepository; class VisitsTracker implements VisitsTrackerInterface @@ -47,17 +48,17 @@ class VisitsTracker implements VisitsTrackerInterface * @return Visit[]|Paginator * @throws ShortUrlNotFoundException */ - public function info(string $shortCode, VisitsParams $params): Paginator + public function info(ShortUrlIdentifier $identifier, VisitsParams $params): Paginator { - /** @var ORM\EntityRepository $repo */ + /** @var ShortUrlRepositoryInterface $repo */ $repo = $this->em->getRepository(ShortUrl::class); - if ($repo->count(['shortCode' => $shortCode]) < 1) { - throw ShortUrlNotFoundException::fromNotFound(new ShortUrlIdentifier($shortCode)); // FIXME + if (! $repo->shortCodeIsInUse($identifier->shortCode(), $identifier->domain())) { + throw ShortUrlNotFoundException::fromNotFound($identifier); } /** @var VisitRepository $repo */ $repo = $this->em->getRepository(Visit::class); - $paginator = new Paginator(new VisitsPaginatorAdapter($repo, $shortCode, $params)); + $paginator = new Paginator(new VisitsPaginatorAdapter($repo, $identifier, $params)); $paginator->setItemCountPerPage($params->getItemsPerPage()) ->setCurrentPageNumber($params->getPage()); diff --git a/module/Core/src/Service/VisitsTrackerInterface.php b/module/Core/src/Service/VisitsTrackerInterface.php index 3f19b054..862ef190 100644 --- a/module/Core/src/Service/VisitsTrackerInterface.php +++ b/module/Core/src/Service/VisitsTrackerInterface.php @@ -8,6 +8,7 @@ use Laminas\Paginator\Paginator; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Model\VisitsParams; @@ -24,5 +25,5 @@ interface VisitsTrackerInterface * @return Visit[]|Paginator * @throws ShortUrlNotFoundException */ - public function info(string $shortCode, VisitsParams $params): Paginator; // FIXME + public function info(ShortUrlIdentifier $identifier, VisitsParams $params): Paginator; // FIXME } diff --git a/module/Core/test-db/Repository/VisitRepositoryTest.php b/module/Core/test-db/Repository/VisitRepositoryTest.php index 80207d5a..d4a757e1 100644 --- a/module/Core/test-db/Repository/VisitRepositoryTest.php +++ b/module/Core/test-db/Repository/VisitRepositoryTest.php @@ -83,15 +83,15 @@ class VisitRepositoryTest extends DatabaseTestCase $this->assertCount(0, $this->repo->findVisitsByShortCode('invalid')); $this->assertCount(6, $this->repo->findVisitsByShortCode($shortUrl->getShortCode())); - $this->assertCount(2, $this->repo->findVisitsByShortCode($shortUrl->getShortCode(), new DateRange( + $this->assertCount(2, $this->repo->findVisitsByShortCode($shortUrl->getShortCode(), null, new DateRange( Chronos::parse('2016-01-02'), Chronos::parse('2016-01-03'), ))); - $this->assertCount(4, $this->repo->findVisitsByShortCode($shortUrl->getShortCode(), new DateRange( + $this->assertCount(4, $this->repo->findVisitsByShortCode($shortUrl->getShortCode(), null, new DateRange( Chronos::parse('2016-01-03'), ))); - $this->assertCount(3, $this->repo->findVisitsByShortCode($shortUrl->getShortCode(), null, 3, 2)); - $this->assertCount(2, $this->repo->findVisitsByShortCode($shortUrl->getShortCode(), null, 5, 4)); + $this->assertCount(3, $this->repo->findVisitsByShortCode($shortUrl->getShortCode(), null, null, 3, 2)); + $this->assertCount(2, $this->repo->findVisitsByShortCode($shortUrl->getShortCode(), null, null, 5, 4)); } /** @test */ @@ -108,11 +108,11 @@ class VisitRepositoryTest extends DatabaseTestCase $this->assertEquals(0, $this->repo->countVisitsByShortCode('invalid')); $this->assertEquals(6, $this->repo->countVisitsByShortCode($shortUrl->getShortCode())); - $this->assertEquals(2, $this->repo->countVisitsByShortCode($shortUrl->getShortCode(), new DateRange( + $this->assertEquals(2, $this->repo->countVisitsByShortCode($shortUrl->getShortCode(), null, new DateRange( Chronos::parse('2016-01-02'), Chronos::parse('2016-01-03'), ))); - $this->assertEquals(4, $this->repo->countVisitsByShortCode($shortUrl->getShortCode(), new DateRange( + $this->assertEquals(4, $this->repo->countVisitsByShortCode($shortUrl->getShortCode(), null, new DateRange( Chronos::parse('2016-01-03'), ))); } diff --git a/module/Core/test/Service/VisitsTrackerTest.php b/module/Core/test/Service/VisitsTrackerTest.php index 61f1e16d..9028d2c7 100644 --- a/module/Core/test/Service/VisitsTrackerTest.php +++ b/module/Core/test/Service/VisitsTrackerTest.php @@ -5,7 +5,6 @@ declare(strict_types=1); namespace ShlinkioTest\Shlink\Core\Service; use Doctrine\ORM\EntityManager; -use Doctrine\ORM\EntityRepository; use Laminas\Stdlib\ArrayUtils; use PHPUnit\Framework\Assert; use PHPUnit\Framework\TestCase; @@ -16,11 +15,17 @@ use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\EventDispatcher\ShortUrlVisited; +use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Model\VisitsParams; +use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; use Shlinkio\Shlink\Core\Repository\VisitRepository; use Shlinkio\Shlink\Core\Service\VisitsTracker; +use function Functional\map; +use function range; + class VisitsTrackerTest extends TestCase { private VisitsTracker $visitsTracker; @@ -71,22 +76,33 @@ class VisitsTrackerTest extends TestCase public function infoReturnsVisitsForCertainShortCode(): void { $shortCode = '123ABC'; - $repo = $this->prophesize(EntityRepository::class); - $count = $repo->count(['shortCode' => $shortCode])->willReturn(1); + $repo = $this->prophesize(ShortUrlRepositoryInterface::class); + $count = $repo->shortCodeIsInUse($shortCode, null)->willReturn(true); $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal())->shouldBeCalledOnce(); - $list = [ - new Visit(new ShortUrl(''), Visitor::emptyInstance()), - new Visit(new ShortUrl(''), Visitor::emptyInstance()), - ]; + $list = map(range(0, 1), fn () => new Visit(new ShortUrl(''), Visitor::emptyInstance())); $repo2 = $this->prophesize(VisitRepository::class); - $repo2->findVisitsByShortCode($shortCode, Argument::type(DateRange::class), 1, 0)->willReturn($list); - $repo2->countVisitsByShortCode($shortCode, Argument::type(DateRange::class))->willReturn(1); + $repo2->findVisitsByShortCode($shortCode, null, Argument::type(DateRange::class), 1, 0)->willReturn($list); + $repo2->countVisitsByShortCode($shortCode, null, Argument::type(DateRange::class))->willReturn(1); $this->em->getRepository(Visit::class)->willReturn($repo2->reveal())->shouldBeCalledOnce(); - $paginator = $this->visitsTracker->info($shortCode, new VisitsParams()); + $paginator = $this->visitsTracker->info(new ShortUrlIdentifier($shortCode), new VisitsParams()); $this->assertEquals($list, ArrayUtils::iteratorToArray($paginator->getCurrentItems())); $count->shouldHaveBeenCalledOnce(); } + + /** @test */ + public function throwsExceptionWhenRequestingVisitsForInvalidShortCode(): void + { + $shortCode = '123ABC'; + $repo = $this->prophesize(ShortUrlRepositoryInterface::class); + $count = $repo->shortCodeIsInUse($shortCode, null)->willReturn(false); + $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal())->shouldBeCalledOnce(); + + $this->expectException(ShortUrlNotFoundException::class); + $count->shouldBeCalledOnce(); + + $this->visitsTracker->info(new ShortUrlIdentifier($shortCode), new VisitsParams()); + } } diff --git a/module/Rest/src/Action/Visit/GetVisitsAction.php b/module/Rest/src/Action/Visit/GetVisitsAction.php index c1fa0095..bd6ae5a5 100644 --- a/module/Rest/src/Action/Visit/GetVisitsAction.php +++ b/module/Rest/src/Action/Visit/GetVisitsAction.php @@ -9,6 +9,7 @@ use Psr\Http\Message\ResponseInterface as Response; use Psr\Http\Message\ServerRequestInterface as Request; use Psr\Log\LoggerInterface; use Shlinkio\Shlink\Common\Paginator\Util\PaginatorUtilsTrait; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\VisitsParams; use Shlinkio\Shlink\Core\Service\VisitsTrackerInterface; use Shlinkio\Shlink\Rest\Action\AbstractRestAction; @@ -30,8 +31,8 @@ class GetVisitsAction extends AbstractRestAction public function handle(Request $request): Response { - $shortCode = $request->getAttribute('shortCode'); - $visits = $this->visitsTracker->info($shortCode, VisitsParams::fromRawData($request->getQueryParams())); + $identifier = ShortUrlIdentifier::fromApiRequest($request); + $visits = $this->visitsTracker->info($identifier, VisitsParams::fromRawData($request->getQueryParams())); return new JsonResponse([ 'visits' => $this->serializePaginator($visits), diff --git a/module/Rest/test/Action/Visit/GetVisitsActionTest.php b/module/Rest/test/Action/Visit/GetVisitsActionTest.php index a445c3b2..a1f1681a 100644 --- a/module/Rest/test/Action/Visit/GetVisitsActionTest.php +++ b/module/Rest/test/Action/Visit/GetVisitsActionTest.php @@ -12,6 +12,7 @@ use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Common\Util\DateRange; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\VisitsParams; use Shlinkio\Shlink\Core\Service\VisitsTracker; use Shlinkio\Shlink\Rest\Action\Visit\GetVisitsAction; @@ -31,7 +32,7 @@ class GetVisitsActionTest extends TestCase public function providingCorrectShortCodeReturnsVisits(): void { $shortCode = 'abc123'; - $this->visitsTracker->info($shortCode, Argument::type(VisitsParams::class))->willReturn( + $this->visitsTracker->info(new ShortUrlIdentifier($shortCode), Argument::type(VisitsParams::class))->willReturn( new Paginator(new ArrayAdapter([])), )->shouldBeCalledOnce(); @@ -43,7 +44,7 @@ class GetVisitsActionTest extends TestCase public function paramsAreReadFromQuery(): void { $shortCode = 'abc123'; - $this->visitsTracker->info($shortCode, new VisitsParams( + $this->visitsTracker->info(new ShortUrlIdentifier($shortCode), new VisitsParams( new DateRange(null, Chronos::parse('2016-01-01 00:00:00')), 3, 10,