From 4533c994a3766a96a3765c20f8e02f757d8ed770 Mon Sep 17 00:00:00 2001 From: Kevin Papst Date: Fri, 23 Oct 2020 00:31:53 +0200 Subject: [PATCH] SQL and performance improvements (#2017) --- src/Controller/PermissionController.php | 3 +- src/Form/FormTrait.php | 16 ++- src/Repository/ActivityRepository.php | 92 +++++++------- src/Repository/CustomerRepository.php | 17 ++- src/Repository/InvoiceRepository.php | 22 ++-- src/Repository/Loader/ActivityIdLoader.php | 19 +-- src/Repository/ProjectRepository.php | 25 ++-- src/Repository/TagRepository.php | 35 ++++-- src/Repository/TimesheetRepository.php | 112 ++++++++++-------- src/Repository/UserRepository.php | 36 +++--- src/Security/CurrentUser.php | 31 ++--- src/Timesheet/UserDateTimeFactory.php | 2 +- tests/Mocks/Security/CurrentUserFactory.php | 9 +- .../Repository/Loader/ActivityLoaderTest.php | 3 +- 14 files changed, 216 insertions(+), 206 deletions(-) diff --git a/src/Controller/PermissionController.php b/src/Controller/PermissionController.php index 5ef777d3..bca4abaf 100644 --- a/src/Controller/PermissionController.php +++ b/src/Controller/PermissionController.php @@ -78,6 +78,7 @@ final class PermissionController extends AbstractController $role->setName($roleName); $this->roleRepository->saveRole($role); $existing[] = $roleName; + $all[] = $role; } } @@ -145,7 +146,7 @@ final class PermissionController extends AbstractController 'ROLE_TEAMLEAD' => null, 'ROLE_USER' => null, ]; - foreach ($this->roleRepository->findAll() as $role) { + foreach ($all as $role) { $roles[$role->getName()] = $role; } diff --git a/src/Form/FormTrait.php b/src/Form/FormTrait.php index d3d5f785..37b417fc 100644 --- a/src/Form/FormTrait.php +++ b/src/Form/FormTrait.php @@ -105,8 +105,11 @@ trait FormTrait $builder ->add('activity', ActivityType::class, [ 'placeholder' => '', - 'query_builder' => function (ActivityRepository $repo) use ($activity, $project) { - return $repo->getQueryBuilderForFormType(new ActivityFormTypeQuery($activity, $project)); + 'query_builder' => function (ActivityRepository $repo) use ($builder, $activity, $project) { + $query = new ActivityFormTypeQuery($activity, $project); + $query->setUser($builder->getOption('user')); + + return $repo->getQueryBuilderForFormType($query); }, ]) ; @@ -114,7 +117,7 @@ trait FormTrait // replaces the activity select after submission, to make sure only activities for the selected project are displayed $builder->addEventListener( FormEvents::PRE_SUBMIT, - function (FormEvent $event) use ($activity) { + function (FormEvent $event) use ($builder, $activity) { $data = $event->getData(); if (!isset($data['project']) || empty($data['project'])) { return; @@ -122,8 +125,11 @@ trait FormTrait $event->getForm()->add('activity', ActivityType::class, [ 'placeholder' => '', - 'query_builder' => function (ActivityRepository $repo) use ($data, $activity) { - return $repo->getQueryBuilderForFormType(new ActivityFormTypeQuery($activity, $data['project'])); + 'query_builder' => function (ActivityRepository $repo) use ($builder, $data, $activity) { + $query = new ActivityFormTypeQuery($activity, $data['project']); + $query->setUser($builder->getOption('user')); + + return $repo->getQueryBuilderForFormType($query); }, ]); } diff --git a/src/Repository/ActivityRepository.php b/src/Repository/ActivityRepository.php index 18e619bd..73173450 100644 --- a/src/Repository/ActivityRepository.php +++ b/src/Repository/ActivityRepository.php @@ -11,6 +11,7 @@ namespace App\Repository; use App\Entity\Activity; use App\Entity\Project; +use App\Entity\Team; use App\Entity\Timesheet; use App\Entity\User; use App\Model\ActivityStatistic; @@ -117,7 +118,7 @@ class ActivityRepository extends EntityRepository return $stats; } - private function addPermissionCriteria(QueryBuilder $qb, ?User $user = null, array $teams = []) + private function addPermissionCriteria(QueryBuilder $qb, ?User $user = null, array $teams = [], bool $globalsOnly = false) { // make sure that all queries without a user see all projects if (null === $user && empty($teams)) { @@ -133,37 +134,41 @@ class ActivityRepository extends EntityRepository $teams = array_merge($teams, $user->getTeams()->toArray()); } - $qb->leftJoin('a.teams', 'teams') - ->leftJoin('p.teams', 'p_teams') - ->leftJoin('c.teams', 'c_teams'); - if (empty($teams)) { - $qb->andWhere($qb->expr()->isNull('teams')); - $qb->andWhere($qb->expr()->isNull('p_teams')); - $qb->andWhere($qb->expr()->isNull('c_teams')); + $qb->andWhere('SIZE(a.teams) = 0'); + if (!$globalsOnly) { + $qb->andWhere('SIZE(p.teams) = 0'); + $qb->andWhere('SIZE(c.teams) = 0'); + } return; } $orActivity = $qb->expr()->orX( - $qb->expr()->isNull('teams'), + 'SIZE(a.teams) = 0', $qb->expr()->isMemberOf(':teams', 'a.teams') ); $qb->andWhere($orActivity); - $orProject = $qb->expr()->orX( - $qb->expr()->isNull('p_teams'), - $qb->expr()->isMemberOf(':teams', 'p.teams') - ); - $qb->andWhere($orProject); + if (!$globalsOnly) { + $orProject = $qb->expr()->orX( + 'SIZE(p.teams) = 0', + $qb->expr()->isMemberOf(':teams', 'p.teams') + ); + $qb->andWhere($orProject); - $orCustomer = $qb->expr()->orX( - $qb->expr()->isNull('c_teams'), - $qb->expr()->isMemberOf(':teams', 'c.teams') - ); - $qb->andWhere($orCustomer); + $orCustomer = $qb->expr()->orX( + 'SIZE(c.teams) = 0', + $qb->expr()->isMemberOf(':teams', 'c.teams') + ); + $qb->andWhere($orCustomer); + } - $qb->setParameter('teams', $teams); + $ids = array_values(array_unique(array_map(function (Team $team) { + return $team->getId(); + }, $teams))); + + $qb->setParameter('teams', $ids); } /** @@ -196,7 +201,7 @@ class ActivityRepository extends EntityRepository $where = $qb->expr()->andX(); - $where->add('a.visible = :visible'); + $where->add($qb->expr()->eq('a.visible', ':visible')); $qb->setParameter('visible', true, \PDO::PARAM_BOOL); if (!$query->isGlobalsOnly()) { @@ -208,19 +213,15 @@ class ActivityRepository extends EntityRepository $where->add( $qb->expr()->orX( - $qb->expr()->eq('c.visible', ':customer_visible'), - $qb->expr()->isNull('c.visible') - ) - ); - $where->add( - $qb->expr()->orX( - $qb->expr()->eq('p.visible', ':project_visible'), - $qb->expr()->isNull('p.visible') + $qb->expr()->isNull('a.project'), + $qb->expr()->andX( + $qb->expr()->eq('p.visible', ':is_visible'), + $qb->expr()->eq('c.visible', ':is_visible') + ) ) ); - $qb->setParameter('project_visible', true, \PDO::PARAM_BOOL); - $qb->setParameter('customer_visible', true, \PDO::PARAM_BOOL); + $qb->setParameter('is_visible', true, \PDO::PARAM_BOOL); } if ($query->isGlobalsOnly()) { @@ -228,8 +229,8 @@ class ActivityRepository extends EntityRepository } elseif ($query->hasProjects()) { $where->add( $qb->expr()->orX( - $qb->expr()->in('a.project', ':project'), - $qb->expr()->isNull('a.project') + $qb->expr()->isNull('a.project'), + $qb->expr()->in('a.project', ':project') ) ); $qb->setParameter('project', $query->getProjects()); @@ -240,6 +241,8 @@ class ActivityRepository extends EntityRepository $qb->setParameter('ignored', $query->getActivityToIgnore()); } + $this->addPermissionCriteria($qb, $query->getUser(), $query->getTeams(), $query->isGlobalsOnly()); + $or = $qb->expr()->orX(); // this must always be the last part before the or @@ -287,21 +290,21 @@ class ActivityRepository extends EntityRepository $where = $qb->expr()->andX(); if (!$query->isShowBoth()) { + $where->add($qb->expr()->eq('a.visible', ':visible')); + if (!$query->isGlobalsOnly()) { $where->add( $qb->expr()->orX( $qb->expr()->isNull('a.project'), $qb->expr()->andX( - $qb->expr()->eq('c.visible', ':is_visible'), - $qb->expr()->eq('p.visible', ':is_visible') + $qb->expr()->eq('p.visible', ':is_visible'), + $qb->expr()->eq('c.visible', ':is_visible') ) ) ); $qb->setParameter('is_visible', true, \PDO::PARAM_BOOL); } - $where->add($qb->expr()->eq('a.visible', ':visible')); - if ($query->isShowVisible()) { $qb->setParameter('visible', true, \PDO::PARAM_BOOL); } elseif ($query->isShowHidden()) { @@ -322,16 +325,16 @@ class ActivityRepository extends EntityRepository $where->add($orX); $qb->setParameter('project', $query->getProjects()); - } elseif (null !== $query->getCustomer()) { - $where->add('p.customer = :customer'); - $qb->setParameter('customer', $query->getCustomer()); + } elseif ($query->hasCustomers()) { + $where->add($qb->expr()->in('p.customer', ':customer')); + $qb->setParameter('customer', $query->getCustomers()); } if ($where->count() > 0) { $qb->andWhere($where); } - $this->addPermissionCriteria($qb, $query->getCurrentUser(), $query->getTeams()); + $this->addPermissionCriteria($qb, $query->getCurrentUser(), $query->getTeams(), $query->isGlobalsOnly()); if ($query->hasSearchTerm()) { $searchAnd = $qb->expr()->andX(); @@ -364,13 +367,6 @@ class ActivityRepository extends EntityRepository } } - // this will make sure, that we do not accidentally create results with multiple rows, - // which would result in a wrong LIMIT with paginated results - $qb->addGroupBy('a'); - - // the second group by is needed to satisfy SQL standard (ONLY_FULL_GROUP_BY) - $qb->addGroupBy($orderBy); - return $qb; } diff --git a/src/Repository/CustomerRepository.php b/src/Repository/CustomerRepository.php index f3e84b90..11408e3a 100644 --- a/src/Repository/CustomerRepository.php +++ b/src/Repository/CustomerRepository.php @@ -13,6 +13,7 @@ use App\Entity\Activity; use App\Entity\Customer; use App\Entity\CustomerComment; use App\Entity\Project; +use App\Entity\Team; use App\Entity\Timesheet; use App\Entity\User; use App\Model\CustomerStatistic; @@ -150,21 +151,23 @@ class CustomerRepository extends EntityRepository $teams = array_merge($teams, $user->getTeams()->toArray()); } - $qb->leftJoin('c.teams', 'teams'); - if (empty($teams)) { - $qb->andWhere($qb->expr()->isNull('teams')); + $qb->andWhere('SIZE(c.teams) = 0'); return; } $or = $qb->expr()->orX( - $qb->expr()->isNull('teams'), + 'SIZE(c.teams) = 0', $qb->expr()->isMemberOf(':teams', 'c.teams') ); $qb->andWhere($or); - $qb->setParameter('teams', $teams); + $ids = array_values(array_unique(array_map(function (Team $team) { + return $team->getId(); + }, $teams))); + + $qb->setParameter('teams', $ids); } /** @@ -270,10 +273,6 @@ class CustomerRepository extends EntityRepository } } - // this will make sure, that we do not accidentally create results with multiple rows, - // which would result in a wrong LIMIT with paginated results - $qb->addGroupBy('c'); - return $qb; } diff --git a/src/Repository/InvoiceRepository.php b/src/Repository/InvoiceRepository.php index f3b68172..b79d4c4b 100644 --- a/src/Repository/InvoiceRepository.php +++ b/src/Repository/InvoiceRepository.php @@ -11,6 +11,7 @@ namespace App\Repository; use App\Entity\Customer; use App\Entity\Invoice; +use App\Entity\Team; use App\Entity\User; use App\Repository\Loader\InvoiceLoader; use App\Repository\Paginator\LoaderPaginator; @@ -115,23 +116,25 @@ class InvoiceRepository extends EntityRepository $teams = array_merge($teams, $user->getTeams()->toArray()); } - $qb - ->leftJoin('i.customer', 'c') - ->leftJoin('c.teams', 'c_teams'); + $qb->leftJoin('i.customer', 'c'); if (empty($teams)) { - $qb->andWhere($qb->expr()->isNull('c_teams')); + $qb->andWhere('SIZE(c.teams) = 0'); return; } $orCustomer = $qb->expr()->orX( - $qb->expr()->isNull('c_teams'), + 'SIZE(c.teams) = 0', $qb->expr()->isMemberOf(':teams', 'c.teams') ); $qb->andWhere($orCustomer); - $qb->setParameter('teams', $teams); + $ids = array_values(array_unique(array_map(function (Team $team) { + return $team->getId(); + }, $teams))); + + $qb->setParameter('teams', $ids); } private function getQueryBuilderForQuery(InvoiceQuery $query): QueryBuilder @@ -154,13 +157,6 @@ class InvoiceRepository extends EntityRepository $this->addPermissionCriteria($qb, $query->getCurrentUser()); - // this will make sure, that we do not accidentally create results with multiple rows, - // which would result in a wrong LIMIT with paginated results - $qb->addGroupBy('i'); - - // the second group by is needed to satisfy SQL standard (ONLY_FULL_GROUP_BY) - $qb->addGroupBy($orderBy); - return $qb; } diff --git a/src/Repository/Loader/ActivityIdLoader.php b/src/Repository/Loader/ActivityIdLoader.php index 85d70315..00fa8736 100644 --- a/src/Repository/Loader/ActivityIdLoader.php +++ b/src/Repository/Loader/ActivityIdLoader.php @@ -56,6 +56,7 @@ final class ActivityIdLoader implements LoaderInterface ->getQuery() ->execute(); + // global activities don't have projects if (!empty($activities)) { $projectIds = array_map(function (Activity $activity) { if (null === $activity->getProject()) { @@ -93,15 +94,15 @@ final class ActivityIdLoader implements LoaderInterface ->andWhere($qb->expr()->in('a.id', $ids)) ->getQuery() ->execute(); - - $qb = $em->createQueryBuilder(); - $qb->select('PARTIAL a.{id}', 'teams', 'teamlead') - ->from(Activity::class, 'a') - ->leftJoin('a.teams', 'teams') - ->leftJoin('teams.teamlead', 'teamlead') - ->andWhere($qb->expr()->in('a.id', $ids)) - ->getQuery() - ->execute(); } + + $qb = $em->createQueryBuilder(); + $qb->select('PARTIAL a.{id}', 'teams', 'teamlead') + ->from(Activity::class, 'a') + ->leftJoin('a.teams', 'teams') + ->leftJoin('teams.teamlead', 'teamlead') + ->andWhere($qb->expr()->in('a.id', $ids)) + ->getQuery() + ->execute(); } } diff --git a/src/Repository/ProjectRepository.php b/src/Repository/ProjectRepository.php index 51c6ccaf..c68d00a8 100644 --- a/src/Repository/ProjectRepository.php +++ b/src/Repository/ProjectRepository.php @@ -12,6 +12,7 @@ namespace App\Repository; use App\Entity\Activity; use App\Entity\Project; use App\Entity\ProjectComment; +use App\Entity\Team; use App\Entity\Timesheet; use App\Entity\User; use App\Model\ProjectStatistic; @@ -142,29 +143,30 @@ class ProjectRepository extends EntityRepository $teams = array_merge($teams, $user->getTeams()->toArray()); } - $qb->leftJoin('p.teams', 'teams') - ->leftJoin('c.teams', 'c_teams'); - if (empty($teams)) { - $qb->andWhere($qb->expr()->isNull('c_teams')); - $qb->andWhere($qb->expr()->isNull('teams')); + $qb->andWhere('SIZE(c.teams) = 0'); + $qb->andWhere('SIZE(p.teams) = 0'); return; } $orProject = $qb->expr()->orX( - $qb->expr()->isNull('teams'), + 'SIZE(p.teams) = 0', $qb->expr()->isMemberOf(':teams', 'p.teams') ); $qb->andWhere($orProject); $orCustomer = $qb->expr()->orX( - $qb->expr()->isNull('c_teams'), + 'SIZE(c.teams) = 0', $qb->expr()->isMemberOf(':teams', 'c.teams') ); $qb->andWhere($orCustomer); - $qb->setParameter('teams', $teams); + $ids = array_values(array_unique(array_map(function (Team $team) { + return $team->getId(); + }, $teams))); + + $qb->setParameter('teams', $ids); } /** @@ -369,13 +371,6 @@ class ProjectRepository extends EntityRepository } } - // this will make sure, that we do not accidentally create results with multiple rows, - // which would result in a wrong LIMIT with paginated results - $qb->addGroupBy('p'); - - // the second group by is needed to satisfy SQL standard (ONLY_FULL_GROUP_BY) - $qb->addGroupBy($orderBy); - return $qb; } diff --git a/src/Repository/TagRepository.php b/src/Repository/TagRepository.php index a6955bc7..085f0002 100644 --- a/src/Repository/TagRepository.php +++ b/src/Repository/TagRepository.php @@ -10,12 +10,12 @@ namespace App\Repository; use App\Entity\Tag; +use App\Repository\Paginator\QueryBuilderPaginator; use App\Repository\Query\TagFormTypeQuery; use App\Repository\Query\TagQuery; use Doctrine\ORM\EntityRepository; use Doctrine\ORM\ORMException; use Doctrine\ORM\QueryBuilder; -use Pagerfanta\Adapter\DoctrineORMAdapter; use Pagerfanta\Pagerfanta; /** @@ -107,15 +107,32 @@ class TagRepository extends EntityRepository * @return Pagerfanta */ public function getTagCount(TagQuery $query) + { + $qb = $this->getQueryBuilderForQuery($query); + $qb + ->resetDQLPart('select') + ->resetDQLPart('orderBy') + ->select($qb->expr()->count('tag.name')) + ; + $counter = (int) $qb->getQuery()->getSingleScalarResult(); + + $qb = $this->getQueryBuilderForQuery($query); + + $paginator = new QueryBuilderPaginator($qb, $counter); + + $pagerfanta = new Pagerfanta($paginator); + $pagerfanta->setMaxPerPage($query->getPageSize()); + $pagerfanta->setCurrentPage($query->getPage()); + + return $pagerfanta; + } + + private function getQueryBuilderForQuery(TagQuery $query): QueryBuilder { $qb = $this->createQueryBuilder('tag'); $qb - ->select('tag.id, tag.name, tag.color, count(timesheets.id) as amount') - ->leftJoin('tag.timesheets', 'timesheets') - ->addGroupBy('tag.id') - ->addGroupBy('tag.name') - ->addGroupBy('tag.color') + ->select('tag.id, tag.name, tag.color, SIZE(tag.timesheets) as amount') ; $orderBy = $query->getOrderBy(); @@ -148,11 +165,7 @@ class TagRepository extends EntityRepository } } - $paginator = new Pagerfanta(new DoctrineORMAdapter($qb->getQuery(), false)); - $paginator->setMaxPerPage($query->getPageSize()); - $paginator->setCurrentPage($query->getPage()); - - return $paginator; + return $qb; } public function getQueryBuilderForFormType(TagFormTypeQuery $query): QueryBuilder diff --git a/src/Repository/TimesheetRepository.php b/src/Repository/TimesheetRepository.php index 2052f6f2..d73716c7 100644 --- a/src/Repository/TimesheetRepository.php +++ b/src/Repository/TimesheetRepository.php @@ -13,6 +13,7 @@ use App\Entity\ActivityRate; use App\Entity\CustomerRate; use App\Entity\ProjectRate; use App\Entity\RateInterface; +use App\Entity\Team; use App\Entity\Timesheet; use App\Entity\User; use App\Model\Statistic\Day; @@ -253,13 +254,13 @@ class TimesheetRepository extends EntityRepository if (!empty($begin)) { $qb - ->andWhere($qb->expr()->gte($this->getDatetimeFieldSql('t.begin'), ':from')) + ->andWhere($qb->expr()->gte('t.begin', ':from')) ->setParameter('from', $begin); } if (!empty($end)) { $qb - ->andWhere($qb->expr()->lte($this->getDatetimeFieldSql('t.end'), ':to')) + ->andWhere($qb->expr()->lte('t.end', ':to')) ->setParameter('to', $end); } @@ -320,14 +321,14 @@ class TimesheetRepository extends EntityRepository ; if (!empty($begin)) { - $qb->andWhere($qb->expr()->gte($this->getDatetimeFieldSql('t.begin'), ':from')) + $qb->andWhere($qb->expr()->gte('t.begin', ':from')) ->setParameter('from', $begin); } else { $qb->andWhere($qb->expr()->isNotNull('t.begin')); } if (!empty($end)) { - $qb->andWhere($qb->expr()->lte($this->getDatetimeFieldSql('t.end'), ':to')) + $qb->andWhere($qb->expr()->lte('t.end', ':to')) ->setParameter('to', $end); } else { $qb->andWhere($qb->expr()->isNotNull('t.end')); @@ -595,46 +596,59 @@ class TimesheetRepository extends EntityRepository return $counter; } - private function addPermissionCriteria(QueryBuilder $qb, ?User $user = null, array $teams = []) + /** + * This method causes me some headaches ... + * + * Activity permissions are currently not checked (which would be easy to add) + * + * Especially the following question is still un-answered! + * + * Should a teamlead: + * 1 . see all records of his team-members, even if they recorded times for projects invisible to him + * 2. only see records for projects which can be accessed by hom (current situation) + */ + private function addPermissionCriteria(QueryBuilder $qb, ?User $user = null, array $teams = []): bool { // make sure that all queries without a user see all projects if (null === $user && empty($teams)) { - return; + return false; } // make sure that admins see all timesheet records if (null !== $user && $user->canSeeAllData()) { - return; + return false; } if (null !== $user) { $teams = array_merge($teams, $user->getTeams()->toArray()); } - $qb - ->leftJoin('p.teams', 'teams') - ->leftJoin('c.teams', 'c_teams'); - if (empty($teams)) { - $qb->andWhere($qb->expr()->isNull('c_teams')); - $qb->andWhere($qb->expr()->isNull('teams')); + $qb->andWhere('SIZE(c.teams) = 0'); + $qb->andWhere('SIZE(p.teams) = 0'); - return; + return true; } $orProject = $qb->expr()->orX( - $qb->expr()->isNull('teams'), + 'SIZE(p.teams) = 0', $qb->expr()->isMemberOf(':teams', 'p.teams') ); $qb->andWhere($orProject); $orCustomer = $qb->expr()->orX( - $qb->expr()->isNull('c_teams'), + 'SIZE(c.teams) = 0', $qb->expr()->isMemberOf(':teams', 'c.teams') ); $qb->andWhere($orCustomer); - $qb->setParameter('teams', $teams); + $ids = array_values(array_unique(array_map(function (Team $team) { + return $team->getId(); + }, $teams))); + + $qb->setParameter('teams', $ids); + + return true; } public function getPagerfantaForQuery(TimesheetQuery $query): Pagerfanta @@ -652,7 +666,8 @@ class TimesheetRepository extends EntityRepository $qb ->resetDQLPart('select') ->resetDQLPart('orderBy') - ->select($qb->expr()->countDistinct('t.id')) + // faster then using "distinct id", as the user field is a separate (and smaller) index + ->select($qb->expr()->count('t.user')) ; $counter = (int) $qb->getQuery()->getSingleScalarResult(); @@ -695,23 +710,27 @@ class TimesheetRepository extends EntityRepository { $qb = $this->getEntityManager()->createQueryBuilder(); + $requiresProject = false; + $requiresCustomer = false; + $requiresActivity = false; + $qb ->select('t') ->from(Timesheet::class, 't') - ->leftJoin('t.project', 'p') - ->leftJoin('p.customer', 'c') ; $orderBy = $query->getOrderBy(); switch ($orderBy) { case 'project': $orderBy = 'p.name'; + $requiresProject = true; break; case 'customer': + $requiresCustomer = true; $orderBy = 'c.name'; break; case 'activity': - $qb->leftJoin('t.activity', 'a'); + $requiresActivity = true; $orderBy = 'a.name'; break; default: @@ -728,18 +747,14 @@ class TimesheetRepository extends EntityRepository $user = array_merge($user, $query->getUsers()); - if (empty($user) && null !== $query->getCurrentUser()) { - $currentUser = $query->getCurrentUser(); + if (empty($user) && null !== ($currentUser = $query->getCurrentUser()) && !$currentUser->canSeeAllData()) { + // make sure that the user himself is in the list of users, if he is part of a team + // if teams are used and the user is not a teamlead, the list of users would be empty and then leading to NOT limit the select by user IDs + $user[] = $currentUser; - if (!$currentUser->canSeeAllData()) { - // make sure that the user himself is in the list of users, if he is part of a team - // if teams are used and the user is not a teamlead, the list of users would be empty and then leading to NOT limit the select by user IDs - $user[] = $currentUser; - - foreach ($currentUser->getTeams() as $team) { - if ($currentUser->isTeamleadOf($team)) { - $query->addTeam($team); - } + foreach ($currentUser->getTeams() as $team) { + if ($currentUser->isTeamleadOf($team)) { + $query->addTeam($team); } } } @@ -766,7 +781,7 @@ class TimesheetRepository extends EntityRepository } if (null !== $query->getBegin()) { - $qb->andWhere($qb->expr()->gte($this->getDatetimeFieldSql('t.begin'), ':begin')) + $qb->andWhere($qb->expr()->gte('t.begin', ':begin')) ->setParameter('begin', $query->getBegin()); } @@ -777,7 +792,7 @@ class TimesheetRepository extends EntityRepository } if (null !== $query->getEnd()) { - $qb->andWhere($qb->expr()->lte($this->getDatetimeFieldSql('t.begin'), ':end')) + $qb->andWhere($qb->expr()->lte('t.begin', ':end')) ->setParameter('end', $query->getEnd()); } @@ -794,7 +809,7 @@ class TimesheetRepository extends EntityRepository } if (null !== $query->getModifiedAfter()) { - $qb->andWhere($qb->expr()->gte($this->getDatetimeFieldSql('t.modifiedAt'), ':modified_at')) + $qb->andWhere($qb->expr()->gte('t.modifiedAt', ':modified_at')) ->setParameter('modified_at', $query->getModifiedAfter()); } @@ -807,6 +822,7 @@ class TimesheetRepository extends EntityRepository $qb->andWhere($qb->expr()->in('t.project', ':project')) ->setParameter('project', $query->getProjects()); } elseif ($query->hasCustomers()) { + $requiresCustomer = true; $qb->andWhere($qb->expr()->in('p.customer', ':customer')) ->setParameter('customer', $query->getCustomers()); } @@ -817,7 +833,7 @@ class TimesheetRepository extends EntityRepository ->setParameter('tags', $query->getTags()); } - $this->addPermissionCriteria($qb, $query->getCurrentUser(), $query->getTeams()); + $requiresTeams = $this->addPermissionCriteria($qb, $query->getCurrentUser(), $query->getTeams()); if ($query->hasSearchTerm()) { $searchAnd = $qb->expr()->andX(); @@ -847,6 +863,18 @@ class TimesheetRepository extends EntityRepository } } + if ($requiresCustomer || $requiresProject || $requiresTeams) { + $qb->leftJoin('t.project', 'p'); + } + + if ($requiresCustomer || $requiresTeams) { + $qb->leftJoin('p.customer', 'c'); + } + + if ($requiresActivity) { + $qb->leftJoin('t.activity', 'a'); + } + return $qb; } @@ -883,7 +911,7 @@ class TimesheetRepository extends EntityRepository } if (null !== $startFrom) { - $qb->andWhere($qb->expr()->gte($this->getDatetimeFieldSql('t.begin'), ':begin')) + $qb->andWhere($qb->expr()->gte('t.begin', ':begin')) ->setParameter('begin', $startFrom); } @@ -926,16 +954,6 @@ class TimesheetRepository extends EntityRepository $em->commit(); } - private function getDatetimeFieldSql(string $field): string - { - // this would change the selected data for queries that join across multiple timezones - // but due to tax laws, this is disabled - exports/invoices should *always* include the data from - // the own timezone, not from the original users timezone - // return sprintf('CONVERT_TZ(%s, \'UTC\', t.timezone)', $field); - - return $field; - } - /** * @param Timesheet $timesheet * @return RateInterface[] diff --git a/src/Repository/UserRepository.php b/src/Repository/UserRepository.php index 08cebe07..129945c2 100644 --- a/src/Repository/UserRepository.php +++ b/src/Repository/UserRepository.php @@ -11,6 +11,7 @@ namespace App\Repository; use App\Entity\Role; use App\Entity\User; +use App\Repository\Loader\UserIdLoader; use App\Repository\Loader\UserLoader; use App\Repository\Paginator\LoaderPaginator; use App\Repository\Paginator\PaginatorInterface; @@ -56,16 +57,15 @@ class UserRepository extends EntityRepository implements UserLoaderInterface */ public function getUserById($id): ?User { - return $this->createQueryBuilder('u') - ->select('u', 'p', 't', 'tu', 'tl') - ->leftJoin('u.preferences', 'p') - ->leftJoin('u.teams', 't') - ->leftJoin('t.users', 'tu') - ->leftJoin('t.teamlead', 'tl') - ->where('u.id = :id') - ->setParameter('id', $id) - ->getQuery() - ->getOneOrNullResult(); + /** @var User|null $user */ + $user = $this->findOneBy(['id' => $id]); + + if ($user !== null) { + $loader = new UserIdLoader($this->getEntityManager()); + $loader->loadResults([$user->getId()]); + } + + return $user; } /** @@ -127,17 +127,21 @@ class UserRepository extends EntityRepository implements UserLoaderInterface */ public function loadUserByUsername($username) { - return $this->createQueryBuilder('u') - ->select('u', 'p', 't', 'tu', 'tl') - ->leftJoin('u.preferences', 'p') - ->leftJoin('u.teams', 't') - ->leftJoin('t.users', 'tu') - ->leftJoin('t.teamlead', 'tl') + /** @var User|null $user */ + $user = $this->createQueryBuilder('u') + ->select('u') ->where('u.username = :username') ->orWhere('u.email = :username') ->setParameter('username', $username) ->getQuery() ->getOneOrNullResult(); + + if ($user !== null) { + $loader = new UserIdLoader($this->getEntityManager()); + $loader->loadResults([$user->getId()]); + } + + return $user; } public function getQueryBuilderForFormType(UserFormTypeQuery $query): QueryBuilder diff --git a/src/Security/CurrentUser.php b/src/Security/CurrentUser.php index 40d1b07e..125fa54b 100644 --- a/src/Security/CurrentUser.php +++ b/src/Security/CurrentUser.php @@ -10,48 +10,37 @@ namespace App\Security; use App\Entity\User; -use App\Repository\UserRepository; use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface; +/** + * @deprecated will be removed with 2.0 + */ final class CurrentUser { /** * @var TokenStorageInterface */ private $storage; - /** - * @var UserRepository - */ - private $repository; /** * @var User|null */ private $user; - /** - * @param TokenStorageInterface $storage - * @param UserRepository $repository - */ - public function __construct(TokenStorageInterface $storage, UserRepository $repository) + public function __construct(TokenStorageInterface $storage) { $this->storage = $storage; - $this->repository = $repository; } - /** - * @return User|null - */ - public function getUser() + public function getUser(): ?User { - if (null === $this->storage->getToken()) { - return null; - } - - // some inline caching to prevent multiple DB lookups if (null !== $this->user) { return $this->user; } + if (null === $this->storage->getToken()) { + return null; + } + /** @var User $user */ $user = $this->storage->getToken()->getUser(); @@ -59,7 +48,7 @@ final class CurrentUser return null; } - $this->user = $this->repository->getUserById($user->getId()); + $this->user = $user; return $this->user; } diff --git a/src/Timesheet/UserDateTimeFactory.php b/src/Timesheet/UserDateTimeFactory.php index e4843be6..288109f0 100644 --- a/src/Timesheet/UserDateTimeFactory.php +++ b/src/Timesheet/UserDateTimeFactory.php @@ -14,7 +14,7 @@ use App\Security\CurrentUser; use DateTimeZone; /** - * @internal use DateTimeFactory instead: this one relies on the global context and will be deprecated in the future + * @deprecated will be removed with 2.0 */ class UserDateTimeFactory extends DateTimeFactory { diff --git a/tests/Mocks/Security/CurrentUserFactory.php b/tests/Mocks/Security/CurrentUserFactory.php index f4917974..7374b2ea 100644 --- a/tests/Mocks/Security/CurrentUserFactory.php +++ b/tests/Mocks/Security/CurrentUserFactory.php @@ -11,10 +11,8 @@ namespace App\Tests\Mocks\Security; use App\Entity\User; use App\Entity\UserPreference; -use App\Repository\UserRepository; use App\Security\CurrentUser; use App\Tests\Mocks\AbstractMockFactory; -use PHPUnit\Framework\TestCase; use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorage; use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken; @@ -34,11 +32,6 @@ class CurrentUserFactory extends AbstractMockFactory $user->addPreference($pref); } - $mock = $this->getMockBuilder(UserRepository::class)->onlyMethods(['getUserById'])->disableOriginalConstructor()->getMock(); - $mock->expects(TestCase::atMost(1))->method('getUserById')->willReturn($user); - /** @var UserRepository $repository */ - $repository = $mock; - $mock = $this->getMockBuilder(UsernamePasswordToken::class)->onlyMethods(['getUser'])->disableOriginalConstructor()->getMock(); $mock->method('getUser')->willReturn($user); /** @var UsernamePasswordToken $token */ @@ -47,6 +40,6 @@ class CurrentUserFactory extends AbstractMockFactory $tokenStorage = new TokenStorage(); $tokenStorage->setToken($token); - return new CurrentUser($tokenStorage, $repository); + return new CurrentUser($tokenStorage); } } diff --git a/tests/Repository/Loader/ActivityLoaderTest.php b/tests/Repository/Loader/ActivityLoaderTest.php index 1abdc033..5ce593ae 100644 --- a/tests/Repository/Loader/ActivityLoaderTest.php +++ b/tests/Repository/Loader/ActivityLoaderTest.php @@ -20,8 +20,7 @@ class ActivityLoaderTest extends AbstractLoaderTest { public function testLoadResults() { - // mock needs improvements, because it should be 5 - $em = $this->getEntityManagerMock(2); + $em = $this->getEntityManagerMock(3); $sut = new ActivityLoader($em);