From ad94310981d160a27c7d027c73f09c78d81f75c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julien=20Fastr=C3=A9?= Date: Thu, 5 Sep 2024 20:46:29 +0200 Subject: [PATCH] Refactor workflow hold functionality to avoid relying on database to perform some checks on "user holds entityworkflow" Simplify the logic in handling workflow on hold status by moving related checks and operations to `EntityWorkflowStep` and `EntityWorkflow` entities. This includes implementing new methods to check if a step or workflow is held by a specific user and refactoring the controller actions to use these methods. --- .../Controller/WorkflowController.php | 8 ------ .../Controller/WorkflowOnHoldController.php | 27 +++++++++++++------ .../Entity/Workflow/EntityWorkflow.php | 7 +++-- .../Entity/Workflow/EntityWorkflowStep.php | 11 ++++++++ .../EntityWorkflowStepHoldRepository.php | 18 ------------- .../Resources/views/Workflow/index.html.twig | 4 +-- 6 files changed, 37 insertions(+), 38 deletions(-) diff --git a/src/Bundle/ChillMainBundle/Controller/WorkflowController.php b/src/Bundle/ChillMainBundle/Controller/WorkflowController.php index 08776fcba..ab3d67307 100644 --- a/src/Bundle/ChillMainBundle/Controller/WorkflowController.php +++ b/src/Bundle/ChillMainBundle/Controller/WorkflowController.php @@ -19,7 +19,6 @@ use Chill\MainBundle\Form\WorkflowStepType; use Chill\MainBundle\Pagination\PaginatorFactory; use Chill\MainBundle\Repository\Workflow\EntityWorkflowRepository; use Chill\MainBundle\Repository\Workflow\EntityWorkflowStepSignatureRepository; -use Chill\MainBundle\Repository\Workflow\EntityWorkflowStepHoldRepository; use Chill\MainBundle\Security\Authorization\EntityWorkflowVoter; use Chill\MainBundle\Security\ChillSecurity; use Chill\MainBundle\Workflow\EntityWorkflowManager; @@ -54,7 +53,6 @@ class WorkflowController extends AbstractController private readonly ChillSecurity $security, private readonly \Doctrine\Persistence\ManagerRegistry $managerRegistry, private readonly ClockInterface $clock, - private readonly EntityWorkflowStepHoldRepository $entityWorkflowStepHoldRepository, private readonly EntityWorkflowStepSignatureRepository $entityWorkflowStepSignatureRepository, ) {} @@ -298,7 +296,6 @@ class WorkflowController extends AbstractController $workflow = $this->registry->get($entityWorkflow, $entityWorkflow->getWorkflowName()); $errors = []; $signatures = $entityWorkflow->getCurrentStep()->getSignatures(); - $onHoldStep = $this->entityWorkflowStepHoldRepository->findByWorkflow($entityWorkflow); if (\count($workflow->getEnabledTransitions($entityWorkflow)) > 0) { // possible transition @@ -343,10 +340,6 @@ class WorkflowController extends AbstractController $this->entityManager->flush(); - if (null !== $onHoldStep) { - $this->entityManager->remove($onHoldStep); - } - return $this->redirectToRoute('chill_main_workflow_show', ['id' => $entityWorkflow->getId()]); } @@ -365,7 +358,6 @@ class WorkflowController extends AbstractController 'entity_workflow' => $entityWorkflow, 'transition_form_errors' => $errors, 'signatures' => $signatures, - 'onHoldStep' => $onHoldStep, ] ); } diff --git a/src/Bundle/ChillMainBundle/Controller/WorkflowOnHoldController.php b/src/Bundle/ChillMainBundle/Controller/WorkflowOnHoldController.php index 75c9fd380..6e4af7e22 100644 --- a/src/Bundle/ChillMainBundle/Controller/WorkflowOnHoldController.php +++ b/src/Bundle/ChillMainBundle/Controller/WorkflowOnHoldController.php @@ -13,6 +13,7 @@ namespace Chill\MainBundle\Controller; use Chill\MainBundle\Entity\User; use Chill\MainBundle\Entity\Workflow\EntityWorkflow; +use Chill\MainBundle\Entity\Workflow\EntityWorkflowStep; use Chill\MainBundle\Entity\Workflow\EntityWorkflowStepHold; use Chill\MainBundle\Repository\Workflow\EntityWorkflowRepository; use Chill\MainBundle\Repository\Workflow\EntityWorkflowStepHoldRepository; @@ -21,6 +22,7 @@ use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException; +use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; use Symfony\Component\Routing\Annotation\Route; use Symfony\Component\Security\Core\Security; use Symfony\Component\Workflow\Registry; @@ -62,20 +64,29 @@ class WorkflowOnHoldController extends AbstractController return $this->redirectToRoute('chill_main_workflow_show', ['id' => $entityWorkflow->getId()]); } - #[Route(path: '/{_locale}/main/workflow/{holdId}/remove_hold', name: 'chill_main_workflow_remove_hold')] - public function removeOnHold(int $holdId, Request $request): Response + #[Route(path: '/{_locale}/main/workflow/{id}/remove_hold', name: 'chill_main_workflow_remove_hold')] + public function removeOnHold(EntityWorkflowStep $entityWorkflowStep, Request $request): Response { - $hold = $this->entityWorkflowStepHoldRepository->findById($holdId); - $entityWorkflow = $hold->getStep()->getEntityWorkflow(); - $currentUser = $this->security->getUser(); + $user = $this->security->getUser(); - if ($hold->getByUser() !== $currentUser) { - throw $this->createAccessDeniedException('You are not allowed to remove the hold status.'); + if (!$user instanceof User) { + throw new AccessDeniedHttpException('only user can remove workflow on hold'); + } + + if (!$entityWorkflowStep->isOnHoldByUser($user)) { + throw new AccessDeniedHttpException('You are not allowed to remove workflow on hold'); + } + + $hold = $this->entityWorkflowStepHoldRepository->findOneByStepAndUser($entityWorkflowStep, $user); + + if (null === $hold) { + // this should not happens... + throw new NotFoundHttpException(); } $this->entityManager->remove($hold); $this->entityManager->flush(); - return $this->redirectToRoute('chill_main_workflow_show', ['id' => $entityWorkflow->getId()]); + return $this->redirectToRoute('chill_main_workflow_show', ['id' => $entityWorkflowStep->getEntityWorkflow()->getId()]); } } diff --git a/src/Bundle/ChillMainBundle/Entity/Workflow/EntityWorkflow.php b/src/Bundle/ChillMainBundle/Entity/Workflow/EntityWorkflow.php index 14b39a065..1f32a1985 100644 --- a/src/Bundle/ChillMainBundle/Entity/Workflow/EntityWorkflow.php +++ b/src/Bundle/ChillMainBundle/Entity/Workflow/EntityWorkflow.php @@ -339,8 +339,6 @@ class EntityWorkflow implements TrackCreationInterface, TrackUpdateInterface public function isFreeze(): bool { - $steps = $this->getStepsChained(); - foreach ($this->getStepsChained() as $step) { if ($step->isFreezeAfter()) { return true; @@ -350,6 +348,11 @@ class EntityWorkflow implements TrackCreationInterface, TrackUpdateInterface return false; } + public function isOnHoldByUser(User $user): bool + { + return $this->getCurrentStep()->isOnHoldByUser($user); + } + public function isUserSubscribedToFinal(User $user): bool { return $this->subscriberToFinal->contains($user); diff --git a/src/Bundle/ChillMainBundle/Entity/Workflow/EntityWorkflowStep.php b/src/Bundle/ChillMainBundle/Entity/Workflow/EntityWorkflowStep.php index 1cdb157b1..90aa632f8 100644 --- a/src/Bundle/ChillMainBundle/Entity/Workflow/EntityWorkflowStep.php +++ b/src/Bundle/ChillMainBundle/Entity/Workflow/EntityWorkflowStep.php @@ -286,6 +286,17 @@ class EntityWorkflowStep return $this->freezeAfter; } + public function isOnHoldByUser(User $user): bool + { + foreach ($this->getHoldsOnStep() as $onHold) { + if ($onHold->getByUser() === $user) { + return true; + } + } + + return false; + } + public function isWaitingForTransition(): bool { if (null !== $this->transitionAfter) { diff --git a/src/Bundle/ChillMainBundle/Repository/Workflow/EntityWorkflowStepHoldRepository.php b/src/Bundle/ChillMainBundle/Repository/Workflow/EntityWorkflowStepHoldRepository.php index 7ef877181..a925246e4 100644 --- a/src/Bundle/ChillMainBundle/Repository/Workflow/EntityWorkflowStepHoldRepository.php +++ b/src/Bundle/ChillMainBundle/Repository/Workflow/EntityWorkflowStepHoldRepository.php @@ -12,7 +12,6 @@ declare(strict_types=1); namespace Chill\MainBundle\Repository\Workflow; use Chill\MainBundle\Entity\User; -use Chill\MainBundle\Entity\Workflow\EntityWorkflow; use Chill\MainBundle\Entity\Workflow\EntityWorkflowStep; use Chill\MainBundle\Entity\Workflow\EntityWorkflowStepHold; use Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepository; @@ -58,23 +57,6 @@ class EntityWorkflowStepHoldRepository extends ServiceEntityRepository return $this->findBy(['step' => $step]); } - /** - * @throws NonUniqueResultException - */ - public function findByWorkflow(EntityWorkflow $workflow): ?EntityWorkflowStepHold - { - $qb = $this->getEntityManager()->createQueryBuilder(); - - $qb->select('h') - ->from(EntityWorkflowStepHold::class, 'h') - ->join('h.step', 's') - ->join('s.entityWorkflow', 'w') - ->where('w = :workflow') - ->setParameter('workflow', $workflow); - - return $qb->getQuery()->getOneOrNullResult(); - } - /** * Find a single EntityWorkflowStepHold by step and user. * diff --git a/src/Bundle/ChillMainBundle/Resources/views/Workflow/index.html.twig b/src/Bundle/ChillMainBundle/Resources/views/Workflow/index.html.twig index 20f617c0e..c5daff201 100644 --- a/src/Bundle/ChillMainBundle/Resources/views/Workflow/index.html.twig +++ b/src/Bundle/ChillMainBundle/Resources/views/Workflow/index.html.twig @@ -68,9 +68,9 @@
{% include '@ChillMain/Workflow/_history.html.twig' %}