new system-config to prevent overlapping records (#1720)

.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)

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": "*"

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');

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)

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()

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[]

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);
+    }
 }

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.';

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
     }
 
     /**

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
      */

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
         );

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);

tests/DependencyInjection/AppExtensionTest.php

@@ -199,6 +199,7 @@ class AppExtensionTest extends TestCase
                 ],
                 'rules' => [
                     'allow_future_times' => true,
+                    'allow_overlapping_records' => true,
                 ],
                 'default_begin' => 'now',
             ],

tests/DependencyInjection/ConfigurationTest.php

@@ -278,6 +278,7 @@ class ConfigurationTest extends TestCase
                 ],
                 'rules' => [
                     'allow_future_times' => true,
+                    'allow_overlapping_records' => true,
                 ],
             ],
             'user' => [

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()

translations/system-configuration.de.xlf

@@ -66,6 +66,10 @@
                 <source>label.timesheet.rules.allow_future_times</source>
                 <target>Erlaube Zeiteinträge in der Zukunft</target>
             </trans-unit>
+            <trans-unit id="label.timesheet.rules.allow_overlapping_records">
+                <source>label.timesheet.rules.allow_overlapping_records</source>
+                <target>Erlaube überlappende Zeiteinträge</target>
+            </trans-unit>
             <trans-unit id="label.timesheet.active_entries.hard_limit">
                 <source>label.timesheet.active_entries.hard_limit</source>
                 <target>Erlaubte Anzahl an gleichzeitig laufenden Zeiteinträgen</target>

translations/system-configuration.en.xlf

@@ -66,6 +66,10 @@
                 <source>label.timesheet.rules.allow_future_times</source>
                 <target>Allow time entries in the future</target>
             </trans-unit>
+            <trans-unit id="label.timesheet.rules.allow_overlapping_records">
+                <source>label.timesheet.rules.allow_overlapping_records</source>
+                <target>Allow overlapping time entries</target>
+            </trans-unit>
             <trans-unit id="label.timesheet.active_entries.hard_limit">
                 <source>label.timesheet.active_entries.hard_limit</source>
                 <target>Permitted number of simultaneously running time entries</target>