From f0dc32b6e5049456570d7861cb72dc9bc3c03aaf Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 16 May 2021 09:51:52 +0200 Subject: [PATCH] Added logic for new tracking options --- config/autoload/tracking.global.php | 3 -- docker/config/shlink_in_docker.local.php | 1 - module/Core/src/Model/Visitor.php | 13 ++++++++ module/Core/src/Options/TrackingOptions.php | 11 ------- module/Core/src/Visit/VisitsTracker.php | 34 +++++++++++++------- module/Core/test/Model/VisitorTest.php | 25 ++++++++++++++ module/Core/test/Visit/VisitsTrackerTest.php | 16 +++++++++ 7 files changed, 76 insertions(+), 27 deletions(-) diff --git a/config/autoload/tracking.global.php b/config/autoload/tracking.global.php index 1ddec8b8..4fdb5b19 100644 --- a/config/autoload/tracking.global.php +++ b/config/autoload/tracking.global.php @@ -21,9 +21,6 @@ return [ // If true, visits will be tracked, but neither the IP address or the location will be resolved 'disable_ip_tracking' => false, - // If true, visits will be tracked including the IP address, but the location won't be resolved - 'disable_location_tracking' => false, - // If true, the referrer will not be tracked 'disable_referrer_tracking' => false, diff --git a/docker/config/shlink_in_docker.local.php b/docker/config/shlink_in_docker.local.php index b467237e..2a8369d7 100644 --- a/docker/config/shlink_in_docker.local.php +++ b/docker/config/shlink_in_docker.local.php @@ -122,7 +122,6 @@ return [ 'disable_track_param' => env('DISABLE_TRACK_PARAM'), 'disable_tracking' => (bool) env('DISABLE_TRACKING', false), 'disable_ip_tracking' => (bool) env('DISABLE_IP_TRACKING', false), - 'disable_location_tracking' => (bool) env('DISABLE_LOCATION_TRACKING', false), 'disable_referrer_tracking' => (bool) env('DISABLE_REFERRER_TRACKING', false), 'disable_ua_tracking' => (bool) env('DISABLE_UA_TRACKING', false), ], diff --git a/module/Core/src/Model/Visitor.php b/module/Core/src/Model/Visitor.php index 7438bdce..9564a41c 100644 --- a/module/Core/src/Model/Visitor.php +++ b/module/Core/src/Model/Visitor.php @@ -6,6 +6,7 @@ namespace Shlinkio\Shlink\Core\Model; use Psr\Http\Message\ServerRequestInterface; use Shlinkio\Shlink\Common\Middleware\IpAddressMiddlewareFactory; +use Shlinkio\Shlink\Core\Options\TrackingOptions; use function substr; @@ -68,4 +69,16 @@ final class Visitor { return $this->visitedUrl; } + + public function normalizeForTrackingOptions(TrackingOptions $options): self + { + $instance = self::emptyInstance(); + + $instance->userAgent = $options->disableUaTracking() ? '' : $this->userAgent; + $instance->referer = $options->disableReferrerTracking() ? '' : $this->referer; + $instance->remoteAddress = $options->disableIpTracking() ? null : $this->remoteAddress; + $instance->visitedUrl = $this->visitedUrl; + + return $instance; + } } diff --git a/module/Core/src/Options/TrackingOptions.php b/module/Core/src/Options/TrackingOptions.php index 0e1762b0..98e09085 100644 --- a/module/Core/src/Options/TrackingOptions.php +++ b/module/Core/src/Options/TrackingOptions.php @@ -13,7 +13,6 @@ class TrackingOptions extends AbstractOptions private ?string $disableTrackParam = null; private bool $disableTracking = false; private bool $disableIpTracking = false; - private bool $disableLocationTracking = false; private bool $disableReferrerTracking = false; private bool $disableUaTracking = false; @@ -67,16 +66,6 @@ class TrackingOptions extends AbstractOptions $this->disableIpTracking = $disableIpTracking; } - public function disableLocationTracking(): bool - { - return $this->disableLocationTracking; - } - - protected function setDisableLocationTracking(bool $disableLocationTracking): void - { - $this->disableLocationTracking = $disableLocationTracking; - } - public function disableReferrerTracking(): bool { return $this->disableReferrerTracking; diff --git a/module/Core/src/Visit/VisitsTracker.php b/module/Core/src/Visit/VisitsTracker.php index fb820b24..68c1891a 100644 --- a/module/Core/src/Visit/VisitsTracker.php +++ b/module/Core/src/Visit/VisitsTracker.php @@ -32,39 +32,49 @@ class VisitsTracker implements VisitsTrackerInterface { $this->trackVisit( Visit::forValidShortUrl($shortUrl, $visitor, $this->options->anonymizeRemoteAddr()), - $visitor, + $visitor->normalizeForTrackingOptions($this->options), ); } public function trackInvalidShortUrlVisit(Visitor $visitor): void { - if (! $this->options->trackOrphanVisits()) { - return; - } - - $this->trackVisit(Visit::forInvalidShortUrl($visitor, $this->options->anonymizeRemoteAddr()), $visitor); + $this->trackOrphanVisit( + Visit::forInvalidShortUrl($visitor, $this->options->anonymizeRemoteAddr()), + $visitor->normalizeForTrackingOptions($this->options), + ); } public function trackBaseUrlVisit(Visitor $visitor): void { - if (! $this->options->trackOrphanVisits()) { - return; - } - - $this->trackVisit(Visit::forBasePath($visitor, $this->options->anonymizeRemoteAddr()), $visitor); + $this->trackOrphanVisit( + Visit::forBasePath($visitor, $this->options->anonymizeRemoteAddr()), + $visitor->normalizeForTrackingOptions($this->options), + ); } public function trackRegularNotFoundVisit(Visitor $visitor): void + { + $this->trackOrphanVisit( + Visit::forRegularNotFound($visitor, $this->options->anonymizeRemoteAddr()), + $visitor->normalizeForTrackingOptions($this->options), + ); + } + + private function trackOrphanVisit(Visit $visit, Visitor $visitor): void { if (! $this->options->trackOrphanVisits()) { return; } - $this->trackVisit(Visit::forRegularNotFound($visitor, $this->options->anonymizeRemoteAddr()), $visitor); + $this->trackVisit($visit, $visitor); } private function trackVisit(Visit $visit, Visitor $visitor): void { + if ($this->options->disableTracking()) { + return; + } + $this->em->transactional(function () use ($visit, $visitor): void { $this->em->persist($visit); $this->em->flush(); diff --git a/module/Core/test/Model/VisitorTest.php b/module/Core/test/Model/VisitorTest.php index e1003056..50c277c4 100644 --- a/module/Core/test/Model/VisitorTest.php +++ b/module/Core/test/Model/VisitorTest.php @@ -6,6 +6,7 @@ namespace ShlinkioTest\Shlink\Core\Model; use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\Core\Model\Visitor; +use Shlinkio\Shlink\Core\Options\TrackingOptions; use function random_int; use function str_repeat; @@ -71,4 +72,28 @@ class VisitorTest extends TestCase } return $randomString; } + + /** @test */ + public function newNormalizedInstanceIsCreatedFromTrackingOptions(): void + { + $visitor = new Visitor( + $this->generateRandomString(2000), + $this->generateRandomString(2000), + $this->generateRandomString(2000), + $this->generateRandomString(2000), + ); + $normalizedVisitor = $visitor->normalizeForTrackingOptions(new TrackingOptions([ + 'disableIpTracking' => true, + 'disableReferrerTracking' => true, + 'disableUaTracking' => true, + ])); + + self::assertNotSame($visitor, $normalizedVisitor); + self::assertEmpty($normalizedVisitor->getUserAgent()); + self::assertNotEmpty($visitor->getUserAgent()); + self::assertEmpty($normalizedVisitor->getReferer()); + self::assertNotEmpty($visitor->getReferer()); + self::assertNull($normalizedVisitor->getRemoteAddress()); + self::assertNotNull($visitor->getRemoteAddress()); + } } diff --git a/module/Core/test/Visit/VisitsTrackerTest.php b/module/Core/test/Visit/VisitsTrackerTest.php index e39a4522..45188f6c 100644 --- a/module/Core/test/Visit/VisitsTrackerTest.php +++ b/module/Core/test/Visit/VisitsTrackerTest.php @@ -57,6 +57,22 @@ class VisitsTrackerTest extends TestCase $this->eventDispatcher->dispatch(Argument::type(UrlVisited::class))->shouldHaveBeenCalled(); } + /** + * @test + * @dataProvider provideTrackingMethodNames + */ + public function trackingIsSkippedCompletelyWhenDisabledFromOptions(string $method, array $args): void + { + $this->options->disableTracking = true; + + $this->visitsTracker->{$method}(...$args); + + $this->eventDispatcher->dispatch(Argument::cetera())->shouldNotHaveBeenCalled(); + $this->em->transactional(Argument::cetera())->shouldNotHaveBeenCalled(); + $this->em->persist(Argument::cetera())->shouldNotHaveBeenCalled(); + $this->em->flush()->shouldNotHaveBeenCalled(); + } + public function provideTrackingMethodNames(): iterable { yield 'track' => ['track', [ShortUrl::createEmpty(), Visitor::emptyInstance()]];