Merge pull request #2268 from acelaya-forks/feature/delete-visits-fix

Fix visits counts not being deleted when deleting short URL or orphan visits

CHANGELOG.md

@@ -38,7 +38,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this
 * *Nothing*
 
 ### Fixed
-* *Nothing*
+* [#2264](https://github.com/shlinkio/shlink/issues/2264) Fix visits counts not being deleted when deleting short URL or orphan visits.
 
 
 ## [4.2.5] - 2024-11-03

module/Core/src/Visit/Repository/VisitDeleterRepository.php

@@ -6,15 +6,28 @@ namespace Shlinkio\Shlink\Core\Visit\Repository;
 
 use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepository;
 use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl;
+use Shlinkio\Shlink\Core\Visit\Entity\OrphanVisitsCount;
+use Shlinkio\Shlink\Core\Visit\Entity\ShortUrlVisitsCount;
 use Shlinkio\Shlink\Core\Visit\Entity\Visit;
 
 /** @extends EntitySpecificationRepository<Visit> */
 class VisitDeleterRepository extends EntitySpecificationRepository implements VisitDeleterRepositoryInterface
 {
     public function deleteShortUrlVisits(ShortUrl $shortUrl): int
+    {
+        return $this->getEntityManager()->wrapInTransaction(function () use ($shortUrl): int {
+            $this->deleteByShortUrl(ShortUrlVisitsCount::class, $shortUrl);
+            return $this->deleteByShortUrl(Visit::class, $shortUrl);
+        });
+    }
+
+    /**
+     * @param class-string<Visit | ShortUrlVisitsCount> $entityName
+     */
+    private function deleteByShortUrl(string $entityName, ShortUrl $shortUrl): int
     {
         $qb = $this->getEntityManager()->createQueryBuilder();
-        $qb->delete(Visit::class, 'v')
+        $qb->delete($entityName, 'v')
            ->where($qb->expr()->eq('v.shortUrl', ':shortUrl'))
            ->setParameter('shortUrl', $shortUrl);
 
@@ -23,10 +36,15 @@ class VisitDeleterRepository extends EntitySpecificationRepository implements Vi
 
     public function deleteOrphanVisits(): int
     {
-        $qb = $this->getEntityManager()->createQueryBuilder();
+        $em = $this->getEntityManager();
+        return $em->wrapInTransaction(function () use ($em): int {
+            $em->createQueryBuilder()->delete(OrphanVisitsCount::class, 'v')->getQuery()->execute();
+
+            $qb = $em->createQueryBuilder();
             $qb->delete(Visit::class, 'v')
                ->where($qb->expr()->isNull('v.shortUrl'));
 
             return $qb->getQuery()->execute();
+        });
     }
 }

module/Core/test-db/Visit/Repository/VisitDeleterRepositoryTest.php

@@ -9,18 +9,26 @@ use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl;
 use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlCreation;
 use Shlinkio\Shlink\Core\ShortUrl\Model\Validation\ShortUrlInputFilter;
 use Shlinkio\Shlink\Core\ShortUrl\Resolver\PersistenceShortUrlRelationResolver;
+use Shlinkio\Shlink\Core\Visit\Entity\OrphanVisitsCount;
+use Shlinkio\Shlink\Core\Visit\Entity\ShortUrlVisitsCount;
 use Shlinkio\Shlink\Core\Visit\Entity\Visit;
 use Shlinkio\Shlink\Core\Visit\Model\Visitor;
+use Shlinkio\Shlink\Core\Visit\Repository\OrphanVisitsCountRepository;
+use Shlinkio\Shlink\Core\Visit\Repository\ShortUrlVisitsCountRepository;
 use Shlinkio\Shlink\Core\Visit\Repository\VisitDeleterRepository;
 use Shlinkio\Shlink\TestUtils\DbTest\DatabaseTestCase;
 
 class VisitDeleterRepositoryTest extends DatabaseTestCase
 {
     private VisitDeleterRepository $repo;
+    private ShortUrlVisitsCountRepository $visitsCountRepo;
+    private OrphanVisitsCountRepository $orphanVisitsCountRepo;
 
     protected function setUp(): void
     {
         $this->repo = $this->createRepository(Visit::class, VisitDeleterRepository::class);
+        $this->visitsCountRepo = $this->getEntityManager()->getRepository(ShortUrlVisitsCount::class);
+        $this->orphanVisitsCountRepo = $this->getEntityManager()->getRepository(OrphanVisitsCount::class);
     }
 
     #[Test]
@@ -51,12 +59,20 @@ class VisitDeleterRepositoryTest extends DatabaseTestCase
 
         $this->getEntityManager()->flush();
 
-        self::assertEquals(2, $this->repo->deleteShortUrlVisits($shortUrl1));
-        self::assertEquals(0, $this->repo->deleteShortUrlVisits($shortUrl1));
-        self::assertEquals(4, $this->repo->deleteShortUrlVisits($shortUrl2));
-        self::assertEquals(0, $this->repo->deleteShortUrlVisits($shortUrl2));
-        self::assertEquals(1, $this->repo->deleteShortUrlVisits($shortUrl3));
-        self::assertEquals(0, $this->repo->deleteShortUrlVisits($shortUrl3));
+        $this->assertVisitsDeletionForShortUrl($shortUrl1, 2);
+        $this->assertVisitsDeletionForShortUrl($shortUrl2, 4);
+        $this->assertVisitsDeletionForShortUrl($shortUrl3, 1);
+    }
+
+    private function assertVisitsDeletionForShortUrl(ShortUrl $shortUrl, int $expectedDeleteCount): void
+    {
+        // There should be at least one visit count before deletion
+        self::assertGreaterThan(0, $this->visitsCountRepo->count(['shortUrl' => $shortUrl]));
+        self::assertEquals($expectedDeleteCount, $this->repo->deleteShortUrlVisits($shortUrl));
+
+        // Visits counts are also deleted, and trying to delete again results in no visits deleted
+        self::assertEquals(0, $this->visitsCountRepo->count(['shortUrl' => $shortUrl]));
+        self::assertEquals(0, $this->repo->deleteShortUrlVisits($shortUrl));
     }
 
     #[Test]
@@ -72,7 +88,9 @@ class VisitDeleterRepositoryTest extends DatabaseTestCase
 
         $this->getEntityManager()->flush();
 
+        self::assertGreaterThan(0, $this->orphanVisitsCountRepo->count());
         self::assertEquals(6, $this->repo->deleteOrphanVisits());
         self::assertEquals(0, $this->repo->deleteOrphanVisits());
+        self::assertEquals(0, $this->orphanVisitsCountRepo->count());
     }
 }