From 4a3e04ced9b9d6a99318ac3f28694b3ce26e82d2 Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandro@alejandrocelaya.com>
Date: Sun, 16 Jan 2022 11:44:12 +0100
Subject: [PATCH] Added tests covering count non orphan visits with different
 combinations of filters

---
 .../Core/src/Repository/VisitRepository.php   |  5 +--
 .../src/Visit/Spec/CountOfNonOrphanVisits.php | 39 +++++++++++++++++++
 .../src/Visit/Spec/CountOfShortUrlVisits.php  | 27 -------------
 .../Repository/VisitRepositoryTest.php        | 10 +++++
 4 files changed, 51 insertions(+), 30 deletions(-)
 create mode 100644 module/Core/src/Visit/Spec/CountOfNonOrphanVisits.php
 delete mode 100644 module/Core/src/Visit/Spec/CountOfShortUrlVisits.php

diff --git a/module/Core/src/Repository/VisitRepository.php b/module/Core/src/Repository/VisitRepository.php
index 6c48a318..67b4256f 100644
--- a/module/Core/src/Repository/VisitRepository.php
+++ b/module/Core/src/Repository/VisitRepository.php
@@ -14,8 +14,8 @@ use Shlinkio\Shlink\Core\Entity\VisitLocation;
 use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier;
 use Shlinkio\Shlink\Core\Visit\Persistence\VisitsCountFiltering;
 use Shlinkio\Shlink\Core\Visit\Persistence\VisitsListFiltering;
+use Shlinkio\Shlink\Core\Visit\Spec\CountOfNonOrphanVisits;
 use Shlinkio\Shlink\Core\Visit\Spec\CountOfOrphanVisits;
-use Shlinkio\Shlink\Core\Visit\Spec\CountOfShortUrlVisits;
 
 use const PHP_INT_MAX;
 
@@ -179,10 +179,9 @@ class VisitRepository extends EntitySpecificationRepository implements VisitRepo
         return $this->resolveVisitsWithNativeQuery($qb, $filtering->limit(), $filtering->offset());
     }
 
-    // TODO This should support counting in a date range or excluding bots
     public function countNonOrphanVisits(VisitsCountFiltering $filtering): int
     {
-        return (int) $this->matchSingleScalarResult(new CountOfShortUrlVisits($filtering->apiKey()));
+        return (int) $this->matchSingleScalarResult(new CountOfNonOrphanVisits($filtering));
     }
 
     private function createAllVisitsQueryBuilder(VisitsListFiltering $filtering): QueryBuilder
diff --git a/module/Core/src/Visit/Spec/CountOfNonOrphanVisits.php b/module/Core/src/Visit/Spec/CountOfNonOrphanVisits.php
new file mode 100644
index 00000000..52be52a8
--- /dev/null
+++ b/module/Core/src/Visit/Spec/CountOfNonOrphanVisits.php
@@ -0,0 +1,39 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core\Visit\Spec;
+
+use Happyr\DoctrineSpecification\Spec;
+use Happyr\DoctrineSpecification\Specification\BaseSpecification;
+use Happyr\DoctrineSpecification\Specification\Specification;
+use Shlinkio\Shlink\Core\Spec\InDateRange;
+use Shlinkio\Shlink\Core\Visit\Persistence\VisitsCountFiltering;
+use Shlinkio\Shlink\Rest\ApiKey\Spec\WithApiKeySpecsEnsuringJoin;
+
+class CountOfNonOrphanVisits extends BaseSpecification
+{
+    public function __construct(private VisitsCountFiltering $filtering)
+    {
+        parent::__construct();
+    }
+
+    protected function getSpec(): Specification
+    {
+        $conditions = [
+            Spec::isNotNull('shortUrl'),
+            new InDateRange($this->filtering->dateRange()),
+        ];
+
+        if ($this->filtering->excludeBots()) {
+            $conditions[] = Spec::eq('potentialBot', false);
+        }
+
+        $apiKey = $this->filtering->apiKey();
+        if ($apiKey !== null) {
+            $conditions[] = new WithApiKeySpecsEnsuringJoin($apiKey, 'shortUrl');
+        }
+
+        return Spec::countOf(Spec::andX(...$conditions));
+    }
+}
diff --git a/module/Core/src/Visit/Spec/CountOfShortUrlVisits.php b/module/Core/src/Visit/Spec/CountOfShortUrlVisits.php
deleted file mode 100644
index 49d8db93..00000000
--- a/module/Core/src/Visit/Spec/CountOfShortUrlVisits.php
+++ /dev/null
@@ -1,27 +0,0 @@
-<?php
-
-declare(strict_types=1);
-
-namespace Shlinkio\Shlink\Core\Visit\Spec;
-
-use Happyr\DoctrineSpecification\Spec;
-use Happyr\DoctrineSpecification\Specification\BaseSpecification;
-use Happyr\DoctrineSpecification\Specification\Specification;
-use Shlinkio\Shlink\Rest\ApiKey\Spec\WithApiKeySpecsEnsuringJoin;
-use Shlinkio\Shlink\Rest\Entity\ApiKey;
-
-class CountOfShortUrlVisits extends BaseSpecification
-{
-    public function __construct(private ?ApiKey $apiKey)
-    {
-        parent::__construct();
-    }
-
-    protected function getSpec(): Specification
-    {
-        return Spec::countOf(Spec::andX(
-            Spec::isNotNull('shortUrl'),
-            new WithApiKeySpecsEnsuringJoin($this->apiKey, 'shortUrl'),
-        ));
-    }
-}
diff --git a/module/Core/test-db/Repository/VisitRepositoryTest.php b/module/Core/test-db/Repository/VisitRepositoryTest.php
index a7981ea1..950bfc8a 100644
--- a/module/Core/test-db/Repository/VisitRepositoryTest.php
+++ b/module/Core/test-db/Repository/VisitRepositoryTest.php
@@ -301,6 +301,16 @@ class VisitRepositoryTest extends DatabaseTestCase
         self::assertEquals(4, $this->repo->countNonOrphanVisits(VisitsCountFiltering::withApiKey($apiKey1)));
         self::assertEquals(5 + 7, $this->repo->countNonOrphanVisits(VisitsCountFiltering::withApiKey($apiKey2)));
         self::assertEquals(4 + 7, $this->repo->countNonOrphanVisits(VisitsCountFiltering::withApiKey($domainApiKey)));
+        self::assertEquals(4, $this->repo->countNonOrphanVisits(new VisitsCountFiltering(DateRange::withStartDate(
+            Chronos::parse('2016-01-05')->startOfDay(),
+        ))));
+        self::assertEquals(2, $this->repo->countNonOrphanVisits(new VisitsCountFiltering(DateRange::withStartDate(
+            Chronos::parse('2016-01-03')->startOfDay(),
+        ), false, $apiKey1)));
+        self::assertEquals(1, $this->repo->countNonOrphanVisits(new VisitsCountFiltering(DateRange::withStartDate(
+            Chronos::parse('2016-01-07')->startOfDay(),
+        ), false, $apiKey2)));
+        self::assertEquals(3 + 5, $this->repo->countNonOrphanVisits(new VisitsCountFiltering(null, true, $apiKey2)));
         self::assertEquals(4, $this->repo->countOrphanVisits(new VisitsCountFiltering()));
         self::assertEquals(3, $this->repo->countOrphanVisits(new VisitsCountFiltering(null, true)));
     }