From 27c60d9fc4430582543d34e7d0844596a515b615 Mon Sep 17 00:00:00 2001 From: Lukas Steinmann Date: Tue, 17 May 2022 11:12:08 +0200 Subject: [PATCH] Prevent bookings with same start / stop time (#3304) --- src/Configuration/SystemConfiguration.php | 5 + .../SystemConfigurationController.php | 4 + src/DependencyInjection/Configuration.php | 3 + .../Constraints/TimesheetZeroDuration.php | 26 +++++ .../TimesheetZeroDurationValidator.php | 56 +++++++++++ .../Configuration/SystemConfigurationTest.php | 1 + .../SystemConfigurationControllerTest.php | 5 +- tests/Controller/TimesheetControllerTest.php | 54 +++++++++++ .../DependencyInjection/AppExtensionTest.php | 1 + .../DependencyInjection/ConfigurationTest.php | 1 + .../TimesheetZeroDurationValidatorTest.php | 95 +++++++++++++++++++ translations/system-configuration.de.xlf | 4 + translations/system-configuration.en.xlf | 4 + translations/validators.de.xlf | 4 + translations/validators.en.xlf | 4 + 15 files changed, 266 insertions(+), 1 deletion(-) create mode 100644 src/Validator/Constraints/TimesheetZeroDuration.php create mode 100644 src/Validator/Constraints/TimesheetZeroDurationValidator.php create mode 100644 tests/Validator/Constraints/TimesheetZeroDurationValidatorTest.php diff --git a/src/Configuration/SystemConfiguration.php b/src/Configuration/SystemConfiguration.php index f7fc9002..8afbaf58 100644 --- a/src/Configuration/SystemConfiguration.php +++ b/src/Configuration/SystemConfiguration.php @@ -237,6 +237,11 @@ class SystemConfiguration implements SystemBundleConfiguration return (bool) $this->find('timesheet.rules.allow_future_times'); } + public function isTimesheetAllowZeroDuration(): bool + { + return (bool) $this->find('timesheet.rules.allow_zero_duration'); + } + public function isTimesheetAllowOverbookingBudget(): bool { return (bool) $this->find('timesheet.rules.allow_overbooking_budget'); diff --git a/src/Controller/SystemConfigurationController.php b/src/Controller/SystemConfigurationController.php index 377348d2..0d26fd07 100644 --- a/src/Controller/SystemConfigurationController.php +++ b/src/Controller/SystemConfigurationController.php @@ -338,6 +338,10 @@ final class SystemConfigurationController extends AbstractController ->setName('timesheet.rules.allow_future_times') ->setType(CheckboxType::class) ->setTranslationDomain('system-configuration'), + (new Configuration()) + ->setName('timesheet.rules.allow_zero_duration') + ->setType(CheckboxType::class) + ->setTranslationDomain('system-configuration'), (new Configuration()) ->setName('timesheet.rules.allow_overlapping_records') ->setType(CheckboxType::class) diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php index 86c3f500..a6e632ba 100644 --- a/src/DependencyInjection/Configuration.php +++ b/src/DependencyInjection/Configuration.php @@ -243,6 +243,9 @@ class Configuration implements ConfigurationInterface ->booleanNode('allow_future_times') ->defaultTrue() ->end() + ->booleanNode('allow_zero_duration') + ->defaultTrue() + ->end() ->booleanNode('allow_overbooking_budget') ->defaultTrue() ->end() diff --git a/src/Validator/Constraints/TimesheetZeroDuration.php b/src/Validator/Constraints/TimesheetZeroDuration.php new file mode 100644 index 00000000..c314fbdc --- /dev/null +++ b/src/Validator/Constraints/TimesheetZeroDuration.php @@ -0,0 +1,26 @@ + 'Duration cannot be zero.', + ]; + + public $message = 'Duration cannot be zero.'; + + public function getTargets() + { + return self::CLASS_CONSTRAINT; + } +} diff --git a/src/Validator/Constraints/TimesheetZeroDurationValidator.php b/src/Validator/Constraints/TimesheetZeroDurationValidator.php new file mode 100644 index 00000000..6d7a8c40 --- /dev/null +++ b/src/Validator/Constraints/TimesheetZeroDurationValidator.php @@ -0,0 +1,56 @@ +configuration = $configuration; + } + + /** + * @param TimesheetEntity $timesheet + * @param Constraint $constraint + */ + public function validate($timesheet, Constraint $constraint) + { + if (!($constraint instanceof TimesheetZeroDuration)) { + throw new UnexpectedTypeException($constraint, TimesheetZeroDuration::class); + } + + if (!\is_object($timesheet) || !($timesheet instanceof TimesheetEntity)) { + throw new UnexpectedTypeException($timesheet, TimesheetEntity::class); + } + + if ($this->configuration->isTimesheetAllowZeroDuration()) { + return; + } + + if ($timesheet->getDuration() == 0) { + $this->context->buildViolation($constraint->message) + ->atPath('duration') + ->setTranslationDomain('validators') + ->setCode(TimesheetZeroDuration::ZERO_DURATION_ERROR) + ->addViolation(); + } + } +} diff --git a/tests/Configuration/SystemConfigurationTest.php b/tests/Configuration/SystemConfigurationTest.php index b81bf010..986f1fcc 100644 --- a/tests/Configuration/SystemConfigurationTest.php +++ b/tests/Configuration/SystemConfigurationTest.php @@ -37,6 +37,7 @@ class SystemConfigurationTest extends TestCase 'timesheet' => [ 'rules' => [ 'allow_future_times' => false, + 'allow_zero_duration' => true, 'lockdown_period_start' => null, 'lockdown_period_end' => null, 'lockdown_grace_period' => null, diff --git a/tests/Controller/SystemConfigurationControllerTest.php b/tests/Controller/SystemConfigurationControllerTest.php index 7aeaac0a..418005af 100644 --- a/tests/Controller/SystemConfigurationControllerTest.php +++ b/tests/Controller/SystemConfigurationControllerTest.php @@ -93,6 +93,7 @@ class SystemConfigurationControllerTest extends ControllerBaseTest $configService = static::$kernel->getContainer()->get(SystemConfiguration::class); $this->assertEquals('default', $configService->find('timesheet.mode')); $this->assertTrue($configService->find('timesheet.rules.allow_future_times')); + $this->assertTrue($configService->find('timesheet.rules.allow_zero_duration')); $this->assertEquals(1, $configService->find('timesheet.active_entries.hard_limit')); $form = $client->getCrawler()->filter('form[name=system_configuration_form_timesheet]')->form(); @@ -102,6 +103,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_zero_duration', 'value' => true], ['name' => 'timesheet.rules.allow_overlapping_records', 'value' => false], ['name' => 'timesheet.rules.allow_overbooking_budget', 'value' => false], ['name' => 'timesheet.active_entries.hard_limit', 'value' => 99], @@ -168,6 +170,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_zero_duration', 'value' => 1], ['name' => 'timesheet.rules.allow_overlapping_records', 'value' => 1], ['name' => 'timesheet.rules.allow_overbooking_budget', 'value' => 1], ['name' => 'timesheet.active_entries.hard_limit', 'value' => -1], @@ -176,7 +179,7 @@ class SystemConfigurationControllerTest extends ControllerBaseTest ], [ '#system_configuration_form_timesheet_configuration_0_value', // mode - '#system_configuration_form_timesheet_configuration_5_value', // hard_limit + '#system_configuration_form_timesheet_configuration_6_value', // hard_limit ], true ); diff --git a/tests/Controller/TimesheetControllerTest.php b/tests/Controller/TimesheetControllerTest.php index 57415b0f..8d0b52ae 100644 --- a/tests/Controller/TimesheetControllerTest.php +++ b/tests/Controller/TimesheetControllerTest.php @@ -370,6 +370,7 @@ class TimesheetControllerTest extends ControllerBaseTest ['name' => 'timesheet.mode', 'value' => 'default'], ['name' => 'timesheet.active_entries.default_begin', 'value' => '08:00'], ['name' => 'timesheet.rules.allow_future_times', 'value' => true], + ['name' => 'timesheet.rules.allow_zero_duration', 'value' => true], ['name' => 'timesheet.rules.allow_overlapping_records', 'value' => false], ['name' => 'timesheet.rules.allow_overbooking_budget', 'value' => true], ['name' => 'timesheet.active_entries.hard_limit', 'value' => 1], @@ -458,6 +459,59 @@ class TimesheetControllerTest extends ControllerBaseTest ); } + public function testCreateActionWithEmptyDuration() + { + $client = $this->getClientForAuthenticatedUser(User::ROLE_SUPER_ADMIN); + + $fixture = new ActivityFixtures(); + $fixture->setAmount(1); + $fixture->setIsGlobal(true); + $fixture->setIsVisible(true); + $fixture->setCallback(function (Activity $activity) { + $activity->setBudget(1000); + $activity->setTimeBudget(3600); + }); + $activities = $this->importFixture($fixture); + /** @var Activity $activity */ + $activity = $activities[0]; + + $fixture = new TimesheetFixtures(); + $fixture->setAmount(1); + $fixture->setActivities([$activity]); + $fixture->setUser($this->getUserByRole(User::ROLE_USER)); + $timesheets = $this->importFixture($fixture); + $id = $timesheets[0]->getId(); + + $this->request($client, '/timesheet/' . $id . '/edit'); + + $response = $client->getResponse(); + $this->assertTrue($response->isSuccessful()); + + /** @var ConfigurationRepository $repository */ + $repository = $this->getEntityManager()->getRepository(Configuration::class); + $config = new Configuration(); + $config->setName('timesheet.rules.allow_zero_duration'); + $config->setValue(false); + $repository->saveConfiguration($config); + + $this->assertHasValidationError( + $client, + '/timesheet/' . $id . '/edit', + 'form[name=timesheet_edit_form]', + [ + 'timesheet_edit_form' => [ + 'hourlyRate' => 100, + 'begin' => '2020-02-18 01:00', + 'end' => '2020-02-18 01:00', + 'duration' => '00:00', + 'project' => 1, + 'activity' => $activity->getId(), + ] + ], + ['#timesheet_edit_form_duration'] + ); + } + public function testCreateActionWithBeginAndEndAndTagValues() { $client = $this->getClientForAuthenticatedUser(User::ROLE_ADMIN); diff --git a/tests/DependencyInjection/AppExtensionTest.php b/tests/DependencyInjection/AppExtensionTest.php index 81b5f749..94b9e0b6 100644 --- a/tests/DependencyInjection/AppExtensionTest.php +++ b/tests/DependencyInjection/AppExtensionTest.php @@ -202,6 +202,7 @@ class AppExtensionTest extends TestCase ], 'rules' => [ 'allow_future_times' => true, + 'allow_zero_duration' => true, 'allow_overlapping_records' => true, 'lockdown_period_start' => null, 'lockdown_period_end' => null, diff --git a/tests/DependencyInjection/ConfigurationTest.php b/tests/DependencyInjection/ConfigurationTest.php index fbd387b8..eda2a615 100644 --- a/tests/DependencyInjection/ConfigurationTest.php +++ b/tests/DependencyInjection/ConfigurationTest.php @@ -296,6 +296,7 @@ class ConfigurationTest extends TestCase ], 'rules' => [ 'allow_future_times' => true, + 'allow_zero_duration' => true, 'allow_overlapping_records' => true, 'lockdown_period_start' => null, 'lockdown_period_end' => null, diff --git a/tests/Validator/Constraints/TimesheetZeroDurationValidatorTest.php b/tests/Validator/Constraints/TimesheetZeroDurationValidatorTest.php new file mode 100644 index 00000000..0d8b63ae --- /dev/null +++ b/tests/Validator/Constraints/TimesheetZeroDurationValidatorTest.php @@ -0,0 +1,95 @@ +createMyValidator(false); + } + + protected function createMyValidator(bool $allowZeroDuration = false) + { + $loader = $this->createMock(ConfigLoaderInterface::class); + $config = new SystemConfiguration($loader, [ + 'timesheet' => [ + 'rules' => [ + 'allow_zero_duration' => $allowZeroDuration, + ], + ] + ]); + + return new TimesheetZeroDurationValidator($config); + } + + public function testConstraintIsInvalid() + { + $this->expectException(UnexpectedTypeException::class); + + $this->validator->validate(new Timesheet(), new NotBlank()); + } + + public function testInvalidValueThrowsException() + { + $this->expectException(UnexpectedTypeException::class); + + $this->validator->validate(new NotBlank(), new TimesheetZeroDuration(['message' => 'Duration cannot be zero.'])); + } + + private function prepareTimesheet() + { + // creates Timesheet with same begin and endtime + $begin = new \DateTime(); + $timesheet = new Timesheet(); + $timesheet->setBegin(clone $begin); + $timesheet->setEnd(clone $begin); + $timesheet->setDuration(0); + + return $timesheet; + } + + public function testZeroDurationIsDisallowed() + { + $timesheet = $this->prepareTimesheet(); + + $this->validator->validate($timesheet, new TimesheetZeroDuration(['message' => 'Duration cannot be zero.'])); + + $this->buildViolation('Duration cannot be zero.') + ->atPath('property.path.duration') + ->setCode(TimesheetZeroDuration::ZERO_DURATION_ERROR) + ->assertRaised(); + } + + public function testZeroDurationIsAllowed() + { + $this->validator = $this->createMyValidator(true); + $this->validator->initialize($this->context); + + $timesheet = $this->prepareTimesheet(); + + $this->validator->validate($timesheet, new TimesheetZeroDuration(['message' => 'Duration cannot be zero.'])); + + $this->assertNoViolation(); + } +} diff --git a/translations/system-configuration.de.xlf b/translations/system-configuration.de.xlf index c59b357e..f6d514f5 100644 --- a/translations/system-configuration.de.xlf +++ b/translations/system-configuration.de.xlf @@ -50,6 +50,10 @@ label.timesheet.rules.allow_future_times Erlaube Zeiteinträge in der Zukunft + + label.timesheet.rules.allow_zero_duration + Erlaube Zeiteinträge mit einer leeren Dauer + label.timesheet.rules.allow_overbooking_budget Überbuchung hinterlegter Budgets erlauben diff --git a/translations/system-configuration.en.xlf b/translations/system-configuration.en.xlf index 048196d6..6da63d24 100644 --- a/translations/system-configuration.en.xlf +++ b/translations/system-configuration.en.xlf @@ -50,6 +50,10 @@ label.timesheet.rules.allow_future_times Allow time entries in the future + + label.timesheet.rules.allow_zero_duration + Allow time entries with an empty duration + label.timesheet.rules.allow_overbooking_budget Allow overbooking of stored budgets diff --git a/translations/validators.de.xlf b/translations/validators.de.xlf index 8a30c432..9079d10a 100644 --- a/translations/validators.de.xlf +++ b/translations/validators.de.xlf @@ -38,6 +38,10 @@ The begin date cannot be in the future. Das Startdatum darf nicht in der Zukunft liegen. + + Duration cannot be zero. + Eine leere Dauer ist nicht erlaubt. + You must select at least one user or team. Sie müssen mindestens einen Benutzer oder ein Team auswählen. diff --git a/translations/validators.en.xlf b/translations/validators.en.xlf index 47d34848..331cad3d 100644 --- a/translations/validators.en.xlf +++ b/translations/validators.en.xlf @@ -38,6 +38,10 @@ The begin date cannot be in the future. The begin date cannot be in the future. + + Duration cannot be zero. + An empty duration is not allowed. + You must select at least one user or team. You must select at least one user or team.