Some random tests and fixes (#6000)

* added tests for form models
* apply form theme only to the own custom form type
* change route order for improves matching

config/routes.yaml

@@ -1,3 +1,28 @@
+home:
+    path: /
+    defaults:
+        _controller: Symfony\Bundle\FrameworkBundle\Controller\RedirectController::redirectAction
+        route: homepage
+        permanent: true
+
+# SF DefaultAuthenticationSuccessHandler uses /login as default target
+loginFallback:
+    path: /login
+    defaults:
+        _controller: Symfony\Bundle\FrameworkBundle\Controller\RedirectController::redirectAction
+        _locale: '%locale%'
+        route: login
+        permanent: true
+
+# SF LogoutListener uses /logout as default target
+logoutFallback:
+    path: /logout
+    defaults:
+        _controller: Symfony\Bundle\FrameworkBundle\Controller\RedirectController::redirectAction
+        _locale: '%locale%'
+        route: logout
+        permanent: true
+
 controllers:
     resource: ../src/Controller/
     type: attribute
@@ -31,13 +56,6 @@ kernel:
     resource: ../src/Kernel.php
     type: attribute
 
-home:
-    path: /
-    defaults:
-        _controller: Symfony\Bundle\FrameworkBundle\Controller\RedirectController::redirectAction
-        route: homepage
-        permanent: true
-
 homeLocale:
     path: /{_locale}
     defaults:
@@ -46,24 +64,6 @@ homeLocale:
         route: homepage
         permanent: true
 
-# SF DefaultAuthenticationSuccessHandler uses /login as default target
-loginFallback:
-    path: /login
-    defaults:
-        _controller: Symfony\Bundle\FrameworkBundle\Controller\RedirectController::redirectAction
-        _locale: '%locale%'
-        route: login
-        permanent: true
-
-# SF LogoutListener uses /logout as default target
-logoutFallback:
-    path: /logout
-    defaults:
-        _controller: Symfony\Bundle\FrameworkBundle\Controller\RedirectController::redirectAction
-        _locale: '%locale%'
-        route: logout
-        permanent: true
-
 2fa_login:
     path: /{_locale}/auth/2fa
     defaults:

src/Form/Model/Configuration.php

@@ -156,8 +156,10 @@ final class Configuration
         return $this->formTheme;
     }
 
-    public function setFormTheme(string $formTheme): void
+    public function setFormTheme(string $formTheme): Configuration
     {
         $this->formTheme = $formTheme;
+
+        return $this;
     }
 }

src/Form/Model/MultiUserTimesheet.php

@@ -51,7 +51,7 @@ final class MultiUserTimesheet extends Timesheet
     public function removeUser(User $user): void
     {
         if ($this->users->contains($user)) {
-            $this->users->remove($user);
+            $this->users->removeElement($user);
         }
     }
 
@@ -71,7 +71,7 @@ final class MultiUserTimesheet extends Timesheet
     public function removeTeam(Team $team): void
     {
         if ($this->teams->contains($team)) {
-            $this->teams->remove($team);
+            $this->teams->removeElement($team);
         }
     }
 }

src/Form/Model/TotpActivation.php

@@ -15,7 +15,7 @@ final class TotpActivation
 {
     private ?string $code = null;
 
-    public function __construct(private User $user)
+    public function __construct(private readonly User $user)
     {
     }
 

templates/system-configuration/index.html.twig

@@ -39,7 +39,7 @@
                         {% block form_body %}
                             {% for pref in form.children.configuration %}
                                 {% if pref.vars.data.formTheme is not null %}
-                                    {% form_theme form pref.vars.data.formTheme %}
+                                    {% form_theme pref pref.vars.data.formTheme %}
                                 {% endif %}
                             {% endfor %}
                             {% for pref in form.children.configuration %}

templates/system-configuration/section.html.twig

@@ -13,7 +13,7 @@
             {% block form_body %}
                 {% for pref in form.children.configuration %}
                     {% if pref.vars.data.formTheme is not null %}
-                        {% form_theme form pref.vars.data.formTheme %}
+                        {% form_theme pref pref.vars.data.formTheme %}
                     {% endif %}
                 {% endfor %}
                 {% for pref in form.children.configuration %}

tests/Form/Model/ConfigurationTest.php

@@ -0,0 +1,125 @@
+<?php
+
+/*
+ * This file is part of the Kimai time-tracking app.
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace App\Tests\Form\Model;
+
+use App\Form\Model\Configuration;
+use App\Form\Type\YesNoType;
+use PHPUnit\Framework\Attributes\CoversClass;
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\TestCase;
+use Symfony\Component\Form\Extension\Core\Type\CheckboxType;
+use Symfony\Component\Validator\Constraints\NotBlank;
+
+#[CoversClass(Configuration::class)]
+class ConfigurationTest extends TestCase
+{
+    public function testDefaultValues(): void
+    {
+        $sut = new Configuration('foo');
+
+        self::assertSame('foo', $sut->getName());
+        self::assertNull($sut->getLabel());
+        self::assertSame('messages', $sut->getTranslationDomain());
+        self::assertNull($sut->getValue());
+        self::assertNull($sut->getType());
+        self::assertSame([], $sut->getOptions());
+        self::assertTrue($sut->isEnabled());
+        self::assertTrue($sut->isRequired());
+        self::assertNull($sut->getFormTheme());
+        self::assertSame([], $sut->getConstraints());
+    }
+
+    public function testFluentSetterAndGetter(): void
+    {
+        $sut = new Configuration('foo');
+        $constraints = [new NotBlank()];
+        $options = ['attr' => ['data-test' => 'value'], 'empty_data' => 0];
+
+        $result = $sut
+            ->setLabel('Foo label')
+            ->setTranslationDomain('admin')
+            ->setType('custom-type')
+            ->setValue(42.5)
+            ->setOptions($options)
+            ->setConstraints($constraints)
+            ->setEnabled(false)
+            ->setRequired(false)
+            ->setFormTheme('@MyBundle/form/test.html.twig');
+
+        self::assertSame($sut, $result);
+        self::assertSame('Foo label', $sut->getLabel());
+        self::assertSame('admin', $sut->getTranslationDomain());
+        self::assertSame('custom-type', $sut->getType());
+        self::assertSame(42.5, $sut->getValue());
+        self::assertSame($options, $sut->getOptions());
+        self::assertSame($constraints, $sut->getConstraints());
+        self::assertFalse($sut->isEnabled());
+        self::assertFalse($sut->isRequired());
+        self::assertSame('@MyBundle/form/test.html.twig', $sut->getFormTheme());
+    }
+
+    #[DataProvider('provideScalarValues')]
+    public function testSetValueKeepsOriginalTypeForNonBooleanField(string|int|null|bool|float $value): void
+    {
+        $sut = new Configuration('foo');
+
+        self::assertSame($sut, $sut->setValue($value));
+        self::assertSame($value, $sut->getValue());
+    }
+
+    /**
+     * @return iterable<string, array{0: string, 1: string|int|null|bool|float, 2: bool}>
+     */
+    public static function provideBooleanTypeValues(): iterable
+    {
+        yield 'checkbox true string' => [CheckboxType::class, '1', true];
+        yield 'checkbox zero string' => [CheckboxType::class, '0', false];
+        yield 'checkbox integer zero' => [CheckboxType::class, 0, false];
+        yield 'checkbox integer one' => [CheckboxType::class, 1, true];
+        yield 'checkbox null' => [CheckboxType::class, null, false];
+        yield 'checkbox empty string' => [CheckboxType::class, '', false];
+        yield 'yes no text value' => [YesNoType::class, 'yes', true];
+        yield 'yes no false bool' => [YesNoType::class, false, false];
+        yield 'yes no float' => [YesNoType::class, 3.14, true];
+    }
+
+    #[DataProvider('provideBooleanTypeValues')]
+    public function testSetValueCastsToBoolForBooleanTypes(string $type, string|int|null|bool|float $value, bool $expected): void
+    {
+        $sut = new Configuration('foo');
+
+        self::assertSame($sut, $sut->setType($type));
+        self::assertSame($sut, $sut->setValue($value));
+        self::assertSame($expected, $sut->getValue());
+    }
+
+    public function testChangingTypeAfterSettingValueDoesNotRetroactivelyCastValue(): void
+    {
+        $sut = new Configuration('foo');
+
+        $sut->setValue('1');
+        $sut->setType(CheckboxType::class);
+
+        self::assertSame('1', $sut->getValue());
+    }
+
+    /**
+     * @return iterable<string, array{0: string|int|null|bool|float}>
+     */
+    public static function provideScalarValues(): iterable
+    {
+        yield 'null' => [null];
+        yield 'string' => ['hello world'];
+        yield 'integer' => [123];
+        yield 'float' => [123.45];
+        yield 'true' => [true];
+        yield 'false' => [false];
+    }
+}

tests/Form/Model/MultiUserTimesheetTest.php

@@ -0,0 +1,122 @@
+<?php
+
+/*
+ * This file is part of the Kimai time-tracking app.
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace App\Tests\Form\Model;
+
+use App\Entity\Team;
+use App\Entity\Timesheet;
+use App\Entity\User;
+use App\Form\Model\MultiUserTimesheet;
+use PHPUnit\Framework\Attributes\CoversClass;
+use PHPUnit\Framework\TestCase;
+
+#[CoversClass(MultiUserTimesheet::class)]
+class MultiUserTimesheetTest extends TestCase
+{
+    public function testDefaultCollectionsAreEmpty(): void
+    {
+        $sut = new MultiUserTimesheet();
+
+        self::assertInstanceOf(Timesheet::class, $sut);
+        self::assertEmpty($sut->getTeams());
+        self::assertEmpty($sut->getUsers());
+    }
+
+    public function testAddAndRemoveUser(): void
+    {
+        $sut = new MultiUserTimesheet();
+        $user = $this->createUser('alpha');
+
+        $sut->addUser($user);
+
+        self::assertCount(1, $sut->getUsers());
+        self::assertSame($user, $sut->getUsers()->first());
+
+        $sut->removeUser($user);
+
+        self::assertCount(0, $sut->getUsers());
+    }
+
+    public function testRemovingUnknownUserDoesNothing(): void
+    {
+        $sut = new MultiUserTimesheet();
+        $user = $this->createUser('alpha');
+
+        $sut->removeUser($user);
+
+        self::assertCount(0, $sut->getUsers());
+    }
+
+    public function testRemovingUserOnlyRemovesOneDuplicateEntry(): void
+    {
+        $sut = new MultiUserTimesheet();
+        $user = $this->createUser('alpha');
+
+        $sut->addUser($user);
+        $sut->addUser($user);
+
+        self::assertCount(2, $sut->getUsers());
+
+        $sut->removeUser($user);
+
+        self::assertCount(1, $sut->getUsers());
+        self::assertSame($user, $sut->getUsers()->first());
+    }
+
+    public function testAddAndRemoveTeam(): void
+    {
+        $sut = new MultiUserTimesheet();
+        $team = new Team('Team Alpha');
+
+        $sut->addTeam($team);
+
+        self::assertCount(1, $sut->getTeams());
+        self::assertSame($team, $sut->getTeams()->first());
+
+        $sut->removeTeam($team);
+
+        self::assertCount(0, $sut->getTeams());
+    }
+
+    public function testRemovingUnknownTeamDoesNothing(): void
+    {
+        $sut = new MultiUserTimesheet();
+        $team = new Team('Team Alpha');
+
+        $sut->removeTeam($team);
+
+        self::assertCount(0, $sut->getTeams());
+    }
+
+    public function testRemovingTeamOnlyRemovesOneDuplicateEntry(): void
+    {
+        $sut = new MultiUserTimesheet();
+        $team = new Team('Team Alpha');
+
+        $sut->addTeam($team);
+        $sut->addTeam($team);
+
+        self::assertCount(2, $sut->getTeams());
+
+        $sut->removeTeam($team);
+
+        self::assertCount(1, $sut->getTeams());
+        self::assertSame($team, $sut->getTeams()->first());
+    }
+
+    private function createUser(string $username): User
+    {
+        $user = new User();
+        $user->setUsername($username);
+        $user->setAlias($username);
+        $user->setEmail($username . '@example.com');
+
+        return $user;
+    }
+}

tests/Form/Model/SystemConfigurationTest.php

@@ -14,7 +14,6 @@ use App\Form\Model\SystemConfiguration;
 use PHPUnit\Framework\Attributes\CoversClass;
 use PHPUnit\Framework\TestCase;
 
-#[CoversClass(Configuration::class)]
 #[CoversClass(SystemConfiguration::class)]
 class SystemConfigurationTest extends TestCase
 {

tests/Form/Model/TotpActivationTest.php

@@ -0,0 +1,34 @@
+<?php
+
+/*
+ * This file is part of the Kimai time-tracking app.
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace App\Tests\Form\Model;
+
+use App\Entity\User;
+use App\Form\Model\TotpActivation;
+use PHPUnit\Framework\Attributes\CoversClass;
+use PHPUnit\Framework\TestCase;
+
+#[CoversClass(TotpActivation::class)]
+class TotpActivationTest extends TestCase
+{
+    public function testDefaultValues(): void
+    {
+        $user = new User();
+        $sut = new TotpActivation($user);
+
+        self::assertSame($user, $sut->getUser());
+        self::assertNull($sut->getCode());
+        $sut->setCode('');
+        self::assertEquals('', $sut->getCode());
+        $sut->setCode('jztfztfjzfjhgfjhgfjhgfjtzfiuzgbljv');
+        self::assertEquals('jztfztfjzfjhgfjhgfjhgfjtzfiuzgbljv', $sut->getCode());
+        $sut->setCode(null);
+        self::assertNull($sut->getCode());
+    }
+}

tests/Form/Model/UserContractModelTest.php

@@ -0,0 +1,89 @@
+<?php
+
+/*
+ * This file is part of the Kimai time-tracking app.
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace App\Tests\Form\Model;
+
+use App\Entity\User;
+use App\Form\Model\UserContractModel;
+use PHPUnit\Framework\Attributes\CoversClass;
+use PHPUnit\Framework\TestCase;
+
+#[CoversClass(UserContractModel::class)]
+class UserContractModelTest extends TestCase
+{
+    public function testIssetAlwaysReturnsTrue(): void
+    {
+        $user = new User();
+        $sut = new UserContractModel($user);
+
+        self::assertTrue($sut->__isset('alias'));
+        self::assertTrue($sut->__isset('unknownPreference'));
+    }
+
+    public function testSetAndGetExistingUserPropertyUsesUserMethods(): void
+    {
+        $user = new User();
+        $sut = new UserContractModel($user);
+
+        $sut->__set('alias', 'contract-user');
+
+        self::assertSame('contract-user', $sut->__get('alias'));
+        self::assertSame('contract-user', $user->getAlias());
+    }
+
+    public function testSetAndGetExistingPreferenceBackedMethodUsesUserMethod(): void
+    {
+        $user = new User();
+        $sut = new UserContractModel($user);
+
+        $sut->__set('workContractMode', 'default');
+
+        self::assertSame('default', $sut->__get('workContractMode'));
+        self::assertSame('default', $user->getWorkContractMode());
+    }
+
+    public function testSetAndGetUnknownPropertyUsesPreferenceFallback(): void
+    {
+        $user = new User();
+        $sut = new UserContractModel($user);
+
+        $sut->__set('customContractField', 'weekly');
+
+        self::assertSame('weekly', $sut->__get('customContractField'));
+        self::assertSame('weekly', $user->getPreferenceValue('customContractField'));
+    }
+
+    public function testSetUnknownPropertyAllowsNullPreferenceValue(): void
+    {
+        $user = new User();
+        $sut = new UserContractModel($user);
+
+        $sut->__set('customContractField', null);
+
+        self::assertNull($sut->__get('customContractField'));
+        self::assertNull($user->getPreferenceValue('customContractField'));
+    }
+
+    public function testGetUnknownPropertyWithoutPreferenceReturnsNull(): void
+    {
+        $sut = new UserContractModel(new User());
+
+        self::assertNull($sut->__get('missingPreference'));
+    }
+
+    public function testSetUnknownPropertyRejectsNonScalarValues(): void
+    {
+        $sut = new UserContractModel(new User());
+
+        $this->expectException(\InvalidArgumentException::class);
+        $this->expectExceptionMessage('Invalid value passed');
+
+        $sut->__set('customContractField', ['invalid']);
+    }
+}