From 86c30ee731ed1f1cce3adefaf9ebd81eb1ab92da Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 6 Oct 2018 11:41:26 +0200 Subject: [PATCH 1/5] Added new visits_threshold config to installation --- .../Plugin/ApplicationConfigCustomizer.php | 19 +++++++++++++++++++ .../src/Model/CustomizableAppConfig.php | 6 ++++++ 2 files changed, 25 insertions(+) diff --git a/module/Installer/src/Config/Plugin/ApplicationConfigCustomizer.php b/module/Installer/src/Config/Plugin/ApplicationConfigCustomizer.php index 7843833a..2e56d1e2 100644 --- a/module/Installer/src/Config/Plugin/ApplicationConfigCustomizer.php +++ b/module/Installer/src/Config/Plugin/ApplicationConfigCustomizer.php @@ -15,9 +15,13 @@ class ApplicationConfigCustomizer implements ConfigCustomizerInterface public const SECRET = 'SECRET'; public const DISABLE_TRACK_PARAM = 'DISABLE_TRACK_PARAM'; + public const CHECK_VISITS_THRESHOLD = 'CHECK_VISITS_THRESHOLD'; + public const VISITS_THRESHOLD = 'VISITS_THRESHOLD'; private const EXPECTED_KEYS = [ self::SECRET, self::DISABLE_TRACK_PARAM, + self::CHECK_VISITS_THRESHOLD, + self::VISITS_THRESHOLD, ]; public function process(SymfonyStyle $io, CustomizableAppConfig $appConfig): void @@ -31,6 +35,11 @@ class ApplicationConfigCustomizer implements ConfigCustomizerInterface $io->title('APPLICATION'); foreach ($keysToAskFor as $key) { + // Skip visits threshold when the user decided not to check visits on deletions + if ($key === self::VISITS_THRESHOLD && ! $app[self::CHECK_VISITS_THRESHOLD]) { + continue; + } + $app[$key] = $this->ask($io, $key); } $appConfig->setApp($app); @@ -49,6 +58,16 @@ class ApplicationConfigCustomizer implements ConfigCustomizerInterface 'Provide a parameter name that you will be able to use to disable tracking on specific request to ' . 'short URLs (leave empty and this feature won\'t be enabled)' ); + case self::CHECK_VISITS_THRESHOLD: + return $io->confirm( + 'Do you want to enable a safety check which will not allow short URLs to be deleted when they ' + . 'have more than a specific amount of visits?' + ); + case self::VISITS_THRESHOLD: + return $io->ask( + 'What is the amount of visits from which the system will not allow short URLs to be deleted?', + 15 + ); } return ''; diff --git a/module/Installer/src/Model/CustomizableAppConfig.php b/module/Installer/src/Model/CustomizableAppConfig.php index 253cba21..2e298f42 100644 --- a/module/Installer/src/Model/CustomizableAppConfig.php +++ b/module/Installer/src/Model/CustomizableAppConfig.php @@ -121,6 +121,8 @@ final class CustomizableAppConfig implements ArraySerializableInterface $this->setApp($this->mapExistingPathsToKeys([ ApplicationConfigCustomizer::SECRET => ['app_options', 'secret_key'], ApplicationConfigCustomizer::DISABLE_TRACK_PARAM => ['app_options', 'disable_track_param'], + ApplicationConfigCustomizer::CHECK_VISITS_THRESHOLD => ['delete_short_urls', 'check_visits_threshold'], + ApplicationConfigCustomizer::VISITS_THRESHOLD => ['delete_short_urls', 'visits_threshold'], ], $array)); $this->setDatabase($this->mapExistingPathsToKeys([ @@ -165,6 +167,10 @@ final class CustomizableAppConfig implements ArraySerializableInterface 'secret_key' => $this->app[ApplicationConfigCustomizer::SECRET] ?? '', 'disable_track_param' => $this->app[ApplicationConfigCustomizer::DISABLE_TRACK_PARAM] ?? null, ], + 'delete_short_urls' => [ + 'check_visits_threshold' => $this->app[ApplicationConfigCustomizer::CHECK_VISITS_THRESHOLD] ?? true, + 'visits_threshold' => $this->app[ApplicationConfigCustomizer::VISITS_THRESHOLD] ?? 15, + ], 'entity_manager' => [ 'connection' => [ 'driver' => $dbDriver, From 5337eb48e74f4faa3d48596059116dca7ccbcd43 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 6 Oct 2018 11:43:34 +0200 Subject: [PATCH 2/5] Added missing type hint --- .../Installer/src/Config/Plugin/ApplicationConfigCustomizer.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/module/Installer/src/Config/Plugin/ApplicationConfigCustomizer.php b/module/Installer/src/Config/Plugin/ApplicationConfigCustomizer.php index 2e56d1e2..c7987284 100644 --- a/module/Installer/src/Config/Plugin/ApplicationConfigCustomizer.php +++ b/module/Installer/src/Config/Plugin/ApplicationConfigCustomizer.php @@ -64,7 +64,7 @@ class ApplicationConfigCustomizer implements ConfigCustomizerInterface . 'have more than a specific amount of visits?' ); case self::VISITS_THRESHOLD: - return $io->ask( + return (int) $io->ask( 'What is the amount of visits from which the system will not allow short URLs to be deleted?', 15 ); From 75f6160432affa4df8da3244e18267ce0e985951 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 6 Oct 2018 12:01:18 +0200 Subject: [PATCH 3/5] Improved ApplicationConfigCustomizer while asking for visits threshold --- .../Plugin/ApplicationConfigCustomizer.php | 19 +++- .../InvalidConfigOptionException.php | 10 +++ .../ApplicationConfigCustomizerTest.php | 90 ++++++++++++++++++- 3 files changed, 114 insertions(+), 5 deletions(-) create mode 100644 module/Installer/src/Exception/InvalidConfigOptionException.php diff --git a/module/Installer/src/Config/Plugin/ApplicationConfigCustomizer.php b/module/Installer/src/Config/Plugin/ApplicationConfigCustomizer.php index c7987284..e6c02a98 100644 --- a/module/Installer/src/Config/Plugin/ApplicationConfigCustomizer.php +++ b/module/Installer/src/Config/Plugin/ApplicationConfigCustomizer.php @@ -4,10 +4,13 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Installer\Config\Plugin; use Shlinkio\Shlink\Common\Util\StringUtilsTrait; +use Shlinkio\Shlink\Installer\Exception\InvalidConfigOptionException; use Shlinkio\Shlink\Installer\Model\CustomizableAppConfig; use Symfony\Component\Console\Style\SymfonyStyle; use function array_diff; use function array_keys; +use function is_numeric; +use function sprintf; class ApplicationConfigCustomizer implements ConfigCustomizerInterface { @@ -64,12 +67,24 @@ class ApplicationConfigCustomizer implements ConfigCustomizerInterface . 'have more than a specific amount of visits?' ); case self::VISITS_THRESHOLD: - return (int) $io->ask( + return $io->ask( 'What is the amount of visits from which the system will not allow short URLs to be deleted?', - 15 + 15, + [$this, 'validateVisitsThreshold'] ); } return ''; } + + public function validateVisitsThreshold($value): int + { + if (! is_numeric($value) || $value < 1) { + throw new InvalidConfigOptionException( + sprintf('Provided value "%s" is invalid. Expected a number greater than 1', $value) + ); + } + + return (int) $value; + } } diff --git a/module/Installer/src/Exception/InvalidConfigOptionException.php b/module/Installer/src/Exception/InvalidConfigOptionException.php new file mode 100644 index 00000000..647f8edc --- /dev/null +++ b/module/Installer/src/Exception/InvalidConfigOptionException.php @@ -0,0 +1,10 @@ +io->ask(Argument::cetera())->willReturn('the_secret'); + $ask = $this->io->ask(Argument::cetera())->willReturn('asked'); + $confirm = $this->io->confirm(Argument::cetera())->willReturn(false); + $config = new CustomizableAppConfig(); $this->plugin->process($this->io->reveal(), $config); $this->assertTrue($config->hasApp()); $this->assertEquals([ - 'SECRET' => 'the_secret', - 'DISABLE_TRACK_PARAM' => 'the_secret', + 'SECRET' => 'asked', + 'DISABLE_TRACK_PARAM' => 'asked', + 'CHECK_VISITS_THRESHOLD' => false, ], $config->getApp()); $ask->shouldHaveBeenCalledTimes(2); + $confirm->shouldHaveBeenCalledTimes(1); + } + + /** + * @test + */ + public function visitsThresholdIsRequestedIfCheckIsEnabled() + { + $ask = $this->io->ask(Argument::cetera())->will(function (array $args) { + $message = array_shift($args); + return strpos($message, 'What is the amount of visits') === 0 ? 20 : 'asked'; + }); + $confirm = $this->io->confirm(Argument::cetera())->willReturn(true); + + $config = new CustomizableAppConfig(); + + $this->plugin->process($this->io->reveal(), $config); + + $this->assertTrue($config->hasApp()); + $this->assertEquals([ + 'SECRET' => 'asked', + 'DISABLE_TRACK_PARAM' => 'asked', + 'CHECK_VISITS_THRESHOLD' => true, + 'VISITS_THRESHOLD' => 20, + ], $config->getApp()); + $ask->shouldHaveBeenCalledTimes(3); + $confirm->shouldHaveBeenCalledTimes(1); } /** @@ -56,6 +89,8 @@ class ApplicationConfigCustomizerTest extends TestCase $config = new CustomizableAppConfig(); $config->setApp([ 'SECRET' => 'foo', + 'CHECK_VISITS_THRESHOLD' => true, + 'VISITS_THRESHOLD' => 20, ]); $this->plugin->process($this->io->reveal(), $config); @@ -63,6 +98,8 @@ class ApplicationConfigCustomizerTest extends TestCase $this->assertEquals([ 'SECRET' => 'foo', 'DISABLE_TRACK_PARAM' => 'disable_param', + 'CHECK_VISITS_THRESHOLD' => true, + 'VISITS_THRESHOLD' => 20, ], $config->getApp()); $ask->shouldHaveBeenCalledTimes(1); } @@ -78,6 +115,8 @@ class ApplicationConfigCustomizerTest extends TestCase $config->setApp([ 'SECRET' => 'foo', 'DISABLE_TRACK_PARAM' => 'the_new_secret', + 'CHECK_VISITS_THRESHOLD' => true, + 'VISITS_THRESHOLD' => 20, ]); $this->plugin->process($this->io->reveal(), $config); @@ -85,7 +124,52 @@ class ApplicationConfigCustomizerTest extends TestCase $this->assertEquals([ 'SECRET' => 'foo', 'DISABLE_TRACK_PARAM' => 'the_new_secret', + 'CHECK_VISITS_THRESHOLD' => true, + 'VISITS_THRESHOLD' => 20, ], $config->getApp()); $ask->shouldNotHaveBeenCalled(); } + + /** + * @test + * @dataProvider provideInvalidValues + * @param mixed $value + */ + public function validateVisitsThresholdThrowsExceptionWhenProvidedValueIsInvalid($value) + { + $this->expectException(InvalidConfigOptionException::class); + $this->plugin->validateVisitsThreshold($value); + } + + public function provideInvalidValues(): array + { + return [ + 'string' => ['foo'], + 'empty string' => [''], + 'negative number' => [-5], + 'negative number as string' => ['-5'], + 'zero' => [0], + 'zero as string' => ['0'], + ]; + } + + /** + * @test + * @dataProvider provideValidValues + * @param mixed $value + */ + public function validateVisitsThresholdCastsToIntWhenProvidedValueIsValid($value, int $expected) + { + $this->assertEquals($expected, $this->plugin->validateVisitsThreshold($value)); + } + + public function provideValidValues(): array + { + return [ + 'positive as string' => ['20', 20], + 'positive as integer' => [5, 5], + 'one as string' => ['1', 1], + 'one as integer' => [1, 1], + ]; + } } From 0458c4f798bd7357957a479726889231beb50d6e Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 6 Oct 2018 12:08:51 +0200 Subject: [PATCH 4/5] Updated changelog --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 64865b60..1ef2397c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,10 +9,18 @@ It also allows automating the dist file generation in travis-ci builds. +* [#207](https://github.com/shlinkio/shlink/issues/207) Added two new config options which are asked during installation process. The config options already existed in previous shlink version, but you had to manually set their values. + + * Visits threshold to allow short URLs to be deleted. + * Check the visits threshold when trying to delete a short URL via REST API. + #### Changed * [#211](https://github.com/shlinkio/shlink/issues/211) Extracted installer to its own module, which will simplify moving it to a separated package in the future. * [#200](https://github.com/shlinkio/shlink/issues/200) and [#201](https://github.com/shlinkio/shlink/issues/201) Renamed REST Action classes and CLI Command classes to use the concept of `ShortUrl` instead of the concept of `ShortCode` when referring to the entity, and left the `short code` concept to the identifier which is used as a unique code for a specific `Short URL`. +* [#181](https://github.com/shlinkio/shlink/issues/181) When importing the configuration from a previous shlink installation, it no longer asks to import every block. Instead, it is capable oif detecting only new config options introduced in the new version, and ask only for those. + + If no new options are found and you have selected to import config, no further questions will be asked and shlink will just import the old config. #### Deprecated From e5f21a88fac39abc46a9a418042844eba71e40b2 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 6 Oct 2018 12:13:55 +0200 Subject: [PATCH 5/5] Fixed typo --- CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ef2397c..6967b65b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ * [#207](https://github.com/shlinkio/shlink/issues/207) Added two new config options which are asked during installation process. The config options already existed in previous shlink version, but you had to manually set their values. + These are the new options: + * Visits threshold to allow short URLs to be deleted. * Check the visits threshold when trying to delete a short URL via REST API. @@ -18,7 +20,7 @@ * [#211](https://github.com/shlinkio/shlink/issues/211) Extracted installer to its own module, which will simplify moving it to a separated package in the future. * [#200](https://github.com/shlinkio/shlink/issues/200) and [#201](https://github.com/shlinkio/shlink/issues/201) Renamed REST Action classes and CLI Command classes to use the concept of `ShortUrl` instead of the concept of `ShortCode` when referring to the entity, and left the `short code` concept to the identifier which is used as a unique code for a specific `Short URL`. -* [#181](https://github.com/shlinkio/shlink/issues/181) When importing the configuration from a previous shlink installation, it no longer asks to import every block. Instead, it is capable oif detecting only new config options introduced in the new version, and ask only for those. +* [#181](https://github.com/shlinkio/shlink/issues/181) When importing the configuration from a previous shlink installation, it no longer asks to import every block. Instead, it is capable of detecting only new config options introduced in the new version, and ask only for those. If no new options are found and you have selected to import config, no further questions will be asked and shlink will just import the old config.