From 7824dddef723eb5aea9477a1a9330b4c77123614 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 3 Feb 2021 19:22:47 +0100 Subject: [PATCH] Added tracking to tell if short URL titles were autogenerated or not --- data/migrations/Version20210202181026.php | 12 ++++++++++++ .../Shlinkio.Shlink.Core.Entity.ShortUrl.php | 5 +++++ module/Core/src/Entity/ShortUrl.php | 2 ++ module/Core/src/Model/ShortUrlMeta.php | 9 ++++++++- module/Core/src/Service/UrlShortener.php | 19 ++++++++++++------- module/Core/test/Service/UrlShortenerTest.php | 14 ++++++++++++++ 6 files changed, 53 insertions(+), 8 deletions(-) diff --git a/data/migrations/Version20210202181026.php b/data/migrations/Version20210202181026.php index 00e82a40..c964559c 100644 --- a/data/migrations/Version20210202181026.php +++ b/data/migrations/Version20210202181026.php @@ -21,6 +21,9 @@ final class Version20210202181026 extends AbstractMigration 'notnull' => false, 'length' => 512, ]); + $shortUrls->addColumn('title_was_auto_resolved', Types::BOOLEAN, [ + 'default' => false, + ]); } public function down(Schema $schema): void @@ -28,5 +31,14 @@ final class Version20210202181026 extends AbstractMigration $shortUrls = $schema->getTable('short_urls'); $this->skipIf(! $shortUrls->hasColumn(self::TITLE)); $shortUrls->dropColumn(self::TITLE); + $shortUrls->dropColumn('title_was_auto_resolved'); + } + + /** + * @fixme Workaround for https://github.com/doctrine/migrations/issues/1104 + */ + public function isTransactional(): bool + { + return false; } } diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php index df17ad95..751e513c 100644 --- a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php @@ -90,4 +90,9 @@ return static function (ClassMetadata $metadata, array $emConfig): void { ->length(512) ->nullable() ->build(); + + $builder->createField('titleWasAutoResolved', Types::BOOLEAN) + ->columnName('title_was_auto_resolved') + ->option('default', false) + ->build(); }; diff --git a/module/Core/src/Entity/ShortUrl.php b/module/Core/src/Entity/ShortUrl.php index ba7f34f4..84c74874 100644 --- a/module/Core/src/Entity/ShortUrl.php +++ b/module/Core/src/Entity/ShortUrl.php @@ -39,6 +39,7 @@ class ShortUrl extends AbstractEntity private ?string $importOriginalShortCode = null; private ?ApiKey $authorApiKey = null; private ?string $title = null; + private bool $titleWasAutoResolved = false; private function __construct() { @@ -74,6 +75,7 @@ class ShortUrl extends AbstractEntity $instance->domain = $relationResolver->resolveDomain($meta->getDomain()); $instance->authorApiKey = $meta->getApiKey(); $instance->title = $meta->getTitle(); + $instance->titleWasAutoResolved = $meta->titleWasAutoResolved(); return $instance; } diff --git a/module/Core/src/Model/ShortUrlMeta.php b/module/Core/src/Model/ShortUrlMeta.php index a069062c..a741f78c 100644 --- a/module/Core/src/Model/ShortUrlMeta.php +++ b/module/Core/src/Model/ShortUrlMeta.php @@ -29,6 +29,7 @@ final class ShortUrlMeta private ?ApiKey $apiKey = null; private array $tags = []; private ?string $title = null; + private bool $titleWasAutoResolved = false; private function __construct() { @@ -173,10 +174,16 @@ final class ShortUrlMeta return $this->title !== null; } - public function withResolvedTitle(?string $title): self + public function titleWasAutoResolved(): bool + { + return $this->titleWasAutoResolved; + } + + public function withResolvedTitle(string $title): self { $copy = clone $this; $copy->title = $title; + $copy->titleWasAutoResolved = true; return $copy; } diff --git a/module/Core/src/Service/UrlShortener.php b/module/Core/src/Service/UrlShortener.php index aa0908fe..6083e1f4 100644 --- a/module/Core/src/Service/UrlShortener.php +++ b/module/Core/src/Service/UrlShortener.php @@ -45,13 +45,7 @@ class UrlShortener implements UrlShortenerInterface return $existingShortUrl; } - if ($meta->hasTitle()) { - $this->urlValidator->validateUrl($meta->getLongUrl(), $meta->doValidateUrl()); - } else { - $meta = $meta->withResolvedTitle( - $this->urlValidator->validateUrlWithTitle($meta->getLongUrl(), $meta->doValidateUrl()), - ); - } + $meta = $this->processTitleAndValidateUrl($meta); return $this->em->transactional(function () use ($meta) { $shortUrl = ShortUrl::fromMeta($meta, $this->relationResolver); @@ -88,4 +82,15 @@ class UrlShortener implements UrlShortenerInterface throw NonUniqueSlugException::fromSlug($shortUrlToBeCreated->getShortCode(), $domainAuthority); } } + + private function processTitleAndValidateUrl(ShortUrlMeta $meta): ShortUrlMeta + { + if ($meta->hasTitle()) { + $this->urlValidator->validateUrl($meta->getLongUrl(), $meta->doValidateUrl()); + return $meta; + } + + $title = $this->urlValidator->validateUrlWithTitle($meta->getLongUrl(), $meta->doValidateUrl()); + return $title === null ? $meta : $meta->withResolvedTitle($title); + } } diff --git a/module/Core/test/Service/UrlShortenerTest.php b/module/Core/test/Service/UrlShortenerTest.php index beeda044..9fbf82ed 100644 --- a/module/Core/test/Service/UrlShortenerTest.php +++ b/module/Core/test/Service/UrlShortenerTest.php @@ -75,6 +75,20 @@ class UrlShortenerTest extends TestCase $this->urlValidator->validateUrl($longUrl, null)->shouldHaveBeenCalledTimes($validateCallsNum); } + /** + * @test + * @dataProvider provideTitles + */ + public function urlIsProperlyShortenedWithExpectedResolvedTitle(?string $title): void + { + $validateWithTitle = $this->urlValidator->validateUrlWithTitle(Argument::cetera())->willReturn($title); + + $shortUrl = $this->urlShortener->shorten(ShortUrlMeta::fromRawData(['longUrl' => 'foo'])); + + self::assertEquals($title, $shortUrl->getTitle()); + $validateWithTitle->shouldHaveBeenCalledOnce(); + } + public function provideTitles(): iterable { yield 'no title' => [null, 1, 0];