From 7b4456e73f6e04d930408646ecacbf0286e90a91 Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandro@alejandrocelaya.com>
Date: Thu, 8 Apr 2021 14:09:26 +0200
Subject: [PATCH] Ensured events triggered as a result of a new visit are never
 skipped

---
 .gitignore                                           |  3 +--
 .../test/Command/ShortUrl/GetVisitsCommandTest.php   |  1 +
 module/Core/src/EventDispatcher/LocateVisit.php      | 12 ++++++------
 module/Core/src/EventDispatcher/UpdateGeoLiteDb.php  |  6 ++++--
 module/Core/test/EventDispatcher/LocateVisitTest.php |  2 +-
 .../test/EventDispatcher/UpdateGeoLiteDbTest.php     |  4 ++++
 6 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/.gitignore b/.gitignore
index 03b2790e..32942a29 100644
--- a/.gitignore
+++ b/.gitignore
@@ -6,8 +6,7 @@ composer.phar
 vendor/
 data/database.sqlite
 data/shlink-tests.db
-data/GeoLite2-City.mmdb
-data/GeoLite2-City.mmdb.*
+data/GeoLite2-City.*
 docs/swagger-ui*
 docs/mercure.html
 docker-compose.override.yml
diff --git a/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php b/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php
index 42f008be..044866ed 100644
--- a/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php
+++ b/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php
@@ -22,6 +22,7 @@ use Shlinkio\Shlink\Core\Visit\VisitsStatsHelperInterface;
 use Shlinkio\Shlink\IpGeolocation\Model\Location;
 use ShlinkioTest\Shlink\CLI\CliTestUtilsTrait;
 use Symfony\Component\Console\Tester\CommandTester;
+
 use function sprintf;
 
 class GetVisitsCommandTest extends TestCase
diff --git a/module/Core/src/EventDispatcher/LocateVisit.php b/module/Core/src/EventDispatcher/LocateVisit.php
index 4d98f454..80dc18eb 100644
--- a/module/Core/src/EventDispatcher/LocateVisit.php
+++ b/module/Core/src/EventDispatcher/LocateVisit.php
@@ -52,6 +52,12 @@ class LocateVisit
             return;
         }
 
+        $this->locateVisit($visitId, $shortUrlVisited->originalIpAddress(), $visit);
+        $this->eventDispatcher->dispatch(new VisitLocated($visitId));
+    }
+
+    private function locateVisit(string $visitId, ?string $originalIpAddress, Visit $visit): void
+    {
         if (! $this->dbUpdater->databaseFileExists()) {
             $this->logger->warning('Tried to locate visit with id "{visitId}", but a GeoLite2 db was not found.', [
                 'visitId' => $visitId,
@@ -59,12 +65,6 @@ class LocateVisit
             return;
         }
 
-        $this->locateVisit($visitId, $shortUrlVisited->originalIpAddress(), $visit);
-        $this->eventDispatcher->dispatch(new VisitLocated($visitId));
-    }
-
-    private function locateVisit(string $visitId, ?string $originalIpAddress, Visit $visit): void
-    {
         $isLocatable = $originalIpAddress !== null || $visit->isLocatable();
         $addr = $originalIpAddress ?? $visit->getRemoteAddr();
 
diff --git a/module/Core/src/EventDispatcher/UpdateGeoLiteDb.php b/module/Core/src/EventDispatcher/UpdateGeoLiteDb.php
index 7b985026..f17a7ffb 100644
--- a/module/Core/src/EventDispatcher/UpdateGeoLiteDb.php
+++ b/module/Core/src/EventDispatcher/UpdateGeoLiteDb.php
@@ -26,11 +26,13 @@ class UpdateGeoLiteDb
         $beforeDownload = fn (bool $olderDbExists) => $this->logger->notice(
             sprintf('%s GeoLite2 db file...', $olderDbExists ? 'Updating' : 'Downloading'),
         );
-        $handleProgress = function (int $total, int $downloaded, bool $olderDbExists): void {
-            if ($total > $downloaded) {
+        $messageLogged = false;
+        $handleProgress = function (int $total, int $downloaded, bool $olderDbExists) use (&$messageLogged): void {
+            if ($messageLogged || $total > $downloaded) {
                 return;
             }
 
+            $messageLogged = true;
             $this->logger->notice(sprintf('Finished %s GeoLite2 db file', $olderDbExists ? 'updating' : 'downloading'));
         };
 
diff --git a/module/Core/test/EventDispatcher/LocateVisitTest.php b/module/Core/test/EventDispatcher/LocateVisitTest.php
index 68c7496f..fda45c58 100644
--- a/module/Core/test/EventDispatcher/LocateVisitTest.php
+++ b/module/Core/test/EventDispatcher/LocateVisitTest.php
@@ -97,7 +97,7 @@ class LocateVisitTest extends TestCase
         $this->em->flush()->shouldNotHaveBeenCalled();
         $this->ipLocationResolver->resolveIpLocation(Argument::cetera())->shouldNotHaveBeenCalled();
         $logWarning->shouldHaveBeenCalled();
-        $dispatch->shouldNotHaveBeenCalled();
+        $dispatch->shouldHaveBeenCalledOnce();
     }
 
     /** @test */
diff --git a/module/Core/test/EventDispatcher/UpdateGeoLiteDbTest.php b/module/Core/test/EventDispatcher/UpdateGeoLiteDbTest.php
index a5edcbcb..a492f9dd 100644
--- a/module/Core/test/EventDispatcher/UpdateGeoLiteDbTest.php
+++ b/module/Core/test/EventDispatcher/UpdateGeoLiteDbTest.php
@@ -84,6 +84,10 @@ class UpdateGeoLiteDbTest extends TestCase
         $checkDbUpdate = $this->dbUpdater->checkDbUpdate(Argument::cetera())->will(
             function (array $args) use ($total, $downloaded, $oldDbExists): void {
                 [, $secondCallback] = $args;
+
+                // Invoke several times to ensure the log is printed only once
+                $secondCallback($total, $downloaded, $oldDbExists);
+                $secondCallback($total, $downloaded, $oldDbExists);
                 $secondCallback($total, $downloaded, $oldDbExists);
             },
         );