diff --git a/src/API/TeamController.php b/src/API/TeamController.php index 1b9bb9e9..ad35fd4c 100644 --- a/src/API/TeamController.php +++ b/src/API/TeamController.php @@ -15,7 +15,6 @@ use App\Entity\Activity; use App\Entity\Customer; use App\Entity\Project; use App\Entity\Team; -use App\Entity\TeamMember; use App\Entity\User; use App\Form\API\TeamApiEditForm; use App\Repository\ActivityRepository; @@ -201,7 +200,7 @@ final class TeamController extends BaseApiController * Update an existing team * * @SWG\Patch( - * description="Update an existing team, you can pass all or just a subset of all attributes (passing users will replace all existing ones)", + * description="Update an existing team, you can pass all or just a subset of all attributes (passing members will replace all existing ones)", * @SWG\Response( * response=200, * description="Returns the updated team", @@ -235,11 +234,12 @@ final class TeamController extends BaseApiController throw new NotFoundException(); } - // cache the current memberlist - /** @var TeamMember[] $originalMembers */ - $originalMembers = []; - foreach ($team->getMembers() as $member) { - $originalMembers[] = $member; + if ($request->request->has('members')) { + foreach ($team->getMembers() as $member) { + $team->removeMember($member); + $this->repository->removeTeamMember($member); + } + $this->repository->saveTeam($team); } $form = $this->createForm(TeamApiEditForm::class, $team); @@ -254,14 +254,6 @@ final class TeamController extends BaseApiController return $this->viewHandler->handle($view); } - // and now remove the ones, which are not in the list any longer - foreach ($originalMembers as $member) { - if (!$team->hasMember($member)) { - $member->getUser()->removeMembership($member); - $this->repository->removeTeamMember($member); - } - } - $this->repository->saveTeam($team); $view = new View($team, Response::HTTP_OK); diff --git a/src/Controller/ProfileController.php b/src/Controller/ProfileController.php index 8cf17ce5..db6e2d61 100644 --- a/src/Controller/ProfileController.php +++ b/src/Controller/ProfileController.php @@ -9,7 +9,6 @@ namespace App\Controller; -use App\Entity\TeamMember; use App\Entity\User; use App\Entity\UserPreference; use App\Event\PrepareUserEvent; @@ -19,7 +18,9 @@ use App\Form\UserPasswordType; use App\Form\UserPreferencesForm; use App\Form\UserRolesType; use App\Form\UserTeamsType; +use App\Repository\TeamRepository; use App\Repository\TimesheetRepository; +use App\Repository\UserRepository; use App\Timesheet\TimesheetStatisticService; use App\User\UserService; use Doctrine\Common\Collections\ArrayCollection; @@ -78,15 +79,13 @@ final class ProfileController extends AbstractController * @Route(path="/{username}/edit", name="user_profile_edit", methods={"GET", "POST"}) * @Security("is_granted('edit', profile)") */ - public function editAction(User $profile, Request $request): Response + public function editAction(User $profile, Request $request, UserRepository $userRepository): Response { $form = $this->createEditForm($profile); $form->handleRequest($request); if ($form->isSubmitted() && $form->isValid()) { - $entityManager = $this->getDoctrine()->getManager(); - $entityManager->persist($profile); - $entityManager->flush(); + $userRepository->saveUser($profile); $this->flashSuccess('action.update.success'); @@ -152,7 +151,7 @@ final class ProfileController extends AbstractController * @Route(path="/{username}/roles", name="user_profile_roles", methods={"GET", "POST"}) * @Security("is_granted('roles', profile)") */ - public function rolesAction(User $profile, Request $request): Response + public function rolesAction(User $profile, Request $request, UserRepository $userRepository): Response { $isSuperAdmin = $profile->isSuperAdmin(); @@ -166,9 +165,7 @@ final class ProfileController extends AbstractController $profile->setSuperAdmin(true); } - $entityManager = $this->getDoctrine()->getManager(); - $entityManager->persist($profile); - $entityManager->flush(); + $userRepository->saveUser($profile); $this->flashSuccess('action.update.success'); @@ -186,7 +183,7 @@ final class ProfileController extends AbstractController * @Route(path="/{username}/teams", name="user_profile_teams", methods={"GET", "POST"}) * @Security("is_granted('teams', profile)") */ - public function teamsAction(User $profile, Request $request, UserService $service): Response + public function teamsAction(User $profile, Request $request, UserRepository $userRepository, TeamRepository $teamRepository): Response { $originalMembers = new ArrayCollection(); foreach ($profile->getMemberships() as $member) { @@ -197,18 +194,15 @@ final class ProfileController extends AbstractController $form->handleRequest($request); if ($form->isSubmitted() && $form->isValid()) { - $entityManager = $this->getDoctrine()->getManager(); - - /** @var TeamMember $member */ foreach ($originalMembers as $member) { if (!$profile->hasMembership($member)) { - $member->getTeam()->removeMember($member); - $entityManager->remove($profile); + $member->setTeam(null); + $member->setUser(null); + $teamRepository->removeTeamMember($member); } } - $entityManager->persist($profile); - $entityManager->flush(); + $userRepository->saveUser($profile); $this->flashSuccess('action.update.success'); diff --git a/src/Entity/Team.php b/src/Entity/Team.php index 74d9310b..e88b466c 100644 --- a/src/Entity/Team.php +++ b/src/Entity/Team.php @@ -135,22 +135,11 @@ class Team } /** - * Indexed by ID to use it within collection type forms. - * - * @return TeamMember[] + * @return Collection */ - public function getMembers(): iterable + public function getMembers(): Collection { - $all = []; - foreach ($this->members as $member) { - if ($member->getId() === null) { - $all[] = $member; - } else { - $all[$member->getId()] = $member; - } - } - - return $all; + return $this->members; } public function addMember(TeamMember $member): void @@ -167,12 +156,12 @@ class Team throw new \InvalidArgumentException('Cannot set foreign team membership'); } - // when using the API an invalid user id does not trigger the validation first, but after calling this method :-( + // when using the API an invalid User ID triggers the validation too late if ($member->getUser() === null) { return; } - if (null !== ($existing = $this->findMember($member))) { + if (null !== $this->findMemberByUser($member->getUser())) { return; } @@ -185,22 +174,11 @@ class Team return $this->members->contains($member); } - private function findMember(TeamMember $member): ?TeamMember - { - foreach ($this->members as $oldMember) { - if ($oldMember->getUser() === $member->getUser() && $oldMember->getTeam() === $member->getTeam()) { - return $oldMember; - } - } - - return null; - } - private function findMemberByUser(User $user): ?TeamMember { - foreach ($this->members as $oldMember) { - if ($oldMember->getUser() === $user) { - return $oldMember; + foreach ($this->members as $member) { + if ($member->getUser() === $user) { + return $member; } } @@ -209,12 +187,14 @@ class Team public function removeMember(TeamMember $member): void { - if (null === ($existingMember = $this->findMember($member))) { + if (!$this->members->contains($member)) { return; } - $this->members->removeElement($existingMember); - $existingMember->getUser()->removeMembership($existingMember); + $this->members->removeElement($member); + $member->getUser()->removeMembership($member); + $member->setTeam(null); + $member->setUser(null); } /** diff --git a/src/Entity/TeamMember.php b/src/Entity/TeamMember.php index 67c3d742..4ad4aa04 100644 --- a/src/Entity/TeamMember.php +++ b/src/Entity/TeamMember.php @@ -107,7 +107,7 @@ class TeamMember public function __clone() { if ($this->id !== null) { - $id = null; + $this->id = null; } } } diff --git a/src/Entity/User.php b/src/Entity/User.php index a0447b5f..57344bf1 100644 --- a/src/Entity/User.php +++ b/src/Entity/User.php @@ -579,12 +579,12 @@ class User implements UserInterface, EquatableInterface, \Serializable throw new \InvalidArgumentException('Cannot set foreign user membership'); } - // when using the API an invalid user id does not trigger the validation first, but after calling this method :-( + // when using the API an invalid Team ID triggers the validation too late if ($member->getTeam() === null) { return; } - if (null !== ($existing = $this->findMember($member))) { + if (null !== $this->findMemberByTeam($member->getTeam())) { return; } @@ -592,33 +592,35 @@ class User implements UserInterface, EquatableInterface, \Serializable $member->getTeam()->addMember($member); } + private function findMemberByTeam(Team $team): ?TeamMember + { + foreach ($this->memberships as $member) { + if ($member->getTeam() === $team) { + return $member; + } + } + + return null; + } + public function removeMembership(TeamMember $member): void { - if (null === ($member = $this->findMember($member))) { + if (!$this->memberships->contains($member)) { return; } $this->memberships->removeElement($member); - $member->getUser()->removeMembership($member); + $member->getTeam()->removeMember($member); + $member->setUser(null); + $member->setTeam(null); } /** - * Indexed by ID to use it within collection type forms. - * - * @return TeamMember[] + * @return Collection */ - public function getMemberships(): iterable + public function getMemberships(): Collection { - $all = []; - foreach ($this->memberships as $member) { - if ($member->getId() === null) { - $all[] = $member; - } else { - $all[$member->getId()] = $member; - } - } - - return $all; + return $this->memberships; } public function hasMembership(TeamMember $member): bool @@ -626,17 +628,6 @@ class User implements UserInterface, EquatableInterface, \Serializable return $this->memberships->contains($member); } - private function findMember(TeamMember $member): ?TeamMember - { - foreach ($this->memberships as $oldMember) { - if ($oldMember->getUser() === $member->getUser() && $oldMember->getTeam() === $member->getTeam()) { - return $oldMember; - } - } - - return null; - } - /** * Checks if the user is member of any team. * @@ -750,10 +741,8 @@ class User implements UserInterface, EquatableInterface, \Serializable public function isTeamleadOf(Team $team): bool { - foreach ($this->memberships as $membership) { - if ($membership->getTeam() === $team) { - return $membership->isTeamlead(); - } + if (null !== ($member = $this->findMemberByTeam($team))) { + return $member->isTeamlead(); } return false; @@ -830,10 +819,7 @@ class User implements UserInterface, EquatableInterface, \Serializable return $this->auth === null || $this->auth === self::AUTH_INTERNAL; } - /** - * {@inheritdoc} - */ - public function addRole($role) + public function addRole(string $role) { $role = strtoupper($role); if ($role === static::DEFAULT_ROLE) { diff --git a/tests/API/TeamControllerTest.php b/tests/API/TeamControllerTest.php index ed6b1a45..4af59300 100644 --- a/tests/API/TeamControllerTest.php +++ b/tests/API/TeamControllerTest.php @@ -170,6 +170,13 @@ class TeamControllerTest extends APIControllerBaseTest $this->assertNotEmpty($result['id']); self::assertCount(3, $result['users']); + $this->request($client, '/api/teams/' . $updateId); + $result = json_decode($client->getResponse()->getContent(), true); + + $this->assertIsArray($result); + self::assertApiResponseTypeStructure('TeamEntity', $result); + self::assertCount(3, $result['users']); + self::assertFalse($result['members'][1]['teamlead']); self::assertEquals(1, $result['members'][1]['user']['id']); self::assertEquals('clara_customer', $result['members'][1]['user']['username']); diff --git a/tests/Entity/TeamTest.php b/tests/Entity/TeamTest.php index 702a59a7..9f86c94f 100644 --- a/tests/Entity/TeamTest.php +++ b/tests/Entity/TeamTest.php @@ -92,6 +92,9 @@ class TeamTest extends TestCase self::assertCount(0, $sut->getMembers()); self::assertFalse($sut->isTeamlead($user)); + $member = new TeamMember(); + $member->setUser($user); + $sut->addMember($member); $member->setTeamlead(true); self::assertTrue($sut->isTeamlead($user)); diff --git a/tests/Entity/UserTest.php b/tests/Entity/UserTest.php index d4bb5c52..dd7fc841 100644 --- a/tests/Entity/UserTest.php +++ b/tests/Entity/UserTest.php @@ -449,6 +449,9 @@ class UserTest extends TestCase self::assertFalse($sut->isTeamleadOf($team)); + $member = new TeamMember(); + $member->setTeam($team); + $sut->addMembership($member); self::assertCount(1, $sut->getMemberships()); self::assertFalse($sut->isTeamleadOf($team));