From 45254606d45856033f389c9e1cea9e6a453196a9 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 27 Nov 2018 21:09:27 +0100 Subject: [PATCH 1/7] Added DTO used to pass filtering params to VisitsTracker --- .../src/Command/ShortUrl/GetVisitsCommand.php | 3 +- .../Command/ShortUrl/GetVisitsCommandTest.php | 10 +++-- module/Core/src/Model/VisitsParams.php | 43 +++++++++++++++++++ module/Core/src/Service/VisitsTracker.php | 8 ++-- .../src/Service/VisitsTrackerInterface.php | 6 +-- .../Core/test/Service/VisitsTrackerTest.php | 6 ++- .../Rest/src/Action/Visit/GetVisitsAction.php | 10 +---- .../test/Action/Visit/GetVisitsActionTest.php | 24 +++-------- 8 files changed, 69 insertions(+), 41 deletions(-) create mode 100644 module/Core/src/Model/VisitsParams.php diff --git a/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php b/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php index 467a76fa..55c3a569 100644 --- a/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php +++ b/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php @@ -6,6 +6,7 @@ namespace Shlinkio\Shlink\CLI\Command\ShortUrl; use Cake\Chronos\Chronos; use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\Visit; +use Shlinkio\Shlink\Core\Model\VisitsParams; use Shlinkio\Shlink\Core\Service\VisitsTrackerInterface; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputArgument; @@ -72,7 +73,7 @@ class GetVisitsCommand extends Command $startDate = $this->getDateOption($input, 'startDate'); $endDate = $this->getDateOption($input, 'endDate'); - $visits = $this->visitsTracker->info($shortCode, new DateRange($startDate, $endDate)); + $visits = $this->visitsTracker->info($shortCode, new VisitsParams(new DateRange($startDate, $endDate))); $rows = array_map(function (Visit $visit) { $rowData = $visit->jsonSerialize(); $rowData['country'] = $visit->getVisitLocation()->getCountryName(); diff --git a/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php b/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php index 9c4947dd..33a7c662 100644 --- a/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php @@ -13,6 +13,7 @@ use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Entity\VisitLocation; use Shlinkio\Shlink\Core\Model\Visitor; +use Shlinkio\Shlink\Core\Model\VisitsParams; use Shlinkio\Shlink\Core\Service\VisitsTrackerInterface; use Symfony\Component\Console\Application; use Symfony\Component\Console\Tester\CommandTester; @@ -40,8 +41,8 @@ class GetVisitsCommandTest extends TestCase public function noDateFlagsTriesToListWithoutDateRange() { $shortCode = 'abc123'; - $this->visitsTracker->info($shortCode, new DateRange(null, null))->willReturn([]) - ->shouldBeCalledOnce(); + $this->visitsTracker->info($shortCode, new VisitsParams(new DateRange(null, null)))->willReturn([]) + ->shouldBeCalledOnce(); $this->commandTester->execute([ 'command' => 'shortcode:visits', @@ -57,7 +58,10 @@ class GetVisitsCommandTest extends TestCase $shortCode = 'abc123'; $startDate = '2016-01-01'; $endDate = '2016-02-01'; - $this->visitsTracker->info($shortCode, new DateRange(Chronos::parse($startDate), Chronos::parse($endDate))) + $this->visitsTracker->info( + $shortCode, + new VisitsParams(new DateRange(Chronos::parse($startDate), Chronos::parse($endDate))) + ) ->willReturn([]) ->shouldBeCalledOnce(); diff --git a/module/Core/src/Model/VisitsParams.php b/module/Core/src/Model/VisitsParams.php new file mode 100644 index 00000000..734dbf57 --- /dev/null +++ b/module/Core/src/Model/VisitsParams.php @@ -0,0 +1,43 @@ +dateRange = $dateRange ?? new DateRange(); + $this->page = $page; + $this->itemsPerPage = $itemsPerPage; + } + + public function getDateRange(): DateRange + { + return $this->dateRange; + } + + public function getPage(): int + { + return $this->page; + } + + public function getItemsPerPage(): ?int + { + return $this->itemsPerPage; + } + + public function hasItemsPerPage(): bool + { + return $this->itemsPerPage !== null; + } +} diff --git a/module/Core/src/Service/VisitsTracker.php b/module/Core/src/Service/VisitsTracker.php index 4bf8111a..d05735ed 100644 --- a/module/Core/src/Service/VisitsTracker.php +++ b/module/Core/src/Service/VisitsTracker.php @@ -4,11 +4,11 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Service; use Doctrine\ORM; -use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Exception\InvalidArgumentException; use Shlinkio\Shlink\Core\Model\Visitor; +use Shlinkio\Shlink\Core\Model\VisitsParams; use Shlinkio\Shlink\Core\Repository\VisitRepository; use function sprintf; @@ -43,12 +43,10 @@ class VisitsTracker implements VisitsTrackerInterface /** * Returns the visits on certain short code * - * @param string $shortCode - * @param DateRange $dateRange * @return Visit[] * @throws InvalidArgumentException */ - public function info(string $shortCode, DateRange $dateRange = null): array + public function info(string $shortCode, VisitsParams $params): array { /** @var ShortUrl|null $shortUrl */ $shortUrl = $this->em->getRepository(ShortUrl::class)->findOneBy([ @@ -60,6 +58,6 @@ class VisitsTracker implements VisitsTrackerInterface /** @var VisitRepository $repo */ $repo = $this->em->getRepository(Visit::class); - return $repo->findVisitsByShortUrl($shortUrl, $dateRange); + return $repo->findVisitsByShortUrl($shortUrl, $params->getDateRange()); } } diff --git a/module/Core/src/Service/VisitsTrackerInterface.php b/module/Core/src/Service/VisitsTrackerInterface.php index 79676864..e895dae3 100644 --- a/module/Core/src/Service/VisitsTrackerInterface.php +++ b/module/Core/src/Service/VisitsTrackerInterface.php @@ -3,10 +3,10 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Service; -use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Exception\InvalidArgumentException; use Shlinkio\Shlink\Core\Model\Visitor; +use Shlinkio\Shlink\Core\Model\VisitsParams; interface VisitsTrackerInterface { @@ -18,10 +18,8 @@ interface VisitsTrackerInterface /** * Returns the visits on certain short code * - * @param string $shortCode - * @param DateRange $dateRange * @return Visit[] * @throws InvalidArgumentException */ - public function info(string $shortCode, DateRange $dateRange = null): array; + public function info(string $shortCode, VisitsParams $params): array; } diff --git a/module/Core/test/Service/VisitsTrackerTest.php b/module/Core/test/Service/VisitsTrackerTest.php index 11d71968..cc1d938c 100644 --- a/module/Core/test/Service/VisitsTrackerTest.php +++ b/module/Core/test/Service/VisitsTrackerTest.php @@ -8,9 +8,11 @@ use Doctrine\ORM\EntityRepository; use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; +use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Model\Visitor; +use Shlinkio\Shlink\Core\Model\VisitsParams; use Shlinkio\Shlink\Core\Repository\VisitRepository; use Shlinkio\Shlink\Core\Service\VisitsTracker; @@ -80,9 +82,9 @@ class VisitsTrackerTest extends TestCase new Visit(new ShortUrl(''), Visitor::emptyInstance()), ]; $repo2 = $this->prophesize(VisitRepository::class); - $repo2->findVisitsByShortUrl($shortUrl, null)->willReturn($list); + $repo2->findVisitsByShortUrl($shortUrl, Argument::type(DateRange::class))->willReturn($list); $this->em->getRepository(Visit::class)->willReturn($repo2->reveal())->shouldBeCalledOnce(); - $this->assertEquals($list, $this->visitsTracker->info($shortCode)); + $this->assertEquals($list, $this->visitsTracker->info($shortCode, new VisitsParams())); } } diff --git a/module/Rest/src/Action/Visit/GetVisitsAction.php b/module/Rest/src/Action/Visit/GetVisitsAction.php index bddc113d..aa128c8f 100644 --- a/module/Rest/src/Action/Visit/GetVisitsAction.php +++ b/module/Rest/src/Action/Visit/GetVisitsAction.php @@ -4,12 +4,12 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Rest\Action\Visit; use Cake\Chronos\Chronos; -use Exception; use Psr\Http\Message\ResponseInterface as Response; use Psr\Http\Message\ServerRequestInterface as Request; use Psr\Log\LoggerInterface; use Shlinkio\Shlink\Common\Exception\InvalidArgumentException; use Shlinkio\Shlink\Common\Util\DateRange; +use Shlinkio\Shlink\Core\Model\VisitsParams; use Shlinkio\Shlink\Core\Service\VisitsTrackerInterface; use Shlinkio\Shlink\Rest\Action\AbstractRestAction; use Shlinkio\Shlink\Rest\Util\RestUtils; @@ -42,7 +42,7 @@ class GetVisitsAction extends AbstractRestAction $endDate = $this->getDateQueryParam($request, 'endDate'); try { - $visits = $this->visitsTracker->info($shortCode, new DateRange($startDate, $endDate)); + $visits = $this->visitsTracker->info($shortCode, new VisitsParams(new DateRange($startDate, $endDate))); return new JsonResponse([ 'visits' => [ @@ -55,12 +55,6 @@ class GetVisitsAction extends AbstractRestAction 'error' => RestUtils::getRestErrorCodeFromException($e), 'message' => sprintf('Provided short code %s does not exist', $shortCode), ], self::STATUS_NOT_FOUND); - } catch (Exception $e) { - $this->logger->error('Unexpected error while parsing short code {e}', ['e' => $e]); - return new JsonResponse([ - 'error' => RestUtils::UNKNOWN_ERROR, - 'message' => 'Unexpected error occurred', - ], self::STATUS_INTERNAL_SERVER_ERROR); } } diff --git a/module/Rest/test/Action/Visit/GetVisitsActionTest.php b/module/Rest/test/Action/Visit/GetVisitsActionTest.php index f9dd2e54..ed34e284 100644 --- a/module/Rest/test/Action/Visit/GetVisitsActionTest.php +++ b/module/Rest/test/Action/Visit/GetVisitsActionTest.php @@ -4,12 +4,12 @@ declare(strict_types=1); namespace ShlinkioTest\Shlink\Rest\Action\Visit; use Cake\Chronos\Chronos; -use Exception; use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Common\Exception\InvalidArgumentException; use Shlinkio\Shlink\Common\Util\DateRange; +use Shlinkio\Shlink\Core\Model\VisitsParams; use Shlinkio\Shlink\Core\Service\VisitsTracker; use Shlinkio\Shlink\Rest\Action\Visit\GetVisitsAction; use Zend\Diactoros\ServerRequestFactory; @@ -33,7 +33,7 @@ class GetVisitsActionTest extends TestCase public function providingCorrectShortCodeReturnsVisits() { $shortCode = 'abc123'; - $this->visitsTracker->info($shortCode, Argument::type(DateRange::class))->willReturn([]) + $this->visitsTracker->info($shortCode, Argument::type(VisitsParams::class))->willReturn([]) ->shouldBeCalledOnce(); $response = $this->action->handle(ServerRequestFactory::fromGlobals()->withAttribute('shortCode', $shortCode)); @@ -46,7 +46,7 @@ class GetVisitsActionTest extends TestCase public function providingInvalidShortCodeReturnsError() { $shortCode = 'abc123'; - $this->visitsTracker->info($shortCode, Argument::type(DateRange::class))->willThrow( + $this->visitsTracker->info($shortCode, Argument::type(VisitsParams::class))->willThrow( InvalidArgumentException::class )->shouldBeCalledOnce(); @@ -54,27 +54,15 @@ class GetVisitsActionTest extends TestCase $this->assertEquals(404, $response->getStatusCode()); } - /** - * @test - */ - public function unexpectedExceptionWillReturnError() - { - $shortCode = 'abc123'; - $this->visitsTracker->info($shortCode, Argument::type(DateRange::class))->willThrow( - Exception::class - )->shouldBeCalledOnce(); - - $response = $this->action->handle(ServerRequestFactory::fromGlobals()->withAttribute('shortCode', $shortCode)); - $this->assertEquals(500, $response->getStatusCode()); - } - /** * @test */ public function datesAreReadFromQuery() { $shortCode = 'abc123'; - $this->visitsTracker->info($shortCode, new DateRange(null, Chronos::parse('2016-01-01 00:00:00'))) + $this->visitsTracker->info($shortCode, new VisitsParams( + new DateRange(null, Chronos::parse('2016-01-01 00:00:00')) + )) ->willReturn([]) ->shouldBeCalledOnce(); From b0f250ed8a2955b1b306ad2d4175f8bef8b58adf Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 28 Nov 2018 19:58:45 +0100 Subject: [PATCH 2/7] Created factory method to build VisitParams from a raw dataset --- module/Core/src/Model/VisitsParams.php | 14 ++++++++++++++ module/Rest/src/Action/Visit/GetVisitsAction.php | 12 +----------- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/module/Core/src/Model/VisitsParams.php b/module/Core/src/Model/VisitsParams.php index 734dbf57..fe6f1a30 100644 --- a/module/Core/src/Model/VisitsParams.php +++ b/module/Core/src/Model/VisitsParams.php @@ -3,6 +3,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Model; +use Cake\Chronos\Chronos; use Shlinkio\Shlink\Common\Util\DateRange; final class VisitsParams @@ -21,6 +22,19 @@ final class VisitsParams $this->itemsPerPage = $itemsPerPage; } + public static function fromRawData(array $query): self + { + $startDate = self::getDateQueryParam($query, 'startDate'); + $endDate = self::getDateQueryParam($query, 'endDate'); + + return new self(new DateRange($startDate, $endDate), $query['page'] ?? 1, $query['itemsPerPage'] ?? null); + } + + private static function getDateQueryParam(array $query, string $key): ?Chronos + { + return ! isset($query[$key]) || empty($query[$key]) ? null : Chronos::parse($query[$key]); + } + public function getDateRange(): DateRange { return $this->dateRange; diff --git a/module/Rest/src/Action/Visit/GetVisitsAction.php b/module/Rest/src/Action/Visit/GetVisitsAction.php index aa128c8f..bdb62e0b 100644 --- a/module/Rest/src/Action/Visit/GetVisitsAction.php +++ b/module/Rest/src/Action/Visit/GetVisitsAction.php @@ -3,12 +3,10 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Rest\Action\Visit; -use Cake\Chronos\Chronos; use Psr\Http\Message\ResponseInterface as Response; use Psr\Http\Message\ServerRequestInterface as Request; use Psr\Log\LoggerInterface; use Shlinkio\Shlink\Common\Exception\InvalidArgumentException; -use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Model\VisitsParams; use Shlinkio\Shlink\Core\Service\VisitsTrackerInterface; use Shlinkio\Shlink\Rest\Action\AbstractRestAction; @@ -38,11 +36,9 @@ class GetVisitsAction extends AbstractRestAction public function handle(Request $request): Response { $shortCode = $request->getAttribute('shortCode'); - $startDate = $this->getDateQueryParam($request, 'startDate'); - $endDate = $this->getDateQueryParam($request, 'endDate'); try { - $visits = $this->visitsTracker->info($shortCode, new VisitsParams(new DateRange($startDate, $endDate))); + $visits = $this->visitsTracker->info($shortCode, VisitsParams::fromRawData($request->getQueryParams())); return new JsonResponse([ 'visits' => [ @@ -57,10 +53,4 @@ class GetVisitsAction extends AbstractRestAction ], self::STATUS_NOT_FOUND); } } - - private function getDateQueryParam(Request $request, string $key): ?Chronos - { - $query = $request->getQueryParams(); - return ! isset($query[$key]) || empty($query[$key]) ? null : Chronos::parse($query[$key]); - } } From d0e0aea0f12c16649d8e1d0031ae5e74a9e06bc4 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 28 Nov 2018 20:39:08 +0100 Subject: [PATCH 3/7] Updated visits to support pagination --- .../src/Command/ShortUrl/GetVisitsCommand.php | 5 +- .../Command/ShortUrl/GetVisitsCommandTest.php | 28 ++++++----- .../Adapter/VisitsPaginatorAdapter.php | 40 ++++++++++++++++ .../Core/src/Repository/VisitRepository.php | 48 +++++++++++++------ .../Repository/VisitRepositoryInterface.php | 12 +++-- module/Core/src/Service/VisitsTracker.php | 20 ++++---- .../src/Service/VisitsTrackerInterface.php | 5 +- .../Repository/VisitRepositoryTest.php | 35 ++++++++++++-- .../Core/test/Service/VisitsTrackerTest.php | 18 ++++--- .../Rest/src/Action/Visit/GetVisitsAction.php | 7 +-- .../test/Action/Visit/GetVisitsActionTest.php | 9 ++-- 11 files changed, 167 insertions(+), 60 deletions(-) create mode 100644 module/Core/src/Paginator/Adapter/VisitsPaginatorAdapter.php diff --git a/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php b/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php index 55c3a569..48c3a255 100644 --- a/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php +++ b/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php @@ -14,6 +14,7 @@ use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Input\InputOption; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Style\SymfonyStyle; +use Zend\Stdlib\ArrayUtils; use function array_map; use function Functional\select_keys; @@ -73,7 +74,9 @@ class GetVisitsCommand extends Command $startDate = $this->getDateOption($input, 'startDate'); $endDate = $this->getDateOption($input, 'endDate'); - $visits = $this->visitsTracker->info($shortCode, new VisitsParams(new DateRange($startDate, $endDate))); + $paginator = $this->visitsTracker->info($shortCode, new VisitsParams(new DateRange($startDate, $endDate))); + $visits = ArrayUtils::iteratorToArray($paginator->getCurrentItems()); + $rows = array_map(function (Visit $visit) { $rowData = $visit->jsonSerialize(); $rowData['country'] = $visit->getVisitLocation()->getCountryName(); diff --git a/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php b/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php index 33a7c662..87ee003d 100644 --- a/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php @@ -17,7 +17,8 @@ use Shlinkio\Shlink\Core\Model\VisitsParams; use Shlinkio\Shlink\Core\Service\VisitsTrackerInterface; use Symfony\Component\Console\Application; use Symfony\Component\Console\Tester\CommandTester; -use function strpos; +use Zend\Paginator\Adapter\ArrayAdapter; +use Zend\Paginator\Paginator; class GetVisitsCommandTest extends TestCase { @@ -41,8 +42,9 @@ class GetVisitsCommandTest extends TestCase public function noDateFlagsTriesToListWithoutDateRange() { $shortCode = 'abc123'; - $this->visitsTracker->info($shortCode, new VisitsParams(new DateRange(null, null)))->willReturn([]) - ->shouldBeCalledOnce(); + $this->visitsTracker->info($shortCode, new VisitsParams(new DateRange(null, null)))->willReturn( + new Paginator(new ArrayAdapter([])) + )->shouldBeCalledOnce(); $this->commandTester->execute([ 'command' => 'shortcode:visits', @@ -62,7 +64,7 @@ class GetVisitsCommandTest extends TestCase $shortCode, new VisitsParams(new DateRange(Chronos::parse($startDate), Chronos::parse($endDate))) ) - ->willReturn([]) + ->willReturn(new Paginator(new ArrayAdapter([]))) ->shouldBeCalledOnce(); $this->commandTester->execute([ @@ -79,19 +81,21 @@ class GetVisitsCommandTest extends TestCase public function outputIsProperlyGenerated() { $shortCode = 'abc123'; - $this->visitsTracker->info($shortCode, Argument::any())->willReturn([ - (new Visit(new ShortUrl(''), new Visitor('bar', 'foo', '')))->locate( - new VisitLocation(['country_name' => 'Spain']) - ), - ])->shouldBeCalledOnce(); + $this->visitsTracker->info($shortCode, Argument::any())->willReturn( + new Paginator(new ArrayAdapter([ + (new Visit(new ShortUrl(''), new Visitor('bar', 'foo', '')))->locate( + new VisitLocation(['country_name' => 'Spain']) + ), + ])) + )->shouldBeCalledOnce(); $this->commandTester->execute([ 'command' => 'shortcode:visits', 'shortCode' => $shortCode, ]); $output = $this->commandTester->getDisplay(); - $this->assertGreaterThan(0, strpos($output, 'foo')); - $this->assertGreaterThan(0, strpos($output, 'Spain')); - $this->assertGreaterThan(0, strpos($output, 'bar')); + $this->assertContains('foo', $output); + $this->assertContains('Spain', $output); + $this->assertContains('bar', $output); } } diff --git a/module/Core/src/Paginator/Adapter/VisitsPaginatorAdapter.php b/module/Core/src/Paginator/Adapter/VisitsPaginatorAdapter.php new file mode 100644 index 00000000..9066433e --- /dev/null +++ b/module/Core/src/Paginator/Adapter/VisitsPaginatorAdapter.php @@ -0,0 +1,40 @@ +visitRepository = $visitRepository; + $this->shortCode = $shortCode; + $this->params = $params; + } + + public function getItems($offset, $itemCountPerPage): array + { + return $this->visitRepository->findVisitsByShortCode( + $this->shortCode, + $this->params->getDateRange(), + $itemCountPerPage, + $offset + ); + } + + public function count(): int + { + return $this->visitRepository->countVisitsByShortCode($this->shortCode, $this->params->getDateRange()); + } +} diff --git a/module/Core/src/Repository/VisitRepository.php b/module/Core/src/Repository/VisitRepository.php index 1fa67579..f67bb726 100644 --- a/module/Core/src/Repository/VisitRepository.php +++ b/module/Core/src/Repository/VisitRepository.php @@ -4,8 +4,8 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Repository; use Doctrine\ORM\EntityRepository; +use Doctrine\ORM\QueryBuilder; use Shlinkio\Shlink\Common\Util\DateRange; -use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; class VisitRepository extends EntityRepository implements VisitRepositoryInterface @@ -19,24 +19,42 @@ class VisitRepository extends EntityRepository implements VisitRepositoryInterfa } /** - * @param ShortUrl|int $shortUrlOrId - * @param DateRange|null $dateRange * @return Visit[] */ - public function findVisitsByShortUrl($shortUrlOrId, DateRange $dateRange = null): array - { - /** @var ShortUrl|null $shortUrl */ - $shortUrl = $shortUrlOrId instanceof ShortUrl - ? $shortUrlOrId - : $this->getEntityManager()->find(ShortUrl::class, $shortUrlOrId); + public function findVisitsByShortCode( + string $shortCode, + ?DateRange $dateRange = null, + ?int $limit = null, + ?int $offset = null + ): array { + $qb = $this->createVisitsByShortCodeQueryBuilder($shortCode, $dateRange); + $qb->select('v'); - if ($shortUrl === null) { - return []; + if ($limit !== null) { + $qb->setMaxResults($limit); + } + if ($offset !== null) { + $qb->setFirstResult($offset); } - $qb = $this->createQueryBuilder('v'); - $qb->where($qb->expr()->eq('v.shortUrl', ':shortUrl')) - ->setParameter('shortUrl', $shortUrl) + return $qb->getQuery()->getResult(); + } + + public function countVisitsByShortCode(string $shortCode, ?DateRange $dateRange = null): int + { + $qb = $this->createVisitsByShortCodeQueryBuilder($shortCode, $dateRange); + $qb->select('COUNT(DISTINCT v.id)'); + + return (int) $qb->getQuery()->getSingleScalarResult(); + } + + private function createVisitsByShortCodeQueryBuilder(string $shortCode, ?DateRange $dateRange = null): QueryBuilder + { + $qb = $this->getEntityManager()->createQueryBuilder(); + $qb->from(Visit::class, 'v') + ->join('v.shortUrl', 'su') + ->where($qb->expr()->eq('su.shortCode', ':shortCode')) + ->setParameter('shortCode', $shortCode) ->orderBy('v.date', 'DESC') ; // Apply date range filtering @@ -49,6 +67,6 @@ class VisitRepository extends EntityRepository implements VisitRepositoryInterfa ->setParameter('endDate', $dateRange->getEndDate()); } - return $qb->getQuery()->getResult(); + return $qb; } } diff --git a/module/Core/src/Repository/VisitRepositoryInterface.php b/module/Core/src/Repository/VisitRepositoryInterface.php index 72df2ed1..4de9024e 100644 --- a/module/Core/src/Repository/VisitRepositoryInterface.php +++ b/module/Core/src/Repository/VisitRepositoryInterface.php @@ -5,7 +5,6 @@ namespace Shlinkio\Shlink\Core\Repository; use Doctrine\Common\Persistence\ObjectRepository; use Shlinkio\Shlink\Common\Util\DateRange; -use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; interface VisitRepositoryInterface extends ObjectRepository @@ -13,9 +12,14 @@ interface VisitRepositoryInterface extends ObjectRepository public function findUnlocatedVisits(): iterable; /** - * @param ShortUrl|int $shortUrl - * @param DateRange|null $dateRange * @return Visit[] */ - public function findVisitsByShortUrl($shortUrl, DateRange $dateRange = null): array; + public function findVisitsByShortCode( + string $shortCode, + ?DateRange $dateRange = null, + ?int $limit = null, + ?int $offset = null + ): array; + + public function countVisitsByShortCode(string $shortCode, ?DateRange $dateRange = null): int; } diff --git a/module/Core/src/Service/VisitsTracker.php b/module/Core/src/Service/VisitsTracker.php index d05735ed..27ccf0ea 100644 --- a/module/Core/src/Service/VisitsTracker.php +++ b/module/Core/src/Service/VisitsTracker.php @@ -9,7 +9,9 @@ use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Exception\InvalidArgumentException; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Model\VisitsParams; +use Shlinkio\Shlink\Core\Paginator\Adapter\VisitsPaginatorAdapter; use Shlinkio\Shlink\Core\Repository\VisitRepository; +use Zend\Paginator\Paginator; use function sprintf; class VisitsTracker implements VisitsTrackerInterface @@ -43,21 +45,23 @@ class VisitsTracker implements VisitsTrackerInterface /** * Returns the visits on certain short code * - * @return Visit[] + * @return Visit[]|Paginator * @throws InvalidArgumentException */ - public function info(string $shortCode, VisitsParams $params): array + public function info(string $shortCode, VisitsParams $params): Paginator { - /** @var ShortUrl|null $shortUrl */ - $shortUrl = $this->em->getRepository(ShortUrl::class)->findOneBy([ - 'shortCode' => $shortCode, - ]); - if ($shortUrl === null) { + /** @var ORM\EntityRepository $repo */ + $repo = $this->em->getRepository(ShortUrl::class); + if ($repo->count(['shortCode' => $shortCode]) < 1) { throw new InvalidArgumentException(sprintf('Short code "%s" not found', $shortCode)); } /** @var VisitRepository $repo */ $repo = $this->em->getRepository(Visit::class); - return $repo->findVisitsByShortUrl($shortUrl, $params->getDateRange()); + $paginator = new Paginator(new VisitsPaginatorAdapter($repo, $shortCode, $params)); + $paginator->setItemCountPerPage($params->hasItemsPerPage() ? $params->getItemsPerPage() : -1) + ->setCurrentPageNumber($params->getPage()); + + return $paginator; } } diff --git a/module/Core/src/Service/VisitsTrackerInterface.php b/module/Core/src/Service/VisitsTrackerInterface.php index e895dae3..5623860f 100644 --- a/module/Core/src/Service/VisitsTrackerInterface.php +++ b/module/Core/src/Service/VisitsTrackerInterface.php @@ -7,6 +7,7 @@ use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Exception\InvalidArgumentException; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Model\VisitsParams; +use Zend\Paginator\Paginator; interface VisitsTrackerInterface { @@ -18,8 +19,8 @@ interface VisitsTrackerInterface /** * Returns the visits on certain short code * - * @return Visit[] + * @return Visit[]|Paginator * @throws InvalidArgumentException */ - public function info(string $shortCode, VisitsParams $params): array; + public function info(string $shortCode, VisitsParams $params): Paginator; } diff --git a/module/Core/test-func/Repository/VisitRepositoryTest.php b/module/Core/test-func/Repository/VisitRepositoryTest.php index 8c6ef11a..452e543e 100644 --- a/module/Core/test-func/Repository/VisitRepositoryTest.php +++ b/module/Core/test-func/Repository/VisitRepositoryTest.php @@ -62,7 +62,7 @@ class VisitRepositoryTest extends DatabaseTestCase /** * @test */ - public function findVisitsByShortUrlReturnsProperData() + public function findVisitsByShortCodeReturnsProperData() { $shortUrl = new ShortUrl(''); $this->getEntityManager()->persist($shortUrl); @@ -73,13 +73,38 @@ class VisitRepositoryTest extends DatabaseTestCase } $this->getEntityManager()->flush(); - $this->assertCount(0, $this->repo->findVisitsByShortUrl('invalid')); - $this->assertCount(6, $this->repo->findVisitsByShortUrl($shortUrl->getId())); - $this->assertCount(2, $this->repo->findVisitsByShortUrl($shortUrl->getId(), new DateRange( + $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( Chronos::parse('2016-01-02'), Chronos::parse('2016-01-03') ))); - $this->assertCount(4, $this->repo->findVisitsByShortUrl($shortUrl->getId(), new DateRange( + $this->assertCount(4, $this->repo->findVisitsByShortCode($shortUrl->getShortCode(), new DateRange( + Chronos::parse('2016-01-03') + ))); + } + + /** + * @test + */ + public function countVisitsByShortCodeReturnsProperData() + { + $shortUrl = new ShortUrl(''); + $this->getEntityManager()->persist($shortUrl); + + for ($i = 0; $i < 6; $i++) { + $visit = new Visit($shortUrl, Visitor::emptyInstance(), Chronos::parse(sprintf('2016-01-0%s', $i + 1))); + $this->getEntityManager()->persist($visit); + } + $this->getEntityManager()->flush(); + + $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( + Chronos::parse('2016-01-02'), + Chronos::parse('2016-01-03') + ))); + $this->assertEquals(4, $this->repo->countVisitsByShortCode($shortUrl->getShortCode(), new DateRange( Chronos::parse('2016-01-03') ))); } diff --git a/module/Core/test/Service/VisitsTrackerTest.php b/module/Core/test/Service/VisitsTrackerTest.php index cc1d938c..3df44880 100644 --- a/module/Core/test/Service/VisitsTrackerTest.php +++ b/module/Core/test/Service/VisitsTrackerTest.php @@ -5,6 +5,7 @@ namespace ShlinkioTest\Shlink\Core\Service; use Doctrine\ORM\EntityManager; use Doctrine\ORM\EntityRepository; +use PHPUnit\Framework\Assert; use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; @@ -15,6 +16,7 @@ use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Model\VisitsParams; use Shlinkio\Shlink\Core\Repository\VisitRepository; use Shlinkio\Shlink\Core\Service\VisitsTracker; +use Zend\Stdlib\ArrayUtils; class VisitsTrackerTest extends TestCase { @@ -51,15 +53,14 @@ class VisitsTrackerTest extends TestCase public function trackedIpAddressGetsObfuscated() { $shortCode = '123ABC'; - $test = $this; $repo = $this->prophesize(EntityRepository::class); $repo->findOneBy(['shortCode' => $shortCode])->willReturn(new ShortUrl('')); $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal())->shouldBeCalledOnce(); - $this->em->persist(Argument::any())->will(function ($args) use ($test) { + $this->em->persist(Argument::any())->will(function ($args) { /** @var Visit $visit */ $visit = $args[0]; - $test->assertEquals('4.3.2.0', $visit->getRemoteAddr()); + Assert::assertEquals('4.3.2.0', $visit->getRemoteAddr()); })->shouldBeCalledOnce(); $this->em->flush(Argument::type(Visit::class))->shouldBeCalledOnce(); @@ -72,9 +73,8 @@ class VisitsTrackerTest extends TestCase public function infoReturnsVisistForCertainShortCode() { $shortCode = '123ABC'; - $shortUrl = new ShortUrl('http://domain.com/foo/bar'); $repo = $this->prophesize(EntityRepository::class); - $repo->findOneBy(['shortCode' => $shortCode])->willReturn($shortUrl); + $count = $repo->count(['shortCode' => $shortCode])->willReturn(1); $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal())->shouldBeCalledOnce(); $list = [ @@ -82,9 +82,13 @@ class VisitsTrackerTest extends TestCase new Visit(new ShortUrl(''), Visitor::emptyInstance()), ]; $repo2 = $this->prophesize(VisitRepository::class); - $repo2->findVisitsByShortUrl($shortUrl, Argument::type(DateRange::class))->willReturn($list); + $repo2->findVisitsByShortCode($shortCode, Argument::type(DateRange::class), 1, 0)->willReturn($list); + $repo2->countVisitsByShortCode($shortCode, Argument::type(DateRange::class))->willReturn(1); $this->em->getRepository(Visit::class)->willReturn($repo2->reveal())->shouldBeCalledOnce(); - $this->assertEquals($list, $this->visitsTracker->info($shortCode, new VisitsParams())); + $paginator = $this->visitsTracker->info($shortCode, new VisitsParams()); + + $this->assertEquals($list, ArrayUtils::iteratorToArray($paginator->getCurrentItems())); + $count->shouldHaveBeenCalledOnce(); } } diff --git a/module/Rest/src/Action/Visit/GetVisitsAction.php b/module/Rest/src/Action/Visit/GetVisitsAction.php index bdb62e0b..38093080 100644 --- a/module/Rest/src/Action/Visit/GetVisitsAction.php +++ b/module/Rest/src/Action/Visit/GetVisitsAction.php @@ -7,6 +7,7 @@ use Psr\Http\Message\ResponseInterface as Response; use Psr\Http\Message\ServerRequestInterface as Request; use Psr\Log\LoggerInterface; use Shlinkio\Shlink\Common\Exception\InvalidArgumentException; +use Shlinkio\Shlink\Common\Paginator\Util\PaginatorUtilsTrait; use Shlinkio\Shlink\Core\Model\VisitsParams; use Shlinkio\Shlink\Core\Service\VisitsTrackerInterface; use Shlinkio\Shlink\Rest\Action\AbstractRestAction; @@ -16,6 +17,8 @@ use function sprintf; class GetVisitsAction extends AbstractRestAction { + use PaginatorUtilsTrait; + protected const ROUTE_PATH = '/short-urls/{shortCode}/visits'; protected const ROUTE_ALLOWED_METHODS = [self::METHOD_GET]; @@ -41,9 +44,7 @@ class GetVisitsAction extends AbstractRestAction $visits = $this->visitsTracker->info($shortCode, VisitsParams::fromRawData($request->getQueryParams())); return new JsonResponse([ - 'visits' => [ - 'data' => $visits, - ], + 'visits' => $this->serializePaginator($visits), ]); } catch (InvalidArgumentException $e) { $this->logger->warning('Provided nonexistent short code {e}', ['e' => $e]); diff --git a/module/Rest/test/Action/Visit/GetVisitsActionTest.php b/module/Rest/test/Action/Visit/GetVisitsActionTest.php index ed34e284..c3d9f81a 100644 --- a/module/Rest/test/Action/Visit/GetVisitsActionTest.php +++ b/module/Rest/test/Action/Visit/GetVisitsActionTest.php @@ -13,6 +13,8 @@ use Shlinkio\Shlink\Core\Model\VisitsParams; use Shlinkio\Shlink\Core\Service\VisitsTracker; use Shlinkio\Shlink\Rest\Action\Visit\GetVisitsAction; use Zend\Diactoros\ServerRequestFactory; +use Zend\Paginator\Adapter\ArrayAdapter; +use Zend\Paginator\Paginator; class GetVisitsActionTest extends TestCase { @@ -33,8 +35,9 @@ class GetVisitsActionTest extends TestCase public function providingCorrectShortCodeReturnsVisits() { $shortCode = 'abc123'; - $this->visitsTracker->info($shortCode, Argument::type(VisitsParams::class))->willReturn([]) - ->shouldBeCalledOnce(); + $this->visitsTracker->info($shortCode, Argument::type(VisitsParams::class))->willReturn( + new Paginator(new ArrayAdapter([])) + )->shouldBeCalledOnce(); $response = $this->action->handle(ServerRequestFactory::fromGlobals()->withAttribute('shortCode', $shortCode)); $this->assertEquals(200, $response->getStatusCode()); @@ -63,7 +66,7 @@ class GetVisitsActionTest extends TestCase $this->visitsTracker->info($shortCode, new VisitsParams( new DateRange(null, Chronos::parse('2016-01-01 00:00:00')) )) - ->willReturn([]) + ->willReturn(new Paginator(new ArrayAdapter([]))) ->shouldBeCalledOnce(); $response = $this->action->handle( From 6947805b5c0ca2374f2ac8b0a89bc68c0db01b76 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 28 Nov 2018 20:43:44 +0100 Subject: [PATCH 4/7] Updated to zend-expressive-swoole 2.0.1 removing all workarounds --- composer.json | 2 +- config/pipeline.php | 6 ------ config/routes.php | 6 ------ data/infra/swoole.Dockerfile | 5 +---- 4 files changed, 2 insertions(+), 17 deletions(-) delete mode 100644 config/pipeline.php delete mode 100644 config/routes.php diff --git a/composer.json b/composer.json index 898ce0ed..41f880f7 100644 --- a/composer.json +++ b/composer.json @@ -42,7 +42,7 @@ "zendframework/zend-expressive-fastroute": "^3.0", "zendframework/zend-expressive-helpers": "^5.0", "zendframework/zend-expressive-platesrenderer": "^2.0", - "zendframework/zend-expressive-swoole": "^2.0", + "zendframework/zend-expressive-swoole": "^2.0.1", "zendframework/zend-i18n": "^2.7", "zendframework/zend-inputfilter": "^2.8", "zendframework/zend-paginator": "^2.6", diff --git a/config/pipeline.php b/config/pipeline.php deleted file mode 100644 index 82d3aeea..00000000 --- a/config/pipeline.php +++ /dev/null @@ -1,6 +0,0 @@ - Date: Wed, 28 Nov 2018 20:46:52 +0100 Subject: [PATCH 5/7] Updated swagger docs for visits including everything related to pagination --- .../v1_short-urls_{shortCode}_visits.json | 30 ++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/docs/swagger/paths/v1_short-urls_{shortCode}_visits.json b/docs/swagger/paths/v1_short-urls_{shortCode}_visits.json index acae7155..a3cf5f10 100644 --- a/docs/swagger/paths/v1_short-urls_{shortCode}_visits.json +++ b/docs/swagger/paths/v1_short-urls_{shortCode}_visits.json @@ -33,6 +33,24 @@ "schema": { "type": "string" } + }, + { + "name": "page", + "in": "query", + "description": "The page to display. Defaults to 1", + "required": false, + "schema": { + "type": "number" + } + }, + { + "name": "itemsPerPage", + "in": "query", + "description": "The amount of items to return on every page. Defaults to all the items", + "required": false, + "schema": { + "type": "number" + } } ], "security": [ @@ -59,6 +77,9 @@ "items": { "$ref": "../definitions/Visit.json" } + }, + "pagination": { + "$ref": "../definitions/Pagination.json" } } } @@ -96,7 +117,14 @@ "userAgent": "some_web_crawler/1.4", "visitLocation": null } - ] + ], + "pagination": { + "currentPage": 5, + "pagesCount": 12, + "itemsPerPage": 10, + "itemsInCurrentPage": 10, + "totalItems": 115 + } } } } From 1d4ef4e9a43401b64da444787a423594594ab9e5 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 28 Nov 2018 20:53:04 +0100 Subject: [PATCH 6/7] Ensured pagination params in visits list are properly parsed to integer --- data/infra/swoole.Dockerfile | 5 ++++- module/Core/src/Model/VisitsParams.php | 6 +++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/data/infra/swoole.Dockerfile b/data/infra/swoole.Dockerfile index 07082220..40293eaa 100644 --- a/data/infra/swoole.Dockerfile +++ b/data/infra/swoole.Dockerfile @@ -92,4 +92,7 @@ WORKDIR /home/shlink # Expose swoole port EXPOSE 8080 -CMD /usr/local/bin/composer update && ./vendor/bin/zend-expressive-swoole start +CMD /usr/local/bin/composer update && \ + # When restarting the container, swoole might think it is already in execution + # This forces the app to be started every second until the exit code is 0 + until php ./vendor/bin/zend-expressive-swoole start; do sleep 1 ; done diff --git a/module/Core/src/Model/VisitsParams.php b/module/Core/src/Model/VisitsParams.php index fe6f1a30..e583fb73 100644 --- a/module/Core/src/Model/VisitsParams.php +++ b/module/Core/src/Model/VisitsParams.php @@ -27,7 +27,11 @@ final class VisitsParams $startDate = self::getDateQueryParam($query, 'startDate'); $endDate = self::getDateQueryParam($query, 'endDate'); - return new self(new DateRange($startDate, $endDate), $query['page'] ?? 1, $query['itemsPerPage'] ?? null); + return new self( + new DateRange($startDate, $endDate), + (int) ($query['page'] ?? 1), + isset($query['itemsPerPage']) ? (int) $query['itemsPerPage'] : null + ); } private static function getDateQueryParam(array $query, string $key): ?Chronos From d6c158ce98127ea69edf9547e9c5420d77007853 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 28 Nov 2018 20:55:07 +0100 Subject: [PATCH 7/7] Updated changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e4d50f30..8bf2ce8e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), Adding the `-d` flag, it will be started as a background service. Then you can use the `./vendor/bin/zend-expressive-swoole stop` command in order to stop it. +* [#266](https://github.com/shlinkio/shlink/issues/266) Added pagination to `GET /short-urls/{shortCode}/visits` endpoint. + + In order to make it backwards compatible, it keeps returning all visits by default, but it now allows to provide the `page` and `itemsPerPage` query parameters in order to configure the number of items to get. + #### Changed * [#267](https://github.com/shlinkio/shlink/issues/267) API responses and the CLI interface is no longer translated and uses english always. Only not found error templates are still translated.