diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index ceea6f92..4ab04b20 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -7,6 +7,6 @@ A clear and concise description of what this pull request adds or changes. - [ ] Breaking change (fix or feature that would cause existing functionality to change) ## Checklist -- [ ] I verified that my code applies to the guidelines (`composer kimai:code-check`) +- [ ] I verified that my code applies to the guidelines (`composer code-check`) - [ ] I updated the documentation (see [here](https://github.com/kimai/www.kimai.org/tree/master/_documentation)) - [ ] I agree that this code is used in Kimai and will be published under the [MIT license](https://github.com/kevinpapst/kimai2/blob/master/LICENSE) diff --git a/composer.json b/composer.json index edb45917..3255254f 100644 --- a/composer.json +++ b/composer.json @@ -152,7 +152,8 @@ "@kimai:code-lint", "@kimai:tests-unit" ], - "kimai:code-check": [ + "kimai:code-check": "@code-check", + "code-check": [ "@kimai:pre-commit", "@kimai:tests-integration" ], @@ -166,14 +167,15 @@ "kimai:tests": "vendor/bin/phpunit tests/", "kimai:tests-unit": "vendor/bin/phpunit --exclude-group integration tests/", "kimai:tests-integration": "vendor/bin/phpunit --group integration tests/", - "kimai:phpstan": [ + "kimai:phpstan": "@phpstan", + "phpstan": [ "vendor/bin/phpstan analyse src -c phpstan.neon --level=5", "vendor/bin/phpstan analyse tests -c tests/phpstan.neon --level=4" ], - "kimai:codestyle": "vendor/bin/php-cs-fixer fix --dry-run --verbose --show-progress=none", - "codestyle": "@kimai:codestyle", - "kimai:codestyle-fix": "vendor/bin/php-cs-fixer fix", - "codestyle-fix": "@kimai:codestyle-fix" + "kimai:codestyle": "@codestyle", + "codestyle": "vendor/bin/php-cs-fixer fix --dry-run --verbose --show-progress=none", + "kimai:codestyle-fix": "@codestyle-fix", + "codestyle-fix": "vendor/bin/php-cs-fixer fix" }, "conflict": { "symfony/symfony": "*" diff --git a/src/Configuration/TimesheetConfiguration.php b/src/Configuration/TimesheetConfiguration.php index 19d26eb5..3f261c46 100644 --- a/src/Configuration/TimesheetConfiguration.php +++ b/src/Configuration/TimesheetConfiguration.php @@ -23,6 +23,11 @@ class TimesheetConfiguration implements SystemBundleConfiguration return (bool) $this->find('rules.allow_future_times'); } + public function isAllowOverlappingRecords(): bool + { + return (bool) $this->find('rules.allow_overlapping_records'); + } + public function getTrackingMode(): string { return (string) $this->find('mode'); diff --git a/src/Controller/SystemConfigurationController.php b/src/Controller/SystemConfigurationController.php index c2f6e3f5..4f415858 100644 --- a/src/Controller/SystemConfigurationController.php +++ b/src/Controller/SystemConfigurationController.php @@ -228,6 +228,10 @@ final class SystemConfigurationController extends AbstractController ->setName('timesheet.rules.allow_future_times') ->setType(CheckboxType::class) ->setTranslationDomain('system-configuration'), + (new Configuration()) + ->setName('timesheet.rules.allow_overlapping_records') + ->setType(CheckboxType::class) + ->setTranslationDomain('system-configuration'), (new Configuration()) ->setName('timesheet.active_entries.hard_limit') ->setType(IntegerType::class) diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php index 9f7aa021..2081829b 100644 --- a/src/DependencyInjection/Configuration.php +++ b/src/DependencyInjection/Configuration.php @@ -195,6 +195,9 @@ class Configuration implements ConfigurationInterface ->booleanNode('allow_future_times') ->defaultTrue() ->end() + ->booleanNode('allow_overlapping_records') + ->defaultTrue() + ->end() ->end() ->end() ->end() diff --git a/src/Repository/ConfigurationRepository.php b/src/Repository/ConfigurationRepository.php index 78a8b971..38d7e804 100644 --- a/src/Repository/ConfigurationRepository.php +++ b/src/Repository/ConfigurationRepository.php @@ -20,9 +20,10 @@ class ConfigurationRepository extends EntityRepository implements ConfigLoaderIn private static $cacheByPrefix = null; private static $cacheAll = []; - private function clearCache() + public function clearCache() { static::$cacheByPrefix = null; + static::$cacheAll = null; } private function prefillCache() @@ -44,6 +45,14 @@ class ConfigurationRepository extends EntityRepository implements ConfigLoaderIn } } + public function saveConfiguration(Configuration $configuration) + { + $entityManager = $this->getEntityManager(); + $entityManager->persist($configuration); + $entityManager->flush(); + $this->clearCache(); + } + /** * @param string $prefix * @return Configuration[] diff --git a/src/Repository/TimesheetRepository.php b/src/Repository/TimesheetRepository.php index c99cfad9..effd070a 100644 --- a/src/Repository/TimesheetRepository.php +++ b/src/Repository/TimesheetRepository.php @@ -939,4 +939,33 @@ class TimesheetRepository extends EntityRepository return $results; } + + public function hasRecordForTime(Timesheet $timesheet): bool + { + $qb = $this->getEntityManager()->createQueryBuilder(); + + $or = $qb->expr()->orX( + $qb->expr()->between(':begin', 't.begin', 't.end') + ); + + if (null !== $timesheet->getEnd()) { + $or->add($qb->expr()->between(':end', 't.begin', 't.end')); + $or->add($qb->expr()->between('t.begin', ':begin', ':end')); + $or->add($qb->expr()->between('t.end', ':begin', ':end')); + $qb->setParameter('end', $timesheet->getEnd()); + } + + $qb->select('t') + ->from(Timesheet::class, 't') + ->andWhere($qb->expr()->eq('t.user', ':user')) + ->andWhere($qb->expr()->isNotNull('t.end')) + ->andWhere($or) + ->setParameter('begin', $timesheet->getBegin()) + ->setParameter('user', $timesheet->getUser()) + ; + + $result = $qb->getQuery()->getResult(); + + return !empty($result); + } } diff --git a/src/Validator/Constraints/Timesheet.php b/src/Validator/Constraints/Timesheet.php index a372f563..4cabab17 100644 --- a/src/Validator/Constraints/Timesheet.php +++ b/src/Validator/Constraints/Timesheet.php @@ -30,6 +30,7 @@ class Timesheet extends Constraint public const START_DISALLOWED = 'kimai-timesheet-90'; public const PROJECT_NOT_STARTED = 'kimai-timesheet-91'; public const PROJECT_ALREADY_ENDED = 'kimai-timesheet-92'; + public const RECORD_OVERLAPPING = 'kimai-timesheet-93'; protected static $errorNames = [ self::MISSING_BEGIN_ERROR => 'You must submit a begin date.', @@ -44,6 +45,7 @@ class Timesheet extends Constraint self::START_DISALLOWED => 'You are not allowed to start this timesheet record.', self::PROJECT_NOT_STARTED => 'The project has not started at that time.', self::PROJECT_ALREADY_ENDED => 'The project is finished at that time.', + self::RECORD_OVERLAPPING => 'You already have an entry for this time.', ]; public $message = 'This timesheet has invalid settings.'; diff --git a/src/Validator/Constraints/TimesheetValidator.php b/src/Validator/Constraints/TimesheetValidator.php index b30af822..4feca36e 100644 --- a/src/Validator/Constraints/TimesheetValidator.php +++ b/src/Validator/Constraints/TimesheetValidator.php @@ -11,6 +11,7 @@ namespace App\Validator\Constraints; use App\Configuration\TimesheetConfiguration; use App\Entity\Timesheet as TimesheetEntity; +use App\Repository\TimesheetRepository; use App\Timesheet\TrackingModeService; use App\Validator\Constraints\Timesheet as TimesheetConstraint; use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; @@ -33,16 +34,17 @@ class TimesheetValidator extends ConstraintValidator * @var TrackingModeService */ protected $trackingModeService; - /** - * @param AuthorizationCheckerInterface $auth - * @param TimesheetConfiguration $configuration + * @var TimesheetRepository */ - public function __construct(AuthorizationCheckerInterface $auth, TimesheetConfiguration $configuration, TrackingModeService $service) + private $repository; + + public function __construct(AuthorizationCheckerInterface $auth, TimesheetConfiguration $configuration, TrackingModeService $service, TimesheetRepository $repository) { $this->auth = $auth; $this->configuration = $configuration; $this->trackingModeService = $service; + $this->repository = $repository; } /** @@ -62,6 +64,38 @@ class TimesheetValidator extends ConstraintValidator $this->validateBeginAndEnd($value, $this->context); $this->validateActivityAndProject($value, $this->context); $this->validatePermissions($value, $this->context); + $this->validateActiveLimit($value, $this->context); + $this->validateOverlapping($value, $this->context); + } + + /** + * @param TimesheetEntity $timesheet + * @param ExecutionContextInterface $context + */ + protected function validateOverlapping(TimesheetEntity $timesheet, ExecutionContextInterface $context) + { + if ($this->configuration->isAllowOverlappingRecords()) { + return; + } + + if (!$this->repository->hasRecordForTime($timesheet)) { + return; + } + + $context->buildViolation('You already have an entry for this time.') + ->atPath('begin') + ->setTranslationDomain('validators') + ->setCode(TimesheetConstraint::RECORD_OVERLAPPING) + ->addViolation(); + } + + /** + * @param TimesheetEntity $timesheet + * @param ExecutionContextInterface $context + */ + protected function validateActiveLimit(TimesheetEntity $timesheet, ExecutionContextInterface $context) + { + // TODO check active entries against hard_limit } /** @@ -91,8 +125,6 @@ class TimesheetValidator extends ConstraintValidator return; } } - - // TODO check active entries against hard_limit } /** diff --git a/tests/Controller/ControllerBaseTest.php b/tests/Controller/ControllerBaseTest.php index b48355d1..d8991e9b 100644 --- a/tests/Controller/ControllerBaseTest.php +++ b/tests/Controller/ControllerBaseTest.php @@ -217,16 +217,15 @@ abstract class ControllerBaseTest extends WebTestCase } /** - * @param string $role the USER role to use for the request + * @param HttpKernelBrowser $client the client to use * @param string $url the URL of the page displaying the initial form to submit * @param string $formSelector a selector to find the form to test * @param array $formData values to fill in the form * @param array $fieldNames array of form-fields that should fail * @param bool $disableValidation whether the form should validate before submitting or not */ - protected function assertFormHasValidationError($role, $url, $formSelector, array $formData, array $fieldNames, $disableValidation = true) + protected function assertHasValidationError(HttpKernelBrowser $client, $url, $formSelector, array $formData, array $fieldNames, $disableValidation = true) { - $client = $this->getClientForAuthenticatedUser($role); $crawler = $client->request('GET', $this->createUrl($url)); $form = $crawler->filter($formSelector)->form(); if ($disableValidation) { @@ -260,6 +259,20 @@ abstract class ControllerBaseTest extends WebTestCase } } + /** + * @param string $role the USER role to use for the request + * @param string $url the URL of the page displaying the initial form to submit + * @param string $formSelector a selector to find the form to test + * @param array $formData values to fill in the form + * @param array $fieldNames array of form-fields that should fail + * @param bool $disableValidation whether the form should validate before submitting or not + */ + protected function assertFormHasValidationError($role, $url, $formSelector, array $formData, array $fieldNames, $disableValidation = true) + { + $client = $this->getClientForAuthenticatedUser($role); + $this->assertHasValidationError($client, $url, $formSelector, $formData, $fieldNames, $disableValidation); + } + /** * @param HttpKernelBrowser $client */ diff --git a/tests/Controller/SystemConfigurationControllerTest.php b/tests/Controller/SystemConfigurationControllerTest.php index 5e6b2594..ac4df806 100644 --- a/tests/Controller/SystemConfigurationControllerTest.php +++ b/tests/Controller/SystemConfigurationControllerTest.php @@ -100,6 +100,7 @@ class SystemConfigurationControllerTest extends ControllerBaseTest ['name' => 'timesheet.mode', 'value' => 'duration_only'], ['name' => 'timesheet.active_entries.default_begin', 'value' => '23:59'], ['name' => 'timesheet.rules.allow_future_times', 'value' => false], + ['name' => 'timesheet.rules.allow_overlapping_records', 'value' => false], ['name' => 'timesheet.active_entries.hard_limit', 'value' => 99], ['name' => 'timesheet.active_entries.soft_limit', 'value' => 77], ] @@ -114,6 +115,7 @@ class SystemConfigurationControllerTest extends ControllerBaseTest $configService = static::$kernel->getContainer()->get(SystemConfiguration::class); $this->assertEquals('duration_only', $configService->find('timesheet.mode')); $this->assertEquals(false, $configService->find('timesheet.rules.allow_future_times')); + $this->assertEquals(false, $configService->find('timesheet.rules.allow_overlapping_records')); $this->assertEquals(99, $configService->find('timesheet.active_entries.hard_limit')); $this->assertEquals(77, $configService->find('timesheet.active_entries.soft_limit')); } @@ -130,6 +132,7 @@ class SystemConfigurationControllerTest extends ControllerBaseTest ['name' => 'timesheet.mode', 'value' => 'foo'], ['name' => 'timesheet.active_entries.default_begin', 'value' => '23:59'], ['name' => 'timesheet.rules.allow_future_times', 'value' => 1], + ['name' => 'timesheet.rules.allow_overlapping_records', 'value' => 1], ['name' => 'timesheet.active_entries.hard_limit', 'value' => -1], ['name' => 'timesheet.active_entries.soft_limit', 'value' => -1], ] @@ -137,8 +140,8 @@ class SystemConfigurationControllerTest extends ControllerBaseTest ], [ '#system_configuration_form_timesheet_configuration_0_value', // mode - '#system_configuration_form_timesheet_configuration_3_value', // hard_limit - '#system_configuration_form_timesheet_configuration_4_value', // soft_limit + '#system_configuration_form_timesheet_configuration_4_value', // hard_limit + '#system_configuration_form_timesheet_configuration_5_value', // soft_limit ], true ); diff --git a/tests/Controller/TimesheetControllerTest.php b/tests/Controller/TimesheetControllerTest.php index 12b03eae..0d367978 100644 --- a/tests/Controller/TimesheetControllerTest.php +++ b/tests/Controller/TimesheetControllerTest.php @@ -9,10 +9,12 @@ namespace App\Tests\Controller; +use App\Entity\Configuration; use App\Entity\Timesheet; use App\Entity\TimesheetMeta; use App\Entity\User; use App\Form\Type\DateRangeType; +use App\Repository\ConfigurationRepository; use App\Tests\DataFixtures\TimesheetFixtures; use App\Tests\Mocks\TimesheetTestMetaFieldSubscriberMock; @@ -250,6 +252,115 @@ class TimesheetControllerTest extends ControllerBaseTest $this->assertEquals($expected->format(\DateTime::ATOM), $timesheet->getEnd()->format(\DateTime::ATOM)); } + public function testCreateActionWithFromAndToValuesTwice() + { + $client = $this->getClientForAuthenticatedUser(User::ROLE_ADMIN); + $this->request($client, '/timesheet/create?from=2018-08-02T20%3A00%3A00&to=2018-08-02T20%3A30%3A00'); + $this->assertTrue($client->getResponse()->isSuccessful()); + + $form = $client->getCrawler()->filter('form[name=timesheet_edit_form]')->form(); + $client->submit($form, [ + 'timesheet_edit_form' => [ + 'hourlyRate' => 100, + 'project' => 1, + 'activity' => 1, + ] + ]); + + $this->assertIsRedirect($client, $this->createUrl('/timesheet/')); + $client->followRedirect(); + $this->assertTrue($client->getResponse()->isSuccessful()); + $this->assertHasFlashSuccess($client); + + $em = $this->getEntityManager(); + /** @var Timesheet $timesheet */ + $timesheet = $em->getRepository(Timesheet::class)->find(1); + $this->assertInstanceOf(\DateTime::class, $timesheet->getBegin()); + $this->assertInstanceOf(\DateTime::class, $timesheet->getEnd()); + $this->assertEquals(50, $timesheet->getRate()); + + $expected = new \DateTime('2018-08-02T20:00:00'); + $this->assertEquals($expected->format(\DateTime::ATOM), $timesheet->getBegin()->format(\DateTime::ATOM)); + + $expected = new \DateTime('2018-08-02T20:30:00'); + $this->assertEquals($expected->format(\DateTime::ATOM), $timesheet->getEnd()->format(\DateTime::ATOM)); + + // create a second entry that is overlapping + $this->request($client, '/timesheet/create?from=2018-08-02T20%3A02%3A00&to=2018-08-02T20%3A20%3A00'); + $this->assertTrue($client->getResponse()->isSuccessful()); + + $form = $client->getCrawler()->filter('form[name=timesheet_edit_form]')->form(); + $client->submit($form, [ + 'timesheet_edit_form' => [ + 'hourlyRate' => 100, + 'project' => 1, + 'activity' => 1, + ] + ]); + $this->assertIsRedirect($client, $this->createUrl('/timesheet/')); + $client->followRedirect(); + $this->assertTrue($client->getResponse()->isSuccessful()); + $this->assertHasFlashSuccess($client); + } + + public function testCreateActionWithFromAndToValuesTwiceFailsOnOverlappingRecord() + { + $client = $this->getClientForAuthenticatedUser(User::ROLE_ADMIN); + $this->request($client, '/timesheet/create?from=2018-08-02T20%3A00%3A00&to=2018-08-02T20%3A30%3A00'); + $this->assertTrue($client->getResponse()->isSuccessful()); + + $form = $client->getCrawler()->filter('form[name=timesheet_edit_form]')->form(); + $client->submit($form, [ + 'timesheet_edit_form' => [ + 'hourlyRate' => 100, + 'project' => 1, + 'activity' => 1, + ] + ]); + + $this->assertIsRedirect($client, $this->createUrl('/timesheet/')); + $client->followRedirect(); + $this->assertTrue($client->getResponse()->isSuccessful()); + $this->assertHasFlashSuccess($client); + + $em = $this->getEntityManager(); + /** @var Timesheet $timesheet */ + $timesheet = $em->getRepository(Timesheet::class)->find(1); + $this->assertInstanceOf(\DateTime::class, $timesheet->getBegin()); + $this->assertInstanceOf(\DateTime::class, $timesheet->getEnd()); + $this->assertEquals(50, $timesheet->getRate()); + + $expected = new \DateTime('2018-08-02T20:00:00'); + $this->assertEquals($expected->format(\DateTime::ATOM), $timesheet->getBegin()->format(\DateTime::ATOM)); + + $expected = new \DateTime('2018-08-02T20:30:00'); + $this->assertEquals($expected->format(\DateTime::ATOM), $timesheet->getEnd()->format(\DateTime::ATOM)); + + /** @var ConfigurationRepository $configurations */ + $configurations = $em->getRepository(Configuration::class); + $config = new Configuration(); + $config->setName('timesheet.rules.allow_overlapping_records'); + $config->setValue(false); + $configurations->saveConfiguration($config); + + // create a second entry that is overlapping fails due to the changed config above + $this->assertHasValidationError( + $client, + '/timesheet/create?from=2018-08-02T20%3A02%3A00&to=2018-08-02T20%3A20%3A00', + 'form[name=timesheet_edit_form]', + [ + 'timesheet_edit_form' => [ + 'hourlyRate' => 100, + 'project' => 1, + 'activity' => 1, + ] + ], + ['#timesheet_edit_form_begin'] + ); + + $configurations->clearCache(); + } + public function testCreateActionWithBeginAndEndAndTagValues() { $client = $this->getClientForAuthenticatedUser(User::ROLE_ADMIN); diff --git a/tests/DependencyInjection/AppExtensionTest.php b/tests/DependencyInjection/AppExtensionTest.php index 98a21e2f..fb2c99f8 100644 --- a/tests/DependencyInjection/AppExtensionTest.php +++ b/tests/DependencyInjection/AppExtensionTest.php @@ -199,6 +199,7 @@ class AppExtensionTest extends TestCase ], 'rules' => [ 'allow_future_times' => true, + 'allow_overlapping_records' => true, ], 'default_begin' => 'now', ], diff --git a/tests/DependencyInjection/ConfigurationTest.php b/tests/DependencyInjection/ConfigurationTest.php index 5791d12b..7bdee3ff 100644 --- a/tests/DependencyInjection/ConfigurationTest.php +++ b/tests/DependencyInjection/ConfigurationTest.php @@ -278,6 +278,7 @@ class ConfigurationTest extends TestCase ], 'rules' => [ 'allow_future_times' => true, + 'allow_overlapping_records' => true, ], ], 'user' => [ diff --git a/tests/Validator/Constraints/TimesheetValidatorTest.php b/tests/Validator/Constraints/TimesheetValidatorTest.php index 4b7eaa01..016f54cb 100644 --- a/tests/Validator/Constraints/TimesheetValidatorTest.php +++ b/tests/Validator/Constraints/TimesheetValidatorTest.php @@ -15,6 +15,7 @@ use App\Entity\Activity; use App\Entity\Customer; use App\Entity\Project; use App\Entity\Timesheet; +use App\Repository\TimesheetRepository; use App\Tests\Mocks\TrackingModeServiceFactory; use App\Validator\Constraints\Timesheet as TimesheetConstraint; use App\Validator\Constraints\TimesheetValidator; @@ -38,6 +39,7 @@ class TimesheetValidatorTest extends ConstraintValidatorTestCase $config = new TimesheetConfiguration($loader, [ 'rules' => [ 'allow_future_times' => false, + 'allow_overlapping_records' => true, ], 'rounding' => [ 'default' => [ @@ -46,8 +48,9 @@ class TimesheetValidatorTest extends ConstraintValidatorTestCase ] ]); $service = (new TrackingModeServiceFactory($this))->create('default'); + $repository = $this->createMock(TimesheetRepository::class); - return new TimesheetValidator($authMock, $config, $service); + return new TimesheetValidator($authMock, $config, $service, $repository); } public function testConstraintIsInvalid() diff --git a/translations/system-configuration.de.xlf b/translations/system-configuration.de.xlf index 58e9d17f..d8afe6cf 100644 --- a/translations/system-configuration.de.xlf +++ b/translations/system-configuration.de.xlf @@ -66,6 +66,10 @@ label.timesheet.rules.allow_future_times Erlaube Zeiteinträge in der Zukunft + + label.timesheet.rules.allow_overlapping_records + Erlaube überlappende Zeiteinträge + label.timesheet.active_entries.hard_limit Erlaubte Anzahl an gleichzeitig laufenden Zeiteinträgen diff --git a/translations/system-configuration.en.xlf b/translations/system-configuration.en.xlf index 31816501..177040b6 100644 --- a/translations/system-configuration.en.xlf +++ b/translations/system-configuration.en.xlf @@ -66,6 +66,10 @@ label.timesheet.rules.allow_future_times Allow time entries in the future + + label.timesheet.rules.allow_overlapping_records + Allow overlapping time entries + label.timesheet.active_entries.hard_limit Permitted number of simultaneously running time entries