From 130e6ac05718c1ea1a0b7e8a6e4be8792f92e89b Mon Sep 17 00:00:00 2001 From: Kevin Papst Date: Sun, 26 Jul 2020 13:13:06 +0200 Subject: [PATCH] handle hidden users in teams (#1841) * allow to add disabled users to team * allow to include disabled users in user-select * include disabled users when editing team members ONLY if they are already part of team --- src/API/TeamController.php | 4 --- src/Form/TeamEditForm.php | 5 +++ src/Form/Type/UserType.php | 16 +++++++++ src/Repository/Query/BaseFormTypeQuery.php | 11 +++++++ src/Repository/Query/UserFormTypeQuery.php | 33 ++++++++++++++++++- src/Repository/UserRepository.php | 18 ++++++++-- tests/API/TeamControllerTest.php | 6 ---- .../Query/BaseFormTypeQueryTest.php | 9 +++-- .../Query/UserFormTypeQueryTest.php | 12 +++++++ 9 files changed, 99 insertions(+), 15 deletions(-) diff --git a/src/API/TeamController.php b/src/API/TeamController.php index f075e98b..5c2922b5 100644 --- a/src/API/TeamController.php +++ b/src/API/TeamController.php @@ -305,10 +305,6 @@ final class TeamController extends BaseApiController throw new NotFoundException('User not found'); } - if (!$user->isEnabled()) { - throw new BadRequestHttpException('Cannot add disabled user to team'); - } - if ($user->isInTeam($team)) { throw new BadRequestHttpException('User is already member of the team'); } diff --git a/src/Form/TeamEditForm.php b/src/Form/TeamEditForm.php index 476fe968..8308bfe4 100644 --- a/src/Form/TeamEditForm.php +++ b/src/Form/TeamEditForm.php @@ -23,6 +23,9 @@ class TeamEditForm extends AbstractType */ public function buildForm(FormBuilderInterface $builder, array $options) { + /** @var Team|null $team */ + $team = $options['data'] ?? null; + $builder ->add('name', TextType::class, [ 'label' => 'label.name', @@ -55,6 +58,8 @@ class TeamEditForm extends AbstractType 'title' => 'Team member', 'description' => 'Array of team member IDs', ], + // make sure that disabled users show up in the result list + 'include_users' => (null !== $team && $team->getUsers()->count() > 0 ? $team->getUsers()->toArray() : []) ]) ; } diff --git a/src/Form/Type/UserType.php b/src/Form/Type/UserType.php index bc1c3977..90597805 100644 --- a/src/Form/Type/UserType.php +++ b/src/Form/Type/UserType.php @@ -11,6 +11,7 @@ namespace App\Form\Type; use App\Entity\User; use App\Repository\Query\UserFormTypeQuery; +use App\Repository\Query\VisibilityInterface; use App\Repository\UserRepository; use Symfony\Bridge\Doctrine\Form\Type\EntityType; use Symfony\Component\Form\AbstractType; @@ -34,6 +35,13 @@ class UserType extends AbstractType return $user->getDisplayName(); }, 'choice_translation_domain' => false, + // whether disabled users should be included in the result list + 'include_disabled' => false, + // an array of users, which will always be included in the result list + // why? if the base entity could include disabled users, which should not be hidden in/removed from the list + // eg. when editing a team that has disabled users, these users would be removed silently + // see https://github.com/kevinpapst/kimai2/pull/1841 + 'include_users' => [], 'documentation' => [ 'type' => 'integer', 'description' => 'User ID', @@ -45,6 +53,14 @@ class UserType extends AbstractType $query = new UserFormTypeQuery(); $query->setUser($options['user']); + if ($options['include_disabled'] === true) { + $query->setVisibility(VisibilityInterface::SHOW_BOTH); + } + + if (!empty($options['include_users'])) { + $query->setUsersAlwaysIncluded($options['include_users']); + } + return $repo->getQueryBuilderForFormType($query); }; }); diff --git a/src/Repository/Query/BaseFormTypeQuery.php b/src/Repository/Query/BaseFormTypeQuery.php index b15c7bb1..200e4ff4 100644 --- a/src/Repository/Query/BaseFormTypeQuery.php +++ b/src/Repository/Query/BaseFormTypeQuery.php @@ -247,6 +247,17 @@ abstract class BaseFormTypeQuery return $this; } + /** + * @param Team[] $teams + * @return self + */ + public function setTeams(array $teams): self + { + $this->teams = $teams; + + return $this; + } + /** * @return Team[] */ diff --git a/src/Repository/Query/UserFormTypeQuery.php b/src/Repository/Query/UserFormTypeQuery.php index fafb3c34..8c0e3441 100644 --- a/src/Repository/Query/UserFormTypeQuery.php +++ b/src/Repository/Query/UserFormTypeQuery.php @@ -9,9 +9,40 @@ namespace App\Repository\Query; +use App\Entity\User; + /** - * Can be used for pre-filling form types with the: UserRepository + * Can be used to pre-fill form types with: UserRepository::getQueryBuilderForFormType() */ final class UserFormTypeQuery extends BaseFormTypeQuery { + use VisibilityTrait; + + /** + * @var User[] + */ + private $includeUsers = []; + + /** + * Sets a list of users which must be included in the result always. + * + * @param array $users + * @return UserFormTypeQuery + */ + public function setUsersAlwaysIncluded(array $users): UserFormTypeQuery + { + $this->includeUsers = $users; + + return $this; + } + + /** + * Get the list of users which should always be included in the result. + * + * @return User[] + */ + public function getUsersAlwaysIncluded(): array + { + return $this->includeUsers; + } } diff --git a/src/Repository/UserRepository.php b/src/Repository/UserRepository.php index 1149e726..14f7fda0 100644 --- a/src/Repository/UserRepository.php +++ b/src/Repository/UserRepository.php @@ -141,8 +141,22 @@ class UserRepository extends EntityRepository implements UserLoaderInterface { $qb = $this->createQueryBuilder('u'); - $qb->andWhere($qb->expr()->eq('u.enabled', ':enabled')); - $qb->setParameter('enabled', true, \PDO::PARAM_BOOL); + $or = $qb->expr()->orX(); + + if ($query->isShowVisible()) { + $or->add($qb->expr()->eq('u.enabled', ':enabled')); + $qb->setParameter('enabled', true, \PDO::PARAM_BOOL); + } + + $includeAlways = $query->getUsersAlwaysIncluded(); + if (!empty($includeAlways)) { + $or->add($qb->expr()->in('u', ':users')); + $qb->setParameter('users', $includeAlways); + } + + if ($or->count() > 0) { + $qb->andWhere($or); + } $qb->orderBy('u.username', 'ASC'); diff --git a/tests/API/TeamControllerTest.php b/tests/API/TeamControllerTest.php index 7da5dd62..bb5c6883 100644 --- a/tests/API/TeamControllerTest.php +++ b/tests/API/TeamControllerTest.php @@ -249,12 +249,6 @@ class TeamControllerTest extends APIControllerBaseTest self::assertEquals(Response::HTTP_BAD_REQUEST, $client->getResponse()->getStatusCode()); $json = json_decode($client->getResponse()->getContent(), true); self::assertEquals('User is already member of the team', $json['message']); - - // cannot add disabled user - $this->request($client, '/api/teams/' . $result['id'] . '/members/3', 'POST'); - self::assertEquals(Response::HTTP_BAD_REQUEST, $client->getResponse()->getStatusCode()); - $json = json_decode($client->getResponse()->getContent(), true); - self::assertEquals('Cannot add disabled user to team', $json['message']); } public function testDeleteMemberAction() diff --git a/tests/Repository/Query/BaseFormTypeQueryTest.php b/tests/Repository/Query/BaseFormTypeQueryTest.php index 236a3a3c..05e6b02f 100644 --- a/tests/Repository/Query/BaseFormTypeQueryTest.php +++ b/tests/Repository/Query/BaseFormTypeQueryTest.php @@ -44,12 +44,17 @@ abstract class BaseFormTypeQueryTest extends TestCase self::assertEmpty($sut->getTeams()); self::assertInstanceOf(BaseFormTypeQuery::class, $sut->addTeam(new Team())); - self::assertEquals(1, \count($sut->getTeams())); + self::assertCount(1, $sut->getTeams()); $team = new Team(); self::assertInstanceOf(BaseFormTypeQuery::class, $sut->addTeam($team)); - self::assertEquals(1, \count($sut->getTeams())); + self::assertCount(1, $sut->getTeams()); self::assertSame($team, $sut->getTeams()[0]); + + self::assertInstanceOf(BaseFormTypeQuery::class, $sut->setTeams([])); + self::assertEmpty($sut->getTeams()); + self::assertInstanceOf(BaseFormTypeQuery::class, $sut->setTeams([new Team(), new Team()])); + self::assertCount(2, $sut->getTeams()); } protected function assertActivity(BaseFormTypeQuery $sut) diff --git a/tests/Repository/Query/UserFormTypeQueryTest.php b/tests/Repository/Query/UserFormTypeQueryTest.php index 6185f288..d69390a6 100644 --- a/tests/Repository/Query/UserFormTypeQueryTest.php +++ b/tests/Repository/Query/UserFormTypeQueryTest.php @@ -9,6 +9,7 @@ namespace App\Tests\Repository\Query; +use App\Entity\User; use App\Repository\Query\UserFormTypeQuery; /** @@ -23,4 +24,15 @@ class UserFormTypeQueryTest extends BaseFormTypeQueryTest $this->assertBaseQuery($sut); } + + public function testUsersAreAlwaysIncluded() + { + $sut = new UserFormTypeQuery(); + + $users = [(new User())->setUsername('foo'), new User(), new User()]; + + self::assertEquals([], $sut->getUsersAlwaysIncluded()); + self::assertInstanceOf(UserFormTypeQuery::class, $sut->setUsersAlwaysIncluded($users)); + self::assertSame($users, $sut->getUsersAlwaysIncluded()); + } }